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

Поиск
Список
Период
Сортировка
От David Fetter
Тема Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full
Дата
Msg-id 20180803060919.GA29092@fetter.org
обсуждение исходный текст
Ответ на Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full  (Julian Markwort <julian.markwort@uni-muenster.de>)
Ответы Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full
Список pgsql-hackers
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

Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Amit Langote
Дата:
Сообщение: Re: Speeding up INSERTs and UPDATEs to partitioned tables
Следующее
От: Ashutosh Bapat
Дата:
Сообщение: Re: Problem while updating a foreign table pointing to a partitionedtable on foreign server