Ensure disposal of native auth state after executing Http Method.

Depending on the selected authorization scheme, it's possible for some native OS handles to leak once the HttpMethodDirector has finished executing an HttpMethod. Leaks have been observed for Kerberos (via SPNEGO - Negotiate) but also seem possible using NTLM authentication. Examples of potentially leaked handles include: * credentials handles (e.g. missing call to FreeCredentialsHandle) * security context (e.g. missing call to DeleteSecurityContext) The HttpMethodDirector now ensures that the selected authorization scheme has a chance to cleanup outstanding native OS state once the HttpMethod has finished execution.
microsoft · Jan 29, 2018 · 649aee1 · 649aee1
1 parent c2d7910
commit 649aee1
Show file tree

Hide file tree

Showing 9 changed files with 71 additions and 23 deletions.
diff --git a/...crosoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/HttpMethodDirector.java b/...crosoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/HttpMethodDirector.java
@@ -224,6 +224,8 @@ public void executeMethod(final HttpMethod method) throws IOException, HttpExcep
 
             } // end of retry loop
         } finally {
+            cleanupHostAndProxyAuthState(method);
+
             if (conn != null) {
                 conn.setLocked(false);
             }
@@ -237,6 +239,17 @@ public void executeMethod(final HttpMethod method) throws IOException, HttpExcep
             }
         }
     }
+
+    private void cleanupHostAndProxyAuthState(final HttpMethod method) {
+        cleanupAuthState(method.getHostAuthState());
+        cleanupAuthState(method.getProxyAuthState());
+    }
+
+    private void cleanupAuthState(AuthState authState) {
+        if (authState != null) {
+            authState.invalidate();
+        }
+    }
 
     private Credentials getPreemptiveCredentials(final String host, final int port) {
         if (host == null) {

diff --git a/....microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthScheme.java b/....microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthScheme.java
@@ -79,6 +79,11 @@ public interface AuthScheme {
      */
     public boolean supportsCredentials(Credentials credentials);
 
+    /**
+     * Perform optional cleanup of auth scheme state (e.g. dispose of native OS handles, etc..).
+     */
+    public void cleanup();
+
     /**
      * Processes the given challenge token. Some authentication schemes may
      * involve multiple challenge-response exchanges. Such schemes must be able

diff --git a/...m.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthState.java b/...m.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthState.java
@@ -76,7 +76,11 @@ public AuthState() {
      * Invalidates the authentication state by resetting its parameters.
      */
     public void invalidate() {
-        authScheme = null;
+        if (authScheme != null) {
+            authScheme.cleanup();
+            authScheme = null;
+        }
+
         authRequested = false;
         authAttempted = false;
         preemptive = false;
@@ -193,6 +197,10 @@ public void setAuthScheme(final AuthScheme authScheme) {
             preemptive = false;
             authAttempted = false;
         }
+
+        if (this.authScheme != null) {
+            this.authScheme.cleanup();
+        }
         this.authScheme = authScheme;
     }
 

diff --git a/...microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/BasicScheme.java b/...microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/BasicScheme.java
@@ -253,4 +253,9 @@ public static String authenticate(final UsernamePasswordCredentials credentials,
             + EncodingUtil.getAsciiString(Base64.encodeBase64(EncodingUtil.getBytes(buffer.toString(), charset)));
     }
 
+    @Override
+    public void cleanup() {
+
+    }
+
 }
diff --git a/...soft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/CookieAuthScheme.java b/...soft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/CookieAuthScheme.java
@@ -14,6 +14,11 @@ public boolean supportsCredentials(final Credentials credentials) {
         return (credentials instanceof CookieCredentials);
     }
 
+    @Override
+    public void cleanup() {
+        //No disposing of credentials needed
+    }
+
     @Override
     public void processChallenge(final String challenge) throws MalformedChallengeException {
         throw new MalformedChallengeException("Cookie authentication is not challenge/response"); //$NON-NLS-1$

diff --git a/...icrosoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/DigestScheme.java b/...icrosoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/DigestScheme.java
@@ -609,4 +609,9 @@ public static String createCnonce() {
 
         return cnonce;
     }
+
+    @Override
+    public void cleanup() {
+
+    }
 }
diff --git a/...crosoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/JwtAuthScheme.java b/...crosoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/JwtAuthScheme.java
@@ -21,6 +21,11 @@ public boolean supportsCredentials(final Credentials credentials) {
         return credentials instanceof JwtCredentials;
     }
 
+    @Override
+    public void cleanup() {
+        //No disposing of credentials needed
+    }
+
     @Override
     public void processChallenge(final String challenge) throws MalformedChallengeException {
         complete = true;

diff --git a/....microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NTLMScheme.java b/....microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NTLMScheme.java
@@ -70,6 +70,16 @@ public boolean supportsCredentials(final Credentials credentials) {
         return supportsCredentials(credentials.getClass());
     }
 
+    @Override
+    public void cleanup() {
+        if(ntlmClient != null) {
+            ntlmClient.dispose();
+            ntlmClient = null;
+        }
+
+        inputToken = null;
+    }
+
     public static boolean supportsCredentials(final Class<?> credentialClass) {
         if (credentialClass == null || !isSupported()) {
             return false;
@@ -221,11 +231,7 @@ else if (status != STATUS_EXCHANGING || ntlmClient == null || inputToken == null
             if (ntlmClient.isComplete()) {
                 status = STATUS_COMPLETE;
 
-                /* Clean up */
-                ntlmClient.dispose();
-
-                ntlmClient = null;
-                inputToken = null;
+                cleanup();
             } else {
                 status = STATUS_EXCHANGING;
             }

diff --git a/...osoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NegotiateScheme.java b/...osoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NegotiateScheme.java
@@ -70,6 +70,16 @@ public boolean supportsCredentials(final Credentials credentials) {
         return supportsCredentials(credentials.getClass());
     }
 
+    @Override
+    public void cleanup() {
+        if(negotiateClient != null) {
+            negotiateClient.dispose();
+            negotiateClient = null;
+        }
+
+        inputToken = null;
+    }
+
     public static boolean supportsCredentials(final Class<?> credentialClass) {
         if (credentialClass == null || !isSupported()) {
             return false;
@@ -150,16 +160,7 @@ public void processChallenge(final String challenge) throws MalformedChallengeEx
              * We may be called for retry at an arbitrary time. If that's the
              * case, tear down any existing objects
              */
-            if (negotiateClient != null) {
-                try {
-                    negotiateClient.dispose();
-                } catch (final Exception e) {
-                }
-            }
-
-            negotiateClient = null;
-            inputToken = null;
-
+            cleanup();
             status = STATUS_INITIATED;
         }
         /* The server has responded with a challenge to our request */
@@ -193,7 +194,7 @@ protected String authenticate(final AuthScope authscope, final Credentials crede
             if (status == STATUS_INITIATED && negotiateClient == null && inputToken == null) {
                 negotiateClient = (NegotiateClient) NegotiateEngine.getInstance().newClient();
                 negotiateClient.setTarget("http@" + authscope.getHost().toUpperCase());
-
+                
                 if (credentials instanceof DefaultNTCredentials) {
                     negotiateClient.setCredentialsDefault();
                 } else if (credentials instanceof UsernamePasswordCredentials) {
@@ -221,12 +222,7 @@ else if (status != STATUS_EXCHANGING || negotiateClient == null || inputToken ==
 
             if (negotiateClient.isComplete()) {
                 status = STATUS_COMPLETE;
-
-                /* Clean up */
-                negotiateClient.dispose();
-
-                negotiateClient = null;
-                inputToken = null;
+                cleanup();
             } else {
                 status = STATUS_EXCHANGING;
             }