From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Steele <david@pgmasters.net>
cc: Stephen Frost <sfrost@snowman.net>,
        PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Allow root ownership of client certificate key
In-reply-to: <716ece6d-7500-8665-05f0-488e75c5525d@pgmasters.net>
References: <f4b7bc55-97ac-9e69-7398-335e212f7743@pgmasters.net> <20211108190405.GL20998@tamriel.snowman.net> <716ece6d-7500-8665-05f0-488e75c5525d@pgmasters.net>
Comments: In-reply-to David Steele <david@pgmasters.net>
	message dated "Mon, 08 Nov 2021 17:36:39 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <377057.1642538462.1@sss.pgh.pa.us>
Date: Tue, 18 Jan 2022 15:41:02 -0500
Message-ID: <377058.1642538462@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/377058.1642538462%40sss.pgh.pa.us>
Precedence: bulk

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