Skip to content
Permalink
Browse files
[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 :)
  • Loading branch information
Wadeck authored and oleg-nenashev committed Dec 16, 2017
1 parent 1270ba3 commit 814d202716a6c61c7d371c6a62755d296fe199a5
Showing with 448 additions and 165 deletions.
  1. +53 −0 core/src/main/java/jenkins/security/ApiCrumbExclusion.java
  2. +5 −3 core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java
  3. +1 −1 test/pom.xml
  4. +2 −1 test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java
  5. +11 −8 test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java
  6. +4 −4 test/src/test/java/hudson/model/AbstractProjectTest.java
  7. +8 −2 test/src/test/java/hudson/model/ExecutorTest.java
  8. +8 −12 test/src/test/java/hudson/model/ItemsTest.java
  9. +7 −4 test/src/test/java/hudson/model/JobTest.java
  10. +11 −4 test/src/test/java/hudson/model/PasswordParameterDefinitionTest.java
  11. +29 −7 test/src/test/java/hudson/model/ProjectTest.java
  12. +12 −6 test/src/test/java/hudson/model/QueueTest.java
  13. +2 −2 test/src/test/java/hudson/model/UserTest.java
  14. +1 −1 test/src/test/java/hudson/model/ViewTest.java
  15. +9 −3 test/src/test/java/hudson/security/ExtendedReadPermissionTest.java
  16. +1 −0 test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java
  17. +2 −1 test/src/test/java/hudson/security/LoginTest.java
  18. +13 −15 test/src/test/java/hudson/util/RobustReflectionConverterTest.java
  19. +16 −9 test/src/test/java/jenkins/model/JenkinsTest.java
  20. +146 −0 test/src/test/java/jenkins/security/ApiCrumbExclusionTest.java
  21. +0 −1 test/src/test/java/jenkins/security/ApiTokenPropertyTest.java
  22. +55 −49 test/src/test/java/jenkins/security/BasicHeaderProcessorTest.java
  23. +1 −0 test/src/test/java/jenkins/security/Security380Test.java
  24. +50 −31 test/src/test/java/lib/form/PasswordTest.java
  25. +1 −1 test/src/test/resources/lib/form/PasswordTest/{test1.jelly → SecretNotPlainText/index.jelly}
@@ -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;
}
}
@@ -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;
}
@@ -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>
@@ -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();
@@ -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());

@@ -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")));
}
@@ -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"))));
}
@@ -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");
}
}

@@ -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);
@@ -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);

0 comments on commit 814d202

Please sign in to comment.