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

QueueItemAuthenticator fallback behavior cleanup

    Details

    • Similar Issues:

      Description

      Currently Queue.Item.authenticate and Tasks.getAuthenticationOf check any configured QueueItemAuthenticator instances for a specific answer, then fall back to Queue.Task.getDefaultAuthentication, which for compatibility is normally SYSTEM. This behavior makes sense if there are no configured authenticators (the default case), or if there is an authenticator which returns a non-null authentication for every task.

      The problematic case is when there is a configured authenticator yet it returns null for the given task or item. This could happen if for example ProjectQueueItemAuthenticator were configured yet a user created a new job and did not add a AuthorizeProjectProperty. Now the job runs as SYSTEM, with full privileges, whereas we would really want it to run as ANONYMOUS, with no special privileges.

      BuildTrigger and ReverseBuildTrigger already specially handle this case by substituting ANONYMOUS for SYSTEM while they run (and in the former case printing a warning to the build console). But this does not extend to other plugin or core access control checks.

      In particular, Computer.BUILD is checked in two places (on Item) when deciding whether to build a job on a particular slave. You would expect to be able to control access to sensitive slaves by selectively granting BUILD only to certain people, and then requiring them to associate themselves with jobs they wish to build on those slaves. But this scheme cannot work if they can simply omit any authentication on the job and still have the build be authorized. (Jenkins Enterprise by CloudBees has a feature using a different scheme for the same use case.)

      It would be better if the callers of QueueItemAuthenticator.authenticate fell back to getDefaultAuthentication but replaced SYSTEM with ANONYMOUS in case there were at least one authenticator consulted. Then the replacement of SYSTEM with ANONYMOUS could be removed from both BuildTrigger and ReverseBuildTrigger. BuildTrigger.warning_you_have_no_plugins_providing_ac and BuildTrigger.warning_access_control_for_builds_in_glo could be left as is, or made more generic and moved into Run.execute; BuildTrigger.warning_this_build_has_no_associated_aut should be made more generic and moved into Run.execute (and Run.running_as_ printed with anonymous).

        Attachments

          Issue Links

            Activity

            Hide
            jglick Jesse Glick added a comment -

            See ada4a21 and 63aba90 (Computer.BUILD) in 1.520.

            Show
            jglick Jesse Glick added a comment - See ada4a21 and 63aba90 ( Computer.BUILD ) in 1.520.
            Hide
            oleg_nenashev Oleg Nenashev added a comment - - edited

            I think that the default user should be configurable from the global security page.
            SYSTEM should be default (with scary red/yellow warning, of course)
            Otherwise, it's hard to predict all possible regressions

            Show
            oleg_nenashev Oleg Nenashev added a comment - - edited I think that the default user should be configurable from the global security page. SYSTEM should be default (with scary red/yellow warning, of course) Otherwise, it's hard to predict all possible regressions
            Hide
            jglick Jesse Glick added a comment -

            The only possible regressions are to people already using the Authorize Project plugin. Currently there are 155 installations, presumably some of them not actually having configured it yet.

            Show
            jglick Jesse Glick added a comment - The only possible regressions are to people already using the Authorize Project plugin. Currently there are 155 installations, presumably some of them not actually having configured it yet.
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            We use an internal plugin on "eval" servers with 1.532.x+, other users may use such plugins as well.
            Actually, I'd definitely use the "Anonymous" user as a default fallback, but I don't think that the regression injection is a good approach in general.

            Show
            oleg_nenashev Oleg Nenashev added a comment - We use an internal plugin on "eval" servers with 1.532.x+, other users may use such plugins as well. Actually, I'd definitely use the "Anonymous" user as a default fallback, but I don't think that the regression injection is a good approach in general.
            Hide
            jglick Jesse Glick added a comment -

            I would consider a system property or similar to provide a temporary workaround to the probably small number of people already using the plugin who really need the current (insecure) behavior until something else if fixed or reconfigured. But I do not think it should be a UI option since it does not make sense going forward.

            Show
            jglick Jesse Glick added a comment - I would consider a system property or similar to provide a temporary workaround to the probably small number of people already using the plugin who really need the current (insecure) behavior until something else if fixed or reconfigured. But I do not think it should be a UI option since it does not make sense going forward.
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            Yes, a system property would be a convenient solution.

            Show
            oleg_nenashev Oleg Nenashev added a comment - Yes, a system property would be a convenient solution.
            Hide
            stephenconnolly Stephen Connolly added a comment -

            I think it would make more sense to introduce a specific system "identity" for builds rather than pushing down to ANONYMOUS. If somebody wants all builds to run by default as ANONYMOUS then use JENKINS-30574 to set that default... falling off the end of the list of authenticators (empty or not) should give you something like ACL.BUILDS instead of ACL.SYSTEM IMHO

            Show
            stephenconnolly Stephen Connolly added a comment - I think it would make more sense to introduce a specific system "identity" for builds rather than pushing down to ANONYMOUS. If somebody wants all builds to run by default as ANONYMOUS then use JENKINS-30574 to set that default... falling off the end of the list of authenticators (empty or not) should give you something like ACL.BUILDS instead of ACL.SYSTEM IMHO
            Hide
            jglick Jesse Glick added a comment -

            Introducing a new identity is not really an option, unless every AuthorizationStrategy’s config UI were adapted to offer this as an option. Much simpler to default to anonymous.

            Show
            jglick Jesse Glick added a comment - Introducing a new identity is not really an option, unless every AuthorizationStrategy ’s config UI were adapted to offer this as an option. Much simpler to default to anonymous.
            Hide
            danielbeck Daniel Beck added a comment -

            Kohsuke Kawaguchi Please use 2.0-rejected so we can keep track of things that were nominated and are no longer being considered.

            Show
            danielbeck Daniel Beck added a comment - Kohsuke Kawaguchi Please use 2.0-rejected so we can keep track of things that were nominated and are no longer being considered.
            Hide
            stephenconnolly Stephen Connolly added a comment -

            Since there is some confusion from people as to why this issue is now redundant. In Authorize Project Plugin 1.2.0 JENKINS-30574 implemented a feature that makes this request irrelevant as configuration can now enable this behaviour for instances that wish to opt-in, thereby removing the risk that would be inherent in implementing JENKINS-22949

            Here is how to configure JENKINS-30574 to behave in this way:

            Show
            stephenconnolly Stephen Connolly added a comment - Since there is some confusion from people as to why this issue is now redundant. In Authorize Project Plugin 1.2.0 JENKINS-30574 implemented a feature that makes this request irrelevant as configuration can now enable this behaviour for instances that wish to opt-in, thereby removing the risk that would be inherent in implementing JENKINS-22949 Here is how to configure JENKINS-30574 to behave in this way:
            Hide
            jglick Jesse Glick added a comment -

            Using the authorize-project feature does sound like the cleanest solution going forward. As alluded to in issue description, that leave a bunch of code in core that needs to be cleaned up.

            Show
            jglick Jesse Glick added a comment - Using the authorize-project feature does sound like the cleanest solution going forward. As alluded to in issue description, that leave a bunch of code in core that needs to be cleaned up.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            core/src/main/java/hudson/model/AbstractProject.java
            core/src/main/java/hudson/tasks/BuildTrigger.java
            http://jenkins-ci.org/commit/jenkins/f1a26582c1b25c470989d19b25378cff5c6d2497
            Log:
            JENKINS-28113 Allow freestyle builds to trigger downstream Pipeline builds (#2873)

            • Offering default methods on ParameterizedJob.
            • Javadoc typo.
            • Cleaner use of default methods in ParameterizedJob.
            • Sketch of pulling disabled functionality into ParameterizedJob.
            • EnableJobCommandTest.groovy → EnableJobCommandTest.java, and replacing deprecated Remoting-based CLI calls with CLICommandInvoker.
            • All CLI commands could be broken by a missing CLI.*.shortDescription key on just one!
            • Forgot to move CLI method short descriptions to new package.
            • Needed a @CLIResolver for ParameterizedJob. Adding an OptionHandler while we are here.
            • Trying to fix up access-modifier versions; started failing in CI today for unknown reasons.
            • Introduced <p:makeDisabled/> by analogy with <p:config-disableBuild/>.
            • Using new type bounds.
            • access-modifier 1.11 released.
            • MatrixProject and MavenModuleSet both expect to have access to makeDisabled.jelly.
            • Trying to generalize some more.
            • Minor simplification.
            • isBuildable
            • Obsolete comment.
            • Updated comments.
            • bridge-method-injector 1.17
            • Unfortunately AbstractProject.schedulePolling cannot delegate to SCMTriggerItem.
            • [FIXED JENKINS-28113] Generalize BuildTrigger to be able to trigger non-AbstractProject downstream ParameterizedJob’s without DependencyGraph.
            • JENKINS-22949 Dropping QueueItemAuthenticator trickiness, as in #2881.
            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/hudson/model/AbstractProject.java core/src/main/java/hudson/tasks/BuildTrigger.java http://jenkins-ci.org/commit/jenkins/f1a26582c1b25c470989d19b25378cff5c6d2497 Log: JENKINS-28113 Allow freestyle builds to trigger downstream Pipeline builds (#2873) Offering default methods on ParameterizedJob. Javadoc typo. Cleaner use of default methods in ParameterizedJob. Need to pick up https://github.com/infradna/bridge-method-injector/pull/15 to be able to build. Sketch of pulling disabled functionality into ParameterizedJob. EnableJobCommandTest.groovy → EnableJobCommandTest.java, and replacing deprecated Remoting-based CLI calls with CLICommandInvoker. All CLI commands could be broken by a missing CLI.*.shortDescription key on just one! Forgot to move CLI method short descriptions to new package. Needed a @CLIResolver for ParameterizedJob. Adding an OptionHandler while we are here. Trying to fix up access-modifier versions; started failing in CI today for unknown reasons. Introduced <p:makeDisabled/> by analogy with <p:config-disableBuild/>. Using new type bounds. access-modifier 1.11 released. MatrixProject and MavenModuleSet both expect to have access to makeDisabled.jelly. Trying to generalize some more. Minor simplification. JENKINS-34716 Generalizing doPolling and schedulePolling. isBuildable Obsolete comment. Updated comments. bridge-method-injector 1.17 Unfortunately AbstractProject.schedulePolling cannot delegate to SCMTriggerItem. [FIXED JENKINS-28113] Generalize BuildTrigger to be able to trigger non-AbstractProject downstream ParameterizedJob’s without DependencyGraph. JENKINS-22949 Dropping QueueItemAuthenticator trickiness, as in #2881.
            Hide
            jglick Jesse Glick added a comment -

            in 2.61

            Show
            jglick Jesse Glick added a comment - in 2.61
            Hide
            danielbeck Daniel Beck added a comment -

            Jesse Glick Should there be a changelog entry for this as well?

            Show
            danielbeck Daniel Beck added a comment - Jesse Glick Should there be a changelog entry for this as well?
            Hide
            jglick Jesse Glick added a comment -

            Daniel Beck yes, I think I proposed some text for that in the PR, no?

            Show
            jglick Jesse Glick added a comment - Daniel Beck yes, I think I proposed some text for that in the PR, no?
            Hide
            danielbeck Daniel Beck added a comment -

            Jesse Glick

            You wrote:

            • Freestyle projects may now list Pipeline jobs as downstream and trigger them, without needing to use the Parameterized Trigger plugin or reverse triggers.

            I used:

            Freestyle projects may now list Pipeline jobs as downstream and trigger them, without needing to use the Parameterized Trigger plugin or reverse triggers ("Build after other projects are built"). (issue 28113)

            No mention of this cleanup AFAICT.

            Show
            danielbeck Daniel Beck added a comment - Jesse Glick You wrote: Freestyle projects may now list Pipeline jobs as downstream and trigger them, without needing to use the Parameterized Trigger plugin or reverse triggers. I used: Freestyle projects may now list Pipeline jobs as downstream and trigger them, without needing to use the Parameterized Trigger plugin or reverse triggers ("Build after other projects are built"). (issue 28113) No mention of this cleanup AFAICT.
            Hide
            jglick Jesse Glick added a comment -

            I think you are mixing this up with an unrelated issue.

            Show
            jglick Jesse Glick added a comment - I think you are mixing this up with an unrelated issue.
            Hide
            danielbeck Daniel Beck added a comment -

            Jesse Glick The merge commit is for PR 2873. The referenced PR, 2881, is still open. The state of this issue is unclear to me.

            Show
            danielbeck Daniel Beck added a comment - Jesse Glick The merge commit is for PR 2873. The referenced PR, 2881, is still open. The state of this issue is unclear to me.
            Hide
            jglick Jesse Glick added a comment -

            Sorry, this is still open.

            Show
            jglick Jesse Glick added a comment - Sorry, this is still open.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            core/src/main/java/hudson/model/Executor.java
            core/src/main/java/hudson/tasks/BuildTrigger.java
            core/src/main/java/jenkins/triggers/ReverseBuildTrigger.java
            core/src/main/resources/hudson/tasks/Messages.properties
            core/src/main/resources/hudson/tasks/Messages_bg.properties
            core/src/main/resources/hudson/tasks/Messages_de.properties
            core/src/main/resources/hudson/tasks/Messages_pt_BR.properties
            core/src/main/resources/hudson/tasks/Messages_sr.properties
            test/src/test/java/hudson/tasks/BuildTriggerTest.java
            test/src/test/java/jenkins/triggers/ReverseBuildTriggerTest.java
            http://jenkins-ci.org/commit/jenkins/915543dca5399d3ba052219ddfe9c3c061e70726
            Log:
            JENKINS-22949 BuildTrigger & ReverseBuildTrigger should respect QueueItemAuthenticatorConfiguration (#2881)

            • JENKINS-22949 Simplifying behavior of BuildTrigger & ReverseBuildTrigger to honor QueueItemAuthenticator’s as defined, rather than falling back to anonymous.
            • There is no need to impersonate what is already the current thread’s authentication.
            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/hudson/model/Executor.java core/src/main/java/hudson/tasks/BuildTrigger.java core/src/main/java/jenkins/triggers/ReverseBuildTrigger.java core/src/main/resources/hudson/tasks/Messages.properties core/src/main/resources/hudson/tasks/Messages_bg.properties core/src/main/resources/hudson/tasks/Messages_de.properties core/src/main/resources/hudson/tasks/Messages_pt_BR.properties core/src/main/resources/hudson/tasks/Messages_sr.properties test/src/test/java/hudson/tasks/BuildTriggerTest.java test/src/test/java/jenkins/triggers/ReverseBuildTriggerTest.java http://jenkins-ci.org/commit/jenkins/915543dca5399d3ba052219ddfe9c3c061e70726 Log: JENKINS-22949 BuildTrigger & ReverseBuildTrigger should respect QueueItemAuthenticatorConfiguration (#2881) JENKINS-22949 Simplifying behavior of BuildTrigger & ReverseBuildTrigger to honor QueueItemAuthenticator’s as defined, rather than falling back to anonymous. There is no need to impersonate what is already the current thread’s authentication.

              People

              • Assignee:
                jglick Jesse Glick
                Reporter:
                jglick Jesse Glick
              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: