1388925 - Adding opaque flags for non-standard behavior of the TLS system

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Comment 1

[Sajjad Arshad]

Attachment: opaque-tls-flags.patch

Comment 2

[Patrick McManus [:mcmanus]]

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a11737bb6262f9373adc5710f78b8fd626783bde

Comment 3

[Patrick McManus [:mcmanus]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=353e217a920f2d5c59b00e286cefd06ff29264c9

Comment 4

[Sajjad Arshad]

Attachment: opaque-tls-flags.patch

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8895642 [details] [diff] [review]
opaque-tls-flags.patch

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

fwiw I don't mind redundancy between tests

::: netwerk/test/unit/test_tls_flags_separate_connections.js
@@ +83,4 @@
>  };
>  
>  function doTest() {
> +  for (let tlsFlags = 0; tlsFlags < 3; tlsFlags++) {

perhaps document that 3 is not really special

Comment 6

[Sajjad Arshad]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8895642 [details] [diff] [review]
> opaque-tls-flags.patch
> 
> Review of attachment 8895642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> fwiw I don't mind redundancy between tests
> 
> ::: netwerk/test/unit/test_tls_flags_separate_connections.js
> @@ +83,4 @@
> >  };
> >  
> >  function doTest() {
> > +  for (let tlsFlags = 0; tlsFlags < 3; tlsFlags++) {
> 
> perhaps document that 3 is not really special

We can improve it when we are writing new tests for the next patch. Thanks for all the feedback.

Comment 7

[Patrick McManus [:mcmanus]]

you have a static analysis fail in that try run that needs to be fixed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=353e217a920f2d5c59b00e286cefd06ff29264c9&selectedJob=122149915&filter-searchStr=Linux%20x64%20debug%20static-analysis-linux64-st-an%2Fdebug%20(S)

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Sajjad Arshad from comment #6)
>\
> We can improve it when we are writing new tests for the next patch. Thanks
> for all the feedback.

I'm not sure what you mean, but please update the comments in the test as a condition of the r+

thanks.

Comment 9

[Sajjad Arshad]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> (In reply to Sajjad Arshad from comment #6)
> >\
> > We can improve it when we are writing new tests for the next patch. Thanks
> > for all the feedback.
> 
> I'm not sure what you mean, but please update the comments in the test as a
> condition of the r+
> 
> thanks.

So, we decided to add the code for interpretation of the TLS flags in NSS in a new patch. We probably need to write a whole bunch of tests for all the supporting flags. I am saying we can modify this test as well to a more proper one at that time.

Comment 10

[Patrick McManus [:mcmanus]]

my comment is about why you chose 3. The test makes it look like that's important, but its not.

Comment 11

[Sajjad Arshad]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> my comment is about why you chose 3. The test makes it look like that's
> important, but its not.

I see what you mean. Let me modify it to resolve this.

Comment 12

[Sajjad Arshad]

Attachment: opaque-tls-flags.patch

This new patch improved the unit test.

Comment 13

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8895989 [details] [diff] [review]
opaque-tls-flags.patch

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

In general this looks good to me (I reviewed the security/manager changes, although I did also look at the others). I just have a few formatting comments and notes about documentation.
As a note on the commit message, the final patch will need a short summary in the line that starts with "Bug 1388925:" of the format "Bug 1388925: add flags to nsIHttpChannelInternal that get passed to the TLS layer r=mcmanus r=keeler" or something.
Also, it looks like this patch was generated with 3 lines of context - we use 8.
Is this a temporary API or do you expect this capability to be around for a long time? If the former, let's file a bug to remove this when we're done with it.

::: netwerk/base/nsISocketTransport.idl
@@ +238,5 @@
>      const unsigned long BE_CONSERVATIVE = (1 << 7);
>  
>      /**
> +     * opaque flags for non-standard behavior of the TLS system
> +     * do not set these unless you are a TLS developer.

In terms of tone, I think "do not set these unless you are a TLS developer" is a bit hostile. Perhaps you can phrase this a bit more neutrally like "it is unlikely these will need to be set outside of telemetry studies relating to the TLS implementation" or something?

::: netwerk/test/unit/test_tls_flags_separate_connections.js
@@ +7,3 @@
>  });
>  
>  // This unit test ensures each container has its own connection pool.

nit: this isn't about containers. You might just s/each container/connections with different tlsFlags/ or similar.

@@ +99,5 @@
> +  for (let i = 0; i < randomFlagValues.length; i++) {
> +    let flag_value = 0;
> +
> +    for (let j = 0; j < 32; j++) {
> +      if (Math.random() < 0.5) {

Perhaps rather than generating random values (which may not exercise all test cases we care about), let's pick a concrete set. I'm thinking we'll want at least one duplicate flag value (to explicitly test the case where the tlsFlags are the same), a pair of flags that have zero bits in common, a pair of flags that have some bits in common but where each have other bits set that are not in common and a pair of flags where all of the bits set in one are set in the other (and where the other has more bits set).

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +75,4 @@
>  
>  extern LazyLogModule gPIPNSSLog;
>  
> +nsNSSSocketInfo::nsNSSSocketInfo(SharedSSLState& aState, uint32_t providerFlags, uint32_t providerTlsFlags)

nit: wrap at 80 characters, please

::: security/manager/ssl/nsNSSIOLayer.h
@@ +35,4 @@
>                                public nsIClientAuthUserDecision
>  {
>  public:
> +  nsNSSSocketInfo(mozilla::psm::SharedSSLState& aState, uint32_t providerFlags, uint32_t providerTlsFlags);

nit: wrap at 80 characters

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8895989 [details] [diff] [review]
opaque-tls-flags.patch

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

refining the test isn't a big deal - but you didn't fix the try run fail (comment 7). You can't land it if the CI will reject your patch.

::: netwerk/test/unit/test_tls_flags_separate_connections.js
@@ +99,5 @@
> +  for (let i = 0; i < randomFlagValues.length; i++) {
> +    let flag_value = 0;
> +
> +    for (let j = 0; j < 32; j++) {
> +      if (Math.random() < 0.5) {

I think that's good advice

Comment 15

[Sajjad Arshad]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13)
> Comment on attachment 8895989 [details] [diff] [review]
> opaque-tls-flags.patch
> 
> Review of attachment 8895989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general this looks good to me (I reviewed the security/manager changes,
> although I did also look at the others). I just have a few formatting
> comments and notes about documentation.
> As a note on the commit message, the final patch will need a short summary
> in the line that starts with "Bug 1388925:" of the format "Bug 1388925: add
> flags to nsIHttpChannelInternal that get passed to the TLS layer r=mcmanus
> r=keeler" or something.
> Also, it looks like this patch was generated with 3 lines of context - we
> use 8.
> Is this a temporary API or do you expect this capability to be around for a
> long time? If the former, let's file a bug to remove this when we're done
> with it.
> 
> ::: netwerk/base/nsISocketTransport.idl
> @@ +238,5 @@
> >      const unsigned long BE_CONSERVATIVE = (1 << 7);
> >  
> >      /**
> > +     * opaque flags for non-standard behavior of the TLS system
> > +     * do not set these unless you are a TLS developer.
> 
> In terms of tone, I think "do not set these unless you are a TLS developer"
> is a bit hostile. Perhaps you can phrase this a bit more neutrally like "it
> is unlikely these will need to be set outside of telemetry studies relating
> to the TLS implementation" or something?
> 
> ::: netwerk/test/unit/test_tls_flags_separate_connections.js
> @@ +7,3 @@
> >  });
> >  
> >  // This unit test ensures each container has its own connection pool.
> 
> nit: this isn't about containers. You might just s/each
> container/connections with different tlsFlags/ or similar.
> 
> @@ +99,5 @@
> > +  for (let i = 0; i < randomFlagValues.length; i++) {
> > +    let flag_value = 0;
> > +
> > +    for (let j = 0; j < 32; j++) {
> > +      if (Math.random() < 0.5) {
> 
> Perhaps rather than generating random values (which may not exercise all
> test cases we care about), let's pick a concrete set. I'm thinking we'll
> want at least one duplicate flag value (to explicitly test the case where
> the tlsFlags are the same), a pair of flags that have zero bits in common, a
> pair of flags that have some bits in common but where each have other bits
> set that are not in common and a pair of flags where all of the bits set in
> one are set in the other (and where the other has more bits set).
> 
> ::: security/manager/ssl/nsNSSIOLayer.cpp
> @@ +75,4 @@
> >  
> >  extern LazyLogModule gPIPNSSLog;
> >  
> > +nsNSSSocketInfo::nsNSSSocketInfo(SharedSSLState& aState, uint32_t providerFlags, uint32_t providerTlsFlags)
> 
> nit: wrap at 80 characters, please
> 
> ::: security/manager/ssl/nsNSSIOLayer.h
> @@ +35,4 @@
> >                                public nsIClientAuthUserDecision
> >  {
> >  public:
> > +  nsNSSSocketInfo(mozilla::psm::SharedSSLState& aState, uint32_t providerFlags, uint32_t providerTlsFlags);
> 
> nit: wrap at 80 characters

I had a question regarding your concern about formatting. As you know, we modified 27 files, and each file contains a little piece of our new feature. So, I was wondering if I should commit them as one giant commit or you want a multiple smaller commits? If you prefer the later, I guess we should generate multiple changset patches. I am using `hg export` to generate these patches? What command is the best that you are suggesting for generating the patches?

Comment 16

[Dana Keeler (she/her) [:keeler]]

I think this change is best expressed as a single changeset so it's clear that everything's related. `hg export` is good, provided you have the appropriate settings in your .hgrc:

[diff]
git = 1
showfunc = 1
unified = 8

Comment 17

[Sajjad Arshad]

Attachment: opaque-tls-flags.patch

This is the final patch that tested on Try server successfully as well.

Comment 18

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=698c0068db437fba774bd5558e5ff2b8ab1edde5

Comment 19

[Patrick McManus [:mcmanus]]

Comment on attachment 8897631 [details] [diff] [review]
opaque-tls-flags.patch

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

please upload a new version of the patch that has the first line that is in the normal format you see in the commit log.. i.e.

Bug XXX - desc r=mcmanus,keeler

(and then a para of stuff is fine)

when you upload it you can just mark it r+

Comment 20

[Sajjad Arshad]

Attachment: opaque-tls-flags.patch

Comment 21

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8897678 [details] [diff] [review]
opaque-tls-flags.patch

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

r=me with nits addressed
I also have a comment on the way this updates the hash key in nsHttpConnectionInfo, but unless :mcmanus says it's required, it's just a suggestion (if it even works).

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +317,5 @@
> +void
> +nsHttpConnectionInfo::SetTlsFlags(uint32_t aTlsFlags) {
> +    mTlsFlags = aTlsFlags;
> +
> +    char buf[15];

For what it's worth, 15 seems like an odd choice since SnprintfLiteral will need 9 bytes.
In any case, I suspect you could use nsPrintfCString directly in the Replace call like so:

mHashKey.Replace(18, 8, nsPrintfCString("%08x", mTlsFlags));

This still leaves the hard-coded 18 and 8, but I'm not sure how many hoops we would want to jump through to make that more general. This isn't my area of code, though, so don't feel like you have to do this unless :mcmanus says so.

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +75,5 @@
>  
>  extern LazyLogModule gPIPNSSLog;
>  
> +nsNSSSocketInfo::nsNSSSocketInfo(SharedSSLState& aState, uint32_t providerFlags,
> +uint32_t providerTlsFlags)

nit: this should line up with the previous line so that the u in uint32_t is below the first S in SharedSSLState

::: security/manager/ssl/nsNSSIOLayer.h
@@ +35,5 @@
>                                public nsIClientAuthUserDecision
>  {
>  public:
> +  nsNSSSocketInfo(mozilla::psm::SharedSSLState& aState, uint32_t providerFlags,
> +  uint32_t providerTlsFlags);

nit: alignment again

Comment 22

[Sajjad Arshad]

Attachment: opaque-tls-flags.patch

Thanks for your feedbacks. To the best of my knowledge, this is the patch that addressed all the comments. Please let me know what the next step is.

Comment 23

[Dana Keeler (she/her) [:keeler]]

Next you'll need to test the patch on the try server: https://wiki.mozilla.org/ReleaseEngineering/TryServer
I would build on one or two platforms you didn't develop on and run the xpcshell test suite.
If that all comes up green, you can paste a link to your try results and add the 'checkin-needed' keyword to this bug ("Edit Bug" -> "Keywords").

Comment 24

[Sajjad Arshad]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #23)
> Next you'll need to test the patch on the try server:
> https://wiki.mozilla.org/ReleaseEngineering/TryServer
> I would build on one or two platforms you didn't develop on and run the
> xpcshell test suite.
> If that all comes up green, you can paste a link to your try results and add
> the 'checkin-needed' keyword to this bug ("Edit Bug" -> "Keywords").

We already did both. Here is the link to Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=698c0068db437fba774bd5558e5ff2b8ab1edde5

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe9b1d9ad11
Add an opaque flags to have a fine-grained control over TLS configurations. r=mcmanus, r=keeler

Comment 26

[Sajjad Arshad]

(In reply to Pulsebot from comment #25)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe9b1d9ad11
> Add an opaque flags to have a fine-grained control over TLS configurations.
> r=mcmanus, r=keeler

Thanks. This is my first official contribution to Firefox :)

Comment 27

[Dana Keeler (she/her) [:keeler]]

Congrats!

Comment 28

[Sajjad Arshad]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #27)
> Congrats!

Thanks for your help. More patches to come :)

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 30

[Sajjad Arshad]

(In reply to Wes Kocher (:KWierso) from comment #29)
> https://hg.mozilla.org/mozilla-central/rev/bfe9b1d9ad11

Sounds great. Thanks.