Thread: Support tls-exporter as channel binding for TLSv1.3
Hi all, RFC9266, that has been released not so long ago, has added tls-exporter as a new channel binding type: https://www.rfc-editor.org/rfc/rfc5929.html An advantage over tls-server-end-point, AFAIK, is that this prevents man-in-the-middle attacks even if the attacker holds the server's private key, which was the kind of job that tls-unique does for TLSv1.2, though we've decided at the end to drop it during the PG11 dev cycle because it does things poorly. This patch provides an implementation, tests and documentation for the so-said feature. An environment variable called PGCHANNELBINDINGTYPE is added, as well as new connection parameter called channel_binding_type. The key point of the implementation is SSL_export_keying_material(), that is available down to 1.0.1 (oldest version supported on HEAD), so this should not require a ./configure check. Perhaps the part about the new libpq parameter could be refactored as of its own patch, with the addition of channel_binding_type in the SCRAM status structures. Note also that tls-exporter is aimed for TLSv1.3 and newer protocols, but OpenSSL allows the thing to work with older protocols (testable with ssl_max_protocol_version, for example), and I don't see a need to prevent this scenario. An extra thing is that attempting to use tls-exporter with a backend <= 15 and a client >= 16 causes a failure during the SASL exchange, where the backend complains about tls-exporter being unsupported. Jacob Champion should be considered as the primary author of the patch, even if I have spent some time on this patch before sending it here. I am adding that to the next commit fest. Thanks, -- Michael
Attachment
On Sun, Aug 28, 2022 at 11:02 PM Michael Paquier <michael@paquier.xyz> wrote: > RFC9266, that has been released not so long ago, has added > tls-exporter as a new channel binding type: > https://www.rfc-editor.org/rfc/rfc5929.html Hi Michael, thank you for sending this! > Note also that tls-exporter is aimed for > TLSv1.3 and newer protocols, but OpenSSL allows the thing to work with > older protocols (testable with ssl_max_protocol_version, for example), > and I don't see a need to prevent this scenario. For protocols less than 1.3 we'll need to ensure that the extended master secret is in use: This channel binding mechanism is defined only when the TLS handshake results in unique master secrets. This is true of TLS versions prior to 1.3 when the extended master secret extension of [RFC7627] is in use, and it is always true for TLS 1.3 (see Appendix D of [RFC8446]). OpenSSL should have an API for that (SSL_get_extms_support); I don't know when it was introduced. If we want to cross all our T's, we should also disallow tls-exporter if the server was unable to set SSL_OP_NO_RENEGOTIATION. > An extra thing is > that attempting to use tls-exporter with a backend <= 15 and a client > >= 16 causes a failure during the SASL exchange, where the backend > complains about tls-exporter being unsupported. Yep. -- Did you have any thoughts about contributing the Python tests (or porting them to Perl, bleh) so that we could test failure modes as well? Unfortunately those Python tests were also OpenSSL-based, so they're less powerful than an independent implementation... Thanks, --Jacob
On Wed, Aug 31, 2022 at 04:16:29PM -0700, Jacob Champion wrote: > For protocols less than 1.3 we'll need to ensure that the extended > master secret is in use: > > This channel binding mechanism is defined only when the TLS handshake > results in unique master secrets. This is true of TLS versions prior > to 1.3 when the extended master secret extension of [RFC7627] is in > use, and it is always true for TLS 1.3 (see Appendix D of [RFC8446]). > > OpenSSL should have an API for that (SSL_get_extms_support); I don't > know when it was introduced. This is only available from 1.1.0, meaning that we'd better disable tls-exporter when building with something older than that :( With 1.0.2 already not supported by upstream even if a bunch of vendors keep it around for compatibility, I guess that's fine as long as the default setting is tls-server-end-point. It would not be complex to switch to tls-exporter by default when using TLSv1.3 and tls-server-end-point for TLS <= v1.2 though, but that makes the code more complicated and OpenSSL does not complain with tls-exporter when using < 1.3. If we switch the default on the fly, we could drop channel_binding_type and control which one gets used depending on ssl_max/min_server_protocol. I don't like that much, TBH, as this creates more dependencies across our the internal code with the initial choice of the connection parameters, making it more complex to maintain in the long-term. At least that's my impression. > If we want to cross all our T's, we should also disallow tls-exporter > if the server was unable to set SSL_OP_NO_RENEGOTIATION. Hmm. Okay. I have not considered that. But TLSv1.3 has no support for renegotiation, isn't it? And you mean to fail hard when using TLS <= v1.2 with tls-exporter on the backend's SSL_CTX_set_options() call? We cannot do that as the backend's SSL context is initialized before authentication, but we could re-check the state of the SSL options afterwards, during authentication, and force a failure. > Did you have any thoughts about contributing the Python tests (or > porting them to Perl, bleh) so that we could test failure modes as > well? Unfortunately those Python tests were also OpenSSL-based, so > they're less powerful than an independent implementation... No. I have not been able to look at that with the time I had, unfortunately. -- Michael
Attachment
On Wed, Aug 31, 2022 at 5:57 PM Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Aug 31, 2022 at 04:16:29PM -0700, Jacob Champion wrote: > > OpenSSL should have an API for that (SSL_get_extms_support); I don't > > know when it was introduced. > > This is only available from 1.1.0, meaning that we'd better disable > tls-exporter when building with something older than that :( With > 1.0.2 already not supported by upstream even if a bunch of vendors > keep it around for compatibility, I guess that's fine as long as > the default setting is tls-server-end-point. Yeah, that should be fine. Requiring newer OpenSSLs for stronger crypto will probably be uncontroversial. > It would not be complex > to switch to tls-exporter by default when using TLSv1.3 and > tls-server-end-point for TLS <= v1.2 though, but that makes the code > more complicated and OpenSSL does not complain with tls-exporter when > using < 1.3. If we switch the default on the fly, we could drop > channel_binding_type and control which one gets used depending on > ssl_max/min_server_protocol. I don't like that much, TBH, as this > creates more dependencies across our the internal code with the > initial choice of the connection parameters, making it more complex to > maintain in the long-term. At least that's my impression. I think there are two separate concerns there -- whether to remove the configuration option, and whether to change the default. I definitely wouldn't want to remove the option. Users of TLS 1.2 should be able to choose tls-exporter if they want the extra power, and users of TLS 1.3 should be able to remain on tls-server-end-point if they need it in the future. Changing the default is trickier. tls-server-end-point is our default in the wild. We're not RFC-compliant already, because we don't implement tls-unique at all. And there's no negotiation, so it seems like switching the default for TLS 1.3 would impact interoperability between newer clients and older servers. The advantage would be that users of newer clients would have to opt in before servers could forward their credentials around on their behalf. Maybe that's something we could switch safely in the future, once tls-exporter is more widely deployed? > > If we want to cross all our T's, we should also disallow tls-exporter > > if the server was unable to set SSL_OP_NO_RENEGOTIATION. > > Hmm. Okay. I have not considered that. But TLSv1.3 has no support > for renegotiation, isn't it? Right. We only need to worry about that when we're using an older TLS. > And you mean to fail hard when using > TLS <= v1.2 with tls-exporter on the backend's SSL_CTX_set_options() > call? We cannot do that as the backend's SSL context is initialized > before authentication, but we could re-check the state of the SSL > options afterwards, during authentication, and force a failure. Sounds reasonable. > > Did you have any thoughts about contributing the Python tests (or > > porting them to Perl, bleh) so that we could test failure modes as > > well? Unfortunately those Python tests were also OpenSSL-based, so > > they're less powerful than an independent implementation... > > No. I have not been able to look at that with the time I had, > unfortunately. All good. Thanks! --Jacob
On Wed, Sep 7, 2022 at 10:03 AM Jacob Champion <jchampion@timescale.com> wrote: > Yeah, that should be fine. Requiring newer OpenSSLs for stronger > crypto will probably be uncontroversial. While looking into this I noticed that I left the following code in place: > #ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH > if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use) In other words, we're still deciding whether to advertise -PLUS based only on whether we support tls-server-end-point. Maybe all the necessary features landed in OpenSSL in the same version, but I haven't double-checked that, and in any case I think I need to make this code more correct in the next version of this patch. --Jacob
On Mon, Sep 19, 2022 at 09:27:41AM -0700, Jacob Champion wrote: > While looking into this I noticed that I left the following code in place: > >> #ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH >> if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use) > > In other words, we're still deciding whether to advertise -PLUS based > only on whether we support tls-server-end-point. X509_get_signature_nid() has been introduced in 1.0.2. SSL_export_keying_material() is older than that, being present since 1.0.1. Considering the fact that we want to always have tls-server-end-point as default, it seems to me that we should always publish SCRAM-SHA-256-PLUS and support channel binding when building with OpenSSL >= 1.0.2. The same can be said about the areas where we have HAVE_BE_TLS_GET_[PEER_]CERTIFICATE_HASH. There could be a point in supporting tls-exporter as default in 1.0.1, or just allow it if the caller gives it in the connection string, but as that's the next version we are going to drop support for (sooner than later would be better IMO), I don't really want to add more maintenance burden in this area as 1.0.1 is not that used anyway as far as I recall. > Maybe all the necessary features landed in OpenSSL in the same > version, but I haven't double-checked that, and in any case I think > I need to make this code more correct in the next version of this > patch. I was planning to continue working on this patch based on your latest review. Anyway, as that's originally your code, I am fine to let you take the lead here. Just let me know which way you prefer, and I'll stick to it :) -- Michael
Attachment
On Mon, Sep 19, 2022 at 5:54 PM Michael Paquier <michael@paquier.xyz> wrote: > X509_get_signature_nid() has been introduced in 1.0.2. > SSL_export_keying_material() is older than that, being present since > 1.0.1. Considering the fact that we want to always have > tls-server-end-point as default, it seems to me that we should always > publish SCRAM-SHA-256-PLUS and support channel binding when building > with OpenSSL >= 1.0.2. The same can be said about the areas where we > have HAVE_BE_TLS_GET_[PEER_]CERTIFICATE_HASH. Should we advertise support even if the client is too old to provide an extended master secret? > I was planning to continue working on this patch based on your latest > review. Anyway, as that's originally your code, I am fine to let you > take the lead here. Just let me know which way you prefer, and I'll > stick to it :) Well, I'm working on a next version, but it's ballooning in complexity as I try to navigate the fix for OpenSSL 1.0.1 (which is currently failing the tests, unsurprisingly). You mentioned not wanting to add maintenance burden for 1.0.1, and I'm curious to see whether the approach you have in mind would be easier than what mine is turning out to be. Maybe we can compare notes? --Jacob
On Tue, Sep 20, 2022 at 11:01 AM Jacob Champion <jchampion@timescale.com> wrote: > Well, I'm working on a next version, but it's ballooning in complexity > as I try to navigate the fix for OpenSSL 1.0.1 (which is currently > failing the tests, unsurprisingly). To be more specific: I think I'm hitting the case that Heikki pointed out several years ago [1]: > The problematic case is when e.g. the server > only supports tls-unique and the client only supports > tls-server-end-point. What we would (usually) like to happen, is to fall > back to not using channel binding. But it's not clear how to make that > work, and still protect from downgrade attacks. The problem was deferred when tls-unique was removed. We might have to actually solve it now. bcc: Heikki, in case he would like to weigh in. --Jacob [1] https://www.postgresql.org/message-id/ec787074-2305-c6f4-86aa-6902f98485a4%40iki.fi
On Tue, Sep 20, 2022 at 11:51:44AM -0700, Jacob Champion wrote: > On Tue, Sep 20, 2022 at 11:01 AM Jacob Champion <jchampion@timescale.com> wrote: >> Well, I'm working on a next version, but it's ballooning in complexity >> as I try to navigate the fix for OpenSSL 1.0.1 (which is currently >> failing the tests, unsurprisingly). > > To be more specific: I think I'm hitting the case that Heikki pointed > out several years ago [1]: > >> The problematic case is when e.g. the server >> only supports tls-unique and the client only supports >> tls-server-end-point. What we would (usually) like to happen, is to fall >> back to not using channel binding. But it's not clear how to make that >> work, and still protect from downgrade attacks. > > The problem was deferred when tls-unique was removed. We might have to > actually solve it now. Right. One thing that would reduce the complexity of the equation is to drop support for tls-server-end-point in the backend in PG >= 16 as the versions of OpenSSL we still support on HEAD would cover completely tls-exporter. We should have in libpq the code to support both tls-server-end-point and tls-exporter as channel bindings, for backward-compatibility. If we were to drop support for OpenSSL 1.0.1, things get a bit easier here, as we would be sure that channel binding is supported by all the code paths of libpq. Having both channel_binding_type with channel_binding=require would offer some protection against downgrade attacks. That does not feel completely water-proof, still default settings like sslmode=prefer are not really secure either.. -- Michael
Attachment
On Wed, Oct 12, 2022 at 11:01 PM Michael Paquier <michael@paquier.xyz> wrote: > One thing that would reduce the complexity of the equation is > to drop support for tls-server-end-point in the backend in PG >= 16 as > the versions of OpenSSL we still support on HEAD would cover > completely tls-exporter. Is the intent to backport tls-exporter client support? Or is the compatibility break otherwise acceptable? It seemed like there was also some general interest in proxy TLS termination (see also the PROXY effort, though it has stalled a bit). For that, I would think tls-server-end-point is an important feature. --Jacob
On Thu, Oct 13, 2022 at 10:30:37AM -0700, Jacob Champion wrote: > Is the intent to backport tls-exporter client support? Or is the > compatibility break otherwise acceptable? Well, I'd rather say yes thanks to the complexity avoided in the backend as that's the most sensible piece security-wise, even if we always require a certificate to exist in PG. A server attempting to trick a client in downgrading would still be a problem :/ tls-exporter would be a new feature, so backporting is out of the picture. > It seemed like there was also some general interest in proxy TLS > termination (see also the PROXY effort, though it has stalled a bit). > For that, I would think tls-server-end-point is an important feature. Oh, okay. That's an argument in favor of not doing that, then. Perhaps we'd better revisit the introduction of tls-exporter once we know more about all that, and it looks like we would need a way to be able to negotiate which channel binding to use (I recall that the surrounding RFCs allowed some extra negotiation, vaguely, but my impression may be wrong). -- Michael
Attachment
On Fri, Oct 14, 2022 at 11:00:10AM +0900, Michael Paquier wrote: > Oh, okay. That's an argument in favor of not doing that, then. > Perhaps we'd better revisit the introduction of tls-exporter once we > know more about all that, and it looks like we would need a way to be > able to negotiate which channel binding to use (I recall that the > surrounding RFCs allowed some extra negotiation, vaguely, but my > impression may be wrong). I am not sure what can be done for that now, so I have marked the patch as returned with feedback. -- Michael