Re: Add "password_protocol" connection parameter to libpq - Mailing list 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