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