Thread: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding

HI all,

When a client connects during a SCRAM exchange, it has multiple ways
to let the server know what the client supports or not when using
channel binding:
- "n" -> client doesn't support channel binding.
- "y" -> client does support channel binding but thinks the server does not.
- "p" -> client requires channel binding.

On a v10 client, we just need to use the "n" flag because the client
does not support channel binding. This way, a v10 client can connect
to a v10 or v11 server with or without SSL, and this even if the
server has published the SASL mechanism SCRAM-SHA-256-PLUS, which is
used to define channel binding use during SCRAM authentication.

With a v11 client though, things are more fancy:
- If the server publishes the SCRAM-PLUS mechanism, then the client
replies with a "p" message. We are here in the context of an SSL
connection. This is the case of a v11 client, v11 server.
- If using an SSL connection, and the server did not publish
SCRAM-PLUS, then the client uses "y". This is the case of a v11 client
and v10 server.
- For a non-SSL connection, "n" is used. (The server would not have
sent the -PLUS mechanism anyway). This happens for all combinations
without SSL.

When using "n" or "y", the data sent by the client to the server about
the use of channel binding is a base64-encoded string of respectively
"n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
v10 server is able to allow connections with "n,,", but not with
"y,,":
https://www.postgresql.org/message-id/887b6fb7-15fe-239e-2aad-5911d2b0945b@2ndquadrant.com

When trying to connect to a v11 client based on current HEAD to a v10
server using SSL, then the connection would fail. The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.

Thoughts?
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> When trying to connect to a v11 client based on current HEAD to a v10
> server using SSL, then the connection would fail.

That's bad ...

> The attached patch,
> for REL_10_STABLE, allows a server to accept as well as input "eSws",
> which is a combination that can now happen. This way, a v10 server
> accepts connections from a v11 and newer client with SSL.

This is not an acceptable fix.  We have to maintain the ability to connect
to unpatched older servers.  If features added to HEAD have broken that,
either we fix those features to be backwards compatible, or we revert
them.
        regards, tom lane

(Adding Heikki here because that concerns him as well)

On Mon, Nov 20, 2017 at 2:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> The attached patch,
>> for REL_10_STABLE, allows a server to accept as well as input "eSws",
>> which is a combination that can now happen. This way, a v10 server
>> accepts connections from a v11 and newer client with SSL.
>
> This is not an acceptable fix.  We have to maintain the ability to connect
> to unpatched older servers.  If features added to HEAD have broken that,
> either we fix those features to be backwards compatible, or we revert
> them.

Well, when doing the SASL exchange the client does not know what is
the backend version. If we'd know that it would then be possible to
enforce a "n" flag conditionally from the client. So in order to be
backward-compatible we could send unconditionally a "n" flag from a
v11 client even in an SSL context. But that would not actually be true
because the client is able to support channel binding in this case, so
we would make libpq not RFC-compliant.

v10 has not been out for a long time, so this can plead in favor of a
change, and SCRAM is not spread yet. It is really unfortunate that we
did not notice that during at the moment SCRAM has been implemented.
That's clearly a bug of v10.

Honestly I am of the opinion to do a proper fix now and have things in
a clean state which is RFC-compliant instead of maintaining for 10
years a backward-compatible change in libpq that would be only valid
for 10.0 and 10.1 (assuming that the server-side fix is added in
10.2). Note that we need to fix the v10 server anyway with something
like the patch upthread. The enforcement of this "n" flag can just
happen in a v11-client.
-- 
Michael

On 11/19/17 23:08, Michael Paquier wrote:
> When using "n" or "y", the data sent by the client to the server about
> the use of channel binding is a base64-encoded string of respectively
> "n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
> v10 server is able to allow connections with "n,,", but not with
> "y,,":
> https://www.postgresql.org/message-id/887b6fb7-15fe-239e-2aad-5911d2b0945b@2ndquadrant.com
> 
> When trying to connect to a v11 client based on current HEAD to a v10
> server using SSL, then the connection would fail. The attached patch,
> for REL_10_STABLE, allows a server to accept as well as input "eSws",
> which is a combination that can now happen. This way, a v10 server
> accepts connections from a v11 and newer client with SSL.

I noticed what I think is an omission in the current v11 code.  We also
need to check whether the channel binding flag (n/y/p) encoded in the
client-final-message is the same one used in the client-first-message.
See attached patch.  This would also affect what we might end up
backpatching.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Nov 23, 2017 at 4:08 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 11/19/17 23:08, Michael Paquier wrote:
>> When using "n" or "y", the data sent by the client to the server about
>> the use of channel binding is a base64-encoded string of respectively
>> "n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
>> v10 server is able to allow connections with "n,,", but not with
>> "y,,":
>> https://www.postgresql.org/message-id/887b6fb7-15fe-239e-2aad-5911d2b0945b@2ndquadrant.com
>>
>> When trying to connect to a v11 client based on current HEAD to a v10
>> server using SSL, then the connection would fail. The attached patch,
>> for REL_10_STABLE, allows a server to accept as well as input "eSws",
>> which is a combination that can now happen. This way, a v10 server
>> accepts connections from a v11 and newer client with SSL.
>
> I noticed what I think is an omission in the current v11 code.  We also
> need to check whether the channel binding flag (n/y/p) encoded in the
> client-final-message is the same one used in the client-first-message.
> See attached patch.  This would also affect what we might end up
> backpatching.

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. I don't see much need for an assertion or
such.
-- 
Michael

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?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
<peter.eisentraut@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.
-- 
Michael

On 11/30/17 00:36, Michael Paquier wrote:
> On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
> <peter.eisentraut@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.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Dec 1, 2017 at 11:55 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 11/30/17 00:36, Michael Paquier wrote:
>> On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
>> <peter.eisentraut@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

On 12/1/17 18:11, Michael Paquier wrote:
> 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.

Committed.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Dec 9, 2017 at 12:23 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 12/1/17 18:11, Michael Paquier wrote:
>> 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.
>
> Committed.

Thanks.
-- 
Michael