Horton

Thread

Commits

  1. Allow root-owned SSL private keys in libpq, not only the backend.

  2. Doc: update libpq.sgml for root-owned SSL private keys.

  1. Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2021-10-22T15:41:21Z

    Hackers,

I noticed recently that permissions checking is done differently for the 
server certificate key than the client key. Specifically, on the server 
the key can have 640 perms if it is owned by root.

On the server side this change was made in 9a83564c and I think the same 
rational applies equally well to the client key. At the time managed 
keys on the client may not have been common but they are now.

Attached is a patch to make this change.

I was able to this this manually by hacking 001_ssltests.pl like so:

-	chmod 0640, "ssl/${key}_tmp.key"
+	chmod 0600, "ssl/${key}_tmp.key"
  	  or die "failed to change permissions on ssl/${key}_tmp.key: $!";
-	system_or_bail("sudo chown root ssl/${key}_tmp.key");

But this is clearly not going to work for general purpose testing. The 
server keys also not tested for root ownership so perhaps we do not need 
that here either.

I looked at trying to make this code common between the server and 
client but due to the differences in error reporting it seemed like more 
trouble than it was worth.

Regards,
-- 
-David
david@pgmasters.net

  2. Re: Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2021-10-28T13:08:38Z

    On 10/22/21 11:41 AM, David Steele wrote:
> 
> I noticed recently that permissions checking is done differently for the 
> server certificate key than the client key. Specifically, on the server 
> the key can have 640 perms if it is owned by root.
> 
> On the server side this change was made in 9a83564c and I think the same 
> rational applies equally well to the client key. At the time managed 
> keys on the client may not have been common but they are now.
> 
> Attached is a patch to make this change.
> 
> I was able to this this manually by hacking 001_ssltests.pl like so:
> 
> -    chmod 0640, "ssl/${key}_tmp.key"
> +    chmod 0600, "ssl/${key}_tmp.key"
>         or die "failed to change permissions on ssl/${key}_tmp.key: $!";
> -    system_or_bail("sudo chown root ssl/${key}_tmp.key");
> 
> But this is clearly not going to work for general purpose testing. The 
> server keys also not tested for root ownership so perhaps we do not need 
> that here either.
> 
> I looked at trying to make this code common between the server and 
> client but due to the differences in error reporting it seemed like more 
> trouble than it was worth.

Added to next CF: https://commitfest.postgresql.org/35/3379

-- 
-David
david@pgmasters.net

  3. Re: Allow root ownership of client certificate key

    Stephen Frost <sfrost@snowman.net> — 2021-11-08T19:04:05Z

    Greetings,

* David Steele (david@pgmasters.net) wrote:
> I noticed recently that permissions checking is done differently for the
> server certificate key than the client key. Specifically, on the server the
> key can have 640 perms if it is owned by root.

Yeah, that strikes me as odd too, particularly given that many many
cases of client-side certificates are application servers and not actual
end users.  If we can justify having a looser check on the PG server
side then it surely makes sense that an app server could also be
justified in having such a permission setup (and it definitely happens
often in Kubernetes/OpenShift and such places where secrets are mounted
from somewhere else).

> On the server side this change was made in 9a83564c and I think the same
> rational applies equally well to the client key. At the time managed keys on
> the client may not have been common but they are now.

Agreed.

> Attached is a patch to make this change.
> 
> I was able to this this manually by hacking 001_ssltests.pl like so:
> 
> -	chmod 0640, "ssl/${key}_tmp.key"
> +	chmod 0600, "ssl/${key}_tmp.key"
>  	  or die "failed to change permissions on ssl/${key}_tmp.key: $!";
> -	system_or_bail("sudo chown root ssl/${key}_tmp.key");
> 
> But this is clearly not going to work for general purpose testing. The
> server keys also not tested for root ownership so perhaps we do not need
> that here either.

Makes sense to me.

> I looked at trying to make this code common between the server and client
> but due to the differences in error reporting it seemed like more trouble
> than it was worth.

Maybe we should at least have the comments refer to each other though,
to hopefully encourage future hackers in this area to maintain
consistency between the two and avoid what happened before..?

> diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
> index 3a7cc8f774..285e772170 100644
> --- a/src/interfaces/libpq/fe-secure-openssl.c
> +++ b/src/interfaces/libpq/fe-secure-openssl.c
> @@ -1234,11 +1234,38 @@ initialize_SSL(PGconn *conn)
>  							  fnbuf);
>  			return -1;
>  		}
> +
> +		/*
> +		* Refuse to load key files owned by users other than us or root.
> +		*
> +		* XXX surely we can check this on Windows somehow, too.
> +		*/

Not really sure if there's actually much point in having this marked in
this way as it's not apparently something we're going to actually fix in
the near term.  Maybe instead something like "Would be nice to find a
way to do this on Windows somehow, too, but it isn't clear today how
to."

> +#ifndef WIN32
> +		if (buf.st_uid != geteuid() && buf.st_uid != 0)
> +		{
> +			appendPQExpBuffer(&conn->errorMessage,
> +							  libpq_gettext("private key file \"%s\" must be owned by the current user or root\n"),
> +							  fnbuf);
> +			return -1;
> +		}
> +#endif

Basically the same check as what is done on the server side, so this
looks good to me.

> +		/*
> +		* Require no public access to key file. If the file is owned by us,
> +		* require mode 0600 or less. If owned by root, require 0640 or less to
> +		* allow read access through our gid, or a supplementary gid that allows
> +		* to read system-wide certificates.
> +		*
> +		* XXX temporarily suppress check when on Windows, because there may not
> +		* be proper support for Unix-y file permissions.  Need to think of a
> +		* reasonable check to apply on Windows.
> +		*/

On the server-side, we also include a reference to postmaster.c.  Not
sure if we need to do that or not but just figured I'd mention it.

>  #ifndef WIN32
> -		if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
> +		if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXG | S_IRWXO)) ||
> +			(buf.st_uid == 0 && buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)))
>  		{
>  			appendPQExpBuffer(&conn->errorMessage,
> -							  libpq_gettext("private key file \"%s\" has group or world access; permissions should be u=rw (0600) or less\n"),
> +							  libpq_gettext("private key file \"%s\" has group or world access; file must have permissions u=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by root.\n"),
>  							  fnbuf);
>  			return -1;
>  		}

Do we really want to remove the S_ISREG() check?  We have that check
(although a bit earlier) on the server side and we've had it for a very
long time, so I don't think that we want to drop it, certainly not
without some additional discussion as to why we should (and why it would
make sense to have that be different between the client side and the
server side).

Thanks,

Stephen

  4. Re: Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2021-11-08T22:36:39Z

    On 11/8/21 2:04 PM, Stephen Frost wrote:
> * David Steele (david@pgmasters.net) wrote:
> 
>> I looked at trying to make this code common between the server and client
>> but due to the differences in error reporting it seemed like more trouble
>> than it was worth.
> 
> Maybe we should at least have the comments refer to each other though,
> to hopefully encourage future hackers in this area to maintain
> consistency between the two and avoid what happened before..?

Done.

>> +
>> +		/*
>> +		* Refuse to load key files owned by users other than us or root.
>> +		*
>> +		* XXX surely we can check this on Windows somehow, too.
>> +		*/
> 
> Not really sure if there's actually much point in having this marked in
> this way as it's not apparently something we're going to actually fix in
> the near term.  Maybe instead something like "Would be nice to find a
> way to do this on Windows somehow, too, but it isn't clear today how
> to."

Done.

>> +		/*
>> +		* Require no public access to key file. If the file is owned by us,
>> +		* require mode 0600 or less. If owned by root, require 0640 or less to
>> +		* allow read access through our gid, or a supplementary gid that allows
>> +		* to read system-wide certificates.
>> +		*
>> +		* XXX temporarily suppress check when on Windows, because there may not
>> +		* be proper support for Unix-y file permissions.  Need to think of a
>> +		* reasonable check to apply on Windows.
>> +		*/
> 
> On the server-side, we also include a reference to postmaster.c.  Not
> sure if we need to do that or not but just figured I'd mention it.

Looks like this moved to miscinit.c so probably this comment deserves an 
update. That might be better as a separate commit.

In the patch I referenced the function name instead since that will come 
up in searches when the original function gets renamed/moved.

>>   #ifndef WIN32
>> -		if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
>> +		if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXG | S_IRWXO)) ||
>> +			(buf.st_uid == 0 && buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)))
>>   		{
>>   			appendPQExpBuffer(&conn->errorMessage,
>> -							  libpq_gettext("private key file \"%s\" has group or world access; permissions should be u=rw (0600) or less\n"),
>> +							  libpq_gettext("private key file \"%s\" has group or world access; file must have permissions u=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by root.\n"),
>>   							  fnbuf);
>>   			return -1;
>>   		}
> 
> Do we really want to remove the S_ISREG() check?  We have that check
> (although a bit earlier) on the server side and we've had it for a very
> long time, so I don't think that we want to drop it, certainly not
> without some additional discussion as to why we should (and why it would
> make sense to have that be different between the client side and the
> server side).

Oof. Definitely a copy-paste error.

A new version is attached with these changes, plus I consolidated the 
checks under one comment block to reduce comment and #ifdef duplication.

We may want to do the same on the server side to make the code blocks 
look more similar.

Also, on the server side the S_ISREG() check gets its own error and that 
might be a good idea on the client side as well. As it is, the error 
message on the client is going to be pretty confusing in this case.

Regards,
-- 
-David
david@pgmasters.net

  5. Re: Allow root ownership of client certificate key

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-18T20:41:02Z

    David Steele <david@pgmasters.net> writes:
> [ client-key-perm-002.patch ]

I took a quick look at this and agree with the proposed behavior
change, but also with your self-criticisms:

> We may want to do the same on the server side to make the code blocks 
> look more similar.
>
> Also, on the server side the S_ISREG() check gets its own error and that 
> might be a good idea on the client side as well. As it is, the error 
> message on the client is going to be pretty confusing in this case.

Particularly, I think the S_ISREG check should happen before any
ownership/permissions checks; it just seems saner that way.

The only other nitpick I have is that I'd make the cross-references be
to the two file names, ie like "Note that similar checks are performed
in fe-secure-openssl.c ..."  References to the specific functions seem
likely to bit-rot in the face of future code rearrangements.
I suppose filename references could become obsolete too, but it
seems less likely.

			regards, tom lane

  6. Re: Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2022-01-18T21:44:29Z

    On 1/18/22 15:41, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
> 
> I took a quick look at this and agree with the proposed behavior
> change, but also with your self-criticisms:
> 
>> We may want to do the same on the server side to make the code blocks
>> look more similar.
>>
>> Also, on the server side the S_ISREG() check gets its own error and that
>> might be a good idea on the client side as well. As it is, the error
>> message on the client is going to be pretty confusing in this case.
> 
> Particularly, I think the S_ISREG check should happen before any
> ownership/permissions checks; it just seems saner that way.

I was worried about doing too much refactoring in this commit since I 
have hopes and dreams of it being back-patched. But I'll go ahead and do 
that and if any part of this can be back-patched we'll consider that 
separately.

> The only other nitpick I have is that I'd make the cross-references be
> to the two file names, ie like "Note that similar checks are performed
> in fe-secure-openssl.c ..."  References to the specific functions seem
> likely to bit-rot in the face of future code rearrangements.
> I suppose filename references could become obsolete too, but it
> seems less likely.

It's true that functions are more likely to be renamed, but when I 
rename a function I then search for all the places where it is used so I 
can update them. If the function name appears in a comment that gets 
updated as well.

If you would still prefer filenames I have no strong argument against 
that, just wanted to explain my logic.

Regards,
-David

  7. Re: Allow root ownership of client certificate key

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-18T21:51:48Z

    David Steele <david@pgmasters.net> writes:
> On 1/18/22 15:41, Tom Lane wrote:
>> The only other nitpick I have is that I'd make the cross-references be
>> to the two file names, ie like "Note that similar checks are performed
>> in fe-secure-openssl.c ..."  References to the specific functions seem
>> likely to bit-rot in the face of future code rearrangements.
>> I suppose filename references could become obsolete too, but it
>> seems less likely.

> It's true that functions are more likely to be renamed, but when I 
> rename a function I then search for all the places where it is used so I 
> can update them. If the function name appears in a comment that gets 
> updated as well.

Harsh experience says that a lot of Postgres contributors have zero
interest in updating comments two lines away from what they're editing,
let alone in some distant branch of the source tree.  But I'm not dead
set on it either way.

			regards, tom lane

  8. Re: Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2022-02-16T18:57:15Z

    Hi Tom,

On 1/18/22 14:41, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>> [ client-key-perm-002.patch ]
> 
> I took a quick look at this and agree with the proposed behavior
> change, but also with your self-criticisms:
> 
>> We may want to do the same on the server side to make the code blocks
>> look more similar.
>>
>> Also, on the server side the S_ISREG() check gets its own error and that
>> might be a good idea on the client side as well. As it is, the error
>> message on the client is going to be pretty confusing in this case.
> 
> Particularly, I think the S_ISREG check should happen before any
> ownership/permissions checks; it just seems saner that way.

The two blocks of code now look pretty much identical except for error 
handling and the reference to the other file. Also, the indentation for 
the comment on the server side is less but I kept the comment formatting 
the same to make it easier to copy the comment back and forth.

> The only other nitpick I have is that I'd make the cross-references be
> to the two file names, ie like "Note that similar checks are performed
> in fe-secure-openssl.c ..."  References to the specific functions seem
> likely to bit-rot in the face of future code rearrangements.
> I suppose filename references could become obsolete too, but it
> seems less likely.

Updated these to reference the file instead of the function.

I still don't think we can commit the test for root ownership, but 
testing it manually got a whole lot easier after the refactor in 
c3b34a0f. Before that you had to hack up the source tree, which is a 
pain depending on how it is mounted (I'm testing in a container).

So, to test the new functionality, just add this snippet on line 57 of 
001_ssltests.pl:

chmod 0640, "$cert_tempdir/client.key"
	or die "failed to change permissions on $cert_tempdir/client.key: $!";
system_or_bail("sudo chown root $cert_tempdir/client.key");

If you can think of a way to add this to the tests I'm all ears. Perhaps 
we could add these lines commented out and explain what they are for?

Regards,
-David

  9. Re: Allow root ownership of client certificate key

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-28T19:20:03Z

    David Steele <david@pgmasters.net> writes:
> [ client-key-perm-003.patch ]

Pushed with a bit of copy-editing of the comments.

> So, to test the new functionality, just add this snippet on line 57 of 
> 001_ssltests.pl:
> chmod 0640, "$cert_tempdir/client.key"
> 	or die "failed to change permissions on $cert_tempdir/client.key: $!";
> system_or_bail("sudo chown root $cert_tempdir/client.key");
> If you can think of a way to add this to the tests I'm all ears. Perhaps 
> we could add these lines commented out and explain what they are for?

I believe we have some precedents for invoking this sort of test
optionally if an appropriate environment variable is set.  However,
I'm having a pretty hard time seeing that there's any real use-case
for a test set up like this.  The TAP tests are meant for automatic
testing, and nobody is going to run automatic tests in an environment
where they'd be allowed to sudo.  (Or at least I sure hope nobody
working on this project is that naive.)

If somebody wants to put this in despite that, I'd merely suggest
that the server-side logic ought to get exercised too.

			regards, tom lane

  10. Re: Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2022-03-01T00:07:28Z

    On 2/28/22 13:20, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>> [ client-key-perm-003.patch ]
> 
> Pushed with a bit of copy-editing of the comments.

Thank you!

Any thoughts on back-patching at least the client portion of this? 
Probably hard to argue that it's a bug, but it is certainly painful.

Regards,
-David

  11. Re: Allow root ownership of client certificate key

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-01T00:12:06Z

    David Steele <david@pgmasters.net> writes:
> Any thoughts on back-patching at least the client portion of this? 
> Probably hard to argue that it's a bug, but it is certainly painful.

I'd be more eager to do that if we had some field complaints
about it.  Since we don't, my inclination is not to, but I'm
only -0.1 or so; anybody else want to vote?

			regards, tom lane

  12. Re: Allow root ownership of client certificate key

    Stephen Frost <sfrost@snowman.net> — 2022-03-01T00:31:53Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> David Steele <david@pgmasters.net> writes:
> > Any thoughts on back-patching at least the client portion of this? 
> > Probably hard to argue that it's a bug, but it is certainly painful.
> 
> I'd be more eager to do that if we had some field complaints
> about it.  Since we don't, my inclination is not to, but I'm
> only -0.1 or so; anybody else want to vote?

This patch was specifically developed in response to field complaints
about it working differently, so there's that.  Currently it's being
worked around in the container environments by copying the key from the
secret that's provided to a temporary space where we can modify the
privileges, but that's pretty terrible.  Would be great to be able to
get rid of that in favor of being able to use it directly.

Thanks,

Stephen

  13. Re: Allow root ownership of client certificate key

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-01T03:15:16Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I'd be more eager to do that if we had some field complaints
>> about it.  Since we don't, my inclination is not to, but I'm
>> only -0.1 or so; anybody else want to vote?

> This patch was specifically developed in response to field complaints
> about it working differently, so there's that.

Hmm ... I didn't recall seeing any on the lists, but a bit of archive
searching found

https://www.postgresql.org/message-id/flat/20170213184323.6099.18278%40wrigleys.postgresql.org

wherein we'd considered the idea and rejected it, or at least decided
that we wanted finer-grained control than the server side needs.
So that's *a* field complaint.  But are we still worried about the
concerns that were raised there?

Re-reading, it looks like the submitter then wanted us to just drop the
prohibition of group-readability without tying it to root ownership,
which I feel would indeed be pretty dangerous given how many systems have
groups like "users".  But I don't think root-owned-group-readable is such
a problem: if you can create such a file then you can make one owned by
the calling user, too.

Anyway, I'd be happier about back-patching if we could document
actual requests to make it work like the server side does.

			regards, tom lane

  14. Re: Allow root ownership of client certificate key

    Chris Bandy <bandy.chris@gmail.com> — 2022-03-02T05:30:25Z

    On 3/1/22 3:15 AM, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> I'd be more eager to do that if we had some field complaints
>>> about it.  Since we don't, my inclination is not to, but I'm
>>> only -0.1 or so; anybody else want to vote?
> 
>> This patch was specifically developed in response to field complaints
>> about it working differently, so there's that.
> 
> Anyway, I'd be happier about back-patching if we could document
> actual requests to make it work like the server side does.
> 

This patch is tidy and addresses an incompatibility with Kubernetes, so
+1 from me for a back-patch.


PGO runs PostgreSQL 10 through 14 in Kubernetes, and we have to work
around this issue when using certificates for system accounts.

For example, we use certificates to encrypt and authenticate streaming
replication connections. We store certificates in the Kubernetes API as
Secrets.[1] Kubernetes then hands those certificates/secrets to a
running container by mounting them as files on the filesystem.

Those files and their directories are managed by Kubernetes (as root)
from outside the container, and processes inside the container (as
not-root) cannot change them. They are mounted with these permissions:

  drwxrwsrwt  root postgres  /pgconf/tls
  -rw-r-----  root postgres  /pgconf/tls/ca.crt
  -rw-r-----  root postgres  /pgconf/tls/tls.crt
  -rw-r-----  root postgres  /pgconf/tls/tls.key

  drwxr-sr-x  root postgres  /pgconf/tls/replication
  -rw-r-----  root postgres  /pgconf/tls/replication/ca.crt
  -rw-r-----  root postgres  /pgconf/tls/replication/tls.crt
  -rw-r-----  root postgres  /pgconf/tls/replication/tls.key

Kubernetes treats the server certificate (top) with the same ownership
and permissions as the client certificate for the replication user
(bottom). The server is happy but anything libpq, including walreceiver,
rejects the latter files for not being "u=rw (0600) or less".


There is an open request in the Kubernetes project to provide more
control over ownership and permissions of mounted secrets.[2] PostgreSQL
is mentioned repeatedly as motivation for the feature.


[1]: https://kubernetes.io/docs/concepts/configuration/secret/
[2]: https://issue.kubernetes.io/81089

-- Chris

  15. Re: Allow root ownership of client certificate key

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-02T14:40:10Z

    Chris Bandy <bandy.chris@gmail.com> writes:
> On 3/1/22 3:15 AM, Tom Lane wrote:
>> Anyway, I'd be happier about back-patching if we could document
>> actual requests to make it work like the server side does.

> PGO runs PostgreSQL 10 through 14 in Kubernetes, and we have to work
> around this issue when using certificates for system accounts.

Sold then, I'll make it so in a bit.

			regards, tom lane

  16. Re: Allow root ownership of client certificate key

    David Steele <david@pgmasters.net> — 2022-03-02T17:17:34Z

    On 3/2/22 08:40, Tom Lane wrote:
> Chris Bandy <bandy.chris@gmail.com> writes:
>> On 3/1/22 3:15 AM, Tom Lane wrote:
>>> Anyway, I'd be happier about back-patching if we could document
>>> actual requests to make it work like the server side does.
> 
>> PGO runs PostgreSQL 10 through 14 in Kubernetes, and we have to work
>> around this issue when using certificates for system accounts.
> 
> Sold then, I'll make it so in a bit.

Thank you! I think the containers community is really going to 
appreciate this.

Regards,
-David