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

On Fri, Feb 16, 2018 at 4:45 PM, Julian Markwort <
julian.markwort@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

On Fri, Mar 9, 2018 at 2:11 PM, Julian Markwort <
julian.markwort@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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
Julian

On Fri, Mar 23, 2018 at 3:45 PM, Julian Markwort <
julian.markwort@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On 1. of April 2018 17:46:38 MESZ wrote Magnus Hagander <magnus@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.

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

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

On 1. of April 2018 17:46:38 MESZ wrote Magnus Hagander <
magnus@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Sun, Apr 1, 2018 at 6:07 PM, Magnus Hagander <magnus@hagander.net> wrote:

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

On 1. of April 2018 17:46:38 MESZ wrote Magnus Hagander <
magnus@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

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

On Tue, Apr 10, 2018 at 2:10 PM, Julian Markwort <
julian.markwort@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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 ?

Julian

On Sat, Apr 14, 2018 at 3:48 AM, Julian Markwort
<julian.markwort@uni-muenster.de> 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).

<literal>cert</literal>

"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
+   <literal>cert</literal> authentication method is used

extra space

when *the* ...

+ In this case, the <literal>CN</literal> (nommon name) provided in

"common name"

+ <literal>CN</literal> (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

Hi Thomas,

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

Thomas Munro wrote on 2018-07-13:

+   In this case, the <literal>CN</literal> (nommon name) provided in
"common name"
+   <literal>CN</literal> (Common Name) in the certificate matches
"common"? (why a capital letter here?)

I've resorted to "<literal>CN</literal> (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

On Sun, Jul 15, 2018 at 12:47 AM, Julian Markwort
<julian.markwort@uni-muenster.de> wrote:

Also, while writing this part of the docs, I tried to stay below 80 characters, but I've exceeded it in some places.
There are several other places (several in the .sgml files touched by this patch), where 80 characters are exceeded; 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 <literal>cn</literal> (Common Name)

is a check that the <literal>cn</literal> attribute matches the database

I guess you should do the same, or if there is a good reason to use
upper case "CN" instead then you should change those to match.

There is still a place in the documentation where we refer to "1":

In a <filename>pg_hba.conf</filename> record specifying certificate
authentication, the authentication option <literal>clientcert</literal> is
assumed to be <literal>1</literal>, and it cannot be turned off
since a client
certificate is necessary for this method. What the <literal>cert</literal>
method adds to the basic <literal>clientcert</literal> certificate
validity test
is a check that the <literal>cn</literal> attribute matches the database
user name.

Maybe we should use "verify-ca" here, though as I noted above I think
it should really "verify-full" and we should simplify the flow.

I think we should also document that "1" and "0" are older spellings
still accepted, where the setting names are introduced.

+typedef enum ClientCertMode
+{
+       clientCertOff,
+       clientCertOn,
+       clientCertFull
+} ClientCertMode;
+

What do you think about using clientCertCA for the enumerator name
instead of clientCertOn? That would correspond better to the names
"verify-ca" and "verify-full".

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

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

kind regards
Julian

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

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!

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

--
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.timmer@uni-muenster.de
https://www.uni-muenster.de/ZIV