Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/18/17 3:57 PM, Robert Haas wrote:

On Wed, Mar 15, 2017 at 3:00 AM, Tsunakawa, Takayuki
 wrote:

I'm on David's side, too.  I don't postmaster to always scan all files at 
startup.


+1.  Even just doing it during crash recovery, it can take a
regrettably long time on machines with tons of relations and a very
slow disk.  I've been sort of thinking that we should add some logging
there so that users know what's happening when that code goes into the
tank - I've seen that come up 3 or 4 times now, and I'm getting tired
of telling people to run strace to find out.


Most of this time is spent scanning for unlogged tables.  We were 
working on a patch to put unlogged tables (except for the init fork) in 
a pg_unlogged directory created for each tablespace, just like 
pgsql_temp.  This would make cleanup a lot faster and make it easier for 
backup software to skip relations that can't be restored and only take 
up space.


Unfortunately the patch was not ready for the last CF and maybe would 
not have been a good candidate anyway due to complexity.  It's still on 
our radar, though.



I think Tom's concerns about people doing insecure stuff are
excessive.  People can do insecure stuff no matter what we do, and
trying to prevent them often leads to them doing even-more-insecure
stuff.  That having been aid, I do wonder whether the idea of allowing
group read privileges specifically might be a better concept than a
umask, though, because (1) it's not obvious that there's a real use
case for anything other than group read privileges, so why not support
exactly that to avoid user confusion and (2) umask is a pretty
specific concept that may not apply on every platform.  For example,
AFS has an ACL list instead of using the traditional UNIX permission
bits, and I'm not sure Windows has the umask concept exactly either.
So wording what we're trying to do a bit more generically might be
smart.


We took Tom's advice to heart and this is the direction the patch is 
currently going in.  Even the GUC may be too much as there are number of 
tools that write into PGDATA but don't read postgresql.conf.  It looks 
like using the permissions of PGDATA may be the best way to go.


In any case, the changes required have enlarged the size and scope of 
the patch considerably and we are not confident that it will be done in 
time to commit for v10.


I have marked this submission "Returned with Feedback".

--
-David
da...@pgmasters.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Robert Haas]

On Wed, Mar 15, 2017 at 3:00 AM, Tsunakawa, Takayuki
 wrote:
> I'm on David's side, too.  I don't postmaster to always scan all files at 
> startup.

+1.  Even just doing it during crash recovery, it can take a
regrettably long time on machines with tons of relations and a very
slow disk.  I've been sort of thinking that we should add some logging
there so that users know what's happening when that code goes into the
tank - I've seen that come up 3 or 4 times now, and I'm getting tired
of telling people to run strace to find out.

I think Tom's concerns about people doing insecure stuff are
excessive.  People can do insecure stuff no matter what we do, and
trying to prevent them often leads to them doing even-more-insecure
stuff.  That having been aid, I do wonder whether the idea of allowing
group read privileges specifically might be a better concept than a
umask, though, because (1) it's not obvious that there's a real use
case for anything other than group read privileges, so why not support
exactly that to avoid user confusion and (2) umask is a pretty
specific concept that may not apply on every platform.  For example,
AFS has an ACL list instead of using the traditional UNIX permission
bits, and I'm not sure Windows has the umask concept exactly either.
So wording what we're trying to do a bit more generically might be
smart.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/15/17 3:00 AM, Tsunakawa, Takayuki wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
>>> But it might be worth thinking about whether we want to encourage
>>> people to do manual chmod's at all; that's fairly easy to get wrong,
>>> particularly given the difference in X bits that should be applied to
>>> files and directories.  Another approach that could be worth
>>> considering is a PGC_POSTMASTER GUC with just two states (group access
>>> or not) and make it the postmaster's responsibility to do the
>>> equivalent of chmod -R to make the file tree match the GUC.  I think
>>> we do a tree scan anyway for other purposes, so correcting any wrong
>>> file permissions might not be much added work in the normal case.
>>
>> The majority of scanning is done in recovery (to find and remove unlogged
>> tables) and I'm not sure we would want to add that overhead to normal 
>> startup.
> 
> I'm on David's side, too.  I don't postmaster to always scan all files at 
> startup.
> 
> On the other hand, just doing "chmod -R $PGDATA" is not enough, because chmod 
> doesn't follow the symbolic links.  Symbolic links are used for pg_tblspc/* 
> and pg_wal at least.  FYI, MySQL's manual describes the pithole like this:

Good point - I think we'll need to add that to the docs as well.

> I think we also need to describe the procedure carefully.  That said, it 
> would be best to make users aware of a configuration alternative (group 
> access) with enough documentation when they first build the database or 
> upgrade the database cluster.  Just describing the alternative only in initdb 
> reference page would result in being unaware of the better configuration, 
> like --data-checksum.

I'm working on a new approach incorporating everybody's suggestions and
enhanced documentation.  It should be ready on Monday.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/15/17 1:56 AM, Tsunakawa, Takayuki wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
>> Sure, but having the private key may allow them to get new data from the
>> server as well as the data from the backup.
> 
> You are right.  My rough intent was that the data is stolen anyway.  So, I 
> thought it might not be so bad to expect to be able to back up the SSL key 
> file in $PGDATA together with the database.  If it's bad, then the default 
> value of ssl_key_file (=$PGDATA/ssl.key) should be disallowed.

I think it really depends on the situation and the user needs to make an
evaluation of the security risks for themselves.

>> If the backup user is in the same group as the postgres user and in the
>> ssl_cert group then backups of the certs would be possible using group reads.
>> Restores will be a little tricky, though, because of the need to set
>> ownership to root.  The restore would need to be run as root or the
>> permissions fixed up after the restore completes.
> 
> Yes, but I thought, from the following message,  that you do not recommend 
> that the backup user be able to read the SSL key file.  So, I proposed to 
> describe the example configuration to achieve that -- postgres user in dba 
> and ssl_cert group, and a separate backup user in only dba group.

That would work as long as the key was not in $PGDATA.  Most database
backup software does not have a --ignore option because of the obvious
dangers.

 It seems to me that it would be best to advise in the docs that these
 files should be relocated if they won't be readable by the backup user.
 In any event, I'm not convinced that backing up server private keys
 is a good idea.
> 
>> To be clear, the default for this patch is to leave permissions exactly
>> as they are now.  It also provides alternatives that may or not be useful
>> in all cases.
> 
> So you think there are configurations that may be useful or not, don't you?  
> Adding a new parameter could possibly complicate what users have to consider. 
>  Maximal flexibility could lead to insecure misuse.  So I think it would be 
> better to describe secure and practical configuration examples in the SSL 
> section and/or the backup chapter.  The configuration factor includes whether 
> the backup user is different from the postgres user, where the SSL key file 
> is placed, the owner of the SSL key file, whether the backup user can read 
> the SSL key file.

I think we are more or less on the same page, and you'd like to see
documentation that shows examples of secure configurations when group
access is allow.  I agree and I'm working on documentation for all the
sections you recommended.

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> > But it might be worth thinking about whether we want to encourage
> > people to do manual chmod's at all; that's fairly easy to get wrong,
> > particularly given the difference in X bits that should be applied to
> > files and directories.  Another approach that could be worth
> > considering is a PGC_POSTMASTER GUC with just two states (group access
> > or not) and make it the postmaster's responsibility to do the
> > equivalent of chmod -R to make the file tree match the GUC.  I think
> > we do a tree scan anyway for other purposes, so correcting any wrong
> > file permissions might not be much added work in the normal case.
> 
> The majority of scanning is done in recovery (to find and remove unlogged
> tables) and I'm not sure we would want to add that overhead to normal startup.

I'm on David's side, too.  I don't postmaster to always scan all files at 
startup.

On the other hand, just doing "chmod -R $PGDATA" is not enough, because chmod 
doesn't follow the symbolic links.  Symbolic links are used for pg_tblspc/* and 
pg_wal at least.  FYI, MySQL's manual describes the pithole like this:

https://dev.mysql.com/doc/refman/8.0/en/changing-mysql-user.html

2. Change the database directories and files so that user_name has privileges 
to read and write files in them (you might need to do this as the Unix root 
user): 
shell> chown -R user_name /path/to/mysql/datadir


If you do not do this, the server will not be able to access databases or 
tables when it runs as user_name. 

If directories or files within the MySQL data directory are symbolic links, 
chown -R might not follow symbolic links for you. If it does not, you will also 
need to follow those links and change the directories and files they point to. 



I think we also need to describe the procedure carefully.  That said, it would 
be best to make users aware of a configuration alternative (group access) with 
enough documentation when they first build the database or upgrade the database 
cluster.  Just describing the alternative only in initdb reference page would 
result in being unaware of the better configuration, like --data-checksum.

Regards
Takayuki Tsunakawa







-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> Sure, but having the private key may allow them to get new data from the
> server as well as the data from the backup.

You are right.  My rough intent was that the data is stolen anyway.  So, I 
thought it might not be so bad to expect to be able to back up the SSL key file 
in $PGDATA together with the database.  If it's bad, then the default value of 
ssl_key_file (=$PGDATA/ssl.key) should be disallowed.


> > https://www.postgresql.org/docs/devel/static/ssl-tcp.html
> >
> > "On Unix systems, the permissions on server.key must disallow any access
> to world or group; achieve this by the command chmod 0600 server.key.
> Alternatively, the file can be owned by root and have group read access
> (that is, 0640 permissions). That setup is intended for installations where
> certificate and key files are managed by the operating system. The user
> under which the PostgreSQL server runs should then be made a member of the
> group that has access to those certificate and key files."
> >
> > In the latter case, the file owner is root and the permission is 0640.
> At first I was a bit confused and misunderstood that the PostgreSQL user
> account and the backup OS user needs to belong to the same OS group.  But
> that's not the case.  The group of the key file can be, for example,
> "ssl_cert", the PostgreSQL user account belongs to the OS group "ssl_cert"
> and "dba", and the backup OS user only belongs to "backup."  This can prevent
> the backup OS user from reading the key file.  I think it would be better
> to have some explanation with examples in the above section.
> 
> If the backup user is in the same group as the postgres user and in the
> ssl_cert group then backups of the certs would be possible using group reads.
> Restores will be a little tricky, though, because of the need to set
> ownership to root.  The restore would need to be run as root or the
> permissions fixed up after the restore completes.

Yes, but I thought, from the following message,  that you do not recommend that 
the backup user be able to read the SSL key file.  So, I proposed to describe 
the example configuration to achieve that -- postgres user in dba and ssl_cert 
group, and a separate backup user in only dba group.

> >> It seems to me that it would be best to advise in the docs that these
> >> files should be relocated if they won't be readable by the backup user.
> >> In any event, I'm not convinced that backing up server private keys
> >> is a good idea.


> To be clear, the default for this patch is to leave permissions exactly
> as they are now.  It also provides alternatives that may or not be useful
> in all cases.

So you think there are configurations that may be useful or not, don't you?  
Adding a new parameter could possibly complicate what users have to consider.  
Maximal flexibility could lead to insecure misuse.  So I think it would be 
better to describe secure and practical configuration examples in the SSL 
section and/or the backup chapter.  The configuration factor includes whether 
the backup user is different from the postgres user, where the SSL key file is 
placed, the owner of the SSL key file, whether the backup user can read the SSL 
key file.

Regards
Takayuki Tsunakawa






-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/13/17 3:03 PM, Tom Lane wrote:
> David Steele  writes:
>> On 3/13/17 2:16 PM, Tom Lane wrote:
>>> I also don't especially want to have to analyze cases like "what
>>> happens if user initdb'd with mask X but then changes the GUC and
>>> restarts the postmaster?".  Maybe the right thing is to not expose
>>> this as a GUC at all, but drive it off the permissions observed on
>>> the data directory at postmaster start.  Viz:
>>>
>>> * $PGDATA has permissions 0700: adopt umask 077
>>>
>>> * $PGDATA has permissions 0750: adopt umask 027
>>>
>>> * anything else: fail
> 
>> How about a GUC, allow_group_access, that when set will enforce
>> permissions and set the umask accordingly, and when not set will follow
>> $PGDATA as proposed above?
> 
> Seems overcomplicated ...

Yeah.  It wouldn't be a lot of fun to document, that's for sure.

>>> That way, a "chmod -R" would be the necessary and sufficient procedure
>>> for switching from one case to the other.
> 
>> I'm OK with that if you think it's the best course, but perhaps the GUC
>> would be better because it can detect accidental permission changes.
> 
> If we're only checking file permissions at postmaster start, I think it's
> illusory to suppose that we're offering very much protection against
> accidental changes.  A chmod applied while the postmaster is running
> could still break things, and we'd not notice till the next restart.

Fair enough.

> But it might be worth thinking about whether we want to encourage people
> to do manual chmod's at all; that's fairly easy to get wrong, particularly
> given the difference in X bits that should be applied to files and
> directories.  Another approach that could be worth considering is
> a PGC_POSTMASTER GUC with just two states (group access or not) and
> make it the postmaster's responsibility to do the equivalent of chmod -R
> to make the file tree match the GUC.  I think we do a tree scan anyway
> for other purposes, so correcting any wrong file permissions might not
> be much added work in the normal case.

The majority of scanning is done in recovery (to find and remove
unlogged tables) and I'm not sure we would want to add that overhead to
normal startup.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/14/17 4:23 AM, Tsunakawa, Takayuki wrote:
> From: David Steele [mailto:da...@pgmasters.net]
 3.The default location of the SSL key file is $PGDATA, so the permission
>> of the key file is likely to become 0640.  But the current postgres requires
>> it to be 0600.  See src/backend/libpq/be-secure-openssl.c.
>>>
>>> Yes, that needs to be addressed.  There was discussion on another
>>> thread that it would be useful to support the SSL key file having
>>> group read access, but since this patch is handling the other files it
>>> seems like it would make sense to do that change here also.
>>
>> Perhaps, but since these files are not setup by initdb I'm not sure if we
>> should be handling their permissions.  This seems to be a distro-specific
>> issue.
>>
>> It seems to me that it would be best to advise in the docs that these files
>> should be relocated if they won't be readable by the backup user.
>> In any event, I'm not convinced that backing up server private keys is a
>> good idea.
> 
> Maybe so, but it's convenient to be able to store the key and certificate 
> files in $PGDATA and back them up together.  If the database backup were 
> stolen through the compromised backup OS user, then the malicious person can 
> read the data anyway regardless of whether the key file is there or not.  
> That's because the key is not for encrypting data at rest.

Sure, but having the private key may allow them to get new data from the
server as well as the data from the backup.

To be clear, the default for this patch is to leave permissions exactly
as they are now.  It also provides alternatives that may or not be
useful in all cases.

> Related to this, please see:
> 
> https://www.postgresql.org/docs/devel/static/ssl-tcp.html
> 
> "On Unix systems, the permissions on server.key must disallow any access to 
> world or group; achieve this by the command chmod 0600 server.key. 
> Alternatively, the file can be owned by root and have group read access (that 
> is, 0640 permissions). That setup is intended for installations where 
> certificate and key files are managed by the operating system. The user under 
> which the PostgreSQL server runs should then be made a member of the group 
> that has access to those certificate and key files."
> 
> In the latter case, the file owner is root and the permission is 0640.  At 
> first I was a bit confused and misunderstood that the PostgreSQL user account 
> and the backup OS user needs to belong to the same OS group.  But that's not 
> the case.  The group of the key file can be, for example, "ssl_cert", the 
> PostgreSQL user account belongs to the OS group "ssl_cert" and "dba", and the 
> backup OS user only belongs to "backup."  This can prevent the backup OS user 
> from reading the key file.  I think it would be better to have some 
> explanation with examples in the above section.

If the backup user is in the same group as the postgres user and in the
ssl_cert group then backups of the certs would be possible using group
reads.  Restores will be a little tricky, though, because of the need to
set ownership to root.  The restore would need to be run as root or the
permissions fixed up after the restore completes.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tsunakawa, Takayuki]

From: David Steele [mailto:da...@pgmasters.net]
> >> 3.The default location of the SSL key file is $PGDATA, so the permission
> of the key file is likely to become 0640.  But the current postgres requires
> it to be 0600.  See src/backend/libpq/be-secure-openssl.c.
> >
> > Yes, that needs to be addressed.  There was discussion on another
> > thread that it would be useful to support the SSL key file having
> > group read access, but since this patch is handling the other files it
> > seems like it would make sense to do that change here also.
> 
> Perhaps, but since these files are not setup by initdb I'm not sure if we
> should be handling their permissions.  This seems to be a distro-specific
> issue.
> 
> It seems to me that it would be best to advise in the docs that these files
> should be relocated if they won't be readable by the backup user.
> In any event, I'm not convinced that backing up server private keys is a
> good idea.

Maybe so, but it's convenient to be able to store the key and certificate files 
in $PGDATA and back them up together.  If the database backup were stolen 
through the compromised backup OS user, then the malicious person can read the 
data anyway regardless of whether the key file is there or not.  That's because 
the key is not for encrypting data at rest.

Related to this, please see:

https://www.postgresql.org/docs/devel/static/ssl-tcp.html

"On Unix systems, the permissions on server.key must disallow any access to 
world or group; achieve this by the command chmod 0600 server.key. 
Alternatively, the file can be owned by root and have group read access (that 
is, 0640 permissions). That setup is intended for installations where 
certificate and key files are managed by the operating system. The user under 
which the PostgreSQL server runs should then be made a member of the group that 
has access to those certificate and key files."

In the latter case, the file owner is root and the permission is 0640.  At 
first I was a bit confused and misunderstood that the PostgreSQL user account 
and the backup OS user needs to belong to the same OS group.  But that's not 
the case.  The group of the key file can be, for example, "ssl_cert", the 
PostgreSQL user account belongs to the OS group "ssl_cert" and "dba", and the 
backup OS user only belongs to "backup."  This can prevent the backup OS user 
from reading the key file.  I think it would be better to have some explanation 
with examples in the above section.


Regards
Takayuki Tsunakawa





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tom Lane]

David Steele  writes:
> On 3/13/17 2:16 PM, Tom Lane wrote:
>> I also don't especially want to have to analyze cases like "what
>> happens if user initdb'd with mask X but then changes the GUC and
>> restarts the postmaster?".  Maybe the right thing is to not expose
>> this as a GUC at all, but drive it off the permissions observed on
>> the data directory at postmaster start.  Viz:
>> 
>> * $PGDATA has permissions 0700: adopt umask 077
>> 
>> * $PGDATA has permissions 0750: adopt umask 027
>> 
>> * anything else: fail

> How about a GUC, allow_group_access, that when set will enforce
> permissions and set the umask accordingly, and when not set will follow
> $PGDATA as proposed above?

Seems overcomplicated ...

> Not much we can do for Windows, though.  I think it would have to WARN
> if the GUC is set and then continue as usual.

Yeah, the Windows port has been weak in this area all along.  I don't
think it's incumbent on you to make it better.

>> That way, a "chmod -R" would be the necessary and sufficient procedure
>> for switching from one case to the other.

> I'm OK with that if you think it's the best course, but perhaps the GUC
> would be better because it can detect accidental permission changes.

If we're only checking file permissions at postmaster start, I think it's
illusory to suppose that we're offering very much protection against
accidental changes.  A chmod applied while the postmaster is running
could still break things, and we'd not notice till the next restart.

But it might be worth thinking about whether we want to encourage people
to do manual chmod's at all; that's fairly easy to get wrong, particularly
given the difference in X bits that should be applied to files and
directories.  Another approach that could be worth considering is
a PGC_POSTMASTER GUC with just two states (group access or not) and
make it the postmaster's responsibility to do the equivalent of chmod -R
to make the file tree match the GUC.  I think we do a tree scan anyway
for other purposes, so correcting any wrong file permissions might not
be much added work in the normal case.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/13/17 2:16 PM, Tom Lane wrote:
> David Steele  writes:
>> On 3/13/17 1:03 PM, Tom Lane wrote:
>>> TBH, the fact that we're relying on 0600 mode for considerations such
>>> as these makes me tremendously afraid of this whole patch.  I think that
>>> the claimed advantages are not anywhere near worth the risk that somebody
>>> is going to destroy their database because we weakened some aspect of the
>>> protection against starting multiple postmasters in a database directory.
> 
>> I don't see a risk if the user uses umask 0027 which is the example
>> given in the docs.  I'm happy to change this example to a strong
>> recommendation.

<...>

> Anyway, given that we do that analysis, I'd rather not expose this
> as a "here, set the umask you want" variable.  I think a bool saying
> "allow group access" (translating to exactly two supported umasks,
> 027 and 077) would be simpler from the user's standpoint and much
> easier to reason about.  I don't see the value in having to think
> about what happens if the user supplies a mask like 037 or 067.

We debated a flag vs a umask and came down on the side of flexibility.
I'm perfectly happy with using a flag instead.

> I also don't especially want to have to analyze cases like "what
> happens if user initdb'd with mask X but then changes the GUC and
> restarts the postmaster?".  Maybe the right thing is to not expose
> this as a GUC at all, but drive it off the permissions observed on
> the data directory at postmaster start.  Viz:
> 
> * $PGDATA has permissions 0700: adopt umask 077
> 
> * $PGDATA has permissions 0750: adopt umask 027
> 
> * anything else: fail

How about a GUC, allow_group_access, that when set will enforce
permissions and set the umask accordingly, and when not set will follow
$PGDATA as proposed above?

Not much we can do for Windows, though.  I think it would have to WARN
if the GUC is set and then continue as usual.

> That way, a "chmod -R" would be the necessary and sufficient procedure
> for switching from one case to the other.

I'm OK with that if you think it's the best course, but perhaps the GUC
would be better because it can detect accidental permission changes.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tom Lane]

David Steele  writes:
> On 3/13/17 1:03 PM, Tom Lane wrote:
>> TBH, the fact that we're relying on 0600 mode for considerations such
>> as these makes me tremendously afraid of this whole patch.  I think that
>> the claimed advantages are not anywhere near worth the risk that somebody
>> is going to destroy their database because we weakened some aspect of the
>> protection against starting multiple postmasters in a database directory.

> I don't see a risk if the user uses umask 0027 which is the example
> given in the docs.  I'm happy to change this example to a strong
> recommendation.

I do not want a "strong recommendation".  I want "we don't let you
break this".  We don't let you run the postmaster as root either,
even though there have been repeated requests to remove that safety
check.

It might be all right if we forcibly or'd 027 into whatever umask
the user tries to provide; not sure.  The existing safety analysis,
such as the cited comment, has all been based on the assumption of
file mode 600 not mode 640.  I'm not certain that we always attempt
to open-for-writing files that we expect to be exclusively accessible
by the postmaster.

Anyway, given that we do that analysis, I'd rather not expose this
as a "here, set the umask you want" variable.  I think a bool saying
"allow group access" (translating to exactly two supported umasks,
027 and 077) would be simpler from the user's standpoint and much
easier to reason about.  I don't see the value in having to think
about what happens if the user supplies a mask like 037 or 067.

I also don't especially want to have to analyze cases like "what
happens if user initdb'd with mask X but then changes the GUC and
restarts the postmaster?".  Maybe the right thing is to not expose
this as a GUC at all, but drive it off the permissions observed on
the data directory at postmaster start.  Viz:

* $PGDATA has permissions 0700: adopt umask 077

* $PGDATA has permissions 0750: adopt umask 027

* anything else: fail

That way, a "chmod -R" would be the necessary and sufficient procedure
for switching from one case to the other.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

Hi Tom,

On 3/13/17 1:03 PM, Tom Lane wrote:
> David Steele  writes:
>> At miscinit.c:893:
> 
>> /* We can treat the EPERM-error case as okay because that error implies
>> that the existing process has a different userid than we do, which means
>> it cannot be a competing postmaster.  A postmaster cannot successfully
>> attach to a data directory owned by a userid other than its own.  (This
>> is now checked directly in checkDataDir(), but has been true for a long
>> time because of the restriction that the data directory isn't group- or
>> world-accessible.)  Also, since we create the lockfiles mode 600, we'd
>> have failed above if the lockfile belonged to another userid --- which
>> means that whatever process kill() is reporting about isn't the one that
>> made the lockfile.  (NOTE: this last consideration is the only one that
>> keeps us from blowing away a Unix socket file belonging to an instance
>> of Postgres being run by someone else, at least on machines where /tmp
>> hasn't got a stickybit.) */
> 
> TBH, the fact that we're relying on 0600 mode for considerations such
> as these makes me tremendously afraid of this whole patch.  I think that
> the claimed advantages are not anywhere near worth the risk that somebody
> is going to destroy their database because we weakened some aspect of the
> protection against starting multiple postmasters in a database directory.

I don't see a risk if the user uses umask 0027 which is the example
given in the docs.  I'm happy to change this example to a strong
recommendation.

> At the very least, I'd want to see much closer analysis of the safety
> issues than I've seen so far in this thread.  

I think it's clear that there would be safety risks with unwise umask
choices.  I also think the example/recommended umask is safe.

Running external processes as the postgres user carries serious risks as
well.  Not only with regards to data access but the danger of corruption
due to bugs.  If a process does not require write access to do its job
then why take that risk?

To (hopefully) address your concerns, I'll perform an analysis of
starting multiple postmasters with a variety of umask choices and report
the outcomes here.

> And since the proposed
> patch falsifies the above-quoted comment (and probably others), why
> hasn't it updated it?

That was an oversight on my part.  I'll update it in the next patch.

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

Hi Tom,

On 3/13/17 1:13 PM, Tom Lane wrote:
> ... oh, and now that I've actually looked at the patch, I think it's
> a seriously bad idea to proceed by removing the mode parameter to
> PathNameOpenFile et al.  That's basically doubling down on an assumption
> that there are NO places in the backend, and never will be any, in which
> we want to create files with nondefault permissions.  That assumption
> seems broken on its face.  It also makes the patch exceedingly invasive
> for extensions.

I think it's a bad idea to have the same parameters copied over and over
throughout the code with slight variations (e.g. 0600 vs S_IRUSR |
S_IWUSR) but the same intent.

In all cases there is another version of the function (added by this
patch) that accepts a mode parameter.  In practice this was only needed
in one place, be_lo_export().  I think this makes a pretty good argument
for standardization/simplification in other areas.

Thanks,
-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tom Lane]

... oh, and now that I've actually looked at the patch, I think it's
a seriously bad idea to proceed by removing the mode parameter to
PathNameOpenFile et al.  That's basically doubling down on an assumption
that there are NO places in the backend, and never will be any, in which
we want to create files with nondefault permissions.  That assumption
seems broken on its face.  It also makes the patch exceedingly invasive
for extensions.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tom Lane]

David Steele  writes:
> At miscinit.c:893:

> /* We can treat the EPERM-error case as okay because that error implies
> that the existing process has a different userid than we do, which means
> it cannot be a competing postmaster.  A postmaster cannot successfully
> attach to a data directory owned by a userid other than its own.  (This
> is now checked directly in checkDataDir(), but has been true for a long
> time because of the restriction that the data directory isn't group- or
> world-accessible.)  Also, since we create the lockfiles mode 600, we'd
> have failed above if the lockfile belonged to another userid --- which
> means that whatever process kill() is reporting about isn't the one that
> made the lockfile.  (NOTE: this last consideration is the only one that
> keeps us from blowing away a Unix socket file belonging to an instance
> of Postgres being run by someone else, at least on machines where /tmp
> hasn't got a stickybit.) */

TBH, the fact that we're relying on 0600 mode for considerations such
as these makes me tremendously afraid of this whole patch.  I think that
the claimed advantages are not anywhere near worth the risk that somebody
is going to destroy their database because we weakened some aspect of the
protection against starting multiple postmasters in a database directory.

At the very least, I'd want to see much closer analysis of the safety
issues than I've seen so far in this thread.  And since the proposed
patch falsifies the above-quoted comment (and probably others), why
hasn't it updated it?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/10/17 8:34 AM, Stephen Frost wrote:
> Greetings,
> 
> * Tsunakawa, Takayuki (tsunakawa.ta...@jp.fujitsu.com) wrote:
>> From: pgsql-hackers-ow...@postgresql.org
>>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
>>> PostgreSQL currently requires the file mode mask (umask) to be 0077.
>>> However, this precludes the possibility of a user in the postgres group
>>> performing a backup (or whatever).  Now that
>>> pg_start_backup()/pg_stop_backup() privileges can be delegated to an
>>> unprivileged user, it makes sense to also allow a (relatively) unprivileged
>>> user to perform the backup at the file system level as well.
>>
>> I'd like to help review this.  First, let me give some questions and 
>> comments.

Much appreciated!

>> 3.The default location of the SSL key file is $PGDATA, so the permission of 
>> the key file is likely to become 0640.  But the current postgres requires it 
>> to be 0600.  See src/backend/libpq/be-secure-openssl.c.
> 
> Yes, that needs to be addressed.  There was discussion on another thread
> that it would be useful to support the SSL key file having group read
> access, but since this patch is handling the other files it seems like
> it would make sense to do that change here also.

Perhaps, but since these files are not setup by initdb I'm not sure if
we should be handling their permissions.  This seems to be a
distro-specific issue.

It seems to me that it would be best to advise in the docs that these
files should be relocated if they won't be readable by the backup user.
In any event, I'm not convinced that backing up server private keys is a
good idea.

>> 5.I think some explanation about the concept of multiple OS users is 
>> necessary, such as here:
>>
>> 16.1. Short Version
>> https://www.postgresql.org/docs/devel/static/install-short.html
>>
>> 18.2. Creating a Database Cluster
>> https://www.postgresql.org/docs/devel/static/creating-cluster.html
> 
> I agree that we should update the documention for this, including those.

We'll add that to the next patch.

Thanks,
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/10/17 8:12 AM, Stephen Frost wrote:
> Peter,
> 
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 2/28/17 20:58, David Steele wrote:
>>> This patch introduces a new initdb param, -u/-file-mode-mask, and a new
>>> GUC, file_mode_mask, to allow the default mode of files and directories
>>> in the $PGDATA directory to be modified.
>>
>> The postmaster.pid file appears not to observe the configured mask.
> 
> Good point, it should.

Leaving the mask on this file as-is was intentional.  At miscinit.c:829:

/* Think not to make the file protection weaker than 0600.  See comments
below. */

At miscinit.c:893:

/* We can treat the EPERM-error case as okay because that error implies
that the existing process has a different userid than we do, which means
it cannot be a competing postmaster.  A postmaster cannot successfully
attach to a data directory owned by a userid other than its own.  (This
is now checked directly in checkDataDir(), but has been true for a long
time because of the restriction that the data directory isn't group- or
world-accessible.)  Also, since we create the lockfiles mode 600, we'd
have failed above if the lockfile belonged to another userid --- which
means that whatever process kill() is reporting about isn't the one that
made the lockfile.  (NOTE: this last consideration is the only one that
keeps us from blowing away a Unix socket file belonging to an instance
of Postgres being run by someone else, at least on machines where /tmp
hasn't got a stickybit.) */

I can't see why this explanation does not continue to hold even if
permissions for other files are changed.  For the use cases I envision,
I don't think being able to read/manipulate postmaster.pid is important,
only to detect that it is present.

>> There ought to be a test, perhaps under src/bin/initdb/, to check for
>> that kind of thing.
> 
> Good idea.

Agreed, will add to next patch.

> >> There is no documentation update for initdb.

The --file-mode-mask option was added to the option list, but you are
probably referring to a paragraph under description.  Will add to the
next patch.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PATCH: Configurable file mode mask

[Stephen Frost]

Greetings,

* Tsunakawa, Takayuki (tsunakawa.ta...@jp.fujitsu.com) wrote:
> From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> > PostgreSQL currently requires the file mode mask (umask) to be 0077.
> > However, this precludes the possibility of a user in the postgres group
> > performing a backup (or whatever).  Now that
> > pg_start_backup()/pg_stop_backup() privileges can be delegated to an
> > unprivileged user, it makes sense to also allow a (relatively) unprivileged
> > user to perform the backup at the file system level as well.
> 
> I'd like to help review this.  First, let me give some questions and comments.

Great!

> 1.What's the concrete use case of this feature?  Do you intend to extend the 
> concept of multiple DBAs to the full range of administration of a single 
> database instance, or just multiple OS users for database backup?

This is to allow a non-postgres user to perform a backup of the
database.  Perhaps this could be leveraged for other administration
functions, but it's not clear how off-hand to me and the backup use-case
is the reason for adding this.

> If you think that multiple OS user support is desirable to reduce the 
> administration burdon on a single person, then isn't the automated backup 
> sufficient (such as with cron)? 

I'm not quite sure what the question here is, but it is desirable to
minimize the amount of access any process requires to only that which is
required to perform its duties.  In the case of backup, only read access
to the data directory and access to connect to PG and run certain
functions is required.  The ability to run those functions as a
non-superuser was added in 9.6, this continues the work to minimize what
a backup user needs to perform a backup of the system by allowing a user
to have only read-only access to the data directory.

There are multiple reasons why matching the privileges a process has to
only that which is required is good practice.  Minimizing impact to
ongoing operations from a compromise of the backup user and reducing the
risk that bugs in backup software could disrupt operations are two of
those.

> 2.Backup should always be considered with recovery.  If you allow another OS 
> user to back up the database, can you allow him to recover the database as 
> well?

That would not be our recommended approach, but it would be possible to
do.  Our recommended solution is for the backup user to only be able to
perform the backup.  In this use-case, the restore would be run by the
OS user who owns the database (eg: postgres), who would have read-only
access to the backup repository.

To be clear, this is not hypothetical, pgBackrest supports these
configurations and has been tested with this approach.  As noted, there
are a few additional items that need to be addressed which weren't
covered in the initial testing (on Debian-based systems, the pid file
and SSL key aren't in the data directory, so they didn't pose a problem,
but they should be addressed so that this can work on other
distributions).

> For example, assume the PostgreSQL user account (the OS user who does initdb 
> and pg_ctl start/stop) is dba1, and dba2 backs up the database using tar or 
> cpio.
> When dba2 restores the backup, the owner of the database cluster becomes 
> dba2.  If the file permission only allows one user to write the file, then 
> dba1 can't start the instance.

If the uesr chose to configure the system with both dba1 and dba2 having
write access to the data directory, performing a restore as dba2 would
be possible and the backup utility could be sure to remove all existing
files and restore them from the backup, ensuring that all of the files
would be owned by a single user (dba2 in this case).  Of course, one
would then need to adjust the startup process to run as dba2 instead.

With group write access to all of the files and directories, it may be
possible to actually run with the dba1 user even though the files are
owned by dba2, but we would not recommend it as the result would be a
mix of files owned by one dba or the other.  This is not the use-case
this is being developed for.

> 3.The default location of the SSL key file is $PGDATA, so the permission of 
> the key file is likely to become 0640.  But the current postgres requires it 
> to be 0600.  See src/backend/libpq/be-secure-openssl.c.

Yes, that needs to be addressed.  There was discussion on another thread
that it would be useful to support the SSL key file having group read
access, but since this patch is handling the other files it seems like
it would make sense to do that change here also.

> 4.I've seen a few users to place .pgpass file in $PGDATA and set the 
> environment variable PGPASSFILE to point to it.  They expect it to be back up 
> with other database files.  So I'm afraid the permission of .pgpass file also 
> becomes 0640 some time.  However, the current code requires it to be 0600.  
> See 

Re: [HACKERS] PATCH: Configurable file mode mask

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2/28/17 20:58, David Steele wrote:
> > This patch introduces a new initdb param, -u/-file-mode-mask, and a new
> > GUC, file_mode_mask, to allow the default mode of files and directories
> > in the $PGDATA directory to be modified.
> 
> The postmaster.pid file appears not to observe the configured mask.

Good point, it should.

> There ought to be a test, perhaps under src/bin/initdb/, to check for
> that kind of thing.

Good idea.

> There is no documentation update for initdb.

Right, that needs to be fixed.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Configurable file mode mask

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> PostgreSQL currently requires the file mode mask (umask) to be 0077.
> However, this precludes the possibility of a user in the postgres group
> performing a backup (or whatever).  Now that
> pg_start_backup()/pg_stop_backup() privileges can be delegated to an
> unprivileged user, it makes sense to also allow a (relatively) unprivileged
> user to perform the backup at the file system level as well.

I'd like to help review this.  First, let me give some questions and comments.


1.What's the concrete use case of this feature?  Do you intend to extend the 
concept of multiple DBAs to the full range of administration of a single 
database instance, or just multiple OS users for database backup?
If you think that multiple OS user support is desirable to reduce the 
administration burdon on a single person, then isn't the automated backup 
sufficient (such as with cron)? 

2.Backup should always be considered with recovery.  If you allow another OS 
user to back up the database, can you allow him to recover the database as well?
For example, assume the PostgreSQL user account (the OS user who does initdb 
and pg_ctl start/stop) is dba1, and dba2 backs up the database using tar or 
cpio.
When dba2 restores the backup, the owner of the database cluster becomes dba2.  
If the file permission only allows one user to write the file, then dba1 can't 
start the instance.

3.The default location of the SSL key file is $PGDATA, so the permission of the 
key file is likely to become 0640.  But the current postgres requires it to be 
0600.  See src/backend/libpq/be-secure-openssl.c.

4.I've seen a few users to place .pgpass file in $PGDATA and set the 
environment variable PGPASSFILE to point to it.  They expect it to be back up 
with other database files.  So I'm afraid the permission of .pgpass file also 
becomes 0640 some time.  However, the current code requires it to be 0600.  See 
src/interface/libpq/fe-connect.c.

5.I think some explanation about the concept of multiple OS users is necessary, 
such as here:

16.1. Short Version
https://www.postgresql.org/docs/devel/static/install-short.html

18.2. Creating a Database Cluster
https://www.postgresql.org/docs/devel/static/creating-cluster.html


[FYI]
Oracle instructs the user, during the software installation, to put "umask 022" 
in ~/.bashrc or so.
MySQL's files in the data directory appears to be 0640.

Regards
Takayuki Tsunakawa


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Peter Eisentraut]

On 2/28/17 20:58, David Steele wrote:
> This patch introduces a new initdb param, -u/-file-mode-mask, and a new
> GUC, file_mode_mask, to allow the default mode of files and directories
> in the $PGDATA directory to be modified.

The postmaster.pid file appears not to observe the configured mask.

There ought to be a test, perhaps under src/bin/initdb/, to check for
that kind of thing.

There is no documentation update for initdb.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/6/17 8:17 AM, Robert Haas wrote:
> On Mon, Mar 6, 2017 at 7:38 AM, Tom Lane  wrote:
>> Simon Riggs  writes:
>>> On 1 March 2017 at 01:58, David Steele  wrote:
 PostgreSQL currently requires the file mode mask (umask) to be 0077.
 However, this precludes the possibility of a user in the postgres group
 performing a backup (or whatever).  Now that
 pg_start_backup()/pg_stop_backup() privileges can be delegated to an
 unprivileged user, it makes sense to also allow a (relatively)
 unprivileged user to perform the backup at the file system level as well.
>>
>>> +1
>>
>> I'd ask what is the point, considering that we don't view "cp -a" as a
>> supported backup technique in the first place.
> 
> /me is confused.
> 
> Surely the idea is that you'd like an unprivileged database user to
> run pg_start_backup(), an operating system user that can read but not
> write the database files to copy them, and then the unprivileged to
> then run pg_stop_backup().  I have no opinion on the patch, but I
> support the goal.  As I said on the surprisingly-controversial thread
> about ripping out hard-coded superuser checks, reducing the level of
> privilege which someone must have in order to perform a necessary
> operation leads to better security.  An exclusive backup taken via the
> filesystem (probably not via cp, but say via tar or cpio) inevitably
> requires the backup user to be able to read the entire cluster
> directory, but it doesn't inherently require the backup user to be
> able to write the cluster directory.

Limiting privileges also serves to guard against any bugs in tools that
run directly against $PGDATA and do not require write privileges.

-- 
-David
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[David Steele]

On 3/6/17 8:50 AM, Stephen Frost wrote:

> * Simon Riggs (si...@2ndquadrant.com) wrote:
>>> to allow the default mode of files and directories
>>> in the $PGDATA directory to be modified.
>>
>> Are you saying if this is changed all files/directories will be
>> changed to the new mode?
> 
> No, new files will be created with the new mode and existing files will
> be allowed to have the mode set.  Changing all of the existing files
> didn't seem like something we should be trying to do at server start.
> 
>> It seems like it would be annoying to have some files in one mode,
>> some in another.
> 
> It's not intended for that to happen, but it is possible for it to.  The
> alternative is to try and forcibly change all files at server start time
> to match what is configured but that didn't seem like a great idea.

Agreed.  It would definitely affect server start time, perhaps
significantly.

I have added a note to the docs that a change in file_mode_mask does not
automatically change the mode of existing files on disk.

This patch applies cleanly on 6f3a13f.

-- 
-David
da...@pgmasters.net
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c 
b/contrib/pg_stat_statements/pg_stat_statements.c
index 62dec87..98f8170 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1858,8 +1858,7 @@ qtext_store(const char *query, int query_len,
*query_offset = off;
 
/* Now write the data into the successfully-reserved part of the file */
-   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY,
-  S_IRUSR | S_IWUSR);
+   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY);
if (fd < 0)
goto error;
 
@@ -1923,7 +1922,7 @@ qtext_load_file(Size *buffer_size)
int fd;
struct stat stat;
 
-   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY, 0);
+   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index cd82c04..6c84082 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -812,6 +812,44 @@ include_dir 'conf.d'
   
  
 
+ 
+  file_mode_mask (integer)
+  
+   file_mode_mask configuration parameter
+  
+  
+  
+   
+Sets the file mode mask (umask) for the data directory. The parameter
+value is expected to be a numeric mode specified in the format
+accepted by the chmod and
+umask system calls. (To use the customary octal
+format the number must start with a 0 (zero).)
+   
+
+   
+The default file_mode_mask is 
0077,
+meaning that the only the database owner can read and write files in
+the data directory.  For example, setting the
+file_mode_mask to 0027 would 
allow
+any user in the same group as the database owner to read all database
+files, which would be useful for producing a backup using a relatively
+unprivileged user.
+   
+
+   
+ Note that changing this parameter does not automatically change the
+ mode of existing files in the cluster.  This must be done manually by
+ an administator.
+   
+
+   
+This parameter can only be set at server start.
+   
+
+  
+ 
+
  
   bonjour (boolean)
   
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 1aaa490..bd1f849 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -296,6 +296,25 @@ PostgreSQL documentation
  
 
  
+  -u mask
+  --file-mode-mask=mask
+  
+   
+Sets the file mode mask (umask) for the data directory. The parameter
+value is expected to be a numeric mode specified in the format
+accepted by the chmod and
+umask system calls. (To use the customary octal
+format the number must start with a 0 (zero).)
+   
+
+   
+Specifying this parameter will automatically set
+ in postgresql.conf.
+   
+  
+ 
+
+ 
   -W
   --pwprompt
   
diff --git a/src/backend/access/heap/rewriteheap.c 
b/src/backend/access/heap/rewriteheap.c
index c7b283c..53a2acc 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1010,8 +1010,7 @@ logical_rewrite_log_mapping(RewriteState state, 
TransactionId xid,
src->off = 0;
memcpy(src->path, path, sizeof(path));
src->vfd = PathNameOpenFile(path,
-   O_CREAT 
| O_EXCL | O_WRONLY | PG_BINARY,
-   S_IRUSR 
| S_IWUSR);
+

Re: [HACKERS] PATCH: Configurable file mode mask

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Simon Riggs  writes:
> > On 1 March 2017 at 01:58, David Steele  wrote:
> >> PostgreSQL currently requires the file mode mask (umask) to be 0077.
> >> However, this precludes the possibility of a user in the postgres group
> >> performing a backup (or whatever).  Now that
> >> pg_start_backup()/pg_stop_backup() privileges can be delegated to an
> >> unprivileged user, it makes sense to also allow a (relatively)
> >> unprivileged user to perform the backup at the file system level as well.
> 
> > +1
> 
> I'd ask what is the point, considering that we don't view "cp -a" as a
> supported backup technique in the first place.

The point is to allow backups to be performed as a user who only has
read-only access to the files and is a non-superuser in the database.
With the changes to allow GRANT'ing of the pg_start/stop_backup and
related functions and these changes to allow the files to be group
readable, that will be possible using a tool such as pgbackrest, not
just with a "cp -a".

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Configurable file mode mask

[Stephen Frost]

Greetings,

* Simon Riggs (si...@2ndquadrant.com) wrote:
> On 1 March 2017 at 01:58, David Steele  wrote:
> > PostgreSQL currently requires the file mode mask (umask) to be 0077.
> > However, this precludes the possibility of a user in the postgres group
> > performing a backup (or whatever).  Now that
> > pg_start_backup()/pg_stop_backup() privileges can be delegated to an
> > unprivileged user, it makes sense to also allow a (relatively)
> > unprivileged user to perform the backup at the file system level as well.
> 
> +1
> 
> > This patch introduces a new initdb param, -u/-file-mode-mask, and a new
> > GUC, file_mode_mask,
> 
> Why both initdb and at server start? Seems like initdb is OK, or in 
> pg_control.

One could imagine someone wishing to change their mind regarding the
permissions after initdb, and for existing systems who may wish to move
to allowing group-read in an environment where that can be safely done
but don't wish to re-initdb.

> > to allow the default mode of files and directories
> > in the $PGDATA directory to be modified.
> 
> Are you saying if this is changed all files/directories will be
> changed to the new mode?

No, new files will be created with the new mode and existing files will
be allowed to have the mode set.  Changing all of the existing files
didn't seem like something we should be trying to do at server start.

> It seems like it would be annoying to have some files in one mode,
> some in another.

It's not intended for that to happen, but it is possible for it to.  The
alternative is to try and forcibly change all files at server start time
to match what is configured but that didn't seem like a great idea.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Configurable file mode mask

[Robert Haas]

On Mon, Mar 6, 2017 at 7:38 AM, Tom Lane  wrote:
> Simon Riggs  writes:
>> On 1 March 2017 at 01:58, David Steele  wrote:
>>> PostgreSQL currently requires the file mode mask (umask) to be 0077.
>>> However, this precludes the possibility of a user in the postgres group
>>> performing a backup (or whatever).  Now that
>>> pg_start_backup()/pg_stop_backup() privileges can be delegated to an
>>> unprivileged user, it makes sense to also allow a (relatively)
>>> unprivileged user to perform the backup at the file system level as well.
>
>> +1
>
> I'd ask what is the point, considering that we don't view "cp -a" as a
> supported backup technique in the first place.

/me is confused.

Surely the idea is that you'd like an unprivileged database user to
run pg_start_backup(), an operating system user that can read but not
write the database files to copy them, and then the unprivileged to
then run pg_stop_backup().  I have no opinion on the patch, but I
support the goal.  As I said on the surprisingly-controversial thread
about ripping out hard-coded superuser checks, reducing the level of
privilege which someone must have in order to perform a necessary
operation leads to better security.  An exclusive backup taken via the
filesystem (probably not via cp, but say via tar or cpio) inevitably
requires the backup user to be able to read the entire cluster
directory, but it doesn't inherently require the backup user to be
able to write the cluster directory.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Tom Lane]

Simon Riggs  writes:
> On 1 March 2017 at 01:58, David Steele  wrote:
>> PostgreSQL currently requires the file mode mask (umask) to be 0077.
>> However, this precludes the possibility of a user in the postgres group
>> performing a backup (or whatever).  Now that
>> pg_start_backup()/pg_stop_backup() privileges can be delegated to an
>> unprivileged user, it makes sense to also allow a (relatively)
>> unprivileged user to perform the backup at the file system level as well.

> +1

I'd ask what is the point, considering that we don't view "cp -a" as a
supported backup technique in the first place.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: Configurable file mode mask

[Simon Riggs]

On 1 March 2017 at 01:58, David Steele  wrote:
> PostgreSQL currently requires the file mode mask (umask) to be 0077.
> However, this precludes the possibility of a user in the postgres group
> performing a backup (or whatever).  Now that
> pg_start_backup()/pg_stop_backup() privileges can be delegated to an
> unprivileged user, it makes sense to also allow a (relatively)
> unprivileged user to perform the backup at the file system level as well.

+1

> This patch introduces a new initdb param, -u/-file-mode-mask, and a new
> GUC, file_mode_mask,

Why both initdb and at server start? Seems like initdb is OK, or in pg_control.

> to allow the default mode of files and directories
> in the $PGDATA directory to be modified.

Are you saying if this is changed all files/directories will be
changed to the new mode?

It seems like it would be annoying to have some files in one mode,
some in another.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] PATCH: Configurable file mode mask

[David Steele]

PostgreSQL currently requires the file mode mask (umask) to be 0077.
However, this precludes the possibility of a user in the postgres group
performing a backup (or whatever).  Now that
pg_start_backup()/pg_stop_backup() privileges can be delegated to an
unprivileged user, it makes sense to also allow a (relatively)
unprivileged user to perform the backup at the file system level as well.

This patch introduces a new initdb param, -u/-file-mode-mask, and a new
GUC, file_mode_mask, to allow the default mode of files and directories
in the $PGDATA directory to be modified.

This obviously required mode changes in a number of places, so at the
same time the BasicOpenFile(), OpenTransientFile(), and
PathNameOpenFile() have been split into versions that either use the
default permissions or allow custom permissions.  In the end there was
only one call to the custom permission version (be-fsstubs.c:505) for
all three variants.

The following three calls (at the least) need to be reviewed:

bin/pg_dump/pg_backup_directory.c:194
src/port/mkdtemp.c:190
bin/pg_basebackup.c:599:655:1399

And this call needs serious consideration:

bin/pg_rewind/file_ops.c:214

Besides that there should be tests to make sure the masks are working as
expected and these could be added to the initdb TAP tests, though no
mask tests exist at this time.  Making sure all file operations produce
the correct modes would need to be placed in a new module, perhaps the
new backup tests proposed in [1].

Adam Brightwell developed the patch based on an initial concept by me
and Stephen Frost.  I added the refactoring in fd.c and some additional
documentation.

This patch applies cleanly on 016c990 but may fare badly over time due
to the number of files modified.

-- 
-David
da...@pgmasters.net

[1]
https://www.postgresql.org/message-id/758e3fd1-45b4-5e28-75cd-e9e7f93a4...@pgmasters.net
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c 
b/contrib/pg_stat_statements/pg_stat_statements.c
index 62dec87..98f8170 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1858,8 +1858,7 @@ qtext_store(const char *query, int query_len,
*query_offset = off;
 
/* Now write the data into the successfully-reserved part of the file */
-   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY,
-  S_IRUSR | S_IWUSR);
+   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDWR | O_CREAT | PG_BINARY);
if (fd < 0)
goto error;
 
@@ -1923,7 +1922,7 @@ qtext_load_file(Size *buffer_size)
int fd;
struct stat stat;
 
-   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY, 0);
+   fd = OpenTransientFile(PGSS_TEXT_FILE, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 1b390a2..2371878 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -812,6 +812,38 @@ include_dir 'conf.d'
   
  
 
+ 
+  file_mode_mask (integer)
+  
+   file_mode_mask configuration parameter
+  
+  
+  
+   
+Sets the file mode mask (umask) for the data directory. The parameter
+value is expected to be a numeric mode specified in the format
+accepted by the chmod and
+umask system calls. (To use the customary octal
+format the number must start with a 0 (zero).)
+   
+
+   
+The default file_mode_mask is 
0077,
+meaning that the only the database owner can read and write files in
+the data directory.  For example, setting the
+file_mode_mask to 0027 would 
allow
+any user in the same group as the database owner to read all database
+files, which would be useful for producing a backup using a relatively
+unprivileged user.
+   
+
+   
+This parameter can only be set at server start.
+   
+
+  
+ 
+
  
   bonjour (boolean)
   
diff --git a/src/backend/access/heap/rewriteheap.c 
b/src/backend/access/heap/rewriteheap.c
index c7b283c..53a2acc 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1010,8 +1010,7 @@ logical_rewrite_log_mapping(RewriteState state, 
TransactionId xid,
src->off = 0;
memcpy(src->path, path, sizeof(path));
src->vfd = PathNameOpenFile(path,
-   O_CREAT 
| O_EXCL | O_WRONLY | PG_BINARY,
-   S_IRUSR 
| S_IWUSR);
+   O_CREAT 
| O_EXCL | O_WRONLY | PG_BINARY);
if (src->vfd < 0)
ereport(ERROR,