Thread: [HACKERS] Channel binding support for SCRAM-SHA-256
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
[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
[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
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
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
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
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
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
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
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
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
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
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
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
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
> 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
> 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
> 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
> 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 +
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 +
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
On Thu, Nov 16, 2017 at 10:47 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Pushed 0001, will continue with 0002. Thanks! -- Michael
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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