(Heads up!)

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.

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.

Dan (et. al.), do you think this is a good idea? It was just something that was annoying me....

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().

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.

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.

(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).

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.

OK, agreed.

Let's still deprecate the property, though, and land the documentation improvements.

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.

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 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).

(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.

(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.

Attachment 366471 [details] pushed as 9e5254e - tlsclientconnection: Update use-ssl3 documentation
Attachment 366472 [details] pushed as 045b805 - tlsclientconnection: Deprecate ssl3 property and functions