After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 782218 - Lack of details in TLS failure
Lack of details in TLS failure
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
2.52.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-05-05 12:54 UTC by Bastien Nocera
Modified: 2017-05-23 15:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnutls: Stop using %LATEST_RECORD_VERSION in priority string (2.41 KB, patch)
2017-05-16 01:37 UTC, Michael Catanzaro
committed Details | Review
gnutls: Provide better error message when TLS alert is received (3.12 KB, patch)
2017-05-17 04:24 UTC, Michael Catanzaro
none Details | Review
gnutls: Provide better error message when TLS alert is received (1.91 KB, patch)
2017-05-20 19:33 UTC, Michael Catanzaro
none Details | Review
gnutls: Provide better error message when TLS alert is received (1.67 KB, patch)
2017-05-22 11:59 UTC, Michael Catanzaro
committed Details | Review

Description Bastien Nocera 2017-05-05 12:54:18 UTC
webkitgtk4-2.16.1-2.fc26.x86_64
epiphany-3.24.1-1.fc26.x86_64

I can't view an image that's served by my employer's Oracle expenses instance because of a TLS failure. The same page shows up in Firefox, and there's not enough information in the error message for me to know how to fix it.

"
Unable to display this website
The site at https://<snip>.redhat.com seems to be unavailable.
It may be temporarily inaccessible or moved to a new address. You may wish to verify that your internet connection is working correctly.

Technical information
The precise error was: Peer failed to perform TLS handshake
"

The first section of the error message looks completely incorrect for this type of error, and there's not enough information for me to be able to report the error to the website owner. If an https site fails to load because of this type of error, a better description would be nice, along with the ability to see the certificate (which is probably what's causing problems).
Comment 1 Michael Catanzaro 2017-05-05 13:02:57 UTC
The best we can do is present the error we get from the GTlsConnection. :/

Can you try running 'gnutls-cli-debug' *and* 'gnutls-cli' and post the output here? You can remove the information about the certificates from the gnutls-cli output if you don't want to reveal the domain you're accessing. Since we couldn't establish a TLS session at all, the certificates just don't matter here. The stuff that GnuTLS prints after that could be useful, though.
Comment 2 Bastien Nocera 2017-05-05 13:14:43 UTC
$ gnutls-cli-debug ebsmarkview.corp.redhat.com:4445
GnuTLS debug client 3.5.11
Checking ebsmarkview.corp.redhat.com:4445
                             for SSL 3.0 (RFC6101) support... yes
                        whether we need to disable TLS 1.2... no
                        whether we need to disable TLS 1.1... no
                        whether we need to disable TLS 1.0... no
                        whether %NO_EXTENSIONS is required... no
                               whether %COMPAT is required... no
                             for TLS 1.0 (RFC2246) support... yes
                             for TLS 1.1 (RFC4346) support... no
                                  fallback from TLS 1.1 to... TLS 1.0
                             for TLS 1.2 (RFC5246) support... no
                                  fallback from TLS 1.6 to... failed (server requires fallback dance)
              for inappropriate fallback (RFC7507) support... no
                                     for HTTPS server name... Oracle-Application-Server-11g
                               for certificate chain order... sorted
                  for safe renegotiation (RFC5746) support... yes
                     for Safe renegotiation support (SCSV)... yes
                    for encrypt-then-MAC (RFC7366) support... no
                   for ext master secret (RFC7627) support... no
                           for heartbeat (RFC6520) support... no
                       for version rollback bug in RSA PMS... no
                  for version rollback bug in Client Hello... no
            whether the server ignores the RSA PMS version... yes
whether small records (512 bytes) are tolerated on handshake... yes
    whether cipher suites not in SSL 3.0 spec are accepted... yes
whether a bogus TLS record version in the client hello is accepted... yes
         whether the server understands TLS closure alerts... yes
            whether the server supports session resumption... yes
                      for anonymous authentication support... no
                      for ephemeral Diffie-Hellman support... no
                   for ephemeral EC Diffie-Hellman support... no
                             for curve SECP256r1 (RFC4492)... no
                             for curve SECP384r1 (RFC4492)... no
                             for curve SECP521r1 (RFC4492)... no
           for curve X25519 (draft-ietf-tls-rfc4492bis-07)... no
                  for AES-128-GCM cipher (RFC5288) support... no
                  for AES-128-CCM cipher (RFC6655) support... no
                for AES-128-CCM-8 cipher (RFC6655) support... no
                  for AES-128-CBC cipher (RFC3268) support... no
             for CAMELLIA-128-GCM cipher (RFC6367) support... no
             for CAMELLIA-128-CBC cipher (RFC5932) support... no
                     for 3DES-CBC cipher (RFC2246) support... yes
                  for ARCFOUR 128 cipher (RFC2246) support... yes
            for CHACHA20-POLY1305 cipher (RFC7905) support... no
                                       for MD5 MAC support... yes
                                      for SHA1 MAC support... yes
                                    for SHA256 MAC support... no
                              for ZLIB compression support... no
                     for max record size (RFC6066) support... yes
                for OCSP status response (RFC6066) support... no
              for OpenPGP authentication (RFC6091) support... no

$ gnutls-cli ebsmarkview.corp.redhat.com:4445
Processed 175 CA certificate(s).
Resolving 'ebsmarkview.corp.redhat.com:4445'...
Connecting to '10.5.12.35:4445'...
- Certificate type: X.509
- Got a certificate list of 2 certificates.
- Certificate[0] info:
 - subject `EMAIL=dba@redhat.com,CN=ebsmarkview.corp.redhat.com,OU=Information Technology,O=Red Hat,ST=North Carolina,C=US', issuer `EMAIL=sysadmin-rdu@redhat.com,CN=Red Hat IS CA,OU=IS,O=Red Hat\, Inc.,L=Raleigh,ST=North Carolina,C=US', serial 0x118e, RSA key 2048 bits, signed using RSA-SHA1, activated `2016-06-28 14:40:18 UTC', expires `2018-06-28 14:40:18 UTC', pin-sha256="3s9lOTd5yL6jcNn4OTMQ58a5B7oXpkJhTJTYUxxauPY="
	Public Key ID:
		sha1:004b383f8e7355d8a78dcb9608b0f47431239188
		sha256:decf65393779c8bea370d9f8393310e7c6b907ba17a642614c94d8531c5ab8f6
	Public Key PIN:
		pin-sha256:3s9lOTd5yL6jcNn4OTMQ58a5B7oXpkJhTJTYUxxauPY=
	Public key's random art:
		+--[ RSA 2048]----+
		| . o=o++         |
		|E *.o+ooo .      |
		| . B.... =       |
		|  . = ..o .      |
		|   o + oSo       |
		|  o o . =        |
		|   o   .         |
		|                 |
		|                 |
		+-----------------+

- Certificate[1] info:
 - subject `EMAIL=sysadmin-rdu@redhat.com,CN=Red Hat IS CA,OU=IS,O=Red Hat\, Inc.,L=Raleigh,ST=North Carolina,C=US', issuer `EMAIL=sysadmin-rdu@redhat.com,CN=Red Hat IS CA,OU=IS,O=Red Hat\, Inc.,L=Raleigh,ST=North Carolina,C=US', serial 0x01, RSA key 1024 bits, signed using RSA-SHA1, activated `2009-09-16 18:45:25 UTC', expires `2019-09-14 18:45:25 UTC', pin-sha256="vTBJHn84IDyq35V6r+7tVq3tAtOJAjWIXvTIAoSEEkg="
- Status: The certificate is trusted. 
- Description: (TLS1.0)-(RSA)-(3DES-CBC)-(SHA1)
- Session ID: D4:4B:EF:8B:21:5B:96:7A:91:8C:A9:6F:C7:E3:CA:16
- Version: TLS1.0
- Key Exchange: RSA
- Cipher: 3DES-CBC
- MAC: SHA1
- Compression: NULL
- Options: safe renegotiation,
- Handshake was completed

- Simple Client Mode:
<hangs for minutes>
- Peer has closed the GnuTLS connection
Comment 3 Michael Catanzaro 2017-05-05 13:41:30 UTC
So it only supports insecure ciphersuites, so it'll break soon when we remove 3DES support, and it's TLS version intolerant, so it'll break again sooner or later when we add TLS 1.3 support. Those are both fatal problems. But they're also future problems. I don't know what's going wrong today; as you can see, GnuTLS was able to negotiate a connection, whereas Epiphany/glib-networking was not. On IRC I recommended to you a TLS expert at Red Hat you might ask to look into this further.
Comment 4 Nikos Mavrogiannopoulos 2017-05-09 13:06:39 UTC
Hi,
 Bastien reached me. I run epiphany with GNUTLS_DEBUG_LEVEL=6 exported, and in the output log I see:

gnutls[2]: selected priority string: NONE:+AEAD:+SHA1:+SHA256:+SHA384:+SHA512:+CURVE-SECP256R1:+CURVE-SECP384R1:+CURVE-SECP521R1:+SIGN-ALL:-SIGN-RSA-MD5:+AES-256-GCM:+AES-256-CCM:+CHACHA20-POLY1305:+CAMELLIA-256-GCM:+AES-256-CBC:+CAMELLIA-256-CBC:+AES-128-GCM:+AES-128-CCM:+CAMELLIA-128-GCM:+AES-128-CBC:+CAMELLIA-128-CBC:+3DES-CBC:+ECDHE-RSA:+ECDHE-ECDSA:+RSA:+DHE-RSA:+DHE-DSS:+PSK:+DHE-PSK:+ECDHE-PSK:+VERS-TLS1.2:+VERS-TLS1.1:+VERS-TLS1.0:+VERS-DTLS1.2:+VERS-DTLS1.0:+COMP-NULL:%PROFILE_LOW:%COMPAT:%LATEST_RECORD_VERSION:!VERS-SSL3.0:!ARCFOUR-128
gnutls[2]: selected priority string: NONE:+AEAD:+SHA1:+SHA256:+SHA384:+SHA512:+CURVE-SECP256R1:+CURVE-SECP384R1:+CURVE-SECP521R1:+SIGN-ALL:-SIGN-RSA-MD5:+AES-256-GCM:+AES-256-CCM:+CHACHA20-POLY1305:+CAMELLIA-256-GCM:+AES-256-CBC:+CAMELLIA-256-CBC:+AES-128-GCM:+AES-128-CCM:+CAMELLIA-128-GCM:+AES-128-CBC:+CAMELLIA-128-CBC:+3DES-CBC:+ECDHE-RSA:+ECDHE-ECDSA:+RSA:+DHE-RSA:+DHE-DSS:+PSK:+DHE-PSK:+ECDHE-PSK:+VERS-TLS1.2:+VERS-TLS1.1:+VERS-TLS1.0:+VERS-DTLS1.2:+VERS-DTLS1.0:+COMP-NULL:%PROFILE_LOW:%COMPAT:%LATEST_RECORD_VERSION:!VERS-SSL3.0:!ARCFOUR-128:%UNSAFE_RENEGOTIATION
gnutls[2]: selected priority string: NONE:+AEAD:+SHA1:+SHA256:+SHA384:+SHA512:+CURVE-SECP256R1:+CURVE-SECP384R1:+CURVE-SECP521R1:+SIGN-ALL:-SIGN-RSA-MD5:+AES-256-GCM:+AES-256-CCM:+CHACHA20-POLY1305:+CAMELLIA-256-GCM:+AES-256-CBC:+CAMELLIA-256-CBC:+AES-128-GCM:+AES-128-CCM:+CAMELLIA-128-GCM:+AES-128-CBC:+CAMELLIA-128-CBC:+3DES-CBC:+ECDHE-RSA:+ECDHE-ECDSA:+RSA:+DHE-RSA:+DHE-DSS:+PSK:+DHE-PSK:+ECDHE-PSK:+VERS-TLS1.2:+VERS-TLS1.1:+VERS-TLS1.0:+VERS-DTLS1.2:+VERS-DTLS1.0:+COMP-NULL:%PROFILE_LOW:%COMPAT:!VERS-SSL3.0:!ARCFOUR-128:%COMPAT:!VERS-TLS-ALL:+VERS-TLS1.0
gnutls[2]: selected priority string: NONE:+AEAD:+SHA1:+SHA256:+SHA384:+SHA512:+CURVE-SECP256R1:+CURVE-SECP384R1:+CURVE-SECP521R1:+SIGN-ALL:-SIGN-RSA-MD5:+AES-256-GCM:+AES-256-CCM:+CHACHA20-POLY1305:+CAMELLIA-256-GCM:+AES-256-CBC:+CAMELLIA-256-CBC:+AES-128-GCM:+AES-128-CCM:+CAMELLIA-128-GCM:+AES-128-CBC:+CAMELLIA-128-CBC:+3DES-CBC:+ECDHE-RSA:+ECDHE-ECDSA:+RSA:+DHE-RSA:+DHE-DSS:+PSK:+DHE-PSK:+ECDHE-PSK:+VERS-TLS1.2:+VERS-TLS1.1:+VERS-TLS1.0:+VERS-DTLS1.2:+VERS-DTLS1.0:+COMP-NULL:%PROFILE_LOW:%COMPAT:!VERS-SSL3.0:!ARCFOUR-128:%COMPAT:!VERS-TLS-ALL:+VERS-TLS1.0:%UNSAFE_RENEGOTIATION

The first two fail because of the flag %LATEST_RECORD_VERSION, this server doesn't seem to like it. The last two succeed however with gnutls-cli on command line, though epiphany fails. I believe we should involve the glib-networking guy(s), as it may be some issue in the fallback.
Comment 5 Michael Catanzaro 2017-05-09 14:14:19 UTC
Thanks a bunch for looking at this, Nikos. Now, see g_tls_connection_gnutls_init_priorities in gtlsconnection-gnutls.c. We unconditionally compute four priority strings: normal, normal with insecure renegotiation, insecure fallback, and insecure fallback with insecure renegotiation. But libsoup no longer actually performs any insecure fallback: this functionality is only kept in glib-networking for use by other GLib clients that need it. So the last strings are simply not being used at all.

I guess the server needs to be fixed to work with %LATEST_RECORD_VERSION, because we know that removing %LATEST_RECORD_VERSION will just break other servers, and we do not want to bring back insecure fallback protocol dance. We finally managed to get rid of it in bug #765940 because Firefox and Chrome stopped doing it. But I'm worried that those browsers might bring it back when they implement TLS 1.3 and that breaks all servers with protocol version intolerance, like this one.

So Nikos, I dunno what to do here. I guess our only options are (a) say the server is broken and close it as WONTFIX, or (b) bring back insecure fallback protocol dance. Or maybe we could do (c) try once with %LATEST_RECORD_VERSION and once without it, but without ever downgrading the protocol version.

Lastly, nothing we've discussed so far addresses the issue of lack of details in the error message. Is "Peer failed to perform TLS handshake" the best error message we can get out of GnuTLS, or is that GLib replacing a more-detailed GnuTLS error with its own generic error message?
Comment 6 Michael Catanzaro 2017-05-09 14:16:06 UTC
I wonder how this works in Firefox and Chrome. (Do we know that it works in Chrome?) How do those browsers handle the record version?
Comment 7 Dan Winship 2017-05-09 17:43:06 UTC
(In reply to Michael Catanzaro from comment #5)
> Lastly, nothing we've discussed so far addresses the issue of lack of
> details in the error message. Is "Peer failed to perform TLS handshake" the
> best error message we can get out of GnuTLS, or is that GLib replacing a
> more-detailed GnuTLS error with its own generic error message?

That string comes from GLib; GnuTLS only provides an error code.

The problem here is that... well, basically, the problem is that g_tls_connection_handshake() can't spend several days debugging like we just did :-). All it knows is that it attempted to do a TLS handshake, and the server responded by abruptly closing the connection. Maybe it's because the server is TLS version intolerant. Maybe it ran out of memory. Maybe it's not an HTTPS server and is freaking out at the apparently-random bytes we just spewed at it.

We could probably make the error message a little bit less nerdy ("Server refused to negotiate an encrypted connection"?) but we can't really make it any more precise.
Comment 8 Dan Winship 2017-05-09 17:44:20 UTC
Or I guess "Server unexpectedly closed connection while negotiating encrypted connection"
Comment 9 Nikos Mavrogiannopoulos 2017-05-09 18:34:42 UTC
I'd leave the %LATEST_RECORD_VERSION out of the first connection. Gnutls has the %COMPAT option to improve compatibility and the %LATEST_RECORD_VERSION is intentionally not in that set. It is known to break more servers than the alternative.

Indeed some versions of gnutls may require it after SSL3.0 deprecation, but these are fairly old. You may safely assume that 3.3.12+ as well as 2.12.24 (2.12.23 in rhel6) do not require that work around.
Comment 10 Michael Catanzaro 2017-05-09 18:44:55 UTC
Dan, I remember you added %LATEST_RECORD_VERSION as some servers were broken without it. At the time, I think we reasoned that it was no problem because the fallback priority string would not use %LATEST_RECORD_VERSION. But we don't have the fallback anymore.
Comment 11 Michael Catanzaro 2017-05-09 18:45:24 UTC
(In reply to Dan Winship from comment #8)
> Or I guess "Server unexpectedly closed connection while negotiating
> encrypted connection"

This seems the most descriptive to me.
Comment 12 Nikos Mavrogiannopoulos 2017-05-09 18:55:49 UTC
Note that about the error description, a well behaved server (I guess not that one), will provide an alert with a more descriptive information. You can obtain the alert and associated text in that case which can be more helpful than the error code (which will be warning/error_alert_received)
Comment 13 Dan Winship 2017-05-09 19:12:38 UTC
(In reply to Nikos Mavrogiannopoulos from comment #9)
> I'd leave the %LATEST_RECORD_VERSION out of the first connection. Gnutls has
> the %COMPAT option to improve compatibility and the %LATEST_RECORD_VERSION
> is intentionally not in that set. It is known to break more servers than the
> alternative.
> 
> Indeed some versions of gnutls may require it after SSL3.0 deprecation, but
> these are fairly old. You may safely assume that 3.3.12+ as well as 2.12.24
> (2.12.23 in rhel6) do not require that work around.

[checks code...] Oh, I see. In current gnutls, the default record version is the minimum version that the session would be willing to negotiate, so since we specify !VERS:SSL-3.0, the record version will be TLS 1.0. Which is what Firefox sends as well, so that should probably fix things...
Comment 14 Michael Catanzaro 2017-05-09 19:24:31 UTC
I guess servers that broke when we did not use %LATEST_RECORD_VERSION have probably fixed themselves by now, if that's what Firefox does, so we should probably stop using %LATEST_RECORD_VERSION? Perhaps we should not backport this to GNOME 3.24, though, since there is high regression potential and it only fixes servers that are already broken anyway.
Comment 15 Dan Winship 2017-05-09 21:28:24 UTC
(In reply to Michael Catanzaro from comment #14)
> I guess servers that broke when we did not use %LATEST_RECORD_VERSION have
> probably fixed themselves by now, if that's what Firefox does

It's not that; the problem (assuming I understood Nikos correctly) is that gnutls used to use SSL 3.0 as the record version (even when SSL 3.0 was disabled), and that upset broken servers. With current versions of gnutls (and our priority string), gnutls will use TLS 1.0 as the record version. So it doesn't matter whether the broken servers were fixed or not.
Comment 16 Nikos Mavrogiannopoulos 2017-05-10 07:39:13 UTC
(In reply to Dan Winship from comment #15)
> (In reply to Michael Catanzaro from comment #14)
> > I guess servers that broke when we did not use %LATEST_RECORD_VERSION have
> > probably fixed themselves by now, if that's what Firefox does
> 
> It's not that; the problem (assuming I understood Nikos correctly) is that
> gnutls used to use SSL 3.0 as the record version (even when SSL 3.0 was
> disabled), and that upset broken servers. With current versions of gnutls
> (and our priority string), gnutls will use TLS 1.0 as the record version. So
> it doesn't matter whether the broken servers were fixed or not.

That's correct.
Comment 17 Michael Catanzaro 2017-05-16 01:17:01 UTC
Note that WebKit overrides the glib-networking priority string anyway, so this needs to be fixed in WebKit too. https://bugs.webkit.org/show_bug.cgi?id=172153
Comment 18 Michael Catanzaro 2017-05-16 01:37:22 UTC
Created attachment 351936 [details] [review]
gnutls: Stop using %LATEST_RECORD_VERSION in priority string

This was added after POODLE to deal with broken servers that conflated
TLS record version with protocol version and started blocking clients
that used the SSLv3 record version. Now that SSLv3 is no longer enabled
by WebKit or newer versions of GnuTLS, there is no longer any reason
to keep doing this, and it's breaking interoperability with other broken
servers. Remove it.

This also adds a comment to clarify the confusing duplication of
%COMPAT in the fallback priority string.
Comment 19 Dan Winship 2017-05-16 13:30:24 UTC
Comment on attachment 351936 [details] [review]
gnutls: Stop using %LATEST_RECORD_VERSION in priority string

Leave the bug open for the error string fix though
Comment 20 Michael Catanzaro 2017-05-16 16:53:47 UTC
Comment on attachment 351936 [details] [review]
gnutls: Stop using %LATEST_RECORD_VERSION in priority string

Attachment 351936 [details] pushed as c0aaea9 - gnutls: Stop using %LATEST_RECORD_VERSION in priority string
Comment 21 Michael Catanzaro 2017-05-16 17:27:30 UTC
Also committed http://trac.webkit.org/changeset/216915/webkit
Comment 22 Michael Catanzaro 2017-05-17 03:26:34 UTC
I'm not sure how to add an automatic test for what happens when the server sends a TLS fatal alert, but it can be tested manually by adding the following implementation of g_tls_server_connection_gnutls_finish_handshake() in gtlsserverconnection-gnutls.c, running 'make check', and observing the error that appears in connection.log when the first connection test fails:

gnutls_session_t session;
session = g_tls_connection_gnutls_get_session (gnutls);
gnutls_alert_send (session, GNUTLS_AL_FATAL, GNUTLS_A_EXPORT_RESTRICTION);
Comment 23 Michael Catanzaro 2017-05-17 03:33:24 UTC
(In reply to Michael Catanzaro from comment #11)
> (In reply to Dan Winship from comment #8)
> > Or I guess "Server unexpectedly closed connection while negotiating
> > encrypted connection"
> 
> This seems the most descriptive to me.

Let's stick with "peer" rather than "server" so that we can use the same error message for both client and server connections.
Comment 24 Michael Catanzaro 2017-05-17 04:24:23 UTC
Created attachment 352001 [details] [review]
gnutls: Provide better error message when TLS alert is received

Print a message describing the alert.

This also changes the error code used when a TLS alert is received from
G_TLS_ERROR_NOT_TLS, which is clearly inappropriate as the peer must
support TLS if it is sending a TLS alert, to G_TLS_ERROR_MISC.

Finally, this removes a condition to support GLib older than 2.35.3, since
glib-networking already requires GLib 2.46.0.

Defect: the error message is always printed in English. I'm not sure
why. GnuTLS has gettext support, and checking the definition of
gnutls_alert_get_name in alert.c, it seems like it should be translated
automatically.
Comment 25 Dan Winship 2017-05-17 12:58:09 UTC
Comment on attachment 352001 [details] [review]
gnutls: Provide better error message when TLS alert is received

>Defect: the error message is always printed in English. I'm not sure
>why. GnuTLS has gettext support, and checking the definition of
>gnutls_alert_get_name in alert.c, it seems like it should be translated
>automatically.

Make sure the bindtextdomain() call in _gnutls_global_init() is getting called (with the expected value of PACKAGE and LOCALEDIR) and that the _() in gnutls_alert_get_name() is expanding to a dgettext() call with the same PACKAGE?

Oh, and that you're testing a supported language? It looks like gnutls isn't translated into that many languages (and notably, not Spanish).

>   if (gnutls->priv->handshaking && !gnutls->priv->ever_handshaked)
>     {
>       if (g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_FAILED) ||

>+	  g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_BROKEN_PIPE))
>+        {
>+          if (status == GNUTLS_E_UNEXPECTED_PACKET_LENGTH ||

This drastically changes the logic here: before it was "if my_error is X or Y or status is A, B, or C". Now it's "if my_error is X or Y AND status is A, B, or C".
Comment 26 Michael Catanzaro 2017-05-17 14:59:28 UTC
(In reply to Dan Winship from comment #25) 
> Make sure the bindtextdomain() call in _gnutls_global_init() is getting
> called (with the expected value of PACKAGE and LOCALEDIR) and that the _()
> in gnutls_alert_get_name() is expanding to a dgettext() call with the same
> PACKAGE?

I'll try.

> Oh, and that you're testing a supported language? It looks like gnutls isn't
> translated into that many languages (and notably, not Spanish).

That indeed tripped me up at first, since Spanish is my usual test language... but I wound up testing German instead, and I'm still getting English.

> >   if (gnutls->priv->handshaking && !gnutls->priv->ever_handshaked)
> >     {
> >       if (g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_FAILED) ||
> 
> >+	  g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_BROKEN_PIPE))
> >+        {
> >+          if (status == GNUTLS_E_UNEXPECTED_PACKET_LENGTH ||
> 
> This drastically changes the logic here: before it was "if my_error is X or
> Y or status is A, B, or C". Now it's "if my_error is X or Y AND status is A,
> B, or C".

Wow, oops... code review is good.
Comment 27 Michael Catanzaro 2017-05-20 19:00:41 UTC
I added a call to setlocale() in main() in connection.c and now the GnuTLS component of the error message is translated. So there was never any problem there, my bad. Here's a patch with the error handling logic fixed.
Comment 28 Michael Catanzaro 2017-05-20 19:33:38 UTC
Created attachment 352244 [details] [review]
gnutls: Provide better error message when TLS alert is received

Print a message describing the alert.

This also changes the error code used when a TLS alert is received from
G_TLS_ERROR_NOT_TLS, which is clearly inappropriate as the peer must
support TLS if it is sending a TLS alert, to G_TLS_ERROR_MISC.

Finally, this removes a condition to support GLib older than 2.35.3, since
glib-networking already requires GLib 2.46.0.
Comment 29 Philip Withnall 2017-05-22 07:39:26 UTC
Review of attachment 352244 [details] [review]:

::: tls/gnutls/gtlsconnection-gnutls.c
@@ -783,3 @@
     {
       if (g_error_matches (my_error, G_IO_ERROR, G_IO_ERROR_FAILED) ||
-#if GLIB_CHECK_VERSION (2, 35, 3)

Please drop this in a separate patch, since it’s not at all related to the error message change. Just go ahead and push the version change.
Comment 30 Michael Catanzaro 2017-05-22 11:59:53 UTC
Created attachment 352350 [details] [review]
gnutls: Provide better error message when TLS alert is received

Print a message describing the alert.

This also changes the error code used when a TLS alert is received from
G_TLS_ERROR_NOT_TLS, which is clearly inappropriate as the peer must
support TLS if it is sending a TLS alert, to G_TLS_ERROR_MISC.
Comment 31 Philip Withnall 2017-05-23 10:05:30 UTC
Review of attachment 352350 [details] [review]:

Looks good to me.
Comment 32 Michael Catanzaro 2017-05-23 15:17:14 UTC
Attachment 352350 [details] pushed as 0160a89 - gnutls: Provide better error message when TLS alert is received