From 8886bea1fb463b8a6c5ade36c6a6850155b7543e Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Thu, 12 Apr 2012 14:36:11 -0700 Subject: [PATCH] fixed a concurrency bug in the impersonation code --- .../main/java/hudson/DependencyRunner.java | 7 +++---- .../java/hudson/model/AsyncAperiodicWork.java | 5 +++-- .../java/hudson/model/AsyncPeriodicWork.java | 5 +++-- .../java/hudson/model/DependencyGraph.java | 5 +---- core/src/main/java/hudson/model/Executor.java | 3 ++- .../main/java/hudson/model/UpdateCenter.java | 8 +++++--- core/src/main/java/hudson/security/ACL.java | 20 +++++++++++++++++++ .../java/hudson/triggers/SafeTimerTask.java | 6 +++--- core/src/main/java/jenkins/model/Jenkins.java | 13 ++++++------ .../java/jenkins/security/ApiTokenFilter.java | 6 ++++-- .../org/jvnet/hudson/test/HudsonTestCase.java | 3 ++- .../org/jvnet/hudson/test/JenkinsRule.java | 3 ++- .../hudson/model/DependencyGraphTest.java | 4 +++- 13 files changed, 58 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/hudson/DependencyRunner.java b/core/src/main/java/hudson/DependencyRunner.java index 449c7e0e73..bd4282f8c2 100644 --- a/core/src/main/java/hudson/DependencyRunner.java +++ b/core/src/main/java/hudson/DependencyRunner.java @@ -36,6 +36,7 @@ import java.util.Collection; import java.util.logging.Logger; import org.acegisecurity.Authentication; +import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; /** @@ -54,9 +55,7 @@ public class DependencyRunner implements Runnable { } public void run() { - Authentication saveAuth = SecurityContextHolder.getContext().getAuthentication(); - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); - + SecurityContext oldContext = ACL.impersonate(ACL.SYSTEM); try { Set topLevelProjects = new HashSet(); // Get all top-level projects @@ -74,7 +73,7 @@ public class DependencyRunner implements Runnable { runnable.run(p); } } finally { - SecurityContextHolder.getContext().setAuthentication(saveAuth); + SecurityContextHolder.setContext(oldContext); } } diff --git a/core/src/main/java/hudson/model/AsyncAperiodicWork.java b/core/src/main/java/hudson/model/AsyncAperiodicWork.java index fceb25a065..f46dd33b61 100644 --- a/core/src/main/java/hudson/model/AsyncAperiodicWork.java +++ b/core/src/main/java/hudson/model/AsyncAperiodicWork.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.util.logging.Level; import jenkins.model.Jenkins; +import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; /** @@ -70,8 +71,8 @@ public abstract class AsyncAperiodicWork extends AperiodicWork { StreamTaskListener l = createListener(); try { - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); - + ACL.impersonate(ACL.SYSTEM); + execute(l); } catch (IOException e) { e.printStackTrace(l.fatalError(e.getMessage())); diff --git a/core/src/main/java/hudson/model/AsyncPeriodicWork.java b/core/src/main/java/hudson/model/AsyncPeriodicWork.java index 2c89594643..52fdb148a1 100644 --- a/core/src/main/java/hudson/model/AsyncPeriodicWork.java +++ b/core/src/main/java/hudson/model/AsyncPeriodicWork.java @@ -3,6 +3,7 @@ package hudson.model; import hudson.security.ACL; import hudson.util.StreamTaskListener; import jenkins.model.Jenkins; +import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; import java.io.File; @@ -47,8 +48,8 @@ public abstract class AsyncPeriodicWork extends PeriodicWork { StreamTaskListener l = createListener(); try { - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); - + ACL.impersonate(ACL.SYSTEM); + execute(l); } catch (IOException e) { e.printStackTrace(l.fatalError(e.getMessage())); diff --git a/core/src/main/java/hudson/model/DependencyGraph.java b/core/src/main/java/hudson/model/DependencyGraph.java index c24ba0f96d..2b1a7db304 100644 --- a/core/src/main/java/hudson/model/DependencyGraph.java +++ b/core/src/main/java/hudson/model/DependencyGraph.java @@ -84,12 +84,9 @@ public class DependencyGraph implements Comparator { public void build() { // Set full privileges while computing to avoid missing any projects the current user cannot see. // Use setContext (NOT getContext().setAuthentication()) so we don't affect concurrent threads for same HttpSession. - SecurityContext saveCtx = SecurityContextHolder.getContext(); + SecurityContext saveCtx = ACL.impersonate(ACL.SYSTEM); try { this.computationalData = new HashMap, Object>(); - NotSerilizableSecurityContext system = new NotSerilizableSecurityContext(); - system.setAuthentication(ACL.SYSTEM); - SecurityContextHolder.setContext(system); for( AbstractProject p : getAllProjects() ) p.buildDependencyGraph(this); diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 36cc8bcbdf..1815a31db8 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -38,6 +38,7 @@ import hudson.security.ACL; import jenkins.model.InterruptedBuildAction; import jenkins.model.Jenkins; import org.acegisecurity.Authentication; +import org.acegisecurity.context.SecurityContext; import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpResponses; @@ -175,7 +176,7 @@ public class Executor extends Thread implements ModelObject { @Override public void run() { // run as the system user. see ACL.SYSTEM for more discussion about why this is somewhat broken - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); + ACL.impersonate(ACL.SYSTEM); try { finishTime = System.currentTimeMillis(); diff --git a/core/src/main/java/hudson/model/UpdateCenter.java b/core/src/main/java/hudson/model/UpdateCenter.java index 03e95315fc..7c3f779d24 100644 --- a/core/src/main/java/hudson/model/UpdateCenter.java +++ b/core/src/main/java/hudson/model/UpdateCenter.java @@ -48,6 +48,7 @@ import hudson.util.XStream2; import jenkins.RestartRequiredException; import jenkins.model.Jenkins; import org.acegisecurity.Authentication; +import org.acegisecurity.context.SecurityContext; import org.apache.commons.io.input.CountingInputStream; import org.apache.commons.io.output.NullOutputStream; import org.jvnet.localizer.Localizable; @@ -1121,13 +1122,14 @@ public class UpdateCenter extends AbstractModelObject implements Saveable { // if this is a bundled plugin, make sure it won't get overwritten PluginWrapper pw = plugin.getInstalled(); - if (pw!=null && pw.isBundled()) + if (pw!=null && pw.isBundled()) { + SecurityContext oldContext = ACL.impersonate(ACL.SYSTEM); try { - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); pw.doPin(); } finally { - SecurityContextHolder.clearContext(); + SecurityContextHolder.setContext(oldContext); } + } if (dynamicLoad) { try { diff --git a/core/src/main/java/hudson/security/ACL.java b/core/src/main/java/hudson/security/ACL.java index 56fa04c626..1d4e47992e 100644 --- a/core/src/main/java/hudson/security/ACL.java +++ b/core/src/main/java/hudson/security/ACL.java @@ -26,6 +26,8 @@ package hudson.security; import jenkins.model.Jenkins; import org.acegisecurity.AccessDeniedException; import org.acegisecurity.Authentication; +import org.acegisecurity.context.SecurityContext; +import org.acegisecurity.context.SecurityContextHolder; import org.acegisecurity.providers.UsernamePasswordAuthenticationToken; import org.acegisecurity.acls.sid.PrincipalSid; import org.acegisecurity.acls.sid.Sid; @@ -113,4 +115,22 @@ public abstract class ACL { * the user who triggered a build.) */ public static final Authentication SYSTEM = new UsernamePasswordAuthenticationToken("SYSTEM","SYSTEM"); + + /** + * Changes the {@link Authentication} associated with the current thread + * to the specified one, and returns the previous security context. + * + *

+ * When the impersonation is over, be sure to restore the previous authentication + * via {@code SecurityContextHolder.setContext(returnValueFromThisMethod)}. + * + *

+ * We need to create a new {@link SecurityContext} instead of {@link SecurityContext#setAuthentication(Authentication)} + * because the same {@link SecurityContext} object is reused for all the concurrent requests from the same session. + */ + public static SecurityContext impersonate(Authentication auth) { + SecurityContext old = SecurityContextHolder.getContext(); + SecurityContextHolder.setContext(new NotSerilizableSecurityContext(ACL.SYSTEM)); + return old; + } } diff --git a/core/src/main/java/hudson/triggers/SafeTimerTask.java b/core/src/main/java/hudson/triggers/SafeTimerTask.java index a331c4e089..49524fbec0 100644 --- a/core/src/main/java/hudson/triggers/SafeTimerTask.java +++ b/core/src/main/java/hudson/triggers/SafeTimerTask.java @@ -23,6 +23,7 @@ */ package hudson.triggers; +import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; import java.util.Timer; @@ -48,14 +49,13 @@ public abstract class SafeTimerTask extends TimerTask { public final void run() { // background activity gets system credential, // just like executors get it. - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); - + SecurityContext oldContext = ACL.impersonate(ACL.SYSTEM); try { doRun(); } catch(Throwable t) { LOGGER.log(Level.SEVERE, "Timer task "+this+" failed",t); } finally { - SecurityContextHolder.clearContext(); + SecurityContextHolder.setContext(oldContext); } } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index d9a4ef1e93..111ff4f8fc 100755 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -200,6 +200,7 @@ import org.acegisecurity.AcegiSecurityException; import org.acegisecurity.Authentication; import org.acegisecurity.GrantedAuthority; import org.acegisecurity.GrantedAuthorityImpl; +import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken; import org.acegisecurity.ui.AbstractProcessingFilter; @@ -702,7 +703,7 @@ public class Jenkins extends AbstractCIBase implements ModifiableItemGroup