Re: Add "password_protocol" connection parameter to libpq

On Thu, Sep 19, 2019 at 05:40:15PM -0700, Jeff Davis wrote:
> On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote:
>> What do you think?  There is no need to save down the connection
>> parameter value into fe_scram_state.
>
> I'm not sure I understand this comment, but I removed the extra boolean
> flags.

Thanks for considering it.  I was just asking about removing those
flags and your thoughts about my thoughts from upthread.

>> is required".  Or you have in mind that this error message is better?
>
> I felt it would be a more useful error message.

Okay, fine by me.

>> I think that pgindent would complain with the comment block in
>> check_expected_areq().
>
> Changed.

A trick to make pgindent to ignore some comment blocks is to use
/*--------- at its top and bottom, FWIW.

+$ENV{PGUSER} = "ssltestuser";
 $ENV{PGPASSWORD} = "pass";
test_connect_ok() can use a complementary string, so I would use that
in the SSL test part instead of relying too much on the environment
for readability, particularly for the last test added with md5testuser.
Using the environment variable in src/test/authentication/ makes
sense.  Maybe that's just a matter of taste :)

+   return (state != NULL && state->state == FE_SCRAM_FINISHED &&
+           strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0);
I think that we should document in the code why those reasons are
chosen.

I would also add a test for an invalid value of channel_binding.

A comment update is forgotten in libpq-int.h.

+# using the password authentication method; channel binding can't
work
+reset_pg_hba($node, 'password');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
+
+# 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);
Those two tests are in the test suite dedicated to SASLprep.  I think
that it would be more consistent to just move them to
001_password.pl.  And this does not impact the error coverage.

Missing some indentation in the perl scripts (per pgperltidy).

Those are mainly nits, and attached are the changes I would do to your
patch.  Please feel free to consider those changes as you see fit.
Anyway, the base logic looks good to me, so I am switching the patch
as ready for committer.
--
Michael