Horton

Thread

Commits

  1. Don't use SCRAM-specific "e=invalid-proof" on invalid password.

  1. Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-03-14T12:36:51Z

    Hi,

I didn't include the authentication TAP tests that Michael wrote in the 
main SCRAM commit last week. The main issue was that the new test was 
tacked on the src/test/recovery test suite, for lack of a better place. 
I propose that we add a whole new src/test/authentication directory for 
it. It would also be logical to merge src/test/ssl into it, but the SSL 
test suite has some complicated setup steps, to create the certificates, 
and it cannot be safely run on a multi-user system. So probably best to 
keep it separate, after all.

While looking at the test, I noticed that the SCRAM patch didn't include 
support for logging in with plain 'password' authentication, when the 
user has a SCRAM verifier stored in pg_authid. That was an oversight. If 
the client gives the server the plain password, it's easy for the server 
to verify that it matches the SCRAM verifier.

Attached patches add the TAP test suite, and implement plain 'password' 
authentication for users with SCRAM verifier. Any comments?

- Heikki

  2. Re: Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-14T13:43:12Z

    On Tue, Mar 14, 2017 at 9:36 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> While looking at the test, I noticed that the SCRAM patch didn't include
> support for logging in with plain 'password' authentication, when the user
> has a SCRAM verifier stored in pg_authid. That was an oversight. If the
> client gives the server the plain password, it's easy for the server to
> verify that it matches the SCRAM verifier.

Right. I forgot about that..

> Attached patches add the TAP test suite, and implement plain 'password'
> authentication for users with SCRAM verifier. Any comments?

+       /*
+        * The password looked like a SCRAM verifier, but could not be
+        * parsed.
+        */
+       elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
This would be sent back to the client, no? I think that you should use
*logdetail as well in scram_verify_plain_password.

+# This test cannot run on Windows as Postgres cannot be set up with Unix
+# sockets and needs to go through SSPI.
Yes, true. Having that in its own folder is fine for me.
-- 
Michael

  3. Re: Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Jeff Janes <jeff.janes@gmail.com> — 2017-03-14T19:02:48Z

    On Tue, Mar 14, 2017 at 5:36 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> Hi,
>
> I didn't include the authentication TAP tests that Michael wrote in the
> main SCRAM commit last week. The main issue was that the new test was
> tacked on the src/test/recovery test suite, for lack of a better place. I
> propose that we add a whole new src/test/authentication directory for it.
> It would also be logical to merge src/test/ssl into it, but the SSL test
> suite has some complicated setup steps, to create the certificates, and it
> cannot be safely run on a multi-user system. So probably best to keep it
> separate, after all.
>
> While looking at the test, I noticed that the SCRAM patch didn't include
> support for logging in with plain 'password' authentication, when the user
> has a SCRAM verifier stored in pg_authid. That was an oversight. If the
> client gives the server the plain password, it's easy for the server to
> verify that it matches the SCRAM verifier.
>

I noticed the asymmetry over plain-text passwords, and didn't know if it
was intentional or not.  It is somewhat disconcerting that the client will
send a plain-text password to a mis-configured (or mal-configured) server,
but I don't think there is anything this patch series can reasonably do
about that.


>
> Attached patches add the TAP test suite, and implement plain 'password'
> authentication for users with SCRAM verifier. Any comments?


Does what it says, says what it does.  There is no installcheck target,
which makes sense because it inherently has to muck around with
pg_hba.conf.  The test should be updated to test the syntax for
0001-Add-clause-PASSWORD-val-USING-protocol-to-CREATE-ALT.patch if that
gets committed.

The message returned to the client for the wrong password differs between
pg_hba-set scram and pg_hba-set md5/password methods.  Is that OK?

psql: error received from server in SASL exchange: invalid-proof

psql: FATAL:  password authentication failed for user "test"

Cheers,

Jeff

  4. Re: Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-03-14T19:25:38Z

    On 03/14/2017 09:02 PM, Jeff Janes wrote:
> It is somewhat disconcerting that the client will send a plain-text
> password to a mis-configured (or mal-configured) server, but I don't
> think there is anything this patch series can reasonably do about
> that.

Yeah. That's one pretty glaring hole with libpq: there's no option to 
disallow insecure authentication mechanisms. That's not new, but now 
that we have SCRAM that's otherwise more secure, it looks worse in 
comparison.

SCRAM also has a nice feature that it provides proof to the client, that 
the server knew the password (or rather, had a verifier for that 
password). In other words, the client knows that it connected to the 
correct server, not a fake one. But currently nothing prevents a fake 
server from simply not doing SCRAM authentication.

We clearly need something similar to sslmode=require in libpq, to 
tighten that up.

> The message returned to the client for the wrong password differs between
> pg_hba-set scram and pg_hba-set md5/password methods.  Is that OK?
>
> psql: error received from server in SASL exchange: invalid-proof
>
> psql: FATAL:  password authentication failed for user "test"

Ah yeah, I was on the fence on that one. Currently, the server returns 
the invalid-proof error to the client, as defined in RFC5802. That 
results in that error message from libpq. Alternatively, the server 
could elog(FATAL), like the other authentication mechanisms do, with the 
same message. The RFC allows that behavior too but returning the 
invalid-proof error code is potentially more friendly to 3rd party SCRAM 
implementations.

One option would be to recognize the "invalid-proof" message in libpq, 
and construct a more informative error message in libpq. Could use the 
same wording, "password authentication failed", but it would behave 
differently wrt. translation, at least.

- Heikki

  5. Re: Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-03-17T09:40:54Z

    On 03/14/2017 03:43 PM, Michael Paquier wrote:
> +       /*
> +        * The password looked like a SCRAM verifier, but could not be
> +        * parsed.
> +        */
> +       elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
> This would be sent back to the client, no? I think that you should use
> *logdetail as well in scram_verify_plain_password.

No, LOG messages are never sent to the client. Well, unless you have 
client_min_messages='log', but then all the LOG messages with details 
would be sent to the clients anyway. (We don't process the GUCs from the 
startup packet until after authentication, so an unauthenticated user 
cannot set client_min_messages='log').

Committed, thanks.

- Heikki

  6. Re: Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-17T11:42:46Z

    On Fri, Mar 17, 2017 at 6:40 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Committed, thanks.

Thanks for the commit.
-- 
Michael

  7. Re: Authentication tests, and plain 'password' authentication with a SCRAM verifier

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-05-05T07:15:17Z

    On 03/14/2017 09:25 PM, Heikki Linnakangas wrote:
> On 03/14/2017 09:02 PM, Jeff Janes wrote:
>> The message returned to the client for the wrong password differs between
>> pg_hba-set scram and pg_hba-set md5/password methods.  Is that OK?
>>
>> psql: error received from server in SASL exchange: invalid-proof
>>
>> psql: FATAL:  password authentication failed for user "test"
>
> Ah yeah, I was on the fence on that one. Currently, the server returns
> the invalid-proof error to the client, as defined in RFC5802. That
> results in that error message from libpq. Alternatively, the server
> could elog(FATAL), like the other authentication mechanisms do, with the
> same message. The RFC allows that behavior too but returning the
> invalid-proof error code is potentially more friendly to 3rd party SCRAM
> implementations.
>
> One option would be to recognize the "invalid-proof" message in libpq,
> and construct a more informative error message in libpq. Could use the
> same wording, "password authentication failed", but it would behave
> differently wrt. translation, at least.

I went ahead and changed the backend code to not send the 
"invalid-proof" error. That seemed like the easiest fix for this. You 
now get the same "password authentication failed" error as with MD5 and 
plain password authentication.

- Heikki