Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-25144] Merge pull request #1427
  • Loading branch information
kohsuke committed Nov 7, 2014
2 parents 24c5e5e + 9e81b8e commit ada8432
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 38 deletions.
3 changes: 3 additions & 0 deletions changelog.html
Expand Up @@ -55,6 +55,9 @@
<!-- 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>)
Expand Down
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 null;

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
Expand Up @@ -56,10 +56,15 @@ public void testVariousWaysToCall() throws Exception {
// 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");
}
Expand Down

0 comments on commit ada8432

Please sign in to comment.