Thread: Re: [PATCHES] Configuration patch
Where are we on this? --------------------------------------------------------------------------- Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > One interesting idea would be for "SET include" to work like this: > > SET include '/var/run/xx' > > Notice there is no equals here. This would allow users to create files > > with various settings and enable them all with one SET command. > > However, this does open a security issue. > > More than one, in fact. In the first place, as the code presently > works, anything coming in from the file would be treated on an equal > footing with values sourced from postgresql.conf, thereby allowing > unprivileged users to set things they shouldn't. This is potentially > fixable, but the other issue isn't: such a facility would allow anyone > to ask the backend to read any file the Postgres user account can > access. Not very successfully, perhaps, but even the error messages > might give useful info about the file's contents to an attacker. This > is the same reason that "COPY FROM file" is a privileged operation. > > I think it's important that include be restricted to appear only in > config files, and not be in any way shape or form a SETtable thing. > > > In summary, I think we need to treat include specially in > > postgresql.conf (no equals) and remove it as an actual GUC parameter and > > just have it do includes immediately. (This will probably require > > special-casing it in the guc-file grammar.) > > Yes. In fact, it'll be a less-than-trivial change in guc-file, at least > if you want the thing to act intuitively (that is, "include" acts like > the target file is actually included right here). This will mean > splitting ProcessConfigFile into a recursive read step followed by a > nonrecursive apply step. Also, I think that invoking the flex lexer > recursively will take a little bit of work. > > I would suggest splitting the patch into two separate patches, one that > handles "include" and one that handles the other changes. The other > stuff is reasonably close to being ready to apply (modulo docs and > fixing the standalone-backend case), but "include" I think is still a > ways off. > > regards, tom lane > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
> > Where are we on this? That's a good question. Tom doesn't like the syntax of "include" and there are a couple bugs he is concered it. I'm pretty agnostic about the syntax, but I wouldn't get overly worried about the metaphor presented either. "include='...'" doesn't bother me at all, but some people have a problem with it. Then there is the design of using a callable function for a configuration parameter, personally, I think this feature is useful for the future, Tom seems to have a problem it it. After that, the discussion sort of ends. I'm willing to adress the bugs. I don't think the syntax is a huge deal, IMHO at most it is a documentation problem. > > --------------------------------------------------------------------------- > > Tom Lane wrote: >> Bruce Momjian <pgman@candle.pha.pa.us> writes: >> > One interesting idea would be for "SET include" to work like this: >> > SET include '/var/run/xx' >> > Notice there is no equals here. This would allow users to create >> files >> > with various settings and enable them all with one SET command. >> > However, this does open a security issue. >> >> More than one, in fact. In the first place, as the code presently >> works, anything coming in from the file would be treated on an equal >> footing with values sourced from postgresql.conf, thereby allowing >> unprivileged users to set things they shouldn't. This is potentially >> fixable, but the other issue isn't: such a facility would allow anyone >> to ask the backend to read any file the Postgres user account can >> access. Not very successfully, perhaps, but even the error messages >> might give useful info about the file's contents to an attacker. This >> is the same reason that "COPY FROM file" is a privileged operation. >> >> I think it's important that include be restricted to appear only in >> config files, and not be in any way shape or form a SETtable thing. >> >> > In summary, I think we need to treat include specially in >> > postgresql.conf (no equals) and remove it as an actual GUC parameter >> and >> > just have it do includes immediately. (This will probably require >> > special-casing it in the guc-file grammar.) >> >> Yes. In fact, it'll be a less-than-trivial change in guc-file, at least >> if you want the thing to act intuitively (that is, "include" acts like >> the target file is actually included right here). This will mean >> splitting ProcessConfigFile into a recursive read step followed by a >> nonrecursive apply step. Also, I think that invoking the flex lexer >> recursively will take a little bit of work. >> >> I would suggest splitting the patch into two separate patches, one that >> handles "include" and one that handles the other changes. The other >> stuff is reasonably close to being ready to apply (modulo docs and >> fixing the standalone-backend case), but "include" I think is still a >> ways off. >> >> regards, tom lane >> > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup. | Newtown Square, Pennsylvania > 19073 > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match >
pgsql@mohawksoft.com wrote: > > > > Where are we on this? > > That's a good question. > > Tom doesn't like the syntax of "include" and there are a couple bugs he is > concered it. > > I'm pretty agnostic about the syntax, but I wouldn't get overly worried > about the metaphor presented either. > > "include='...'" doesn't bother me at all, but some people have a problem > with it. > > Then there is the design of using a callable function for a configuration > parameter, personally, I think this feature is useful for the future, Tom > seems to have a problem it it. > > After that, the discussion sort of ends. > > > I'm willing to adress the bugs. > I don't think the syntax is a huge deal, IMHO at most it is a > documentation problem. Well, it seems pretty clear were we need to go on this. First, we could just add the documentation to the non-include part of the patch based on the version I posted and apply that to be sure it gets into 7.5. Then, for "include", it needs to be an operation and not a variable, probably something in guc-file.l. It should not use an equals and not be something you can say SHOW with. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
>> Tom doesn't like the syntax of "include"
I said more than once that I didn't care about the syntax; it's the
implementation I was objecting to.
However, given that we are going to push it into guc-file.l, it'll
be easier all around if we choose a syntax that doesn't look exactly
like a variable assignment.include 'file'
with no equal sign would probably work as well as anything else.
regards, tom lane
Mark (see I remembered your name), where are we on this patch? It needs docs and include has to be redone. Should I remove the include part of the patch, add docs, and apply it? --------------------------------------------------------------------------- pgsql@mohawksoft.com wrote: > > > > Where are we on this? > > That's a good question. > > Tom doesn't like the syntax of "include" and there are a couple bugs he is > concered it. > > I'm pretty agnostic about the syntax, but I wouldn't get overly worried > about the metaphor presented either. > > "include='...'" doesn't bother me at all, but some people have a problem > with it. > > Then there is the design of using a callable function for a configuration > parameter, personally, I think this feature is useful for the future, Tom > seems to have a problem it it. > > After that, the discussion sort of ends. > > > I'm willing to adress the bugs. > I don't think the syntax is a huge deal, IMHO at most it is a > documentation problem. > > > > > --------------------------------------------------------------------------- > > > > Tom Lane wrote: > >> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> > One interesting idea would be for "SET include" to work like this: > >> > SET include '/var/run/xx' > >> > Notice there is no equals here. This would allow users to create > >> files > >> > with various settings and enable them all with one SET command. > >> > However, this does open a security issue. > >> > >> More than one, in fact. In the first place, as the code presently > >> works, anything coming in from the file would be treated on an equal > >> footing with values sourced from postgresql.conf, thereby allowing > >> unprivileged users to set things they shouldn't. This is potentially > >> fixable, but the other issue isn't: such a facility would allow anyone > >> to ask the backend to read any file the Postgres user account can > >> access. Not very successfully, perhaps, but even the error messages > >> might give useful info about the file's contents to an attacker. This > >> is the same reason that "COPY FROM file" is a privileged operation. > >> > >> I think it's important that include be restricted to appear only in > >> config files, and not be in any way shape or form a SETtable thing. > >> > >> > In summary, I think we need to treat include specially in > >> > postgresql.conf (no equals) and remove it as an actual GUC parameter > >> and > >> > just have it do includes immediately. (This will probably require > >> > special-casing it in the guc-file grammar.) > >> > >> Yes. In fact, it'll be a less-than-trivial change in guc-file, at least > >> if you want the thing to act intuitively (that is, "include" acts like > >> the target file is actually included right here). This will mean > >> splitting ProcessConfigFile into a recursive read step followed by a > >> nonrecursive apply step. Also, I think that invoking the flex lexer > >> recursively will take a little bit of work. > >> > >> I would suggest splitting the patch into two separate patches, one that > >> handles "include" and one that handles the other changes. The other > >> stuff is reasonably close to being ready to apply (modulo docs and > >> fixing the standalone-backend case), but "include" I think is still a > >> ways off. > >> > >> regards, tom lane > >> > > > > -- > > Bruce Momjian | http://candle.pha.pa.us > > pgman@candle.pha.pa.us | (610) 359-1001 > > + If your life is a hard drive, | 13 Roberts Road > > + Christ can be your backup. | Newtown Square, Pennsylvania > > 19073 > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 9: the planner will ignore your desire to choose an index scan if your > > joining column's datatypes do not match > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
I have gotten no reply to my request to either move the include functionality into the guc-file.l or remove it and just add docs for the config location part of the patch. I now would like someone else to take the patch and make those changes to get it applied before feature freeze. If not, I can do it but it will be after the feature freeze: ftp://candle.pha.pa.us/pub/postgresql/mypatches/guc --------------------------------------------------------------------------- pgsql@mohawksoft.com wrote: > > > > Where are we on this? > > That's a good question. > > Tom doesn't like the syntax of "include" and there are a couple bugs he is > concered it. > > I'm pretty agnostic about the syntax, but I wouldn't get overly worried > about the metaphor presented either. > > "include='...'" doesn't bother me at all, but some people have a problem > with it. > > Then there is the design of using a callable function for a configuration > parameter, personally, I think this feature is useful for the future, Tom > seems to have a problem it it. > > After that, the discussion sort of ends. > > > I'm willing to adress the bugs. > I don't think the syntax is a huge deal, IMHO at most it is a > documentation problem. > > > > > --------------------------------------------------------------------------- > > > > Tom Lane wrote: > >> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> > One interesting idea would be for "SET include" to work like this: > >> > SET include '/var/run/xx' > >> > Notice there is no equals here. This would allow users to create > >> files > >> > with various settings and enable them all with one SET command. > >> > However, this does open a security issue. > >> > >> More than one, in fact. In the first place, as the code presently > >> works, anything coming in from the file would be treated on an equal > >> footing with values sourced from postgresql.conf, thereby allowing > >> unprivileged users to set things they shouldn't. This is potentially > >> fixable, but the other issue isn't: such a facility would allow anyone > >> to ask the backend to read any file the Postgres user account can > >> access. Not very successfully, perhaps, but even the error messages > >> might give useful info about the file's contents to an attacker. This > >> is the same reason that "COPY FROM file" is a privileged operation. > >> > >> I think it's important that include be restricted to appear only in > >> config files, and not be in any way shape or form a SETtable thing. > >> > >> > In summary, I think we need to treat include specially in > >> > postgresql.conf (no equals) and remove it as an actual GUC parameter > >> and > >> > just have it do includes immediately. (This will probably require > >> > special-casing it in the guc-file grammar.) > >> > >> Yes. In fact, it'll be a less-than-trivial change in guc-file, at least > >> if you want the thing to act intuitively (that is, "include" acts like > >> the target file is actually included right here). This will mean > >> splitting ProcessConfigFile into a recursive read step followed by a > >> nonrecursive apply step. Also, I think that invoking the flex lexer > >> recursively will take a little bit of work. > >> > >> I would suggest splitting the patch into two separate patches, one that > >> handles "include" and one that handles the other changes. The other > >> stuff is reasonably close to being ready to apply (modulo docs and > >> fixing the standalone-backend case), but "include" I think is still a > >> ways off. > >> > >> regards, tom lane > >> > > > > -- > > Bruce Momjian | http://candle.pha.pa.us > > pgman@candle.pha.pa.us | (610) 359-1001 > > + If your life is a hard drive, | 13 Roberts Road > > + Christ can be your backup. | Newtown Square, Pennsylvania > > 19073 > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 9: the planner will ignore your desire to choose an index scan if your > > joining column's datatypes do not match > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073