Create a single point in which the basic authentication header is processed.

Previously, basic auth header was processed from two different servlet
filters in a single filter chain.

In case the 1st filter (ApiTokenFilter) manages to authenticate the
request, the 2nd filter (BasicProcessingFilter) tries to avoid
interpreting the API token as the password and failing authentication
(see BasicProcessingFilter.authenticationIsRequired), but the check
feels rather fragile.

Although I did eventually discover that the original problem (ZD-19640)
was not caused by this, I've already implemented & tested this change,
and this feels like a good work to be wasted, so I'm pushing this in
anyway.

Refrence: ZD-19640

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

 package jenkins.security;
 
-import hudson.model.User;
-import hudson.security.ACL;
-import hudson.util.Scrambler;
-import org.acegisecurity.context.SecurityContext;
-import org.acegisecurity.context.SecurityContextHolder;
-import org.acegisecurity.userdetails.UsernameNotFoundException;
-import org.springframework.dao.DataAccessException;
-
 import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import java.io.IOException;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-
-import static java.util.logging.Level.WARNING;
+import java.util.Collections;
+import java.util.List;
 
 /**
  * {@link Filter} that performs HTTP basic authentication based on API token.
@@ -31,56 +13,12 @@ import static java.util.logging.Level.WARNING;
  * interfere with the other.
  *
  * @author Kohsuke Kawaguchi
+ * @deprecated as of 1.576
+ *      Use {@link BasicHeaderProcessor}
  */
-public class ApiTokenFilter implements Filter {
-    public void init(FilterConfig filterConfig) throws ServletException {
-    }
-
-    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
-        HttpServletRequest req = (HttpServletRequest) request;
-        HttpServletResponse rsp = (HttpServletResponse) response;
-        String authorization = req.getHeader("Authorization");
-
-        if (authorization!=null) {
-            // authenticate the user
-            String uidpassword = Scrambler.descramble(authorization.substring(6));
-            int idx = uidpassword.indexOf(':');
-            if (idx >= 0) {
-                String username = uidpassword.substring(0, idx);
-                String password = uidpassword.substring(idx+1);
-
-                // attempt to authenticate as API token
-                User u = User.get(username);
-                ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
-                if (t!=null && t.matchesPassword(password)) {
-                    try {
-                        // even if we fail to match the password, we aren't rejecting it.
-                        // as the user might be passing in a real password.
-                        SecurityContext oldContext = ACL.impersonate(u.impersonate());
-                        try {
-                            request.setAttribute(ApiTokenProperty.class.getName(), u);
-                            chain.doFilter(request,response);
-                            return;
-                        } finally {
-                            SecurityContextHolder.setContext(oldContext);
-                        }
-                    } catch (UsernameNotFoundException x) {
-                        // The token was valid, but the impersonation failed. This token is clearly not his real password,
-                        // so there's no point in continuing the request processing. Report this error and abort.
-                        LOGGER.log(WARNING, "API token matched for user "+username+" but the impersonation failed",x);
-                        throw new ServletException(x);
-                    } catch (DataAccessException x) {
-                        throw new ServletException(x);
-                    }
-                }
-            }
-        }
-
-        chain.doFilter(request,response);
+public class ApiTokenFilter extends BasicHeaderProcessor {
+    @Override
+    protected List<? extends BasicHeaderAuthenticator> all() {
+        return Collections.singletonList(new BasicHeaderApiTokenAuthenticator());
     }
-
-    public void destroy() {
-    }
-
-    private static final Logger LOGGER = Logger.getLogger(ApiTokenFilter.class.getName());
 }

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

+package jenkins.security;
+
+import hudson.Extension;
+import hudson.model.User;
+import org.acegisecurity.Authentication;
+import org.acegisecurity.userdetails.UsernameNotFoundException;
+import org.springframework.dao.DataAccessException;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.util.logging.Logger;
+
+import static java.util.logging.Level.*;
+
+/**
+ * Checks if the password given in the BASIC header matches the user's API token.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.576
+ */
+@Extension
+public class BasicHeaderApiTokenAuthenticator extends BasicHeaderAuthenticator {
+    @Override
+    public Authentication authenticate(HttpServletRequest req, HttpServletResponse rsp, String username, String password) throws ServletException {
+        // attempt to authenticate as API token
+        User u = User.get(username);
+        ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
+        if (t!=null && t.matchesPassword(password)) {
+            try {
+                return u.impersonate();
+            } catch (UsernameNotFoundException x) {
+                // The token was valid, but the impersonation failed. This token is clearly not his real password,
+                // so there's no point in continuing the request processing. Report this error and abort.
+                LOGGER.log(WARNING, "API token matched for user "+username+" but the impersonation failed",x);
+                throw new ServletException(x);
+            } catch (DataAccessException x) {
+                throw new ServletException(x);
+            }
+        }
+
+        return null;
+    }
+
+    private static final Logger LOGGER = Logger.getLogger(BasicHeaderApiTokenAuthenticator.class.getName());
+}

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

+package jenkins.security;
+
+import hudson.ExtensionList;
+import hudson.ExtensionPoint;
+import jenkins.model.Jenkins;
+import org.acegisecurity.Authentication;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * When Jenkins receives HTTP basic authentication, this hook will validate the username/password
+ * pair.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.576
+ * @see BasicHeaderProcessor
+ */
+public abstract class BasicHeaderAuthenticator implements ExtensionPoint {
+    /**
+     * Given the parsed username and password field from the basic authentication header,
+     * determine the effective security credential to process the request with.
+     *
+     * <p>
+     * The method must return null if the password or username didn't match what's expected.
+     * When null is returned, other authenticators will get a chance to process the request.
+     * This is necessary because Jenkins accepts both real password as well as API tokens for the password.
+     *
+     * <p>
+     * In contrast, when an exception is thrown the request processing will fail
+     * immediately without providing a chance for other authenticators to process the request.
+     *
+     * <p>
+     * When no processor can validate the username/password pair, caller will make
+     * the request processing fail.
+     */
+    public abstract Authentication authenticate(HttpServletRequest req, HttpServletResponse rsp, String username, String password) throws IOException, ServletException;
+
+    public static ExtensionList<BasicHeaderAuthenticator> all() {
+        return Jenkins.getInstance().getExtensionList(BasicHeaderAuthenticator.class);
+    }
+}

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

+package jenkins.security;
+
+import hudson.security.ACL;
+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.ui.AuthenticationEntryPoint;
+import org.acegisecurity.ui.rememberme.NullRememberMeServices;
+import org.acegisecurity.ui.rememberme.RememberMeServices;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.List;
+import java.util.logging.Logger;
+
+import static java.util.logging.Level.*;
+
+/**
+ * Takes "username:password" given in the <tt>Authorization</tt> HTTP header and authenticates
+ * the request.
+ *
+ * <p>
+ * Implementations of {@link BasicHeaderAuthenticator} includes one that accepts the real password,
+ * then one that checks the user's API token. We call them all from a single Filter like this,
+ * as opposed to using a list of {@link Filter}s, so that multiple filters don't end up trying
+ * to authenticate the same header differently and fail.
+ *
+ * @author Kohsuke Kawaguchi
+ * @see ZD-19640
+ */
+public class BasicHeaderProcessor implements Filter {
+    // these fields are supposed to be injected by Spring
+    private AuthenticationEntryPoint authenticationEntryPoint;
+    private RememberMeServices rememberMeServices = new NullRememberMeServices();
+
+    public void init(FilterConfig filterConfig) throws ServletException {
+    }
+
+    public void setAuthenticationEntryPoint(AuthenticationEntryPoint authenticationEntryPoint) {
+        this.authenticationEntryPoint = authenticationEntryPoint;
+    }
+
+    public void setRememberMeServices(RememberMeServices rememberMeServices) {
+        this.rememberMeServices = rememberMeServices;
+    }
+
+    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
+        HttpServletRequest req = (HttpServletRequest) request;
+        HttpServletResponse rsp = (HttpServletResponse) response;
+        String authorization = req.getHeader("Authorization");
+
+        if (authorization!=null && authorization.startsWith("Basic ")) {
+            // authenticate the user
+            String uidpassword = Scrambler.descramble(authorization.substring(6));
+            int idx = uidpassword.indexOf(':');
+            if (idx >= 0) {
+                String username = uidpassword.substring(0, idx);
+                String password = uidpassword.substring(idx+1);
+
+                for (BasicHeaderAuthenticator a : all()) {
+                    LOGGER.log(FINER, "Attempting to authenticate with {0}", a);
+                    Authentication auth = a.authenticate(req, rsp, username, password);
+                    if (auth!=null) {
+                        LOGGER.log(FINE, "Request authenticated as {0} by {1}", new Object[]{auth,a});
+                        success(req, rsp, chain, auth);
+                        return;
+                    }
+                }
+
+                fail(req, rsp, new BadCredentialsException("Invalid password/token for user: " + username));
+            } else {
+                fail(req, rsp, new BadCredentialsException("Malformed HTTP basic Authorization header"));
+            }
+        } else {
+            // not something we care
+            chain.doFilter(request, response);
+        }
+    }
+
+    protected void success(HttpServletRequest req, HttpServletResponse rsp, FilterChain chain, Authentication auth) throws IOException, ServletException {
+        rememberMeServices.loginSuccess(req, rsp, auth);
+
+        SecurityContext old = ACL.impersonate(auth);
+        try {
+            chain.doFilter(req,rsp);
+        } finally {
+            SecurityContextHolder.setContext(old);
+        }
+    }
+
+    protected void fail(HttpServletRequest req, HttpServletResponse rsp, BadCredentialsException failure) throws IOException, ServletException {
+        LOGGER.log(FINE, "Authentication of BASIC header failed");
+
+        rememberMeServices.loginFail(req, rsp);
+
+        authenticationEntryPoint.commence(req, rsp, failure);
+    }
+
+    protected List<? extends BasicHeaderAuthenticator> all() {
+        return BasicHeaderAuthenticator.all();
+    }
+
+    public void destroy() {
+    }
+
+    private static final Logger LOGGER = Logger.getLogger(ApiTokenFilter.class.getName());
+}

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

+/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package jenkins.security;
+
+import hudson.Extension;
+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;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.logging.Logger;
+
+import static java.util.logging.Level.*;
+
+/**
+ * Checks if the password given in the BASIC header matches the user's actual password,
+ * as opposed to other pseudo-passwords like API tokens.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.576
+ */
+@Extension
+public class BasicHeaderRealPasswordAuthenticator extends BasicHeaderAuthenticator {
+    private AuthenticationDetailsSource authenticationDetailsSource = new AuthenticationDetailsSourceImpl();
+
+    @Override
+    public Authentication authenticate(HttpServletRequest req, HttpServletResponse rsp, String username, String password) throws IOException, ServletException {
+        if (DISABLE)
+            return null;
+
+        if (!authenticationIsRequired(username))
+            return null;
+
+        UsernamePasswordAuthenticationToken authRequest =
+                new UsernamePasswordAuthenticationToken(username, password);
+        authRequest.setDetails(authenticationDetailsSource.buildDetails(req));
+
+        try {
+            Authentication a = Jenkins.getInstance().getSecurityRealm().getSecurityComponents().manager.authenticate(authRequest);
+            // Authentication success
+            LOGGER.log(FINER, "Authentication success: {0}", a);
+            return a;
+        } catch (AuthenticationException failed) {
+            // Authentication failed
+            LOGGER.log(FINER, "Authentication request for user: {0} failed: {1}", new Object[]{username,failed});
+            return null;
+        }
+    }
+
+    // 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());
+
+    /**
+     * Legacy property to disable the real password support.
+     * Now that this is an extension, {@link ExtensionFilter} is a better way to control this.
+     */
+    public static boolean DISABLE = Boolean.getBoolean("jenkins.security.ignoreBasicAuth");
+}

test/pom.xml

@@ -43,6 +43,7 @@ THE SOFTWARE.
     <concurrency>2</concurrency> <!-- may use e.g. 2C for 2 × (number of cores) -->
     <mavenDebug>false</mavenDebug>
     <ignore.random.failures>false</ignore.random.failures>
+    <jacocoSurefireArgs></jacocoSurefireArgs><!-- empty by default -->
   </properties>
 
   <dependencies>
@@ -192,7 +193,7 @@ THE SOFTWARE.
         <artifactId>maven-surefire-plugin</artifactId>
         <!-- version specified in grandparent pom -->
         <configuration>
-          <argLine>-Dfile.encoding=UTF-8 -Xmx256m -XX:MaxPermSize=128m</argLine>
+          <argLine>${jacocoSurefireArgs} -Dfile.encoding=UTF-8 -Xmx256m -XX:MaxPermSize=128m</argLine>
           <systemPropertyVariables>
               <!-- use AntClassLoader that supports predictable file handle release -->
               <hudson.ClassicPluginStrategy.useAntClassLoader>true</hudson.ClassicPluginStrategy.useAntClassLoader>
@@ -303,5 +304,51 @@ THE SOFTWARE.
         </plugins>
       </build>
     </profile>    
+    <profile>
+      <id>jacoco</id>
+      <build>
+        <plugins>
+          <plugin>
+            <groupId>org.jacoco</groupId>
+            <artifactId>jacoco-maven-plugin</artifactId>
+            <version>0.6.3.201306030806</version>
+            <executions>
+              <!--
+                Prepares the property pointing to the JaCoCo runtime agent which
+                is passed as VM argument when Maven the Surefire plugin is executed.
+              -->
+              <execution>
+                <id>pre-unit-test</id>
+                <goals>
+                  <goal>prepare-agent</goal>
+                </goals>
+                <configuration>
+                  <!-- Sets the path to the file which contains the execution data. -->
+                  <destFile>${project.build.directory}/coverage-reports/jacoco-ut.exec</destFile>
+                  <propertyName>jacocoSurefireArgs</propertyName>
+                </configuration>
+              </execution>
+              <!--
+                Ensures that the code coverage report for unit tests is created after
+                unit tests have been run.
+              -->
+              <execution>
+                <id>post-unit-test</id>
+                <phase>test</phase>
+                <goals>
+                  <goal>report</goal>
+                </goals>
+                <configuration>
+                  <!-- Sets the path to the file which contains the execution data. -->
+                  <dataFile>${project.build.directory}/coverage-reports/jacoco-ut.exec</dataFile>
+                  <!-- Sets the output directory for the code coverage report. -->
+                  <outputDirectory>${project.reporting.outputDirectory}/jacoco-ut</outputDirectory>
+                </configuration>
+              </execution>
+            </executions>
+          </plugin>
+        </plugins>
+      </build>
+    </profile>
   </profiles>
 </project>

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

+package jenkins.security;
+
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.Page;
+import com.gargoylesoftware.htmlunit.WebRequestSettings;
+import hudson.model.UnprotectedRootAction;
+import hudson.model.User;
+import hudson.util.HttpResponses;
+import hudson.util.Scrambler;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.JenkinsRule.WebClient;
+import org.jvnet.hudson.test.TestExtension;
+import org.kohsuke.stapler.HttpResponse;
+import org.xml.sax.SAXException;
+
+import java.io.IOException;
+import java.net.URL;
+
+/**
+ * @author Kohsuke Kawaguchi
+ */
+public class BasicHeaderProcessorTest extends Assert {
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    private WebClient wc;
+
+    /**
+     * Tests various ways to send the Basic auth.
+     */
+    @Test
+    public void testVariousWaysToCall() throws Exception {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+        User foo = User.get("foo");
+        User bar = User.get("bar");
+
+        wc = j.createWebClient();
+
+        // call without authentication
+        makeRequestWithAuthAndVerify(null, "anonymous");
+
+        // call with API token
+        ApiTokenProperty t = foo.getProperty(ApiTokenProperty.class);
+        final String token = t.getApiToken();
+        makeRequestWithAuthAndVerify("foo:"+token, "foo");
+
+        // call with invalid API token
+        makeRequestAndFail("foo:abcd"+token);
+
+        // call with password
+        makeRequestWithAuthAndVerify("foo:foo", "foo");
+
+        // call with incorrect password
+        makeRequestAndFail("foo:bar");
+
+        // if the session cookie is valid, then basic header won't be needed
+        wc.login("bar");
+        makeRequestWithAuthAndVerify(null, "bar");
+
+        // but if the password is incorrect, it should fail, instead of silently logging in as the user indicated by session
+        makeRequestAndFail("foo:bar");
+    }
+
+    private void makeRequestAndFail(String userAndPass) throws IOException, SAXException {
+        try {
+            makeRequestWithAuthAndVerify(userAndPass, "-");
+            fail();
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(401, e.getStatusCode());
+        }
+    }
+
+    private void makeRequestWithAuthAndVerify(String userAndPass, String username) throws IOException, SAXException {
+        WebRequestSettings req = new WebRequestSettings(new URL(j.getURL(),"test"));
+        if (userAndPass!=null)
+            req.setAdditionalHeader("Authorization","Basic "+Scrambler.scramble(userAndPass));
+        Page p = wc.getPage(req);
+
+        assertEquals(username, p.getWebResponse().getContentAsString().trim());
+    }
+
+    @TestExtension
+    public static class WhoAmI implements UnprotectedRootAction {
+        @Override
+        public String getIconFileName() {
+            return null;
+        }
+
+        @Override
+        public String getDisplayName() {
+            return null;
+        }
+
+        @Override
+        public String getUrlName() {
+            return "test";
+        }
+
+        public HttpResponse doIndex() {
+            User u = User.current();
+            return HttpResponses.plainText(u!=null ? u.getId() : "anonymous");
+        }
+    }
+}

war/src/main/webapp/WEB-INF/security/SecurityFilters.groovy

@@ -32,6 +32,7 @@ import hudson.security.BasicAuthenticationFilter
 import hudson.security.ChainedServletFilter
 import hudson.security.UnwrapSecurityExceptionFilter
 import hudson.security.HudsonAuthenticationEntryPoint
+import jenkins.security.BasicHeaderProcessor
 import org.acegisecurity.providers.anonymous.AnonymousProcessingFilter
 import jenkins.security.ExceptionTranslationFilter
 import org.acegisecurity.ui.basicauth.BasicProcessingFilter
@@ -70,12 +71,8 @@ filter(ChainedServletFilter) {
             // Instead, we use layout.jelly to create sessions.
             allowSessionCreation = false;
         },
-        bean(ApiTokenFilter),
-        // allow clients to submit basic authentication credential
-        // but allow that to be skipped since it can interfere with reverse proxy setup
-        Boolean.getBoolean("jenkins.security.ignoreBasicAuth") ? bean(NoopFilter) :
-        bean(BasicProcessingFilter) {
-            authenticationManager = securityComponents.manager
+        // if any "Authorization: Basic xxx:yyy" is sent this is the filter that processes it
+        bean(BasicHeaderProcessor) {
             // if basic authentication fails (which only happens incorrect basic auth credential is sent),
             // respond with 401 with basic auth request, instead of redirecting the user to the login page,
             // since users of basic auth tends to be a program and won't see the redirection to the form