On Fri, Dec 1, 2017 at 11:55 PM, Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > On 11/30/17 00:36, Michael Paquier wrote: >> On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut >> <peter.eisentr...@2ndquadrant.com> wrote: >>> On 11/22/17 21:08, Michael Paquier wrote: >>>> Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would >>>> be also nice to add a comment to keep in sync the logics in >>>> build_client_first_message() and build_client_final_message() which >>>> assign the cbind flag value. >>> >>> Could you clarify what comment you would like to have added or changed? >> >> Sure. Here is with the attached patch what I have in mind. The way >> cbind-flag is assigned in the client-first message should be kept >> in-sync with the way the client-final message builds the binding data >> in c=. It could be possible to add more sanity-checks based on >> assertions by keeping track of the cbind-flag assigned in the >> client-first message as your upthread patch is doing in the backend >> code, but I see a simple comment as a sufficient reminder. > > Committed with that comment, thanks.
Cool. Thanks. For REL_10_STABLE, I would suggest the attached patch then. This ensures that eSws is checked in the final message and that the cbind-flag sent in the first message maps with the data of the final message in the backend. I have checked with the following configurations with a v10 backend: - v11 libpq with SSL - v11 libpq without SSL - v10 libpq with SSL - v10 libpq without SSL And in all cases the connection is accepted as it should. -- Michael
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 9161c885e1..7b87d1c14b 100644 --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -112,6 +112,8 @@ typedef struct const char *username; /* username from startup packet */ + char cbind_flag; + int iterations; char *salt; /* base64-encoded */ uint8 StoredKey[SCRAM_KEY_LEN]; @@ -774,6 +776,7 @@ read_client_first_message(scram_state *state, char *input) */ /* read gs2-cbind-flag */ + state->cbind_flag = *input; switch (*input) { case 'n': @@ -1033,10 +1036,13 @@ read_client_final_message(scram_state *state, char *input) /* * Read channel-binding. We don't support channel binding, so it's - * expected to always be "biws", which is "n,,", base64-encoded. + * expected to always be "biws", which is "n,,", base64-encoded or + * "eSws", which is "y,,". We also have to check whether the flag is + * the same one that the client originally sent. */ channel_binding = read_attr_value(&p, 'c'); - if (strcmp(channel_binding, "biws") != 0) + if (!(strcmp(channel_binding, "biws") == 0 && state->cbind_flag == 'n') && + !(strcmp(channel_binding, "eSws") == 0 && state->cbind_flag == 'y')) ereport(ERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), (errmsg("unexpected SCRAM channel-binding attribute in client-final-message"))));