1283604 - Enable building of TLS 1.3 by default

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

As discussed on the call today:

1. Remove the NSS_ENABLE_TLS_1_3 flag.
2. Add a new NSS_DISABLE_TLS_1_3 flag that clamps the max value for versions_defaults_stream and versions_defaults_datagram in sslsock.c to SSL_LIBRARY_VERSION_TLS_1_2.

http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/security/nss/lib/ssl/sslsock.c#93-101

Comment 1

[Martin Thomson [:mt:]]

Attachment: bug1283604-1.patch

This flips the switches.  I have disabled ssl_gtests if you build with the new flag.  I think that Tim needs to look at the changes to the taskcluster config.

In doing this I learned that we already had TLS 1.3 in the code.  The actual change here only really flips the default and makes writing the gtests easier.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8766950 [details] [diff] [review]
bug1283604-1.patch

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

r=me with the config.mk fix. Thanks for taking care of Taskcluster :)

::: coreconf/config.mk
@@ -222,5 @@
> -ifdef NSS_ENABLE_TLS_1_3
> -ifdef NSS_DISABLE_ECC
> -$(error Setting NSS_ENABLE_TLS_1_3 and NSS_DISABLE_ECC isn't a good idea.)
> -endif
> -DEFINES += -DNSS_ENABLE_TLS_1_3

I think we need to keep this and replace it with NSS_DISABLE_TLS_1_3, otherwise sslimpl.h won't see it.

Comment 3

[Martin Thomson [:mt:]]

Actually, the combination of 1.3 and ECC is no longer a requirement; that's vestigal.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

But we at least need:

ifdef NSS_DISABLE_TLS_1_3
DEFINES += -DNSS_DISABLE_TLS_1_3
endif

Although that should probably go into lib/ssl/config.mk?

Comment 5

[Martin Thomson [:mt:]]

Attachment: bug1283604-1.patch

Of course, stupid me.

Comment 6

[Kai Engert (:KaiE:)]

I believe we have non-TLS 1.3 tests, which are contained in the ssl_gtests, only, correct?

RH has started to execute the ssl_gtests on our newest platforms, too.

It would be inconvenient to loose all the ssl_gtests, just because we want to disable TLS 1.3

Instead of removing the the #ifdef's from the external_tests, would it be acceptable to keep them with #ifndef NSS_DISABLE_TLS_1_3 ?

Comment 7

[Kai Engert (:KaiE:)]

Attachment: bug1283604-1-subset-without-external.patch

This is a subset of Martin's patch. It excludes all the changes to the external_tests subdirectory.

r=kaie on that subset

Comment 8

[Kai Engert (:KaiE:)]

Attachment: bug1283604-1-external.patch

This is an alternative patch for the external_tests subdirectory.
It keeps building the ssl_gtests in all scenarios, and uses #ifndef NSS_DISABLE_TLS_1_3 to disable the tests that are specific to TLS 1.3

Is this acceptable for you, or too much baggage?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kai Engert (:kaie) from comment #6)
> Instead of removing the the #ifdef's from the external_tests, would it be
> acceptable to keep them with #ifndef NSS_DISABLE_TLS_1_3 ?

I thought that especially ssl_gtests were littered with NSS_ENABLE_TLS_1_3 checks and it would as important to get rid of those.

Is it possible for RedHat to have a separate ssl_gtests build without NSS_DISABLE_TLS_1_3=1? NSS builds don't take a long time and that seems the easiest way to keep coverage without having to maintain a ton of ifdefs.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> 
> Is it possible for RedHat to have a separate ssl_gtests build without
> NSS_DISABLE_TLS_1_3=1? NSS builds don't take a long time and that seems the
> easiest way to keep coverage without having to maintain a ton of ifdefs.

We currently execute the full NSS test suite, including the ssl_gtests, as part of producing a new build.

Your request would require us to skip the ssl_gtests at build creation, and execute a second layer of tests, decoupled from the primary build environment.

If you want to remove the #ifdef statements, it will probably be simpler for us to add them back to a downstream patch, so we can do it in a single step.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kai Engert (:kaie) from comment #10)
> Your request would require us to skip the ssl_gtests at build creation, and
> execute a second layer of tests, decoupled from the primary build
> environment.

Can't you just run a first pass building with NSS_DISABLE_TLS_1_3 (which btw should disable ssl_gtests in all.sh) and then a second pass without NSS_DISABLE_TLS_1_3 but with NSS_TESTS=ssl_gtests? Maybe there's a simpler way I'm just not seeing.

mt, if we go with removing the define from ssl_gtests we should have all.sh exclude "ssl_gtests" from the default set of tests.

Another possibility could be to make tests a no-op if version_ == TLS 1.3 && NSS_DISABLE_TLS_1_3=1. Not sure if that's really as easy as I imagine it could be.

Comment 12

[Kai Engert (:KaiE:)]

Doing multiple builds with different build flags wouldn't align well with our automated build infrastructure.

We go to great lengths to ensure that each build is executed in a pefectly clean environment, like, bootstrapping a full new OS install, with the minimum amount of required packages.

Doing a first build, with settings that we don't want to ship, has the risk that files are kept, that don't belong to the build that we really want to produce. We could try to have additional scripts that do cleanup, but that has the risk to go wrong, and this kind of risk is what we'd like to avoid in the first place, by bootstrapping a fresh environment for the build.

So, no, I believe the workaround you have suggested doesn't work for us.

I understand that you are trying to make the life as easy for yourselves as possible, but it comes with the consequence that life is more work for downstream consumers.

Having a single, compromise codebase, suitable for everyone reduces the risk of mistakes when adjusting things.

I personally don't understand why the few #ifndef statements in the external tests files would be an issue, but if there's strong desire to remove them for simplicity of development, RH will deal with it.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> Another possibility could be to make tests a no-op if version_ == TLS 1.3 &&
> NSS_DISABLE_TLS_1_3=1. Not sure if that's really as easy as I imagine it
> could be.

Looks like that unfortunately won't work. Tests like EncryptedExtensionsInClearTwoPieces() and others actually do a few things, re-initialize, and then set the version range to [x, TLS 1.3]. I wouldn't know how to detect that before running the test.

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8767146 [details] [diff] [review]
bug1283604-1-external.patch

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

This is what we had before. The point here was to remove that complexity.  Given what has changed in the code, the risk of something breaking when someone disables 1.3 seems low.

Comment 15

[Kai Engert (:KaiE:)]

I don't want to block this bug.

If you think this is what you need, go ahead and check it in, despite my concerns.

Red Hat can use trivial patches to disable TLS 1.3 tests, if they cause trouble for us.

Comment 16

[Martin Thomson [:mt:]]

Attachment: bug1283604-1.patch

Here's a second attempt at this.  I really don't want to create a situation where Redhat have to take extraordinary steps to run at least the basic set of tests.  Hopefully this is a reasonable compromise.

This only disables test files that include non-parameterized tests.  Those are the ones that fail if TLS 1.3 is disabled.

This means that we will have to be careful if we add more parameterized tests, but that's relatively easy to do, as you can see from the changes.  The cost for Redhat is that this will have a fair bit less test coverage when TLS 1.3 is disabled.  The hope is that losing coverage for those lost tests is an acceptable risk.

The number of tests goes from 1245 tests to 309.  Given that we automatically lose 29% just by disabling TLS 1.3, and that most of the gtests were specifically created for TLS 1.3, this isn't as bad as it seems.  The big loss is the extensions tests, see below.

I moved some tests into separate files to make this as simple as possible.  Further splitting of tests into separate files will help improve coverage, and I'd be happy to review patches that did that.  In particular, ssl_extension_unittest.cc could be split further, which is where a lot of the lost coverage is.

Comment 17

[Kai Engert (:KaiE:)]

Martin, thanks a lot, I appreciate your effort to make it more suitable for us.

But we shouldn't cause you more trouble than necessary.

I executed with both TLS 1.3 enabled and disabled, and when disabled, I found that the following tests are skipped:

AgentTests/TlsAgentTest
ClientTests/TlsAgentTestClient
ExtensionDatagramOnly/TlsExtensionTestDtls
ExtensionDatagram/TlsExtensionTestGeneric
ExtensionPre13Datagram/TlsExtensionTestPre13
ExtensionPre13Stream/TlsExtensionTestPre13
ExtensionStream/TlsExtensionTestGeneric
ExtensionTls12Plus/TlsExtensionTest12Plus
ExtensionTls13/TlsExtensionTest13
Pre12Stream/TlsConnectPre12
SSLv2ClientHelloTestF
TlsAgentStreamTestClient
TlsConnectDatagram13
TlsConnectTest
Version12Only/TlsConnectTls12
Version13Only/TlsConnectTls13
VersionsStream10Pre13/SSLv2ClientHelloTest
VersionsStreamPre13/SSLv2ClientHelloTest

Does that mean, your categorization work might exclude some tests that made sense with <=TLS 1.2 ?

If the categorization cannot perfectly distinguish between tests that work without TLS 1.3 and those that don't, that's fine.

I guess it's better that everything builds fine by default, if the disable-tls1-3 flag is set. If we need a more complete set of tests, we can patch to enable more, if necessary.

Comment 18

[Martin Thomson [:mt:]]

Yes, there are plenty of tests here that are valid for <=1.2 but won't run with this patch.  As you say, we can improve things (possibly a lot) by improving the classification.

Comment 19

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/ef6e31fe3745

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Tiny follow-up that removes NSS_ENABLE_TLS_1_3 from the scan-build task definition too:

https://hg.mozilla.org/projects/nss/rev/f13564c93e43

Comment 21

[Kai Engert (:KaiE:)]

FYI, we found software on Fedora that enables all TLS protocol versions, but doesn't enable all ciphersuites. Might be some application-level configuration mechanism, that disables them all, and only enables the ones that have application configuration support.

I think some more time is required, in distribution-like environments, where NSS is used for multiple purposes, before TLS 1.3 can be enabled universally for a stable environment. The call for testing it is helpful, to get feedback from experimental environments. But having it pushed as enabled by default into distributions that want to pick up Firefox security updates, might trigger some unhappy responses.

Right now, for the Firefox 51 update, until the regressions are fixed in the affected applications, we cannot yet enable TLS 1.3 in the shared system library.