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

Access to check for unprotected/never secured paths

    • Icon: Improvement Improvement
    • Resolution: Fixed
    • Icon: Minor Minor
    • core
    • None

      Currently no method exists (that I could find) to allow authentication plugins working on top of Jenkins to test a path to see if it does not require authentication; Jenkins does it (in part, at least) using Jenkins.getTarget() (https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/model/Jenkins.java#L3875). This function does too much for some plugins, e.g. NegotiateSSO, because it first checks for read permissions.

      I propose breaking the functionality of Jenkins.getTarget() into several functions, allowing a plugin to access the checks for paths that shouldn't be protected. The attached diff file gives an example (diff base is commit e014700, on jenkinsci/jenkins).

      The other solutions that have been given to me so far involve maintaining a list of exceptions, which seems unwise to me.

          [JENKINS-32797] Access to check for unprotected/never secured paths

          Daniel Beck added a comment -

          I currently have no way to test against that dynamic list with the currently available functions in Jenkins.java, and have not seen any other function that could supply that functionality to my plugin.

          What's wrong with Jenkins.getInstance().getUnprotectedRootActions(), then getUrlName on each?

          Daniel Beck added a comment - I currently have no way to test against that dynamic list with the currently available functions in Jenkins.java, and have not seen any other function that could supply that functionality to my plugin. What's wrong with Jenkins.getInstance().getUnprotectedRootActions() , then getUrlName on each?

          Actually (because I didn't do a full look over my workaround code) I am using getUnprotectedRootActions(), but I still need to keep a separate list of AlwaysReadablePaths, and a regex for slave agents.

          All told I am duplicating about 50 lines of code from Jenkins.java (as an almost exact duplicate) to do the same operation that is contained in the catch clause of Jenkins.getTarget(). In my case I have to check those exceptions before I try authenticating, because I cannot (unfortunately) control filter chaining after I attempt authentication - that process happens in a filter in external code, that I call using super.doFilter().

          I may have misspoke about this being a problem with kerberos-sso-plugin, only because do not have a way to test it myself; if kerberos-sso-plugin does not have this issue, it is because it is able to execute the filter chain regardless of whether authentication was successful or not. I can try to attempt something similar, but I don't know if I will be successful in doing so. Going to test that before pursuing this further...

          Bryson Gibbons added a comment - Actually (because I didn't do a full look over my workaround code) I am using getUnprotectedRootActions(), but I still need to keep a separate list of AlwaysReadablePaths, and a regex for slave agents. All told I am duplicating about 50 lines of code from Jenkins.java (as an almost exact duplicate) to do the same operation that is contained in the catch clause of Jenkins.getTarget(). In my case I have to check those exceptions before I try authenticating, because I cannot (unfortunately) control filter chaining after I attempt authentication - that process happens in a filter in external code, that I call using super.doFilter(). I may have misspoke about this being a problem with kerberos-sso-plugin, only because do not have a way to test it myself; if kerberos-sso-plugin does not have this issue, it is because it is able to execute the filter chain regardless of whether authentication was successful or not. I can try to attempt something similar, but I don't know if I will be successful in doing so. Going to test that before pursuing this further...

          Okay, I have to process these exception paths before I allow the authorization portion of the filter to run. The authorization portion of the filter (which is outside of my control) automatically creates and sends a "401 Unauthorized" page, and flushes the response buffer, removing any possibility of my code recovering from failed authorization (and then letting Jenkins take care of it via the normal process).

          I may ask about making that portion of the code in the dependency configurable/optional...

          Bryson Gibbons added a comment - Okay, I have to process these exception paths before I allow the authorization portion of the filter to run. The authorization portion of the filter (which is outside of my control) automatically creates and sends a "401 Unauthorized" page, and flushes the response buffer, removing any possibility of my code recovering from failed authorization (and then letting Jenkins take care of it via the normal process). I may ask about making that portion of the code in the dependency configurable/optional...

          Is this about kerberos-sso plugin only? In case it is, please file an issue for whitelist JNLP connection endpoint. I would be more comfortable addressing it in that plugin rather than in core (as it is the plugin what have unusual security handling).

          Oliver Gondža added a comment - Is this about kerberos-sso plugin only? In case it is, please file an issue for whitelist JNLP connection endpoint. I would be more comfortable addressing it in that plugin rather than in core (as it is the plugin what have unusual security handling).

          Bryson Gibbons added a comment - - edited

          Actually, this is an issue for any plugin that attempts to use Kerberos for signing in through servlet filters; right now, that I know of, this includes Kerberos-sso plugin and negotiatesso plugin. Currently I have duplicated parts of the code in core that are inaccessible to provide the needed functionality (see https://github.com/jenkinsci/negotiatesso-plugin/blob/master/src/main/java/com/github/farmgeek4life/jenkins/negotiatesso/NegSecFilter.java), and now a pull request with Kerberos-sso plugin is looking to also skip authentication for unprotected root actions (https://github.com/jenkinsci/kerberos-sso-plugin/pull/9)

          Without adding some small level of access to these functions, any plugin that performs authentication through a servlet filter will have to duplicate code from core, or authenticate access to some paths that actually should be accessible without authentication.

          Bryson Gibbons added a comment - - edited Actually, this is an issue for any plugin that attempts to use Kerberos for signing in through servlet filters; right now, that I know of, this includes Kerberos-sso plugin and negotiatesso plugin. Currently I have duplicated parts of the code in core that are inaccessible to provide the needed functionality (see https://github.com/jenkinsci/negotiatesso-plugin/blob/master/src/main/java/com/github/farmgeek4life/jenkins/negotiatesso/NegSecFilter.java ), and now a pull request with Kerberos-sso plugin is looking to also skip authentication for unprotected root actions ( https://github.com/jenkinsci/kerberos-sso-plugin/pull/9 ) Without adding some small level of access to these functions, any plugin that performs authentication through a servlet filter will have to duplicate code from core, or authenticate access to some paths that actually should be accessible without authentication.

          Oliver Gondža added a comment - - edited

          It makes sense to provide a way for plugins to tell of path should not be protected or not. Commenting on the state of the path as attached (please move this to PR), please consolidate it to one method that accept restOfPath as an argument and performs all of the checks. Provided you do not need the two for some reason.

          Oliver Gondža added a comment - - edited It makes sense to provide a way for plugins to tell of path should not be protected or not. Commenting on the state of the path as attached (please move this to PR), please consolidate it to one method that accept restOfPath as an argument and performs all of the checks. Provided you do not need the two for some reason.

          Will do as one function; after looking at my duplicated code, I was unable to use "Stapler.getCurrentRequest().getRestOfPath()" because at the point I am checking permission requirements the request is still in the filters, and stapler doesn't know about it yet.

          Bryson Gibbons added a comment - Will do as one function; after looking at my duplicated code, I was unable to use "Stapler.getCurrentRequest().getRestOfPath()" because at the point I am checking permission requirements the request is still in the filters, and stapler doesn't know about it yet.

          Bryson Gibbons added a comment - Pull Request: https://github.com/jenkinsci/jenkins/pull/2652

          Code changed in jenkins
          User: Bryson Gibbons
          Path:
          core/src/main/java/jenkins/model/Jenkins.java
          http://jenkins-ci.org/commit/jenkins/0060335b8cf6d36641bd610817bae98873c32746
          Log:
          JENKINS-32797 Break the catch clause contents of Jenkins.getTarget(… (#2652)

          • JENKINS-32797 Break the catch clause contents of Jenkins.getTarget() out into a separate, publicly accessible function.

          This will allow plugins (particularly authentication plugins that override the normal authentication process) to determine if authentication is not required for a particular path by calling isPathUnprotected(restOfPath).

          • Add @since TODO to comment
          • Change name of function to something that is accurate and clear

          isPathUnprotected is misleading, and the Javadoc was worse. isSubjectToMandatoryReadPermissionCheck is a much better name, and the return value is reversed to match the name,

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Bryson Gibbons Path: core/src/main/java/jenkins/model/Jenkins.java http://jenkins-ci.org/commit/jenkins/0060335b8cf6d36641bd610817bae98873c32746 Log: JENKINS-32797 Break the catch clause contents of Jenkins.getTarget(… (#2652) JENKINS-32797 Break the catch clause contents of Jenkins.getTarget() out into a separate, publicly accessible function. This will allow plugins (particularly authentication plugins that override the normal authentication process) to determine if authentication is not required for a particular path by calling isPathUnprotected(restOfPath). Add @since TODO to comment Change name of function to something that is accurate and clear isPathUnprotected is misleading, and the Javadoc was worse. isSubjectToMandatoryReadPermissionCheck is a much better name, and the return value is reversed to match the name,

          Oleg Nenashev added a comment -

          Released in jenkins-2.37

          Oleg Nenashev added a comment - Released in jenkins-2.37

            Unassigned Unassigned
            farmgeek4life Bryson Gibbons
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated:
              Resolved: