Thread: Negotiating the SCRAM channel binding type
Currently, there is no negotiation of the channel binding type between client and server. The server advertises that it supports channel binding, or not, and the client decides what channel binding to use. If the server doesn't support the binding type that the client chose, authentication will fail. Based on recent discussions, it looks like there's going to be differences in this area [1]. OpenSSL can support both tls-unique and tls-server-end-point. Java only supports tls-server-end-point, while GnuTLS only supports tls-unique. And Mac OS Secure Transports supports neither one. Furthermore, it's not clear how TLS v1.3 affects this. tls-unique might no longer be available in TLS v1.3, but we might get new channel binding types to replace it. So this is about to get really messy, if there is no way to negotiate. (Yes, it's going to be messy even with negotiation.) I think we must fix that before we release v11, because this affects the protocol. There needs to be a way for the server to advertise what channel binding types it supports. I propose that the server list each supported channel binding type as a separate SASL mechanism. For example, where currently the server lists: SCRAM-SHA-256-PLUS SCRAM-SHA-256 as the supported mechanisms, we change that into: SCRAM-SHA-256-PLUS tls-unique SCRAM-SHA-256-PLUS tls-server-end-point SCRAM-SHA-256 Thoughts? Unfortunately this breaks compatibility with current v11beta clients, but IMHO we must fix this. If we want to alleviate that, and save a few bytes of network traffic, we could decide that plain "SCRAM-SHA-256-PLUS" implies tls-unique, so the list would be: SCRAM-SHA-256-PLUS SCRAM-SHA-256-PLUS tls-server-end-point SCRAM-SHA-256 That would be mostly compatible with current libpq clients. [1] https://www.postgresql.org/message-id/flat/20180122072936.GB1772%40paquier.xyz - Heikki
On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote: > Currently, there is no negotiation of the channel binding type between > client and server. The server advertises that it supports channel binding, > or not, and the client decides what channel binding to use. If the server > doesn't support the binding type that the client chose, authentication will > fail. > > Based on recent discussions, it looks like there's going to be differences > in this area [1]. OpenSSL can support both tls-unique and > tls-server-end-point. Java only supports tls-server-end-point, while GnuTLS > only supports tls-unique. And Mac OS Secure Transports supports neither one. > Furthermore, it's not clear how TLS v1.3 affects this. tls-unique might no > longer be available in TLS v1.3, but we might get new channel binding types > to replace it. So this is about to get really messy, if there is no way to > negotiate. (Yes, it's going to be messy even with negotiation.) > > I think we must fix that before we release v11, because this affects the > protocol. There needs to be a way for the server to advertise what channel > binding types it supports. Yes, this does make sense. From a security perspective, it doesn't matter which channel binding method we use, assuming there are no unfixed exploits. The difference between the channel binding methods is explained in our PG 11 docs: https://www.postgresql.org/docs/11/static/sasl-authentication.html#SASL-SCRAM-SHA-256 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote: > as the supported mechanisms, we change that into: > > SCRAM-SHA-256-PLUS tls-unique > SCRAM-SHA-256-PLUS tls-server-end-point > SCRAM-SHA-256 Can we say for sure that there won't be other options associated to a given SASL mechanism? It seems to me that something like the following is better long-term, with channel binding available as a comma-separated list of items: SCRAM-SHA-256-PLUS channel_bindings=tls-unique,tls-server-end-point SCRAM-SHA-256 > Thoughts? Unfortunately this breaks compatibility with current v11beta > clients, but IMHO we must fix this. If we want to alleviate that, and save a > few bytes of network traffic, we could decide that plain > "SCRAM-SHA-256-PLUS" implies tls-unique, so the list would be: > > SCRAM-SHA-256-PLUS > SCRAM-SHA-256-PLUS tls-server-end-point > SCRAM-SHA-256 > > That would be mostly compatible with current libpq clients. I am not sure it is worth caring about the compatibility at a beta2 stage, as v11 is not released yet. Still I agree that not specifying anything should mean the default, which is per the RFCs currently existing tls-unique. Another piece of the puzzle is here as well: https://www.postgresql.org/message-id/flat/20180122072936.GB1772@paquier.xyz We need to allow a given SSL implementation to specify what the list of channel bindings it supports is. -- Michael
Attachment
On 11/07/18 14:37, Michael Paquier wrote: > On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote: >> as the supported mechanisms, we change that into: >> >> SCRAM-SHA-256-PLUS tls-unique >> SCRAM-SHA-256-PLUS tls-server-end-point >> SCRAM-SHA-256 > > Can we say for sure that there won't be other options associated to a > given SASL mechanism? It seems to me that something like the following > is better long-term, with channel binding available as a comma-separated > list of items: > > SCRAM-SHA-256-PLUS channel_bindings=tls-unique,tls-server-end-point > SCRAM-SHA-256 That would be more complicated to parse. Yeah, we might need further options for some SASL mechanisms in the future, but we can cross that bridge when we get there. I don't see any need to complicate this case for that. I started digging into this more closely, and ran into a little problem. If channel binding is not used, the client sends a flag to the server to indicate if it's because the client doesn't support channel binding, or because it thought that the server doesn't support it. This is the gs2-cbind-flag. How should the flag be set, if the server supports channel binding type A, while client supports only type B? The purpose of the flag is to detect downgrade attacks, where a man-in-the-middle removes the PLUS variants from the list of supported mechanisms. If we treat incompatible channel binding types as "client doesn't support channel binding", then the downgrade attack becomes possible (the attacker can replace the legit PLUS variants with bogus channel binding types that the client surely doesn't support). If we treat it as "server doesn't support channel binding", then we won't downgrade to the non-channel-binding variant, in the legitimate case that the client and server both support channel binding, but with incompatible types. What we'd really want, is to include the full list of server's supported mechanisms as part of the exchange, not just a boolean "y/n" flag. But that's not what the spec says :-(. I guess we should err on the side of caution, and fail the authentication in that case. That's unfortunate, but it's better than not negotiating at all. At least with the negotiation, the authentication will work if there is any mutually supported channel binding type. - Heikki
On 11/07/18 12:27, Heikki Linnakangas wrote: > Based on recent discussions, it looks like there's going to be > differences in this area [1]. OpenSSL can support both tls-unique and > tls-server-end-point. Java only supports tls-server-end-point, while > GnuTLS only supports tls-unique. And Mac OS Secure Transports supports > neither one. Furthermore, it's not clear how TLS v1.3 affects this. > tls-unique might no longer be available in TLS v1.3, but we might get > new channel binding types to replace it. So this is about to get really > messy, if there is no way to negotiate. (Yes, it's going to be messy > even with negotiation.) I've been reading up on the discussions on GnuTLS and Secure Transport, as well as the specs for tls-server-end-point. In a nutshell, to get the token for tls-server-end-point, you need to get the peer's certificate from the TLS library, in raw DER format, and calculate a hash over it. The hash algorithm depends on the signatureAlgorithm in the certificate, so you need to parse the certificate to extract that. We don't want to re-implement X509 parsing, so realistically we need the TLS library to have support functions for that. Looking at the GnuTLS docs, I believe it has everything we need. gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets the signatureAlgorithm. The macOS Secure Transport documentation is a bit harder to understand, but I think it has everything we need as well. SSLCopyPeerTrust()+SecTrustGetCertificateAtIndex()+SecCertificateCopyData() functions get you the certificate in DER format. You can get the signature algorithm with SecCertificateCopyValues(), with the right constants. Am I missing something? I think we can support tls-server-end-point with all TLS implementations we might care about. - Heikki
On Wed, Jul 11, 2018 at 04:00:47PM +0300, Heikki Linnakangas wrote: > In a nutshell, to get the token for tls-server-end-point, you need to get > the peer's certificate from the TLS library, in raw DER format, and > calculate a hash over it. The hash algorithm depends on the > signatureAlgorithm in the certificate, so you need to parse the certificate > to extract that. We don't want to re-implement X509 parsing, so > realistically we need the TLS library to have support functions for that. > > Looking at the GnuTLS docs, I believe it has everything we need. > gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used > to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets > the signatureAlgorithm. > > The macOS Secure Transport documentation is a bit harder to understand, but > I think it has everything we need as well. > SSLCopyPeerTrust()+SecTrustGetCertificateAtIndex()+SecCertificateCopyData() > functions get you the certificate in DER format. You can get the signature > algorithm with SecCertificateCopyValues(), with the right constants. > > Am I missing something? I think we can support tls-server-end-point with all > TLS implementations we might care about. That seems right to me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote: > That would be more complicated to parse. Yeah, we might need further options > for some SASL mechanisms in the future, but we can cross that bridge when we > get there. I don't see any need to complicate this case for that. Okay, fine for me. > I started digging into this more closely, and ran into a little problem. If > channel binding is not used, the client sends a flag to the server to > indicate if it's because the client doesn't support channel binding, or > because it thought that the server doesn't support it. This is the > gs2-cbind-flag. How should the flag be set, if the server supports channel > binding type A, while client supports only type B? The purpose of the flag > is to detect downgrade attacks, where a man-in-the-middle removes the PLUS > variants from the list of supported mechanisms. If we treat incompatible > channel binding types as "client doesn't support channel binding", then the > downgrade attack becomes possible (the attacker can replace the legit PLUS > variants with bogus channel binding types that the client surely doesn't > support). If we treat it as "server doesn't support channel binding", then > we won't downgrade to the non-channel-binding variant, in the legitimate > case that the client and server both support channel binding, but with > incompatible types. > > What we'd really want, is to include the full list of server's supported > mechanisms as part of the exchange, not just a boolean "y/n" flag. But > that's not what the spec says :-(. Let's not break the spec :) I understand from it that the client is in charge of the choice, so we are rather free to choose the reaction the client should have. In the second phase of the exchange, the client communicates back to the server the channel binding it has decided to choose, it is not up to the server to pick up one if the client thinks that it can use multiple ones. > I guess we should err on the side of caution, and fail the authentication in > that case. That's unfortunate, but it's better than not negotiating at all. > At least with the negotiation, the authentication will work if there is any > mutually supported channel binding type. I think that in this case the client should throw an error unconditionally if it wants to use channel A but server supports only B. It is always possible for the client to adjust the channel binding type it wants to use by using the connection parameter scram_channel_binding, or to recompile. If there is incompatibility between channel binding types, it would actually mean that the client and the server are not compiled with the same SSL implementation, would that actually work? Say a server has only tls-unique with gnu's library and the client uses JDBC which only has tls-server-end-point.. So, to keep things simple, it seems to me that we should just make the server send the list, and then check at client-level if the list sent by server includes what the client wants to use, complaining if the option is not present. -- Michael
Attachment
On 12/07/18 07:14, Michael Paquier wrote: > On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote: >> I started digging into this more closely, and ran into a little problem. If >> channel binding is not used, the client sends a flag to the server to >> indicate if it's because the client doesn't support channel binding, or >> because it thought that the server doesn't support it. This is the >> gs2-cbind-flag. How should the flag be set, if the server supports channel >> binding type A, while client supports only type B? The purpose of the flag >> is to detect downgrade attacks, where a man-in-the-middle removes the PLUS >> variants from the list of supported mechanisms. If we treat incompatible >> channel binding types as "client doesn't support channel binding", then the >> downgrade attack becomes possible (the attacker can replace the legit PLUS >> variants with bogus channel binding types that the client surely doesn't >> support). If we treat it as "server doesn't support channel binding", then >> we won't downgrade to the non-channel-binding variant, in the legitimate >> case that the client and server both support channel binding, but with >> incompatible types. >> >> What we'd really want, is to include the full list of server's supported >> mechanisms as part of the exchange, not just a boolean "y/n" flag. But >> that's not what the spec says :-(. > > Let's not break the spec :) I understand from it that the client is in > charge of the choice, so we are rather free to choose the reaction the > client should have. In the second phase of the exchange, the client > communicates back to the server the channel binding it has decided to > choose, it is not up to the server to pick up one if the client thinks > that it can use multiple ones. The case where the client and the server both support the same channel binding type, we're OK. 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. If the client responds "y", meaning "the client supports channel binding, but it looks like the server doesn't", then the server will reject the authentication, because it did actually support channel binding. Just not the same one that the client did. If the client could reply "y", and also echo back the server's list of supported channel bindings in the same message, that would solve the problem. But the spec doesn't do that. >> I guess we should err on the side of caution, and fail the authentication in >> that case. That's unfortunate, but it's better than not negotiating at all. >> At least with the negotiation, the authentication will work if there is any >> mutually supported channel binding type. > > I think that in this case the client should throw an error > unconditionally if it wants to use channel A but server supports only B. > It is always possible for the client to adjust the channel binding type > it wants to use by using the connection parameter scram_channel_binding, > or to recompile. If there is incompatibility between channel binding > types, it would actually mean that the client and the server are not > compiled with the same SSL implementation, would that actually work? Say > a server has only tls-unique with gnu's library and the client uses JDBC > which only has tls-server-end-point.. We would like to fall back to not using channel binding at all in that scenario, assuming the configuration doesn't require channel binding. But yeah, rejecting the connection seems to be the best we can do, given what the protocol is. > So, to keep things simple, it seems to me that we should just make the > server send the list, and then check at client-level if the list sent by > server includes what the client wants to use, complaining if the option > is not present. Yep. It seems that all implementations can support tls-server-end-point, after all, so I'm not too worried about this anymore. The spec says that it's the default, but I don't actually see any advantage to using it over tls-server-end-point. I think the main reason for tls-unique to exist is that it doesn't require the server to have a TLS certificate, but PostgreSQL requires that anyway. Actually, I wonder if we should just rip out the tls-unique support, after all? We can add it back at a later version, if there is a need for it, but I don't think we will. With security-related code, simpler is better. - Heikki
On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote: > It seems that all implementations can support tls-server-end-point, after > all, so I'm not too worried about this anymore. The spec says that it's the > default, but I don't actually see any advantage to using it over > tls-server-end-point. I think the main reason for tls-unique to exist is > that it doesn't require the server to have a TLS certificate, but PostgreSQL > requires that anyway. Er. My memories about the spec are a bit different: tls-unique must be implemented and is the default. [ ... digging ... ] Here you go: https://tools.ietf.org/html/rfc5802#section-6.1 -- Michael
Attachment
On 12/07/18 12:06, Michael Paquier wrote: > On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote: >> It seems that all implementations can support tls-server-end-point, after >> all, so I'm not too worried about this anymore. The spec says that it's the >> default, but I don't actually see any advantage to using it over >> tls-server-end-point. I think the main reason for tls-unique to exist is >> that it doesn't require the server to have a TLS certificate, but PostgreSQL >> requires that anyway. > > Er. My memories about the spec are a bit different: tls-unique must be > implemented and is the default. > > [ ... digging ... ] > > Here you go: > https://tools.ietf.org/html/rfc5802#section-6.1 Meh. We're not going implement tls-unique, anyway, in some of the upcoming non-OpenSSL TLS implementations that don't support it. Hmm. That is actually in a section called "Default Channel Binding". And the first paragraph says "A default channel binding type agreement process for all SASL application protocols that do not provide their own channel binding type agreement is provided as follows". Given that, it's not entirely clear to me if the requirement to support tls-unique is for all implementations of SCRAM, or only those applications that don't provide their own channel binding type agreement. - Heikki
On Thu, Jul 12, 2018 at 12:34:51PM +0300, Heikki Linnakangas wrote: > Meh. We're not going implement tls-unique, anyway, in some of the upcoming > non-OpenSSL TLS implementations that don't support it. True enough. Only GnuTLS supports it: https://www.gnutls.org/manual/html_node/Channel-Bindings.html > Hmm. That is actually in a section called "Default Channel Binding". And the > first paragraph says "A default channel binding type agreement process for > all SASL application protocols that do not provide their own channel binding > type agreement is provided as follows". Given that, it's not entirely clear > to me if the requirement to support tls-unique is for all implementations of > SCRAM, or only those applications that don't provide their own channel > binding type agreement. I am not sure, but I get that as tls-unique must be the default if available, so if it is technically possible to have it we should have it in priority. If not, then a channel binding type which is supported by both the server and the client can be chosen. -- Michael
Attachment
On Wed, Jul 11, 2018 at 04:00:47PM +0300, Heikki Linnakangas wrote: > Looking at the GnuTLS docs, I believe it has everything we need. > gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used > to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets > the signatureAlgorithm. Looking at the docs, there is gnutls_x509_crt_get_fingerprint() which can provide the certificate hash. So if the signature algorithm is MD5 or SHA-1, it would be simple enough to upgrade it to SHA-256 and calculate the hash. They have way better docs than OpenSSL, which is nice. -- Michael
Attachment
On 12/07/18 16:08, Michael Paquier wrote: > On Thu, Jul 12, 2018 at 12:34:51PM +0300, Heikki Linnakangas wrote: >> Hmm. That is actually in a section called "Default Channel Binding". And the >> first paragraph says "A default channel binding type agreement process for >> all SASL application protocols that do not provide their own channel binding >> type agreement is provided as follows". Given that, it's not entirely clear >> to me if the requirement to support tls-unique is for all implementations of >> SCRAM, or only those applications that don't provide their own channel >> binding type agreement. > > I am not sure, but I get that as tls-unique must be the default if > available, so if it is technically possible to have it we should have it > in priority. If not, then a channel binding type which is supported by > both the server and the client can be chosen. Another interesting piece of legalese is in RFC 5929 Channel Bindings for TLS: > For many applications, there may be two or more potentially > applicable TLS channel binding types. Existing security frameworks > (such as the GSS-API [RFC2743] or the SASL [RFC4422] GS2 framework > [RFC5801]) and security mechanisms generally do not support > negotiation of channel binding types. Therefore, application peers > need to agree a priori as to what channel binding type to use (or > agree to rules for deciding what channel binding type to use). > > The specifics of whether and how to negotiate channel binding types > are beyond the scope of this document. However, it is RECOMMENDED > that application protocols making use of TLS channel bindings, use > 'tls-unique' exclusively, except, perhaps, where server-side proxies > are common in deployments of an application protocol. In the latter > case an application protocol MAY specify that 'tls-server-end-point' > channel bindings must be used when available, with 'tls-unique' being > used when 'tls-server-end-point' channel bindings are not available. > Alternatively, the application may negotiate which channel binding > type to use, or may make the choice of channel binding type > configurable. In any case, I'm quite convinced now that we should just remove tls-unique, and stick to tls-server-end-point. Patch attached, please take a look. - Heikki
Attachment
On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote: > In any case, I'm quite convinced now that we should just remove tls-unique, > and stick to tls-server-end-point. Patch attached, please take a look. You are making good points here. > This removes the scram_channel_binding libpq option altogether, since there > is now only one supported channel binding type. This can also be used to switch off channel binding on the client-side, which is the behavior you could get only by using a version of libpq compiled with v10 in the context of an SSL connection. Do we really want to lose this switch as well? I like feature switches. - "SCRAM authentication with invalid channel binding"); + "Basic SCRAM authentication"); Or "SCRAM authentication with SSL and channel binding?" +PostgreSQL is <literal>tls-server-end-point</literal>. If other channel +binding types are supported in the future, the server will advertise +them as separate SASL mechanisms. I don't think that this is actually true, per my remark of upthread we could also use an option-based approach with each SASL mechanism sent by the server. I would recommend to remove this last sentence. + man-in-the-middle attacks by mixing the signature of the server's + certificate into the transmitted password hash. While a fake server can + retransmit the real server's certificate, it doesn't have access to the + private key matching that certificate, and therefore cannot prove it is + the owner, causing SSL connection failure Nit: insisting on the fact that tls-server-end-point is used in this context. +void +pg_be_scram_get_mechanisms(Port *port, StringInfo buf) +{ + /* + * Advertise the mechanisms in decreasing order of importance. So the + * channel-binding variants go first, if they are supported. Channel + * binding is only supported with SSL, and only if the SSL implementation + * has a function to get the certificate's hash [...] +#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH + if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use) + state->channel_binding_in_use = true; + else +#endif Hm. I think that this should be part of the set of APIs that each SSL implementation has to provide. It is not clear to me yet if using the flavor of SSL in Windows or macos universe will allow end-point to work, and this could make this proposal more complicated. And HAVE_BE_TLS_GET_CERTIFICATE_HASH is something OpenSSL-ish, so I would recommend to remove all SSL-implementation-specific code from auth*.c and fe-auth*.c, keeping those in their own file. One simple way to address this problem would be to make each SSL implementation return a boolean to tell if it supports SCRAM channel binding or not, with Port* of PGconn* in input to be able to filter connections using SSL or not. + if (strcmp(channel_binding_type, "tls-server-end-point") != 0) + ereport(ERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + (errmsg("unsupported SCRAM channel-binding type \"%s\"", + sanitize_str(channel_binding_type))))); Let's give up on sanitize_str. I am not sure that it is a good idea to print in error messages server-side something that the client has sent. - * is advertised in decreasing order of importance. So the - * channel-binding variants go first, if they are supported. Channel - * binding is only supported in SSL builds. + * is advertised in decreasing order of importance. This comment is a duplicate of what is with pg_be_scram_get_mechanisms. +#ifdef HAVE_X509_GET_SIGNATURE_NID char * pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len) { -#ifdef HAVE_X509_GET_SIGNATURE_NID I would have kept the ifdef at its original place, to keep the OpenSSL-specific CFLAGS [be|fe]-secure-openssl.c. And a couple of lines down the call to be_tls_get_certificate_hash in auth-scram.c is only protected by USE_SSL... So compilation would likely break once a new SSL implementation is added, and libpq-be.h gets uglier. + * The username to was provided by the client in the startup message, and is Nit: two spaces between "to" and "was". + errdetail("The client selected SCRAM-SHA-256 without channel binding, but the SCRAM message includes channel binding data,"))); Why a comma at the end? + /* + * Chose channel binding, but the SSL library doesn't support it. + * Shouldn't happen. + */ + termPQExpBuffer(&buf); + return NULL; An error message is misisng here? -- Michael
Attachment
Thanks for the review! On 22/07/18 16:54, Michael Paquier wrote: > On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote: >> This removes the scram_channel_binding libpq option altogether, since there >> is now only one supported channel binding type. > > This can also be used to switch off channel binding on the client-side, > which is the behavior you could get only by using a version of libpq > compiled with v10 in the context of an SSL connection. Do we really > want to lose this switch as well? I like feature switches. Well, it'd be useless for users, there is no reason to switch off channel binding if both the client and server support it. It might not add any security you care about, but it won't do any harm either. The non-channel-binding codepath is still exercised with non-SSL connections. > +PostgreSQL is <literal>tls-server-end-point</literal>. If other channel > +binding types are supported in the future, the server will advertise > +them as separate SASL mechanisms. > I don't think that this is actually true, per my remark of upthread we > could also use an option-based approach with each SASL mechanism sent by > the server. I would recommend to remove this last sentence. Ok. That's how I'm envisioning we'd add future binding types, but since we're not sure, I'll leave it out. > + man-in-the-middle attacks by mixing the signature of the server's > + certificate into the transmitted password hash. While a fake server can > + retransmit the real server's certificate, it doesn't have access to the > + private key matching that certificate, and therefore cannot prove it is > + the owner, causing SSL connection failure > Nit: insisting on the fact that tls-server-end-point is used in this > context. Yeah, that's the assumption. Now that we only do tls-server-end-point, I think we can assume that without further explanation. > +void > +pg_be_scram_get_mechanisms(Port *port, StringInfo buf) > +{ > + /* > + * Advertise the mechanisms in decreasing order of importance. So the > + * channel-binding variants go first, if they are supported. Channel > + * binding is only supported with SSL, and only if the SSL implementation > + * has a function to get the certificate's hash > [...] > +#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH > + if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use) > + state->channel_binding_in_use = true; > + else > +#endif > Hm. I think that this should be part of the set of APIs that each SSL > implementation has to provide. It is not clear to me yet if using the > flavor of SSL in Windows or macos universe will allow end-point to work, > and this could make this proposal more complicated. And > HAVE_BE_TLS_GET_CERTIFICATE_HASH is something OpenSSL-ish, so I would > recommend to remove all SSL-implementation-specific code from auth*.c > and fe-auth*.c, keeping those in their own file. One simple way to > address this problem would be to make each SSL implementation return a > boolean to tell if it supports SCRAM channel binding or not, with Port* > of PGconn* in input to be able to filter connections using SSL or not. The idea here is that if the SSL implementation implements the required functions, it #defines HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH (in the client) and/or HAVE_BE_TLS_GET_CERTIFICATE_HASH (in the server). So the code above is not implementation-specific; it doesn't know the details of OpenSSL, it only refers to the compile-time flag which the SSL implementation-specific code defines. The flag is part of the API that the SSL implementation provides, it's just a compile-time flag rather than run-time. I'll try to clarify the comments on this. > + if (strcmp(channel_binding_type, "tls-server-end-point") != 0) > + ereport(ERROR, > + (errcode(ERRCODE_PROTOCOL_VIOLATION), > + (errmsg("unsupported SCRAM channel-binding type > \"%s\"", > + sanitize_str(channel_binding_type))))); > Let's give up on sanitize_str. I am not sure that it is a good idea to > print in error messages server-side something that the client has sent. Well, the point of sanitize_str is to make it safe. You're right that it's not strictly necessary, but I think it would be helpful to print out the channel binding type that the client attempted to use. > And a couple of lines down the call to be_tls_get_certificate_hash in > auth-scram.c is only protected by USE_SSL... So compilation would > likely break once a new SSL implementation is added, and libpq-be.h gets > uglier. Fixed by changing "#ifdef USE_SSL" to "#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH". It's true that there is some risk for libpq-be.h (and libpq-int.h) to become ugly, if we add more SSL implementations, and if those implementations have complicated conditions on whether they can get the certificate hashes. In practice, I think it's going to be OK. All the SSL implementations we've talked about - GnuTLS, macOS, Windows - do support the functionality, so we don't need complicated #ifdefs in the header. But we can revisit this if it gets messy. I did some further testing with this, compiling with and without HAVE_BE_TLS_GET_CERTIFICATE_HASH and HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH, and fixed a few combinations that did not work. And I fixed the other comment typos etc. that you pointed out. I have committed this now, because I think it's important to get this into the next beta version, and I'd like to get a full cycle on the buildfarm before that. But if you have the chance, please have one more look at the committed version, to make sure I didn't mess something up. - Heikki
On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote: > I did some further testing with this, compiling with and without > HAVE_BE_TLS_GET_CERTIFICATE_HASH and HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH, > and fixed a few combinations that did not work. And I fixed the other > comment typos etc. that you pointed out. Two things that I am really unhappy about is first that you completely wiped out the test suite for channel binding. We know that channel binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why didn't you keep the check on supports_tls_server_end_point to determine if the connection should be a failure or a success? Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and the other flags over-complicated, but I won't fight hard on that point if you want to go your way. > I have committed this now, because I think it's important to get this into > the next beta version, and I'd like to get a full cycle on the buildfarm > before that. But if you have the chance, please have one more look at the > committed version, to make sure I didn't mess something up. This I definitely agree with, getting this patch in before beta 3 is the best thing to do now. -- Michael
Attachment
On 05/08/18 15:08, Michael Paquier wrote: > On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote: >> I did some further testing with this, compiling with and without >> HAVE_BE_TLS_GET_CERTIFICATE_HASH and HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH, >> and fixed a few combinations that did not work. And I fixed the other >> comment typos etc. that you pointed out. > > Two things that I am really unhappy about is first that you completely > wiped out the test suite for channel binding. We know that channel > binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why > didn't you keep the check on supports_tls_server_end_point to determine > if the connection should be a failure or a success? That test just tested that the scram_channel_binding libpq option works, but I removed the option. I know you wanted to keep it as a feature flag, but as discussed earlier, I don't think that'd be useful. > Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and > the other flags over-complicated, but I won't fight hard on that point > if you want to go your way. I don't feel too strongly about this either, so if you want to write a patch to refactor that, I'm all ears. Note that I had to do something, so that the server code knows whether to advertise SCRAM-SHA-256-PLUS or not, and likewise that the client knows whether to choose channel binding or not. - Heikki
On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote: > That test just tested that the scram_channel_binding libpq option works, but > I removed the option. I know you wanted to keep it as a feature flag, but as > discussed earlier, I don't think that'd be useful. Sorry for the noise, I missed that there is still the test "Basic SCRAM authentication with SSL" so that would be fine. I would have preferred keeping around the negative test so as we don't break SSL connections when the client enforced cbind_flag to 'n' as that would be useful when adding new SSL implementations as that would avoid manual tests which people will most likely forget, but well... You can remove $supports_tls_server_end_point in 002_scram.pl by the way. Should I remove it or perhaps you would prefer to do it? -- Michael
Attachment
On 05/08/18 15:45, Michael Paquier wrote: > On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote: >> That test just tested that the scram_channel_binding libpq option works, but >> I removed the option. I know you wanted to keep it as a feature flag, but as >> discussed earlier, I don't think that'd be useful. > > Sorry for the noise, I missed that there is still the test "Basic SCRAM > authentication with SSL" so that would be fine. I would have preferred > keeping around the negative test so as we don't break SSL connections > when the client enforced cbind_flag to 'n' as that would be useful when > adding new SSL implementations as that would avoid manual tests which > people will most likely forget, but well... The only negative test there was, was to check for bogus scram_channel_binding option, "scram_channel_binding=not-exists". Yeah, it would be nice to have some, but this commit didn't really change that situation. I'm hoping that we add a libpq option to actually force channel binding soon. That'll make channel binding actually useful to users, but it will also make it easier to write tests to check that channel binding is actually used or not used, in the right situations. > You can remove $supports_tls_server_end_point in 002_scram.pl by the > way. Should I remove it or perhaps you would prefer to do it? Ah, good catch. I'll go and remove it, thanks! - Heikki
On 05/08/18 17:11, I wrote: > The only negative test there was, was to check for bogus > scram_channel_binding option, "scram_channel_binding=not-exists". Yeah, > it would be nice to have some, but this commit didn't really change that > situation. Sorry, I see now that there was indeed a test for scram_channel_binding='', which meant "no channel binding". That was confusing, I assumed '' was the default. > I'm hoping that we add a libpq option to actually force channel binding > soon. That'll make channel binding actually useful to users, but it will > also make it easier to write tests to check that channel binding is > actually used or not used, in the right situations. Nevertheless, this we should do. - Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes: > Sorry, I see now that there was indeed a test for > scram_channel_binding='', which meant "no channel binding". That was > confusing, I assumed '' was the default. Ugh, it isn't? There's a general principle in libpq that setting a parameter to an empty string is the same as leaving it unset. I think violating that pattern is a bad idea. regards, tom lane
On 5 August 2018 19:01:04 EEST, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Heikki Linnakangas <hlinnaka@iki.fi> writes: >> Sorry, I see now that there was indeed a test for >> scram_channel_binding='', which meant "no channel binding". That was >> confusing, I assumed '' was the default. > >Ugh, it isn't? There's a general principle in libpq that setting a >parameter to an empty string is the same as leaving it unset. I think >violating that pattern is a bad idea. Yeah. In any case, the whole option is gone now, so we're good. - Heikki
On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Well, it'd be useless for users, there is no reason to switch off channel > binding if both the client and server support it. It might not add any > security you care about, but it won't do any harm either. The > non-channel-binding codepath is still exercised with non-SSL connections. Is that true? What if it makes a connection fail that you wanted to succeed? Suppose we discover a bug that makes connections using channel binding fail on Thursdays. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote: > On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> Well, it'd be useless for users, there is no reason to switch off channel >> binding if both the client and server support it. It might not add any >> security you care about, but it won't do any harm either. The >> non-channel-binding codepath is still exercised with non-SSL connections. > > Is that true? What if it makes a connection fail that you wanted to > succeed? Suppose we discover a bug that makes connections using > channel binding fail on Thursdays. Well, as things stand today on HEAD, if channel binding has a bug, this makes the SCRAM authentication not able to work over SSL, hence you need to either drop SSL, SCRAM or patch libpq so as the client tells the server that it does not want to use channel binding. None of those are appealing. Before 7729113, the client still had the choice to enforce channel binding to not be used over SSL, which I think is a win to bypass any potential future bugs. On top of that, we can test automatically for *any* future SSL implementations that (SSL + SCRAM + no channel binding) actually works properly, which is, it seems at least to me, a good thing to get more confidence when a new SSL implementation is added. -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote: > > On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > >> Well, it'd be useless for users, there is no reason to switch off channel > >> binding if both the client and server support it. It might not add any > >> security you care about, but it won't do any harm either. The > >> non-channel-binding codepath is still exercised with non-SSL connections. > > > > Is that true? What if it makes a connection fail that you wanted to > > succeed? Suppose we discover a bug that makes connections using > > channel binding fail on Thursdays. > > Well, as things stand today on HEAD, if channel binding has a bug, this > makes the SCRAM authentication not able to work over SSL, hence you need > to either drop SSL, SCRAM or patch libpq so as the client tells the > server that it does not want to use channel binding. None of those are > appealing. Before 7729113, the client still had the choice to enforce > channel binding to not be used over SSL, which I think is a win to > bypass any potential future bugs. On top of that, we can test > automatically for *any* future SSL implementations that (SSL + SCRAM + > no channel binding) actually works properly, which is, it seems at least > to me, a good thing to get more confidence when a new SSL implementation > is added. Uh, really? We can come up with all kinds of potential bugs that might exist in the world but I don't think we should be writing in options for everything that might fail due to some bug existing that we don't know about. Also, we aren't going to release support for a new SSL library in a minor release, so if we end up needing this option due to some SSL library that we really want to support not having channel binding support then we can add the option then (or realize that maybe we shouldn't be bothering with adding support for an SSL implementation that doesn't support channel binding.... it's not exactly a new thing these days and there's very good reasons for having it). Now- if we thought that maybe there was some connection pooling solution that could be made to work with SSL+SCRAM if channel binding is turned off, then that's a use-case that maybe we should try and support, but this notion that we need to be able to turn it off because there might be a bug is hogwash, imv. Now, I haven't seen a pooling solution actually figure out a way to do SSL+SCRAM even without channel binding, and there might not even be a way, so I'm currently a -1 on adding an option to disable it, but if someone turned up tomorrow with an credible approach to doing that, then I'd +1 adding the option. Thanks! Stephen
Attachment
On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote: > On 12/07/18 07:14, Michael Paquier wrote: > >On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote: > >>I started digging into this more closely, and ran into a little problem. If > >>channel binding is not used, the client sends a flag to the server to > >>indicate if it's because the client doesn't support channel binding, or > >>because it thought that the server doesn't support it. This is the > >>gs2-cbind-flag. How should the flag be set, if the server supports channel > >>binding type A, while client supports only type B? The purpose of the flag > >>is to detect downgrade attacks, where a man-in-the-middle removes the PLUS > >>variants from the list of supported mechanisms. If we treat incompatible > >>channel binding types as "client doesn't support channel binding", then the > >>downgrade attack becomes possible (the attacker can replace the legit PLUS > >>variants with bogus channel binding types that the client surely doesn't > >>support). If we treat it as "server doesn't support channel binding", then > >>we won't downgrade to the non-channel-binding variant, in the legitimate > >>case that the client and server both support channel binding, but with > >>incompatible types. > >> > >>What we'd really want, is to include the full list of server's supported > >>mechanisms as part of the exchange, not just a boolean "y/n" flag. But > >>that's not what the spec says :-(. > > > >Let's not break the spec :) I understand from it that the client is in > >charge of the choice, so we are rather free to choose the reaction the > >client should have. In the second phase of the exchange, the client > >communicates back to the server the channel binding it has decided to > >choose, it is not up to the server to pick up one if the client thinks > >that it can use multiple ones. > > The case where the client and the server both support the same channel > binding type, we're OK. 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. If the client responds "y", meaning "the client supports > channel binding, but it looks like the server doesn't", then the server will > reject the authentication, because it did actually support channel binding. > Just not the same one that the client did. If the client could reply "y", > and also echo back the server's list of supported channel bindings in the > same message, that would solve the problem. But the spec doesn't do that. ---------------------------- I know this is an academic question now, but I am not sure this is true. A man-in-the-middle attacker could say they don't support channel binding to the real client and real server and pretend to be the real server. What would work is to hash the secret in with the supported channel binding list. This is how TLS works --- all previous messages are combined with the secret into a transmitted hash to prevent a MITM from changing it. -- 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 07/08/18 22:34, Bruce Momjian wrote: > On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote: >> On 12/07/18 07:14, Michael Paquier wrote: >>> On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote: >>>> I started digging into this more closely, and ran into a little problem. If >>>> channel binding is not used, the client sends a flag to the server to >>>> indicate if it's because the client doesn't support channel binding, or >>>> because it thought that the server doesn't support it. This is the >>>> gs2-cbind-flag. How should the flag be set, if the server supports channel >>>> binding type A, while client supports only type B? The purpose of the flag >>>> is to detect downgrade attacks, where a man-in-the-middle removes the PLUS >>>> variants from the list of supported mechanisms. If we treat incompatible >>>> channel binding types as "client doesn't support channel binding", then the >>>> downgrade attack becomes possible (the attacker can replace the legit PLUS >>>> variants with bogus channel binding types that the client surely doesn't >>>> support). If we treat it as "server doesn't support channel binding", then >>>> we won't downgrade to the non-channel-binding variant, in the legitimate >>>> case that the client and server both support channel binding, but with >>>> incompatible types. >>>> >>>> What we'd really want, is to include the full list of server's supported >>>> mechanisms as part of the exchange, not just a boolean "y/n" flag. But >>>> that's not what the spec says :-(. >>> >>> Let's not break the spec :) I understand from it that the client is in >>> charge of the choice, so we are rather free to choose the reaction the >>> client should have. In the second phase of the exchange, the client >>> communicates back to the server the channel binding it has decided to >>> choose, it is not up to the server to pick up one if the client thinks >>> that it can use multiple ones. >> >> The case where the client and the server both support the same channel >> binding type, we're OK. 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. If the client responds "y", meaning "the client supports >> channel binding, but it looks like the server doesn't", then the server will >> reject the authentication, because it did actually support channel binding. >> Just not the same one that the client did. If the client could reply "y", >> and also echo back the server's list of supported channel bindings in the >> same message, that would solve the problem. But the spec doesn't do that. > ---------------------------- > > I know this is an academic question now, but I am not sure this is true. > A man-in-the-middle attacker could say they don't support channel > binding to the real client and real server and pretend to be the real > server. What would work is to hash the secret in with the supported > channel binding list. This is how TLS works --- all previous messages > are combined with the secret into a transmitted hash to prevent a MITM > from changing it. Yeah, that is what I meant. Currently, when client chooses to not use channel binding, it the sends a single flag, y/n, to indicate whether it thinks the server supports channel binding or not. That flag is included in the hashes used in the authentication, so if a MITM tries to change it, the authentication will fail. If instead of a single flag, it included a list of channel binding types supported by the server, that would solve the problem with supporting multiple channel binding types. - Heikki
On Tue, Aug 7, 2018 at 11:08:12PM +0300, Heikki Linnakangas wrote: > >I know this is an academic question now, but I am not sure this is true. > >A man-in-the-middle attacker could say they don't support channel > >binding to the real client and real server and pretend to be the real > >server. What would work is to hash the secret in with the supported > >channel binding list. This is how TLS works --- all previous messages > >are combined with the secret into a transmitted hash to prevent a MITM > >from changing it. > > Yeah, that is what I meant. Currently, when client chooses to not use > channel binding, it the sends a single flag, y/n, to indicate whether it > thinks the server supports channel binding or not. That flag is included in > the hashes used in the authentication, so if a MITM tries to change it, the > authentication will fail. If instead of a single flag, it included a list of > channel binding types supported by the server, that would solve the problem > with supporting multiple channel binding types. Yes, agreed. -- 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 07/08/18 17:34, Stephen Frost wrote: > * Michael Paquier (michael@paquier.xyz) wrote: >> On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote: >>> On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>>> Well, it'd be useless for users, there is no reason to switch off channel >>>> binding if both the client and server support it. It might not add any >>>> security you care about, but it won't do any harm either. The >>>> non-channel-binding codepath is still exercised with non-SSL connections. >>> >>> Is that true? What if it makes a connection fail that you wanted to >>> succeed? Suppose we discover a bug that makes connections using >>> channel binding fail on Thursdays. >> >> Well, as things stand today on HEAD, if channel binding has a bug, this >> makes the SCRAM authentication not able to work over SSL, hence you need >> to either drop SSL, SCRAM or patch libpq so as the client tells the >> server that it does not want to use channel binding. None of those are >> appealing. Before 7729113, the client still had the choice to enforce >> channel binding to not be used over SSL, which I think is a win to >> bypass any potential future bugs. On top of that, we can test >> automatically for *any* future SSL implementations that (SSL + SCRAM + >> no channel binding) actually works properly, which is, it seems at least >> to me, a good thing to get more confidence when a new SSL implementation >> is added. > > Uh, really? We can come up with all kinds of potential bugs that might > exist in the world but I don't think we should be writing in options for > everything that might fail due to some bug existing that we don't know > about. Yeah, if there's a bug, we'll fix it and move on, like with any other feature. > Now- if we thought that maybe there was some connection pooling solution > that could be made to work with SSL+SCRAM if channel binding is turned > off, then that's a use-case that maybe we should try and support, but > this notion that we need to be able to turn it off because there might > be a bug is hogwash, imv. Now, I haven't seen a pooling solution > actually figure out a way to do SSL+SCRAM even without channel binding, > and there might not even be a way, so I'm currently a -1 on adding an > option to disable it, but if someone turned up tomorrow with an credible > approach to doing that, then I'd +1 adding the option. Now that's a lot more compelling argument for having an option. Essentially, you might have a legitimate proxy or connection pooler that acts like a Man-In-The-Middle. The removed "channel_binding" libpq option wasn't very user-friendly, and wasn't very good for dealing with that scenario anyway; wouldn't you want to disable channel binding in the server rather than the client in that scenario? So I have no regrets in removing it. But going forward, we do need to put some thought in configuring this. We've talked a lot about a libpq option to require channel binding, but we should also have a server-side option to disable it. - Heikki
Greetings, * Heikki Linnakangas (hlinnaka@iki.fi) wrote: > On 07/08/18 17:34, Stephen Frost wrote: > >Now- if we thought that maybe there was some connection pooling solution > >that could be made to work with SSL+SCRAM if channel binding is turned > >off, then that's a use-case that maybe we should try and support, but > >this notion that we need to be able to turn it off because there might > >be a bug is hogwash, imv. Now, I haven't seen a pooling solution > >actually figure out a way to do SSL+SCRAM even without channel binding, > >and there might not even be a way, so I'm currently a -1 on adding an > >option to disable it, but if someone turned up tomorrow with an credible > >approach to doing that, then I'd +1 adding the option. > > Now that's a lot more compelling argument for having an option. Essentially, > you might have a legitimate proxy or connection pooler that acts like a > Man-In-The-Middle. > > The removed "channel_binding" libpq option wasn't very user-friendly, and > wasn't very good for dealing with that scenario anyway; wouldn't you want to > disable channel binding in the server rather than the client in that > scenario? So I have no regrets in removing it. But going forward, we do need > to put some thought in configuring this. We've talked a lot about a libpq > option to require channel binding, but we should also have a server-side > option to disable it. Yeah, I'm pretty sure we'd need it on both sides. If we had it only on one side or the other then you run into the risk of downgrade attacks where the MITM is able to say "I don't support channel binding!" to both sides, even when the actual libpq client and PG server do. Thanks! Stephen
Attachment
On 05/08/2018 14:45, Michael Paquier wrote: > On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote: >> That test just tested that the scram_channel_binding libpq option works, but >> I removed the option. I know you wanted to keep it as a feature flag, but as >> discussed earlier, I don't think that'd be useful. > > Sorry for the noise, I missed that there is still the test "Basic SCRAM > authentication with SSL" so that would be fine. I would have preferred > keeping around the negative test so as we don't break SSL connections > when the client enforced cbind_flag to 'n' as that would be useful when > adding new SSL implementations as that would avoid manual tests which > people will most likely forget, but well... I was updating the gnutls patch for the changed channel binding setup, and I noticed that the 002_scram.pl test now passes even though the gnutls patch currently does not support channel binding. So AFAICT, we're not testing the channel binding functionality there at all. Is that as intended? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Aug 31, 2018 at 12:18:52PM +0200, Peter Eisentraut wrote: > I was updating the gnutls patch for the changed channel binding setup, > and I noticed that the 002_scram.pl test now passes even though the > gnutls patch currently does not support channel binding. So AFAICT, > we're not testing the channel binding functionality there at all. Is > that as intended? As far as I understood that's the intention. One can still test easily channel binding if you implement it so you can make sure that the default SSL connection still works. And you can also make sure that if you don't implement channel binding then an SSL connection still works. But you cannot make sure that if you have channel binding implemented then the disabled path works. I'd still like to think that having a way to enforce the disabled code path over SSL has value, but you know, votes... -- Michael