Thread: Re: [PATCHES] Configuration patch

Re: [PATCHES] Configuration patch

From
Bruce Momjian
Date:
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
 


Re: [PATCHES] Configuration patch

From
pgsql@mohawksoft.com
Date:
>
> 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
>



Re: [PATCHES] Configuration patch

From
Bruce Momjian
Date:
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
 


Re: [PATCHES] Configuration patch

From
Tom Lane
Date:
>> 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


Re: [PATCHES] Configuration patch

From
Bruce Momjian
Date:
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
 


Re: [PATCHES] Configuration patch

From
Bruce Momjian
Date:
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