Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-7256

Users intermittently gets SYSTEM user

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved (View Workflow)
    • Priority: Critical
    • Resolution: Fixed
    • Component/s: _unsorted
    • Labels:
      None
    • Environment:
      ASF Hudson installation, Hudson 1.372, Tomcat 6, running against LDAP
    • Similar Issues:

      Description

      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.

        Attachments

          Activity

          Hide
          mindless 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?

          Show
          mindless 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?
          Hide
          mindless 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);
                   }
               }
          
          
          Show
          mindless 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); } }
          Hide
          mdillon 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.

          Show
          mdillon 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.
          Hide
          scm_issue_link 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.

          Show
          scm_issue_link 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.
          Hide
          dogfood 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
          Show
          dogfood 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

            People

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

              Dates

              Created:
              Updated:
              Resolved: