Re: Allow root ownership of client certificate key

From: David Steele <david(at)pgmasters(dot)net>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Allow root ownership of client certificate key
Date: 2021-11-08 22:36:39
Message-ID: 716ece6d-7500-8665-05f0-488e75c5525d@pgmasters.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 11/8/21 2:04 PM, Stephen Frost wrote:
> * David Steele (david(at)pgmasters(dot)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(at)pgmasters(dot)net

Attachment Content-Type Size
client-key-perm-002.patch text/plain 3.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jeff Davis 2021-11-08 23:36:21 Extensible Rmgr for Table AMs
Previous Message Daniel Gustafsson 2021-11-08 22:19:33 Re: parallel distinct union and aggregate support patch