1264578 - Http with TLS 1.3

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dragana Damjanovic [:dragana]]

We need to make 0-rtt work in necko.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_nss_part.patch

I have extended nss for 0RTT. May I ask you to  take a look.
I am not sure if DriveHandshake should return NS_ERROR_FAILURE(as it does now) or should it transform PR error into NS? The necko code that is using this, in the current version, calls DriveHandshake and if it gets NS_ERROR_FAILURE it calls PRWrite to pick up the real error (this is closes what it use to do, but this can be changed).

(Maybe I need some locking?)

Thanks.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8771961 [details] [diff] [review]
bug_1264578_nss_part.patch

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

This looks good, but I have a couple of questions:
* What threads are the new APIs expected to be called on? This would influence whether or not locking is necessary (although, as-is, it doesn't look like nsNSSSocketInfo is properly thread-safe in the first place). (Also, for example, SSL_ForceHandshake blocks, so we wouldn't want that to block the main thread.)
* According to https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/nss/lib/ssl/ssl.h#234 at least some part of the 0-rtt implementation hasn't landed yet. Is that out of date?

::: netwerk/socket/nsISSLSocketControl.idl
@@ +43,5 @@
>       * raised.
>       */
>      readonly attribute ACString negotiatedNPN;
>  
> +    /* For 0RTT we need to know alpn protocol selected for the last tls

nit: "... know the alpn protocol..."

@@ +54,5 @@
> +     * the handshake finishes this attribute will be set to appropriate value.
> +     */
> +    readonly attribute bool earlyDataAccepted;
> +
> +    /* When 0RTT is perform, PR_Write will not drive handshake forward. It must

nit: "... performed, PR_Write will not drive the handshake..."

@@ +55,5 @@
> +     */
> +    readonly attribute bool earlyDataAccepted;
> +
> +    /* When 0RTT is perform, PR_Write will not drive handshake forward. It must
> +     * be force by calling this function.

nit: "forced"

::: security/manager/ssl/nsNSSComponent.cpp
@@ +2006,5 @@
>        SSL_OptionSetDefault(SSL_ENABLE_ALPN,
>                             Preferences::GetBool("security.ssl.enable_alpn",
>                                                  ALPN_ENABLED_DEFAULT));
> +    } else if (prefName.EqualsLiteral("security.ssl.enable_0rtt_data")) {
> +      SSL_OptionSetDefault(SSL_ENABLE_FALSE_START,

Looks like this should be "SSL_ENABLE_0RTT_DATA", right?

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +317,5 @@
> +  char chosenAlpn[MAX_ALPN_LENGTH];
> +  unsigned int chosenAlpnLen;
> +  SECStatus rv = SSL_GetNextProto(mFd, &alpnState,
> +                                  reinterpret_cast<unsigned char*>(chosenAlpn),
> +                                  &chosenAlpnLen, sizeof(chosenAlpn));

Let's use ArrayLength and the casts similar to https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/manager/ssl/nsNSSCallbacks.cpp#868 (and line 872).

@@ +319,5 @@
> +  SECStatus rv = SSL_GetNextProto(mFd, &alpnState,
> +                                  reinterpret_cast<unsigned char*>(chosenAlpn),
> +                                  &chosenAlpnLen, sizeof(chosenAlpn));
> +
> +  if ((rv == SECSuccess) &&

Let's switch this around:

if (rv != SECSuccess || alpnState != SSL_NEXT_PROTO_EARLY_VALUE || chosenAlpnLen == 0) {
  return NS_ERROR_NOT_AVAILABLE;
}

// continue with success case...

@@ +360,5 @@
> +  SECStatus rv = SSL_ForceHandshake(mFd);
> +
> +  if (rv != SECSuccess) {
> +    PRErrorCode errorCode;
> +    errorCode = PR_GetError();

nit: PRErrorCode errorCode = PR_GetError();

@@ +365,5 @@
> +    if (errorCode == PR_WOULD_BLOCK_ERROR) {
> +      return NS_BASE_STREAM_WOULD_BLOCK;
> +    }
> +    SSLErrorMessageType errorMessageType = PlainErrorMessage;
> +    SetCanceled(errorCode, errorMessageType);

nit: SetCanceled(errorCode, PlainErrorMessage);

@@ +366,5 @@
> +      return NS_BASE_STREAM_WOULD_BLOCK;
> +    }
> +    SSLErrorMessageType errorMessageType = PlainErrorMessage;
> +    SetCanceled(errorCode, errorMessageType);
> +    return NS_ERROR_FAILURE;

you could return GetXPCOMFromNSSError(errorCode); here

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 8771961 [details] [diff] [review]
bug_1264578_nss_part.patch

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

::: netwerk/base/security-prefs.js
@@ +14,5 @@
>  pref("security.ssl.enable_false_start", true);
>  pref("security.ssl.false_start.require-npn", false);
>  pref("security.ssl.enable_npn", true);
>  pref("security.ssl.enable_alpn", true);
> +pref("security.ssl.enable_0rtt_data", false);

IMO new pref names should use "security.tls.*".

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> Comment on attachment 8771961 [details] [diff] [review]
> bug_1264578_nss_part.patch
> 
> Review of attachment 8771961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks for the feedback.


> This looks good, but I have a couple of questions:
> * What threads are the new APIs expected to be called on? This would
> influence whether or not locking is necessary (although, as-is, it doesn't
> look like nsNSSSocketInfo is properly thread-safe in the first place).
> (Also, for example, SSL_ForceHandshake blocks, so we wouldn't want that to
> block the main thread.)

GetAlpnEarlySelection, GetEarlyDataAccepted and DriveHandshake are called from SocketThread.
SetEarlyDataAccepted is called by PreliminaryHandshakeDone, probably socketThread (I am not sure).
SetCanceled is probably called from multiple threads(e.g. cert verification fails? maybe I am wrong, I am not an expert).

SSL_ForceHandshake will not block on a socket if the socket is non-blocking. Am I right? If there is nothing to read/write or the socket would block, it will not block on something else (anything else)?I want to double check this.

> * According to
> https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/security/nss/lib/ssl/ssl.h#234 at
> least some part of the 0-rtt implementation hasn't landed yet. Is that out
> of date?
> 
 
Everything that we need for 0RTT to work seems to be there, but ekr is the right person to answer this.


> @@ +366,5 @@
> > +      return NS_BASE_STREAM_WOULD_BLOCK;
> > +    }
> > +    SSLErrorMessageType errorMessageType = PlainErrorMessage;
> > +    SetCanceled(errorCode, errorMessageType);
> > +    return NS_ERROR_FAILURE;
> 
> you could return GetXPCOMFromNSSError(errorCode); here

At line 357:
>  if (GetErrorCode()) {
>    return NS_ERROR_FAILURE;

should this be GetXPCOMFromNSSError(GetErrorCode()) as well?

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_nss_part.patch

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_h1_v1.patch

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_h1_v1.patch

Comment 8

[Dragana Damjanovic [:dragana]]

David, may I ask you to look at comment #4, thanks!

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_h1_v1.patch

Current condition to do 0RTT is mRequestHead->IsSafeMethod(), but let's see :)

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8778271 [details] [diff] [review]
bug_1264578_h1_v1.patch

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

some minor things to fixup - but I don't need to review it again.

::: netwerk/protocol/http/nsAHttpTransaction.h
@@ +208,5 @@
> +    // Returns true if early-data is possible.
> +    virtual bool Do0RTT() {
> +        return false;
> +    }
> +    // This function will be call when a tls handshake has been finished and we

s/call/called
please define in the comment what the return code and parameter mean

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +273,5 @@
>      }
>  }
>  
>  bool
> +nsHttpConnection::EnsureNPNComplete(nsresult &aRv, uint32_t &aTransactionBytes)

naming: you can't call a parameter rv or aRv or whatnot. RV means return value :) something like out0RTTWriteHandshakeValue would be better.. same thing with aTransactionBytes.. out0RTTBytesWritten perhaps

these are out parameters and really need to be initialized first thing in this function - they are often undefined.

have you tested this with data that doesn't fit in one write? Which could conceivably by in multiple 0RTT packets before negotiation.. I guess a big cookie would do it. It looks like it should work, and I think if it doesn't its fine to followup (as long as the failure mode is ok) rather than blocking the h1 milestone.

@@ +304,5 @@
>      if (NS_FAILED(rv))
>          goto npnComplete;
>  
>      rv = ssl->GetNegotiatedNPN(negotiatedNPN);
> +    if (!m0RTTChecked && (rv == NS_ERROR_NOT_CONNECTED)) {

I would add !proxy to this conditional

also a high level comment:

// There is no ALPN info (yet!). We need to consider doing 0RTT.. we will do
// so if there is ALPN information from a previous session (AlpnEarlySelection),
// we are using HTTP/1, and the request data can be safely retried

@@ +310,5 @@
> +        m0RTTChecked = true;
> +        nsAutoCString earlyNegotiatedNPN;
> +        nsresult rvEarlyAlpn = ssl->GetAlpnEarlySelection(earlyNegotiatedNPN);
> +        if (rvEarlyAlpn == NS_ERROR_NOT_AVAILABLE) {
> +            // if ssl->DriveHandshake() has never been called the value

it seems like a lot of this code could be simplified if GetAlpnEarlySelection would call DriveHandshake itself before returning NOT_AVAILABLE.. but maybe that screws up the chance to get a real (non early) ALPN token on the second iteration. your call.

@@ +329,5 @@
> +                rvEarlyAlpn = ssl->GetAlpnEarlySelection(earlyNegotiatedNPN);
> +            }
> +        }
> +
> +        if (rvEarlyAlpn == NS_ERROR_NOT_AVAILABLE) {

I feel like we should be checking NS_SUCCEEDED/FAILED here rather than assuming the API can only return one error code

@@ +337,5 @@
> +            LOG(("nsHttpConnection::EnsureNPNComplete %p -"
> +                 "early selected alpn: %s", this, earlyNegotiatedNPN.get()));
> +            uint32_t infoIndex;
> +            const SpdyInformation *info = gHttpHandler->SpdyInfo();
> +            if (!NS_SUCCEEDED(info->GetNPNIndex(earlyNegotiatedNPN, &infoIndex))) {

FAILED instead of !SUCCEEDED

and add a comment that we're only trying to do this for HTTP/1 right now - it took me a while to figure out what was being accomplished :)

@@ +340,5 @@
> +            const SpdyInformation *info = gHttpHandler->SpdyInfo();
> +            if (!NS_SUCCEEDED(info->GetNPNIndex(earlyNegotiatedNPN, &infoIndex))) {
> +                // Check if early-data is allowed for this transaction.
> +                if (mTransaction->Do0RTT()) {
> +                    mWaitingFor0RTTResponse = true;

This is a place for a log if I ever saw one :)

@@ +354,5 @@
> +                                             &aTransactionBytes);
> +            if (NS_FAILED(aRv) && aRv != NS_BASE_STREAM_WOULD_BLOCK) {
> +                goto npnComplete;
> +            }
> +            mContentBytesWritten0RTT += aTransactionBytes;

LOG

@@ +374,5 @@
> +        bool ealyDataAccepted = false;
> +        if (mWaitingFor0RTTResponse) {
> +            mWaitingFor0RTTResponse = false;
> +            // Check if early data has been accepted.
> +            rv = ssl->GetEarlyDataAccepted(&ealyDataAccepted);

LOG

@@ +388,5 @@
> +            if (NS_SUCCEEDED(info->GetNPNIndex(negotiatedNPN, &infoIndex))) {
> +                StartSpdy(info->Version[infoIndex]);
> +            }
> +        } else {
> +          mContentBytesWritten = mContentBytesWritten0RTT;

LOG

@@ +1660,5 @@
>          // request differently for http/1, http/2, spdy, etc.. and that is
>          // negotiated with NPN/ALPN in the SSL handshake.
>  
> +        if (mConnInfo->UsingHttpsProxy() && !EnsureNPNComplete(rv, transactionBytes)) {
> +            if (NS_SUCCEEDED(rv) && !transactionBytes &&

I think if you don't do 0RTT on proxy connects then you can leave the body as it was and just assert !transactionbytes in it.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +2323,5 @@
>  
> +bool
> +nsHttpTransaction::Do0RTT()
> +{
> +   if (mRequestHead->IsSafeMethod()) {

I think maybe mConnection->IsProxyConnectInPrrogress() should also disable 0RTT

@@ +2335,5 @@
> +{
> +    MOZ_ASSERT(m0RTTInProgress);
> +    m0RTTInProgress = false;
> +    if (aRestart) {
> +        // Reset request headers to be send again.

s/send/sent

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Dragana Damjanovic [:dragana] from comment #4)

Sorry for the slow response - I was on vacation.

> (In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> > This looks good, but I have a couple of questions:
> > * What threads are the new APIs expected to be called on? This would
> > influence whether or not locking is necessary (although, as-is, it doesn't
> > look like nsNSSSocketInfo is properly thread-safe in the first place).
> > (Also, for example, SSL_ForceHandshake blocks, so we wouldn't want that to
> > block the main thread.)
> 
> GetAlpnEarlySelection, GetEarlyDataAccepted and DriveHandshake are called
> from SocketThread.
> SetEarlyDataAccepted is called by PreliminaryHandshakeDone, probably
> socketThread (I am not sure).
> SetCanceled is probably called from multiple threads(e.g. cert verification
> fails? maybe I am wrong, I am not an expert).

Looks like SetCanceled is called from the socket thread, the certificate verification thread(s?), and the main thread :/

> SSL_ForceHandshake will not block on a socket if the socket is non-blocking.
> Am I right? If there is nothing to read/write or the socket would block, it
> will not block on something else (anything else)?I want to double check this.

That would make sense, but I'm not certain.

> > @@ +366,5 @@
> > > +      return NS_BASE_STREAM_WOULD_BLOCK;
> > > +    }
> > > +    SSLErrorMessageType errorMessageType = PlainErrorMessage;
> > > +    SetCanceled(errorCode, errorMessageType);
> > > +    return NS_ERROR_FAILURE;
> > 
> > you could return GetXPCOMFromNSSError(errorCode); here
> 
> At line 357:
> >  if (GetErrorCode()) {
> >    return NS_ERROR_FAILURE;
> 
> should this be GetXPCOMFromNSSError(GetErrorCode()) as well?

Sure - it probably won't make a huge difference (it looks like the consumer in the other patch really only cares if it's NS_OK or NS_BASE_STREAM_WOULD_BLOCK, but it might be informative for debugging in the future).

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #11)
> (In reply to Dragana Damjanovic [:dragana] from comment #4)
> 
> Sorry for the slow response - I was on vacation.
> 
> > (In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> > > This looks good, but I have a couple of questions:
> > > * What threads are the new APIs expected to be called on? This would
> > > influence whether or not locking is necessary (although, as-is, it doesn't
> > > look like nsNSSSocketInfo is properly thread-safe in the first place).
> > > (Also, for example, SSL_ForceHandshake blocks, so we wouldn't want that to
> > > block the main thread.)
> > 
> > GetAlpnEarlySelection, GetEarlyDataAccepted and DriveHandshake are called
> > from SocketThread.
> > SetEarlyDataAccepted is called by PreliminaryHandshakeDone, probably
> > socketThread (I am not sure).
> > SetCanceled is probably called from multiple threads(e.g. cert verification
> > fails? maybe I am wrong, I am not an expert).
> 
> Looks like SetCanceled is called from the socket thread, the certificate
> verification thread(s?), and the main thread :/
> 

Set/GetCanceled is locked in TransportSecurityInfo :)

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_nss_part.patch

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_nss_part.patch

Comment 15

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_h1_v1.patch

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8780126 [details] [diff] [review]
bug_1264578_nss_part.patch

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

I think this looks good, but at a higher level, it looks like the NPN code is doing something similar but in a different way - rather than nsNSSSocketInfo::GetNegotiatedNPN calling NSS directly, PreliminaryHandshakeDone gathers that information and calls nsNSSSocketInfo::SetNegotiatedNPN, which holds on to the data until it is used by GetNegotiatedNPN. Would it be better to reorganize things that way, or is this new way better/more efficient in some way?

::: netwerk/socket/nsISSLSocketControl.idl
@@ +49,5 @@
> +     * NS_ERROR_NOT_AVAILABLE.
> +     */
> +    ACString getAlpnEarlySelection();
> +
> +    /* If 0RTT handshake was applied and some data has beed sent, as soon as

typo: s/beed/been

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +311,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsNSSSocketInfo::GetAlpnEarlySelection(nsACString& aAlpnSelected)
> +{

Forgot to mention this needs NSS shut down prevention:

nsNSSShutDownPreventionLock locker;
if (isAlreadyShutDown()) {
  return NS_ERROR_NOT_AVAILABLE;
}

@@ +346,5 @@
> +nsNSSSocketInfo::DriveHandshake()
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_FAILURE;

Typically we return NS_ERROR_NOT_AVAILABLE in these cases.

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #16)
> Comment on attachment 8780126 [details] [diff] [review]
> bug_1264578_nss_part.patch
> 
> Review of attachment 8780126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good, but at a higher level, it looks like the NPN code
> is doing something similar but in a different way - rather than
> nsNSSSocketInfo::GetNegotiatedNPN calling NSS directly,
> PreliminaryHandshakeDone gathers that information and calls
> nsNSSSocketInfo::SetNegotiatedNPN, which holds on to the data until it is
> used by GetNegotiatedNPN. Would it be better to reorganize things that way,
> or is this new way better/more efficient in some way?

I do not know nss code that good.
alpn-early-select will be set as soon as client hello is created. I could make a callback, similar to handshakeCallback of canFalseStartCallback. 

It is your decision should I land this or change it?

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Sorry, nevermind - I misunderstood how this works. LGTM! (with other review comments addressed)

Comment 19

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_nss_part.patch

Comment 20

[Dragana Damjanovic [:dragana]]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/123049a4ba35
Networking support for http with TLS 1.3. r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/80942fb9a0f1
NSS support for http with TLS 1.3. r=keeler

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in browser_clientAuth_connection.js and likely also browser_save_link-perwindowpb.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/157b52794b4680eacd962daa3022d10b6ff7e023
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b564679412b416c7da2b1e7eb06ffbf8ca795a3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e6ac44ef858aa664e83a35df136caee0f8f261a9

Failure log browser_clientAuth_connection.js: https://treeherder.mozilla.org/logviewer.html#?job_id=34106692&repo=mozilla-inbound
Failure log browser_save_link-perwindowpb.js: https://treeherder.mozilla.org/logviewer.html#?job_id=34106698&repo=mozilla-inbound

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There are also timeouts in test_master_password.html:

https://treeherder.mozilla.org/logviewer.html#?job_id=34106335&repo=mozilla-inbound

Comment 24

[Dragana Damjanovic [:dragana]]

We need to check for isPK11LoggedOut() as well.

I will push a try run to check.

Comment 25

[Dragana Damjanovic [:dragana]]

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

Comment 26

[Dragana Damjanovic [:dragana]]

Attachment: bug_1264578_nss_part.patch

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8782864 [details] [diff] [review]
bug_1264578_nss_part.patch

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

Ok - makes sense.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16bcc176a143
Networking support for http with TLS 1.3. r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1fcbe1fc10c
NSS support for http with TLS 1.3. r=keeler

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16bcc176a143
https://hg.mozilla.org/mozilla-central/rev/a1fcbe1fc10c