Bug 1345910 - Allow HTTP connection restart for the first connection-based authentication request in a round. r=mcmanus, a=gchang
authorHonza Bambas <honzab.moz@firemni.cz>
Wed, 26 Apr 2017 17:02:05 +0200
changeset 396094 cb61031b0a99d98b4984cb42d3ad3ec09b92a1e8
parent 396093 dcdffee8cfe9234c2a8af416fe6a684ccd88b6ad
child 396095 7d4d2b3c80021631958ba58fb086de69777d76b6
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus, gchang
bugs1345910
milestone54.0
Bug 1345910 - Allow HTTP connection restart for the first connection-based authentication request in a round. r=mcmanus, a=gchang
netwerk/protocol/http/nsHttp.h
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.h
netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsIHttpAuthenticableChannel.idl
--- a/netwerk/protocol/http/nsHttp.h
+++ b/netwerk/protocol/http/nsHttp.h
@@ -87,16 +87,21 @@ typedef uint8_t nsHttpVersion;
 // Transactions with this flag should react to errors without side effects
 // First user is to prevent clearing of alt-svc cache on failed probe
 #define NS_HTTP_ERROR_SOFTLY         (1<<10)
 
 // This corresponds to nsIHttpChannelInternal.beConservative
 // it disables any cutting edge features that we are worried might result in
 // interop problems with critical infrastructure
 #define NS_HTTP_BE_CONSERVATIVE      (1<<11)
+// NS_HTTP_URGENT_START              (1<<12) Gecko 55+
+
+// A sticky connection of the transaction is explicitly allowed to be restarted
+// on ERROR_NET_RESET.
+#define NS_HTTP_CONNECTION_RESTARTABLE  (1<<13)
 
 //-----------------------------------------------------------------------------
 // some default values
 //-----------------------------------------------------------------------------
 
 #define NS_HTTP_DEFAULT_PORT  80
 #define NS_HTTPS_DEFAULT_PORT 443
 
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -279,16 +279,17 @@ nsHttpChannel::nsHttpChannel()
     , mHasQueryString(0)
     , mConcurrentCacheAccess(0)
     , mIsPartialRequest(0)
     , mHasAutoRedirectVetoNotifier(0)
     , mPinCacheContent(0)
     , mIsCorsPreflightDone(0)
     , mStronglyFramed(false)
     , mUsedNetwork(0)
+    , mAuthConnectionRestartable(0)
     , mPushedStream(nullptr)
     , mLocalBlocklist(false)
     , mWarningReporter(nullptr)
     , mIsReadingFromCache(false)
     , mOnCacheAvailableCalled(false)
     , mRacingNetAndCache(false)
     , mDidReval(false)
 {
@@ -5557,16 +5558,24 @@ NS_IMETHODIMP nsHttpChannel::ForceNoSpdy
 
     if (!(mTransaction->Caps() & NS_HTTP_DISALLOW_SPDY)) {
         mTransaction->DisableSpdy();
     }
 
     return NS_OK;
 }
 
+NS_IMETHODIMP nsHttpChannel::ConnectionRestartable(bool aRestartable)
+{
+    LOG(("nsHttpChannel::ConnectionRestartable this=%p, restartable=%d",
+         this, aRestartable));
+    mAuthConnectionRestartable = aRestartable;
+    return NS_OK;
+}
+
 //-----------------------------------------------------------------------------
 // nsHttpChannel::nsISupports
 //-----------------------------------------------------------------------------
 
 NS_IMPL_ADDREF_INHERITED(nsHttpChannel, HttpBaseChannel)
 NS_IMPL_RELEASE_INHERITED(nsHttpChannel, HttpBaseChannel)
 
 NS_INTERFACE_MAP_BEGIN(nsHttpChannel)
@@ -7561,18 +7570,27 @@ nsHttpChannel::DoAuthRetry(nsAHttpConnec
 
     // rewind the upload stream
     if (mUploadStream) {
         nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mUploadStream);
         if (seekable)
             seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);
     }
 
-    // set sticky connection flag
-    mCaps |=  NS_HTTP_STICKY_CONNECTION;
+    // always set sticky connection flag
+    mCaps |= NS_HTTP_STICKY_CONNECTION;
+    // and when needed, allow restart regardless the sticky flag
+    if (mAuthConnectionRestartable) {
+        LOG(("  connection made restartable"));
+        mCaps |= NS_HTTP_CONNECTION_RESTARTABLE;
+        mAuthConnectionRestartable = false;
+    } else {
+        LOG(("  connection made non-restartable"));
+        mCaps &= ~NS_HTTP_CONNECTION_RESTARTABLE;
+    }
 
     // and create a new one...
     rv = SetupTransaction();
     if (NS_FAILED(rv)) return rv;
 
     // transfer ownership of connection to transaction
     if (conn)
         mTransaction->SetConnection(conn);
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@@ -112,16 +112,17 @@ public:
     NS_IMETHOD GetProxyChallenges(nsACString & aChallenges) override;
     NS_IMETHOD GetWWWChallenges(nsACString & aChallenges) override;
     NS_IMETHOD SetProxyCredentials(const nsACString & aCredentials) override;
     NS_IMETHOD SetWWWCredentials(const nsACString & aCredentials) override;
     NS_IMETHOD OnAuthAvailable() override;
     NS_IMETHOD OnAuthCancelled(bool userCancel) override;
     NS_IMETHOD CloseStickyConnection() override;
     NS_IMETHOD ForceNoSpdy() override;
+    NS_IMETHOD ConnectionRestartable(bool) override;
     // Functions we implement from nsIHttpAuthenticableChannel but are
     // declared in HttpBaseChannel must be implemented in this class. We
     // just call the HttpBaseChannel:: impls.
     NS_IMETHOD GetLoadFlags(nsLoadFlags *aLoadFlags) override;
     NS_IMETHOD GetURI(nsIURI **aURI) override;
     NS_IMETHOD GetNotificationCallbacks(nsIInterfaceRequestor **aCallbacks) override;
     NS_IMETHOD GetLoadGroup(nsILoadGroup **aLoadGroup) override;
     NS_IMETHOD GetRequestMethod(nsACString& aMethod) override;
@@ -592,16 +593,19 @@ private:
     // if the http transaction was performed (i.e. not cached) and
     // the result in OnStopRequest was known to be correctly delimited
     // by chunking, content-length, or h2 end-stream framing
     uint32_t                          mStronglyFramed : 1;
 
     // true if an HTTP transaction is created for the socket thread
     uint32_t                          mUsedNetwork : 1;
 
+    // the next authentication request can be sent on a whole new connection
+    uint32_t                          mAuthConnectionRestartable : 1;
+
     nsTArray<nsContinueRedirectionFunc> mRedirectFuncStack;
 
     // Needed for accurate DNS timing
     RefPtr<nsDNSPrefetch>           mDNSPrefetch;
 
     Http2PushedStream                 *mPushedStream;
     // True if the channel's principal was found on a phishing, malware, or
     // tracking (if tracking protection is enabled) blocklist
--- a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
+++ b/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ -774,16 +774,21 @@ nsHttpChannelAuthProvider::GetCredential
                                      realm.get(), suffix, &entry);
 
     // hold reference to the auth session state (in case we clear our
     // reference to the entry).
     nsCOMPtr<nsISupports> sessionStateGrip;
     if (entry)
         sessionStateGrip = entry->mMetaData;
 
+    // remember if we already had the continuation state.  it means we are in
+    // the middle of the authentication exchange and the connection must be
+    // kept sticky then (and only then).
+    bool authAtProgress = !!*continuationState;
+
     // for digest auth, maybe our cached nonce value simply timed out...
     bool identityInvalid;
     nsISupports *sessionState = sessionStateGrip;
     rv = auth->ChallengeReceived(mAuthChannel,
                                  challenge,
                                  proxyAuth,
                                  &sessionState,
                                  &*continuationState,
@@ -796,37 +801,42 @@ nsHttpChannelAuthProvider::GetCredential
     if (mConnectionBased && identityInvalid) {
         // If the flag is set and identity is invalid, it means we received the first
         // challange for a new negotiation round after negotiating a connection based
         // auth failed (invalid password).
         // The mConnectionBased flag is set later for the newly received challenge,
         // so here it reflects the previous 401/7 response schema.
         mAuthChannel->CloseStickyConnection();
         if (!proxyAuth) {
-          // We must clear proxy ident in the following scenario + explanation:
-          // - we are authenticating to an NTLM proxy and an NTLM server
-          // - we successfully authenticated to the proxy, mProxyIdent keeps
-          //   the user name/domain and password, the identity has also been cached
-          // - we just threw away the connection because we are now asking for
-          //   creds for the server (WWW auth)
-          // - hence, we will have to auth to the proxy again as well
-          // - if we didn't clear the proxy identity, it would be considered
-          //   as non-valid and we would ask the user again ; clearing it forces
-          //   use of the cached identity and not asking the user again
-          mProxyIdent.Clear();
+            // We must clear proxy ident in the following scenario + explanation:
+            // - we are authenticating to an NTLM proxy and an NTLM server
+            // - we successfully authenticated to the proxy, mProxyIdent keeps
+            //   the user name/domain and password, the identity has also been cached
+            // - we just threw away the connection because we are now asking for
+            //   creds for the server (WWW auth)
+            // - hence, we will have to auth to the proxy again as well
+            // - if we didn't clear the proxy identity, it would be considered
+            //   as non-valid and we would ask the user again ; clearing it forces
+            //   use of the cached identity and not asking the user again
+            mProxyIdent.Clear();
         }
-        mConnectionBased = false;
     }
 
     mConnectionBased = !!(authFlags & nsIHttpAuthenticator::CONNECTION_BASED);
     if (mConnectionBased) {
         rv = mAuthChannel->ForceNoSpdy();
         MOZ_ASSERT(NS_SUCCEEDED(rv));
     }
 
+    // It's legal if the peer closes the connection after the first 401/7.
+    // Making the connection sticky will prevent its restart giving the user
+    // a 'network reset' error every time.  Hence, we mark the connection
+    // as restartable.
+    mAuthChannel->ConnectionRestartable(mConnectionBased && !authAtProgress);
+
     if (identityInvalid) {
         if (entry) {
             if (ident->Equals(entry->Identity())) {
                 if (!identFromURI) {
                     LOG(("  clearing bad auth cache entry\n"));
                     // ok, we've already tried this user identity, so clear the
                     // corresponding entry from the auth cache.
                     authCache->ClearAuthEntry(scheme.get(), host,
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -946,17 +946,17 @@ nsHttpTransaction::Close(nsresult reason
     // this bondage.  Major issue may arise when there is an NTLM message auth
     // header on the transaction and we send it to a different NTLM authenticated
     // connection.  It will break that connection and also confuse the channel's
     // auth provider, beliving the cached credentials are wrong and asking for
     // the password mistakenly again from the user.
     if ((reason == NS_ERROR_NET_RESET ||
          reason == NS_OK ||
          reason == psm::GetXPCOMFromNSSError(SSL_ERROR_DOWNGRADE_WITH_EARLY_DATA)) &&
-        !(mCaps & NS_HTTP_STICKY_CONNECTION)) {
+        (!(mCaps & NS_HTTP_STICKY_CONNECTION) || (mCaps & NS_HTTP_CONNECTION_RESTARTABLE))) {
 
         if (mForceRestart && NS_SUCCEEDED(Restart())) {
             if (mResponseHead) {
                 mResponseHead->Reset();
             }
             mContentRead = 0;
             mContentLength = -1;
             delete mChunkedDecoder;
--- a/netwerk/protocol/http/nsIHttpAuthenticableChannel.idl
+++ b/netwerk/protocol/http/nsIHttpAuthenticableChannel.idl
@@ -113,9 +113,16 @@ interface nsIHttpAuthenticableChannel : 
      */
     void closeStickyConnection();
 
     /**
      * Tells the channel to not use SPDY-like protocols, since this will be
      * using connection-oriented auth.
      */
     void forceNoSpdy();
+
+    /**
+     * Tells the channel to mark the connection as allowed to restart on
+     * authentication retry.  This is allowed when the request is a start
+     * of a new authentication round.
+     */
+    void connectionRestartable(in boolean restartable);
 };