On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote:
> Nit here: "scram-sha-256" refers to the HBA entry. I would
> just use "SCRAM" instead.
Done.
> In pg_SASL_init(), if the server sends SCRAM-SHA-256-PLUS as SASL
> mechanism over a non-SSL connection, should we complain even if
> the "disable" mode is used? It seems to me that there is a point to
> complain in this case as a sanity check as the server should really
> publicize "SCRAM-SHA-256-PLUS" only over SSL.
Done.
> When the server only sends SCRAM-SHA-256 in the mechanism list and
> "require" mode is used, we complain about "none of the server's SASL
> authentication mechanisms are supported" which can be confusing. Why
> not generating a custom error if libpq selects SCRAM-SHA-256 when
> "require" is used, say:
> "SASL authentication mechanism SCRAM-SHA-256 selected but channel
> binding is required"
> That could be done by adding an error message when selecting
> SCRAM-SHA-256 and then goto the error step.
Done.
> 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.
> +# 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.
> PGCHANNELBINDING needs documentation.
Done.
Regards,
Jeff Davis