Обсуждение: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Michael Paquier
Дата:
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
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Michael Paquier
Дата:
(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
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Peter Eisentraut
Дата:
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
Вложения
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Michael Paquier
Дата:
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
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Peter Eisentraut
Дата:
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
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Michael Paquier
Дата:
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
Вложения
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Peter Eisentraut
Дата:
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
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Michael Paquier
Дата:
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
Вложения
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Peter Eisentraut
Дата:
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
Re: Allowing SSL connection of v11 client to v10 server with SCRAMchannel binding
От
Michael Paquier
Дата:
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