Interface for Socket Level Access to TLS Version Used

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

29 Branch
mozilla29
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8348796 [details] [diff] [review]
0003-idl-for-getting-the-ssl-handshake-version.patch
Attachment #8348796 - Flags: review?(dkeeler)
(Assignee)

Updated

5 years ago
Attachment #8348796 - Flags: review?(brian)
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
ignore comment 2 - wrong bug/tab. sorry.
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?
Attachment #8348796 - Flags: review?(dkeeler) → review+
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.
Attachment #8348796 - Flags: review?(brian) → review+
(Assignee)

Comment 6

5 years ago
based on review comments I removed the V2 const and the redundant code.
https://hg.mozilla.org/mozilla-central/rev/d48dc641d5c3
Assignee: nobody → mcmanus
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.