Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full

On 07/19/2018 03:00 AM, Thomas Munro wrote:
> Some more comments:
>
> if (parsedline->auth_method == uaCert)
> {
> - parsedline->clientcert = true;
> + parsedline->clientcert = clientCertOn;
> }
>
> The "cert" method is technically redundant with this patch, because
> it's equivalent to "trust clientcert=verify-full", right? That gave
> me an idea: wouldn't the control flow be a bit nicer if you just
> treated uaCert the same as uaTrust in the big switch statement, but
> also enforced that clientcert = clientCertFull in the hunk above?
> Then you could just have one common code path to reach CheckCertAuth()
> for all auth methods after that switch statement, instead of the more
> complicated conditional coding you have now with two ways to reach it.
> Would that be better?
That would result in a couple less LOC and a bit clearer conditionals, I
agree.
If there are no objections to make uaCert a quasi-alias of uaTrust with
clientcert=verify-full, I'll go ahead and change the code accordingly.
I'll make sure that the error messages are still handled based on the
auth method and not only depending on the clientcert flag.

> In the "auth-cert" section there are already a couple of examples
> using lower case "cn":
>
> will be sent to the client. The <literal>cn</literal> (Common Name)
>
> is a check that the <literal>cn</literal> attribute matches the database
>
> I guess you should do the same, or if there is a good reason to use
> upper case "CN" instead then you should change those to match.
I see that there are currently no places that use <literal>CN</literal> 
right now.
However, we refer to Certification Authority as CA in most cases
(without the literal tag surrounding it).
The different fields of certificates seem to be abbreviated with capital
letters in most literature; That was my reasoning for capitalizing it in
the first place.
I'm open for suggestions, but in absence of objections I might just
capitalize all occurrences of CN.

> There is still a place in the documentation where we refer to "1":
>
> In a <filename>pg_hba.conf</filename> record specifying certificate
> authentication, the authentication option <literal>clientcert</literal> is
> assumed to be <literal>1</literal>, and it cannot be turned off
> since a client
> certificate is necessary for this method. What the <literal>cert</literal>
> method adds to the basic <literal>clientcert</literal> certificate
> validity test
> is a check that the <literal>cn</literal> attribute matches the database
> user name.
>
> Maybe we should use "verify-ca" here, though as I noted above I think
> it should really "verify-full" and we should simplify the flow.
Yes, we should adopt the new style in all places.
I'll rewrite that passage to indicate that cert authentication
essentially results in the same behaviour as clientcert=verify-full.
The existing text is somewhat ambiguous anyway, in case somebody decides
to skip over the restriction described in the second sentence.

> I think we should also document that "1" and "0" are older spellings
> still accepted, where the setting names are introduced.
>
> +typedef enum ClientCertMode
> +{
> + clientCertOff,
> + clientCertOn,
> + clientCertFull
> +} ClientCertMode;
> +
+1
> What do you think about using clientCertCA for the enumerator name
> instead of clientCertOn? That would correspond better to the names
> "verify-ca" and "verify-full".
>
+1
I'm not sure if Magnus had any other cases in mind when he named it
clientCertOn?

Should I mark this entry as "Needs review" in the commitfest? It seems
some discussion is still needed to get this commited...

kind regards
Julian