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

Add ACLPermissionOverride Extension Point to grant additional permissions to an ACL regardless of the AuthorizationStrategy being used

XMLWordPrintable

    • Icon: Improvement Improvement
    • Resolution: Unresolved
    • Icon: Minor Minor
    • core

      For the github-oauth-plugin people want to use the existing GlobalMatrixAuthorizationStrategy and enable things like the github-webhook callback. Currently I have my own AuthorizationStrategy that supports these extra callback URL's but I want to be able to transparently support them without caring which specific AuthorizationStrategy is being used.

      My solution is to add a new extension point into Jenkins that is invoked at the base ACL class that checks if any ACLPermissionOverride extensions want to grant the permission before the ACL checks its own authorization logic.

      For the github-oauth-plugin it means that I can add in these extra URL's allow options into my SecurityRealm and then get them applied before the GlobalMatrixAuthorizationStrategy's ACL logic is used.

          [JENKINS-13190] Add ACLPermissionOverride Extension Point to grant additional permissions to an ACL regardless of the AuthorizationStrategy being used

          Michael O'Cleirigh added a comment - - edited

          Added pull request:
          https://github.com/jenkinsci/jenkins/pull/412

          I have tested this using the github-oauth-plugin and it works.

          This is my initial implementation using this extension:
          https://github.com/mocleiri/github-oauth-plugin/commit/28d9114bb10fecf6fb1bb33114b45604d0b20487

          Michael O'Cleirigh added a comment - - edited Added pull request: https://github.com/jenkinsci/jenkins/pull/412 I have tested this using the github-oauth-plugin and it works. This is my initial implementation using this extension: https://github.com/mocleiri/github-oauth-plugin/commit/28d9114bb10fecf6fb1bb33114b45604d0b20487

          SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Michael O'Cleirigh
          Path:
          core/src/main/java/hudson/security/ACL.java
          core/src/main/java/hudson/security/ACLPermissionOverride.java
          http://jenkins-ci.org/commit/jenkins/7d0380f46f013effb7793622707042a452ff1c66
          Log:
          JENKINS-13190 Add ACLPermissionOverride extension point

          Modifies the base ACL class to first check with ACLPermissionOverride extension point
          implementations as to whether the permission should be granted.

          If none of the ACLPermissionOverride instances approve the permission the ACL's permission
          checking logic will be used instead.

          This override provides a mechanism to grant more permissions than the ACL being overridden provides.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Michael O'Cleirigh Path: core/src/main/java/hudson/security/ACL.java core/src/main/java/hudson/security/ACLPermissionOverride.java http://jenkins-ci.org/commit/jenkins/7d0380f46f013effb7793622707042a452ff1c66 Log: JENKINS-13190 Add ACLPermissionOverride extension point Modifies the base ACL class to first check with ACLPermissionOverride extension point implementations as to whether the permission should be granted. If none of the ACLPermissionOverride instances approve the permission the ACL's permission checking logic will be used instead. This override provides a mechanism to grant more permissions than the ACL being overridden provides.

          ikedam added a comment -

          This feature is also required by authorize-project plugin (JENKINS-35081).

          I plan to try an alternate implementation to resolve following problems:

          • Current ACLPermissionOverride (AuthorizationStrategyOverride) doesn't handle ACLs for each items.
          • Plugins cause easily cause security issues by ignoring AuthoricationStrategy configured by the administrator.
            • I plan resolving this issue by providing two types of strategies:
              • AuthorizationStrategyOverride: Both allows and denies permissions. Administrator should enable it explicitly (like QueueItemAuthenticator).
              • AuthorizationStrategyOverrideDeny: Only denies permissions. This get enabled without configuration (only by installing plugins)

          ikedam added a comment - This feature is also required by authorize-project plugin ( JENKINS-35081 ). I plan to try an alternate implementation to resolve following problems: Current ACLPermissionOverride ( AuthorizationStrategyOverride ) doesn't handle ACLs for each items. Plugins cause easily cause security issues by ignoring AuthoricationStrategy configured by the administrator. I plan resolving this issue by providing two types of strategies: AuthorizationStrategyOverride: Both allows and denies permissions. Administrator should enable it explicitly (like QueueItemAuthenticator ). AuthorizationStrategyOverrideDeny: Only denies permissions. This get enabled without configuration (only by installing plugins)

          ikedam added a comment -

          I believe there're two ways to introduce overriding ACLs.
          As ACL itself doesn't hold what item it is for, overriding should be performed to AuthorizationStrategy.

          A: Jenkins#getAuthorizationStrategy returns overridden AuthorizationStrategy. A new method Jenkins#getBaseAuthorizationStrategy is introduced to return the original AuthorizationStrategy.
          B: Jenkins#getAuthorizationStrategy returns the original AuthorizationStrategy. A new method Jenkins#getOverriddenAuthorizationStrategy is introduced to return the overridden AuthorizationStrategy.

          These two ways result following regressions:

          A: Plugins using Jenkins#getAuthorizationStrategy not to resolve ACLs (e.g. to test its own strategy is configured as Jenkins#getAuthorizationStrategy() instanceof XxxxAuthorizationStraetgy) will be broken.
          B: Plugins retrieving ACLs via Jenkins#getAuthorizationStrategy will be broken.

          I searched github of jenkinsci with getAuthorizationStrategy and list up plugins affected by A and B as followings:
          A:

          • ssh2easy-plugin
          • collabnet-plugin
          • role-strategy-plugin
          • gitlab-auth-plugin
          • jenkins-scripts
          • security-no-captcha-plugin
          • matrix-auth-plugin
          • puppet-jenkins

          B.

          • gitlab-auth-plugin
          • pubsub-light-module
          • support-core-plugin

          Though this is really rough investigation, I can conclude that A is a really bad idea as it affects too many plugins.

          Plugins should use Jenkins.getACL, Item.getACL and so on instread of getAuthorizationStrategy().getACL(item).

          ikedam added a comment - I believe there're two ways to introduce overriding ACLs. As ACL itself doesn't hold what item it is for, overriding should be performed to AuthorizationStrategy . A: Jenkins#getAuthorizationStrategy returns overridden AuthorizationStrategy . A new method Jenkins#getBaseAuthorizationStrategy is introduced to return the original AuthorizationStrategy . B: Jenkins#getAuthorizationStrategy returns the original AuthorizationStrategy . A new method Jenkins#getOverriddenAuthorizationStrategy is introduced to return the overridden AuthorizationStrategy . These two ways result following regressions: A: Plugins using Jenkins#getAuthorizationStrategy not to resolve ACLs (e.g. to test its own strategy is configured as Jenkins#getAuthorizationStrategy() instanceof XxxxAuthorizationStraetgy ) will be broken. B: Plugins retrieving ACLs via Jenkins#getAuthorizationStrategy will be broken. I searched github of jenkinsci with getAuthorizationStrategy and list up plugins affected by A and B as followings: A: ssh2easy-plugin collabnet-plugin role-strategy-plugin gitlab-auth-plugin jenkins-scripts security-no-captcha-plugin matrix-auth-plugin puppet-jenkins B. gitlab-auth-plugin pubsub-light-module support-core-plugin Though this is really rough investigation, I can conclude that A is a really bad idea as it affects too many plugins. Plugins should use Jenkins.getACL , Item.getACL and so on instread of getAuthorizationStrategy().getACL(item) .

          ikedam added a comment -

          > Though this is really rough investigation, I can conclude that A is a really bad idea as it affects too many plugins.

          I also note that A contains popular plugins like matrix-auth-plugin and role-strategy-plugin.

          ikedam added a comment - > Though this is really rough investigation, I can conclude that A is a really bad idea as it affects too many plugins. I also note that A contains popular plugins like matrix-auth-plugin and role-strategy-plugin.

          ikedam added a comment -

          Sent a pull request:
          https://github.com/jenkinsci/jenkins/pull/2481

          ikedam added a comment - Sent a pull request: https://github.com/jenkinsci/jenkins/pull/2481

          Jesse Glick added a comment -

          Related proposals in JENKINS-32596.

          Jesse Glick added a comment - Related proposals in JENKINS-32596 .

            Unassigned Unassigned
            mocleiri Michael O'Cleirigh
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: