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
