Re: Add "password_protocol" connection parameter to libpq

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-20 04:07:24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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
+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 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.

Attachment Content-Type Size
channel-binding-additions-michael.patch text/x-diff 10.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2019-09-20 04:48:44 Re: A problem about partitionwise join
Previous Message vignesh C 2019-09-20 03:53:26 Re: psql - add SHOW_ALL_RESULTS option