HTTPCLIENT-936: Fixed bug causing NPE or an infinite loop in the authentication code in case of a SPNEGO authentication failure

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@943620 13f79535-47bb-0310-9956-ffa450edef68

RELEASE_NOTES.txt

@@ -22,6 +22,10 @@ maintained as of 4.1 GA release.
 Changelog
 -------------------
 
+* [HTTPCLIENT-936] Fixed bug causing NPE or an infinite loop in 
+  the authentication code in case of a SPNEGO authentication failure. 
+  Contributed by Oleg Kalnichevski <olegk at apache.org>
+
 * [HTTPCLIENT-427] HTTP caching support
   Contributed by Joe Campbell, David Cleaver, David Mays, Jon Moore, Brad Spenla
 

httpclient-cache/src/test/java/org/apache/http/client/cache/impl/HttpTestUtils.java

@@ -78,6 +78,17 @@ public class HttpTestUtils {
         return false;
     }
 
+    /*
+     * Determines whether a given header name may appear multiple times.
+     */
+    public static boolean isMultiHeader(String name) {
+        for (String s : MULTI_HEADERS) {
+            if (s.equalsIgnoreCase(name))
+                return true;
+        }
+        return false;
+    }
+
     /*
      * Determines whether a given header name may only appear once in a message.
      */
@@ -88,7 +99,6 @@ public class HttpTestUtils {
         }
         return false;
     }
-
     /*
      * Assert.asserts that two request or response bodies are byte-equivalent.
      */

httpclient-cache/src/test/java/org/apache/http/client/cache/impl/TestCacheEntry.java

@@ -26,8 +26,6 @@
  */
 package org.apache.http.client.cache.impl;
 
-import static junit.framework.Assert.assertFalse;
-
 import java.util.Date;
 import java.util.Set;
 
@@ -376,8 +374,7 @@ public class TestCacheEntry {
                 new BasicHeader("Cache-Control", "public") };
 
         CacheEntry entry = getEntry(headers);
-
+        Assert.assertFalse(entry.isRevalidatable());
-        assertFalse(entry.isRevalidatable());
     }
 
 

httpclient/src/main/java/org/apache/http/impl/auth/AuthSchemeBase.java

@@ -125,7 +125,7 @@ public abstract class AuthSchemeBase implements ContextAwareAuthScheme {
     }
 
     protected abstract void parseChallenge(
-            CharArrayBuffer buffer, int pos, int len) throws MalformedChallengeException;
+            CharArrayBuffer buffer, int beginIndex, int endIndex) throws MalformedChallengeException;
 
     /**
      * Returns <code>true</code> if authenticating against a proxy, <code>false</code>

httpclient/src/main/java/org/apache/http/impl/auth/NTLMScheme.java

@@ -99,8 +99,9 @@ public class NTLMScheme extends AuthSchemeBase {
 
     @Override
     protected void parseChallenge(
-            final CharArrayBuffer buffer, int pos, int len) throws MalformedChallengeException {
+            final CharArrayBuffer buffer, 
-        String challenge = buffer.substringTrimmed(pos, len);
+            int beginIndex, int endIndex) throws MalformedChallengeException {
+        String challenge = buffer.substringTrimmed(beginIndex, endIndex);
         if (challenge.length() == 0) {
             if (this.state == State.UNINITIATED) {
                 this.state = State.CHALLENGE_RECEIVED;

httpclient/src/main/java/org/apache/http/impl/auth/NegotiateScheme.java

@@ -33,15 +33,14 @@ import org.apache.commons.logging.LogFactory;
 import org.apache.http.Header;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
-import org.apache.http.auth.AUTH;
 import org.apache.http.auth.AuthenticationException;
-import org.apache.http.auth.ContextAwareAuthScheme;
 import org.apache.http.auth.Credentials;
 import org.apache.http.auth.InvalidCredentialsException;
 import org.apache.http.auth.MalformedChallengeException;
 import org.apache.http.message.BasicHeader;
 import org.apache.http.protocol.ExecutionContext;
 import org.apache.http.protocol.HttpContext;
+import org.apache.http.util.CharArrayBuffer;
 import org.ietf.jgss.GSSContext;
 import org.ietf.jgss.GSSException;
 import org.ietf.jgss.GSSManager;
@@ -54,13 +53,15 @@ import org.ietf.jgss.Oid;
  *
  * @since 4.1
  */
-public class NegotiateScheme implements ContextAwareAuthScheme {
+public class NegotiateScheme extends AuthSchemeBase {
+
+    enum State {
+        UNINITIATED,
+        CHALLENGE_RECEIVED,
+        TOKEN_GENERATED,
+        FAILED,
+    }
     
-    private static final int UNINITIATED         = 0;
-    private static final int INITIATED           = 1;
-    private static final int NEGOTIATING         = 3;
-    private static final int ESTABLISHED         = 4;
-    private static final int FAILED              = Integer.MAX_VALUE;
     private static final String SPNEGO_OID       = "1.3.6.1.5.5.2";
     private static final String KERBEROS_OID     = "1.2.840.113554.1.2.2";
 
@@ -70,12 +71,10 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
 
     private final boolean stripPort;
 
-    private boolean proxy;
-
     private GSSContext gssContext = null;
 
     /** Authentication process state */
-    private int state;
+    private State state;
 
     /** base64 decoded challenge **/
     private byte[] token;
@@ -88,7 +87,7 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
      */
     public NegotiateScheme(final SpnegoTokenGenerator spengoGenerator, boolean stripPort) {
         super();
-        this.state = UNINITIATED;
+        this.state = State.UNINITIATED;
         this.spengoGenerator = spengoGenerator;
         this.stripPort = stripPort;
     }
@@ -109,7 +108,7 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
      *
      */
     public boolean isComplete() {
-        return this.state == ESTABLISHED || this.state == FAILED;
+        return this.state == State.TOKEN_GENERATED || this.state == State.FAILED;
     }
 
     /**
@@ -152,7 +151,7 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
         if (request == null) {
             throw new IllegalArgumentException("HTTP request may not be null");
         }
-        if (state == UNINITIATED) {
+        if (state != State.CHALLENGE_RECEIVED) {
             throw new IllegalStateException(
                     "Negotiation authentication process has not been initiated");
         }
@@ -227,19 +226,13 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
                 gssContext.requestMutualAuth(true);
                 gssContext.requestCredDeleg(true);
             }
-            state = INITIATED;
-
             if (token == null) {
                 token = new byte[0];                
             }
-            // HTTP 1.1 issue:
-            // Mutual auth will never complete to do 200 instead of 401 in
-            // return from server. "state" will never reach ESTABLISHED
-            // but it works anyway
-
             token = gssContext.initSecContext(token, 0, token.length);
             if (token == null) {
-                throw new AuthenticationException("Failed to initialize security context");
+                state = State.FAILED;
+                throw new AuthenticationException("GSS security context initialization failed");
             }
 
             /*
@@ -250,11 +243,14 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
                 token = spengoGenerator.generateSpnegoDERObject(token);
             }
 
+            state = State.TOKEN_GENERATED;
+            String tokenstr = new String(Base64.encodeBase64(token, false));
             if (log.isDebugEnabled()) {
-                log.debug("got token, sending " + token.length + " bytes to server");
+                log.debug("Sending response '" + tokenstr + "' back to the auth server");
             }
+            return new BasicHeader("Authorization", "Negotiate " + tokenstr);
         } catch (GSSException gsse) {
-            state = FAILED;
+            state = State.FAILED;
             if (gsse.getMajor() == GSSException.DEFECTIVE_CREDENTIAL
                     || gsse.getMajor() == GSSException.CREDENTIALS_EXPIRED)
                 throw new InvalidCredentialsException(gsse.getMessage(), gsse);
@@ -267,11 +263,9 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
             // other error
             throw new AuthenticationException(gsse.getMessage());
         } catch (IOException ex){
-            state = FAILED;
+            state = State.FAILED;
             throw new AuthenticationException(ex.getMessage());
         }
-        return new BasicHeader("Authorization", "Negotiate " +
-                new String(Base64.encodeBase64(token, false)) );
     }
 
 
@@ -312,41 +306,21 @@ public class NegotiateScheme implements ContextAwareAuthScheme {
         return true;
     }
 
-    /**
+    @Override
-     * Processes the Negotiate challenge.
+    protected void parseChallenge(
-     *
+            final CharArrayBuffer buffer, 
-     */
+            int beginIndex, int endIndex) throws MalformedChallengeException {
-    public void processChallenge(final Header header) throws MalformedChallengeException {
+        String challenge = buffer.substringTrimmed(beginIndex, endIndex);
         if (log.isDebugEnabled()) {
-            log.debug("Challenge header: " + header);
+            log.debug("Received challenge '" + challenge + "' from the auth server");
         }
-        String authheader = header.getName();
+        if (state == State.UNINITIATED) {
-        String challenge = header.getValue();
+            token = new Base64().decode(challenge.getBytes());
-        if (authheader.equalsIgnoreCase(AUTH.WWW_AUTH)) {
+            state = State.CHALLENGE_RECEIVED;
-            this.proxy = false;
-        } else if (authheader.equalsIgnoreCase(AUTH.PROXY_AUTH)) {
-            this.proxy = true;
         } else {
-            throw new MalformedChallengeException("Unexpected header name: " + authheader);
+            log.debug("Authentication already attempted");
-        }
+            state = State.FAILED;
-
-        if (challenge.startsWith("Negotiate")) {
-            if(isComplete() == false)
-                state = NEGOTIATING;
-
-            if (challenge.startsWith("Negotiate ")){
-                token = new Base64().decode(challenge.substring(10).getBytes());
-                if (log.isDebugEnabled()) {
-                    log.debug("challenge = " + challenge.substring(10));
-                }
-            } else {
-                token = new byte[0];
-            }
         }
     }
     
-    public boolean isProxy() {
-        return this.proxy;
-    }
-
 }

httpclient/src/test/java/org/apache/http/impl/auth/TestNegotiateScheme.java

@@ -26,17 +26,17 @@
  */
 package org.apache.http.impl.auth;
 
-import static org.mockito.Matchers.*;
-import static org.mockito.Mockito.*;
-
 import java.io.IOException;
 import java.security.Principal;
 
+import junit.framework.Assert;
+
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpException;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
+import org.apache.http.HttpStatus;
 import org.apache.http.auth.AuthScheme;
 import org.apache.http.auth.AuthScope;
 import org.apache.http.auth.Credentials;
@@ -44,8 +44,6 @@ import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.params.AuthPolicy;
 import org.apache.http.client.params.ClientPNames;
 import org.apache.http.entity.StringEntity;
-import org.apache.http.impl.auth.NegotiateScheme;
-import org.apache.http.impl.auth.NegotiateSchemeFactory;
 import org.apache.http.impl.client.DefaultHttpClient;
 import org.apache.http.localserver.BasicServerTestBase;
 import org.apache.http.localserver.LocalTestServer;
@@ -59,8 +57,9 @@ import org.ietf.jgss.GSSManager;
 import org.ietf.jgss.GSSName;
 import org.ietf.jgss.Oid;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
+import org.mockito.Matchers;
+import org.mockito.Mockito;
 
 /**
  * Tests for {@link NegotiateScheme}.
@@ -79,13 +78,15 @@ public class TestNegotiateScheme extends BasicServerTestBase {
      * This service will continue to ask for authentication.
      */
     private static class PleaseNegotiateService implements HttpRequestHandler {
-        public void handle(final HttpRequest request,
+
+        public void handle(
+                final HttpRequest request,
                 final HttpResponse response,
                 final HttpContext context) throws HttpException, IOException {
-            response.setStatusCode(401);
+            response.setStatusCode(HttpStatus.SC_UNAUTHORIZED);
             response.addHeader(new BasicHeader("WWW-Authenticate", "Negotiate blablabla"));
-            response.setEntity(new StringEntity("auth required "));
             response.addHeader(new BasicHeader("Connection", "Keep-Alive"));
+            response.setEntity(new StringEntity("auth required "));
         }
     }
 
@@ -96,29 +97,34 @@ public class TestNegotiateScheme extends BasicServerTestBase {
      *
      */
     private static class NegotiateSchemeWithMockGssManager extends NegotiateScheme {
-        GSSManager manager = mock(GSSManager.class);
+        
-        GSSName name = mock(GSSName.class);
+        GSSManager manager = Mockito.mock(GSSManager.class);
-        GSSContext context = mock(GSSContext.class);
+        GSSName name = Mockito.mock(GSSName.class);
+        GSSContext context = Mockito.mock(GSSContext.class);
 
         NegotiateSchemeWithMockGssManager() throws Exception {
             super(null, true);
-
+            Mockito.when(context.initSecContext(
-            when(context.initSecContext(any(byte[].class), anyInt(), anyInt()))
+                    Matchers.any(byte[].class), Matchers.anyInt(), Matchers.anyInt()))
                     .thenReturn("12345678".getBytes());
-            when(manager.createName(any(String.class), any(Oid.class)))
+            Mockito.when(manager.createName(
+                    Matchers.any(String.class), Matchers.any(Oid.class)))
                     .thenReturn(name);
-            when(manager.createContext(any(GSSName.class), any(Oid.class), any(GSSCredential.class), anyInt()))
+            Mockito.when(manager.createContext(
+                    Matchers.any(GSSName.class), Matchers.any(Oid.class), 
+                    Matchers.any(GSSCredential.class), Matchers.anyInt()))
                     .thenReturn(context);
-
         }
 
         @Override
         protected GSSManager getManager() {
             return manager;
         }
+        
     }
 
     private static class UseJaasCredentials implements Credentials {
+        
         public String getPassword() {
             return null;
         }
@@ -126,17 +132,22 @@ public class TestNegotiateScheme extends BasicServerTestBase {
         public Principal getUserPrincipal() {
             return null;
         }
+        
     }
 
     private static class NegotiateSchemeFactoryWithMockGssManager extends NegotiateSchemeFactory {
+        
         NegotiateSchemeWithMockGssManager scheme;
+        
         NegotiateSchemeFactoryWithMockGssManager() throws Exception {
             scheme = new NegotiateSchemeWithMockGssManager();
         }
+        
         @Override
         public AuthScheme newInstance(HttpParams params) {
             return scheme;
         }
+        
     }
 
     /**
@@ -144,7 +155,6 @@ public class TestNegotiateScheme extends BasicServerTestBase {
      * the server still keep asking for a valid ticket.
      */
     @Test
-    @Ignore
     public void testDontTryToAuthenticateEndlessly() throws Exception {
         int port = this.localServer.getServiceAddress().getPort();
         this.localServer.register("*", new PleaseNegotiateService());
@@ -163,25 +173,26 @@ public class TestNegotiateScheme extends BasicServerTestBase {
         HttpGet httpget = new HttpGet(s);
         HttpResponse response = client.execute(httpget);
         HttpEntity e = response.getEntity();
+        if (e != null) {
             e.consumeContent();
         }
-
+        Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getStatusLine().getStatusCode());
+    }
 
     /**
      * Javadoc specifies that {@link GSSContext#initSecContext(byte[], int, int)} can return null
      * if no token is generated. Client should be able to deal with this response.
-     *
      */
     @Test
-    @Ignore
+    public void testNoTokenGeneratedError() throws Exception {
-    public void testNoTokenGeneratedGenerateAnError() throws Exception {
         int port = this.localServer.getServiceAddress().getPort();
         this.localServer.register("*", new PleaseNegotiateService());
 
         HttpHost target = new HttpHost("localhost", port);
         DefaultHttpClient client = new DefaultHttpClient();
         NegotiateSchemeFactoryWithMockGssManager nsf = new NegotiateSchemeFactoryWithMockGssManager();
-        when(nsf.scheme.context.initSecContext(any(byte[].class), anyInt(), anyInt())).thenReturn(null);
+        Mockito.when(nsf.scheme.context.initSecContext(
+                Matchers.any(byte[].class), Matchers.anyInt(), Matchers.anyInt())).thenReturn(null);
         client.getAuthSchemes().register(AuthPolicy.SPNEGO, nsf);
 
         Credentials use_jaas_creds = new UseJaasCredentials();
@@ -193,7 +204,10 @@ public class TestNegotiateScheme extends BasicServerTestBase {
         HttpGet httpget = new HttpGet(s);
         HttpResponse response = client.execute(httpget);
         HttpEntity e = response.getEntity();
+        if (e != null) {
             e.consumeContent();
         }
+        Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getStatusLine().getStatusCode());
+    }
 
 }