Обсуждение: Handling better supported channel binding types for SSLimplementations

Hi all,

Per the recent discussions around support of new SSL implementations for
Postgres, it is becoming rather clear to me that the backend needs to be
a bit smarter with the way it needs to decide if it should publish or
not SCRAM-SHA-256-PLUS in the list that the clients can use to choose a
SASL mechanism for authentication.

This has been discussed here in the MacOS SSL implementation:
https://www.postgresql.org/message-id/20180122014637.GE22690%40paquier.xyz
As well as here for GnuTLS:
https://www.postgresql.org/message-id/CAB7nPqRJteyoyyj8E__v1D1SMoj8jpv6ZPyHuc7Md45%2BED-uMA%40mail.gmail.com

Attached is a patch which is an attempt to make this choice cleaner for
new SSL implementations. As we are (rightly!) calling the APIs to fetch
the channel binding data only when necessary, I think that we need an
extra API for SSL implementations to let the server decide if channel
binding mechanisms should be published or not. There could be multiple
possibilities, like making this API return only a boolean flag. However
I have made a more extensible choice by having each SSL implementation
build a list of supported channel bindings. This matters for each
implementation as:
- GnuTLS only supports tls-unique.
- OpenSSL supports both tls-unique and tls-server-end-point.
- MacOS would support none.
However there is as well the argument that this list's contents are not
directly used now, and based on what I saw from the MacOS SSL and GnuTLS
patches that would not be the case after either.

I am adding that to the next CF for consideration.

Thoughts?
--
Michael

> On 22 Jan 2018, at 08:29, Michael Paquier <michael.paquier@gmail.com> wrote:

> Attached is a patch which is an attempt to make this choice cleaner for
> new SSL implementations. As we are (rightly!) calling the APIs to fetch
> the channel binding data only when necessary, I think that we need an
> extra API for SSL implementations to let the server decide if channel
> binding mechanisms should be published or not. There could be multiple
> possibilities, like making this API return only a boolean flag. However
> I have made a more extensible choice by having each SSL implementation
> build a list of supported channel bindings.

An extensible API makes more sense than on/off (or one on/off call per
binding).  Perhaps a way to validate the contents of the list is required
though?  Or an assertion on the contents to catch errors during testing.

Nitpicking: In src/backend/libpq/auth.c:CheckSCRAMAuth(), this comment reads a
bit strange:

+     * Get the list of channel binding types supported by this SSL
+     * implementation to determine if server should publish -PLUS
+     * mechanisms or not.

Since auth.c isn’t tied to any SSL implementation, shouldn’t it be “supported
by the configured SSL implementation” or something along those lines?

cheers ./daniel

On Mon, Jan 22, 2018 at 11:07:55AM +0100, Daniel Gustafsson wrote:
> An extensible API makes more sense than on/off (or one on/off call per
> binding).  Perhaps a way to validate the contents of the list is
> required though?  Or an assertion on the contents to catch errors
> during testing.

Do you have something specific in mind?

> Nitpicking: In src/backend/libpq/auth.c:CheckSCRAMAuth(), this comment
> reads a bit strange:
>
> +     * Get the list of channel binding types supported by this SSL
> +     * implementation to determine if server should publish -PLUS
> +     * mechanisms or not.
>
> Since auth.c isn’t tied to any SSL implementation, shouldn’t it be
> “supported by the configured SSL implementation” or something along
> those lines?

Yes, your words sound better.
--
Michael

> On 22 Jan 2018, at 14:05, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Mon, Jan 22, 2018 at 11:07:55AM +0100, Daniel Gustafsson wrote:
>> An extensible API makes more sense than on/off (or one on/off call per
>> binding).  Perhaps a way to validate the contents of the list is
>> required though?  Or an assertion on the contents to catch errors
>> during testing.
>
> Do you have something specific in mind?

Not really, but IIUC a bug in this code could lead to channel binding not being
used for a connection even if the user wanted it (and may miss that ixt didn’t
happen due to not looking at logs etc)?  Considering the limited set of values
(currently) it should be quite simple to check for offending entries, if there
is indeed a risk of “silently” not using channel binding.

cheers ./daniel

On Mon, Jan 22, 2018 at 04:52:11PM +0100, Daniel Gustafsson wrote:
> Not really, but IIUC a bug in this code could lead to channel binding
> not being used for a connection even if the user wanted it (and may
> miss that ixt didn’t happen due to not looking at logs etc)?

Possible, if an implementation decides to send a NIL list as return
result of this API when it should not.

> Considering the limited set of values (currently) it should be quite
> simple to check for offending entries, if there is indeed a risk of
> “silently” not using channel binding.

I think I understand the point you are coming at, but a problem is that
the channel binding type the client wants to use is known by the server
in SCRAM authentication only after reading the client-first message,
which happens of course after the client decided to choose if channel
binding is used or not. Now the server needs to emit first a list of
supported SASL mechanisms which are consistent with what the SSL
implementation can do when the mechanism is negotiated. Another, third
approach that I can think of is to have this additional API in betls
emit a list of mechanisms, but I am not sure that this is worth the
complication as at the end what you want to know is if at least one
channel binding type is supported or not.

We could as well live with the existing set of betls APIs, just that the
implementation of secure transport for MacOS will need a hack in auth.c
to stop the -PLUS mechanisms to be sent. Putting this load into each
be-secure-*.c file is cleaner in my opinion.
--
Michael

> On 23 Jan 2018, at 03:24, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Mon, Jan 22, 2018 at 04:52:11PM +0100, Daniel Gustafsson wrote:
>> Not really, but IIUC a bug in this code could lead to channel binding
>> not being used for a connection even if the user wanted it (and may
>> miss that ixt didn’t happen due to not looking at logs etc)?
>
> Possible, if an implementation decides to send a NIL list as return
> result of this API when it should not.
>
>> Considering the limited set of values (currently) it should be quite
>> simple to check for offending entries, if there is indeed a risk of
>> “silently” not using channel binding.
>
> I think I understand the point you are coming at, but a problem is that
> the channel binding type the client wants to use is known by the server
> in SCRAM authentication only after reading the client-first message,
> which happens of course after the client decided to choose if channel
> binding is used or not. Now the server needs to emit first a list of
> supported SASL mechanisms which are consistent with what the SSL
> implementation can do when the mechanism is negotiated. Another, third
> approach that I can think of is to have this additional API in betls
> emit a list of mechanisms, but I am not sure that this is worth the
> complication as at the end what you want to know is if at least one
> channel binding type is supported or not.

Thanks for the explanation, I agree that the proposed approach makes the most
sense.

> We could as well live with the existing set of betls APIs, just that the
> implementation of secure transport for MacOS will need a hack in auth.c
> to stop the -PLUS mechanisms to be sent. Putting this load into each
> be-secure-*.c file is cleaner in my opinion.

I completely agree, let’s avoid such hacks.

cheers ./daniel

On 1/22/18 02:29, Michael Paquier wrote:
> However there is as well the argument that this list's contents are not
> directly used now, and based on what I saw from the MacOS SSL and GnuTLS
> patches that would not be the case after either.

Right, there is no facility for negotiating the channel binding type, so
a boolean result should be enough.

In which case we wouldn't actually need this for GnuTLS yet.

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

On Tue, Jan 23, 2018 at 12:08:37PM -0500, Peter Eisentraut wrote:
> On 1/22/18 02:29, Michael Paquier wrote:
>> However there is as well the argument that this list's contents are not
>> directly used now, and based on what I saw from the MacOS SSL and GnuTLS
>> patches that would not be the case after either.
>
> Right, there is no facility for negotiating the channel binding type, so
> a boolean result should be enough.

I am not completely convinced either that we need to complicate the code
to handle channel binding type negotiation.

> In which case we wouldn't actually need this for GnuTLS yet.

Sure. This depends mainly on how the patch for Mac's Secure Transport
moves forward.
--
Michael

On 1/23/18 21:27, Michael Paquier wrote:
> On Tue, Jan 23, 2018 at 12:08:37PM -0500, Peter Eisentraut wrote:
>> On 1/22/18 02:29, Michael Paquier wrote:
>>> However there is as well the argument that this list's contents are not
>>> directly used now, and based on what I saw from the MacOS SSL and GnuTLS
>>> patches that would not be the case after either.
>>
>> Right, there is no facility for negotiating the channel binding type, so
>> a boolean result should be enough.
> 
> I am not completely convinced either that we need to complicate the code
> to handle channel binding type negotiation.
> 
>> In which case we wouldn't actually need this for GnuTLS yet.
> 
> Sure. This depends mainly on how the patch for Mac's Secure Transport
> moves forward.

Moved to next CF along with those other two patches.

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

On Thu, Mar 08, 2018 at 02:19:55PM -0500, Peter Eisentraut wrote:
> Moved to next CF along with those other two patches.

Attached is a refreshed patch for this thread, which failed to apply
after recent changes.  This is tied with patches for other SSL
implementations, so let's see about it later if necessary.
--
Michael

On 28/05/18 06:46, Michael Paquier wrote:
> On Thu, Mar 08, 2018 at 02:19:55PM -0500, Peter Eisentraut wrote:
>> Moved to next CF along with those other two patches.
> 
> Attached is a refreshed patch for this thread, which failed to apply
> after recent changes.  This is tied with patches for other SSL
> implementations, so let's see about it later if necessary.

This was obsoleted by commit 77291139, I'll close this in the commitfest.

- Heikki