951199 - Interface for Socket Level Access to TLS Version Used

Status: RESOLVED FIXED

Description

[Patrick McManus [:mcmanus]]

This patch allows necko to determine the TLS version used for a particular connection. In particular, HTTP/2 carries a requirement for connections >= TLS 1.1 and this lets sensible application level error checking occur.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: 0003-idl-for-getting-the-ssl-handshake-version.patch

Comment 2

[Patrick McManus [:mcmanus]]

the linux platforms are all reporting (on try):

8:08:19     INFO -  TEST-INFO | skipping /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_spdy.js | run-if: hasNode
18:08:19     INFO -  TEST-INFO | skipping /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_http2.js | run-if: hasNode

Comment 3

[Patrick McManus [:mcmanus]]

ignore comment 2 - wrong bug/tab. sorry.

Comment 4

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8348796 [details] [diff] [review]
0003-idl-for-getting-the-ssl-handshake-version.patch

Review of attachment 8348796 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM - just a few questions.

::: netwerk/socket/nsISSLSocketControl.idl
@@ +70,5 @@
>       */
>      readonly attribute uint32_t providerFlags;
> +
> +    /* These values are defined by TLS. */
> +    const short SSL_VERSION_2   = 0x0002;

SSLv2 is completely disabled, right? We probably don't need this.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1237,4 @@
>      // 0=ssl3, 1=tls1, 2=tls1.1, 3=tls1.2
>      unsigned int versionEnum = channelInfo.protocolVersion & 0xFF;
>      Telemetry::Accumulate(Telemetry::SSL_HANDSHAKE_VERSION, versionEnum);
> +    infoObject->SetSSLVersionUsed(channelInfo.protocolVersion);

Do we also need to do this in CanFalseStartCallback?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8348796 [details] [diff] [review]
0003-idl-for-getting-the-ssl-handshake-version.patch

Review of attachment 8348796 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/socket/nsISSLSocketControl.idl
@@ +70,5 @@
>       */
>      readonly attribute uint32_t providerFlags;
> +
> +    /* These values are defined by TLS. */
> +    const short SSL_VERSION_2   = 0x0002;

I agree.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1237,4 @@
>      // 0=ssl3, 1=tls1, 2=tls1.1, 3=tls1.2
>      unsigned int versionEnum = channelInfo.protocolVersion & 0xFF;
>      Telemetry::Accumulate(Telemetry::SSL_HANDSHAKE_VERSION, versionEnum);
> +    infoObject->SetSSLVersionUsed(channelInfo.protocolVersion);

This is redundant with the call in PreliminaryHandshakeDone.

Comment 6

[Patrick McManus [:mcmanus]]

based on review comments I removed the V2 const and the redundant code.

Comment 7

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d48dc641d5c3

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d48dc641d5c3