On Wed, Sep 04, 2019 at 09:22:33PM -0700, Jeff Davis wrote: > New patch attached.
Thanks for the new version, Jeff. + with <productname>PostgreSQL</productname> 11 or later servers using + the <literal>scram-sha-256</literal> authentication method. Nit here: "scram-sha-256" refers to the HBA entry. I would just use "SCRAM" instead. 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. 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. 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(). +# 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? PGCHANNELBINDING needs documentation. -- Michael
signature.asc
Description: PGP signature