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 792217 - Deprecate GTlsClientConnection:use-ssl3
Deprecate GTlsClientConnection:use-ssl3
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
2.55.x
Other Linux
: Normal minor
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2018-01-04 21:56 UTC by Michael Catanzaro
Modified: 2018-01-09 01:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tlsclientconnection: Deprecate and replace use-ssl3 (9.05 KB, patch)
2018-01-04 22:10 UTC, Michael Catanzaro
none Details | Review
gnutls: Implement use-fallback-protocol-version (8.65 KB, patch)
2018-01-04 22:10 UTC, Michael Catanzaro
none Details | Review
tlsclientconnection: Update use-ssl3 documentation (3.54 KB, patch)
2018-01-07 22:17 UTC, Michael Catanzaro
committed Details | Review
tlsclientconnection: Deprecate ssl3 property and functions (3.24 KB, patch)
2018-01-07 22:17 UTC, Michael Catanzaro
committed Details | Review

Description Michael Catanzaro 2018-01-04 21:56:42 UTC
GTlsClientConnection:use-ssl3 has not caused SSL 3.0 to be used for several years now:

"Despite the property name, the fallback version is not necessarily SSL 3.0; if SSL 3.0 has been disabled, the GTlsClientConnection will use the next highest available version (normally TLS 1.0) as the fallback version."

Every use of this property and its associated functions is confusing. Let's deprecate it, create a new property GTlsClientConnection:use-fallback-protocol-version, and bind it to the old property so they function as aliases. Of course, the associated functions should be replaced as well.
Comment 1 Michael Catanzaro 2018-01-04 22:01:23 UTC
(Heads up!)
Comment 2 Michael Catanzaro 2018-01-04 22:10:38 UTC
Created attachment 366332 [details] [review]
tlsclientconnection: Deprecate and replace use-ssl3

GTlsClientConnection:use-ssl3 has not caused SSL 3.0 to be used for
several years now. Every use of this property and its associated
functions is confusing. Let's deprecate it and create a new property
with a better name. The TLS backend (e.g. glib-networking,
glib-openssl) should bind the new property to the old one, so that they
function as aliases.

Of course, the associated functions should be replaced as well. Improve
the documentation of these functions, since they have not matched the
property documentation (or reality) in a long while.
Comment 3 Michael Catanzaro 2018-01-04 22:10:54 UTC
Created attachment 366333 [details] [review]
gnutls: Implement use-fallback-protocol-version

This implements GTlsClientConnection:use-fallback-protocol-version. It
is bound to the old use-ssl3 property using a GBinding so that they
function as aliases.
Comment 4 Michael Catanzaro 2018-01-04 22:12:06 UTC
Dan (et. al.), do you think this is a good idea? It was just something that was annoying me....
Comment 5 Michael Catanzaro 2018-01-04 22:15:40 UTC
Review of attachment 366332 [details] [review]:

::: gio/gtlsclientconnection.h
@@ +71,3 @@
 								    GSocketConnectable      *identity);
+
+GLIB_DEPRECATED_IN_2_56_FOR(g_tls_client_connection_get_use_fallback_protocol_version)

Just noticed I need to fix these deprecation macros before landing. I settled on g_tls_client_connection_get_force_fallback_protocol_version() rather than g_tls_client_connection_get_use_fallback_protocol_version().

@@ +74,2 @@
 gboolean              g_tls_client_connection_get_use_ssl3         (GTlsClientConnection    *conn);
+GLIB_DEPRECATED_IN_2_56_FOR(g_tls_client_connection_get_use_fallback_protocol_version)

And this one is a double fail, should be g_tls_client_connection_set_force_fallback_protocol_version().
Comment 6 Ignacio Casal Quinteiro (nacho) 2018-01-04 22:46:54 UTC
Just a note here. For glib-openssl we are only supporting tls 1.2. I know there might be use cases for a fallback case, but maybe one more interesting property would be to force the highest version....

Otherwise I am ok also with deprecating the property, it was always giving me bad feelings.
Comment 7 Dan Winship 2018-01-05 14:13:09 UTC
We had talked about doing this back when we first changed the semantics of the property (bug 738633 comment 24), but I eventually decided not to bother since AFAIK the only user of this property was libsoup, and it no longer uses it as of bug 765940.

No major HTTP implementation still does TLS fallback (right? It hasn't made a comeback for TLS 1.3 intolerance?), and it's unlikely anyone was using it for any protocol except HTTP, since TLS intolerance was generally a problem with terrible middleware, not with actual servers themselves. And most of the terrible middleware been fixed/replaced at this point anyway, because none of the browsers do fallback any more.

So you can probably just deprecate the property and NOT replace it. Keep it around for ABI compat, but change the docs to say it's a no-op, and remove the fallback code from glib-networking.
Comment 8 Michael Catanzaro 2018-01-05 17:08:04 UTC
(In reply to Dan Winship from comment #7)
> We had talked about doing this back when we first changed the semantics of
> the property (bug 738633 comment 24), but I eventually decided not to bother
> since AFAIK the only user of this property was libsoup, and it no longer
> uses it as of bug 765940.
> 
> No major HTTP implementation still does TLS fallback (right? It hasn't made
> a comeback for TLS 1.3 intolerance?)

Well that's the problem. I think it has indeed returned. See https://bugzilla.mozilla.org/show_bug.cgi?id=1310516.

I don't know what the future holds, but I think there's a high chance that we will need to reintroduce this feature in libsoup as soon as GnuTLS starts doing TLS 1.3. :/ Certainly, if Firefox is still doing insecure fallback, as that bug indicates, we will need to.

However, if major browsers instead decide to drop support for servers that are intolerant of TLS 1.3, then it would indeed be better to not add this new property, and instead make the original property do nothing, like you suggest. So perhaps we should hold off another year or so on this, to see what happens. (Of course, we should eventually deprecate it either way.)

(In reply to Ignacio Casal Quinteiro (nacho) from comment #6)
> Just a note here. For glib-openssl we are only supporting tls 1.2. I know
> there might be use cases for a fallback case, but maybe one more interesting
> property would be to force the highest version....
> 
> Otherwise I am ok also with deprecating the property, it was always giving
> me bad feelings.

As long as you only support TLS 1.2, then it makes sense to ignore this new property, except you would want to use a GBinding to make sure it always matches the value of the old property, same as glib-networking has done. But when you add support for TLS 1.3, you would want to use this property to force a fallback to TLS 1.2 (i.e. advertise TLS 1.2 as the highest-supported protocol version in the handshake).
Comment 9 Dan Winship 2018-01-07 16:39:11 UTC
If WebKitGTK does fallback in the future, you'd probably want something like bug 745637 so you can control exactly how you're falling back, rather than just having a big dumb "fallback" switch that either has unclear semantics, or else falls back way too far.
Comment 10 Michael Catanzaro 2018-01-07 21:52:38 UTC
OK, agreed.

Let's still deprecate the property, though, and land the documentation improvements.
Comment 11 Michael Catanzaro 2018-01-07 22:17:06 UTC
Created attachment 366471 [details] [review]
tlsclientconnection: Update use-ssl3 documentation

The property documentation correctly indicates how this code works
nowadays, but the function documentation is obsolete and misleading.
Update it.
Comment 12 Michael Catanzaro 2018-01-07 22:17:09 UTC
Created attachment 366472 [details] [review]
tlsclientconnection: Deprecate ssl3 property and functions

I originally planned to introduce a new property and functions to
replace these, with the same behavior but less-confusing names. But that
might not be the best approach in the long run. Instead, let's just
deprecate them without replacement.

TLS 1.2 intolerance is no longer a thing in the wild, and no known
GTlsBackend supports TLS 1.3 yet. But you might need to use this
property in the future, even though it's deprecated, if your
GTlsBackend has added support for TLS 1.3 and you need to talk to a
server that is TLS 1.3 intolerant.

Independently of all that, these APIs simply no longer do what their
names suggest, so deprecation is sensible regardless.
Comment 13 Dan Winship 2018-01-08 23:08:49 UTC
Comment on attachment 366472 [details] [review]
tlsclientconnection: Deprecate ssl3 property and functions

>+   * Deprecated: 2.56

Despite insisting that no one is using this and so no one will care, I still feel like you need to document why it's deprecated (and in particular, clarify that this is a "you shouldn't be doing this at all" deprecation, not a "we've replaced it with a better new API" deprecation).
Comment 14 Philip Withnall 2018-01-08 23:30:59 UTC
(In reply to Dan Winship from comment #13)
> Comment on attachment 366472 [details] [review] [review]
> tlsclientconnection: Deprecate ssl3 property and functions
> 
> >+   * Deprecated: 2.56
> 
> Despite insisting that no one is using this and so no one will care, I still
> feel like you need to document why it's deprecated (and in particular,
> clarify that this is a "you shouldn't be doing this at all" deprecation, not
> a "we've replaced it with a better new API" deprecation).

We should deprecate the gtk-doc syntax which supports `Deprecated: $version` without a description.

Hah, it seems I already filed a bug about that 3 years ago: bug #744055.
Comment 15 Michael Catanzaro 2018-01-09 00:46:55 UTC
(In reply to Dan Winship from comment #13)
> Despite insisting that no one is using this and so no one will care, I still
> feel like you need to document why it's deprecated (and in particular,
> clarify that this is a "you shouldn't be doing this at all" deprecation, not
> a "we've replaced it with a better new API" deprecation).

The reason I didn't was that it's not really a "you shouldn't be doing this at all" deprecation, but a "this API is too confusing to leave un-deprecrated" deprecation. If you need to communicate with a TLS-intolerant server, you really do need to use it. Anyway, I'll come up with some blurb for it.
Comment 16 Michael Catanzaro 2018-01-09 01:17:41 UTC
Attachment 366471 [details] pushed as 9e5254e - tlsclientconnection: Update use-ssl3 documentation
Attachment 366472 [details] pushed as 045b805 - tlsclientconnection: Deprecate ssl3 property and functions