On Tue, Apr 10, 2018 at 2:10 PM, Julian Markwort < julian.markw...@uni-muenster.de> wrote:
> On Fri, 2018-04-06 at 20:31 +0200, Magnus Hagander wrote: > > I've been through this one again. > > Thanks for taking the time! > > There is one big omission from it -- it fails to work with the view > pg_hba_file_rules. When fixing that, things started to look kind of ugly > with the "two booleans to indicate one thing", so I went ahead and changed > it to instead be an enum of 3 values. > > Oh, I missed that view. To be honest, it wasn't even on my mind that there > could be a view depending on pg_hba.... > > Also, now when using verify-full or verify-ca, I figured its a lot easier > to misspell the value, and we don't want that to mean "off". Instead, I > made it give an error in this case instead of silently treating it as 0. > > Good catch! > > The option "2" for clientcert was never actually documented, and I think > we should get rid of that. We need to keep "1" for backwards compatibility > (which arguably was a bad choice originally, but that was many years ago), > but let's not add another one. > I also made a couple of very small changes to the docs. > > Attached is an updated patch with these changes. I'd appreciate it if you > can run it through your tests to confirm that it didn't break any of those > usecases. > > I've tested a couple of things with this and it seems to work as expected. > Unforunately, there are no tests for libpq, afaik. But testing such > features would become complicated quite quickly, with the need to generate > certificates and such... > As Peter mentionde, there are in src/test/ssl. I forgot about those, but yes, it would be useful to have that. I've noticed that the error message for mismatching CN is now printed twice > when using password prompts, as all authentication details are checked upon > initiation of a connection and again later, after sending the password. > That depends on your authenticaiton method. Specifically for passwords, what happens is there are actually two separate connections -- the first one with no password supplied, and the second one with a password in it. We could directly reject the connection after the first one and not ask for a password. I'm not sure if that's better though -- that would leak the information on *why* we rejected the connection. 2018-04-10 13:54:19.882 CEST [8735] LOG: provided user name (testuser) and > authenticated user name (nottestuser) do not match > 2018-04-10 13:54:19.882 CEST [8735] LOG: certificate validation failed > for user "testuser": common name in client certificate does not match user > name or mapping, but clientcert=verify-full is enabled. > 2018-04-10 13:54:21.515 CEST [8736] LOG: provided user name (testuser) > and authenticated user name (nottestuser) do not match > 2018-04-10 13:54:21.515 CEST [8736] LOG: certificate validation failed > for user "testuser": common name in client certificate does not match user > name or mapping, but clientcert=verify-full is enabled. > 2018-04-10 13:54:21.515 CEST [8736] FATAL: password authentication failed > for user "testuser" > 2018-04-10 13:54:21.515 CEST [8736] DETAIL: Connection matched > pg_hba.conf line 94: "hostssl all testuser 127.0.0.1/32 password > clientcert=verify-full" > > > Also, as you can see, there are two LOG messages indicating the mismatch > -- "provided user name (testuser) and authenticated user name (nottestuser) > do not match" comes from hba.c:check_usermap() and "certificate validation > failed for user "testuser": common name in client certificate does not > match user name or mapping, but clientcert=verify-full is enabled." is the > message added in auth.c:CheckCertAuth() by the patch. > Maybe we should shorten the latter LOG message? > It might definitely be worth shorting it yeah. For one, we can just use "cn" :) -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>