Thread: Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256
[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: > - 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
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
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
> 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 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 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
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
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
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
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 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
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
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 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
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
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
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
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