• Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Critical Critical
    • _unsorted
    • None
    • ASF Hudson installation, Hudson 1.372, Tomcat 6, running against LDAP

      In our Hudson installation, user are authenticated using LDAP. Regular users gets assigned what we call job admin (using a group for this purpose in LDAP), meaning they can administer jobs, but not access the Manage Hudson pieces. However, several users have reported that the intermittently get elevated access, being able to access Manage Hudson. When this happens, the user name in the upper right corner will say "SYSTEM". This happens on refreshing the Hudson web GUI. Users have tried logging out and login using the regular user, which have gotten them back into their expected access rights.

      Marking this as critical as it gives users elevated access.

      Let me know if there is anything further I can assist in, for example involving our LDAP setup.

          [JENKINS-7256] Users intermittently gets SYSTEM user

          protocol7b added a comment -

          Given the behavior we're seeing with this happening on project configuration saving, an often when the save times-out, you're description seems likely. Given this is a frequent problem for our ASF installation, having a fix for this would be most appreciated.

          protocol7b added a comment - Given the behavior we're seeing with this happening on project configuration saving, an often when the save times-out, you're description seems likely. Given this is a frequent problem for our ASF installation, having a fix for this would be most appreciated.

          mdillon added a comment - - edited

          Mindless- that code is setting a value in a ThreadLocal; how would another thread see it?

          To be more clear, the call to SecurityContextHolder.getContext() gets the SecurityContext for the current thread based on a ThreadLocal inside the holder.

          mdillon added a comment - - edited Mindless- that code is setting a value in a ThreadLocal; how would another thread see it? To be more clear, the call to SecurityContextHolder.getContext() gets the SecurityContext for the current thread based on a ThreadLocal inside the holder.

          Alan Harder added a comment -

          mdillon-- I haven't dug deep yet.. that just looked like a likely candidate from a quick scan over code setting SYSTEM ACL. protocol7b, do you have any setting for "acegi.security.strategy" system propery in your hudson jvm?

          Alan Harder added a comment - mdillon-- I haven't dug deep yet.. that just looked like a likely candidate from a quick scan over code setting SYSTEM ACL. protocol7b, do you have any setting for "acegi.security.strategy" system propery in your hudson jvm?

          mdillon added a comment -

          That was the only place I noticed that could conceivably leave the SYSTEM ACL on a UI request thread as well, but it seems like that particular code couldn't possibly leave a thread in that state (assuming "acegi.security.strategy" was still set to the default threadlocal strategy). Quite a mystery...

          Other useful data points to see would be:

          1. Does this happen with later Hudson versions?
          2. Which plugins are installed? (in case they are misusing ACL.SYSTEM in some way)

          mdillon added a comment - That was the only place I noticed that could conceivably leave the SYSTEM ACL on a UI request thread as well, but it seems like that particular code couldn't possibly leave a thread in that state (assuming "acegi.security.strategy" was still set to the default threadlocal strategy). Quite a mystery... Other useful data points to see would be: 1. Does this happen with later Hudson versions? 2. Which plugins are installed? (in case they are misusing ACL.SYSTEM in some way)

          Alan Harder added a comment -

          mdillon-- another possibly relevant tidbit:
          From HttpSessionContextIntegrationFilter.cloneFromHttpSession..

          The default is to simply reference (ie the default is false). The default may cause issues if concurrent threads need to have a different security identity from other threads being concurrently processed that share the same HttpSession.

          Like ThreadLocal, doesn't seem like it should be a problem, but still digging..

          Alan Harder added a comment - mdillon-- another possibly relevant tidbit: From HttpSessionContextIntegrationFilter.cloneFromHttpSession .. The default is to simply reference (ie the default is false). The default may cause issues if concurrent threads need to have a different security identity from other threads being concurrently processed that share the same HttpSession. Like ThreadLocal, doesn't seem like it should be a problem, but still digging..

          Alan Harder added a comment -

          mdillon-- ya, this code really is the problem.
          ThreadLocal ensures one thread won't get a value that was set by another thread.. however, multiple threads can conceivably set() the same object.. in this case, HttpSessionContextIntegrationFilter gets the SessionContext object from the HttpSession and sets it in the ThreadLocal for all requests of this thread.
          Since the DependencyGraph code calls a method ON that object (setAuthentication), rather than doing setContext (putting a new object in the ThreadLocal), it does affect other threads.. does this sound right?

          Alan Harder added a comment - mdillon-- ya, this code really is the problem. ThreadLocal ensures one thread won't get a value that was set by another thread.. however, multiple threads can conceivably set() the same object.. in this case, HttpSessionContextIntegrationFilter gets the SessionContext object from the HttpSession and sets it in the ThreadLocal for all requests of this thread. Since the DependencyGraph code calls a method ON that object (setAuthentication), rather than doing setContext (putting a new object in the ThreadLocal), it does affect other threads.. does this sound right?

          Alan Harder added a comment -

          mdillon-- proposed fix..

          --- core/src/main/java/hudson/model/DependencyGraph.java	(revision 36205)
          +++ core/src/main/java/hudson/model/DependencyGraph.java	(working copy)
          @@ -25,7 +25,8 @@
           package hudson.model;
           
           import hudson.security.ACL;
          -import org.acegisecurity.Authentication;
          +import hudson.security.NotSerilizableSecurityContext;
          +import org.acegisecurity.context.SecurityContext;
           import org.acegisecurity.context.SecurityContextHolder;
           import org.kohsuke.stapler.StaplerRequest;
           import org.kohsuke.stapler.StaplerResponse;
          @@ -92,10 +93,13 @@
                * Builds the dependency graph.
                */
               public DependencyGraph() {
          -        // Set full privileges while computing to avoid missing any projects the current user cannot see
          -        Authentication saveAuth = SecurityContextHolder.getContext().getAuthentication();
          +        // 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();
                   try {
          -            SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM);
          +            NotSerilizableSecurityContext system = new NotSerilizableSecurityContext();
          +            system.setAuthentication(ACL.SYSTEM);
          +            SecurityContextHolder.setContext(system);
                       for( AbstractProject p : Hudson.getInstance().getAllItems(AbstractProject.class) )
                           p.buildDependencyGraph(this);
           
          @@ -104,7 +108,7 @@
           
                       built = true;
                   } finally {
          -            SecurityContextHolder.getContext().setAuthentication(saveAuth);
          +            SecurityContextHolder.setContext(saveCtx);
                   }
               }
          
          

          Alan Harder added a comment - mdillon-- proposed fix.. --- core/src/main/java/hudson/model/DependencyGraph.java (revision 36205) +++ core/src/main/java/hudson/model/DependencyGraph.java (working copy) @@ -25,7 +25,8 @@ package hudson.model; import hudson.security.ACL; - import org.acegisecurity.Authentication; + import hudson.security.NotSerilizableSecurityContext; + import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; @@ -92,10 +93,13 @@ * Builds the dependency graph. */ public DependencyGraph() { - // Set full privileges while computing to avoid missing any projects the current user cannot see - Authentication saveAuth = SecurityContextHolder.getContext().getAuthentication(); + // 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(); try { - SecurityContextHolder.getContext().setAuthentication(ACL.SYSTEM); + NotSerilizableSecurityContext system = new NotSerilizableSecurityContext(); + system.setAuthentication(ACL.SYSTEM); + SecurityContextHolder.setContext(system); for ( AbstractProject p : Hudson.getInstance().getAllItems(AbstractProject.class) ) p.buildDependencyGraph( this ); @@ -104,7 +108,7 @@ built = true ; } finally { - SecurityContextHolder.getContext().setAuthentication(saveAuth); + SecurityContextHolder.setContext(saveCtx); } }

          mdillon added a comment -

          Seeing the hudson.security.HttpSessionContextIntegrationFilter2 class, it isn't clear to me whether a shared security context in the HttpSession is the problem here. That being said, this seems like a straightforward way to ensure that change in the SecurityContext's authentication has no effect outside of the DependencyGraph constructor or that one thread calling the constructor.

          It seems like in general this part of Hudson's internal security could use some hardening. Something akin to java.security.PrivilegedAction and java.security.auth.Subject.doAsPrivileged instead of directly mucking about with setting ACL.SYSTEM in a possibly error-prone way.

          mdillon added a comment - Seeing the hudson.security.HttpSessionContextIntegrationFilter2 class, it isn't clear to me whether a shared security context in the HttpSession is the problem here. That being said, this seems like a straightforward way to ensure that change in the SecurityContext's authentication has no effect outside of the DependencyGraph constructor or that one thread calling the constructor. It seems like in general this part of Hudson's internal security could use some hardening. Something akin to java.security.PrivilegedAction and java.security.auth.Subject.doAsPrivileged instead of directly mucking about with setting ACL.SYSTEM in a possibly error-prone way.

          Code changed in hudson
          User: : mindless
          Path:
          trunk/hudson/main/core/src/main/java/hudson/model/DependencyGraph.java
          trunk/www/changelog.html
          http://jenkins-ci.org/commit/36210
          Log:
          [FIXED JENKINS-7256] use setContext() instead of getContext().setAuthentication()
          to temporarily set SYSTEM ACL, so this temporary access change does not affect
          other concurrent threads for same HttpSession.. conflict between such requests was
          creating the possibility of getting SYSTEM permission actually set in the session.

          SCM/JIRA link daemon added a comment - Code changed in hudson User: : mindless Path: trunk/hudson/main/core/src/main/java/hudson/model/DependencyGraph.java trunk/www/changelog.html http://jenkins-ci.org/commit/36210 Log: [FIXED JENKINS-7256] use setContext() instead of getContext().setAuthentication() to temporarily set SYSTEM ACL, so this temporary access change does not affect other concurrent threads for same HttpSession.. conflict between such requests was creating the possibility of getting SYSTEM permission actually set in the session.

          dogfood added a comment -

          Integrated in hudson_main_trunk #351
          [FIXED JENKINS-7256] use setContext() instead of getContext().setAuthentication()
          to temporarily set SYSTEM ACL, so this temporary access change does not affect
          other concurrent threads for same HttpSession.. conflict between such requests was
          creating the possibility of getting SYSTEM permission actually set in the session.

          mindless :
          Files :

          • /trunk/hudson/main/core/src/main/java/hudson/model/DependencyGraph.java
          • /trunk/www/changelog.html

          dogfood added a comment - Integrated in hudson_main_trunk #351 [FIXED JENKINS-7256] use setContext() instead of getContext().setAuthentication() to temporarily set SYSTEM ACL, so this temporary access change does not affect other concurrent threads for same HttpSession.. conflict between such requests was creating the possibility of getting SYSTEM permission actually set in the session. mindless : Files : /trunk/hudson/main/core/src/main/java/hudson/model/DependencyGraph.java /trunk/www/changelog.html

            mindless Alan Harder
            protocol7b protocol7b
            Votes:
            4 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated:
              Resolved: