Thread: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full
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). Have a nice weekend, Julian Markwort
Attachment
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.
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
Attachment
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?
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
Attachment
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.
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)
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 youYou, 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.
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.
Attachment
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 match2018-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 match2018-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
Attachment
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 match2018-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 match2018-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" :)
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)Makes sense.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.
This wouldn't be desirable, I think...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.
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
Attachment
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; Howclose 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
Attachment
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;
Howclose 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
Attachment
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
Attachment
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.scheffer@uni-muenster.de https://www.uni-muenster.de/ZIV
Attachment
On Fri, Oct 26, 2018 at 2:08 AM Marius Timmer
<marius.timmer@uni-muenster.de> 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 verifydb yetanotheruser $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
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.timmer@uni-muenster.de https://www.uni-muenster.de/ZIV
Attachment
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
Attachment
On Tue, Dec 25, 2018 at 9:08 AM Michael Paquier <michael@paquier.xyz> 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!
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
Attachment
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.timmer@uni-muenster.de https://www.uni-muenster.de/ZIV
Attachment
Hi, On 2019-02-06 15:19:56 +0000, 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
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
Attachment
On Sun, Feb 17, 2019 at 7:50 PM Michael Paquier <michael@paquier.xyz> 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)
Attachment
On Sat, Mar 9, 2019 at 11:03 AM Magnus Hagander <magnus@hagander.net> wrote:
On Sun, Feb 17, 2019 at 7:50 PM Michael Paquier <michael@paquier.xyz> 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!
--