Bug 1345910 - Allow HTTP connection restart for the first connection-based authentication request in a round. r=mcmanus
authorHonza Bambas <honzab.moz@firemni.cz>
Wed, 26 Apr 2017 17:02:05 +0200
changeset 403221 3c182ffc4e7655e529541d06c4b6b598c3373d6a
parent 403220 6fbda056532ebc49e993d169799e2b19205b36ba
child 403222 c9210201f671ae0ca61e517aff6c0dcdc7419f7b
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus
bugs1345910
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1345910 - Allow HTTP connection restart for the first connection-based authentication request in a round. r=mcmanus
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
@@ -91,16 +91,20 @@ typedef uint8_t nsHttpVersion;
 // 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)
 
 // Transactions with this flag should be processed first.
 #define NS_HTTP_URGENT_START         (1<<12)
 
+// 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
 
 #define NS_HTTP_HEADER_SEPS ", \t"
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -284,16 +284,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)
     , mRaceCacheWithNetwork(false)
     , mDidReval(false)
 {
@@ -5662,16 +5663,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)
@@ -7830,18 +7839,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
@@ -113,16 +113,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;
@@ -623,16 +624,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
@@ -775,16 +775,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,
@@ -798,37 +803,42 @@ nsHttpChannelAuthProvider::GetCredential
         // 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.
         rv = mAuthChannel->CloseStickyConnection();
         MOZ_ASSERT(NS_SUCCEEDED(rv));
         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
@@ -993,17 +993,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 : 
      */
     [must_use] void closeStickyConnection();
 
     /**
      * Tells the channel to not use SPDY-like protocols, since this will be
      * using connection-oriented auth.
      */
     [must_use] 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);
 };