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

[Magnus Hagander]

On Sat, Mar 9, 2019 at 11:03 AM Magnus Hagander  wrote:

> On Sun, Feb 17, 2019 at 7:50 PM Michael Paquier 
> wrote:
>
>> On Fri, Feb 15, 2019 at 08:03:24PM -0800, Andres Freund wrote:
>> > I see you've marked the patch as needs review - but as the patch
>> > previously was marked as ready-for-committer, and I assume nothing
>> > substantial has changed, I think RFC might still be the accurate state?
>>
>> Yes, RFC sounds good to me.  I think that I could look at this patch
>> and get it into shape for PG12 as that's an area I am rather familiar
>> with.  After that it depends on Magnus as he is registered as the
>> committer of the patch.  I'll be fine to jump into the ship if need
>> be.
>>
>
> Sorry, for some reason this entire thread ended up getting tracked in my
> spam folder :/ Yay the new gmail antispam breakage.
>
> I was definitely still planning to work on it!
>
> The first thing I noticed is that it needed  yet another rebase because of
> the changes in the SSL tests. PFA an updated patch (very trivial rebase).
>
> (I'll keep working on it, just wanted to throw the rebased patch out there)
>
>
Patched pushed, with some very minor formatting changes and a pgindent run.

Apologies for the long processing time, and thanks again for the patch!

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Magnus Hagander]

On Sun, Feb 17, 2019 at 7:50 PM Michael Paquier  wrote:

> On Fri, Feb 15, 2019 at 08:03:24PM -0800, Andres Freund wrote:
> > I see you've marked the patch as needs review - but as the patch
> > previously was marked as ready-for-committer, and I assume nothing
> > substantial has changed, I think RFC might still be the accurate state?
>
> Yes, RFC sounds good to me.  I think that I could look at this patch
> and get it into shape for PG12 as that's an area I am rather familiar
> with.  After that it depends on Magnus as he is registered as the
> committer of the patch.  I'll be fine to jump into the ship if need
> be.
>

Sorry, for some reason this entire thread ended up getting tracked in my
spam folder :/ Yay the new gmail antispam breakage.

I was definitely still planning to work on it!

The first thing I noticed is that it needed  yet another rebase because of
the changes in the SSL tests. PFA an updated patch (very trivial rebase).

(I'll keep working on it, just wanted to throw the rebased patch out there)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c2114021c3..411f1e1679 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   cn (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behavior is similar to the cert authentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
@@ -1865,11 +1872,11 @@ host ... ldap ldapserver=ldap.example.net ldapbasedn="dc=example, dc=net" ldapse

 In a pg_hba.conf record specifying certificate
 authentication, the authentication option clientcert is
-assumed to be 1, and it cannot be turned off since a client
-certificate is necessary for this method.  What the cert
-method adds to the basic clientcert certificate validity test
-is a check that the cn attribute matches the database
-user name.
+assumed to be verify-ca or verify-full,
+and it cannot be turned off since a client certificate is necessary for this
+method. What the cert method adds to the basic
+clientcert certificate validity test is a check that the
+cn attribute matches the database user name.

   
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 7de26e98ad..d786ebfb71 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2316,13 +2316,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or its mapping
+   matches the cn (Common Name) of the provided certificate.
+   Note that certificate chain validation is always ensured when the
+   cert authentication method is used
+   (see ).
   
 
   
@@ -2341,18 +2353,34 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
The clientcert authentication option is available for
all authentication methods, but only in pg_hba.conf lines
specified as hostssl.  When clientcert is
-   not specified or is set to 0, the server will still verify any presented
-   client certificates against its CA file, if one is con

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

[Michael Paquier]

On Fri, Feb 15, 2019 at 08:03:24PM -0800, Andres Freund wrote:
> I see you've marked the patch as needs review - but as the patch
> previously was marked as ready-for-committer, and I assume nothing
> substantial has changed, I think RFC might still be the accurate state?

Yes, RFC sounds good to me.  I think that I could look at this patch
and get it into shape for PG12 as that's an area I am rather familiar
with.  After that it depends on Magnus as he is registered as the
committer of the patch.  I'll be fine to jump into the ship if need
be.
--
Michael


signature.asc
Description: PGP signature

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

[Andres Freund]

Hi,

On 2019-02-06 15:19:56 +, Timmer, Marius wrote:
> On Mon, Jan 04, 2019 at 03:06, Michael Paquier wrote:
> > I have moved the patch to next CF, waiting on author as the latest
> > patch does not apply.  Could it be rebased?

> The patch is rebased and applies now.

I see you've marked the patch as needs review - but as the patch
previously was marked as ready-for-committer, and I assume nothing
substantial has changed, I think RFC might still be the accurate state?

- Andres

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

[Timmer, Marius]

On Mon, Jan 04, 2019 at 03:06, Michael Paquier wrote:
> On Thu, Dec 27, 2018 at 12:14:03PM +0100, Magnus Hagander wrote:
>> I definitely am. In fact, I was ages ago (was planning for early December,
>> but hey, see wher that let me), so my apologies for failing at that. But it
>> definitely remains on my list of things to get to!
> 
> So, Magnus, where are we on this?
> 
> I have moved the patch to next CF, waiting on author as the latest
> patch does not apply.  Could it be rebased?
The patch is rebased and applies now.


Kind regards,

Marius




-- 
Westfälische Wilhelms-Universität Münster (WWU)
Zentrum für Informationsverarbeitung (ZIV)
Röntgenstraße 7-13
Besucheradresse: Einsteinstraße 60 - Raum 101
48149 Münster
+49 251 83 31158
marius.tim...@uni-muenster.de
https://www.uni-muenster.de/ZIV
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c2114021c3..411f1e1679 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   cn (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behavior is similar to the cert authentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
@@ -1865,11 +1872,11 @@ host ... ldap ldapserver=ldap.example.net ldapbasedn="dc=example, dc=net" ldapse

 In a pg_hba.conf record specifying certificate
 authentication, the authentication option clientcert is
-assumed to be 1, and it cannot be turned off since a client
-certificate is necessary for this method.  What the cert
-method adds to the basic clientcert certificate validity test
-is a check that the cn attribute matches the database
-user name.
+assumed to be verify-ca or verify-full,
+and it cannot be turned off since a client certificate is necessary for this
+method. What the cert method adds to the basic
+clientcert certificate validity test is a check that the
+cn attribute matches the database user name.

   
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 1f78f6c956..36a45f7f0c 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2316,13 +2316,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or its mapping
+   matches the cn (Common Name) of the provided certificate.
+   Note that certificate chain validation is always ensured when the
+   cert authentication method is used
+   (see ).
   
 
   
@@ -2341,18 +2353,34 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
The clientcert authentication option is available for
all authentication methods, but only in pg_hba.conf lines
specified as hostssl.  When clientcert is
-   not specified or is set to 0, the server will still verify any presented
-   client certificates against its CA file, if one is configured — but
-   it will not insist that a client certificate be presented.
+   not specified or is set to no-verify, the server will still
+   verify any presented client certificates against its CA file, if one is
+   configured — but it will not insist that a client certificate be presented.
+  
+
+  
+   There are two approaches to enforce that users provide a certif

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

[Michael Paquier]

On Thu, Dec 27, 2018 at 12:14:03PM +0100, Magnus Hagander wrote:
> I definitely am. In fact, I was ages ago (was planning for early December,
> but hey, see wher that let me), so my apologies for failing at that. But it
> definitely remains on my list of things to get to!

So, Magnus, where are we on this?

I have moved the patch to next CF, waiting on author as the latest
patch does not apply.  Could it be rebased?
--
Michael


signature.asc
Description: PGP signature

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

[Magnus Hagander]

On Tue, Dec 25, 2018 at 9:08 AM Michael Paquier  wrote:

> On Fri, Nov 30, 2018 at 12:24:04PM +1300, Thomas Munro wrote:
> > The tests pass and show the feature working correctly.  I think this
> > is getting close to committable.  I see that Magnus has signed up as
> > committer.
>
> It has been one month since this message, and the patch is marked as
> ready for committer.  Magnus, are you planning to look at it?
>

Hi!

I definitely am. In fact, I was ages ago (was planning for early December,
but hey, see wher that let me), so my apologies for failing at that. But it
definitely remains on my list of things to get to!

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Michael Paquier]

On Fri, Nov 30, 2018 at 12:24:04PM +1300, Thomas Munro wrote:
> The tests pass and show the feature working correctly.  I think this
> is getting close to committable.  I see that Magnus has signed up as
> committer.

It has been one month since this message, and the patch is marked as
ready for committer.  Magnus, are you planning to look at it?
--
Michael


signature.asc
Description: PGP signature

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

[Marius Timmer]

Hello Thomas,

thank you for reviewing our patch.

> Why did you put "trust" there instead of "$authmethod" like the previous 
> lines?
That is a good question in deed. We changed that accordingly.

> The tests pass and show the feature working correctly.  I think this
> is getting close to committable. 

We implemented all other suggestions in the attached patch version...

> I see that Magnus has signed up a committer.
... and changed status to "Ready for Committer" :-).


Kind regards,

Marius Timmer




-- 
Westfälische Wilhelms-Universität Münster (WWU)
Zentrum für Informationsverarbeitung (ZIV)
Röntgenstraße 7-13
Besucheradresse: Einsteinstraße 60 - Raum 101
48149 Münster
+49 251 83 31158
marius.tim...@uni-muenster.de
https://www.uni-muenster.de/ZIV
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c2114021c3..411f1e1679 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   cn (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behavior is similar to the cert authentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
@@ -1865,11 +1872,11 @@ host ... ldap ldapserver=ldap.example.net ldapbasedn="dc=example, dc=net" ldapse

 In a pg_hba.conf record specifying certificate
 authentication, the authentication option clientcert is
-assumed to be 1, and it cannot be turned off since a client
-certificate is necessary for this method.  What the cert
-method adds to the basic clientcert certificate validity test
-is a check that the cn attribute matches the database
-user name.
+assumed to be verify-ca or verify-full,
+and it cannot be turned off since a client certificate is necessary for this
+method. What the cert method adds to the basic
+clientcert certificate validity test is a check that the
+cn attribute matches the database user name.

   
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 8d9d40664b..ef515535a8 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2287,13 +2287,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or its mapping
+   matches the cn (Common Name) of the provided certificate.
+   Note that certificate chain validation is always ensured when the
+   cert authentication method is used
+   (see ).
   
 
   
@@ -2312,18 +2324,34 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
The clientcert authentication option is available for
all authentication methods, but only in pg_hba.conf lines
specified as hostssl.  When clientcert is
-   not specified or is set to 0, the server will still verify any presented
-   client certificates against its CA file, if one is configured — but
-   it will not insist that a client certificate be presented.
+   not specified or is set to no-verify, the server will still
+   verify any presented client certificates against its CA file, if one is
+   configured — but it will not insist that a client certificate be presented.
+  
+
+  
+   There are two approaches to enforce that users provide a certificate during login.
+  
+
+  
+   The firs

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

[Thomas Munro]

On Fri, Oct 26, 2018 at 2:08 AM Marius Timmer
 wrote:
> We (Julian and I) would like to show you the seventh version of this
> patch which includes all the things mentioned before. Unfortunately
> we did not find the time to do this earlier.

+case uaCert:
 case uaTrust:

Maybe add a note there that this will be treated as if
clientcert=verify-full below?

+else if(strcmp(val, "2") == 0

The "1" is needed for backwards compatibility, but is there any need
for "2" as an alternative for "verify-full"?

+# Check that connecting with auth-optionverify-full in pg_hba :

Missing space.

+  "hostssl verifydbyetanotheruser  $serverhost/32
 trust  clientcert=verify-ca\n";

Why did you put "trust" there instead of "$authmethod" like the previous lines?

The tests pass and show the feature working correctly.  I think this
is getting close to committable.  I see that Magnus has signed up as
committer.

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

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

[Arne Scheffer]

Hi,

after talking with Marius:
The last sentence in his mail concerning the progress
suffers from poor translation, and can safely be ignored ;-)

We didn't intend to push anybody.

VlG-(Marius Timmer &) Arne Scheffer


On 25.10.18 15:08, Marius Timmer wrote:

Dear hackers,

We (Julian and I) would like to show you the seventh version of this
patch which includes all the things mentioned before. Unfortunately
we did not find the time to do this earlier.


On 07/19/2018 03:00 AM, Thomas Munro wrote:

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.

There is only one path now to call CheckCertAuth(). I don't think we
have left too many complicated conditions.



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.

uaCert and uaTrust are handled the same within the switch statement.



I'll make sure that the error messages are still handled based on
the auth method and not only depending on the clientcert flag.

As far as I know we already handled the error message based on the auth
method and clientcert flag.


On 07/30/2018 12:20, Julian Markwort wrote:

I'm open for suggestions, but in absence of objections I might just
capitalize all occurrences of CN.

We decided to stick with the old style for now. So we changed all
occurrences of cn to lower case.



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

We fixed that. Additionally we added the alias "no-verify" for
clientcert=0 since it seems to be a good idea to have aliases for all
three available values.



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?

We agree that clientCertCA is a better name for it. Since Magnus does
not seem to have any concerns about it we changed that as well.

Julian and I think the time has come for this patch to make some
progress. After a few months I think there is not that much to discuss
anymore.


Kind regards,

Marius Timmer







--
Arne Scheffer
Webanwendungen
Beratung und Service (mit R. Mersch)

Westfälische Wilhelms-Universität Münster (WWU)

Zentrum für Informationsverarbeitung (ZIV)

Röntgenstraße 7-13
Besucheradresse: Einsteinstraße 60, Raum 104
48149 Münster
+49 251 83 31581
arne.schef...@uni-muenster.de
https://www.uni-muenster.de/ZIV



smime.p7s
Description: S/MIME Cryptographic Signature

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

[Julian Markwort]

On 03.08.2018 at 08:09 David Fetter wrote:


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

Best,
David.

Much appreciated!

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

[David Fetter]

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 cn (Common Name)
> >
> > is a check that the cn 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 CN 
> 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 pg_hba.conf record specifying certificate
> > authentication, the authentication option clientcert 
> > is
> > assumed to be 1, and it cannot be turned off
> >since a client
> > certificate is necessary for this method.  What the 
> > cert
> > method adds to the basic clientcert certificate
> >validity test
> > is a check that the cn 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  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 4dc30c3af69ced946b2ceab8c6b8e003d3583a00 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Thu, 2 Aug 2018 23:07:29 -0700
Subject: [PATCH] clientcert_verify_full v06
To: pgsql-hack...@postgresql.org

---
 doc/src/sgml/client-auth.sgml  | 15 +++---
 doc/src/sgml/runtime.sgml  | 51 +++---
 src/backend/libpq/auth.c   | 39 --
 src/backend/libpq/hba.c| 28 ++-
 src/include/libpq/hba.h|  9 +-
 src/test/ssl/ServerSetup.pm| 10 ++-
 src/test/ssl/t/001_ssltests.pl | 21 ++
 7 files changed, 147 insertions(+), 26 deletions(-)
 mode change 100644 => 100755 src/test/ssl/t/001_ssltests.pl

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c2114021c3..6b261aea92 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
   

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

[Julian Markwort]

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 cn (Common Name)

 is a check that the cn 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 CN  
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 pg_hba.conf record specifying certificate
 authentication, the authentication option clientcert is
 assumed to be 1, and it cannot be turned off
since a client
 certificate is necessary for this method.  What the cert
 method adds to the basic clientcert certificate
validity test
 is a check that the cn 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

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

[Thomas Munro]

On Sun, Jul 15, 2018 at 12:47 AM, Julian Markwort
 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; How close 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 cn (Common Name)

is a check that the cn 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 pg_hba.conf record specifying certificate
authentication, the authentication option clientcert is
assumed to be 1, and it cannot be turned off
since a client
certificate is necessary for this method.  What the cert
method adds to the basic clientcert certificate
validity test
is a check that the cn 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

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

[Julian Markwort]

Hi Thomas,

here's a rebased patch, with your observations corrected.

Thomas Munro wrote on 2018-07-13:
> +   In this case, the CN (nommon name) provided in
> "common name"
> +   CN (Common Name) in the certificate matches
> "common"? (why a capital letter here?)

I've resorted to "CN (Common Name)" on all occurences in 
this patch now.

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; How close should one adhere to that 
limit nowadays?


> This line isn't modified by your patch, but I saw it while in
> proof-reading mode:
>   *err_msg = "clientcert can not be set to 0 when using \"cert\"
> authentication";
> I think "can not" is usually written "cannot"?

I'm not sure about can not, cannot, can't... There are 56, respectively 12697, 
and 2024 occurrences in master right now.
We could touch those lines now and change them to the more common cannot, or we 
can leave it as is...


> 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?

kind regards
Julian
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 656d5f94..40dc0ef7 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   CN (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behavior is similar to the cert authentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 1c92e7df..afdcc729 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2287,13 +2287,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with 
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or its mapping
+   matches the CN (Common Name) of the provided certificate.
+   Note that certificate chain validation is always ensured when the
+   cert authentication method is used
+   (see ).
   
 
   
@@ -2317,14 +2329,33 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
client certificates against its CA file, if one is configured — but
it will not insist that a client certificate be presented.
   
+  
 
+  
+   Enforcing Client Certificates
   
-   If you are setting up client certificates, you may wish to use
-   the cert authentication method, so that the certificates
-   control user authentication as well as providing connection security.
-   See  for details.  (It is not necessary to
-   specify clientcert=1 explicitly when using
-   the cert authentication method.)
+   There are two approaches to enforce that users provide a certificate durin

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

[Thomas Munro]

On Sat, Apr 14, 2018 at 3:48 AM, Julian Markwort
 wrote:
> [a patch]

Hello Julian,

Could you please post a rebased patch?

I haven't reviewed or tested any code yet, but here's some proof-reading:

+   This behaviour is similar to the cert autentication method

"behavior" (our manual is written in en_US, "cd doc/src/sgml ; git
grep behavior | wc -l" -> 895, "git grep behaviour" -> 0).

cert

"authentication"

+   chain, but it will also check whether the username or it's
+   mapping match the common name (CN) of the provided certificate.

"its"

"matches"

+   Note that certificate chain validation  is always ensured when
+   cert authentication method is used

extra space

when *the* ...

+   In this case, the CN (nommon name) provided in

"common name"

+   CN (Common Name) in the certificate matches

"common"? (why a capital letter here?)

This line isn't modified by your patch, but I saw it while in
proof-reading mode:

  *err_msg = "clientcert can not be set to 0 when using \"cert\"
authentication";

I think "can not" is usually written "cannot"?

> slightly offtopic opinion:
> While creating the test cases, I stumbled upon the problem of missing
> depencies to run the tests...
> It's complicated enough that the binaries used by these perl tests are not
> named similar to the packages which provide them (the 'prove' binary is
> supplied by 'Test-Harness'), so maybe in the interest of providing a lower
> entry-barrier to running these tests, we could give a more detailed error
> message in the configure script, when using --enable-tap-tests ?

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

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

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

[Julian Markwort]

On Tue, 2018-04-10 at 18:35 +0200, Magnus Hagander wrote:
> As Peter mentionde, there are in src/test/ssl. I forgot about those,
> but yes, it would be useful to have that.
I've added three tests:
- verify-full specified, CN and username match -- should connect ok
- verify-full specified, CN and username do not match -- should fail
- verify-ca specified, CN and username do not match -- should connect
ok (This is current behaviour)
> That depends on your authenticaiton method. Specifically for
> passwords, what happens is there are actually two separate
> connections -- the first one with no password supplied, and the
> second one with a password in it.
Makes sense.
> We could directly reject the connection after the first one and not
> ask for a password. I'm not sure if that's better though -- that
> would leak the information on *why* we rejected the connection.
This wouldn't be desirable, I think...
Most applications will probably supply the password in the connection
string anyway, so there would be only one connection, right?

> It might definitely be worth shorting it yeah. For one, we can just
> use "cn" :) 
I've shortened the clientcert=verify-full specific error-message to
say:
"certificate validation (clientcert=verify-full) failed for
user \"%s\": CN mismatch"


slightly offtopic opinion:
While creating the test cases, I stumbled upon the problem of missing
depencies to run the tests...
It's complicated enough that the binaries used by these perl tests are
not named similar to the packages which provide them (the 'prove'
binary is supplied by 'Test-Harness'), so maybe in the interest of
providing a lower entry-barrier to running these tests, we could give a
more detailed error message in the configure script, when using --
enable-tap-tests ?


Juliandiff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 53832d0..5ee8cbe 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   CN (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behaviour is similar to the cert autentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 330e38a..6502df3 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2287,13 +2287,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with 
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or it's
+   mapping match the common name (CN) of the provided certificate.
+   Note that certificate chain validation  is always ensured when
+   cert authentication method is used
+   (see ).
   
 
   
@@ -2317,14 +2329,33 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
client certificates against its CA file, if one is configured — but
it will not insist that a client certificate be presented.
   
+  
 
+  
+   Enforcing Client Certificates
   
-   If you are setting up client certificates, you may wish to use
-   the cert authentication method, so that the certificates
-   control user authentication as well as providing connection security.
-   See  for details.  (It is not necessary to
-   specify clientcert=1 explicitly when using
-   the ce

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

[Magnus Hagander]

On Tue, Apr 10, 2018 at 2:10 PM, Julian Markwort <
julian.markw...@uni-muenster.de> wrote:

> On Fri, 2018-04-06 at 20:31 +0200, Magnus Hagander wrote:
>
> I've been through this one again.
>
> Thanks for taking the time!
>
> There is one big omission from it -- it fails to work with the view
> pg_hba_file_rules. When fixing that, things started to look kind of ugly
> with the "two booleans to indicate one thing", so I went ahead and changed
> it to instead be an enum of 3 values.
>
> Oh, I missed that view. To be honest, it wasn't even on my mind that there
> could be a view depending on pg_hba
>
> Also, now when using verify-full or verify-ca, I figured its a lot easier
> to misspell the value, and we don't want that to mean "off". Instead, I
> made it give an error in this case instead of silently treating it as 0.
>
> Good catch!
>
> The option "2" for clientcert was never actually documented, and I think
> we should get rid of that. We need to keep "1" for backwards compatibility
> (which arguably was a bad choice originally, but that was many years ago),
> but let's not add another one.
> I also made a couple of very small changes to the docs.
>
> Attached is an updated patch with these changes. I'd appreciate it if you
> can run it through your tests to confirm that it didn't break any of those
> usecases.
>
> I've tested a couple of things with this and it seems to work as expected.
> Unforunately, there are no tests for libpq, afaik. But testing such
> features would become complicated quite quickly, with the need to generate
> certificates and such...
>

As Peter mentionde, there are in src/test/ssl. I forgot about those, but
yes, it would be useful to have that.



I've noticed that the error message for mismatching CN is now printed twice
> when using password prompts, as all authentication details are checked upon
> initiation of a connection and again later, after sending the password.
>

That depends on your authenticaiton method. Specifically for passwords,
what happens is there are actually two separate connections -- the first
one with no password supplied, and the second one with a password in it.

We could directly reject the connection after the first one and not ask for
a password. I'm not sure if that's better though -- that would leak the
information on *why* we rejected the connection.


2018-04-10 13:54:19.882 CEST [8735] LOG:  provided user name (testuser) and
> authenticated user name (nottestuser) do not match
> 2018-04-10 13:54:19.882 CEST [8735] LOG:  certificate validation failed
> for user "testuser": common name in client certificate does not match user
> name or mapping, but clientcert=verify-full is enabled.
> 2018-04-10 13:54:21.515 CEST [8736] LOG:  provided user name (testuser)
> and authenticated user name (nottestuser) do not match
> 2018-04-10 13:54:21.515 CEST [8736] LOG:  certificate validation failed
> for user "testuser": common name in client certificate does not match user
> name or mapping, but clientcert=verify-full is enabled.
> 2018-04-10 13:54:21.515 CEST [8736] FATAL:  password authentication failed
> for user "testuser"
> 2018-04-10 13:54:21.515 CEST [8736] DETAIL:  Connection matched
> pg_hba.conf line 94: "hostssl all testuser 127.0.0.1/32 password
> clientcert=verify-full"
>
>
> Also, as you can see, there are two LOG messages indicating the mismatch
> -- "provided user name (testuser) and authenticated user name (nottestuser)
> do not match" comes from hba.c:check_usermap() and "certificate validation
> failed for user "testuser": common name in client certificate does not
> match user name or mapping, but clientcert=verify-full is enabled." is the
> message added in auth.c:CheckCertAuth() by the patch.
> Maybe we should shorten the latter LOG message?
>


It might definitely be worth shorting it yeah. For one, we can just use
"cn" :)


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Peter Eisentraut]

On 4/10/18 08:10, Julian Markwort wrote:
>> Attached is an updated patch with these changes. I'd appreciate it if
>> you can run it through your tests to confirm that it didn't break any
>> of those usecases.
> I've tested a couple of things with this and it seems to work as
> expected. Unforunately, there are no tests for libpq, afaik. But testing
> such features would become complicated quite quickly, with the need to
> generate certificates and such...

There are tests in src/test/ssl/ that would probably be a good fit to
extend for this.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

[Julian Markwort]

On Fri, 2018-04-06 at 20:31 +0200, Magnus Hagander wrote:
> I've been through this one again.
Thanks for taking the time!

> There is one big omission from it -- it fails to work with the view
> pg_hba_file_rules. When fixing that, things started to look kind of
> ugly with the "two booleans to indicate one thing", so I went ahead
> and changed it to instead be an enum of 3 values.
Oh, I missed that view. To be honest, it wasn't even on my mind that
there could be a view depending on pg_hba

> Also, now when using verify-full or verify-ca, I figured its a lot
> easier to misspell the value, and we don't want that to mean "off".
> Instead, I made it give an error in this case instead of silently
> treating it as 0.
Good catch!

> The option "2" for clientcert was never actually documented, and I
> think we should get rid of that. We need to keep "1" for backwards
> compatibility (which arguably was a bad choice originally, but that
> was many years ago), but let's not add another one. 
> I also made a couple of very small changes to the docs.
> 
> Attached is an updated patch with these changes. I'd appreciate it if
> you can run it through your tests to confirm that it didn't break any
> of those usecases.
I've tested a couple of things with this and it seems to work as
expected. Unforunately, there are no tests for libpq, afaik. But
testing such features would become complicated quite quickly, with the
need to generate certificates and such...

I've noticed that the error message for mismatching CN is now printed
twice when using password prompts, as all authentication details are
checked upon initiation of a connection and again later, after sending
the password.

> 2018-04-10 13:54:19.882 CEST [8735] LOG:  provided user name
> (testuser) and authenticated user name (nottestuser) do not match
> 2018-04-10 13:54:19.882 CEST [8735] LOG:  certificate validation
> failed for user "testuser": common name in client certificate does
> not match user name or mapping, but clientcert=verify-full is
> enabled.
> 2018-04-10 13:54:21.515 CEST [8736] LOG:  provided user name
> (testuser) and authenticated user name (nottestuser) do not match
> 2018-04-10 13:54:21.515 CEST [8736] LOG:  certificate validation
> failed for user "testuser": common name in client certificate does
> not match user name or mapping, but clientcert=verify-full is
> enabled.
> 2018-04-10 13:54:21.515 CEST [8736] FATAL:  password authentication
> failed for user "testuser"
> 2018-04-10 13:54:21.515 CEST [8736] DETAIL:  Connection matched
> pg_hba.conf line 94: "hostssl all testuser
> 127.0.0.1/32  password clientcert=verify-full"

Also, as you can see, there are two LOG messages indicating the
mismatch -- "provided user name (testuser) and authenticated user name
(nottestuser) do not match" comes from hba.c:check_usermap() and
"certificate validation failed for user "testuser": common name in
client certificate does not match user name or mapping, but
clientcert=verify-full is enabled." is the message added in
auth.c:CheckCertAuth()  by the patch.
Maybe we should shorten the latter LOG message?

regards
Julian

smime.p7s
Description: S/MIME cryptographic signature

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

[Magnus Hagander]

On Sun, Apr 1, 2018 at 6:07 PM, Magnus Hagander  wrote:

> On Sun, Apr 1, 2018 at 6:01 PM, Julian Markwort <
> julian.markw...@uni-muenster.de> wrote:
>
>> On 1. of April 2018 17:46:38 MESZ wrote Magnus Hagander <
>> mag...@hagander.net>:
>>
>> >I assume this is a patch that's intended to be applied on top of the
>> >previous patch? If so, please submit the complete pach to make sure the
>> >correct combination ends up actually being reviewed.
>>
>> The v02.patch attached to my last mail contains both source and
>> documentation changes.
>>
>
> Hmm. I think I may have been looking at the wrong file. Sorry!
>
>
> >Keeping patches as short as possible is not a good thing itself. The
>> >important part is that the resulting code and documentation is the best
>> >possible. Sometimes you might want to turn it into two patches
>> >submitted at
>> >the same time if one is clearly just reorganisation, but avoiding code
>> >restructure just to keep the lines of patch down is not helpful.
>>
>> I think I've made the right compromises regarding readability of the
>> documentation in my patch then.
>>
>> A happy Easter, passover, or Sunday to you
>>
>
> You, too!
>
> (I shall return to reviewing it after the holidays are over)
>
>
I've been through this one again.

There is one big omission from it -- it fails to work with the view
pg_hba_file_rules. When fixing that, things started to look kind of ugly
with the "two booleans to indicate one thing", so I went ahead and changed
it to instead be an enum of 3 values.

Also, now when using verify-full or verify-ca, I figured its a lot easier
to misspell the value, and we don't want that to mean "off". Instead, I
made it give an error in this case instead of silently treating it as 0.

The option "2" for clientcert was never actually documented, and I think we
should get rid of that. We need to keep "1" for backwards compatibility
(which arguably was a bad choice originally, but that was many years ago),
but let's not add another one.

I also made a couple of very small changes to the docs.

Attached is an updated patch with these changes. I'd appreciate it if you
can run it through your tests to confirm that it didn't break any of those
usecases.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 53832d08e2..5ee8cbe01a 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   CN (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behaviour is similar to the cert autentication method
+   (see  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 587b430527..19af9bd7e0 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2263,13 +2263,25 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with 
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or it's
+   mapping match the common name (CN) of the provided certificate.
+   Note that certificate chain validation  is always ensured when
+   cert authentication method is used

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

[Magnus Hagander]

On Sun, Apr 1, 2018 at 6:01 PM, Julian Markwort <
julian.markw...@uni-muenster.de> wrote:

> On 1. of April 2018 17:46:38 MESZ wrote Magnus Hagander <
> mag...@hagander.net>:
>
> >I assume this is a patch that's intended to be applied on top of the
> >previous patch? If so, please submit the complete pach to make sure the
> >correct combination ends up actually being reviewed.
>
> The v02.patch attached to my last mail contains both source and
> documentation changes.
>

Hmm. I think I may have been looking at the wrong file. Sorry!


>Keeping patches as short as possible is not a good thing itself. The
> >important part is that the resulting code and documentation is the best
> >possible. Sometimes you might want to turn it into two patches
> >submitted at
> >the same time if one is clearly just reorganisation, but avoiding code
> >restructure just to keep the lines of patch down is not helpful.
>
> I think I've made the right compromises regarding readability of the
> documentation in my patch then.
>
> A happy Easter, passover, or Sunday to you
>

You, too!

(I shall return to reviewing it after the holidays are over)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Julian Markwort]

On 1. of April 2018 17:46:38 MESZ wrote Magnus Hagander :

>I assume this is a patch that's intended to be applied on top of the
>previous patch? If so, please submit the complete pach to make sure the
>correct combination ends up actually being reviewed.

The v02.patch attached to my last mail contains both source and documentation 
changes.

>Keeping patches as short as possible is not a good thing itself. The
>important part is that the resulting code and documentation is the best
>possible. Sometimes you might want to turn it into two patches
>submitted at
>the same time if one is clearly just reorganisation, but avoiding code
>restructure just to keep the lines of patch down is not helpful.

I think I've made the right compromises regarding readability of the 
documentation in my patch then.

A happy Easter, passover, or Sunday to you
Julian

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

[Magnus Hagander]

On Fri, Mar 23, 2018 at 3:45 PM, Julian Markwort <
julian.markw...@uni-muenster.de> wrote:

> On Sat, 2018-03-17 at 18:24 +0100, Magnus Hagander wrote:
>
> The error message "certificate authentication failed for user XYZ:
> client certificate contains no user name" is the result of calling
> CheckCertAuth when the user presented a certificate without a CN in it.
>
>
> That is arguably wrong, since it's actually password authentication that
> fails. That is the authentication type that was picked in pg_hba.conf. It's
> more "certificate validation" that failed.
>
>
> I think we got confused about this; maybe I didn't graps it fully before:
> CheckCertAuth is currently only called when auth method cert is used. So it
> actually makes sense to say that certificate authentication failed, I think.
>
> I agree that the log message is useful. Though it could be good to clearly
> indicate that it was caused specifically because of the verify-full, to
> differentiate it from other cases of the same message.
>
> I've modified my patch so it still uses CheckCertAuth, but now a different
> message is written to the log when clientcert=verify-full was used.
> For auth method cert, the function should behave as before.
>
> For example, what about the scenario where I use GSSAPI authentication and
> clientcert=verify-full. Then we suddenly have three usernames (gssapi,
> certificate and specified) -- how is the user supposed to know which one
> came from the cert and which one came from gssapi for example?
>
> The user will only see what's printed in the auth_failed() function in
> auth.c with the addition of the logdetail string, which I don't touch with
> this patch.
> As you said, it makes sense that more detailed information is only
> available in the server's log.
>
> I've attached an updated version of the patch.
>

I assume this is a patch that's intended to be applied on top of the
previous patch? If so, please submit the complete pach to make sure the
correct combination ends up actually being reviewed.



> I'm not sure if it is preferred to keep patches as short as possible
> (mostly with respect to the changed lines in the documentation) or to
> organize changes so that the text matches the surrounding column width und
> text flow? Also, I've omitted mentions of the current usage 'clientcert=1'
> - this is still supported in code, but I think telling new users only about
> 'clientcert=verify-ca' and 'clientcert=verify-full' is clearer. Or am I
> wrong on this one?
>
>
I have not had time to look at the updated verison of the patch yet, but I
wanted to get a response in for your last question here.

Keeping patches as short as possible is not a good thing itself. The
important part is that the resulting code and documentation is the best
possible. Sometimes you might want to turn it into two patches submitted at
the same time if one is clearly just reorganisation, but avoiding code
restructure just to keep the lines of patch down is not helpful.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Julian Markwort]

On Sat, 2018-03-17 at 18:24 +0100, Magnus Hagander wrote:
> > The error message "certificate authentication failed for user XYZ:
> > 
> > client certificate contains no user name" is the result of calling
> > 
> > CheckCertAuth when the user presented a certificate without a CN in
> > it.
> 
> That is arguably wrong, since it's actually password authentication
> that fails. That is the authentication type that was picked in
> pg_hba.conf. It's more "certificate validation" that failed.

I think we got confused about this; maybe I didn't graps it fully
before: CheckCertAuth is currently only called when auth method cert is
used. So it actually makes sense to say that certificate authentication
failed, I think.

> I agree that the log message is useful. Though it could be good to
> clearly indicate that it was caused specifically because of the
> verify-full, to differentiate it from other cases of the same
> message.
I've modified my patch so it still uses CheckCertAuth, but now a
different message is written to the log when clientcert=verify-full was
used.
For auth method cert, the function should behave as before.


> For example, what about the scenario where I use GSSAPI
> authentication and clientcert=verify-full. Then we suddenly have
> three usernames (gssapi, certificate and specified) -- how is the
> user supposed to know which one came from the cert and which one came
> from gssapi for example?
The user will only see what's printed in the auth_failed() function in
auth.c with the addition of the logdetail string, which I don't touch
with this patch.
As you said, it makes sense that more detailed information is only
available in the server's log.

I've attached an updated version of the patch.
I'm not sure if it is preferred to keep patches as short as possible
(mostly with respect to the changed lines in the documentation) or to
organize changes so that the text matches the surrounding column width
und text flow? Also, I've omitted mentions of the current usage
'clientcert=1' - this is still supported in code, but I think telling
new users only about 'clientcert=verify-ca' and 'clientcert=verify-
full' is clearer. Or am I wrong on this one?

Greetings
Juliandiff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 53832d0..11c5961 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -563,10 +563,17 @@ hostnossl  database  user
   
In addition to the method-specific options listed below, there is one
method-independent authentication option clientcert, which
-   can be specified in any hostssl record.  When set
-   to 1, this option requires the client to present a valid
-   (trusted) SSL certificate, in addition to the other requirements of the
-   authentication method.
+   can be specified in any hostssl record.
+   This option can be set to verify-ca or
+   verify-full. Both options require the client
+   to present a valid (trusted) SSL certificate, while
+   verify-full additionally enforces that the
+   CN (Common Name) in the certificate matches
+   the username or an applicable mapping.
+   This behaviour is similar to the cert autentication method
+   ( See  ) but enables pairing
+   the verification of client certificates with any authentication
+   method that supports hostssl entries.
   
  
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 587b430..a3eb180 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2263,13 +2263,24 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(CAs) you trust in a file in the data
directory, set the parameter  in
postgresql.conf to the new file name, and add the
-   authentication option clientcert=1 to the appropriate
+   authentication option clientcert=verify-ca or
+   clientcert=verify-full to the appropriate
hostssl line(s) in pg_hba.conf.
A certificate will then be requested from the client during SSL
connection startup.  (See  for a description
-   of how to set up certificates on the client.)  The server will
-   verify that the client's certificate is signed by one of the trusted
-   certificate authorities.
+   of how to set up certificates on the client.)
+  
+
+  
+   For a hostssl entry with 
+   clientcert=verify-ca, the server will verify
+   that the client's certificate is signed by one of the trusted
+   certificate authorities. If clientcert=verify-full
+   is specified, the server will not only verify the certificate
+   chain, but it will also check whether the username or it's
+   mapping match the common name (CN) of the provided certificate.
+   Note that this is always ensured when cert
+   authentication method is used (See ).
   
 
   
@@ -2293,14 +2304,33 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
client certificates against its CA file, if one is configured — but
it will not insist that a client certif

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

[Magnus Hagander]

On Fri, Mar 9, 2018 at 2:11 PM, Julian Markwort <
julian.markw...@uni-muenster.de> wrote:

> Hello Magnus,
>
> > I think this makes a lot of sense, and can definitely be a useful
> > option.
>
> I was hesistant to write a long and elaborate patch as I wasn't certain
> if there was any interest for such an addition, but I'm thankful for
> your input.
>
> > However, the patch is completely lacking documentation, which
> > obviously make it a no-starter.
>
> I'll write the missing documentation shortly.
>

Great!



> > Also if I read it right, if the CN is not correct, it will give the
> > error message "certificate authentication failed for user ...". I
> > realize this comes from the re-use of the code, but I don't think
> > this makes it very useful. We  need to separate these two things.
>
> The error message "certificate authentication failed for user XYZ:
> client certificate contains no user name" is the result of calling
> CheckCertAuth when the user presented a certificate without a CN in it.
>

That is arguably wrong, since it's actually password authentication that
fails. That is the authentication type that was picked in pg_hba.conf. It's
more "certificate validation" that failed.


The error message that is presented to the user upon trying to connect
> with a certificate containing a CN other than the username is:
>
> -
> psql: FATAL: password authentication failed for user "nottestuser"
> -
>
> The server's log contains the lines:
>
> -
> 2018-03-09 13:06:43.111 CET [3310] LOG:  provided user name
> (nottestuser) and authenticated user name (testuser) do not match
> 2018-03-09 13:06:43.111 CET [3310] FATAL:  password authentication
> failed for user "nottestuser"
> 2018-03-09 13:06:43.111 CET [3310] DETAIL:  Connection matched
> pg_hba.conf line 97: "hostssl all nottestuser 127.0.0.1/32 password
> clientcert=verify-full"
> -
>
> I'd argue that the message in the log file is consistent and useful,
> however the message given by psql (or any libpq application for that
> matter) leaves uncertainty regarding the correctness of a provided
> password, for example.

I could attach the log message of CheckCertAuth to the logdetail,
> however then I'd have issues if there is already something written to
> the logdetail.
> I could also use an additional ereport() call whenever clientcert was
> set to verify-full and the user name didn't match the CN.
>

It's hard to do too much about the client connection one when failing,
without leaking too much. It's pretty common that you have to actually look
in the server logfile to figure out what actually went wrong. I think that
message is fine.

I agree that the log message is useful. Though it could be good to clearly
indicate that it was caused specifically because of the verify-full, to
differentiate it from other cases of the same message.

For example, what about the scenario where I use GSSAPI authentication and
clientcert=verify-full. Then we suddenly have three usernames (gssapi,
certificate and specified) -- how is the user supposed to know which one
came from the cert and which one came from gssapi for example?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

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

[Julian Markwort]

Hello Magnus,

> I think this makes a lot of sense, and can definitely be a useful
> option.

I was hesistant to write a long and elaborate patch as I wasn't certain
if there was any interest for such an addition, but I'm thankful for
your input.

> However, the patch is completely lacking documentation, which
> obviously make it a no-starter. 

I'll write the missing documentation shortly.

> Also if I read it right, if the CN is not correct, it will give the
> error message "certificate authentication failed for user ...". I
> realize this comes from the re-use of the code, but I don't think
> this makes it very useful. We  need to separate these two things.

The error message "certificate authentication failed for user XYZ:
client certificate contains no user name" is the result of calling
CheckCertAuth when the user presented a certificate without a CN in it.

The error message that is presented to the user upon trying to connect
with a certificate containing a CN other than the username is:

-
psql: FATAL: password authentication failed for user "nottestuser"
-

The server's log contains the lines:

-
2018-03-09 13:06:43.111 CET [3310] LOG:  provided user name
(nottestuser) and authenticated user name (testuser) do not match
2018-03-09 13:06:43.111 CET [3310] FATAL:  password authentication
failed for user "nottestuser"
2018-03-09 13:06:43.111 CET [3310] DETAIL:  Connection matched
pg_hba.conf line 97: "hostssl all nottestuser 127.0.0.1/32 password
clientcert=verify-full"
-

I'd argue that the message in the log file is consistent and useful,
however the message given by psql (or any libpq application for that
matter) leaves uncertainty regarding the correctness of a provided
password, for example.
I could attach the log message of CheckCertAuth to the logdetail,
however then I'd have issues if there is already something written to
the logdetail.
I could also use an additional ereport() call whenever clientcert was
set to verify-full and the user name didn't match the CN.

Kind regards
Julian

smime.p7s
Description: S/MIME cryptographic signature

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

[Magnus Hagander]

On Fri, Feb 16, 2018 at 4:45 PM, Julian Markwort <
julian.markw...@uni-muenster.de> wrote:

> Dear Postgresql Hackers,
>
> as of now, pg_hba.conf allows us to enable authentification by
> certificate through the auth-method "cert", in which case the user must
> provide a valid certificate with a certificate common name(CN) matching
> the database user's name or an entry in a pg_ident map.
>
> Additionaly, for every other auth-method it is possible to set the
> auth-option "clientcert=1", so clients must present a valid certificate
> at login. The logic behind this only checks the validity of the
> certificate itself, but the certificate common name(CN) is not
> relevant.
>
> I wrote a very small patch that adds another auth-option:
> - clientcert=verify-full (analogous to server certificates; you could
> also use 2 instead of verify-full for backwards compatibility, or
> verify-ca instead of 1)
> which also checks the certificate common name,
> so all 3 factors get checked:
> 1.) auth-method, e.g. scram or md5 password passes
> 2.) client cert is in truststore
> 3.) CN is correct.
>
> (The patch simply makes use of the function that is used for auth-
> method "cert" to avoid code duplication).
>

I think this makes a lot of sense, and can definitely be a useful option.

However, the patch is completely lacking documentation, which obviously
make it a no-starter.

Also if I read it right, if the CN is not correct, it will give the error
message "certificate authentication failed for user ...". I realize this
comes from the re-use of the code, but I don't think this makes it very
useful. We  need to separate these two things.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/