diff --git a/changelog.html b/changelog.html
index a4e3424559..83dedd98c9 100644
--- a/changelog.html
+++ b/changelog.html
@@ -55,6 +55,9 @@ Upcoming changes
+ -
+ Basic Authentication in combination with Session is broken
+ (issue 25144)
-
Some plugins broken since 1.584 if they expected certain events to be fired under a specific user ID.
(issue 25400)
diff --git a/core/src/main/java/jenkins/security/BasicHeaderProcessor.java b/core/src/main/java/jenkins/security/BasicHeaderProcessor.java
index 60d217a67e..6bec1a2e4a 100644
--- a/core/src/main/java/jenkins/security/BasicHeaderProcessor.java
+++ b/core/src/main/java/jenkins/security/BasicHeaderProcessor.java
@@ -1,12 +1,14 @@
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);
diff --git a/core/src/main/java/jenkins/security/BasicHeaderRealPasswordAuthenticator.java b/core/src/main/java/jenkins/security/BasicHeaderRealPasswordAuthenticator.java
index 4d3e5554e0..3b41c006dd 100644
--- a/core/src/main/java/jenkins/security/BasicHeaderRealPasswordAuthenticator.java
+++ b/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());
/**
diff --git a/test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java b/test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java
index 803767f9a4..c11726fd93 100644
--- a/test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java
+++ b/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");
}