Re: Allow root ownership of client certificate key

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