Обсуждение: Allow root ownership of client certificate key
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
Вложения
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
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
beu=rw (0600) or less\n"),
> + libpq_gettext("private key file \"%s\" has group or world access; file must have
permissionsu=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
Вложения
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
beu=rw (0600) or less\n"),
>> + libpq_gettext("private key file \"%s\" has group or world access; file must have
permissionsu=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
Вложения
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
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
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
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
Вложения
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
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
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
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
Вложения
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
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
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
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