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

Crumb must be sent with POST requests even when using authentication token

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      If you are making a POST request from a tool like curl, and Jenkins is configured with a CrumbIssuer, you are required to obtain and pass back a valid crumb (preferably as an HTTP header) even when you are using an API token. This seems unnecessary, since the purpose of crumbs is to defend against CSRF attacks, which are possible only when the victim has a Jenkins session cookie in a browser or some browserlike client and unwittingly makes an HTTP request instigated by the attacker. But a request using basic authentication with an API token could not have come from such a client—you would not try to log in using an API token rather than using security realm-specific means, and in fact you cannot do so unless Jenkins sent a 401 response with a WWW-Authenticate header, which it almost never does.

      (loginError.jelly includes a 401 response without that header, which is probably wrong. BasicAuthenticationFilter, used with the legacy security realm, does send the challenge, though this does not seem to let you actually log in using an API token either—only pass through, like ApiTokenFilter would do.)

      Assuming that it is true that you cannot get a login session using an API token even if you tried, I would suggest that CrumbFilter check for req.getAttribute(ApiTokenProperty.class.getName()) instanceof User, which would be true if ApiTokenFilter has already run successfully, and in this case do nothing. (Or, for better modularity, ApiTokenFilter could implement a CrumbExclusion.) This would make the REST API significantly easier to use.

        Attachments

          Issue Links

            Activity

            Hide
            jglick Jesse Glick added a comment -

            Best if tokens are not retrievable, so there is no risk of /me/configure being retrieved by a cross-site script.

            Show
            jglick Jesse Glick added a comment - Best if tokens are not retrievable, so there is no risk of /me/configure being retrieved by a cross-site script.
            Hide
            wfollonier Wadeck Follonier added a comment -

            As the ApiTokenFilter is deprecated now, I do the attribute setting in BasicHeaderApiTokenAuthenticator after having check the API Token.

            Concerning the retrievability, it will be addressed in JENKINS-32776.

            Show
            wfollonier Wadeck Follonier added a comment - As the ApiTokenFilter is deprecated now, I do the attribute setting in BasicHeaderApiTokenAuthenticator after having check the API Token. Concerning the retrievability, it will be addressed in JENKINS-32776 .
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Wadeck Follonier
            Path:
            core/src/main/java/jenkins/security/ApiCrumbExclusion.java
            core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java
            test/pom.xml
            test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java
            test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java
            test/src/test/java/hudson/model/AbstractProjectTest.java
            test/src/test/java/hudson/model/ExecutorTest.java
            test/src/test/java/hudson/model/ItemsTest.java
            test/src/test/java/hudson/model/JobTest.java
            test/src/test/java/hudson/model/PasswordParameterDefinitionTest.java
            test/src/test/java/hudson/model/ProjectTest.java
            test/src/test/java/hudson/model/QueueTest.java
            test/src/test/java/hudson/model/UserTest.java
            test/src/test/java/hudson/model/ViewTest.java
            test/src/test/java/hudson/security/ExtendedReadPermissionTest.java
            test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java
            test/src/test/java/hudson/security/LoginTest.java
            test/src/test/java/hudson/util/RobustReflectionConverterTest.java
            test/src/test/java/jenkins/model/JenkinsTest.java
            test/src/test/java/jenkins/security/ApiCrumbExclusionTest.java
            test/src/test/java/jenkins/security/ApiTokenPropertyTest.java
            test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java
            test/src/test/java/jenkins/security/Security380Test.java
            test/src/test/java/lib/form/PasswordTest.java
            test/src/test/resources/lib/form/PasswordTest/SecretNotPlainText/index.jelly
            test/src/test/resources/lib/form/PasswordTest/test1.jelly
            http://jenkins-ci.org/commit/jenkins/814d202716a6c61c7d371c6a62755d296fe199a5
            Log:
            JENKINS-22474 API Token does not require CSRF token (#3129)

            • in order to ease the use of the api, we are not requiring the request to have a crumb
            • in terms of security it's not a problem normally since the CSRF attacks use the cookies and in case of API Token, it's session-less / cookie-less
            • - adjust the license header
            • - add test for basic authentication
            • add test for login process
            • - add test for form submission using ui (htmlunit), not just login form
            • - modification requested by Jesse
            • - pom.xml update to use the last version of jenkins-test-harness (with the token helper methods)
            • beginning of the simplification of tests
            • - using the try-with-resource approach to ease readability
            • - using closure method now
            • - add missing login transformation
            • - add unit test
            • - use withToken
            • remove useless crumb for GET method
            • add fail (otherwise the assert in catch is not as useful as it could be)
            • another bunch of test cases
            • - for HudsonTestCase, some additional modifications are required: changing the view / different type of management for the variable inside the views
            • - small other tests
            • - last batch for the login method
            • - crumb is not more required since we are using API Token
            • - converting auth to ApiToken to avoid crumb method
            • - converting auth to ApiToken to avoid crumb method (second)
            • - remove usage of closure aware methods
            • - update the pom using the snapshot as adviced by Jesse
            • modifications on other class to adapt to the last modifications in JTH
            • - modifications requested during code review
            • - also put back my changes to the conflicted file
            • - correction of the merge
            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Wadeck Follonier Path: core/src/main/java/jenkins/security/ApiCrumbExclusion.java core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java test/pom.xml test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java test/src/test/java/hudson/model/AbstractProjectTest.java test/src/test/java/hudson/model/ExecutorTest.java test/src/test/java/hudson/model/ItemsTest.java test/src/test/java/hudson/model/JobTest.java test/src/test/java/hudson/model/PasswordParameterDefinitionTest.java test/src/test/java/hudson/model/ProjectTest.java test/src/test/java/hudson/model/QueueTest.java test/src/test/java/hudson/model/UserTest.java test/src/test/java/hudson/model/ViewTest.java test/src/test/java/hudson/security/ExtendedReadPermissionTest.java test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java test/src/test/java/hudson/security/LoginTest.java test/src/test/java/hudson/util/RobustReflectionConverterTest.java test/src/test/java/jenkins/model/JenkinsTest.java test/src/test/java/jenkins/security/ApiCrumbExclusionTest.java test/src/test/java/jenkins/security/ApiTokenPropertyTest.java test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java test/src/test/java/jenkins/security/Security380Test.java test/src/test/java/lib/form/PasswordTest.java test/src/test/resources/lib/form/PasswordTest/SecretNotPlainText/index.jelly test/src/test/resources/lib/form/PasswordTest/test1.jelly http://jenkins-ci.org/commit/jenkins/814d202716a6c61c7d371c6a62755d296fe199a5 Log: JENKINS-22474 API Token does not require CSRF token (#3129) JENKINS-22474 API Token does not require CSRF token in order to ease the use of the api, we are not requiring the request to have a crumb in terms of security it's not a problem normally since the CSRF attacks use the cookies and in case of API Token, it's session-less / cookie-less - adjust the license header - add test for basic authentication add test for login process - add test for form submission using ui (htmlunit), not just login form - modification requested by Jesse - pom.xml update to use the last version of jenkins-test-harness (with the token helper methods) beginning of the simplification of tests - using the try-with-resource approach to ease readability - using closure method now - add missing login transformation - add unit test - use withToken remove useless crumb for GET method add fail (otherwise the assert in catch is not as useful as it could be) another bunch of test cases - for HudsonTestCase, some additional modifications are required: changing the view / different type of management for the variable inside the views - small other tests - last batch for the login method - crumb is not more required since we are using API Token - converting auth to ApiToken to avoid crumb method - converting auth to ApiToken to avoid crumb method (second) - remove usage of closure aware methods - update the pom using the snapshot as adviced by Jesse modifications on other class to adapt to the last modifications in JTH - modifications requested during code review - also put back my changes to the conflicted file - correction of the merge
            Hide
            danielbeck Daniel Beck added a comment -

            Fixed towards 2.96.

            Show
            danielbeck Daniel Beck added a comment - Fixed towards 2.96.

              People

              • Assignee:
                wfollonier Wadeck Follonier
                Reporter:
                jglick Jesse Glick
              • Votes:
                6 Vote for this issue
                Watchers:
                12 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: