add tls 1.3 xpcshell test and add tls version configuration function to nsITLSServerSocket in preparation of enabling tls 1.3

RESOLVED FIXED

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Comment 1

2 years ago
Created attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

Review commit: https://reviewboard.mozilla.org/r/64114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64114/
Attachment #8770739 - Flags: review?(dkeeler)
(Assignee)

Comment 2

2 years ago
I also changed the fallback order to reduce fallback count.
Currently the fallback order is TLS 1.2-DHE -> TLS 1.2+DHE. If TLS 1.3 is enabled, it would be TLS 1.3 -> TLS 1.2+DHE. We enabled only CBC cipher suites for DHE that are not usable in TLS 1.3 anyway.
Assignee: nobody → VYV03354
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

https://reviewboard.mozilla.org/r/64114/#review61372

Looks good in general, but needs a bit of cleanup/clarification.

::: netwerk/base/TLSServerSocket.cpp:249
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +TLSServerSocket::SetVersionRange(uint16_t aMinVersion, uint16_t aMaxVersion)

Maybe we should filter out invalid versions here?

::: netwerk/base/nsITLSServerSocket.idl:74
(Diff revision 1)
>     */
>    void setCipherSuites([array, size_is(aLength)] in unsigned short aCipherSuites,
>                         in unsigned long aLength);
> +
> +  /**
> +   * setVersionRange

A necko peer should sign off on this new API and implementation.

::: netwerk/base/nsITLSServerSocket.idl:78
(Diff revision 1)
> +  /**
> +   * setVersionRange
> +   *
> +   * The server's TLS versions that is used by the TLS handshake.
> +   * This is required to be set before calling |asyncListen|.
> +   */

We should document what values are valid to pass to this function.

::: netwerk/test/unit/test_tls_server.js:44
(Diff revision 1)
>    });
>    return deferred.promise;
>  }
>  
> -function startServer(cert, expectingPeerCert, clientCertificateConfig) {
> +function startServer(cert, expectingPeerCert, clientCertificateConfig,
> +                     version, versionStr) {

Maybe s/version/expectedVersion/

::: netwerk/test/unit/test_tls_server.js:127
(Diff revision 1)
>          let data = NetUtil.readInputStreamToString(input, input.available());
>          equal(data, "HELLO", "Echoed data received");
>          input.close();
>          output.close();
>          inputDeferred.resolve();
> +        ok(!expectingBadCertAlert, "No bad cert alert expected");

I think this should happen before resolving inputDeferred, right?

::: netwerk/test/unit/test_tls_server.js:173
(Diff revision 1)
>  
>  // Replace the UI dialog that prompts the user to pick a client certificate.
>  do_load_manifest("client_cert_chooser.manifest");
>  
> -add_task(function*() {
> -  let cert = yield getCert();
> +const tests = [{
> +  expectingPeerCert: true, clientCertificateConfig: Ci.nsITLSServerSocket.REQUIRE_ALWAYS,

I find this a little hard to read. Maybe have one parameter per line:

{
  expectingPeerCert: true,
  clientCertificateConfig: Ci.nsITLSServerSocket.REQUIRE_ALWAYS,
  sendClientCert: true,
  expectingBadCertAlert: true,
},

::: netwerk/test/unit/test_tls_server.js:193
(Diff revision 1)
> -add_task(function*() {
> -  let cert = yield getCert();
> -  ok(!!cert, "Got self-signed cert");
> -  let port = startServer(cert, true, Ci.nsITLSServerSocket.REQUEST_ALWAYS);
> -  storeCertOverride(port, cert);
> -  yield startClient(port, cert, false);
> +  expectingPeerCert: false, clientCertificateConfig: Ci.nsITLSServerSocket.REQUEST_NEVER,
> +  sendClientCert: false, expectingBadCertAlert: false
> +}];
> +
> +const versions = [{
> +  prefValue: 3, version: Ci.nsITLSClientStatus.TLS_VERSION_1_2, versionStr: "TLS 1.2"

Similar situation here.

::: security/manager/ssl/nsNSSIOLayer.cpp:1107
(Diff revision 1)
>        nsNSSComponent::AreAnyFallbackCiphersEnabled()) {
> +    // There is no point in trying fallback cipher suites with TLS 1.3
> +    // because TLS 1.3 only supports AEAD cipher suites.
>      if (helpers.rememberStrongCiphersFailed(socketInfo->GetHostName(),
> -                                            socketInfo->GetPort(), err)) {
> +                                            socketInfo->GetPort(), err) &&
> +        range.max < SSL_LIBRARY_VERSION_TLS_1_3) {

Let's hoist this up to line 1102 (in particular, we don't want to run rememberStrongCiphersFailed if we're using TLS 1.3, right?

::: security/manager/ssl/tests/unit/test_fallback_cipher.js:209
(Diff revision 1)
> +    Services.prefs.setIntPref("security.tls.version.max", versionPref);
> +
> -  // disable fallback cipher suites
> +    // disable fallback cipher suites
> -  for (let pref of fallback_cipher_prefs) {
> +    for (let pref of fallback_cipher_prefs) {
> -    Services.prefs.setBoolPref(pref, false);
> +      Services.prefs.setBoolPref(pref, false);
> +      Services.prefs.setIntPref("security.tls.version.fallback-limit", 4);

Why is it necessary to set this when the client is using TLS 1.2 and TLS 1.3? I think this would be a more effective test if we left it at the default value (i.e. how it ships).
Attachment #8770739 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 4

2 years ago
https://reviewboard.mozilla.org/r/64114/#review61372

> Maybe we should filter out invalid versions here?

SSL_VersionRangeSet will check the parameter and return the error appropreately. I don't think we need to add a redundant and maybe inconsistent check.

> A necko peer should sign off on this new API and implementation.

:mcmanus, please review the API change.

> Let's hoist this up to line 1102 (in particular, we don't want to run rememberStrongCiphersFailed if we're using TLS 1.3, right?

We need to call rememberStrongCiphersFailed to offer DHE cipher suites in the second (TLS 1.2) handshake.

> Why is it necessary to set this when the client is using TLS 1.2 and TLS 1.3? I think this would be a more effective test if we left it at the default value (i.e. how it ships).

It is necessary to prevent the 1.3-to-1.2 fallback when the client uses TLS 1.3. I slightly modified the logic. Now we will change the pref to a non-default value only when the client uses TLS 1.3.
(Assignee)

Comment 5

2 years ago
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64114/diff/1-2/
Attachment #8770739 - Attachment description: Bug 1286694 - Enable TLS 1.3 in Nightly. → Bug 1286694 - Enable TLS 1.3 in Nightly.
Attachment #8770739 - Flags: review?(mcmanus)
Attachment #8770739 - Flags: review?(dkeeler)
Attachment #8770739 - Flags: review-
Attachment #8770739 - Flags: review?(mcmanus) → review?(dd.mozilla)
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

This needs review from the team that have been working on TLS 1.3.  Adding a n-i to :mt to figure out who should review.  Putting an r- on this just to hold it while we sort things out.
Flags: needinfo?(martin.thomson)
Attachment #8770739 - Flags: review-
https://reviewboard.mozilla.org/r/64114/#review61604

::: security/manager/ssl/nsNSSIOLayer.cpp:1103
(Diff revision 2)
> +    // There is no point in trying fallback cipher suites with TLS 1.3
> +    // because TLS 1.3 only supports AEAD cipher suites.
>      if (helpers.rememberStrongCiphersFailed(socketInfo->GetHostName(),
> -                                            socketInfo->GetPort(), err)) {
> +                                            socketInfo->GetPort(), err) &&
> +        range.max < SSL_LIBRARY_VERSION_TLS_1_3) {

We can still send out a 1.3 ClientHello with bad cipher suites in it.  So this just has the strange effect of disabling cipher suite fallback if 1.3 is enabled.
I think that keeler, dragana/mcpersonus, and (as Richard says) myself/ekr should review this too.

In particular, we need more telemetry before turning on 1.3.  That includes fixes to the fallback telemetry, which is busted for 1.3, as well as performance-related telemetry (some thinking is required there).
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 9

2 years ago
(In reply to Martin Thomson [:mt:] from comment #7)
> https://reviewboard.mozilla.org/r/64114/#review61604
> 
> ::: security/manager/ssl/nsNSSIOLayer.cpp:1103
> (Diff revision 2)
> > +    // There is no point in trying fallback cipher suites with TLS 1.3
> > +    // because TLS 1.3 only supports AEAD cipher suites.
> >      if (helpers.rememberStrongCiphersFailed(socketInfo->GetHostName(),
> > -                                            socketInfo->GetPort(), err)) {
> > +                                            socketInfo->GetPort(), err) &&
> > +        range.max < SSL_LIBRARY_VERSION_TLS_1_3) {
> 
> We can still send out a 1.3 ClientHello with bad cipher suites in it.  So
> this just has the strange effect of disabling cipher suite fallback if 1.3
> is enabled.

We can, but we decided not to send it in the first handshake (bug 1279479). Although retryDueToTLSIntolerance doesn't return here, it will call rememberIntolerantAtVersion and return false eventually. So it will fallback with TLS 1.2 with fallback cipher suites. I modified test_fallback_cipher.js to verify this behavior.
This is exactly the kind of mess that really makes this stuff impossible to reason about (though that could be lack of sleep).  What is the status of our weak cipher suites?  Are we in a position to remove "weak" ciphers entirely?
(Assignee)

Comment 11

2 years ago
duplicate
https://reviewboard.mozilla.org/r/64114/#review61604

> We can still send out a 1.3 ClientHello with bad cipher suites in it.  So this just has the strange effect of disabling cipher suite fallback if 1.3 is enabled.

We can, but we decided not to send it in the first handshake (bug 1279479). Although retryDueToTLSIntolerance doesn't return here, it will call rememberIntolerantAtVersion and return false eventually. So it will fallback with TLS 1.2 with fallback cipher suites. I modified test_fallback_cipher.js to verify this behavior.
(Assignee)

Comment 12

2 years ago
(In reply to Martin Thomson [:mt:] from comment #10)
> This is exactly the kind of mess that really makes this stuff impossible to
> reason about (though that could be lack of sleep).  What is the status of
> our weak cipher suites?  Are we in a position to remove "weak" ciphers
> entirely?

Currently DHE-based cipher suites are hidden from the first handshake. They will be completely removed in the future (bug 1227519).

Other "weak" cipher suites (e.g. static RSA, CBC, and SHA-1) will be removed in the remote future, but no ETA at the moment.
(Assignee)

Updated

2 years ago
Depends on: 1250582
(Assignee)

Comment 13

2 years ago
Created attachment 8771685 [details]
Bug 1286694 - Part 1: Modify test_tls_server.js to test TLS 1.3.

Review commit: https://reviewboard.mozilla.org/r/64754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64754/
Attachment #8770739 - Attachment description: Bug 1286694 - Enable TLS 1.3 in Nightly. → Bug 1286694 - Part 1: Add TLS version configuration function to nsITLSServerSocket.
Attachment #8771685 - Flags: review?(dkeeler)
Attachment #8771686 - Flags: review?(dkeeler)
Attachment #8771687 - Flags: review?(martin.thomson)
Attachment #8770739 - Flags: review?(dkeeler)
Attachment #8770739 - Flags: review-
(Assignee)

Comment 14

2 years ago
Created attachment 8771686 [details]
Bug 1286694 - Part 3: Change the fallback sequence to reduce a redundant fallback when TLS 1.3 is enabled.

Review commit: https://reviewboard.mozilla.org/r/64756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64756/
(Assignee)

Comment 16

2 years ago
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64114/diff/2-3/
(Assignee)

Comment 17

2 years ago
I divided the patch to make review easier.
(Assignee)

Updated

2 years ago
Depends on: 1287300
Comment on attachment 8771687 [details]
Bug 1286694 - Part 4: Enable TLS 1.3 in Nightly

https://reviewboard.mozilla.org/r/64758/#review61788

This is fine, but I can't approve this without having a way to measure the impact of this change.
Attachment #8771687 - Flags: review?(martin.thomson)
Comment on attachment 8771685 [details]
Bug 1286694 - Part 1: Modify test_tls_server.js to test TLS 1.3.

https://reviewboard.mozilla.org/r/64754/#review62292

This looks good.

::: netwerk/test/unit/test_tls_server.js:23
(Diff revision 1)
>                              .getService(Ci.nsICertOverrideService);
>  const socketTransportService =
>    Cc["@mozilla.org/network/socket-transport-service;1"]
>    .getService(Ci.nsISocketTransportService);
>  
> +const prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);

nit: watch out for lines >80 characters

::: netwerk/test/unit/test_tls_server.js:215
(Diff revision 1)
> +
> +add_task(function*() {
> +  for (let v of versions) {
> +    prefs.setIntPref("security.tls.version.max", v.prefValue);
> +    for (let t of tests) {
> +      let cert = yield getCert();

Might as well hoist this outside of the t of tests loop (we don't need to get a new copy each time, right?)
Attachment #8771685 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 20

2 years ago
Comment on attachment 8771685 [details]
Bug 1286694 - Part 1: Modify test_tls_server.js to test TLS 1.3.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64754/diff/1-2/
Attachment #8771685 - Attachment description: Bug 1286694 - Part 2: Modify test_tls_server.js to test TLS 1.3. → Bug 1286694 - Part 1: Modify test_tls_server.js to test TLS 1.3.
Attachment #8770739 - Attachment description: Bug 1286694 - Part 1: Add TLS version configuration function to nsITLSServerSocket. → Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.
Attachment #8771687 - Flags: review?(martin.thomson)
(Assignee)

Comment 21

2 years ago
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64114/diff/3-4/
(Assignee)

Comment 22

2 years ago
Comment on attachment 8771686 [details]
Bug 1286694 - Part 3: Change the fallback sequence to reduce a redundant fallback when TLS 1.3 is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64756/diff/1-2/
(Assignee)

Comment 23

2 years ago
Comment on attachment 8771687 [details]
Bug 1286694 - Part 4: Enable TLS 1.3 in Nightly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64758/diff/1-2/
(Assignee)

Comment 24

2 years ago
Comment on attachment 8771687 [details]
Bug 1286694 - Part 4: Enable TLS 1.3 in Nightly

Forgot to remove the review request from the changeset summary
Attachment #8771687 - Flags: review?(martin.thomson)
Comment on attachment 8771686 [details]
Bug 1286694 - Part 3: Change the fallback sequence to reduce a redundant fallback when TLS 1.3 is enabled.

https://reviewboard.mozilla.org/r/64756/#review62298

Thanks for breaking this up into separate components. r- for now, though - see comments.

::: security/manager/ssl/nsNSSIOLayer.cpp:1105
(Diff revision 1)
>    if ((err == SSL_ERROR_NO_CYPHER_OVERLAP || err == PR_END_OF_FILE_ERROR ||
>         err == PR_CONNECT_RESET_ERROR) &&
>        nsNSSComponent::AreAnyFallbackCiphersEnabled()) {
> +    // There is no point in trying fallback cipher suites with TLS 1.3
> +    // because TLS 1.3 only supports AEAD cipher suites.
>      if (helpers.rememberStrongCiphersFailed(socketInfo->GetHostName(),

Ok - now I get what's going on here. By calling rememberStrongCiphersFailed but not returning true until after we've also called rememberIntolerantAtVersion, this will go from TLS 1.3 without weak ciphers straight to TLS 1.2 with weak ciphers. I think we should instead go from TLS 1.3 without weak ciphers to TLS 1.2 without weak ciphers and then to TLS 1.2 with weak ciphers, because if TLS 1.3 doesn't even support non-AEAD cipher suites, the problem is likely to be TLS 1.3-intolerance, not a cipher suite mismatch.

::: security/manager/ssl/tests/unit/test_fallback_cipher.js:210
(Diff revision 1)
> +
> -  // disable fallback cipher suites
> +    // disable fallback cipher suites
> -  for (let pref of fallback_cipher_prefs) {
> +    for (let pref of fallback_cipher_prefs) {
> -    Services.prefs.setBoolPref(pref, false);
> +      Services.prefs.setBoolPref(pref, false);
> +      if (versionPref >= 4) {
> +        Services.prefs.setIntPref("security.tls.version.fallback-limit", 4);

This shouldn't be in the for loop, right?

::: security/manager/ssl/tests/unit/test_fallback_cipher.js:215
(Diff revision 1)
> +        Services.prefs.setIntPref("security.tls.version.fallback-limit", 4);
> +      }
> -  }
> +    }
>  
> -  yield startClient("server: fallback only, client: fallback disabled, public",
> +    yield startClient("server: fallback only, client: fallback disabled, public",
> -                    port, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));
> +                      port, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));

It occurs to me that this doesn't really test what we think it does, since even in the fallback cases we get the same error once and then it succeeds. We should probably duplicate each of these startClient lines (this can be in a follow-up bug, though).

::: security/manager/ssl/tests/unit/test_fallback_cipher.js:223
(Diff revision 1)
> -                    {isPrivate: true});
> +                      {isPrivate: true});
>  
> -  // enable fallback cipher suites
> +    // enable fallback cipher suites
> -  for (let pref of fallback_cipher_prefs) {
> +    for (let pref of fallback_cipher_prefs) {
> -    Services.prefs.setBoolPref(pref, true);
> +      Services.prefs.setBoolPref(pref, true);
> +      Services.prefs.clearUserPref("security.tls.version.fallback-limit");

Similarly, this should be outside the for loop.
Attachment #8771686 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 26

2 years ago
https://reviewboard.mozilla.org/r/64756/#review62298

> Ok - now I get what's going on here. By calling rememberStrongCiphersFailed but not returning true until after we've also called rememberIntolerantAtVersion, this will go from TLS 1.3 without weak ciphers straight to TLS 1.2 with weak ciphers. I think we should instead go from TLS 1.3 without weak ciphers to TLS 1.2 without weak ciphers and then to TLS 1.2 with weak ciphers, because if TLS 1.3 doesn't even support non-AEAD cipher suites, the problem is likely to be TLS 1.3-intolerance, not a cipher suite mismatch.

Server can choose TLS 1.2 and CBC ciphers even if ClientHello.client_version indicates TLS 1.3. So IMO the "TLS 1.2 with weak ciphers" step is redundant. Although TLS 1.3 intolerant servers will see DHE ciphers, I would not like to add an extra fallback dance only for such an edge case. This omission will not cause connection failures.
(Assignee)

Comment 27

2 years ago
Comment on attachment 8771686 [details]
Bug 1286694 - Part 3: Change the fallback sequence to reduce a redundant fallback when TLS 1.3 is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64756/diff/2-3/
Attachment #8771687 - Attachment description: Bug 1286694 - Part 4: Enable TLS 1.3 in Nightly. → Bug 1286694 - Part 4: Enable TLS 1.3 in Nightly
Attachment #8771686 - Flags: review- → review?(dkeeler)
(Assignee)

Comment 28

2 years ago
Comment on attachment 8771687 [details]
Bug 1286694 - Part 4: Enable TLS 1.3 in Nightly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64758/diff/2-3/
(Assignee)

Comment 29

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a5c29d781b6ba6222f4be26094e4c8ae8d8943
Bug 1286694 - Part 1: Modify test_tls_server.js to test TLS 1.3. r=keeler

Comment 30

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a5c29d781b
Part 1: Modify test_tls_server.js to test TLS 1.3. r=keeler
(Assignee)

Updated

2 years ago
Keywords: leave-open
Attachment #8770739 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

https://reviewboard.mozilla.org/r/64114/#review62550
Comment on attachment 8771686 [details]
Bug 1286694 - Part 3: Change the fallback sequence to reduce a redundant fallback when TLS 1.3 is enabled.

https://reviewboard.mozilla.org/r/64756/#review62972

I had to think about this for a while, and I've come to the conclusion that this is not sufficiently necessary to change how fallback works right now. See below.

::: security/manager/ssl/nsNSSIOLayer.cpp:1058
(Diff revision 3)
>  
>    if ((err == SSL_ERROR_NO_CYPHER_OVERLAP || err == PR_END_OF_FILE_ERROR ||
>         err == PR_CONNECT_RESET_ERROR) &&
>        nsNSSComponent::AreAnyFallbackCiphersEnabled()) {
> +    // There is no point in trying fallback cipher suites with TLS 1.3
> +    // because TLS 1.3 only supports AEAD cipher suites.

Given comment 26, this comment is a bit misleading, since as you said, a server could choose TLS 1.2 and a CBC cipher suite even if the client advertised TLS 1.3. Echoing comment 10, I think this is an unnecessary optimization that makes this (already difficult) code harder to reason about. Let's just leave the cipher suite fallback as it is for now. Maybe after some additional telemetry we could see if this would actually have a significant performance impact.

::: security/manager/ssl/tests/unit/test_fallback_cipher.js:216
(Diff revision 3)
> +      Services.prefs.setIntPref("security.tls.version.fallback-limit", 4);
> +    }
>  
> -  yield startClient("server: fallback only, client: fallback disabled, public",
> +    yield startClient("server: fallback only, client: fallback disabled, public",
> -                    port, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));
> +                      port, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));
> +    // make sure we do not remember the intolerance

I would say "Retry manually to simulate the HTTP layer. This should still fail."
Attachment #8771686 - Flags: review?(dkeeler) → review-

Updated

a year ago
Depends on: 1305561
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1310516
:emk, is there any reason you couldn't land those changes that you have positive reviews on?  They look pretty reasonable to me.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 36

a year ago
Again, probably because of bug 1096768. I'll try to land r+'ed reviews manually.
Flags: needinfo?(VYV03354)

Comment 38

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c326d0e55f74
Part 2: Add TLS version configuration function to nsITLSServerSocket. r=dragana
Updating bug summary to describe the work that was checked in to the tree as a result of it. From what I can tell, there is still some follow-up work (e.g. in fallback, maybe more tests). That can be done in follow-up bugs so as to not complicate this bug further.
status-firefox50: affected → ---
status-firefox52: --- → fixed
Resolution: DUPLICATE → FIXED
Summary: Enable TLS 1.3 in Nightly → add tls 1.3 xpcshell test and add tls version configuration function to nsITLSServerSocket in preparation of enabling tls 1.3
Keywords: leave-open
Attachment #8771686 - Attachment is obsolete: true
Attachment #8771687 - Attachment is obsolete: true
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

Approval Request Comment
[Feature/Bug causing the regression]: required for the test in bug 1323843, which is required for the feature added in bug 1321783 (the "use conservative TLS settings" flag)
[User impact if declined]: there won't be a test for the new feature
[Is this code covered by automated tests?]: well, it basically adds a feature that is only used in tests
[Has the fix been verified in Nightly?]: not verified by QE, no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none (I'm pretty sure this time - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a30548d275189095e8a62cef20ccd09bfbc3124 looks good)
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is borderline test-only, but since it does add a feature to a component that is used in things other than tests (i.e. the devtools), I thought it best to get explicit approval. That said, I think this is a very low risk change since the code being added will only be called from the test in bug 1323843.
[String changes made/needed]: none

(Note to uplifter: only this patch needs uplift, as the other on actually landed in 50)
Attachment #8770739 - Flags: approval-mozilla-beta?
Comment on attachment 8770739 [details]
Bug 1286694 - Part 2: Add TLS version configuration function to nsITLSServerSocket.

This is required by bug 1323843 and we also need bug 1323843 in Beta51 as well because bug 1321783 got uplifted to beta. Beta51+. Should be in 51 beta 14.
Attachment #8770739 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox51: --- → affected
You need to log in before you can comment on or make changes to this bug.