[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 :)
jenkinsci · Dec 16, 2017 · 814d202 · 814d202
1 parent 1270ba3
commit 814d202
Show file tree

Hide file tree

Showing 25 changed files with 448 additions and 165 deletions.
diff --git a/core/src/main/java/jenkins/security/ApiCrumbExclusion.java b/core/src/main/java/jenkins/security/ApiCrumbExclusion.java
@@ -0,0 +1,53 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2017 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package jenkins.security;
+
+import hudson.Extension;
+import hudson.security.csrf.CrumbExclusion;
+import org.jenkinsci.Symbol;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.DoNotUse;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * JENKINS-22474: Makes API Token calls bypass CSRF protection to ease usage
+ */
+@Symbol("apiToken")
+@Extension
+@Restricted(DoNotUse.class)
+public class ApiCrumbExclusion extends CrumbExclusion {
+    @Override
+    public boolean process(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException {
+        if (Boolean.TRUE.equals(request.getAttribute(BasicHeaderApiTokenAuthenticator.class.getName()))) {
+            chain.doFilter(request, response);
+            return true;
+        }
+        return false;
+    }
+}
diff --git a/core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java b/core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java
@@ -32,13 +32,12 @@ public Authentication authenticate(HttpServletRequest req, HttpServletResponse r
         User u = User.getById(username, true);
         ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
         if (t!=null && t.matchesPassword(password)) {
+            Authentication auth;
             try {
                 UserDetails userDetails = u.getUserDetailsForImpersonation();
-                Authentication auth = u.impersonate(userDetails);
+                auth = u.impersonate(userDetails);
 
                 SecurityListener.fireAuthenticated(userDetails);
-
-                return auth;
             } catch (UsernameNotFoundException x) {
                 // The token was valid, but the impersonation failed. This token is clearly not his real password,
                 // so there's no point in continuing the request processing. Report this error and abort.
@@ -47,6 +46,9 @@ public Authentication authenticate(HttpServletRequest req, HttpServletResponse r
             } catch (DataAccessException x) {
                 throw new ServletException(x);
             }
+
+            req.setAttribute(BasicHeaderApiTokenAuthenticator.class.getName(), true);
+            return auth;
         }
         return null;
     }

diff --git a/test/pom.xml b/test/pom.xml
@@ -53,7 +53,7 @@ THE SOFTWARE.
     <dependency>
       <groupId>${project.groupId}</groupId>
       <artifactId>jenkins-test-harness</artifactId>
-      <version>2.31</version>
+      <version>2.32</version>
       <scope>test</scope>
       <exclusions>
         <exclusion>

diff --git a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java
@@ -31,6 +31,7 @@
 import hudson.cli.util.ScriptLoader;
 import hudson.model.Node.Mode;
 import hudson.model.Slave;
+import hudson.model.User;
 import hudson.remoting.Channel;
 import hudson.slaves.JNLPLauncher;
 import hudson.slaves.RetentionStrategy;
@@ -86,7 +87,7 @@ protected Slave createNewJnlpSlave(String name) throws Exception {
     public void anonymousCanAlwaysLoadJARs() throws Exception {
         r.jenkins.setNodes(Collections.singletonList(createNewJnlpSlave("test")));
         JenkinsRule.WebClient wc = r.createWebClient();
-        HtmlPage p = wc.login("alice").goTo("computer/test/");
+        HtmlPage p = wc.withBasicApiToken(User.getById("alice", true)).goTo("computer/test/");
 
         // this fresh WebClient doesn't have a login cookie and represent JNLP launcher
         JenkinsRule.WebClient jnlpAgent = r.createWebClient();

diff --git a/test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java b/test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java
@@ -11,10 +11,7 @@
 import com.gargoylesoftware.htmlunit.util.NameValuePair;
 import hudson.model.User;
 import hudson.security.GlobalMatrixAuthorizationStrategy;
-import hudson.security.HudsonPrivateSecurityRealm;
-import hudson.security.Permission;
 import jenkins.model.Jenkins;
-import org.acegisecurity.context.SecurityContextHolder;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -25,6 +22,7 @@
 import com.gargoylesoftware.htmlunit.ElementNotFoundException;
 
 import java.io.IOException;
+import java.net.URL;
 import java.util.Collections;
 
 /**
@@ -73,28 +71,33 @@ public void noAccessForNonAdmin() throws Exception {
         auth.add(Jenkins.READ, "users");
         j.jenkins.setAuthorizationStrategy(auth);
 
-        WebRequest request = new WebRequest(wc.createCrumbedUrl("administrativeMonitor/hudsonHomeIsFull/act"), HttpMethod.POST);
+        User bob = User.getById("bob", true);
+        User administrator = User.getById("administrator", true);
+
+        WebRequest request = new WebRequest(new URL(wc.getContextPath() + "administrativeMonitor/hudsonHomeIsFull/act"), HttpMethod.POST);
         NameValuePair param = new NameValuePair("no", "true");
         request.setRequestParameters(Collections.singletonList(param));
 
         HudsonHomeDiskUsageMonitor mon = HudsonHomeDiskUsageMonitor.get();
 
+        wc.withBasicApiToken(bob);
         try {
-            wc.login("bob");
             wc.getPage(request);
+            fail();
         } catch (FailingHttpStatusCodeException e) {
             assertEquals(403, e.getStatusCode());
         }
         assertTrue(mon.isEnabled());
 
+        WebRequest requestReadOnly = new WebRequest(new URL(wc.getContextPath() + "administrativeMonitor/hudsonHomeIsFull"), HttpMethod.GET);
         try {
-            WebRequest getIndex = new WebRequest(wc.createCrumbedUrl("administrativeMonitor/hudsonHomeIsFull"), HttpMethod.GET);
-            wc.getPage(getIndex);
+            wc.getPage(requestReadOnly);
+            fail();
         } catch (FailingHttpStatusCodeException e) {
             assertEquals(403, e.getStatusCode());
         }
 
-        wc.login("administrator");
+        wc.withBasicApiToken(administrator);
         wc.getPage(request);
         assertFalse(mon.isEnabled());
 

diff --git a/test/src/test/java/hudson/model/AbstractProjectTest.java b/test/src/test/java/hudson/model/AbstractProjectTest.java
@@ -629,16 +629,16 @@ public void upstreamDownstreamExportApi() throws Exception {
                 grant(Item.READ).everywhere().to("alice").
                 grant(Item.READ).onItems(us).to("bob").
                 grant(Item.READ).onItems(ds).to("charlie"));
-        String api = j.createWebClient().login("alice").goTo(us.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
+        String api = j.createWebClient().withBasicCredentials("alice").goTo(us.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
         System.out.println(api);
         assertThat(api, containsString("downstream-project"));
-        api = j.createWebClient().login("alice").goTo(ds.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
+        api = j.createWebClient().withBasicCredentials("alice").goTo(ds.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
         System.out.println(api);
         assertThat(api, containsString("upstream-project"));
-        api = j.createWebClient().login("bob").goTo(us.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
+        api = j.createWebClient().withBasicCredentials("bob").goTo(us.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
         System.out.println(api);
         assertThat(api, not(containsString("downstream-project")));
-        api = j.createWebClient().login("charlie").goTo(ds.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
+        api = j.createWebClient().withBasicCredentials("charlie").goTo(ds.getUrl() + "api/json?pretty", null).getWebResponse().getContentAsString();
         System.out.println(api);
         assertThat(api, not(containsString("upstream-project")));
     }

diff --git a/test/src/test/java/hudson/model/ExecutorTest.java b/test/src/test/java/hudson/model/ExecutorTest.java
@@ -136,10 +136,16 @@ public void apiPermissions() throws Exception {
             grant(Jenkins.READ).everywhere().toEveryone().
             grant(Item.READ).onItems(publicProject).toEveryone().
             grant(Item.READ).onItems(secretProject).to("has-security-clearance"));
-        String api = j.createWebClient().login("has-security-clearance").goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString();
+
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.withBasicCredentials("has-security-clearance");
+        String api = wc.goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString();
         System.out.println(api);
         assertThat(api, allOf(containsString("public-project"), containsString("secret-project")));
-        api = j.createWebClient().login("regular-joe").goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString();
+
+        wc = j.createWebClient();
+        wc.withBasicCredentials("regular-joe");
+        api = wc.goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString();
         System.out.println(api);
         assertThat(api, allOf(containsString("public-project"), not(containsString("secret-project"))));
     }

diff --git a/test/src/test/java/hudson/model/ItemsTest.java b/test/src/test/java/hudson/model/ItemsTest.java
@@ -45,6 +45,7 @@
 import org.acegisecurity.context.SecurityContextHolder;
 import org.apache.commons.httpclient.HttpStatus;
 
+import org.junit.Before;
 import org.junit.Test;
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -123,6 +124,8 @@ public void allItems() throws Exception {
     // TODO would be more efficient to run these all as a single test case, but after a few Jetty seems to stop serving new content and new requests just hang.
 
     private void overwriteTargetSetUp() throws Exception {
+        User.getById("attacker", true);
+
         // A fully visible item:
         r.createFreeStyleProject("visible").setDescription("visible");
         // An item known to exist but not visible:
@@ -185,7 +188,7 @@ private enum OverwriteTactic {
                 JenkinsRule.WebClient wc = wc(r);
                 wc.getOptions().setRedirectEnabled(false);
                 wc.getOptions().setThrowExceptionOnFailingStatusCode(false); // redirect perversely counts as a failure
-                WebResponse webResponse = wc.getPage(new WebRequest(createCrumbedUrl(r, wc, "createItem?name=" + target + "&mode=hudson.model.FreeStyleProject"), HttpMethod.POST)).getWebResponse();
+                WebResponse webResponse = wc.getPage(new WebRequest(new URL(wc.getContextPath() + "createItem?name=" + target + "&mode=hudson.model.FreeStyleProject"), HttpMethod.POST)).getWebResponse();
                 if (webResponse.getStatusCode() != HttpStatus.SC_MOVED_TEMPORARILY) {
                     throw new FailingHttpStatusCodeException(webResponse);
                 }
@@ -198,7 +201,7 @@ private enum OverwriteTactic {
                 JenkinsRule.WebClient wc = wc(r);
                 wc.getOptions().setRedirectEnabled(false);
                 wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
-                WebResponse webResponse = wc.getPage(new WebRequest(createCrumbedUrl(r, wc, "createItem?name=" + target + "&mode=copy&from=dupe"), HttpMethod.POST)).getWebResponse();
+                WebResponse webResponse = wc.getPage(new WebRequest(new URL(wc.getContextPath() + "createItem?name=" + target + "&mode=copy&from=dupe"), HttpMethod.POST)).getWebResponse();
                 r.jenkins.getItem("dupe").delete();
                 if (webResponse.getStatusCode() != HttpStatus.SC_MOVED_TEMPORARILY) {
                     throw new FailingHttpStatusCodeException(webResponse);
@@ -209,7 +212,7 @@ private enum OverwriteTactic {
         REST_CREATE {
             @Override void run(JenkinsRule r, String target) throws Exception {
                 JenkinsRule.WebClient wc = wc(r);
-                WebRequest req = new WebRequest(createCrumbedUrl(r, wc, "createItem?name=" + target), HttpMethod.POST);
+                WebRequest req = new WebRequest(new URL(wc.getContextPath() + "createItem?name=" + target), HttpMethod.POST);
                 req.setAdditionalHeader("Content-Type", "application/xml");
                 req.setRequestBody("<project/>");
                 wc.getPage(req);
@@ -222,7 +225,7 @@ private enum OverwriteTactic {
                 JenkinsRule.WebClient wc = wc(r);
                 wc.getOptions().setRedirectEnabled(false);
                 wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
-                WebResponse webResponse = wc.getPage(new WebRequest(createCrumbedUrl(r, wc, "job/dupe/doRename?newName=" + target), HttpMethod.POST)).getWebResponse();
+                WebResponse webResponse = wc.getPage(new WebRequest(new URL(wc.getContextPath() + "job/dupe/doRename?newName=" + target), HttpMethod.POST)).getWebResponse();
                 if (webResponse.getStatusCode() != HttpStatus.SC_MOVED_TEMPORARILY) {
                     r.jenkins.getItem("dupe").delete();
                     throw new FailingHttpStatusCodeException(webResponse);
@@ -275,14 +278,7 @@ private enum OverwriteTactic {
         };
         abstract void run(JenkinsRule r, String target) throws Exception;
         private static final JenkinsRule.WebClient wc(JenkinsRule r) throws Exception {
-            return r.createWebClient().login("attacker");
-        }
-        // TODO replace with standard version once it is fixed to detect an existing query string
-        private static URL createCrumbedUrl(JenkinsRule r, JenkinsRule.WebClient wc, String relativePath) throws IOException {
-            CrumbIssuer issuer = r.jenkins.getCrumbIssuer();
-            String crumbName = issuer.getDescriptor().getCrumbRequestField();
-            String crumb = issuer.getCrumb(null);
-            return new URL(wc.getContextPath() + relativePath + (relativePath.contains("?") ? "&" : "?") + crumbName + "=" + crumb);
+            return r.createWebClient().withBasicApiToken("attacker");
         }
     }
 

diff --git a/test/src/test/java/hudson/model/JobTest.java b/test/src/test/java/hudson/model/JobTest.java
@@ -202,7 +202,7 @@ public String getDisplayName() {
         JenkinsRule.WebClient wc = j.createWebClient();
         wc.assertFails("job/testJob/", HttpURLConnection.HTTP_NOT_FOUND);
         wc.assertFails("jobCaseInsensitive/testJob/", HttpURLConnection.HTTP_NOT_FOUND);
-        wc.login("joe");  // Has Item.READ permission
+        wc.withBasicCredentials("joe");  // Has Item.READ permission
         // Verify we can access both URLs:
         wc.goTo("job/testJob/");
         wc.goTo("jobCaseInsensitive/TESTJOB/");
@@ -216,11 +216,14 @@ public String getDisplayName() {
         Item.EXTENDED_READ.setEnabled(true);
         try {
             wc.assertFails("job/testJob/config.xml", HttpURLConnection.HTTP_FORBIDDEN);
-            wc.login("alice");  // Has CONFIGURE and EXTENDED_READ permission
+
+            wc.withBasicApiToken(User.getById("alice", true));  // Has CONFIGURE and EXTENDED_READ permission
             tryConfigDotXml(wc, 500, "Both perms; should get 500");
-            wc.login("bob");  // Has only CONFIGURE permission (this should imply EXTENDED_READ)
+
+            wc.withBasicApiToken(User.getById("bob", true));  // Has only CONFIGURE permission (this should imply EXTENDED_READ)
             tryConfigDotXml(wc, 500, "Config perm should imply EXTENDED_READ");
-            wc.login("charlie");  // Has only EXTENDED_READ permission
+
+            wc.withBasicApiToken(User.getById("charlie", true));  // Has only EXTENDED_READ permission
             tryConfigDotXml(wc, 403, "No permission, should get 403");
         } finally {
             Item.EXTENDED_READ.setEnabled(saveEnabled);

diff --git a/test/src/test/java/hudson/model/PasswordParameterDefinitionTest.java b/test/src/test/java/hudson/model/PasswordParameterDefinitionTest.java
@@ -62,31 +62,38 @@ public class PasswordParameterDefinitionTest {
                 return true;
             }
         });
+
+        User admin = User.getById("admin", true);
+        User dev = User.getById("dev", true);
+
         JenkinsRule.WebClient wc = j.createWebClient();
         wc.getOptions().setThrowExceptionOnFailingStatusCode(false); // ParametersDefinitionProperty/index.jelly sends a 405 but really it is OK
         // Control case: admin can use default value.
-        j.submit(wc.login("admin").getPage(p, "build?delay=0sec").getFormByName("parameters"));
+        j.submit(wc.withBasicApiToken(admin).getPage(p, "build?delay=0sec").getFormByName("parameters"));
         j.waitUntilNoActivity();
         FreeStyleBuild b1 = p.getLastBuild();
         assertEquals(1, b1.getNumber());
         j.assertLogContains("I heard about a s3cr3t!", j.assertBuildStatusSuccess(b1));
+
         // Another control case: anyone can enter a different value.
-        HtmlForm form = wc.login("dev").getPage(p, "build?delay=0sec").getFormByName("parameters");
+        HtmlForm form = wc.withBasicApiToken(dev).getPage(p, "build?delay=0sec").getFormByName("parameters");
         HtmlPasswordInput input = form.getInputByName("value");
         input.setText("rumor");
         j.submit(form);
         j.waitUntilNoActivity();
         FreeStyleBuild b2 = p.getLastBuild();
         assertEquals(2, b2.getNumber());
         j.assertLogContains("I heard about a rumor!", j.assertBuildStatusSuccess(b2));
+
         // Test case: anyone can use default value.
-        j.submit(wc.login("dev").getPage(p, "build?delay=0sec").getFormByName("parameters"));
+        j.submit(wc.withBasicApiToken(dev).getPage(p, "build?delay=0sec").getFormByName("parameters"));
         j.waitUntilNoActivity();
         FreeStyleBuild b3 = p.getLastBuild();
         assertEquals(3, b3.getNumber());
         j.assertLogContains("I heard about a s3cr3t!", j.assertBuildStatusSuccess(b3));
+
         // Another control case: blank values.
-        form = wc.login("dev").getPage(p, "build?delay=0sec").getFormByName("parameters");
+        form = wc.withBasicApiToken(dev).getPage(p, "build?delay=0sec").getFormByName("parameters");
         input = form.getInputByName("value");
         input.setText("");
         j.submit(form);