Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full - Mailing list pgsql-hackers
| From | Magnus Hagander |
|---|---|
| Subject | Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full |
| Date | |
| Msg-id | CABUevEyZBVzgD+H=ocY=uRx=FHBpwckAOqJsjxR5Wqjx23O20w@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full (Julian Markwort <julian.markwort@uni-muenster.de>) |
| Responses |
Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full
|
| List | pgsql-hackers |
On Fri, Mar 23, 2018 at 3:45 PM, Julian Markwort <julian.markwort@uni-muenster.de> wrote:
On Sat, 2018-03-17 at 18:24 +0100, Magnus Hagander wrote:The error message "certificate authentication failed for user XYZ:
client certificate contains no user name" is the result of calling
CheckCertAuth when the user presented a certificate without a CN in it.That is arguably wrong, since it's actually password authentication that fails. That is the authentication type that was picked in pg_hba.conf. It's more "certificate validation" that failed.I think we got confused about this; maybe I didn't graps it fully before: CheckCertAuth is currently only called when auth method cert is used. So it actually makes sense to say that certificate authentication failed, I think.I agree that the log message is useful. Though it could be good to clearly indicate that it was caused specifically because of the verify-full, to differentiate it from other cases of the same message.I've modified my patch so it still uses CheckCertAuth, but now a different message is written to the log when clientcert=verify-full was used.For auth method cert, the function should behave as before.For example, what about the scenario where I use GSSAPI authentication and clientcert=verify-full. Then we suddenly have three usernames (gssapi, certificate and specified) -- how is the user supposed to know which one came from the cert and which one came from gssapi for example?The user will only see what's printed in the auth_failed() function in auth.c with the addition of the logdetail string, which I don't touch with this patch.As you said, it makes sense that more detailed information is only available in the server's log.I've attached an updated version of the patch.
I assume this is a patch that's intended to be applied on top of the previous patch? If so, please submit the complete pach to make sure the correct combination ends up actually being reviewed.
I'm not sure if it is preferred to keep patches as short as possible (mostly with respect to the changed lines in the documentation) or to organize changes so that the text matches the surrounding column width und text flow? Also, I've omitted mentions of the current usage 'clientcert=1' - this is still supported in code, but I think telling new users only about 'clientcert=verify-ca' and 'clientcert=verify-full' is clearer. Or am I wrong on this one?
I have not had time to look at the updated verison of the patch yet, but I wanted to get a response in for your last question here.
Keeping patches as short as possible is not a good thing itself. The important part is that the resulting code and documentation is the best possible. Sometimes you might want to turn it into two patches submitted at the same time if one is clearly just reorganisation, but avoiding code restructure just to keep the lines of patch down is not helpful.
pgsql-hackers by date: