Thread: [HACKERS] Channel binding support for SCRAM-SHA-256

[HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
Hi all,

Please find attached a patch to add support for channel binding for
SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802
(https://tools.ietf.org/html/rfc5802), servers supporting channel
binding need to add support for tls-unique, and this is what this
patch does.

As defined in RFC5056 (exactly here =>
https://tools.ietf.org/html/rfc5056#section-4.1), servers can use the
TLS finish message to determine if the client is actually the same as
the one who initiated the connection, to eliminate the risk of MITM
attacks. OpenSSL offers two undocumented APIs for this purpose:
- SSL_get_finished() to get the latest TLS finish message that the
client has sent.
- SSL_get_peer_finished(), to get the latest TLS finish message
expected by the server.
So basically what is needed here is saving the latest message
generated by client in libpq using get_finished(), send it to the
server using the SASL exchange messages (value sent with the final
client message), and then compare it with what the server expected.
Connection is successful if what the client has sent and what the
server expects match.

There is also a clear documentation about what is expected from the
client and the server in terms of how both should behave using the
first client message https://tools.ietf.org/html/rfc5802#section-6. So
I have tried to follow it, reviews are welcome particularly regarding
that. The implementation is done in such a way that channel binding is
used in the context of an SSL connection, which is what the RFCs
expect from an implementation.

Before even that, the server needs to send to the client the list of
SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if
the server has been built with SSL support to the list.

Comments are welcome, I am parking that in the next CF for integration in PG11.
Thanks,
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:
[cross-posting to pgjdbc]


On 25/05/17 17:17, Michael Paquier wrote:
> Hi all,
>
> Please find attached a patch to add support for channel binding for
> SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802
> (https://tools.ietf.org/html/rfc5802), servers supporting channel
> binding need to add support for tls-unique, and this is what this
> patch does.

     This is awesome, Michael :) Thank you! You have prevented me
eventually writing the patch and having then PostgreSQL source tainted
with Java-to-C "transpiled" code ^_^
>
> As defined in RFC5056 (exactly here =>
> https://tools.ietf.org/html/rfc5056#section-4.1), servers can use the
> TLS finish message to determine if the client is actually the same as
> the one who initiated the connection, to eliminate the risk of MITM
> attacks. OpenSSL offers two undocumented APIs for this purpose:
> - SSL_get_finished() to get the latest TLS finish message that the
> client has sent.
> - SSL_get_peer_finished(), to get the latest TLS finish message
> expected by the server.
> So basically what is needed here is saving the latest message
> generated by client in libpq using get_finished(), send it to the
> server using the SASL exchange messages (value sent with the final
> client message), and then compare it with what the server expected.
> Connection is successful if what the client has sent and what the
> server expects match.
>
> There is also a clear documentation about what is expected from the
> client and the server in terms of how both should behave using the
> first client message https://tools.ietf.org/html/rfc5802#section-6. So
> I have tried to follow it, reviews are welcome particularly regarding
> that. The implementation is done in such a way that channel binding is
> used in the context of an SSL connection, which is what the RFCs
> expect from an implementation.

     After a deeper analysis, I have some concerns/comments here:

- tls-unique, as you mentioned, uses two undocumented APIs. This raises
a small flag about the stability and future of those APIs.

- More importantly, some drivers (not libpq-based) may have unexpected
difficulties implementing tls-unique. In particular, there is not an API
in Java to get the finished message. I expect other languages to maybe
have similar limitations. For Java I see two options:
     * Also implement tls-server-end-point, which rather relies on the
server certificate. This information seems to be exposed as part of the
Java APIs:
https://docs.oracle.com/javase/8/docs/api/java/security/cert/Certificate.html#getEncoded--
     * Use the BouncyCastle library (http://bouncycastle.org/), which
explicitly supports tls-unique
(https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/est/TLSUniqueProvider.html#getTLSUnique()
,
https://www.bouncycastle.org/docs/tlsdocs1.5on/org/bouncycastle/tls/ChannelBinding.html#tls_unique
). This would require, however, non-trivial changes to the driver and, I
expect, a lot of extra effort.

- It seems to me that tls-unique might be a bit fragile. In particular,
it requires the server to be aware of TSL renegotiations and avoid
performing one while the SCRAM authentication is being performed. I
don't know if this is already guaranteed by the current patch, but it
seems to me it requires complex interactions between levels of
abstraction that are quite separate (SSL and SCRAM). This is explained
by the RFC:

"
server applications MUST NOT request TLS renegotiation during phases of
the application protocol during which application-layer authentication
occurs
"
(https://tools.ietf.org/html/rfc5929#section-3.1)


     Based on this facts, I'd suggest to either implement
tls-server-end-point or implement both tls-server-end-point and
tls-unique. The latter would require a more complete protocol message to
advertise separately SCRAM mechanisms and channel binding names. One of
such structures could be the one I suggested earlier:
https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638fc8@8kdata.com


>
> Before even that, the server needs to send to the client the list of
> SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if
> the server has been built with SSL support to the list.

     And I'd say the list of channel binding names supported.


     Álvaro


--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:
[cross-posting to pgjdbc]


On 25/05/17 17:17, Michael Paquier wrote:
> Hi all,
>
> Please find attached a patch to add support for channel binding for
> SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802
> (https://tools.ietf.org/html/rfc5802), servers supporting channel
> binding need to add support for tls-unique, and this is what this
> patch does.

     This is awesome, Michael :) Thank you! You have prevented me
eventually writing the patch and having then PostgreSQL source tainted
with Java-to-C "transpiled" code ^_^
>
> As defined in RFC5056 (exactly here =>
> https://tools.ietf.org/html/rfc5056#section-4.1), servers can use the
> TLS finish message to determine if the client is actually the same as
> the one who initiated the connection, to eliminate the risk of MITM
> attacks. OpenSSL offers two undocumented APIs for this purpose:
> - SSL_get_finished() to get the latest TLS finish message that the
> client has sent.
> - SSL_get_peer_finished(), to get the latest TLS finish message
> expected by the server.
> So basically what is needed here is saving the latest message
> generated by client in libpq using get_finished(), send it to the
> server using the SASL exchange messages (value sent with the final
> client message), and then compare it with what the server expected.
> Connection is successful if what the client has sent and what the
> server expects match.
>
> There is also a clear documentation about what is expected from the
> client and the server in terms of how both should behave using the
> first client message https://tools.ietf.org/html/rfc5802#section-6. So
> I have tried to follow it, reviews are welcome particularly regarding
> that. The implementation is done in such a way that channel binding is
> used in the context of an SSL connection, which is what the RFCs
> expect from an implementation.

     After a deeper analysis, I have some concerns/comments here:

- tls-unique, as you mentioned, uses two undocumented APIs. This raises
a small flag about the stability and future of those APIs.

- More importantly, some drivers (not libpq-based) may have unexpected
difficulties implementing tls-unique. In particular, there is not an API
in Java to get the finished message. I expect other languages to maybe
have similar limitations. For Java I see two options:
     * Also implement tls-server-end-point, which rather relies on the
server certificate. This information seems to be exposed as part of the
Java APIs:
https://docs.oracle.com/javase/8/docs/api/java/security/cert/Certificate.html#getEncoded--
     * Use the BouncyCastle library (http://bouncycastle.org/), which
explicitly supports tls-unique
(https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/est/TLSUniqueProvider.html#getTLSUnique()
,
https://www.bouncycastle.org/docs/tlsdocs1.5on/org/bouncycastle/tls/ChannelBinding.html#tls_unique
). This would require, however, non-trivial changes to the driver and, I
expect, a lot of extra effort.

- It seems to me that tls-unique might be a bit fragile. In particular,
it requires the server to be aware of TSL renegotiations and avoid
performing one while the SCRAM authentication is being performed. I
don't know if this is already guaranteed by the current patch, but it
seems to me it requires complex interactions between levels of
abstraction that are quite separate (SSL and SCRAM). This is explained
by the RFC:

"
server applications MUST NOT request TLS renegotiation during phases of
the application protocol during which application-layer authentication
occurs
"
(https://tools.ietf.org/html/rfc5929#section-3.1)


     Based on this facts, I'd suggest to either implement
tls-server-end-point or implement both tls-server-end-point and
tls-unique. The latter would require a more complete protocol message to
advertise separately SCRAM mechanisms and channel binding names. One of
such structures could be the one I suggested earlier:
https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638fc8@8kdata.com


>
> Before even that, the server needs to send to the client the list of
> SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if
> the server has been built with SSL support to the list.

     And I'd say the list of channel binding names supported.


     Álvaro


--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
> On 25/05/17 17:17, Michael Paquier wrote:
>> Please find attached a patch to add support for channel binding for
>> SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802
>> (https://tools.ietf.org/html/rfc5802), servers supporting channel
>> binding need to add support for tls-unique, and this is what this
>> patch does.
>
>     This is awesome, Michael :) Thank you! You have prevented me eventually
> writing the patch and having then PostgreSQL source tainted with Java-to-C
> "transpiled" code ^_^

The thing would have been reviewed and rewritten a couple of times :)
So that would have unlikely (hopefully) finished in the shape of a
Java-C-like patch.

>     After a deeper analysis, I have some concerns/comments here:
>
> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> small flag about the stability and future of those APIs.

Those are 5 lines of code each in OpenSSL, not that hard to maintain.
Those APIs are clunky by the way as there is no way to know in advance
the length of the message for memory allocation.

> - More importantly, some drivers (not libpq-based) may have unexpected
> difficulties implementing tls-unique. In particular, there is not an API in
> Java to get the finished message. I expect other languages to maybe have
> similar limitations. For Java I see two options:
>     * Also implement tls-server-end-point, which rather relies on the server
> certificate. This information seems to be exposed as part of the Java APIs:
> https://docs.oracle.com/javase/8/docs/api/java/security/cert/Certificate.html#getEncoded--
>     * Use the BouncyCastle library (http://bouncycastle.org/), which
> explicitly supports tls-unique
> (https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/est/TLSUniqueProvider.html#getTLSUnique()
> ,
> https://www.bouncycastle.org/docs/tlsdocs1.5on/org/bouncycastle/tls/ChannelBinding.html#tls_unique
> ). This would require, however, non-trivial changes to the driver and, I
> expect, a lot of extra effort.

I am wondering how other languages are regarding that. Python has
added a method in 2011:
https://hg.python.org/cpython/rev/cb44fef5ea1d

> - It seems to me that tls-unique might be a bit fragile. In particular, it
> requires the server to be aware of TSL renegotiations and avoid performing
> one while the SCRAM authentication is being performed. I don't know if this
> is already guaranteed by the current patch, but it seems to me it requires
> complex interactions between levels of abstraction that are quite separate
> (SSL and SCRAM). This is explained by the RFC:

RFC 5802, section 6 gives a good level of details on the matter:
https://tools.ietf.org/html/rfc5802#section-6
There is indeed interaction between SSL and scram, and the patch makes
use of USE_SSL for this purpose.

> "
> server applications MUST NOT request TLS renegotiation during phases of the
> application protocol during which application-layer authentication occurs
> "
> (https://tools.ietf.org/html/rfc5929#section-3.1)

The SSL hanshake happens only once at connection obtention, before the
authentication exchange happens. So there is no renegociation. SSL
renegotiation has been removed in PG anyway, disabled on
back-branches, and removed as well in TLS 1.3 (right? I don't recall
the spec in details).

>     Based on this facts, I'd suggest to either implement
> tls-server-end-point or implement both tls-server-end-point and tls-unique.
> The latter would require a more complete protocol message to advertise
> separately SCRAM mechanisms and channel binding names. One of such
> structures could be the one I suggested earlier:
> https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638fc8@8kdata.com

The RFC says that any server implementing channel binding must
implement tls-unique, so having only end-point is not compliant.

>> Before even that, the server needs to send to the client the list of
>> SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if
>> the server has been built with SSL support to the list.
>
>     And I'd say the list of channel binding names supported.

 It seems to me as well that this gives us an indication that it
should be the default channel binding used, or if the exchange list
has no channel names included, the client can assume that tls-unique
will be used. Such an approach has as well the merit to keep the patch
simple. In short, I would advocate an incremental approach that adds
tls-unique first. This has value anyway as there is no need for
certificate configuration. This also does not require a message format
extension.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
> On 25/05/17 17:17, Michael Paquier wrote:
>> Please find attached a patch to add support for channel binding for
>> SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802
>> (https://tools.ietf.org/html/rfc5802), servers supporting channel
>> binding need to add support for tls-unique, and this is what this
>> patch does.
>
>     This is awesome, Michael :) Thank you! You have prevented me eventually
> writing the patch and having then PostgreSQL source tainted with Java-to-C
> "transpiled" code ^_^

The thing would have been reviewed and rewritten a couple of times :)
So that would have unlikely (hopefully) finished in the shape of a
Java-C-like patch.

>     After a deeper analysis, I have some concerns/comments here:
>
> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> small flag about the stability and future of those APIs.

Those are 5 lines of code each in OpenSSL, not that hard to maintain.
Those APIs are clunky by the way as there is no way to know in advance
the length of the message for memory allocation.

> - More importantly, some drivers (not libpq-based) may have unexpected
> difficulties implementing tls-unique. In particular, there is not an API in
> Java to get the finished message. I expect other languages to maybe have
> similar limitations. For Java I see two options:
>     * Also implement tls-server-end-point, which rather relies on the server
> certificate. This information seems to be exposed as part of the Java APIs:
> https://docs.oracle.com/javase/8/docs/api/java/security/cert/Certificate.html#getEncoded--
>     * Use the BouncyCastle library (http://bouncycastle.org/), which
> explicitly supports tls-unique
> (https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/est/TLSUniqueProvider.html#getTLSUnique()
> ,
> https://www.bouncycastle.org/docs/tlsdocs1.5on/org/bouncycastle/tls/ChannelBinding.html#tls_unique
> ). This would require, however, non-trivial changes to the driver and, I
> expect, a lot of extra effort.

I am wondering how other languages are regarding that. Python has
added a method in 2011:
https://hg.python.org/cpython/rev/cb44fef5ea1d

> - It seems to me that tls-unique might be a bit fragile. In particular, it
> requires the server to be aware of TSL renegotiations and avoid performing
> one while the SCRAM authentication is being performed. I don't know if this
> is already guaranteed by the current patch, but it seems to me it requires
> complex interactions between levels of abstraction that are quite separate
> (SSL and SCRAM). This is explained by the RFC:

RFC 5802, section 6 gives a good level of details on the matter:
https://tools.ietf.org/html/rfc5802#section-6
There is indeed interaction between SSL and scram, and the patch makes
use of USE_SSL for this purpose.

> "
> server applications MUST NOT request TLS renegotiation during phases of the
> application protocol during which application-layer authentication occurs
> "
> (https://tools.ietf.org/html/rfc5929#section-3.1)

The SSL hanshake happens only once at connection obtention, before the
authentication exchange happens. So there is no renegociation. SSL
renegotiation has been removed in PG anyway, disabled on
back-branches, and removed as well in TLS 1.3 (right? I don't recall
the spec in details).

>     Based on this facts, I'd suggest to either implement
> tls-server-end-point or implement both tls-server-end-point and tls-unique.
> The latter would require a more complete protocol message to advertise
> separately SCRAM mechanisms and channel binding names. One of such
> structures could be the one I suggested earlier:
> https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638fc8@8kdata.com

The RFC says that any server implementing channel binding must
implement tls-unique, so having only end-point is not compliant.

>> Before even that, the server needs to send to the client the list of
>> SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if
>> the server has been built with SSL support to the list.
>
>     And I'd say the list of channel binding names supported.

 It seems to me as well that this gives us an indication that it
should be the default channel binding used, or if the exchange list
has no channel names included, the client can assume that tls-unique
will be used. Such an approach has as well the merit to keep the patch
simple. In short, I would advocate an incremental approach that adds
tls-unique first. This has value anyway as there is no need for
certificate configuration. This also does not require a message format
extension.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> small flag about the stability and future of those APIs.

It seems to me that the question is not just whether those APIs will
be available in future versions of OpenSSL, but whether they will be
available in every current and future version of every SSL
implementation that we may wish to use in core or that any client may
wish to use.  We've talked before about being able to use the Windows
native SSL implementation rather than OpenSSL and it seems that there
would be significant advantages in having that capability.  Meanwhile,
a client that reimplements the PostgreSQL wire protocol is free to use
any SSL implementation it likes.  If channel binding can't work unless
both sides are speaking OpenSSL, then I'd say we've blown it.

Also, Heikki pointed out in his PGCon talk that there's currently no
way for libpq to insist on the use of SCRAM rather than plain password
authentication or md5.  So, if someone trojans the server, they only
need to hack it to ask the client for plaintext authentication rather
than SCRAM and the client will cheerfully hand over the password with
no questions asked.  Presumably, we need to add a connection option to
insist (a) on SCRAM or (b) SCRAM + channel binding, or else this isn't
going to actually provide us with any increased security.  Even
without that, channel binding will still let the server verify that
it's really connected to the client, but that's not really doing much
for us in terms of security because the client (who has the password)
can always let someone else impersonate it perfectly by just handing
over the password.  Channel binding can't prevent that.  It *could*
prevent someone from impersonating the *server* but not if the server
can disable the check whenever it likes by ceasing to advertise
channel binding as an available option -- with no notification to the
client that anything has changed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> small flag about the stability and future of those APIs.

It seems to me that the question is not just whether those APIs will
be available in future versions of OpenSSL, but whether they will be
available in every current and future version of every SSL
implementation that we may wish to use in core or that any client may
wish to use.  We've talked before about being able to use the Windows
native SSL implementation rather than OpenSSL and it seems that there
would be significant advantages in having that capability.  Meanwhile,
a client that reimplements the PostgreSQL wire protocol is free to use
any SSL implementation it likes.  If channel binding can't work unless
both sides are speaking OpenSSL, then I'd say we've blown it.

Also, Heikki pointed out in his PGCon talk that there's currently no
way for libpq to insist on the use of SCRAM rather than plain password
authentication or md5.  So, if someone trojans the server, they only
need to hack it to ask the client for plaintext authentication rather
than SCRAM and the client will cheerfully hand over the password with
no questions asked.  Presumably, we need to add a connection option to
insist (a) on SCRAM or (b) SCRAM + channel binding, or else this isn't
going to actually provide us with any increased security.  Even
without that, channel binding will still let the server verify that
it's really connected to the client, but that's not really doing much
for us in terms of security because the client (who has the password)
can always let someone else impersonate it perfectly by just handing
over the password.  Channel binding can't prevent that.  It *could*
prevent someone from impersonating the *server* but not if the server
can disable the check whenever it likes by ceasing to advertise
channel binding as an available option -- with no notification to the
client that anything has changed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
> <aht@8kdata.com> wrote:
>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>> small flag about the stability and future of those APIs.

> It seems to me that the question is not just whether those APIs will
> be available in future versions of OpenSSL, but whether they will be
> available in every current and future version of every SSL
> implementation that we may wish to use in core or that any client may
> wish to use.  We've talked before about being able to use the Windows
> native SSL implementation rather than OpenSSL and it seems that there
> would be significant advantages in having that capability.

Another thing of the same sort that should be on our radar is making
use of Apple's TLS code on macOS.  The handwriting on the wall is
unmistakable that they intend to stop shipping OpenSSL before long,
and I do not think we really want to be in a position of having to
bundle OpenSSL into our distribution on macOS.

I'm not volunteering to do that, mind you.  But +1 for not tying new
features to any single TLS implementation.

            regards, tom lane


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
> <aht@8kdata.com> wrote:
>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>> small flag about the stability and future of those APIs.

> It seems to me that the question is not just whether those APIs will
> be available in future versions of OpenSSL, but whether they will be
> available in every current and future version of every SSL
> implementation that we may wish to use in core or that any client may
> wish to use.  We've talked before about being able to use the Windows
> native SSL implementation rather than OpenSSL and it seems that there
> would be significant advantages in having that capability.

Another thing of the same sort that should be on our radar is making
use of Apple's TLS code on macOS.  The handwriting on the wall is
unmistakable that they intend to stop shipping OpenSSL before long,
and I do not think we really want to be in a position of having to
bundle OpenSSL into our distribution on macOS.

I'm not volunteering to do that, mind you.  But +1 for not tying new
features to any single TLS implementation.

            regards, tom lane


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
> > <aht@8kdata.com> wrote:
> >> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> >> small flag about the stability and future of those APIs.
>
> > It seems to me that the question is not just whether those APIs will
> > be available in future versions of OpenSSL, but whether they will be
> > available in every current and future version of every SSL
> > implementation that we may wish to use in core or that any client may
> > wish to use.  We've talked before about being able to use the Windows
> > native SSL implementation rather than OpenSSL and it seems that there
> > would be significant advantages in having that capability.
>
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
>
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Work has been done in that area already, as well as for LibNSS support.

I've not had a chance to closely look over the proposed approach here,
but generally speaking, we need to be looking for a standard way to
generate and transmit the channel binding information across the
wire.  The specific APIs are, certainly, going to be different between
different TLS implementations and I don't believe we need to be
particularly concerned with that (or if they change in the future), as
long as we abstract those APIs.  Perhaps there's some question as to
just how to abstract them, but I'm at least a bit less concerned with
that as I expect we'll be able to adjust those abstraction layers later
(presuming they aren't exposed, which I wouldn't expect them to be).

I skimmed over RFC5929, which looks to be the correct RFC when it comes
to channel binding with TLS.  Hopefully the various TLS implementations
work in a similar manner, following that RFC (or whichever is relevant).

If that wasn't the case then, for example, it wouldn't be possible to do
channel binding with a LibNSS client and an OpenSSL server or with other
combinations and I find that rather hard to believe.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
> > <aht@8kdata.com> wrote:
> >> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
> >> small flag about the stability and future of those APIs.
>
> > It seems to me that the question is not just whether those APIs will
> > be available in future versions of OpenSSL, but whether they will be
> > available in every current and future version of every SSL
> > implementation that we may wish to use in core or that any client may
> > wish to use.  We've talked before about being able to use the Windows
> > native SSL implementation rather than OpenSSL and it seems that there
> > would be significant advantages in having that capability.
>
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
>
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Work has been done in that area already, as well as for LibNSS support.

I've not had a chance to closely look over the proposed approach here,
but generally speaking, we need to be looking for a standard way to
generate and transmit the channel binding information across the
wire.  The specific APIs are, certainly, going to be different between
different TLS implementations and I don't believe we need to be
particularly concerned with that (or if they change in the future), as
long as we abstract those APIs.  Perhaps there's some question as to
just how to abstract them, but I'm at least a bit less concerned with
that as I expect we'll be able to adjust those abstraction layers later
(presuming they aren't exposed, which I wouldn't expect them to be).

I skimmed over RFC5929, which looks to be the correct RFC when it comes
to channel binding with TLS.  Hopefully the various TLS implementations
work in a similar manner, following that RFC (or whichever is relevant).

If that wasn't the case then, for example, it wouldn't be possible to do
channel binding with a LibNSS client and an OpenSSL server or with other
combinations and I find that rather hard to believe.

Thanks!

Stephen

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, May 30, 2017 at 8:14 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Work has been done in that area already, as well as for LibNSS support.
>
> I've not had a chance to closely look over the proposed approach here,
> but generally speaking, we need to be looking for a standard way to
> generate and transmit the channel binding information across the
> wire.

The RFC of SCRAM specifies that the client sends the type of channel
binding in its first message data, and then provides the contents of
the TLS message it generated in the challenge response.

> The specific APIs are, certainly, going to be different between
> different TLS implementations and I don't believe we need to be
> particularly concerned with that (or if they change in the future), as
> long as we abstract those APIs.  Perhaps there's some question as to
> just how to abstract them, but I'm at least a bit less concerned with
> that as I expect we'll be able to adjust those abstraction layers later
> (presuming they aren't exposed, which I wouldn't expect them to be).

The current patch fetches the TLS finish message data just after the
handshake in be_tls_open_server() using the dedicated OpenSSL API. We
could definitely go with a more generic routine on our side that
sticks with the concepts of be_tls_get_compression():
- For the backend, this routine returns an allocated string with the
contents of the expected TLS message, and its size:
char *be_tls_get_tls_finish(Port *, int *)
- For the frontend, a routine to get the generated TLS finish message:
char *pgtls_get_tls_finish(PGconn *, int *)

> I skimmed over RFC5929, which looks to be the correct RFC when it comes
> to channel binding with TLS.  Hopefully the various TLS implementations
> work in a similar manner, following that RFC (or whichever is relevant).

That's the one I use as a reference.

> If that wasn't the case then, for example, it wouldn't be possible to do
> channel binding with a LibNSS client and an OpenSSL server or with other
> combinations and I find that rather hard to believe.

As far as I can see, Windows has some APIs to get the TLS finish message:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa382420(v=vs.85).aspx
On macos though it is another story, I am not seeing anything:
https://developer.apple.com/reference/security/secure_transport#symbols

Depending on the SSL implementation the server is compiled with, it
will be up to the backend to decide if it should advertise to the
client the -PLUS mechanism or not, so we can stay modular here.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, May 30, 2017 at 8:14 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Work has been done in that area already, as well as for LibNSS support.
>
> I've not had a chance to closely look over the proposed approach here,
> but generally speaking, we need to be looking for a standard way to
> generate and transmit the channel binding information across the
> wire.

The RFC of SCRAM specifies that the client sends the type of channel
binding in its first message data, and then provides the contents of
the TLS message it generated in the challenge response.

> The specific APIs are, certainly, going to be different between
> different TLS implementations and I don't believe we need to be
> particularly concerned with that (or if they change in the future), as
> long as we abstract those APIs.  Perhaps there's some question as to
> just how to abstract them, but I'm at least a bit less concerned with
> that as I expect we'll be able to adjust those abstraction layers later
> (presuming they aren't exposed, which I wouldn't expect them to be).

The current patch fetches the TLS finish message data just after the
handshake in be_tls_open_server() using the dedicated OpenSSL API. We
could definitely go with a more generic routine on our side that
sticks with the concepts of be_tls_get_compression():
- For the backend, this routine returns an allocated string with the
contents of the expected TLS message, and its size:
char *be_tls_get_tls_finish(Port *, int *)
- For the frontend, a routine to get the generated TLS finish message:
char *pgtls_get_tls_finish(PGconn *, int *)

> I skimmed over RFC5929, which looks to be the correct RFC when it comes
> to channel binding with TLS.  Hopefully the various TLS implementations
> work in a similar manner, following that RFC (or whichever is relevant).

That's the one I use as a reference.

> If that wasn't the case then, for example, it wouldn't be possible to do
> channel binding with a LibNSS client and an OpenSSL server or with other
> combinations and I find that rather hard to believe.

As far as I can see, Windows has some APIs to get the TLS finish message:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa382420(v=vs.85).aspx
On macos though it is another story, I am not seeing anything:
https://developer.apple.com/reference/security/secure_transport#symbols

Depending on the SSL implementation the server is compiled with, it
will be up to the backend to decide if it should advertise to the
client the -PLUS mechanism or not, so we can stay modular here.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, May 30, 2017 at 8:14 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > The specific APIs are, certainly, going to be different between
> > different TLS implementations and I don't believe we need to be
> > particularly concerned with that (or if they change in the future), as
> > long as we abstract those APIs.  Perhaps there's some question as to
> > just how to abstract them, but I'm at least a bit less concerned with
> > that as I expect we'll be able to adjust those abstraction layers later
> > (presuming they aren't exposed, which I wouldn't expect them to be).
>
> The current patch fetches the TLS finish message data just after the
> handshake in be_tls_open_server() using the dedicated OpenSSL API. We
> could definitely go with a more generic routine on our side that
> sticks with the concepts of be_tls_get_compression():
> - For the backend, this routine returns an allocated string with the
> contents of the expected TLS message, and its size:
> char *be_tls_get_tls_finish(Port *, int *)
> - For the frontend, a routine to get the generated TLS finish message:
> char *pgtls_get_tls_finish(PGconn *, int *)

Those look pretty reasonable to me and seem to go along with the
concepts from the RFC, making them hopefully the right API.

> > If that wasn't the case then, for example, it wouldn't be possible to do
> > channel binding with a LibNSS client and an OpenSSL server or with other
> > combinations and I find that rather hard to believe.
>
> As far as I can see, Windows has some APIs to get the TLS finish message:
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa382420(v=vs.85).aspx

Good.

> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

That's a bit unfortunate, if that's the case.  Perhaps Apple will see
fit to address that deficiency though.

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

Right, of course.

All-in-all, this sounds like it's heading in the right direction, at
least at a high level.  Glad to see that there's been consideration of
other TLS implementations, and as such I don't think we need to be
overly concerned about the specifics of the OpenSSL API here.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, May 30, 2017 at 8:14 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > The specific APIs are, certainly, going to be different between
> > different TLS implementations and I don't believe we need to be
> > particularly concerned with that (or if they change in the future), as
> > long as we abstract those APIs.  Perhaps there's some question as to
> > just how to abstract them, but I'm at least a bit less concerned with
> > that as I expect we'll be able to adjust those abstraction layers later
> > (presuming they aren't exposed, which I wouldn't expect them to be).
>
> The current patch fetches the TLS finish message data just after the
> handshake in be_tls_open_server() using the dedicated OpenSSL API. We
> could definitely go with a more generic routine on our side that
> sticks with the concepts of be_tls_get_compression():
> - For the backend, this routine returns an allocated string with the
> contents of the expected TLS message, and its size:
> char *be_tls_get_tls_finish(Port *, int *)
> - For the frontend, a routine to get the generated TLS finish message:
> char *pgtls_get_tls_finish(PGconn *, int *)

Those look pretty reasonable to me and seem to go along with the
concepts from the RFC, making them hopefully the right API.

> > If that wasn't the case then, for example, it wouldn't be possible to do
> > channel binding with a LibNSS client and an OpenSSL server or with other
> > combinations and I find that rather hard to believe.
>
> As far as I can see, Windows has some APIs to get the TLS finish message:
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa382420(v=vs.85).aspx

Good.

> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

That's a bit unfortunate, if that's the case.  Perhaps Apple will see
fit to address that deficiency though.

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

Right, of course.

All-in-all, this sounds like it's heading in the right direction, at
least at a high level.  Glad to see that there's been consideration of
other TLS implementations, and as such I don't think we need to be
overly concerned about the specifics of the OpenSSL API here.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Daniel Gustafsson
Date:
> On 30 May 2017, at 18:25, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

The Secure Transport documentation unfortunately excel at being incomplete and
hard to search in.  That being said, I think you’re right, AFAICT there is no
published API for this (SSLProcessFinished() seems like the best match but is
not public).

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

+1

cheers ./daniel

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Daniel Gustafsson
Date:
> On 30 May 2017, at 18:25, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On macos though it is another story, I am not seeing anything:
> https://developer.apple.com/reference/security/secure_transport#symbols

The Secure Transport documentation unfortunately excel at being incomplete and
hard to search in.  That being said, I think you’re right, AFAICT there is no
published API for this (SSLProcessFinished() seems like the best match but is
not public).

> Depending on the SSL implementation the server is compiled with, it
> will be up to the backend to decide if it should advertise to the
> client the -PLUS mechanism or not, so we can stay modular here.

+1

cheers ./daniel

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Daniel Gustafsson
Date:
> On 30 May 2017, at 16:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
>> <aht@8kdata.com> wrote:
>>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>>> small flag about the stability and future of those APIs.
>
>> It seems to me that the question is not just whether those APIs will
>> be available in future versions of OpenSSL, but whether they will be
>> available in every current and future version of every SSL
>> implementation that we may wish to use in core or that any client may
>> wish to use.  We've talked before about being able to use the Windows
>> native SSL implementation rather than OpenSSL and it seems that there
>> would be significant advantages in having that capability.
>
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
>
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Big +1.  The few settings we have already make it hard to provide other
implementations as drop-in replacements (Secure Transport doesn’t support
.crl files for example, only CRL loaded in Keychains).

cheers ./daniel

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Daniel Gustafsson
Date:
> On 30 May 2017, at 16:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa
>> <aht@8kdata.com> wrote:
>>> - tls-unique, as you mentioned, uses two undocumented APIs. This raises a
>>> small flag about the stability and future of those APIs.
>
>> It seems to me that the question is not just whether those APIs will
>> be available in future versions of OpenSSL, but whether they will be
>> available in every current and future version of every SSL
>> implementation that we may wish to use in core or that any client may
>> wish to use.  We've talked before about being able to use the Windows
>> native SSL implementation rather than OpenSSL and it seems that there
>> would be significant advantages in having that capability.
>
> Another thing of the same sort that should be on our radar is making
> use of Apple's TLS code on macOS.  The handwriting on the wall is
> unmistakable that they intend to stop shipping OpenSSL before long,
> and I do not think we really want to be in a position of having to
> bundle OpenSSL into our distribution on macOS.
>
> I'm not volunteering to do that, mind you.  But +1 for not tying new
> features to any single TLS implementation.

Big +1.  The few settings we have already make it hard to provide other
implementations as drop-in replacements (Secure Transport doesn’t support
.crl files for example, only CRL loaded in Keychains).

cheers ./daniel

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, May 30, 2017 at 1:00 PM, Stephen Frost <sfrost@snowman.net> wrote:
> All-in-all, this sounds like it's heading in the right direction, at
> least at a high level.  Glad to see that there's been consideration of
> other TLS implementations, and as such I don't think we need to be
> overly concerned about the specifics of the OpenSSL API here.

That sounds like undue optimism to me.  Unless somebody's tested that
Michael's proposed implementation, which uses undocumented OpenSSL
APIs, actually interoperates properly with a SCRAM + channel binding
implementation based on some other underlying SSL implementation, we
can't really know that it's going to work.  It's not like we're
calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
We're calling SSL_do_something_undocumented() and hoping that
something_undocumented ==
the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
but without actual interoperability testing it sounds pretty
speculative to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, May 30, 2017 at 1:00 PM, Stephen Frost <sfrost@snowman.net> wrote:
> All-in-all, this sounds like it's heading in the right direction, at
> least at a high level.  Glad to see that there's been consideration of
> other TLS implementations, and as such I don't think we need to be
> overly concerned about the specifics of the OpenSSL API here.

That sounds like undue optimism to me.  Unless somebody's tested that
Michael's proposed implementation, which uses undocumented OpenSSL
APIs, actually interoperates properly with a SCRAM + channel binding
implementation based on some other underlying SSL implementation, we
can't really know that it's going to work.  It's not like we're
calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
We're calling SSL_do_something_undocumented() and hoping that
something_undocumented ==
the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
but without actual interoperability testing it sounds pretty
speculative to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, May 30, 2017 at 5:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> That sounds like undue optimism to me.  Unless somebody's tested that
> Michael's proposed implementation, which uses undocumented OpenSSL
> APIs, actually interoperates properly with a SCRAM + channel binding
> implementation based on some other underlying SSL implementation, we
> can't really know that it's going to work.  It's not like we're
> calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
> We're calling SSL_do_something_undocumented() and hoping that
> something_undocumented ==
> the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
> but without actual interoperability testing it sounds pretty
> speculative to me.

Hm? Python is using that stuff for a certain amount of years now, for
the same goal of providing channel binding for SSL authentication. You
can look at this thread:
https://bugs.python.org/issue12551
So qualifying that as a speculative method sounds a quite hard word to me.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, May 30, 2017 at 5:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> That sounds like undue optimism to me.  Unless somebody's tested that
> Michael's proposed implementation, which uses undocumented OpenSSL
> APIs, actually interoperates properly with a SCRAM + channel binding
> implementation based on some other underlying SSL implementation, we
> can't really know that it's going to work.  It's not like we're
> calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
> We're calling SSL_do_something_undocumented() and hoping that
> something_undocumented ==
> the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
> but without actual interoperability testing it sounds pretty
> speculative to me.

Hm? Python is using that stuff for a certain amount of years now, for
the same goal of providing channel binding for SSL authentication. You
can look at this thread:
https://bugs.python.org/issue12551
So qualifying that as a speculative method sounds a quite hard word to me.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 31/05/17 03:39, Michael Paquier wrote:
> On Tue, May 30, 2017 at 5:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> That sounds like undue optimism to me.  Unless somebody's tested that
>> Michael's proposed implementation, which uses undocumented OpenSSL
>> APIs, actually interoperates properly with a SCRAM + channel binding
>> implementation based on some other underlying SSL implementation, we
>> can't really know that it's going to work.  It's not like we're
>> calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
>> We're calling SSL_do_something_undocumented() and hoping that
>> something_undocumented ==
>> the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
>> but without actual interoperability testing it sounds pretty
>> speculative to me.
> Hm? Python is using that stuff for a certain amount of years now, for
> the same goal of providing channel binding for SSL authentication. You
> can look at this thread:
> https://bugs.python.org/issue12551
> So qualifying that as a speculative method sounds a quite hard word to me.

     Actually this Python patch seems to ultimately rely on the same
OpenSSL functions as your implementation.

     I don't have anything against implementing tls-unique, specially as
per the RFC it is mandatory (if channel binding support is provided).
However, given the use of undocumented API, given that at least the Java
driver would have a very difficult time implementing it (basically
replacing Java's standard SSL stack with a completely external one,
BouncyCastle, which adds load, size and complexity), I would rather
focus the efforts on tls-server-end-point which only needs to access the
certificate data, something that most APIs/frameworks/languages seem to
cover much more naturally than tls-unique.

     Both are equally safe and effective and so having support for both
seems sensible to me.


     Álvaro

--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 31/05/17 03:39, Michael Paquier wrote:
> On Tue, May 30, 2017 at 5:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> That sounds like undue optimism to me.  Unless somebody's tested that
>> Michael's proposed implementation, which uses undocumented OpenSSL
>> APIs, actually interoperates properly with a SCRAM + channel binding
>> implementation based on some other underlying SSL implementation, we
>> can't really know that it's going to work.  It's not like we're
>> calling SSL_do_the_right_thing_for_channel_binding_thing_per_rfc5929().
>> We're calling SSL_do_something_undocumented() and hoping that
>> something_undocumented ==
>> the_right_thing_for_channel_binding_thing_per_rfc5929.  Could be true,
>> but without actual interoperability testing it sounds pretty
>> speculative to me.
> Hm? Python is using that stuff for a certain amount of years now, for
> the same goal of providing channel binding for SSL authentication. You
> can look at this thread:
> https://bugs.python.org/issue12551
> So qualifying that as a speculative method sounds a quite hard word to me.

     Actually this Python patch seems to ultimately rely on the same
OpenSSL functions as your implementation.

     I don't have anything against implementing tls-unique, specially as
per the RFC it is mandatory (if channel binding support is provided).
However, given the use of undocumented API, given that at least the Java
driver would have a very difficult time implementing it (basically
replacing Java's standard SSL stack with a completely external one,
BouncyCastle, which adds load, size and complexity), I would rather
focus the efforts on tls-server-end-point which only needs to access the
certificate data, something that most APIs/frameworks/languages seem to
cover much more naturally than tls-unique.

     Both are equally safe and effective and so having support for both
seems sensible to me.


     Álvaro

--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> but without actual interoperability testing it sounds pretty
> speculative to me.

I'm all for interoperability testing.

When we have multiple implementations of TLS using different libraries
with various versions of PostgreSQL and libpq and are able to test those
against other versions of PostgreSQL and libpq compiled with other TLS
libraries, I'll be downright ecstatic.  We are a small ways from that
right now, however, and I don't believe that we should be asking the
implementors of channel binding to also implement support for multiple
TLS libraries in PostgreSQL in order to test that their RFC-following
(at least, as far as they can tell) implementation actually works.

I'm not exactly sure what to characterize that as, given that the old
fall-back of "feature creep" feels woefully inadequate as a description.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> but without actual interoperability testing it sounds pretty
> speculative to me.

I'm all for interoperability testing.

When we have multiple implementations of TLS using different libraries
with various versions of PostgreSQL and libpq and are able to test those
against other versions of PostgreSQL and libpq compiled with other TLS
libraries, I'll be downright ecstatic.  We are a small ways from that
right now, however, and I don't believe that we should be asking the
implementors of channel binding to also implement support for multiple
TLS libraries in PostgreSQL in order to test that their RFC-following
(at least, as far as they can tell) implementation actually works.

I'm not exactly sure what to characterize that as, given that the old
fall-back of "feature creep" feels woefully inadequate as a description.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> ... and I don't believe that we should be asking the
> implementors of channel binding to also implement support for multiple
> TLS libraries in PostgreSQL in order to test that their RFC-following
> (at least, as far as they can tell) implementation actually works.

You're of course free to believe what you wish, but that sounds
short-sighted to me.  If we implement channel binding and it turns out
not to be interoperable with other SSL implementations, then what?  We
can't change it later without breaking compatibility with our own
prior implementation.  Note that Álvaro Hernández Tortosa said about
two hours before you sent this email that it doesn't seem possible to
implement something comparable in Java's standard SSL stack.  If
that's the case, adopting this implementation is dooming everyone who
connects to the database server using JDBC to be unable to use channel
binding.  And that's a large percentage of our user base.

And then what happens when we do add support for Windows SSL or macOS
SSL?  It sounds like you're equally willing to throw people using
those implementations under the bus.  So we'll end up with channel
binding that works only when everybody's running the same operating
system and nobody's using Java.  Uggh.  At that point you might as
well just go back to using SSL certificates to solve this problem.  If
you use SSL certificates, then (1) it should work with any SSL
implementation and (2) the client can force SSL certificate
verification, whereas currently the client cannot force even the use
of SCRAM, let alone channel binding.

So what is being proposed here does not (yet, anyway) provide any
actual security and seems to have poor prospects for interoperability,
whereas we have an existing feature (SSL certificates) that has
neither of those problems.  Are you sure you're not putting
buzzword-compliance ahead of real progress?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> ... and I don't believe that we should be asking the
> implementors of channel binding to also implement support for multiple
> TLS libraries in PostgreSQL in order to test that their RFC-following
> (at least, as far as they can tell) implementation actually works.

You're of course free to believe what you wish, but that sounds
short-sighted to me.  If we implement channel binding and it turns out
not to be interoperable with other SSL implementations, then what?  We
can't change it later without breaking compatibility with our own
prior implementation.  Note that Álvaro Hernández Tortosa said about
two hours before you sent this email that it doesn't seem possible to
implement something comparable in Java's standard SSL stack.  If
that's the case, adopting this implementation is dooming everyone who
connects to the database server using JDBC to be unable to use channel
binding.  And that's a large percentage of our user base.

And then what happens when we do add support for Windows SSL or macOS
SSL?  It sounds like you're equally willing to throw people using
those implementations under the bus.  So we'll end up with channel
binding that works only when everybody's running the same operating
system and nobody's using Java.  Uggh.  At that point you might as
well just go back to using SSL certificates to solve this problem.  If
you use SSL certificates, then (1) it should work with any SSL
implementation and (2) the client can force SSL certificate
verification, whereas currently the client cannot force even the use
of SCRAM, let alone channel binding.

So what is being proposed here does not (yet, anyway) provide any
actual security and seems to have poor prospects for interoperability,
whereas we have an existing feature (SSL certificates) that has
neither of those problems.  Are you sure you're not putting
buzzword-compliance ahead of real progress?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, May 30, 2017 at 6:50 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
>     Actually this Python patch seems to ultimately rely on the same OpenSSL
> functions as your implementation.

Yup. My point is that even if those APIs are undocumented, I think
that they are not going to disappear either.

>     I don't have anything against implementing tls-unique, specially as per
> the RFC it is mandatory (if channel binding support is provided). However,
> given the use of undocumented API, given that at least the Java driver would
> have a very difficult time implementing it (basically replacing Java's
> standard SSL stack with a completely external one, BouncyCastle, which adds
> load, size and complexity), I would rather focus the efforts on
> tls-server-end-point which only needs to access the certificate data,
> something that most APIs/frameworks/languages seem to cover much more
> naturally than tls-unique.

Point taken. Well, this brings us to the point where we should have
both tls-unique and end-point to allow JDBC to work with it. Now,
thinking about it, do we really need to advertise the available
channel binding types when the client chooses the -PLUS mechanism?
Wouldn't it make sense to let the client choose what it thinks is
better and just fail the exchange with the backend if the channel type
is not supported?

>     Both are equally safe and effective and so having support for both seems
> sensible to me.

I have some automatic setups that would be really simplified by just
using libpq with SSL connections if there is channel binding. And that
involves certificate deployments, I think I am not the only one to see
that present even if JDBC just supports end-point.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, May 30, 2017 at 6:50 PM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
>     Actually this Python patch seems to ultimately rely on the same OpenSSL
> functions as your implementation.

Yup. My point is that even if those APIs are undocumented, I think
that they are not going to disappear either.

>     I don't have anything against implementing tls-unique, specially as per
> the RFC it is mandatory (if channel binding support is provided). However,
> given the use of undocumented API, given that at least the Java driver would
> have a very difficult time implementing it (basically replacing Java's
> standard SSL stack with a completely external one, BouncyCastle, which adds
> load, size and complexity), I would rather focus the efforts on
> tls-server-end-point which only needs to access the certificate data,
> something that most APIs/frameworks/languages seem to cover much more
> naturally than tls-unique.

Point taken. Well, this brings us to the point where we should have
both tls-unique and end-point to allow JDBC to work with it. Now,
thinking about it, do we really need to advertise the available
channel binding types when the client chooses the -PLUS mechanism?
Wouldn't it make sense to let the client choose what it thinks is
better and just fail the exchange with the backend if the channel type
is not supported?

>     Both are equally safe and effective and so having support for both seems
> sensible to me.

I have some automatic setups that would be really simplified by just
using libpq with SSL connections if there is channel binding. And that
involves certificate deployments, I think I am not the only one to see
that present even if JDBC just supports end-point.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > ... and I don't believe that we should be asking the
> > implementors of channel binding to also implement support for multiple
> > TLS libraries in PostgreSQL in order to test that their RFC-following
> > (at least, as far as they can tell) implementation actually works.
>
> You're of course free to believe what you wish, but that sounds
> short-sighted to me.  If we implement channel binding and it turns out
> not to be interoperable with other SSL implementations, then what?  We
> can't change it later without breaking compatibility with our own
> prior implementation.  Note that Álvaro Hernández Tortosa said about
> two hours before you sent this email that it doesn't seem possible to
> implement something comparable in Java's standard SSL stack.  If
> that's the case, adopting this implementation is dooming everyone who
> connects to the database server using JDBC to be unable to use channel
> binding.  And that's a large percentage of our user base.

I'm hopeful that we're closer to agreement here than we are the
opposite.

It was absolutely not my intent that the ongoing discussion between
Alvaro and Michael be stopped or changed, quite the opposite, in fact.

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we need to make sure that the JDBC driver is able to work with the
chosen solution (or, perhaps, that we support multuple options, one of
which works with the JDBC changes), and that we review the other TLS
libraries with the goal of making sure that what we agree on in core
should work with those also, then that's great and exactly what I'd like
to see also.

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we must have a complete alternative TLS solution, while that would
actually play a great deal to a goal I've had for a while to have PG
support multiple TLS implementations (LibNSS being top-of-mind, at least
for me, as I've mentioned a couple times), I can't agree that we should
require that before accepting channel binding as a feature.  To do so
would be akin to requiring that we support multiple TLS implementations
before we agreed to support client-side certificates, in my opinion.

I would be rather surprised if the solution which Michael and Alvaro
come to results in a solution which requires us to break compatibility
down the road, though it's of course a risk we need to consider.  The
ongoing discussion around finding a way to support both methods outlined
in the RFC sounds exactly correct to me and I encourage further
discussion there.

> And then what happens when we do add support for Windows SSL or macOS
> SSL?  It sounds like you're equally willing to throw people using
> those implementations under the bus.  So we'll end up with channel
> binding that works only when everybody's running the same operating
> system and nobody's using Java.  Uggh.  At that point you might as
> well just go back to using SSL certificates to solve this problem.  If
> you use SSL certificates, then (1) it should work with any SSL
> implementation and (2) the client can force SSL certificate
> verification, whereas currently the client cannot force even the use
> of SCRAM, let alone channel binding.

I hope it's clear that it's not my intent to throw anyone "under the
bus."  The only reason that "SSL certificates should work with any SSL
implementation" is because those SSL implementations are based on RFCs
which define how they should work and what I was encouraging up-thread
is exactly that we should be looking at RFCs to determine the right path
forward.  There are cases, of course, where the RFCs have alternatives
and the ideal approach is to find a way to work with all of those
alternatives, or at least implement a solution which is flexible, such
that later changes could add support without breaking existing users.

I'm certainly on-board with the general idea of having a way for the
client to require both SCRAM and channel binding and I agree that's a
hole in our current system, but that is really an orthogonal concern.

> So what is being proposed here does not (yet, anyway) provide any
> actual security and seems to have poor prospects for interoperability,
> whereas we have an existing feature (SSL certificates) that has
> neither of those problems.  Are you sure you're not putting
> buzzword-compliance ahead of real progress?

Adding support for channel binding is quite important and valuable, in
its own right.  I concur that we want to provide ways for the client to
require SCRAM and to require channel binding, but I don't see any
particular reason that adding support for channel binding has to be held
up behind that.

As for your question, I have to say that I find it inappropriate to
characterize channel binding as a "buzzword" or to suggest that this
particular feature which Michael and Alvaro are working to add to PG is
only being done to fulfill some marketing requirement or checkbox but
without adding any technical merit in its own right.  Channel binding
isn't new, it's supported by quite a few different technologies, and we
would absolutely be better off including support for it, from an
entirely technical perspective.  If I were one of the individuals
working on this feature, I'd be rather put-off and upset at the
suggestion that the good work they're doing is viewed by the PostgreSQL
community and one of our major committers as only being done to check a
box or to be "buzzword compliant" and ultimately without technical
merit.

Thankfully, I know both Michael and Alvaro and had excellent discussions
with them at PGCon last week, and I don't doubt that they will continue
their efforts regardless, but I worry about the signal it sends to
individuals who are not yet contributors or who have not gone through
the process of trying to contribute to PostgreSQL.  We make it more than
difficult enough to get code into core, let's try to avoid discouraging
contributors by casting doubt on the technical merits of their efforts
when they're very clearly adding a useful feature, even if it isn't
perfect until we also add X, Y or Z.  Instead, let's encourage their
efforts to complete the work they've started and then work with them to
add those other components necessary to have a complete solution.

Thanks!

Stephen

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > ... and I don't believe that we should be asking the
> > implementors of channel binding to also implement support for multiple
> > TLS libraries in PostgreSQL in order to test that their RFC-following
> > (at least, as far as they can tell) implementation actually works.
>
> You're of course free to believe what you wish, but that sounds
> short-sighted to me.  If we implement channel binding and it turns out
> not to be interoperable with other SSL implementations, then what?  We
> can't change it later without breaking compatibility with our own
> prior implementation.  Note that Álvaro Hernández Tortosa said about
> two hours before you sent this email that it doesn't seem possible to
> implement something comparable in Java's standard SSL stack.  If
> that's the case, adopting this implementation is dooming everyone who
> connects to the database server using JDBC to be unable to use channel
> binding.  And that's a large percentage of our user base.

I'm hopeful that we're closer to agreement here than we are the
opposite.

It was absolutely not my intent that the ongoing discussion between
Alvaro and Michael be stopped or changed, quite the opposite, in fact.

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we need to make sure that the JDBC driver is able to work with the
chosen solution (or, perhaps, that we support multuple options, one of
which works with the JDBC changes), and that we review the other TLS
libraries with the goal of making sure that what we agree on in core
should work with those also, then that's great and exactly what I'd like
to see also.

If your comments regarding the requirement that we have interoperability
testing of this feature before accepting it were intended to mean that
we must have a complete alternative TLS solution, while that would
actually play a great deal to a goal I've had for a while to have PG
support multiple TLS implementations (LibNSS being top-of-mind, at least
for me, as I've mentioned a couple times), I can't agree that we should
require that before accepting channel binding as a feature.  To do so
would be akin to requiring that we support multiple TLS implementations
before we agreed to support client-side certificates, in my opinion.

I would be rather surprised if the solution which Michael and Alvaro
come to results in a solution which requires us to break compatibility
down the road, though it's of course a risk we need to consider.  The
ongoing discussion around finding a way to support both methods outlined
in the RFC sounds exactly correct to me and I encourage further
discussion there.

> And then what happens when we do add support for Windows SSL or macOS
> SSL?  It sounds like you're equally willing to throw people using
> those implementations under the bus.  So we'll end up with channel
> binding that works only when everybody's running the same operating
> system and nobody's using Java.  Uggh.  At that point you might as
> well just go back to using SSL certificates to solve this problem.  If
> you use SSL certificates, then (1) it should work with any SSL
> implementation and (2) the client can force SSL certificate
> verification, whereas currently the client cannot force even the use
> of SCRAM, let alone channel binding.

I hope it's clear that it's not my intent to throw anyone "under the
bus."  The only reason that "SSL certificates should work with any SSL
implementation" is because those SSL implementations are based on RFCs
which define how they should work and what I was encouraging up-thread
is exactly that we should be looking at RFCs to determine the right path
forward.  There are cases, of course, where the RFCs have alternatives
and the ideal approach is to find a way to work with all of those
alternatives, or at least implement a solution which is flexible, such
that later changes could add support without breaking existing users.

I'm certainly on-board with the general idea of having a way for the
client to require both SCRAM and channel binding and I agree that's a
hole in our current system, but that is really an orthogonal concern.

> So what is being proposed here does not (yet, anyway) provide any
> actual security and seems to have poor prospects for interoperability,
> whereas we have an existing feature (SSL certificates) that has
> neither of those problems.  Are you sure you're not putting
> buzzword-compliance ahead of real progress?

Adding support for channel binding is quite important and valuable, in
its own right.  I concur that we want to provide ways for the client to
require SCRAM and to require channel binding, but I don't see any
particular reason that adding support for channel binding has to be held
up behind that.

As for your question, I have to say that I find it inappropriate to
characterize channel binding as a "buzzword" or to suggest that this
particular feature which Michael and Alvaro are working to add to PG is
only being done to fulfill some marketing requirement or checkbox but
without adding any technical merit in its own right.  Channel binding
isn't new, it's supported by quite a few different technologies, and we
would absolutely be better off including support for it, from an
entirely technical perspective.  If I were one of the individuals
working on this feature, I'd be rather put-off and upset at the
suggestion that the good work they're doing is viewed by the PostgreSQL
community and one of our major committers as only being done to check a
box or to be "buzzword compliant" and ultimately without technical
merit.

Thankfully, I know both Michael and Alvaro and had excellent discussions
with them at PGCon last week, and I don't doubt that they will continue
their efforts regardless, but I worry about the signal it sends to
individuals who are not yet contributors or who have not gone through
the process of trying to contribute to PostgreSQL.  We make it more than
difficult enough to get code into core, let's try to avoid discouraging
contributors by casting doubt on the technical merits of their efforts
when they're very clearly adding a useful feature, even if it isn't
perfect until we also add X, Y or Z.  Instead, let's encourage their
efforts to complete the work they've started and then work with them to
add those other components necessary to have a complete solution.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Wed, May 31, 2017 at 7:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
> If your comments regarding the requirement that we have interoperability
> testing of this feature before accepting it were intended to mean that
> we need to make sure that the JDBC driver is able to work with the
> chosen solution (or, perhaps, that we support multuple options, one of
> which works with the JDBC changes), and that we review the other TLS
> libraries with the goal of making sure that what we agree on in core
> should work with those also, then that's great and exactly what I'd like
> to see also.

OK, great.  That's what I mean (mostly - see below).

> If your comments regarding the requirement that we have interoperability
> testing of this feature before accepting it were intended to mean that
> we must have a complete alternative TLS solution, while that would
> actually play a great deal to a goal I've had for a while to have PG
> support multiple TLS implementations (LibNSS being top-of-mind, at least
> for me, as I've mentioned a couple times), I can't agree that we should
> require that before accepting channel binding as a feature.  To do so
> would be akin to requiring that we support multiple TLS implementations
> before we agreed to support client-side certificates, in my opinion.

I don't think that we need to have a committed patch allowing multiple
SSL implementations before we accept a patch for channel binding, but
I think it might be a good idea for someone to hack either libpq or
the server enough to make it work with some other SSL implementation
(Windows SSL would probably be the best candidate) and test that what
we think will work for channel binding actually does work.  It
wouldn't need to be a committable patch, but it should be enough to
make a successful connection in the laboratory.  Now, there might also
be other ways besides that of testing that interoperability is
possible, so don't take me as being of the view that someone has to
necessarily do exactly that thing that I just said.  But I do think
that we probably should do more than say "hey, we've got this
undocumented SSL API, and this other Windows API, and it looks like
they do about the same thing, so we're good".  There's sometimes a big
difference between what sounds like it should work on paper and what
actually does work.

> I would be rather surprised if the solution which Michael and Alvaro
> come to results in a solution which requires us to break compatibility
> down the road, though it's of course a risk we need to consider.  The
> ongoing discussion around finding a way to support both methods outlined
> in the RFC sounds exactly correct to me and I encourage further
> discussion there.

Sure, I have no objection to the discussion.  Discussion is always
cool; what I'm worried about is a tls-unique implementation that is
supremely unportable, and there's no current evidence that the
currently-proposed patch is anything else.  There is some optimism,
but optimism <> evidence.

> I'm certainly on-board with the general idea of having a way for the
> client to require both SCRAM and channel binding and I agree that's a
> hole in our current system, but that is really an orthogonal concern.

Orthogonal but *very* closely related.

> entirely technical perspective.  If I were one of the individuals
> working on this feature, I'd be rather put-off and upset at the
> suggestion that the good work they're doing is viewed by the PostgreSQL
> community and one of our major committers as only being done to check a
> box or to be "buzzword compliant" and ultimately without technical
> merit.

I did not say that the feature was "ultimately without technical
merit"; I said that the patch as submitted seemed like it might not
interoperate, and that without a libpq option to force it to be used
it wouldn't add any real security.  I stand by those statements and I
think it is 100% appropriate for me to raise those issues.  Other
people, including you, do the same thing about other patches all the
time.  It is not a broadside against the contributors or the patch; it
is a short, specific list of concerns that are IMHO quite justifiable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Wed, May 31, 2017 at 7:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
> If your comments regarding the requirement that we have interoperability
> testing of this feature before accepting it were intended to mean that
> we need to make sure that the JDBC driver is able to work with the
> chosen solution (or, perhaps, that we support multuple options, one of
> which works with the JDBC changes), and that we review the other TLS
> libraries with the goal of making sure that what we agree on in core
> should work with those also, then that's great and exactly what I'd like
> to see also.

OK, great.  That's what I mean (mostly - see below).

> If your comments regarding the requirement that we have interoperability
> testing of this feature before accepting it were intended to mean that
> we must have a complete alternative TLS solution, while that would
> actually play a great deal to a goal I've had for a while to have PG
> support multiple TLS implementations (LibNSS being top-of-mind, at least
> for me, as I've mentioned a couple times), I can't agree that we should
> require that before accepting channel binding as a feature.  To do so
> would be akin to requiring that we support multiple TLS implementations
> before we agreed to support client-side certificates, in my opinion.

I don't think that we need to have a committed patch allowing multiple
SSL implementations before we accept a patch for channel binding, but
I think it might be a good idea for someone to hack either libpq or
the server enough to make it work with some other SSL implementation
(Windows SSL would probably be the best candidate) and test that what
we think will work for channel binding actually does work.  It
wouldn't need to be a committable patch, but it should be enough to
make a successful connection in the laboratory.  Now, there might also
be other ways besides that of testing that interoperability is
possible, so don't take me as being of the view that someone has to
necessarily do exactly that thing that I just said.  But I do think
that we probably should do more than say "hey, we've got this
undocumented SSL API, and this other Windows API, and it looks like
they do about the same thing, so we're good".  There's sometimes a big
difference between what sounds like it should work on paper and what
actually does work.

> I would be rather surprised if the solution which Michael and Alvaro
> come to results in a solution which requires us to break compatibility
> down the road, though it's of course a risk we need to consider.  The
> ongoing discussion around finding a way to support both methods outlined
> in the RFC sounds exactly correct to me and I encourage further
> discussion there.

Sure, I have no objection to the discussion.  Discussion is always
cool; what I'm worried about is a tls-unique implementation that is
supremely unportable, and there's no current evidence that the
currently-proposed patch is anything else.  There is some optimism,
but optimism <> evidence.

> I'm certainly on-board with the general idea of having a way for the
> client to require both SCRAM and channel binding and I agree that's a
> hole in our current system, but that is really an orthogonal concern.

Orthogonal but *very* closely related.

> entirely technical perspective.  If I were one of the individuals
> working on this feature, I'd be rather put-off and upset at the
> suggestion that the good work they're doing is viewed by the PostgreSQL
> community and one of our major committers as only being done to check a
> box or to be "buzzword compliant" and ultimately without technical
> merit.

I did not say that the feature was "ultimately without technical
merit"; I said that the patch as submitted seemed like it might not
interoperate, and that without a libpq option to force it to be used
it wouldn't add any real security.  I stand by those statements and I
think it is 100% appropriate for me to raise those issues.  Other
people, including you, do the same thing about other patches all the
time.  It is not a broadside against the contributors or the patch; it
is a short, specific list of concerns that are IMHO quite justifiable.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, May 31, 2017 at 7:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > If your comments regarding the requirement that we have interoperability
> > testing of this feature before accepting it were intended to mean that
> > we need to make sure that the JDBC driver is able to work with the
> > chosen solution (or, perhaps, that we support multuple options, one of
> > which works with the JDBC changes), and that we review the other TLS
> > libraries with the goal of making sure that what we agree on in core
> > should work with those also, then that's great and exactly what I'd like
> > to see also.
>
> OK, great.  That's what I mean (mostly - see below).

Glad to hear it.

> > If your comments regarding the requirement that we have interoperability
> > testing of this feature before accepting it were intended to mean that
> > we must have a complete alternative TLS solution, while that would
> > actually play a great deal to a goal I've had for a while to have PG
> > support multiple TLS implementations (LibNSS being top-of-mind, at least
> > for me, as I've mentioned a couple times), I can't agree that we should
> > require that before accepting channel binding as a feature.  To do so
> > would be akin to requiring that we support multiple TLS implementations
> > before we agreed to support client-side certificates, in my opinion.
>
> I don't think that we need to have a committed patch allowing multiple
> SSL implementations before we accept a patch for channel binding, but
> I think it might be a good idea for someone to hack either libpq or
> the server enough to make it work with some other SSL implementation
> (Windows SSL would probably be the best candidate) and test that what
> we think will work for channel binding actually does work.  It
> wouldn't need to be a committable patch, but it should be enough to
> make a successful connection in the laboratory.  Now, there might also
> be other ways besides that of testing that interoperability is
> possible, so don't take me as being of the view that someone has to
> necessarily do exactly that thing that I just said.  But I do think
> that we probably should do more than say "hey, we've got this
> undocumented SSL API, and this other Windows API, and it looks like
> they do about the same thing, so we're good".  There's sometimes a big
> difference between what sounds like it should work on paper and what
> actually does work.

I certainly wouldn't object to someone working on this, but I feel like
it's a good deal more work than perhaps you're realizing (and I tend to
think trying to use the Windows SSL implementation would increase the
level of effort, not minimize it).  Perhaps it wouldn't be too bad to
write a one-off piece of code which just connects and then returns the
channel binding information on each side and then one could hand-check
that what's returned matches what it's supposed to based on the RFC, but
if it doesn't, then what?  In the specific case we're talking about,
there's two approaches in the RFC and it seems like we should support
both because at least our current JDBC implementation only works with
one, and ideally we can allow for that to be extended to other methods,
but I wouldn't want to implement a method that only works on Windows SSL
because that implementation, for whatever reason, doesn't follow either
of the methods available in the RFC.

Thanks!

Stephen

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, May 31, 2017 at 7:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > If your comments regarding the requirement that we have interoperability
> > testing of this feature before accepting it were intended to mean that
> > we need to make sure that the JDBC driver is able to work with the
> > chosen solution (or, perhaps, that we support multuple options, one of
> > which works with the JDBC changes), and that we review the other TLS
> > libraries with the goal of making sure that what we agree on in core
> > should work with those also, then that's great and exactly what I'd like
> > to see also.
>
> OK, great.  That's what I mean (mostly - see below).

Glad to hear it.

> > If your comments regarding the requirement that we have interoperability
> > testing of this feature before accepting it were intended to mean that
> > we must have a complete alternative TLS solution, while that would
> > actually play a great deal to a goal I've had for a while to have PG
> > support multiple TLS implementations (LibNSS being top-of-mind, at least
> > for me, as I've mentioned a couple times), I can't agree that we should
> > require that before accepting channel binding as a feature.  To do so
> > would be akin to requiring that we support multiple TLS implementations
> > before we agreed to support client-side certificates, in my opinion.
>
> I don't think that we need to have a committed patch allowing multiple
> SSL implementations before we accept a patch for channel binding, but
> I think it might be a good idea for someone to hack either libpq or
> the server enough to make it work with some other SSL implementation
> (Windows SSL would probably be the best candidate) and test that what
> we think will work for channel binding actually does work.  It
> wouldn't need to be a committable patch, but it should be enough to
> make a successful connection in the laboratory.  Now, there might also
> be other ways besides that of testing that interoperability is
> possible, so don't take me as being of the view that someone has to
> necessarily do exactly that thing that I just said.  But I do think
> that we probably should do more than say "hey, we've got this
> undocumented SSL API, and this other Windows API, and it looks like
> they do about the same thing, so we're good".  There's sometimes a big
> difference between what sounds like it should work on paper and what
> actually does work.

I certainly wouldn't object to someone working on this, but I feel like
it's a good deal more work than perhaps you're realizing (and I tend to
think trying to use the Windows SSL implementation would increase the
level of effort, not minimize it).  Perhaps it wouldn't be too bad to
write a one-off piece of code which just connects and then returns the
channel binding information on each side and then one could hand-check
that what's returned matches what it's supposed to based on the RFC, but
if it doesn't, then what?  In the specific case we're talking about,
there's two approaches in the RFC and it seems like we should support
both because at least our current JDBC implementation only works with
one, and ideally we can allow for that to be extended to other methods,
but I wouldn't want to implement a method that only works on Windows SSL
because that implementation, for whatever reason, doesn't follow either
of the methods available in the RFC.

Thanks!

Stephen

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Bruce Momjian
Date:
On Wed, May 31, 2017 at 09:37:02AM -0400, Robert Haas wrote:
> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > ... and I don't believe that we should be asking the
> > implementors of channel binding to also implement support for multiple
> > TLS libraries in PostgreSQL in order to test that their RFC-following
> > (at least, as far as they can tell) implementation actually works.
>
> You're of course free to believe what you wish, but that sounds
> short-sighted to me.  If we implement channel binding and it turns out
> not to be interoperable with other SSL implementations, then what?  We
> can't change it later without breaking compatibility with our own
> prior implementation.  Note that Álvaro Hernández Tortosa said about
> two hours before you sent this email that it doesn't seem possible to
> implement something comparable in Java's standard SSL stack.  If
> that's the case, adopting this implementation is dooming everyone who
> connects to the database server using JDBC to be unable to use channel
> binding.  And that's a large percentage of our user base.

Just to step back, exactly how does channel binding work?  Is each side
of the SSL connection hashing the password hash with the shared SSL
session secret in some way that each side knows the other end knows
the password hash, but not disclosing the secret or password hash?  Is
there some other way JDBC can get that information?

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Bruce Momjian
Date:
On Wed, May 31, 2017 at 09:37:02AM -0400, Robert Haas wrote:
> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > ... and I don't believe that we should be asking the
> > implementors of channel binding to also implement support for multiple
> > TLS libraries in PostgreSQL in order to test that their RFC-following
> > (at least, as far as they can tell) implementation actually works.
>
> You're of course free to believe what you wish, but that sounds
> short-sighted to me.  If we implement channel binding and it turns out
> not to be interoperable with other SSL implementations, then what?  We
> can't change it later without breaking compatibility with our own
> prior implementation.  Note that Álvaro Hernández Tortosa said about
> two hours before you sent this email that it doesn't seem possible to
> implement something comparable in Java's standard SSL stack.  If
> that's the case, adopting this implementation is dooming everyone who
> connects to the database server using JDBC to be unable to use channel
> binding.  And that's a large percentage of our user base.

Just to step back, exactly how does channel binding work?  Is each side
of the SSL connection hashing the password hash with the shared SSL
session secret in some way that each side knows the other end knows
the password hash, but not disclosing the secret or password hash?  Is
there some other way JDBC can get that information?

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Thu, Jun 1, 2017 at 11:50 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I certainly wouldn't object to someone working on this, but I feel like
> it's a good deal more work than perhaps you're realizing (and I tend to
> think trying to use the Windows SSL implementation would increase the
> level of effort, not minimize it).

I agree that it's a fair amount of work, but if nobody does it, then I
think it's pretty speculative to suppose that the feature actually
works correctly.

> Perhaps it wouldn't be too bad to
> write a one-off piece of code which just connects and then returns the
> channel binding information on each side and then one could hand-check
> that what's returned matches what it's supposed to based on the RFC, but
> if it doesn't, then what?

Then something's broken and we need to fix it before we start
committing patches.

> In the specific case we're talking about,
> there's two approaches in the RFC and it seems like we should support
> both because at least our current JDBC implementation only works with
> one, and ideally we can allow for that to be extended to other methods,
> but I wouldn't want to implement a method that only works on Windows SSL
> because that implementation, for whatever reason, doesn't follow either
> of the methods available in the RFC.

I am very skeptical that if two people on two different SSL
interpretations both do something that appears to comply with the RFC,
we can just assume those two people will get the same answer.  In an
ideal world, that would definitely work, but in the real world, I
think it needs to be tested or you don't really know.  I agree that if
a given SSL implementation is such that it can't support either of the
possible channel binding methods, then that's not our problem; people
using that SSL implementation just can't get channel binding, and if
they don't like that they can switch SSL implementations.  But I also
think that if two SSL implementations both claim to support what's
needed to make channel binding work and we don't do any testing that
they actually interoperate, then I don't think we can really know that
we've got it right.

Another way of validating Michael's problem which I would find
satisfactory is to go look at some other OpenSSL-based implementations
of channel binding.  If in each case they are using the same functions
that Michael used in the same way, then that's good evidence that his
implementation is doing the right thing, especially if any of those
implementations also support other SSL implementations and have
verified that the OpenSSL mechanism does in fact interoperate.

I don't really know why we're arguing about this.  It seems blindingly
obvious to me that we can't just take it on faith that the functions
Michael identified for this purpose are the right ones and will work
perfectly in complete compliance with the RFC.  We are in general very
reluctant to make such assumptions and if we were going to start,
changes that affect wire protocol compatibility wouldn't be my first
choice.  Is it really all that revolutionary or controversial to
suggest that this patch has not yet had enough validation to really
know that it does what we want?  To me, it seems like verifying that a
patch which supposedly implements a standard interoperates with
something other than itself is so obvious that it should barely need
to be mentioned, let alone become a bone of contention.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Thu, Jun 1, 2017 at 11:50 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I certainly wouldn't object to someone working on this, but I feel like
> it's a good deal more work than perhaps you're realizing (and I tend to
> think trying to use the Windows SSL implementation would increase the
> level of effort, not minimize it).

I agree that it's a fair amount of work, but if nobody does it, then I
think it's pretty speculative to suppose that the feature actually
works correctly.

> Perhaps it wouldn't be too bad to
> write a one-off piece of code which just connects and then returns the
> channel binding information on each side and then one could hand-check
> that what's returned matches what it's supposed to based on the RFC, but
> if it doesn't, then what?

Then something's broken and we need to fix it before we start
committing patches.

> In the specific case we're talking about,
> there's two approaches in the RFC and it seems like we should support
> both because at least our current JDBC implementation only works with
> one, and ideally we can allow for that to be extended to other methods,
> but I wouldn't want to implement a method that only works on Windows SSL
> because that implementation, for whatever reason, doesn't follow either
> of the methods available in the RFC.

I am very skeptical that if two people on two different SSL
interpretations both do something that appears to comply with the RFC,
we can just assume those two people will get the same answer.  In an
ideal world, that would definitely work, but in the real world, I
think it needs to be tested or you don't really know.  I agree that if
a given SSL implementation is such that it can't support either of the
possible channel binding methods, then that's not our problem; people
using that SSL implementation just can't get channel binding, and if
they don't like that they can switch SSL implementations.  But I also
think that if two SSL implementations both claim to support what's
needed to make channel binding work and we don't do any testing that
they actually interoperate, then I don't think we can really know that
we've got it right.

Another way of validating Michael's problem which I would find
satisfactory is to go look at some other OpenSSL-based implementations
of channel binding.  If in each case they are using the same functions
that Michael used in the same way, then that's good evidence that his
implementation is doing the right thing, especially if any of those
implementations also support other SSL implementations and have
verified that the OpenSSL mechanism does in fact interoperate.

I don't really know why we're arguing about this.  It seems blindingly
obvious to me that we can't just take it on faith that the functions
Michael identified for this purpose are the right ones and will work
perfectly in complete compliance with the RFC.  We are in general very
reluctant to make such assumptions and if we were going to start,
changes that affect wire protocol compatibility wouldn't be my first
choice.  Is it really all that revolutionary or controversial to
suggest that this patch has not yet had enough validation to really
know that it does what we want?  To me, it seems like verifying that a
patch which supposedly implements a standard interoperates with
something other than itself is so obvious that it should barely need
to be mentioned, let alone become a bone of contention.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 01/06/17 18:11, Bruce Momjian wrote:
> On Wed, May 31, 2017 at 09:37:02AM -0400, Robert Haas wrote:
>> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> ... and I don't believe that we should be asking the
>>> implementors of channel binding to also implement support for multiple
>>> TLS libraries in PostgreSQL in order to test that their RFC-following
>>> (at least, as far as they can tell) implementation actually works.
>> You're of course free to believe what you wish, but that sounds
>> short-sighted to me.  If we implement channel binding and it turns out
>> not to be interoperable with other SSL implementations, then what?  We
>> can't change it later without breaking compatibility with our own
>> prior implementation.  Note that Álvaro Hernández Tortosa said about
>> two hours before you sent this email that it doesn't seem possible to
>> implement something comparable in Java's standard SSL stack.  If
>> that's the case, adopting this implementation is dooming everyone who
>> connects to the database server using JDBC to be unable to use channel
>> binding.  And that's a large percentage of our user base.
> Just to step back, exactly how does channel binding work?  Is each side
> of the SSL connection hashing the password hash with the shared SSL
> session secret in some way that each side knows the other end knows
> the password hash, but not disclosing the secret or password hash?  Is
> there some other way JDBC can get that information?
>

     In short, channel binding augments SCRAM (could be also other
authentication methods, I guess) by adding to the mix of data to be
exchanged, data that comes off-band. Normal SCRAM exchange involves
user, a unique token, a salt and some other information. If you add into
the mix off-band information, that is uniquely known by only client and
server, you can avoid MITM attacks. It's not as simple as "hashing" the
password, though. SCRAM involves 4 steps (2 messages each way) with
somehow complex hashing of data in the last 2 steps.

     There are basically 2 channel binding options, that is, 2 possible
data pieces that could be taken off-band of the SCRAM authentication and
throw them into this mix:

- tls-unique. It's like a unique identifier for each client-server SSL
connection. It should be get from the SSL channel and there doesn't seem
to be a super consistent way of getting it from the channel (in OpenSSL
is an undocumented API, it's not available in normal Java stack, etc).
- tls-server-end-point. Channel binding data is just a hash of a SSL
certificate, as is. As such, seems to be easier to be supported across
multiple OSs/SSL stacks.

     However, SCRAM RFC states that if channel binding is implemented,
at least tls-unique must be implemented, with others being optional
(there is as of today a third one, but only applies to telnet). So in my
opinion, I'd implement both of the above, for maximum flexibility, and
add a field to the required scram authentication febe message with a
list (aka CSV) of the channel binding mechanisms supported by this
server. In this way, I believe covers support for several channel
binding mechanisms and could be extended in the future should other
mechanisms appear.


     Álvaro

--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 01/06/17 18:11, Bruce Momjian wrote:
> On Wed, May 31, 2017 at 09:37:02AM -0400, Robert Haas wrote:
>> On Tue, May 30, 2017 at 11:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> ... and I don't believe that we should be asking the
>>> implementors of channel binding to also implement support for multiple
>>> TLS libraries in PostgreSQL in order to test that their RFC-following
>>> (at least, as far as they can tell) implementation actually works.
>> You're of course free to believe what you wish, but that sounds
>> short-sighted to me.  If we implement channel binding and it turns out
>> not to be interoperable with other SSL implementations, then what?  We
>> can't change it later without breaking compatibility with our own
>> prior implementation.  Note that Álvaro Hernández Tortosa said about
>> two hours before you sent this email that it doesn't seem possible to
>> implement something comparable in Java's standard SSL stack.  If
>> that's the case, adopting this implementation is dooming everyone who
>> connects to the database server using JDBC to be unable to use channel
>> binding.  And that's a large percentage of our user base.
> Just to step back, exactly how does channel binding work?  Is each side
> of the SSL connection hashing the password hash with the shared SSL
> session secret in some way that each side knows the other end knows
> the password hash, but not disclosing the secret or password hash?  Is
> there some other way JDBC can get that information?
>

     In short, channel binding augments SCRAM (could be also other
authentication methods, I guess) by adding to the mix of data to be
exchanged, data that comes off-band. Normal SCRAM exchange involves
user, a unique token, a salt and some other information. If you add into
the mix off-band information, that is uniquely known by only client and
server, you can avoid MITM attacks. It's not as simple as "hashing" the
password, though. SCRAM involves 4 steps (2 messages each way) with
somehow complex hashing of data in the last 2 steps.

     There are basically 2 channel binding options, that is, 2 possible
data pieces that could be taken off-band of the SCRAM authentication and
throw them into this mix:

- tls-unique. It's like a unique identifier for each client-server SSL
connection. It should be get from the SSL channel and there doesn't seem
to be a super consistent way of getting it from the channel (in OpenSSL
is an undocumented API, it's not available in normal Java stack, etc).
- tls-server-end-point. Channel binding data is just a hash of a SSL
certificate, as is. As such, seems to be easier to be supported across
multiple OSs/SSL stacks.

     However, SCRAM RFC states that if channel binding is implemented,
at least tls-unique must be implemented, with others being optional
(there is as of today a third one, but only applies to telnet). So in my
opinion, I'd implement both of the above, for maximum flexibility, and
add a field to the required scram authentication febe message with a
list (aka CSV) of the channel binding mechanisms supported by this
server. In this way, I believe covers support for several channel
binding mechanisms and could be extended in the future should other
mechanisms appear.


     Álvaro

--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 01/06/17 17:50, Stephen Frost wrote:
> Robert,
>
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> On Wed, May 31, 2017 at 7:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> If your comments regarding the requirement that we have interoperability
>>> testing of this feature before accepting it were intended to mean that
>>> we need to make sure that the JDBC driver is able to work with the
>>> chosen solution (or, perhaps, that we support multuple options, one of
>>> which works with the JDBC changes), and that we review the other TLS
>>> libraries with the goal of making sure that what we agree on in core
>>> should work with those also, then that's great and exactly what I'd like
>>> to see also.
>> OK, great.  That's what I mean (mostly - see below).
> Glad to hear it.
>
>>> If your comments regarding the requirement that we have interoperability
>>> testing of this feature before accepting it were intended to mean that
>>> we must have a complete alternative TLS solution, while that would
>>> actually play a great deal to a goal I've had for a while to have PG
>>> support multiple TLS implementations (LibNSS being top-of-mind, at least
>>> for me, as I've mentioned a couple times), I can't agree that we should
>>> require that before accepting channel binding as a feature.  To do so
>>> would be akin to requiring that we support multiple TLS implementations
>>> before we agreed to support client-side certificates, in my opinion.
>> I don't think that we need to have a committed patch allowing multiple
>> SSL implementations before we accept a patch for channel binding, but
>> I think it might be a good idea for someone to hack either libpq or
>> the server enough to make it work with some other SSL implementation
>> (Windows SSL would probably be the best candidate) and test that what
>> we think will work for channel binding actually does work.  It
>> wouldn't need to be a committable patch, but it should be enough to
>> make a successful connection in the laboratory.  Now, there might also
>> be other ways besides that of testing that interoperability is
>> possible, so don't take me as being of the view that someone has to
>> necessarily do exactly that thing that I just said.  But I do think
>> that we probably should do more than say "hey, we've got this
>> undocumented SSL API, and this other Windows API, and it looks like
>> they do about the same thing, so we're good".  There's sometimes a big
>> difference between what sounds like it should work on paper and what
>> actually does work.
> I certainly wouldn't object to someone working on this, but I feel like
> it's a good deal more work than perhaps you're realizing (and I tend to
> think trying to use the Windows SSL implementation would increase the
> level of effort, not minimize it).  Perhaps it wouldn't be too bad to
> write a one-off piece of code which just connects and then returns the
> channel binding information on each side and then one could hand-check
> that what's returned matches what it's supposed to based on the RFC, but
> if it doesn't, then what?  In the specific case we're talking about,
> there's two approaches in the RFC and it seems like we should support
> both because at least our current JDBC implementation only works with
> one, and ideally we can allow for that to be extended to other methods,
> but I wouldn't want to implement a method that only works on Windows SSL
> because that implementation, for whatever reason, doesn't follow either
> of the methods available in the RFC.

     To make things even more complicated, I think the RFC is not
helping very much, as the definition is not very clear itself:

"
Description: The first TLS Finished message sent (note: the Finished
    struct, not the TLS record layer message containing it) in the most
    recent TLS handshake of the TLS connection being bound to (note: TLS
    connection, not session, so that the channel binding is specific to
    each connection regardless of whether session resumption is used).
    If TLS renegotiation takes place before the channel binding
    operation, then the first TLS Finished message sent of the latest/
    inner-most TLS connection is used.  Note that for full TLS
    handshakes, the first Finished message is sent by the client, while
    for abbreviated TLS handshakes (session resumption), the first
    Finished message is sent by the server.
"
https://tools.ietf.org/html/rfc5929#section-3.1

     If you read further, it becomes even less clear what it is and that
if there are re-negotiations, it gets more complicated. Server-end-point
is kind of better specified:

"
The hash of the TLS server's certificate [RFC5280] as it
    appears, octet for octet, in the server's Certificate message.
"


     Álvaro



--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 01/06/17 17:50, Stephen Frost wrote:
> Robert,
>
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> On Wed, May 31, 2017 at 7:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> If your comments regarding the requirement that we have interoperability
>>> testing of this feature before accepting it were intended to mean that
>>> we need to make sure that the JDBC driver is able to work with the
>>> chosen solution (or, perhaps, that we support multuple options, one of
>>> which works with the JDBC changes), and that we review the other TLS
>>> libraries with the goal of making sure that what we agree on in core
>>> should work with those also, then that's great and exactly what I'd like
>>> to see also.
>> OK, great.  That's what I mean (mostly - see below).
> Glad to hear it.
>
>>> If your comments regarding the requirement that we have interoperability
>>> testing of this feature before accepting it were intended to mean that
>>> we must have a complete alternative TLS solution, while that would
>>> actually play a great deal to a goal I've had for a while to have PG
>>> support multiple TLS implementations (LibNSS being top-of-mind, at least
>>> for me, as I've mentioned a couple times), I can't agree that we should
>>> require that before accepting channel binding as a feature.  To do so
>>> would be akin to requiring that we support multiple TLS implementations
>>> before we agreed to support client-side certificates, in my opinion.
>> I don't think that we need to have a committed patch allowing multiple
>> SSL implementations before we accept a patch for channel binding, but
>> I think it might be a good idea for someone to hack either libpq or
>> the server enough to make it work with some other SSL implementation
>> (Windows SSL would probably be the best candidate) and test that what
>> we think will work for channel binding actually does work.  It
>> wouldn't need to be a committable patch, but it should be enough to
>> make a successful connection in the laboratory.  Now, there might also
>> be other ways besides that of testing that interoperability is
>> possible, so don't take me as being of the view that someone has to
>> necessarily do exactly that thing that I just said.  But I do think
>> that we probably should do more than say "hey, we've got this
>> undocumented SSL API, and this other Windows API, and it looks like
>> they do about the same thing, so we're good".  There's sometimes a big
>> difference between what sounds like it should work on paper and what
>> actually does work.
> I certainly wouldn't object to someone working on this, but I feel like
> it's a good deal more work than perhaps you're realizing (and I tend to
> think trying to use the Windows SSL implementation would increase the
> level of effort, not minimize it).  Perhaps it wouldn't be too bad to
> write a one-off piece of code which just connects and then returns the
> channel binding information on each side and then one could hand-check
> that what's returned matches what it's supposed to based on the RFC, but
> if it doesn't, then what?  In the specific case we're talking about,
> there's two approaches in the RFC and it seems like we should support
> both because at least our current JDBC implementation only works with
> one, and ideally we can allow for that to be extended to other methods,
> but I wouldn't want to implement a method that only works on Windows SSL
> because that implementation, for whatever reason, doesn't follow either
> of the methods available in the RFC.

     To make things even more complicated, I think the RFC is not
helping very much, as the definition is not very clear itself:

"
Description: The first TLS Finished message sent (note: the Finished
    struct, not the TLS record layer message containing it) in the most
    recent TLS handshake of the TLS connection being bound to (note: TLS
    connection, not session, so that the channel binding is specific to
    each connection regardless of whether session resumption is used).
    If TLS renegotiation takes place before the channel binding
    operation, then the first TLS Finished message sent of the latest/
    inner-most TLS connection is used.  Note that for full TLS
    handshakes, the first Finished message is sent by the client, while
    for abbreviated TLS handshakes (session resumption), the first
    Finished message is sent by the server.
"
https://tools.ietf.org/html/rfc5929#section-3.1

     If you read further, it becomes even less clear what it is and that
if there are re-negotiations, it gets more complicated. Server-end-point
is kind of better specified:

"
The hash of the TLS server's certificate [RFC5280] as it
    appears, octet for octet, in the server's Certificate message.
"


     Álvaro



--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Jun 1, 2017 at 11:50 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > I certainly wouldn't object to someone working on this, but I feel like
> > it's a good deal more work than perhaps you're realizing (and I tend to
> > think trying to use the Windows SSL implementation would increase the
> > level of effort, not minimize it).
>
> I agree that it's a fair amount of work, but if nobody does it, then I
> think it's pretty speculative to suppose that the feature actually
> works correctly.

We've considered "works with OpenSSL" (and, to some extent, JDBC, but
I'm pretty sure that came later and just happened to be able to work...)
to be sufficient to include things like client-side certificate based
authentication, so this is raising the bar quite a bit for a feature
that, while important and valuable, frankly isn't as important as
client-side cert auth.

> > Perhaps it wouldn't be too bad to
> > write a one-off piece of code which just connects and then returns the
> > channel binding information on each side and then one could hand-check
> > that what's returned matches what it's supposed to based on the RFC, but
> > if it doesn't, then what?
>
> Then something's broken and we need to fix it before we start
> committing patches.

... Or that implementation doesn't follow the RFC, which is what I was
getting at.

> > In the specific case we're talking about,
> > there's two approaches in the RFC and it seems like we should support
> > both because at least our current JDBC implementation only works with
> > one, and ideally we can allow for that to be extended to other methods,
> > but I wouldn't want to implement a method that only works on Windows SSL
> > because that implementation, for whatever reason, doesn't follow either
> > of the methods available in the RFC.
>
> I am very skeptical that if two people on two different SSL
> interpretations both do something that appears to comply with the RFC,
> we can just assume those two people will get the same answer.  In an
> ideal world, that would definitely work, but in the real world, I
> think it needs to be tested or you don't really know.  I agree that if
> a given SSL implementation is such that it can't support either of the
> possible channel binding methods, then that's not our problem; people
> using that SSL implementation just can't get channel binding, and if
> they don't like that they can switch SSL implementations.  But I also
> think that if two SSL implementations both claim to support what's
> needed to make channel binding work and we don't do any testing that
> they actually interoperate, then I don't think we can really know that
> we've got it right.

I'm all for doing testing, as I've tried to point out a few times, and I
would like to see an implementation which is able to be extended in the
future to other channel binding methods, as we clearly need to support
at least the two listed in the RFC based on this discussion and there
might be a still better way down the road anyway.

> Another way of validating Michael's problem which I would find
> satisfactory is to go look at some other OpenSSL-based implementations
> of channel binding.  If in each case they are using the same functions
> that Michael used in the same way, then that's good evidence that his
> implementation is doing the right thing, especially if any of those
> implementations also support other SSL implementations and have
> verified that the OpenSSL mechanism does in fact interoperate.

I don't have any issue with asking that Michael, or someone, to go look
at other OpenSSL-using implementations which support channel binding.

> I don't really know why we're arguing about this.  It seems blindingly
> obvious to me that we can't just take it on faith that the functions
> Michael identified for this purpose are the right ones and will work
> perfectly in complete compliance with the RFC.  We are in general very
> reluctant to make such assumptions and if we were going to start,
> changes that affect wire protocol compatibility wouldn't be my first
> choice.  Is it really all that revolutionary or controversial to
> suggest that this patch has not yet had enough validation to really
> know that it does what we want?  To me, it seems like verifying that a
> patch which supposedly implements a standard interoperates with
> something other than itself is so obvious that it should barely need
> to be mentioned, let alone become a bone of contention.

As I said in an earlier reply, I'm hopefuly that we're closer to
agreement here than we are disagreement, but there seems to be some
confusion regarding my position on this specific patch.  I'm not
advocating for this patch to be committed as-is or even anytime soon,
and I'm unsure of where I gave that impression.  I'm encouraged by the
ongoing discussion between Michael and Alvaro and hope to see a new
patch down the road which incorporates the results of their discussion
and works with both OpenSSL and the JDBC implementation, at least.

What I find somewhat objectionable is the notion that if we don't have 5
different TLS/SSL implementations supported in PG and that we've tested
that channel binding works correctly among all combinations of all of
them, then we can't accept a patch implementing it.  I'm exaggerating
for effect here, of course, and you've agreed that a full, committable,
implementation isn't necessary, and I agreed with that, but we still
seem to have a different level of expectation regarding what needs doing
here.  I don't know that we'll ever be able to nail down the exact
location of the line in the sand that needs to be crossed here, or agree
to it, so I'd suggest that we let them continue their efforts to work
together and provide a patch which they feel is ready to be commented on
by committers.  Perhaps we'll find that, regardless of where the line
was drawn exactly, they've crossed it.

Thanks!

Stephen

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Jun 1, 2017 at 11:50 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > I certainly wouldn't object to someone working on this, but I feel like
> > it's a good deal more work than perhaps you're realizing (and I tend to
> > think trying to use the Windows SSL implementation would increase the
> > level of effort, not minimize it).
>
> I agree that it's a fair amount of work, but if nobody does it, then I
> think it's pretty speculative to suppose that the feature actually
> works correctly.

We've considered "works with OpenSSL" (and, to some extent, JDBC, but
I'm pretty sure that came later and just happened to be able to work...)
to be sufficient to include things like client-side certificate based
authentication, so this is raising the bar quite a bit for a feature
that, while important and valuable, frankly isn't as important as
client-side cert auth.

> > Perhaps it wouldn't be too bad to
> > write a one-off piece of code which just connects and then returns the
> > channel binding information on each side and then one could hand-check
> > that what's returned matches what it's supposed to based on the RFC, but
> > if it doesn't, then what?
>
> Then something's broken and we need to fix it before we start
> committing patches.

... Or that implementation doesn't follow the RFC, which is what I was
getting at.

> > In the specific case we're talking about,
> > there's two approaches in the RFC and it seems like we should support
> > both because at least our current JDBC implementation only works with
> > one, and ideally we can allow for that to be extended to other methods,
> > but I wouldn't want to implement a method that only works on Windows SSL
> > because that implementation, for whatever reason, doesn't follow either
> > of the methods available in the RFC.
>
> I am very skeptical that if two people on two different SSL
> interpretations both do something that appears to comply with the RFC,
> we can just assume those two people will get the same answer.  In an
> ideal world, that would definitely work, but in the real world, I
> think it needs to be tested or you don't really know.  I agree that if
> a given SSL implementation is such that it can't support either of the
> possible channel binding methods, then that's not our problem; people
> using that SSL implementation just can't get channel binding, and if
> they don't like that they can switch SSL implementations.  But I also
> think that if two SSL implementations both claim to support what's
> needed to make channel binding work and we don't do any testing that
> they actually interoperate, then I don't think we can really know that
> we've got it right.

I'm all for doing testing, as I've tried to point out a few times, and I
would like to see an implementation which is able to be extended in the
future to other channel binding methods, as we clearly need to support
at least the two listed in the RFC based on this discussion and there
might be a still better way down the road anyway.

> Another way of validating Michael's problem which I would find
> satisfactory is to go look at some other OpenSSL-based implementations
> of channel binding.  If in each case they are using the same functions
> that Michael used in the same way, then that's good evidence that his
> implementation is doing the right thing, especially if any of those
> implementations also support other SSL implementations and have
> verified that the OpenSSL mechanism does in fact interoperate.

I don't have any issue with asking that Michael, or someone, to go look
at other OpenSSL-using implementations which support channel binding.

> I don't really know why we're arguing about this.  It seems blindingly
> obvious to me that we can't just take it on faith that the functions
> Michael identified for this purpose are the right ones and will work
> perfectly in complete compliance with the RFC.  We are in general very
> reluctant to make such assumptions and if we were going to start,
> changes that affect wire protocol compatibility wouldn't be my first
> choice.  Is it really all that revolutionary or controversial to
> suggest that this patch has not yet had enough validation to really
> know that it does what we want?  To me, it seems like verifying that a
> patch which supposedly implements a standard interoperates with
> something other than itself is so obvious that it should barely need
> to be mentioned, let alone become a bone of contention.

As I said in an earlier reply, I'm hopefuly that we're closer to
agreement here than we are disagreement, but there seems to be some
confusion regarding my position on this specific patch.  I'm not
advocating for this patch to be committed as-is or even anytime soon,
and I'm unsure of where I gave that impression.  I'm encouraged by the
ongoing discussion between Michael and Alvaro and hope to see a new
patch down the road which incorporates the results of their discussion
and works with both OpenSSL and the JDBC implementation, at least.

What I find somewhat objectionable is the notion that if we don't have 5
different TLS/SSL implementations supported in PG and that we've tested
that channel binding works correctly among all combinations of all of
them, then we can't accept a patch implementing it.  I'm exaggerating
for effect here, of course, and you've agreed that a full, committable,
implementation isn't necessary, and I agreed with that, but we still
seem to have a different level of expectation regarding what needs doing
here.  I don't know that we'll ever be able to nail down the exact
location of the line in the sand that needs to be crossed here, or agree
to it, so I'd suggest that we let them continue their efforts to work
together and provide a patch which they feel is ready to be commented on
by committers.  Perhaps we'll find that, regardless of where the line
was drawn exactly, they've crossed it.

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost <sfrost@snowman.net> wrote:
> What I find somewhat objectionable is the notion that if we don't have 5
> different TLS/SSL implementations supported in PG and that we've tested
> that channel binding works correctly among all combinations of all of
> them, then we can't accept a patch implementing it.

It seems to me that any testing in this area won't fly high as long as
there is no way to enforce the list of TLS implementations that a
server allows. There have been discussions about being able to control
that after the OpenSSL vulnerabilities that were protocol-specific and
there were even patches adding GUCs for this purpose. At the end,
everything has been rejected as Postgres enforces the use of the
newest one when doing the SSL handshake.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost <sfrost@snowman.net> wrote:
> What I find somewhat objectionable is the notion that if we don't have 5
> different TLS/SSL implementations supported in PG and that we've tested
> that channel binding works correctly among all combinations of all of
> them, then we can't accept a patch implementing it.

It seems to me that any testing in this area won't fly high as long as
there is no way to enforce the list of TLS implementations that a
server allows. There have been discussions about being able to control
that after the OpenSSL vulnerabilities that were protocol-specific and
there were even patches adding GUCs for this purpose. At the end,
everything has been rejected as Postgres enforces the use of the
newest one when doing the SSL handshake.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> What I find somewhat objectionable is the notion that if we don't have 5
>> different TLS/SSL implementations supported in PG and that we've tested
>> that channel binding works correctly among all combinations of all of
>> them, then we can't accept a patch implementing it.
>
> It seems to me that any testing in this area won't fly high as long as
> there is no way to enforce the list of TLS implementations that a
> server allows. There have been discussions about being able to control
> that after the OpenSSL vulnerabilities that were protocol-specific and
> there were even patches adding GUCs for this purpose. At the end,
> everything has been rejected as Postgres enforces the use of the
> newest one when doing the SSL handshake.

TLS implementations, or TLS versions?  What does the TLS version have
to do with this issue?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> What I find somewhat objectionable is the notion that if we don't have 5
>> different TLS/SSL implementations supported in PG and that we've tested
>> that channel binding works correctly among all combinations of all of
>> them, then we can't accept a patch implementing it.
>
> It seems to me that any testing in this area won't fly high as long as
> there is no way to enforce the list of TLS implementations that a
> server allows. There have been discussions about being able to control
> that after the OpenSSL vulnerabilities that were protocol-specific and
> there were even patches adding GUCs for this purpose. At the end,
> everything has been rejected as Postgres enforces the use of the
> newest one when doing the SSL handshake.

TLS implementations, or TLS versions?  What does the TLS version have
to do with this issue?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jun 2, 2017 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> It seems to me that any testing in this area won't fly high as long as
>> there is no way to enforce the list of TLS implementations that a
>> server allows. There have been discussions about being able to control
>> that after the OpenSSL vulnerabilities that were protocol-specific and
>> there were even patches adding GUCs for this purpose. At the end,
>> everything has been rejected as Postgres enforces the use of the
>> newest one when doing the SSL handshake.
>
> TLS implementations, or TLS versions?  What does the TLS version have
> to do with this issue?

I really mean *version* here. Unlike OpenSSL, the Windows TLS
implementation does not offer an API to choose the latest TLS version
available:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa380513(v=vs.85).aspx
It is up to the server and the client to negotiate that, so it seems
to me that we want some control in this area, which would be important
for testing as well because the TLS finish message differs a bit
across versions, in length mainly. On top of that per the aggressive
updates that Windows does from time to time they may as well forcibly
expose users to a broken TLS implementation...
MacOS has something similar to OpenSSL, with
SSLGetProtocolVersionMax(), which is nice.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jun 2, 2017 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> It seems to me that any testing in this area won't fly high as long as
>> there is no way to enforce the list of TLS implementations that a
>> server allows. There have been discussions about being able to control
>> that after the OpenSSL vulnerabilities that were protocol-specific and
>> there were even patches adding GUCs for this purpose. At the end,
>> everything has been rejected as Postgres enforces the use of the
>> newest one when doing the SSL handshake.
>
> TLS implementations, or TLS versions?  What does the TLS version have
> to do with this issue?

I really mean *version* here. Unlike OpenSSL, the Windows TLS
implementation does not offer an API to choose the latest TLS version
available:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa380513(v=vs.85).aspx
It is up to the server and the client to negotiate that, so it seems
to me that we want some control in this area, which would be important
for testing as well because the TLS finish message differs a bit
across versions, in length mainly. On top of that per the aggressive
updates that Windows does from time to time they may as well forcibly
expose users to a broken TLS implementation...
MacOS has something similar to OpenSSL, with
SSLGetProtocolVersionMax(), which is nice.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I don't have any issue with asking that Michael, or someone, to go look
> at other OpenSSL-using implementations which support channel binding.

I don't see the implementation of other TLS/SSL as a requirement for
channel binding with OpenSSL, and I have no plans to investigate that.
What is clearly a requirement though is to design a set of API generic
enough for the backend and the frontend to fetch the TLS unique
message, and what's needed for endpoint so as any implementation can
plug in easily with PG's channel binding code.

> What I find somewhat objectionable is the notion that if we don't have 5
> different TLS/SSL implementations supported in PG and that we've tested
> that channel binding works correctly among all combinations of all of
> them, then we can't accept a patch implementing it.  I'm exaggerating
> for effect here, of course, and you've agreed that a full, committable,
> implementation isn't necessary, and I agreed with that, but we still
> seem to have a different level of expectation regarding what needs doing
> here.  I don't know that we'll ever be able to nail down the exact
> location of the line in the sand that needs to be crossed here, or agree
> to it, so I'd suggest that we let them continue their efforts to work
> together and provide a patch which they feel is ready to be commented on
> by committers.  Perhaps we'll find that, regardless of where the line
> was drawn exactly, they've crossed it.

As far as I can see, there are a couple of things that I still need to
work on to make people happy:
- Rework the generic APIs for TLS finish and endpoint so as any
implementation can use channel binding without inducing any extra code
footprint to be-secure.c and fe-secure.c.
- Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
- Have a couple of tests for channel binding to allow people to test
the feature easily. Those will be in src/test/ssl/. It would be nice
as well to be able to enforce the channel binding type on libpq-side,
which is useful at least for testing. So we are going to need an
environment variable for this purpose, and a connection parameter.
--
Michael


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jun 2, 2017 at 10:08 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I don't have any issue with asking that Michael, or someone, to go look
> at other OpenSSL-using implementations which support channel binding.

I don't see the implementation of other TLS/SSL as a requirement for
channel binding with OpenSSL, and I have no plans to investigate that.
What is clearly a requirement though is to design a set of API generic
enough for the backend and the frontend to fetch the TLS unique
message, and what's needed for endpoint so as any implementation can
plug in easily with PG's channel binding code.

> What I find somewhat objectionable is the notion that if we don't have 5
> different TLS/SSL implementations supported in PG and that we've tested
> that channel binding works correctly among all combinations of all of
> them, then we can't accept a patch implementing it.  I'm exaggerating
> for effect here, of course, and you've agreed that a full, committable,
> implementation isn't necessary, and I agreed with that, but we still
> seem to have a different level of expectation regarding what needs doing
> here.  I don't know that we'll ever be able to nail down the exact
> location of the line in the sand that needs to be crossed here, or agree
> to it, so I'd suggest that we let them continue their efforts to work
> together and provide a patch which they feel is ready to be commented on
> by committers.  Perhaps we'll find that, regardless of where the line
> was drawn exactly, they've crossed it.

As far as I can see, there are a couple of things that I still need to
work on to make people happy:
- Rework the generic APIs for TLS finish and endpoint so as any
implementation can use channel binding without inducing any extra code
footprint to be-secure.c and fe-secure.c.
- Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
- Have a couple of tests for channel binding to allow people to test
the feature easily. Those will be in src/test/ssl/. It would be nice
as well to be able to enforce the channel binding type on libpq-side,
which is useful at least for testing. So we are going to need an
environment variable for this purpose, and a connection parameter.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Fri, Jun 2, 2017 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
> > <michael.paquier@gmail.com> wrote:
> >> It seems to me that any testing in this area won't fly high as long as
> >> there is no way to enforce the list of TLS implementations that a
> >> server allows. There have been discussions about being able to control
> >> that after the OpenSSL vulnerabilities that were protocol-specific and
> >> there were even patches adding GUCs for this purpose. At the end,
> >> everything has been rejected as Postgres enforces the use of the
> >> newest one when doing the SSL handshake.
> >
> > TLS implementations, or TLS versions?  What does the TLS version have
> > to do with this issue?
>
> I really mean *version* here. Unlike OpenSSL, the Windows TLS
> implementation does not offer an API to choose the latest TLS version
> available:
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa380513(v=vs.85).aspx
> It is up to the server and the client to negotiate that, so it seems
> to me that we want some control in this area, which would be important
> for testing as well because the TLS finish message differs a bit
> across versions, in length mainly. On top of that per the aggressive
> updates that Windows does from time to time they may as well forcibly
> expose users to a broken TLS implementation...
> MacOS has something similar to OpenSSL, with
> SSLGetProtocolVersionMax(), which is nice.

We mainly need to know what version was used, right..?  Isn't that
available?

Thanks!

Stephen

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Stephen Frost
Date:
Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Fri, Jun 2, 2017 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Thu, Jun 1, 2017 at 9:13 PM, Michael Paquier
> > <michael.paquier@gmail.com> wrote:
> >> It seems to me that any testing in this area won't fly high as long as
> >> there is no way to enforce the list of TLS implementations that a
> >> server allows. There have been discussions about being able to control
> >> that after the OpenSSL vulnerabilities that were protocol-specific and
> >> there were even patches adding GUCs for this purpose. At the end,
> >> everything has been rejected as Postgres enforces the use of the
> >> newest one when doing the SSL handshake.
> >
> > TLS implementations, or TLS versions?  What does the TLS version have
> > to do with this issue?
>
> I really mean *version* here. Unlike OpenSSL, the Windows TLS
> implementation does not offer an API to choose the latest TLS version
> available:
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa380513(v=vs.85).aspx
> It is up to the server and the client to negotiate that, so it seems
> to me that we want some control in this area, which would be important
> for testing as well because the TLS finish message differs a bit
> across versions, in length mainly. On top of that per the aggressive
> updates that Windows does from time to time they may as well forcibly
> expose users to a broken TLS implementation...
> MacOS has something similar to OpenSSL, with
> SSLGetProtocolVersionMax(), which is nice.

We mainly need to know what version was used, right..?  Isn't that
available?

Thanks!

Stephen

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, Jun 6, 2017 at 2:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>>> At the end,
>>> everything has been rejected as Postgres enforces the use of the
>>> newest one when doing the SSL handshake.
>>
>> TLS implementations, or TLS versions?  What does the TLS version have
>> to do with this issue?
>
> I really mean *version* here.

I don't think it's true that we force the latest TLS version to be
used.  The comment says:

        /*
         * We use SSLv23_method() because it can negotiate use of the highest
         * mutually supported protocol version, while alternatives like
         * TLSv1_2_method() permit only one specific version.  Note
that we don't
         * actually allow SSL v2 or v3, only TLS protocols (see below).
         */

IIUC, this is specifically so that we don't force the use of TLS 1.2
or TLS 1.1 or TLS 1.0.

It could well be that there's something I don't understand here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, Jun 6, 2017 at 2:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>>> At the end,
>>> everything has been rejected as Postgres enforces the use of the
>>> newest one when doing the SSL handshake.
>>
>> TLS implementations, or TLS versions?  What does the TLS version have
>> to do with this issue?
>
> I really mean *version* here.

I don't think it's true that we force the latest TLS version to be
used.  The comment says:

        /*
         * We use SSLv23_method() because it can negotiate use of the highest
         * mutually supported protocol version, while alternatives like
         * TLSv1_2_method() permit only one specific version.  Note
that we don't
         * actually allow SSL v2 or v3, only TLS protocols (see below).
         */

IIUC, this is specifically so that we don't force the use of TLS 1.2
or TLS 1.1 or TLS 1.0.

It could well be that there's something I don't understand here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> I don't think it's true that we force the latest TLS version to be
> used.  The comment says:

>         /*
>          * We use SSLv23_method() because it can negotiate use of the highest
>          * mutually supported protocol version, while alternatives like
>          * TLSv1_2_method() permit only one specific version.  Note
> that we don't
>          * actually allow SSL v2 or v3, only TLS protocols (see below).
>          */

> IIUC, this is specifically so that we don't force the use of TLS 1.2
> or TLS 1.1 or TLS 1.0.

Right.  IIUC, there's no way (at least in older OpenSSL versions) to say
directly "we only want TLS >= 1.0", so we have to do it like this.
I found a comment on the web saying "SSLv23_method would be better named
AutoNegotiate_method", which seems accurate.

            regards, tom lane


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> I don't think it's true that we force the latest TLS version to be
> used.  The comment says:

>         /*
>          * We use SSLv23_method() because it can negotiate use of the highest
>          * mutually supported protocol version, while alternatives like
>          * TLSv1_2_method() permit only one specific version.  Note
> that we don't
>          * actually allow SSL v2 or v3, only TLS protocols (see below).
>          */

> IIUC, this is specifically so that we don't force the use of TLS 1.2
> or TLS 1.1 or TLS 1.0.

Right.  IIUC, there's no way (at least in older OpenSSL versions) to say
directly "we only want TLS >= 1.0", so we have to do it like this.
I found a comment on the web saying "SSLv23_method would be better named
AutoNegotiate_method", which seems accurate.

            regards, tom lane


Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> As far as I can see, there are a couple of things that I still need to
> work on to make people happy:
> - Rework the generic APIs for TLS finish and endpoint so as any
> implementation can use channel binding without inducing any extra code
> footprint to be-secure.c and fe-secure.c.
> - Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
> - Have a couple of tests for channel binding to allow people to test
> the feature easily. Those will be in src/test/ssl/. It would be nice
> as well to be able to enforce the channel binding type on libpq-side,
> which is useful at least for testing. So we are going to need an
> environment variable for this purpose, and a connection parameter.

Okay, here we go. Attached is a set of four patches:
- 0001 is some refactoring for the SSL tests so as other test suite in
src/test/ssl can take advantage of the connection routines. There is
nothing fancy here.
- 0002 is the implementation of tls-unique as channel binding. This
has been largely reworked since last submission, I have found on the
way a couple of bugs and some correctness issues.
- 0003 is a patch to add as connection parameters saslname and
saslchannelbinding. With support of more SASL mechanisms (PG10 has
SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
used to enforce on the client-side the value of the SASL mechanism
chosen. saslchannelbinding does the same for the channel binding name.
This is very useful for testing, and a set of tests are added in
src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
many scenarios, like downgrade attacks for example.
- 0004 is the implementation of tls-server-end-point, as Alvaro has
asked. Per RFC 5929, the binding data needs to be a hash of the server
certificate. If the signature algorithm of the certificate is MD5 or
SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
are used to hash the data. The hashed data is then encoded in base64
and sent to the server for verification. Tests using saslchannelname
have been added as well. It took me a while to find out that
OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
find out the algorithm of a certificate with OpenSSL.

With the tests directly in the patch, things are easy to run. WIth
PG10 stabilization work, of course I don't expect much feedback :)
But this set of patches looks like the direction we want to go so as
JDBC and libpq users can take advantage of channel binding with SCRAM.
-- 
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> As far as I can see, there are a couple of things that I still need to
> work on to make people happy:
> - Rework the generic APIs for TLS finish and endpoint so as any
> implementation can use channel binding without inducing any extra code
> footprint to be-secure.c and fe-secure.c.
> - Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
> - Have a couple of tests for channel binding to allow people to test
> the feature easily. Those will be in src/test/ssl/. It would be nice
> as well to be able to enforce the channel binding type on libpq-side,
> which is useful at least for testing. So we are going to need an
> environment variable for this purpose, and a connection parameter.

Okay, here we go. Attached is a set of four patches:
- 0001 is some refactoring for the SSL tests so as other test suite in
src/test/ssl can take advantage of the connection routines. There is
nothing fancy here.
- 0002 is the implementation of tls-unique as channel binding. This
has been largely reworked since last submission, I have found on the
way a couple of bugs and some correctness issues.
- 0003 is a patch to add as connection parameters saslname and
saslchannelbinding. With support of more SASL mechanisms (PG10 has
SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
used to enforce on the client-side the value of the SASL mechanism
chosen. saslchannelbinding does the same for the channel binding name.
This is very useful for testing, and a set of tests are added in
src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
many scenarios, like downgrade attacks for example.
- 0004 is the implementation of tls-server-end-point, as Alvaro has
asked. Per RFC 5929, the binding data needs to be a hash of the server
certificate. If the signature algorithm of the certificate is MD5 or
SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
are used to hash the data. The hashed data is then encoded in base64
and sent to the server for verification. Tests using saslchannelname
have been added as well. It took me a while to find out that
OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
find out the algorithm of a certificate with OpenSSL.

With the tests directly in the patch, things are easy to run. WIth
PG10 stabilization work, of course I don't expect much feedback :)
But this set of patches looks like the direction we want to go so as
JDBC and libpq users can take advantage of channel binding with SCRAM.
-- 
Michael

Attachment

Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 20/06/17 06:11, Michael Paquier wrote:
> On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> As far as I can see, there are a couple of things that I still need to
>> work on to make people happy:
>> - Rework the generic APIs for TLS finish and endpoint so as any
>> implementation can use channel binding without inducing any extra code
>> footprint to be-secure.c and fe-secure.c.
>> - Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
>> - Have a couple of tests for channel binding to allow people to test
>> the feature easily. Those will be in src/test/ssl/. It would be nice
>> as well to be able to enforce the channel binding type on libpq-side,
>> which is useful at least for testing. So we are going to need an
>> environment variable for this purpose, and a connection parameter.
> Okay, here we go. Attached is a set of four patches:
> - 0001 is some refactoring for the SSL tests so as other test suite in
> src/test/ssl can take advantage of the connection routines. There is
> nothing fancy here.
> - 0002 is the implementation of tls-unique as channel binding. This
> has been largely reworked since last submission, I have found on the
> way a couple of bugs and some correctness issues.
> - 0003 is a patch to add as connection parameters saslname and
> saslchannelbinding. With support of more SASL mechanisms (PG10 has
> SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
> used to enforce on the client-side the value of the SASL mechanism
> chosen. saslchannelbinding does the same for the channel binding name.
> This is very useful for testing, and a set of tests are added in
> src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
> many scenarios, like downgrade attacks for example.
> - 0004 is the implementation of tls-server-end-point, as Alvaro has
> asked. Per RFC 5929, the binding data needs to be a hash of the server
> certificate. If the signature algorithm of the certificate is MD5 or
> SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
> are used to hash the data. The hashed data is then encoded in base64
> and sent to the server for verification. Tests using saslchannelname
> have been added as well. It took me a while to find out that
> OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
> find out the algorithm of a certificate with OpenSSL.
>
> With the tests directly in the patch, things are easy to run. WIth
> PG10 stabilization work, of course I don't expect much feedback :)
> But this set of patches looks like the direction we want to go so as
> JDBC and libpq users can take advantage of channel binding with SCRAM.

     This is awesome, Michael.

     In the coming weeks, and once my PR for pgjdbc has been added, I
will work towards another patch to implement channel binding. Should be
reasonably easy now, thanks to this.

     Appreciated!

     Álvaro



--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Álvaro Hernández Tortosa
Date:

On 20/06/17 06:11, Michael Paquier wrote:
> On Tue, Jun 6, 2017 at 3:40 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> As far as I can see, there are a couple of things that I still need to
>> work on to make people happy:
>> - Rework the generic APIs for TLS finish and endpoint so as any
>> implementation can use channel binding without inducing any extra code
>> footprint to be-secure.c and fe-secure.c.
>> - Implement endpoint, as Alvaro is saying for JDBC that would be nicer.
>> - Have a couple of tests for channel binding to allow people to test
>> the feature easily. Those will be in src/test/ssl/. It would be nice
>> as well to be able to enforce the channel binding type on libpq-side,
>> which is useful at least for testing. So we are going to need an
>> environment variable for this purpose, and a connection parameter.
> Okay, here we go. Attached is a set of four patches:
> - 0001 is some refactoring for the SSL tests so as other test suite in
> src/test/ssl can take advantage of the connection routines. There is
> nothing fancy here.
> - 0002 is the implementation of tls-unique as channel binding. This
> has been largely reworked since last submission, I have found on the
> way a couple of bugs and some correctness issues.
> - 0003 is a patch to add as connection parameters saslname and
> saslchannelbinding. With support of more SASL mechanisms (PG10 has
> SCRAM-SHA-256, I am adding SCRAM-SHA-256-PLUS here), saslname can be
> used to enforce on the client-side the value of the SASL mechanism
> chosen. saslchannelbinding does the same for the channel binding name.
> This is very useful for testing, and a set of tests are added in
> src/test/ssl/ for tls-unique and the SASL mechanisms. The tests cover
> many scenarios, like downgrade attacks for example.
> - 0004 is the implementation of tls-server-end-point, as Alvaro has
> asked. Per RFC 5929, the binding data needs to be a hash of the server
> certificate. If the signature algorithm of the certificate is MD5 or
> SHA-1, then SHA-256 is used. Other signature algos like SHA-384 or 512
> are used to hash the data. The hashed data is then encoded in base64
> and sent to the server for verification. Tests using saslchannelname
> have been added as well. It took me a while to find out that
> OBJ_find_sigid_algs(X509_get_signature_nid(X509*)) needs to be used to
> find out the algorithm of a certificate with OpenSSL.
>
> With the tests directly in the patch, things are easy to run. WIth
> PG10 stabilization work, of course I don't expect much feedback :)
> But this set of patches looks like the direction we want to go so as
> JDBC and libpq users can take advantage of channel binding with SCRAM.

     This is awesome, Michael.

     In the coming weeks, and once my PR for pgjdbc has been added, I
will work towards another patch to implement channel binding. Should be
reasonably easy now, thanks to this.

     Appreciated!

     Álvaro



--

Álvaro Hernández Tortosa


-----------
<8K>data



Re: [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Jun 21, 2017 at 4:04 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
>     In the coming weeks, and once my PR for pgjdbc has been added, I will
> work towards another patch to implement channel binding. Should be
> reasonably easy now, thanks to this.

So you basically have an equivalent of OpenSSL stuff in java, right?
- SSL_get_peer_certificate to get the X509 point of the server.
- X509_digest to hash it.
- OBJ_find_sigid_algs and X509_get_signature_nid to guess the
signature algorithm of a certificate. I think that this part can be
tricky depending on the SSL implementation, but I have designed a
generic API for this purpose.
That's all it took me to get end-point to work. Plus the error
handling of course.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Jun 21, 2017 at 4:04 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:
>     In the coming weeks, and once my PR for pgjdbc has been added, I will
> work towards another patch to implement channel binding. Should be
> reasonably easy now, thanks to this.

So you basically have an equivalent of OpenSSL stuff in java, right?
- SSL_get_peer_certificate to get the X509 point of the server.
- X509_digest to hash it.
- OBJ_find_sigid_algs and X509_get_signature_nid to guess the
signature algorithm of a certificate. I think that this part can be
tricky depending on the SSL implementation, but I have designed a
generic API for this purpose.
That's all it took me to get end-point to work. Plus the error
handling of course.
--
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> With the tests directly in the patch, things are easy to run. WIth
> PG10 stabilization work, of course I don't expect much feedback :)
> But this set of patches looks like the direction we want to go so as
> JDBC and libpq users can take advantage of channel binding with SCRAM.

Attached is a new patch set, rebased as of c6293249.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Mon, Aug 21, 2017 at 9:51 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> With the tests directly in the patch, things are easy to run. WIth
>> PG10 stabilization work, of course I don't expect much feedback :)
>> But this set of patches looks like the direction we want to go so as
>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>
> Attached is a new patch set, rebased as of c6293249.

And again a new set to fix the rotten bits caused by 85f4d63.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 9/10/17 22:37, Michael Paquier wrote:
> On Mon, Aug 21, 2017 at 9:51 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> With the tests directly in the patch, things are easy to run. WIth
>>> PG10 stabilization work, of course I don't expect much feedback :)
>>> But this set of patches looks like the direction we want to go so as
>>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>>
>> Attached is a new patch set, rebased as of c6293249.
> 
> And again a new set to fix the rotten bits caused by 85f4d63.

It seems we should start by sorting out the mechanism by which the
client can control what authentication mechanisms it accepts.  In your
patch set you introduce a connection parameter saslname.  I think we
should expand that to non-SASL mechanisms and have it be some kind of
whitelist or blacklist.  It might be reasonable for a client to require
"gssapi" or "cert" for example or do an exclusion like "!password !md5
!ldap".

Thoughts?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Sep 12, 2017 at 11:38 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> It seems we should start by sorting out the mechanism by which the
> client can control what authentication mechanisms it accepts.  In your
> patch set you introduce a connection parameter saslname.  I think we
> should expand that to non-SASL mechanisms and have it be some kind of
> whitelist or blacklist.  It might be reasonable for a client to require
> "gssapi" or "cert" for example or do an exclusion like "!password !md5
> !ldap".
>
> Thoughts?

That looks like a sensible approach to begin with at the end: there
have been complains that a client can be tricked into using MD5 by a
rogue server even if it was willing to use SCRAM. So what about a
parameter called pgauthfilter, which uses a comma-separated list of
keywords. As you say, using an exclamation point to negate an
authentication method is fine for me. For SCRAM, we could just use
"scram-sha-256" as keyword.

Once channel binding is involved though.. This needs to be extended
and this needs careful thoughts:
* "scram-sha-256" means that the version without channel binding is
accepted. "!scram-sha-256" means that scram without channel binding is
refused.
* "scram-sha-256-plus" means that all channel bindings are accepted.
"!scram-sha-256-plus" means that no channel binding are accepted.
After that there is some filtering per channel binding name. Do we
want a separate parameter or just filter with longer names like
"scram-sha-256-plus-tls-unique" and
"scram-sha-256-plus-tls-server-end-point"? The last one gets
particularly long, this does not help users with typos :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 9/12/17 19:03, Michael Paquier wrote:
> Once channel binding is involved though.. This needs to be extended
> and this needs careful thoughts:
> * "scram-sha-256" means that the version without channel binding is
> accepted. "!scram-sha-256" means that scram without channel binding is
> refused.
> * "scram-sha-256-plus" means that all channel bindings are accepted.
> "!scram-sha-256-plus" means that no channel binding are accepted.
> After that there is some filtering per channel binding name. Do we
> want a separate parameter or just filter with longer names like
> "scram-sha-256-plus-tls-unique" and
> "scram-sha-256-plus-tls-server-end-point"? The last one gets
> particularly long, this does not help users with typos :)

Second thoughts, to make things simpler.  All we need for channel
binding is a connection flag that says "I require channel binding".  It
could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
for debugging), cbind=prefer (default), cbind=require.  If you specify
"require", then libpq would refuse to proceed unless scram-sha2-256-plus
(or future similar mechanisms) was offered for authentication.

We don't even need a parameter that specifies which channel binding type
to use.  If libpq implements tls-unique, it should always use that.  We
might need a flag for testing other types, but that should not be an
in-the-user's-face option.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Sep 15, 2017 at 1:58 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Second thoughts, to make things simpler.  All we need for channel
> binding is a connection flag that says "I require channel binding".  It
> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
> for debugging), cbind=prefer (default), cbind=require.  If you specify
> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
> (or future similar mechanisms) was offered for authentication.
>
> We don't even need a parameter that specifies which channel binding type
> to use.  If libpq implements tls-unique, it should always use that.  We
> might need a flag for testing other types, but that should not be an
> in-the-user's-face option.

JDBC folks are willing to have end-point, and we should have a way to
enforce it in my opinion for at least testing with an implementation
at client-level in Postgres core. I agree that without the JDBC needs
having a on/off switch would be sufficient, and actually RFC compliant
as tls-unique is mandatory.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Thu, Sep 14, 2017 at 12:58 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Second thoughts, to make things simpler.  All we need for channel
> binding is a connection flag that says "I require channel binding".  It
> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
> for debugging), cbind=prefer (default), cbind=require.  If you specify
> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
> (or future similar mechanisms) was offered for authentication.

+1, although I think cbind is too brief.  I'd spell it out.

> We don't even need a parameter that specifies which channel binding type
> to use.  If libpq implements tls-unique, it should always use that.  We
> might need a flag for testing other types, but that should not be an
> in-the-user's-face option.

I'm not so sure about this part.  Why don't we want to let users control this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Sat, Sep 16, 2017 at 12:42 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Sep 14, 2017 at 12:58 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Second thoughts, to make things simpler.  All we need for channel
>> binding is a connection flag that says "I require channel binding".  It
>> could be modeled after the sslmode parameter, e.g., cbind=disable (maybe
>> for debugging), cbind=prefer (default), cbind=require.  If you specify
>> "require", then libpq would refuse to proceed unless scram-sha2-256-plus
>> (or future similar mechanisms) was offered for authentication.
>
> +1, although I think cbind is too brief.  I'd spell it out.

I would like to point out that per the RFC, if the client attempts a
SSL connection with SCRAM and that the server supports channel
binding, then it has to publish the SASL mechanism for channel
binding, aka SCRAM-PLUS. If the client tries to force the use of SCRAM
even if SCRAM-PLUS is specified, this is seen as a downgrade attack by
the server which must reject the connection. So this parameter has
meaning only if you try to connect to a PG10 server using a PG11
client (assuming that channel binding gets into PG11). If you connect
with a PG11 client to a PG11 server with SSL, the server publishes
SCRAM-PLUS, the client has to use it, hence this turns out to make
cbind=disable and prefer meaningless in the long-term. If the client
does not use SSL, then there is no channel binding, and cbind=require
loses its value. So cbind's fate is actually linked to sslmode.

>> We don't even need a parameter that specifies which channel binding type
>> to use.  If libpq implements tls-unique, it should always use that.  We
>> might need a flag for testing other types, but that should not be an
>> in-the-user's-face option.
>
> I'm not so sure about this part.  Why don't we want to let users control this?

That's at least useful for at least testing, tls-unique should be the
default, but we need as well end-point which we much likely would like
to be able to enforce from the client, and being able to enforce the
type of channel binding used does not betray the RFC.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 9/10/17 22:37, Michael Paquier wrote:
>>> With the tests directly in the patch, things are easy to run. WIth
>>> PG10 stabilization work, of course I don't expect much feedback :)
>>> But this set of patches looks like the direction we want to go so as
>>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>>
>> Attached is a new patch set, rebased as of c6293249.
> 
> And again a new set to fix the rotten bits caused by 85f4d63.

Here is a review of the meat of the code, leaving aside the discussion
of the libpq connection parameters.

Overall, the structure of the code makes sense and it fits in well with
the existing SCRAM code.


I think the channel-binding negotiation on the client side is wrong.
The logic in the patch is

+#ifdef USE_SSL
+   if (state->ssl_in_use)
+       appendPQExpBuffer(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
+   else
+       appendPQExpBuffer(&buf, "y");
+#else
+   appendPQExpBuffer(&buf, "n");
+#endif

But if SSL is compiled in but not used, the client does not in fact
support channel binding (for that connection), so it should send "n".

The "y" flag should be sent if ssl_in_use but the client did not see the
server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
this patch.


You have the server reject a client that does not support channel
binding ("n") on all SSL connections.  I don't think this is correct.
It is up to the client to use channel binding or not, even on SSL
connections.


We should update pg_hba.conf to allow a method specification of
"scram-sha256-plus", i.e., only advertise the channel binding variant to
the client.  Then you could make policy decisions like rejecting clients
that do not use channel binding on SSL connections.  This could be a
separate patch later.


The error message in the "p" case if SSL is not in use is a bit
confusing: "but the server does not need it".  I think this could be
left at the old message "but it is not supported".  This ties into my
interpretation from above that whether channel binding is "supported"
depends on whether SSL is in use for a particular connection.


Some small code things:

- prefer to use size_t over int for length (tls_finish_len etc.)

- tls_finish should be tls_finished

- typos: certificate_bash -> certificate_hash


In the patch for tls-server-end-point, I think the selection of the hash
function deviates slightly from the RFC.  The RFC only says to
substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
for example.  There is also the problem that the code as written will
turn any unrecognized hash method into SHA-256.  If think the code
should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
for the rest.  (I don't know anything about the details of OpenSSL APIs,
but that function sounded right to me.)

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Here is a review of the meat of the code, leaving aside the discussion
> of the libpq connection parameters.
>
> Overall, the structure of the code makes sense and it fits in well with
> the existing SCRAM code.

Thanks for the review, Peter.

> I think the channel-binding negotiation on the client side is wrong.
> The logic in the patch is
>
> +#ifdef USE_SSL
> +   if (state->ssl_in_use)
> +       appendPQExpBuffer(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
> +   else
> +       appendPQExpBuffer(&buf, "y");
> +#else
> +   appendPQExpBuffer(&buf, "n");
> +#endif
>
> But if SSL is compiled in but not used, the client does not in fact
> support channel binding (for that connection), so it should send "n".

For others, details about this flag are here:  gs2-cbind-flag  = ("p=" cb-name) / "n" / "y"                    ;; "n"
->client doesn't support channel binding.                    ;; "y" -> client does support channel binding
     ;;        but thinks the server does not.                    ;; "p" -> client requires channel binding.
       ;; The selected channel binding follows "p=".
 

And channel binding description is here:
https://tools.ietf.org/html/rfc5802#section-6

> The "y" flag should be sent if ssl_in_use but the client did not see the
> server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
> this patch.

Okay. I think I get your point here. I agree that the client is
deficient here. This needs some more work.

> You have the server reject a client that does not support channel
> binding ("n") on all SSL connections.  I don't think this is correct.
> It is up to the client to use channel binding or not, even on SSL
> connections.

It seems that I got confused with the meaning of "y" mixed with
ssl_in_use. The server should reject "y" instead only if SSL is in
use.

> We should update pg_hba.conf to allow a method specification of
> "scram-sha256-plus", i.e., only advertise the channel binding variant to
> the client.  Then you could make policy decisions like rejecting clients
> that do not use channel binding on SSL connections.  This could be a
> separate patch later.

OK, I agree that there could be some value in that. This complicates a
bit hba rule checks, but nothing really complicated either.

> The error message in the "p" case if SSL is not in use is a bit
> confusing: "but the server does not need it".  I think this could be
> left at the old message "but it is not supported".  This ties into my
> interpretation from above that whether channel binding is "supported"
> depends on whether SSL is in use for a particular connection.

Check.

> Some small code things:
> - prefer to use size_t over int for length (tls_finish_len etc.)
> - tls_finish should be tls_finished
> - typos: certificate_bash -> certificate_hash

Yes, thanks for spotting those.

> In the patch for tls-server-end-point, I think the selection of the hash
> function deviates slightly from the RFC.  The RFC only says to
> substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
> for example.  There is also the problem that the code as written will
> turn any unrecognized hash method into SHA-256.  If think the code
> should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
> for the rest.  (I don't know anything about the details of OpenSSL APIs,
> but that function sounded right to me.)

Relevant bits from the RFC: https://tools.ietf.org/html/rfc5929#section-4.1  o  if the certificate's signatureAlgorithm
usesa single hash     function, and that hash function is either MD5 [RFC1321] or SHA-1     [RFC3174], then use SHA-256
[FIPS-180-3]; o  if the certificate's signatureAlgorithm uses no hash functions or     uses multiple hash functions,
thenthis channel binding type's     channel bindings are undefined at this time (updates to is channel     binding type
mayoccur to address this issue if it ever arises).
 

OK. Hm. I think that we need as well to check for the case where
EVP_get_digestbynid() returns NULL then and issue an ERROR on it. That
seems to be the second point outlined by the RFC I am quoting here.

This patch got the feedback I was looking for, and this requires some
rework anyway. So I am marking the patch as returned with feedback for
now. This won't make it in time for this CF.

Thanks Peter for looking at it and pointing out some deficiencies.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Fri, Sep 15, 2017 at 6:29 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> I would like to point out that per the RFC, if the client attempts a
> SSL connection with SCRAM and that the server supports channel
> binding, then it has to publish the SASL mechanism for channel
> binding, aka SCRAM-PLUS. If the client tries to force the use of SCRAM
> even if SCRAM-PLUS is specified, this is seen as a downgrade attack by
> the server which must reject the connection. So this parameter has
> meaning only if you try to connect to a PG10 server using a PG11
> client (assuming that channel binding gets into PG11). If you connect
> with a PG11 client to a PG11 server with SSL, the server publishes
> SCRAM-PLUS, the client has to use it, hence this turns out to make
> cbind=disable and prefer meaningless in the long-term. If the client
> does not use SSL, then there is no channel binding, and cbind=require
> loses its value. So cbind's fate is actually linked to sslmode.

That seems problematic.  What if the client supports SCRAM but not
channel binding?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Oct 3, 2017 at 1:30 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Sep 15, 2017 at 6:29 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> I would like to point out that per the RFC, if the client attempts a
>> SSL connection with SCRAM and that the server supports channel
>> binding, then it has to publish the SASL mechanism for channel
>> binding, aka SCRAM-PLUS. If the client tries to force the use of SCRAM
>> even if SCRAM-PLUS is specified, this is seen as a downgrade attack by
>> the server which must reject the connection. So this parameter has
>> meaning only if you try to connect to a PG10 server using a PG11
>> client (assuming that channel binding gets into PG11). If you connect
>> with a PG11 client to a PG11 server with SSL, the server publishes
>> SCRAM-PLUS, the client has to use it, hence this turns out to make
>> cbind=disable and prefer meaningless in the long-term. If the client
>> does not use SSL, then there is no channel binding, and cbind=require
>> loses its value. So cbind's fate is actually linked to sslmode.
>
> That seems problematic.  What if the client supports SCRAM but not
> channel binding?

Peter has outlined here that my interpretation of the RFC was wrong on
the client side to begin with:
https://www.postgresql.org/message-id/f74525e4-6c53-c653-6860-a8cc8d7c8ad9@2ndquadrant.com
If a client does not support channel binding (it is not compiled with
SSL or the connection is done without SSL), it should not send 'y' but
'n'. It should be up to the client to decide if it wants to use
channel binding or not. libpq is also going to need some extra logic
to send 'y' when it thinks that the server should have channel binding
support. This can be done by looking at the backend version.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Sep 26, 2017 at 11:09 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> I think the channel-binding negotiation on the client side is wrong.
>> The logic in the patch is
>>
>> +#ifdef USE_SSL
>> +   if (state->ssl_in_use)
>> +       appendPQExpBuffer(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
>> +   else
>> +       appendPQExpBuffer(&buf, "y");
>> +#else
>> +   appendPQExpBuffer(&buf, "n");
>> +#endif
>>
>> But if SSL is compiled in but not used, the client does not in fact
>> support channel binding (for that connection), so it should send "n".
>
> For others, details about this flag are here:
>    gs2-cbind-flag  = ("p=" cb-name) / "n" / "y"
>                      ;; "n" -> client doesn't support channel binding.
>                      ;; "y" -> client does support channel binding
>                      ;;        but thinks the server does not.
>                      ;; "p" -> client requires channel binding.
>                      ;; The selected channel binding follows "p=".
>
> And channel binding description is here:
> https://tools.ietf.org/html/rfc5802#section-6

Changed the patch to do that. Note that if the client enforces the
SASL mechanism to SCRAM-SHA-256-PLUS in a non-SSL context then libpq
complains. This can be done in libpq using pgsaslname.

>> The "y" flag should be sent if ssl_in_use but the client did not see the
>> server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
>> this patch.
>
> Okay. I think I get your point here. I agree that the client is
> deficient here. This needs some more work.

Hm. I take back the argument that we can use the backend version here.
conn->sversion is only filled at the end of authentication. So the
best thing I think can be done here is to check if channel binding has
been advertised or not, and send "y" if the client thinks that the
server should do support it. If the client has chosen not to use
channel binding, by for example enforcing pgsaslname in libpq to
SCRAM-SHA-256, then "n" should be sent. I have changed the patch
accordingly, doing at the same time some refactoring in pg_SASL_init.
pg_fe_scram_init() gets initialized only when the mechanism is
selected.

>> You have the server reject a client that does not support channel
>> binding ("n") on all SSL connections.  I don't think this is correct.
>> It is up to the client to use channel binding or not, even on SSL
>> connections.
>
> It seems that I got confused with the meaning of "y" mixed with
> ssl_in_use. The server should reject "y" instead only if SSL is in
> use.

Okay. In order to address that, what just needs to be done is to
remove the error message that I have added in the backend when "n" is
sent by the client. So this thing is ripped off. This way, when a v10
libpq connects to a v11 backend, the connection is able to work even
with SSL connections.

>> We should update pg_hba.conf to allow a method specification of
>> "scram-sha256-plus", i.e., only advertise the channel binding variant to
>> the client.  Then you could make policy decisions like rejecting clients
>> that do not use channel binding on SSL connections.  This could be a
>> separate patch later.
>
> OK, I agree that there could be some value in that. This complicates a
> bit hba rule checks, but nothing really complicated either.

This would be useful, but I have let it for now to not complicate the
series more.

>> The error message in the "p" case if SSL is not in use is a bit
>> confusing: "but the server does not need it".  I think this could be
>> left at the old message "but it is not supported".  This ties into my
>> interpretation from above that whether channel binding is "supported"
>> depends on whether SSL is in use for a particular connection.
>
> Check.

Okay, I switched the error message to that (diff with previous patch series):
@@ -850,7 +850,7 @@ read_client_first_message(scram_state *state, char *input)
                if (!state->ssl_in_use)
                    ereport(ERROR,
                            (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                            errmsg("client supports SCRAM channel
binding, but server does not need it for non-SSL connections")));
+                            errmsg("client requires SCRAM channel
binding, but it is not supported")));

>> Some small code things:
>> - prefer to use size_t over int for length (tls_finish_len etc.)
>> - tls_finish should be tls_finished
>> - typos: certificate_bash -> certificate_hash
>
> Yes, thanks for spotting those.

Fixed.

>> In the patch for tls-server-end-point, I think the selection of the hash
>> function deviates slightly from the RFC.  The RFC only says to
>> substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
>> for example.  There is also the problem that the code as written will
>> turn any unrecognized hash method into SHA-256.  If think the code
>> should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
>> for the rest.  (I don't know anything about the details of OpenSSL APIs,
>> but that function sounded right to me.)
>
> Relevant bits from the RFC: https://tools.ietf.org/html/rfc5929#section-4.1
>    o  if the certificate's signatureAlgorithm uses a single hash
>       function, and that hash function is either MD5 [RFC1321] or SHA-1
>       [RFC3174], then use SHA-256 [FIPS-180-3];
>    o  if the certificate's signatureAlgorithm uses no hash functions or
>       uses multiple hash functions, then this channel binding type's
>       channel bindings are undefined at this time (updates to is channel
>       binding type may occur to address this issue if it ever arises).
>
> OK. Hm. I think that we need as well to check for the case where
> EVP_get_digestbynid() returns NULL then and issue an ERROR on it. That
> seems to be the second point outlined by the RFC I am quoting here.

This has been fixed. i have noticed on the way that the error messages
in the frontend when a certificate hash cannot be generated just
complain about an OOM. This is not always correct, so I have made
things more verbose with better error messages for each error code
path.

> This patch got the feedback I was looking for, and this requires some
> rework anyway. So I am marking the patch as returned with feedback for
> now. This won't make it in time for this CF.

Attached is a new patch set with the comments from above. On top of
that, I have changed a couple of things:
- 0001 is unchanged, still the same refactoring for the SSL tests.
- 0002 implements tls-unique, now including tests using the default
channel binding tls-unique with something in the SSL test suite. This
patch also now introduces all the infrastructure to plug in correctly
new libpq parameters and more channel binding types.
- 0003 is shorter, and introduces a set of libpq parameters useful for
tests, taking advantage of 0002. Another case where the connection
parameter saslname is useful is to enforce not using channel binding
when connecting to a v10 server using a SSL context with a v11 libpq.
- 0004 introduces tls-server-end-point.
This has required some work to get it shaped as wanted, I am adding it
to the next CF, as version 2.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Oct 10, 2017 at 10:12 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Attached is a new patch set with the comments from above. On top of
> that, I have changed a couple of things:
> - 0001 is unchanged, still the same refactoring for the SSL tests.
> - 0002 implements tls-unique, now including tests using the default
> channel binding tls-unique with something in the SSL test suite. This
> patch also now introduces all the infrastructure to plug in correctly
> new libpq parameters and more channel binding types.
> - 0003 is shorter, and introduces a set of libpq parameters useful for
> tests, taking advantage of 0002. Another case where the connection
> parameter saslname is useful is to enforce not using channel binding
> when connecting to a v10 server using a SSL context with a v11 libpq.
> - 0004 introduces tls-server-end-point.
> This has required some work to get it shaped as wanted, I am adding it
> to the next CF, as version 2.

Documentation in protocol.sgml has rotten again as markups need proper
handling. So rebased.
-- 
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 11/14/17 03:55, Michael Paquier wrote:
> On Tue, Oct 10, 2017 at 10:12 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Attached is a new patch set with the comments from above. On top of
>> that, I have changed a couple of things:
>> - 0001 is unchanged, still the same refactoring for the SSL tests.
>> - 0002 implements tls-unique, now including tests using the default
>> channel binding tls-unique with something in the SSL test suite. This
>> patch also now introduces all the infrastructure to plug in correctly
>> new libpq parameters and more channel binding types.
>> - 0003 is shorter, and introduces a set of libpq parameters useful for
>> tests, taking advantage of 0002. Another case where the connection
>> parameter saslname is useful is to enforce not using channel binding
>> when connecting to a v10 server using a SSL context with a v11 libpq.
>> - 0004 introduces tls-server-end-point.
>> This has required some work to get it shaped as wanted, I am adding it
>> to the next CF, as version 2.
> 
> Documentation in protocol.sgml has rotten again as markups need proper
> handling. So rebased.

Pushed 0001, will continue with 0002.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Thu, Nov 16, 2017 at 10:47 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Pushed 0001, will continue with 0002.

Thanks!
-- 
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 11/16/17 16:50, Michael Paquier wrote:
> On Thu, Nov 16, 2017 at 10:47 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Pushed 0001, will continue with 0002.
> 
> Thanks!

I have combed through the 0002 patch in detail now.  I have attached my
result.

I cleaned up a bunch of variable names to be more precise.  I also
removed some unnecessary code.  For example the whole deal about
channel_binding_advertised was not necessary, because it's implied by
the chosen SASL mechanism.  We also don't need to have separate USE_SSL
sections in most cases, because state->ssl_in_use already takes care of
that.

I made some significant changes to the logic.

The selection of the channel binding flag (n/y/p) in the client seemed
wrong.  Your code treated 'y' as an error, but I think that is a
legitimate case, for example a PG11 client connecting to a PG10 server
over SSL.  The client supports channel binding in that case and
(correctly) thinks the server does not, so we use the 'y' flag and
proceed normally without channel binding.

The selection of the mechanism in the client was similarly incorrect, I
think.  The code would not tolerate a situation were an SSL connection
is in use but the server does not advertise the -PLUS mechanism, which
again could be from a PG10 server.

The creation of the channel binding data didn't match the spec, because
the gs2-header (p=type,,) was not included in the data put through
base64.  This was done incorrectly on both server and client, so the
protocol still worked.  (However, in the non-channel-binding case we
hardcode "biws", which is exactly the base64-encoding of the gs2-header.
 So that was inconsistent.)

I think we also need to backpatch a bug fix into PG10 so that the server
can accept base64("y,,") as channel binding data.  Otherwise, the above
scenario of a PG11 client connecting to a PG10 server over SSL will
currently fail because the server will not accept the channel binding data.

Please check my patch and think through these changes.  I'm happy to
commit the patch as is if there are no additional insights.

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

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Sat, Nov 18, 2017 at 4:31 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I made some significant changes to the logic.

Thanks!

> The selection of the channel binding flag (n/y/p) in the client seemed
> wrong.  Your code treated 'y' as an error, but I think that is a
> legitimate case, for example a PG11 client connecting to a PG10 server
> over SSL.  The client supports channel binding in that case and
> (correctly) thinks the server does not, so we use the 'y' flag and
> proceed normally without channel binding.
>
> The selection of the mechanism in the client was similarly incorrect, I
> think.  The code would not tolerate a situation were an SSL connection
> is in use but the server does not advertise the -PLUS mechanism, which
> again could be from a PG10 server.

Hm, OK. I have been likely confused by the fact that eSws is a valid
b64-encoded cbind-input on v10 servers. And the spec has no direct
mention of the matter, only of biws.

> The creation of the channel binding data didn't match the spec, because
> the gs2-header (p=type,,) was not included in the data put through
> base64.  This was done incorrectly on both server and client, so the
> protocol still worked.  (However, in the non-channel-binding case we
> hardcode "biws", which is exactly the base64-encoding of the gs2-header.
>  So that was inconsistent.)

Meh-to-self, you are right. Still it seems to me that your version is
forgetting something.. Please see below.

> I think we also need to backpatch a bug fix into PG10 so that the server
> can accept base64("y,,") as channel binding data.  Otherwise, the above
> scenario of a PG11 client connecting to a PG10 server over SSL will
> currently fail because the server will not accept the channel binding data.

Yes, without the additional comparison, the failure is weird for the
user. Here is what I have with an unpatched v10 server:
psql: FATAL:  unexpected SCRAM channel-binding attribute in client-final-message
This is going to need a one-liner in read_client_final_message()'s auth-scram.c.

> Please check my patch and think through these changes.  I'm happy to
> commit the patch as is if there are no additional insights.

Here are some comments.

+            * The client requires channel biding.  Channel binding type
s/biding/binding/.
               if (!state->ssl_in_use)
+                   /*
+                    * Without SSL, we don't support channel binding.
+                    *
Better to add brackets if adding a comment.

+                * Read value provided by client; only tls-unique is supported
+                * for now.  XXX Not sure whether it would be safe to print
+                * the name of an unsupported binding type in the error
+                * message.  Pranksters could print arbitrary strings into the
+                * log that way.
That's why I didn't show those strings in the error messages of the
previous versions. Those are useless as well, except for debugging the
feature and the protocol.

+       cbind_header_len = 4 + strlen(state->channel_binding_type); /*
p=type,, */
+       cbind_input_len = cbind_header_len + cbind_data_len;
+       cbind_input = malloc(cbind_input_len);
+       if (!cbind_input)
+           goto oom_error;
+       snprintf(cbind_input, cbind_input_len, "p=%s",
state->channel_binding_type);
+       memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
By looking at RFC5802, a base64 encoding of cbind-input is used:
cbind-input   = gs2-header [ cbind-data ]
gs2-cbind-flag "," [ authzid ] ","
However you are missing two commands after p=%s, no?
-- 
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 11/18/17 06:32, Michael Paquier wrote:
> Here are some comments.
> 
> +            * The client requires channel biding.  Channel binding type
> s/biding/binding/.

fixed

>                 if (!state->ssl_in_use)
> +                   /*
> +                    * Without SSL, we don't support channel binding.
> +                    *
> Better to add brackets if adding a comment.

done

> +                * Read value provided by client; only tls-unique is supported
> +                * for now.  XXX Not sure whether it would be safe to print
> +                * the name of an unsupported binding type in the error
> +                * message.  Pranksters could print arbitrary strings into the
> +                * log that way.
> That's why I didn't show those strings in the error messages of the
> previous versions. Those are useless as well, except for debugging the
> feature and the protocol.

Right.  I left the comment in there as a note to future hackers who want
to improve error messages (often myself).

> +       cbind_header_len = 4 + strlen(state->channel_binding_type); /*
> p=type,, */
> +       cbind_input_len = cbind_header_len + cbind_data_len;
> +       cbind_input = malloc(cbind_input_len);
> +       if (!cbind_input)
> +           goto oom_error;
> +       snprintf(cbind_input, cbind_input_len, "p=%s",
> state->channel_binding_type);
> +       memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
> By looking at RFC5802, a base64 encoding of cbind-input is used:
> cbind-input   = gs2-header [ cbind-data ]
> gs2-cbind-flag "," [ authzid ] ","
> However you are missing two commands after p=%s, no?

fixed

I have committed the patch with the above fixes.

I'll be off for a week, so perhaps by that time you could make a rebased
version of the rest?  I'm not sure how much more time I'll have, so
maybe it will end up being moved to the next CF.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Sun, Nov 19, 2017 at 12:56 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 11/18/17 06:32, Michael Paquier wrote:
>> +       cbind_header_len = 4 + strlen(state->channel_binding_type); /*
>> p=type,, */
>> +       cbind_input_len = cbind_header_len + cbind_data_len;
>> +       cbind_input = malloc(cbind_input_len);
>> +       if (!cbind_input)
>> +           goto oom_error;
>> +       snprintf(cbind_input, cbind_input_len, "p=%s",
>> state->channel_binding_type);
>> +       memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
>> By looking at RFC5802, a base64 encoding of cbind-input is used:
>> cbind-input   = gs2-header [ cbind-data ]
>> gs2-cbind-flag "," [ authzid ] ","
>> However you are missing two commands after p=%s, no?
>
> fixed

s/commands/commas/. You caught my words correctly.

> I have committed the patch with the above fixes.

Thanks, Peter!

> I'll be off for a week, so perhaps by that time you could make a rebased
> version of the rest?  I'm not sure how much more time I'll have, so
> maybe it will end up being moved to the next CF.

OK, let's see then. That's not an issue for me if this gets bumped.
-- 
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Sun, Nov 19, 2017 at 8:13 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sun, Nov 19, 2017 at 12:56 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> I'll be off for a week, so perhaps by that time you could make a rebased
>> version of the rest?  I'm not sure how much more time I'll have, so
>> maybe it will end up being moved to the next CF.
>
> OK, let's see then. That's not an issue for me if this gets bumped.

I have been considering the set of connection parameters introduced
previously, and I am thinking that the one to enforce the SASL
mechanism name shold be dropped. It becomes useful if willing to test
the code path where an incorrect mechanism name is used, or to enforce
the client to not ask for channel binding. Still I think that we
should keep the code simple and always assume that channel binding is
supported if the SSL implementation allows it to not give users the
option to use something less secure. Perhaps this could be replaced
with a on/off switch to disable channel binding, but I am not much
into the idea either. This also keeps libpq slightly cleaner.

So attached are rebased patches:
- 0001 to introduce the connection parameter saslchannelbinding, which
allows libpq to enforce the type of channel binding used during an
exchange.
- 0002 to add tls-endpoint as channel binding type, which is where 0001 shines.

I have created a different thread with a patch about the bug of v10
servers not able to accept "y,," as valid input for channel binding as
this needs its own discussion:
https://www.postgresql.org/message-id/CAB7nPqSFcNsuQcWcqhX8QSz0R8oKz8ZM4Yw4ky=cfO9rpVdTUA@mail.gmail.com

Peter, also in CheckSCRAMAuth() the server publishes
SCRAM-SHA-256-PLUS is SSL is being used. However, seeing recent
patches to add more SSL implementations (gnuTLS, macos and windows
channel), it seems to me that the choice of publishing
SCRAM-SHA-256-PLUS should be done depending on if there is any binding
data available instead, either TLS finish message data or certificate
hash. On the client-side, it seems also to me that the "y" flag should
not be sent if there is no binding data available because the client
may not support it. All things stand now because OpenSSL is the only
implementation in core and can support both tls-unique and
tls-server-end-point, this may not stand true should any other
implementations get added. For example SSL implementation of MacOS has
no documented API to get a hash of the server or the TLS-finished
message bytes.
-- 
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Nov 21, 2017 at 1:36 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> So attached are rebased patches:
> - 0001 to introduce the connection parameter saslchannelbinding, which
> allows libpq to enforce the type of channel binding used during an
> exchange.
> - 0002 to add tls-endpoint as channel binding type, which is where 0001 shines.

Attached is a rebased patch set, documentation failing to compile. I
am moving at the same time this patch set to the next commit fest.
-- 
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 11/26/17 06:59, Michael Paquier wrote:
> On Tue, Nov 21, 2017 at 1:36 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> So attached are rebased patches:
>> - 0001 to introduce the connection parameter saslchannelbinding, which
>> allows libpq to enforce the type of channel binding used during an
>> exchange.
>> - 0002 to add tls-endpoint as channel binding type, which is where 0001 shines.
> 
> Attached is a rebased patch set, documentation failing to compile. I
> am moving at the same time this patch set to the next commit fest.

I think these are SCRAM channel bindings, not SASL channel bindings, so
the parameter should be named accordingly.

I also wonder whether there should be a mechanism to turn off channel
binding from the client.  Right now, there is no way to test the
non-PLUS mechanism in an SSL build.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Robert Haas
Date:
On Tue, Nov 28, 2017 at 11:10 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I also wonder whether there should be a mechanism to turn off channel
> binding from the client.  Right now, there is no way to test the
> non-PLUS mechanism in an SSL build.

I think that would be a good thing to have.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Nov 29, 2017 at 2:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Nov 28, 2017 at 11:10 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> I also wonder whether there should be a mechanism to turn off channel
>> binding from the client.  Right now, there is no way to test the
>> non-PLUS mechanism in an SSL build.
>
> I think that would be a good thing to have.

Sure. How do we shape that though? I would think about an extra option
for a scram-sha-256 entry with channel-binding=on|off|choice, choice
being what is currently on HEAD with letting the client decide to use
it or not.
-- 
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Nov 29, 2017 at 7:08 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 29, 2017 at 2:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Nov 28, 2017 at 11:10 AM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> I also wonder whether there should be a mechanism to turn off channel
>>> binding from the client.  Right now, there is no way to test the
>>> non-PLUS mechanism in an SSL build.
>>
>> I think that would be a good thing to have.
>
> Sure. How do we shape that though? I would think about an extra option
> for a scram-sha-256 entry with channel-binding=on|off|choice, choice
> being what is currently on HEAD with letting the client decide to use
> it or not.

Sorry, mind-slipping of the morning. Having an option from the server
would help in restricting access, so there could be some use for it
but not for testing coverage. Still how do we want to shape that for
the client? I can think of two possibilities:
1) Have a special value in the parameter saslchannelbinding proposed
in patch 0001. For example by specifying "none" then no channel
binding is used.
2) Use a dedicated parameter which is a on-off switch.
Any thoughts?
-- 
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 11/28/17 17:33, Michael Paquier wrote:
> 1) Have a special value in the parameter saslchannelbinding proposed
> in patch 0001. For example by specifying "none" then no channel
> binding is used.

I was thinking if it's empty then don't use channel binding.  Right now,
empty means the same thing as tls-unique.  In any case, some variant of
that should be fine.  I don't think we need a separate server option
that this point.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 11/30/17 21:11, Michael Paquier wrote:
> OK, here is a reworked version with the following changes:
> - renamed saslchannelbinding to scramchannelbinding, with a default
> set to tls-unique.
> - An empty value of scramchannelbinding allows client to not use
> channel binding, or in short use use SCRAM-SHA-256 and cbind-flag set
> to 'n'.
> 
> While reviewing the code, I have found something a bit disturbing with
> the header definitions: the libpq frontend code includes scram.h,
> which references backend-side routines. So I think that the definition
> of the SCRAM mechanisms as well as the channel binding types should be
> moved to scram-common.h. This cleanup is included in 0001.

I have committed 0001 and 0002 (renaming to scram_channel_binding).

The 0003 patch looks mostly fine as well.  The only concern I have is
that the way it is set up now, we make the server compute the channel
binding data for both tls-unique and tls-server-end-point, even though
we only end up using one.  This might need some restructuring so that we
only get the data we need once we have learned which channel binding
type the client requested.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Dec 20, 2017 at 1:19 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I have committed 0001 and 0002 (renaming to scram_channel_binding).

Thanks!

> The 0003 patch looks mostly fine as well.  The only concern I have is
> that the way it is set up now, we make the server compute the channel
> binding data for both tls-unique and tls-server-end-point, even though
> we only end up using one.  This might need some restructuring so that we
> only get the data we need once we have learned which channel binding
> type the client requested.

The current patch is focused on simplicity and it has the advantage
that there is no need to depend on any SSL structures or Port* in
fe-auth-scram.c and auth-scram.c. So I would really like to keep the
code simple with this goal in mind.

Speaking of which, making the server-side code is going to be in my
opinion grotty, because the server only knows about the channel
binding type used by the client after reading its first message, so we
would need to call directly the SSL-related APIs in auth-scram.c, and
update scram_state with the appropriate data in the middle of the
exchange. This causes the addition of two dependencies to Port* and
the SSL APIs into auth-scram.c.

However, it is possible to simply optimize the frontend code as in
pg_SASL_init() we already know the channel binding type selected when
calling pgtls_get_finished() and pgtls_get_peer_certificate_hash(). So
while I agree with your point, my opinion is to keep the code as
simple as possible, and then just optimize the frontend code. What do
you think?
-- 
Michael


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Dec 20, 2017 at 09:35:55AM +0900, Michael Paquier wrote:
> However, it is possible to simply optimize the frontend code as in
> pg_SASL_init() we already know the channel binding type selected when
> calling pgtls_get_finished() and pgtls_get_peer_certificate_hash(). So
> while I agree with your point, my opinion is to keep the code as
> simple as possible, and then just optimize the frontend code. What do
> you think?

I have looked at how things could be done in symmetry for both the frontend
and backend code, and I have produced the attached patch 0002, which
can be applied on top of 0001 implementing tls-server-end-point. This
simplifies the interfaces to initialize the SCRAM status data by saving
into scram_state and fe_scram_state respectively Port* and PGconn* which
holds most of the data needed for the exchange. With this patch, cbind_data
is generated only if a specific channel binding type is used with the
appropriate data. So if no channel binding is used there is no additional
SSL call done to get the TLS finished data or the server certificate hash.

0001 has no real changes compared to the last versions.

Peter, thoughts?
--
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Dec 22, 2017 at 11:59 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> I have looked at how things could be done in symmetry for both the frontend
> and backend code, and I have produced the attached patch 0002, which
> can be applied on top of 0001 implementing tls-server-end-point. This
> simplifies the interfaces to initialize the SCRAM status data by saving
> into scram_state and fe_scram_state respectively Port* and PGconn* which
> holds most of the data needed for the exchange. With this patch, cbind_data
> is generated only if a specific channel binding type is used with the
> appropriate data. So if no channel binding is used there is no additional
> SSL call done to get the TLS finished data or the server certificate hash.
>
> 0001 has no real changes compared to the last versions.

Second thoughts on 0002 as there is actually no need to move around
errorMessage if the PGconn* pointer is saved in the SCRAM status data
as both are linked. The attached simplifies the logic even more.
-- 
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 12/22/17 03:10, Michael Paquier wrote:
> On Fri, Dec 22, 2017 at 11:59 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> I have looked at how things could be done in symmetry for both the frontend
>> and backend code, and I have produced the attached patch 0002, which
>> can be applied on top of 0001 implementing tls-server-end-point. This
>> simplifies the interfaces to initialize the SCRAM status data by saving
>> into scram_state and fe_scram_state respectively Port* and PGconn* which
>> holds most of the data needed for the exchange. With this patch, cbind_data
>> is generated only if a specific channel binding type is used with the
>> appropriate data. So if no channel binding is used there is no additional
>> SSL call done to get the TLS finished data or the server certificate hash.
>>
>> 0001 has no real changes compared to the last versions.
> 
> Second thoughts on 0002 as there is actually no need to move around
> errorMessage if the PGconn* pointer is saved in the SCRAM status data
> as both are linked. The attached simplifies the logic even more.
> 

That all looks pretty reasonable.

I'm working through patch 0001 now.  I haven't found any documentation
on the function OBJ_find_sigid_algs().  What does it do?  One might
think that the nid returned by X509_get_signature_nid() is already the
algo_nid we want to use, but there appears to be more to this.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Tue, Dec 26, 2017 at 03:28:09PM -0500, Peter Eisentraut wrote:
> On 12/22/17 03:10, Michael Paquier wrote:
> > Second thoughts on 0002 as there is actually no need to move around
> > errorMessage if the PGconn* pointer is saved in the SCRAM status data
> > as both are linked. The attached simplifies the logic even more.
> >
>
> That all looks pretty reasonable.

Thanks for the review. Don't you think that the the refactoring
simplifications should be done first though? This would result in
producing the patch set in reverse order. I'll be fine to produce them
if need be.

> I'm working through patch 0001 now.  I haven't found any documentation
> on the function OBJ_find_sigid_algs().  What does it do?  One might
> think that the nid returned by X509_get_signature_nid() is already the
> algo_nid we want to use, but there appears to be more to this.

All the objects returned by X509_get_signature_nid() are listed in
crypto/objects/obj_dat.h which may include more information than just
the algorithm type, like for example if RSA encryption is used or not,
etc. I found about the low-level OBJ_find_sigid_algs() to actually get
the real hashing algorithm after diving into X509* informations. And by
looking at X509_signature_print() I found out that this returns the
information we are looking for. This has the damn advantage that we rely
on a minimal lists of algorithms and we don't need to worry about any
future options linked with X509_get_signature_nid(), so this simplifies
Postgres code as well as long-term maintenance.
--
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Wed, Dec 27, 2017 at 09:27:40AM +0900, Michael Paquier wrote:
> On Tue, Dec 26, 2017 at 03:28:09PM -0500, Peter Eisentraut wrote:
>> On 12/22/17 03:10, Michael Paquier wrote:
>>> Second thoughts on 0002 as there is actually no need to move around
>>> errorMessage if the PGconn* pointer is saved in the SCRAM status data
>>> as both are linked. The attached simplifies the logic even more.
>>>
>>
>> That all looks pretty reasonable.
>
> Thanks for the review. Don't you think that the the refactoring
> simplifications should be done first though? This would result in
> producing the patch set in reverse order. I'll be fine to produce them
> if need be.

Well, here is a patch set doing the reverse operation: refactoring does
first in 0001 and support for tls-server-end-point is in 0002. Hope this
helps.
--
Michael

Attachment

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 12/28/17 02:19, Michael Paquier wrote:
> On Wed, Dec 27, 2017 at 09:27:40AM +0900, Michael Paquier wrote:
>> On Tue, Dec 26, 2017 at 03:28:09PM -0500, Peter Eisentraut wrote:
>>> On 12/22/17 03:10, Michael Paquier wrote:
>>>> Second thoughts on 0002 as there is actually no need to move around
>>>> errorMessage if the PGconn* pointer is saved in the SCRAM status data
>>>> as both are linked. The attached simplifies the logic even more.
>>>>
>>>
>>> That all looks pretty reasonable.
>>
>> Thanks for the review. Don't you think that the the refactoring
>> simplifications should be done first though? This would result in
>> producing the patch set in reverse order. I'll be fine to produce them
>> if need be.
> 
> Well, here is a patch set doing the reverse operation: refactoring does
> first in 0001 and support for tls-server-end-point is in 0002. Hope this
> helps.

committed

I reorganized the be_tls_get_certificate_hash() and
pgtls_get_peer_certificate_hash() functions a bit to not have most of
the code in a big if statement.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 1/4/18 15:41, Peter Eisentraut wrote:
> On 12/28/17 02:19, Michael Paquier wrote:
>> On Wed, Dec 27, 2017 at 09:27:40AM +0900, Michael Paquier wrote:
>>> On Tue, Dec 26, 2017 at 03:28:09PM -0500, Peter Eisentraut wrote:
>>>> On 12/22/17 03:10, Michael Paquier wrote:
>>>>> Second thoughts on 0002 as there is actually no need to move around
>>>>> errorMessage if the PGconn* pointer is saved in the SCRAM status data
>>>>> as both are linked. The attached simplifies the logic even more.
>>>>>
>>>>
>>>> That all looks pretty reasonable.
>>>
>>> Thanks for the review. Don't you think that the the refactoring
>>> simplifications should be done first though? This would result in
>>> producing the patch set in reverse order. I'll be fine to produce them
>>> if need be.
>>
>> Well, here is a patch set doing the reverse operation: refactoring does
>> first in 0001 and support for tls-server-end-point is in 0002. Hope this
>> helps.
> 
> committed

Some hosts don't seem to have X509_get_signature_nid().  Looking into
that ...

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Some hosts don't seem to have X509_get_signature_nid().  Looking into
> that ...

dromedary is whinging about OBJ_find_sigid_algs, as well.

            regards, tom lane


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Peter Eisentraut
Date:
On 1/4/18 16:17, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> Some hosts don't seem to have X509_get_signature_nid().  Looking into
>> that ...
> 
> dromedary is whinging about OBJ_find_sigid_algs, as well.

Yeah, it seems like we might need to fine-tune this a bit more to make
it work across all OpenSSL versions.  I'm going to let the buildfarm
take a run through the current code and see what other issues arise.

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


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 1/4/18 16:17, Tom Lane wrote:
>> dromedary is whinging about OBJ_find_sigid_algs, as well.

> Yeah, it seems like we might need to fine-tune this a bit more to make
> it work across all OpenSSL versions.  I'm going to let the buildfarm
> take a run through the current code and see what other issues arise.

Well, it looks like the older machines don't like OBJ_find_sigid_algs,
and the newest machines don't like what you're trying to pass to it:

/home/andres/build/buildfarm-calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/libpq/be-secure-openssl.c: In function
‘be_tls_get_certificate_hash’:
/home/andres/build/buildfarm-calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/libpq/be-secure-openssl.c:1268:50:
error:dereferencing pointer to incomplete type ‘X509 {aka struct x509_st}’ 
  if (!OBJ_find_sigid_algs(OBJ_obj2nid(server_cert->sig_alg->algorithm),
                                                  ^~

so this is looking mighty like a crashed and burned patch from here :-(

            regards, tom lane


Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

From
Michael Paquier
Date:
On Fri, Jan 5, 2018 at 7:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> so this is looking mighty like a crashed and burned patch from here :-(

Sorry for arriving late to the party, timezone and such..

The lack of access to the signature algorithm type is being covered by
this commit from upstream which introduced X509_get_signature_nid():
commit: dfcf48f499f19fd17a3aee03151ea301814ea6ec
author: Dr. Stephen Henson <steve@openssl.org>
date: Wed, 13 Jun 2012 13:08:12 +0000
New functions to retrieve certificate signatures and signature OID NID.

So any versions of OpenSSL older than 1.0.1 included would not compile
on that. There is only X509_get_signature_type() before that, but this
returns the signature type, and that's the hashing type we are looking
for here. RFC 5929, which defines the channel binding types, is from
July 2010. I have not checked the OpenSSL threads, but I would bet a
nickel that one of the reasons why X509_get_signature_nid() has been
introduced is to support cases similar to tls-server-end-point where
you want to know what's the hash function used for a certificate.

That's my fault at the end, my apologies. I can reproduce manually the
compilation failure of this code when compiling by myself past
versions of OpenSSL. So I think that 054e8c6c is doing the right move.
Thanks Peter and all others involved.
-- 
Michael