Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-25144] Revisiting the attempted fix in the previous co…
…mmit.

IIUC, the issue here is that the request in question contains both a
valid session cookie AND basic authentication header, and that path
results in a failure because BasicHeaderProcessor expects one of
BasicHeaderAuthenticators to validate the basic authentication header
(without knowing that there's already a valid Authentication object that
came from the HTTP session, yet no BasicHeaderAuthenticator actually
processes this because BasicHeaderRealPasswordAuthenticator backs away
from doing that.

I think the corect fix is for BasicHeaderRealPasswordAuthenticator to
get rid of authenticationIsRequired check. This check instead belongs to
BasicHeaderProcessor, where it should be used to check if any
BasicHeaderAuthenticator should be consulted or not.

The problem with having this logic in
BasicHeaderRealPasswordAuthenticator is that this is just an
implementation of an extension point, and thus it needs to be removable.
As it stands right now in this fix, if this impl is removed,
JENKINS-25144 will be back again.
  • Loading branch information
kohsuke committed Nov 7, 2014
1 parent 0176b6d commit 9e81b8e
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 37 deletions.
47 changes: 46 additions & 1 deletion 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;
Expand Down Expand Up @@ -67,6 +69,11 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
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);
Expand All @@ -87,6 +94,44 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
}
}

/**
* 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);

Expand Down
Expand Up @@ -19,9 +19,7 @@
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;

Expand Down Expand Up @@ -49,9 +47,6 @@ public Authentication authenticate(HttpServletRequest req, HttpServletResponse r
if (DISABLE)
return null;

if (!authenticationIsRequired(username))
return SecurityContextHolder.getContext().getAuthentication();

UsernamePasswordAuthenticationToken authRequest =
new UsernamePasswordAuthenticationToken(username, password);
authRequest.setDetails(authenticationDetailsSource.buildDetails(req));
Expand All @@ -68,37 +63,6 @@ public Authentication authenticate(HttpServletRequest req, HttpServletResponse r
}
}

// 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());

/**
Expand Down

0 comments on commit 9e81b8e

Please sign in to comment.