Closed Bug 1264578 Opened 4 years ago Closed 3 years ago

Http with TLS 1.3

Categories

(Core :: Networking: HTTP, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: dragana, Assigned: dragana)

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 8 obsolete files)

We need to make 0-rtt work in necko.
Whiteboard: [necko-active]
Summary: Http over TLS 1.3 → Http with TLS 1.3
Attached patch bug_1264578_nss_part.patch (obsolete) — Splinter Review
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.
Attachment #8771961 - Flags: feedback?(dkeeler)
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
Attachment #8771961 - Flags: feedback?(dkeeler) → feedback+
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.*".
(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?
Attached patch bug_1264578_nss_part.patch (obsolete) — Splinter Review
Attachment #8771961 - Attachment is obsolete: true
Attached patch bug_1264578_h1_v1.patch (obsolete) — Splinter Review
Attached patch bug_1264578_h1_v1.patch (obsolete) — Splinter Review
Attachment #8772492 - Attachment is obsolete: true
David, may I ask you to look at comment #4, thanks!
Flags: needinfo?(dkeeler)
Attached patch bug_1264578_h1_v1.patch (obsolete) — Splinter Review
Current condition to do 0RTT is mRequestHead->IsSafeMethod(), but let's see :)
Attachment #8772495 - Attachment is obsolete: true
Attachment #8778271 - Flags: review?(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
Attachment #8778271 - Flags: review?(mcmanus) → review+
(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).
Flags: needinfo?(dkeeler)
(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 :)
Attached patch bug_1264578_nss_part.patch (obsolete) — Splinter Review
Attachment #8772491 - Attachment is obsolete: true
Attachment #8780045 - Flags: review?(dkeeler)
Attachment #8780045 - Flags: review?(dkeeler)
Attached patch bug_1264578_nss_part.patch (obsolete) — Splinter Review
Attachment #8780045 - Attachment is obsolete: true
Attachment #8780126 - Flags: review?(dkeeler)
Attachment #8778271 - Attachment is obsolete: true
Attachment #8780128 - Flags: review+
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.
Attachment #8780126 - Flags: review?(dkeeler) → review+
(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?
Flags: needinfo?(dkeeler)
Sorry, nevermind - I misunderstood how this works. LGTM! (with other review comments addressed)
Flags: needinfo?(dkeeler)
Attached patch bug_1264578_nss_part.patch (obsolete) — Splinter Review
Attachment #8780126 - Attachment is obsolete: true
Attachment #8782138 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
We need to check for isPK11LoggedOut() as well.

I will push a try run to check.
Flags: needinfo?(dd.mozilla)
Attachment #8782138 - Attachment is obsolete: true
Attachment #8782864 - Flags: review?(dkeeler)
Comment on attachment 8782864 [details] [diff] [review]
bug_1264578_nss_part.patch

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

Ok - makes sense.
Attachment #8782864 - Flags: review?(dkeeler) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/16bcc176a143
https://hg.mozilla.org/mozilla-central/rev/a1fcbe1fc10c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.