Skip to content
Permalink
Browse files

[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.

(cherry picked from commit 9e81b8e)
  • Loading branch information...
kohsuke authored and olivergondza committed Nov 7, 2014
1 parent 7169a39 commit be34b675f0f9cb59a67c09dbe42364d34c3eaff1
@@ -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 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);
@@ -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);

@@ -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;

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

/**

0 comments on commit be34b67

Please sign in to comment.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.