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

On Sun, Jul 15, 2018 at 12:47 AM, Julian Markwort
<julian.markwort@uni-muenster.de> wrote:
> Also, while writing this part of the docs, I tried to stay below 80 characters, but I've exceeded it in some places.
> There are several other places (several in the .sgml files touched by this patch), where 80 characters are exceeded;
Howclose should one adhere to that limit nowadays?
 

Not sure if there is a strict policy, but I usually rewrap with Emacs
M-q unless I'm making a small edit in the middle of an existing
paragraph and want to minimise the diff for clarity.  Here you are
replacing all or most of some paragraphs, so +1 for rewrapping.

>> Yeah.  The packages to install depend on your operating system, and in
>> some cases (macOS, Windows?) which bolt-on package thingamajig you
>> use, though.  Perhaps the READMEs could be improved with details for
>> systems we have reports about (like the recently added "Requirements"
>> section of src/test/ldap/README).
>
> That would be nice, however I could only provide the package names for Fedora right now...
> Would It make sense to add those on their own?
> Or should somebody (maybe myself, when I'm less busy) gather those for most supported systems and commit them as a
whole?

Let's save that for another patch.  I can supply data for a couple of OSes.

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?

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.

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.

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;
+

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".

-- 
Thomas Munro
http://www.enterprisedb.com