[JENKINS-25144] Merge pull request #1427

changelog.html

@@ -55,6 +55,9 @@ Upcoming changes</a>
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class=bug>
+    Basic Authentication in combination with Session is broken
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-25144">issue 25144</a>)
   <li class=bug>
     Some plugins broken since 1.584 if they expected certain events to be fired under a specific user ID.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-25400">issue 25400</a>)

core/src/main/java/jenkins/security/BasicHeaderProcessor.java

 package jenkins.security;
 
 import hudson.security.ACL;
+import hudson.security.SecurityRealm;
 import hudson.util.Scrambler;
 import org.acegisecurity.Authentication;
-import org.acegisecurity.AuthenticationManager;
 import org.acegisecurity.BadCredentialsException;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;
+import org.acegisecurity.providers.UsernamePasswordAuthenticationToken;
+import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
 import org.acegisecurity.ui.AuthenticationEntryPoint;
 import org.acegisecurity.ui.rememberme.NullRememberMeServices;
 import org.acegisecurity.ui.rememberme.RememberMeServices;
@@ -67,6 +69,11 @@ public class BasicHeaderProcessor implements Filter {
                 String username = uidpassword.substring(0, idx);
                 String password = uidpassword.substring(idx+1);
 
+                if (!authenticationIsRequired(username)) {
+                    chain.doFilter(request, response);
+                    return;
+                }
+
                 for (BasicHeaderAuthenticator a : all()) {
                     LOGGER.log(FINER, "Attempting to authenticate with {0}", a);
                     Authentication auth = a.authenticate(req, rsp, username, password);
@@ -87,6 +94,44 @@ public class BasicHeaderProcessor implements Filter {
         }
     }
 
+    /**
+     * If the request is already authenticated to the same user that the Authorization header claims,
+     * for example through the HTTP session, then there's no need to re-authenticate the Authorization header,
+     * so we skip that. This avoids stressing {@link SecurityRealm}.
+     *
+     * This method returns false if we can take this short-cut.
+     */
+    // taken from BasicProcessingFilter.java
+    protected boolean authenticationIsRequired(String username) {
+        // Only reauthenticate if username doesn't match SecurityContextHolder and user isn't authenticated
+        // (see SEC-53)
+        Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication();
+
+        if(existingAuth == null || !existingAuth.isAuthenticated()) {
+            return true;
+        }
+
+        // Limit username comparison to providers which use usernames (ie UsernamePasswordAuthenticationToken)
+        // (see SEC-348)
+
+        if (existingAuth instanceof UsernamePasswordAuthenticationToken && !existingAuth.getName().equals(username)) {
+            return true;
+        }
+
+        // Handle unusual condition where an AnonymousAuthenticationToken is already present
+        // This shouldn't happen very often, as BasicProcessingFitler is meant to be earlier in the filter
+        // chain than AnonymousProcessingFilter. Nevertheless, presence of both an AnonymousAuthenticationToken
+        // together with a BASIC authentication request header should indicate reauthentication using the
+        // BASIC protocol is desirable. This behaviour is also consistent with that provided by form and digest,
+        // both of which force re-authentication if the respective header is detected (and in doing so replace
+        // any existing AnonymousAuthenticationToken). See SEC-610.
+        if (existingAuth instanceof AnonymousAuthenticationToken) {
+            return true;
+        }
+
+        return false;
+    }
+
     protected void success(HttpServletRequest req, HttpServletResponse rsp, FilterChain chain, Authentication auth) throws IOException, ServletException {
         rememberMeServices.loginSuccess(req, rsp, auth);
 

core/src/main/java/jenkins/security/BasicHeaderRealPasswordAuthenticator.java

@@ -19,9 +19,7 @@ import jenkins.ExtensionFilter;
 import jenkins.model.Jenkins;
 import org.acegisecurity.Authentication;
 import org.acegisecurity.AuthenticationException;
-import org.acegisecurity.context.SecurityContextHolder;
 import org.acegisecurity.providers.UsernamePasswordAuthenticationToken;
-import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
 import org.acegisecurity.ui.AuthenticationDetailsSource;
 import org.acegisecurity.ui.AuthenticationDetailsSourceImpl;
 
@@ -49,9 +47,6 @@ public class BasicHeaderRealPasswordAuthenticator extends BasicHeaderAuthenticat
         if (DISABLE)
             return null;
 
-        if (!authenticationIsRequired(username))
-            return null;
-
         UsernamePasswordAuthenticationToken authRequest =
                 new UsernamePasswordAuthenticationToken(username, password);
         authRequest.setDetails(authenticationDetailsSource.buildDetails(req));
@@ -68,37 +63,6 @@ public class BasicHeaderRealPasswordAuthenticator extends BasicHeaderAuthenticat
         }
     }
 
-    // taken from BasicProcessingFilter.java
-    protected boolean authenticationIsRequired(String username) {
-        // Only reauthenticate if username doesn't match SecurityContextHolder and user isn't authenticated
-        // (see SEC-53)
-        Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication();
-
-        if(existingAuth == null || !existingAuth.isAuthenticated()) {
-            return true;
-        }
-
-        // Limit username comparison to providers which use usernames (ie UsernamePasswordAuthenticationToken)
-        // (see SEC-348)
-
-        if (existingAuth instanceof UsernamePasswordAuthenticationToken && !existingAuth.getName().equals(username)) {
-            return true;
-        }
-
-        // Handle unusual condition where an AnonymousAuthenticationToken is already present
-        // This shouldn't happen very often, as BasicProcessingFitler is meant to be earlier in the filter
-        // chain than AnonymousProcessingFilter. Nevertheless, presence of both an AnonymousAuthenticationToken
-        // together with a BASIC authentication request header should indicate reauthentication using the
-        // BASIC protocol is desirable. This behaviour is also consistent with that provided by form and digest,
-        // both of which force re-authentication if the respective header is detected (and in doing so replace
-        // any existing AnonymousAuthenticationToken). See SEC-610.
-        if (existingAuth instanceof AnonymousAuthenticationToken) {
-            return true;
-        }
-
-        return false;
-    }
-
     private static final Logger LOGGER = Logger.getLogger(BasicHeaderRealPasswordAuthenticator.class.getName());
 
     /**

test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java

@@ -56,10 +56,15 @@ public class BasicHeaderProcessorTest extends Assert {
         // call with incorrect password
         makeRequestAndFail("foo:bar");
 
-        // if the session cookie is valid, then basic header won't be needed
+
         wc.login("bar");
+
+        // if the session cookie is valid, then basic header won't be needed
         makeRequestWithAuthAndVerify(null, "bar");
 
+        // if the session cookie is valid, and basic header is set anyway login should not fail either
+        makeRequestWithAuthAndVerify("bar:bar", "bar");
+
         // but if the password is incorrect, it should fail, instead of silently logging in as the user indicated by session
         makeRequestAndFail("foo:bar");
     }