All users were logged out of Bugzilla on October 13th, 2018

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

48 Branch
mozilla51
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 affected, firefox51 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

3 years ago
We need to make 0-rtt work in necko.
(Assignee)

Updated

3 years ago
Whiteboard: [necko-active]
(Assignee)

Updated

3 years ago
Summary: Http over TLS 1.3 → Http with TLS 1.3
(Assignee)

Comment 1

2 years ago
Created attachment 8771961 [details] [diff] [review]
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.
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.*".
(Assignee)

Comment 4

2 years ago
(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?
(Assignee)

Comment 5

2 years ago
Created attachment 8772491 [details] [diff] [review]
bug_1264578_nss_part.patch
Attachment #8771961 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Created attachment 8772492 [details] [diff] [review]
bug_1264578_h1_v1.patch
(Assignee)

Comment 7

2 years ago
Created attachment 8772495 [details] [diff] [review]
bug_1264578_h1_v1.patch
Attachment #8772492 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
David, may I ask you to look at comment #4, thanks!
Flags: needinfo?(dkeeler)
(Assignee)

Comment 9

2 years ago
Created attachment 8778271 [details] [diff] [review]
bug_1264578_h1_v1.patch

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)
(Assignee)

Comment 12

2 years ago
(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 :)
(Assignee)

Comment 13

2 years ago
Created attachment 8780045 [details] [diff] [review]
bug_1264578_nss_part.patch
Attachment #8772491 - Attachment is obsolete: true
Attachment #8780045 - Flags: review?(dkeeler)
(Assignee)

Updated

2 years ago
Attachment #8780045 - Flags: review?(dkeeler)
(Assignee)

Comment 14

2 years ago
Created attachment 8780126 [details] [diff] [review]
bug_1264578_nss_part.patch
Attachment #8780045 - Attachment is obsolete: true
Attachment #8780126 - Flags: review?(dkeeler)
(Assignee)

Comment 15

2 years ago
Created attachment 8780128 [details] [diff] [review]
bug_1264578_h1_v1.patch
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+
(Assignee)

Comment 17

2 years ago
(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)
(Assignee)

Comment 19

2 years ago
Created attachment 8782138 [details] [diff] [review]
bug_1264578_nss_part.patch
Attachment #8780126 - Attachment is obsolete: true
Attachment #8782138 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 21

2 years ago
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
(Assignee)

Comment 24

2 years ago
We need to check for isPK11LoggedOut() as well.

I will push a try run to check.
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 26

2 years ago
Created attachment 8782864 [details] [diff] [review]
bug_1264578_nss_part.patch
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 28

2 years ago
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

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16bcc176a143
https://hg.mozilla.org/mozilla-central/rev/a1fcbe1fc10c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
status-firefox48: affected → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → affected
You need to log in before you can comment on or make changes to this bug.