From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Jeff Davis <pgsql(at)j-davis(dot)com> |
Cc: | "Jonathan S(dot) Katz" <jkatz(at)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Add "password_protocol" connection parameter to libpq |
Date: | 2019-09-17 07:04:14 |
Message-ID: | 20190917070414.GH1660@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Sep 14, 2019 at 08:42:53AM -0700, Jeff Davis wrote:
> On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote:
>> Actually, it looks that the handling of channel_bound is incorrect.
>> If the server sends AUTH_REQ_SASL and libpq processes it, then the
>> flag gets already set. Now let's imagine that the server is a rogue
>> one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check
>> would pass. It seems to me that we should switch the flag once we
>> are
>> sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when
>> the final message is done within pg_SASL_continue().
>
> Thank you! Fixed. I now track whether channel binding is selected, and
> also whether SASL actually finished successfully.
Ah, I see. So you have added an extra flag "sasl_finished" to make
sure of that, and still kept around "channel_bound". Hence the two
flags have to be set to make sure that the SASL exchanged has been
finished and that channel binding has been enabled. "channel_bound"
is linked to the selected mechanism when the exchange begins, meaning
that it could be possible to do the same check with the selected
mechanism directly from fe_scram_state instead. "sasl_finished" is
linked to the state where the SASL exchange is finished, so this
basically maps into checking after FE_SCRAM_FINISHED. Instead of
those two flags, wouldn't it be cleaner to add an extra routine to
fe-auth-scram.c which does the same sanity checks, say
pg_fe_scram_check_state()? This needs to be careful about three
things, taking in input an opaque pointer to fe_scram_state if channel
binding is required:
- The data is not NULL.
- The sasl mechanism selected is SCRAM-SHA-256-PLUS.
- The state is FE_SCRAM_FINISHED.
What do you think? There is no need to save down the connection
parameter value into fe_scram_state.
>> +# SSL not in use; channel binding still can't work
>> +reset_pg_hba($node, 'scram-sha-256');
>> +$ENV{"PGCHANNELBINDING"} = 'require';
>> +test_login($node, 'saslpreptest4a_role', "a", 2);
>> Worth testing md5 here?
>
> I added a new md5 test in the ssl test suite. Testing it in the non-SSL
> path doesn't seem like it adds much.
Good idea.
+ if (conn->channel_binding[0] == 'r' && /* require */
+ strcmp(selected_mechanism, SCRAM_SHA_256_NAME) == 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("SASL authentication mechanism
SCRAM-SHA-256 selected but channel binding is required\n"));
+ goto error;
+ }
Nit here as there are only two mechanisms handled: I would rather
cause the error if the selected mechanism does not match
SCRAM-SHA-256-PLUS, instead of complaining if the selected mechanism
matches SCRAM-SHA-256. Either way is actually fine :)
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Channel binding required but not
offered by server\n"));
+ result = false;
Should that be "completed by server" instead?
+ if (areq == AUTH_REQ_SASL_FIN)
+ conn->sasl_finished = true;
This should have a comment about the why it is done if you go this way
with the two flags added to PGconn.
+ if (conn->channel_binding[0] == 'r' && /* require */
+ !conn->ssl_in_use)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Channel binding required, but
SSL not in use\n"));
+ goto error;
+ }
This is not necessary? If SSL is not in use but the server publishes
SCRAM-SHA-256-PLUS, libpq complains. If the server sends only
SCRAM-SHA-256 but channel binding is required, we complain down on
"SASL authentication mechanism SCRAM-SHA selected but channel binding
is required". Or you have in mind that this error message is better?
I think that pgindent would complain with the comment block in
check_expected_areq().
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2019-09-17 07:17:21 | Re: Leakproofness of texteq()/textne() |
Previous Message | David Fetter | 2019-09-17 07:01:57 | Re: Efficient output for integer types |