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

On Mon, Jul 30, 2018 at 02:20:43PM +0200, Julian Markwort wrote:
> 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...

I've rebased the patch atop master so it applies and passes 'make
check-world'. I didn't make any other changes.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate