public inbox for [email protected]
help / color / mirror / Atom feedAllow root ownership of client certificate key
4+ messages / 2 participants
[nested] [flat]
* Allow root ownership of client certificate key
@ 2021-10-22 15:41 David Steele <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: David Steele @ 2021-10-22 15:41 UTC (permalink / raw)
To: PostgreSQL Developers <[email protected]>
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
[email protected]
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.
+ */
+#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
+
+ /*
+ * 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.
+ */
#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;
}
Attachments:
[text/plain] client-key-perm-001.patch (1.8K, ../../[email protected]/2-client-key-perm-001.patch)
download | inline diff:
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.
+ */
+#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
+
+ /*
+ * 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.
+ */
#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;
}
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Allow root ownership of client certificate key
@ 2021-10-28 13:08 David Steele <[email protected]>
parent: David Steele <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: David Steele @ 2021-10-28 13:08 UTC (permalink / raw)
To: PostgreSQL Developers <[email protected]>
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
[email protected]
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Allow root ownership of client certificate key
@ 2021-11-08 19:04 Stephen Frost <[email protected]>
parent: David Steele <[email protected]>
1 sibling, 1 reply; 4+ messages in thread
From: Stephen Frost @ 2021-11-08 19:04 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: PostgreSQL Developers <[email protected]>
Greetings,
* David Steele ([email protected]) 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
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Allow root ownership of client certificate key
@ 2021-11-08 22:36 David Steele <[email protected]>
parent: Stephen Frost <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: David Steele @ 2021-11-08 22:36 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: PostgreSQL Developers <[email protected]>
On 11/8/21 2:04 PM, Stephen Frost wrote:
> * David Steele ([email protected]) 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
[email protected]
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index a212308666..c6e4329653 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -155,6 +155,9 @@ check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
/*
* Refuse to load key files owned by users other than us or root.
*
+ * Note that a similar check is performed in initialize_SSL() so any changes
+ * here may need to be made there as well.
+ *
* XXX surely we can check this on Windows somehow, too.
*/
#if !defined(WIN32) && !defined(__CYGWIN__)
@@ -174,6 +177,9 @@ check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
* allow read access through our gid, or a supplementary gid that allows
* to read system-wide certificates.
*
+ * Note that a similar check is performed in initialize_SSL() so any changes
+ * here may need to be made there as well.
+ *
* 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. (See also the data directory
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index a90d891c6c..d581ff6ca5 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1240,11 +1240,37 @@ initialize_SSL(PGconn *conn)
fnbuf);
return -1;
}
+
+ /*
+ * Refuse to load key files owned by users other than us or root and
+ * require no public access to the 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
+ * us to read system-wide certificates.
+ *
+ * Note that similar checks are performed in
+ * check_ssl_key_file_permissions() so any changes here may need to be
+ * made there as well.
+ *
+ * Ideally we would do similar checks on Windows but it is not clear how
+ * that would work since unix-style permissions may not be available. See
+ * also the data directory permissions checks in checkDataDir().
+ */
#ifndef WIN32
- if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
+ 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;
+ }
+
+ if (!S_ISREG(buf.st_mode) ||
+ (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;
}
Attachments:
[text/plain] client-key-perm-002.patch (3.1K, ../../[email protected]/2-client-key-perm-002.patch)
download | inline diff:
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index a212308666..c6e4329653 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -155,6 +155,9 @@ check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
/*
* Refuse to load key files owned by users other than us or root.
*
+ * Note that a similar check is performed in initialize_SSL() so any changes
+ * here may need to be made there as well.
+ *
* XXX surely we can check this on Windows somehow, too.
*/
#if !defined(WIN32) && !defined(__CYGWIN__)
@@ -174,6 +177,9 @@ check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
* allow read access through our gid, or a supplementary gid that allows
* to read system-wide certificates.
*
+ * Note that a similar check is performed in initialize_SSL() so any changes
+ * here may need to be made there as well.
+ *
* 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. (See also the data directory
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index a90d891c6c..d581ff6ca5 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1240,11 +1240,37 @@ initialize_SSL(PGconn *conn)
fnbuf);
return -1;
}
+
+ /*
+ * Refuse to load key files owned by users other than us or root and
+ * require no public access to the 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
+ * us to read system-wide certificates.
+ *
+ * Note that similar checks are performed in
+ * check_ssl_key_file_permissions() so any changes here may need to be
+ * made there as well.
+ *
+ * Ideally we would do similar checks on Windows but it is not clear how
+ * that would work since unix-style permissions may not be available. See
+ * also the data directory permissions checks in checkDataDir().
+ */
#ifndef WIN32
- if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
+ 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;
+ }
+
+ if (!S_ISREG(buf.st_mode) ||
+ (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;
}
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2021-11-08 22:36 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 15:41 Allow root ownership of client certificate key David Steele <[email protected]>
2021-10-28 13:08 ` David Steele <[email protected]>
2021-11-08 19:04 ` Stephen Frost <[email protected]>
2021-11-08 22:36 ` David Steele <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox