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

On Tue, Apr 10, 2018 at 2:10 PM, Julian Markwort <
julian(dot)markwort(at)uni-muenster(dot)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/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>