From a377d2dea6c9551281058c2eb3b632b8727a1726 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard Date: Thu, 10 Oct 2019 16:09:03 +0200 Subject: [PATCH 1/6] fix crumb issue with Jenkins 2.176.2+/2.186+ Always pass the JSESSIONID in addition to the crumb See https://jenkins.io/security/advisory/2019-07-17/#SECURITY-626 Closes #67 --- .../jenkins/rest/domain/crumb/Crumb.java | 26 +++++++++++++++---- .../filters/JenkinsAuthenticationFilter.java | 5 ++-- .../jenkins/rest/parsers/CrumbParser.java | 16 ++++++++++-- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/cdancy/jenkins/rest/domain/crumb/Crumb.java b/src/main/java/com/cdancy/jenkins/rest/domain/crumb/Crumb.java index 6227feb7..b0125e7a 100644 --- a/src/main/java/com/cdancy/jenkins/rest/domain/crumb/Crumb.java +++ b/src/main/java/com/cdancy/jenkins/rest/domain/crumb/Crumb.java @@ -24,18 +24,34 @@ import com.cdancy.jenkins.rest.domain.common.Error; import com.cdancy.jenkins.rest.domain.common.ErrorsHolder; -import com.cdancy.jenkins.rest.domain.common.Value; import com.cdancy.jenkins.rest.JenkinsUtils; import com.google.auto.value.AutoValue; @AutoValue -public abstract class Crumb implements Value, ErrorsHolder { +public abstract class Crumb implements ErrorsHolder { + + @Nullable + public abstract String value(); + + @Nullable + public abstract String sessionIdCookie(); @SerializedNames({ "value", "errors" }) - public static Crumb create(@Nullable final String value, + public static Crumb create(final String value, + final List errors) { + + return create(value, null, errors); + } + + @SerializedNames({ "value", "sessionIdCookie" }) + public static Crumb create(final String value, final String sessionIdCookie) { + return create(value, sessionIdCookie, null); + } + + private static Crumb create(final String value, final String sessionIdCookie, final List errors) { - return new AutoValue_Crumb(value, - JenkinsUtils.nullToEmpty(errors)); + return new AutoValue_Crumb(JenkinsUtils.nullToEmpty(errors), value, + sessionIdCookie); } } diff --git a/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java b/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java index 80e8ade1..670b5a7a 100644 --- a/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java +++ b/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java @@ -39,7 +39,7 @@ public class JenkinsAuthenticationFilter implements HttpRequestFilter { private final JenkinsApi jenkinsApi; // key = Crumb, value = true if exception is ResourceNotFoundException false otherwise - private static volatile Pair crumbPair = null; + private static volatile Pair crumbPair = null; private static final String CRUMB_HEADER = "Jenkins-Crumb"; @Inject @@ -61,6 +61,7 @@ public HttpRequest filter(final HttpRequest request) throws HttpException { final Pair localCrumb = getCrumb(); if (localCrumb.getKey().value() != null) { builder.addHeader(CRUMB_HEADER, localCrumb.getKey().value()); + builder.addHeader(HttpHeaders.COOKIE, localCrumb.getKey().sessionIdCookie()); } else { if (localCrumb.getValue() == false) { throw new RuntimeException("Unexpected exception being thrown: error=" + localCrumb.getKey().errors().get(0)); @@ -87,7 +88,7 @@ private Pair getCrumb() { } return crumbValueInit; } - + // simple impl/copy of javafx.util.Pair private class Pair { private final A a; diff --git a/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java b/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java index ea1a6188..3d637f9b 100644 --- a/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java +++ b/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java @@ -23,6 +23,7 @@ import java.io.IOException; import javax.inject.Singleton; +import javax.ws.rs.core.HttpHeaders; import org.jclouds.http.HttpResponse; import org.jclouds.util.Strings2; @@ -38,8 +39,7 @@ public Crumb apply(final HttpResponse input) { final int statusCode = input.getStatusCode(); if (statusCode >= 200 && statusCode < 400) { try { - final String response = Strings2.toStringAndClose(input.getPayload().openStream()); - return Crumb.create(response.split(":")[1], null); + return Crumb.create(crumbValue(input), sessionIdCookie(input)); } catch (final IOException e) { throw new RuntimeException(input.getStatusLine(), e); } @@ -47,4 +47,16 @@ public Crumb apply(final HttpResponse input) { throw new RuntimeException(input.getStatusLine()); } } + + private static String crumbValue(HttpResponse input) throws IOException { + return Strings2.toStringAndClose(input.getPayload().openStream()) + .split(":")[1]; + } + + private static String sessionIdCookie(HttpResponse input) { + return input.getHeaders().get(HttpHeaders.SET_COOKIE).stream() + .filter(c -> c.startsWith("JSESSIONID")) + .findFirst() + .orElse(null); + } } From b468c87ef5ba545ee4811c1b64747852eb7c2260 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard Date: Thu, 10 Oct 2019 17:22:26 +0200 Subject: [PATCH 2/6] defect: make it work when there is no JSESSIONID My tests shown that there is no JSESSIONID in Jenkins 2.176.1 and as we cannot set cookie with null value, put an empty String in that case --- src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java b/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java index 3d637f9b..9e04cdb9 100644 --- a/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java +++ b/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java @@ -57,6 +57,6 @@ private static String sessionIdCookie(HttpResponse input) { return input.getHeaders().get(HttpHeaders.SET_COOKIE).stream() .filter(c -> c.startsWith("JSESSIONID")) .findFirst() - .orElse(null); + .orElse(""); } } From ece39ce68ac045a879a1f462600602e87a335174 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard Date: Thu, 10 Oct 2019 17:42:56 +0200 Subject: [PATCH 3/6] refactor: do not send cookie if not available This avoids passing empty cookie in requestq --- .../jenkins/rest/filters/JenkinsAuthenticationFilter.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java b/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java index 670b5a7a..9228f87d 100644 --- a/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java +++ b/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java @@ -17,6 +17,7 @@ package com.cdancy.jenkins.rest.filters; +import java.util.Optional; import javax.inject.Inject; import javax.inject.Singleton; @@ -61,7 +62,8 @@ public HttpRequest filter(final HttpRequest request) throws HttpException { final Pair localCrumb = getCrumb(); if (localCrumb.getKey().value() != null) { builder.addHeader(CRUMB_HEADER, localCrumb.getKey().value()); - builder.addHeader(HttpHeaders.COOKIE, localCrumb.getKey().sessionIdCookie()); + Optional.ofNullable(localCrumb.getKey().sessionIdCookie()) + .ifPresent(sessionId -> builder.addHeader(sessionId)); } else { if (localCrumb.getValue() == false) { throw new RuntimeException("Unexpected exception being thrown: error=" + localCrumb.getKey().errors().get(0)); From 68950f6bb08b7838ba82abe6a363aa1f84bfedb9 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard Date: Thu, 31 Oct 2019 15:38:01 +0100 Subject: [PATCH 4/6] refactor: introduce constant for the JSESSIONID cookie --- src/main/java/com/cdancy/jenkins/rest/JenkinsConstants.java | 2 ++ .../java/com/cdancy/jenkins/rest/parsers/CrumbParser.java | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/cdancy/jenkins/rest/JenkinsConstants.java b/src/main/java/com/cdancy/jenkins/rest/JenkinsConstants.java index b0263214..a5259744 100644 --- a/src/main/java/com/cdancy/jenkins/rest/JenkinsConstants.java +++ b/src/main/java/com/cdancy/jenkins/rest/JenkinsConstants.java @@ -41,6 +41,8 @@ public class JenkinsConstants { public static final String OPTIONAL_FOLDER_PATH_PARAM = "optionalFolderPath"; + public static final String JENKINS_COOKIES_JSESSIONID = "JSESSIONID"; + protected JenkinsConstants() { throw new UnsupportedOperationException("Purposefully not implemented"); } diff --git a/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java b/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java index 9e04cdb9..92853ac7 100644 --- a/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java +++ b/src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java @@ -17,6 +17,8 @@ package com.cdancy.jenkins.rest.parsers; +import static com.cdancy.jenkins.rest.JenkinsConstants.JENKINS_COOKIES_JSESSIONID; + import com.cdancy.jenkins.rest.domain.crumb.Crumb; import com.google.common.base.Function; @@ -55,7 +57,7 @@ private static String crumbValue(HttpResponse input) throws IOException { private static String sessionIdCookie(HttpResponse input) { return input.getHeaders().get(HttpHeaders.SET_COOKIE).stream() - .filter(c -> c.startsWith("JSESSIONID")) + .filter(c -> c.startsWith(JENKINS_COOKIES_JSESSIONID)) .findFirst() .orElse(""); } From 0fe36fb899b186f61e109f8597996bfb051f1e29 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard Date: Fri, 13 Dec 2019 10:07:19 +0100 Subject: [PATCH 5/6] Correctly pass the JSESSIONID cookie Fix regression introduced by 9d991e2d --- .../jenkins/rest/filters/JenkinsAuthenticationFilter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java b/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java index 9228f87d..b743d08d 100644 --- a/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java +++ b/src/main/java/com/cdancy/jenkins/rest/filters/JenkinsAuthenticationFilter.java @@ -63,7 +63,7 @@ public HttpRequest filter(final HttpRequest request) throws HttpException { if (localCrumb.getKey().value() != null) { builder.addHeader(CRUMB_HEADER, localCrumb.getKey().value()); Optional.ofNullable(localCrumb.getKey().sessionIdCookie()) - .ifPresent(sessionId -> builder.addHeader(sessionId)); + .ifPresent(sessionId -> builder.addHeader(HttpHeaders.COOKIE, sessionId)); } else { if (localCrumb.getValue() == false) { throw new RuntimeException("Unexpected exception being thrown: error=" + localCrumb.getKey().errors().get(0)); From d96396906924a8b54464492a6309e092c55b0f64 Mon Sep 17 00:00:00 2001 From: Thomas Bouffard Date: Fri, 13 Dec 2019 15:55:29 +0100 Subject: [PATCH 6/6] Dockerfile: bump Jenkins to 2.190.3 --- src/main/docker/Dockerfile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/docker/Dockerfile b/src/main/docker/Dockerfile index 387e9c22..26b5f722 100644 --- a/src/main/docker/Dockerfile +++ b/src/main/docker/Dockerfile @@ -1,5 +1,4 @@ -# api calls 403 error with jenkins 2.176.2+/2.186+ because of security changes: https://jenkins.io/security/advisory/2019-07-17/#SECURITY-626 -ARG jenkins_tag=2.176.1-alpine +ARG jenkins_tag=2.190.3-alpine FROM jenkins/jenkins:$jenkins_tag