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