fix crumb issue with Jenkins 2.176.2+/2.186+ #70

tbouffard · 2019-10-10T15:07:06Z

Always pass the JSESSIONID in addition to the crumb
See https://jenkins.io/security/advisory/2019-07-17/#SECURITY-626

closes #67

Implementation note
I am not used to playing with AutoValue and jclouds, so don't hesitate to spot any bad practices on these areas 😸

src/main/java/com/cdancy/jenkins/rest/domain/crumb/Crumb.java

cdancy · 2019-10-10T15:36:25Z

src/main/java/com/cdancy/jenkins/rest/parsers/CrumbParser.java

+
+    private static String sessionIdCookie(HttpResponse input) {
+        return input.getHeaders().get(HttpHeaders.SET_COOKIE).stream()
+            .filter(c -> c.startsWith("JSESSIONID"))


Can we turn JSESSIONID into a static final String constant and put it somewhere that makes the most sense? Believe there is a static-variables class around here somewhere

I found the com.cdancy.jenkins.rest.JenkinsConstants class. Is that the one you had in mind?

Yeah exactly

done, see d0a16f2

tbouffard · 2019-10-11T11:21:33Z

Thanks @cdancy for the quick feedbacks, I will manage them soon
I have done some extra testing and the current implementation seems not working anymore after my 2 last commits. I am going to review them and come back to you when the integration tests pass

cdancy · 2019-10-11T13:11:43Z

@tbouffard sounds good and thanks! Let me know if you need any help one way or another.

cdancy · 2019-10-31T16:29:00Z

@tbouffard have you tested the impl yet? Are you happy with things? Any gotchas one way or another?

tbouffard · 2019-10-31T17:21:01Z

I didn't have much time to work on this PR except to ensure that I have a correct Jenkins instance for integration tests (see #72).
I still have a failling integration test (and dependent tests are then skipped) when running Jenkins 2.176.1 with the new implementation
I currently can't provide the error here but I will share it on the begining of next week

tbouffard · 2019-12-12T17:34:08Z

I have some time today to work on this. The issue I mentionned in #70 (comment) was the one seen on #75
Here my tests results as of today

On Jenkins 2.164.3 without fix (master commit 401f4f3)
no CSRF: a single test is failing when retrieving the Crumb. This is the normal behaviour as in this case, no Crumb is available

java.lang.AssertionError: expected object to not be null
	at org.testng.Assert.fail(Assert.java:93)
	at org.testng.Assert.assertNotNull(Assert.java:422)
	at org.testng.Assert.assertNotNull(Assert.java:407)
	at com.cdancy.jenkins.rest.features.CrumbIssuerApiLiveTest.testGetCrumb(CrumbIssuerApiLiveTest.java:34)

16:43:47.721 [Test worker] DEBUG org.jclouds.rest.internal.InvokeHttpMethod - >> invoking crumb-issuer:crumb
16:43:47.726 [Test worker] DEBUG org.jclouds.http.internal.JavaUrlHttpCommandExecutorService - Sending request 559347004: GET http://127.0.0.1:8080/crumbIssuer/api/xml?xpath=concat%28//crumbRequestField,%22%3A%22,//crumb%29 HTTP/1.1
16:43:47.726 [Test worker] DEBUG jclouds.headers - >> GET http://127.0.0.1:8080/crumbIssuer/api/xml?xpath=concat%28//crumbRequestField,%22%3A%22,//crumb%29 HTTP/1.1
16:43:47.727 [Test worker] DEBUG jclouds.headers - >> Accept: text/plain
16:43:47.727 [Test worker] DEBUG jclouds.headers - >> Authorization: Basic YWRtaW46YWRtaW4=
16:43:47.820 [Test worker] DEBUG org.jclouds.http.internal.JavaUrlHttpCommandExecutorService - Receiving response 559347004: HTTP/1.1 404 Not Found
16:43:47.820 [Test worker] DEBUG jclouds.headers - << HTTP/1.1 404 Not Found
16:43:47.820 [Test worker] DEBUG jclouds.headers - << Server: Jetty(9.4.z-SNAPSHOT)
16:43:47.821 [Test worker] DEBUG jclouds.headers - << X-Content-Type-Options: nosniff
16:43:47.821 [Test worker] DEBUG jclouds.headers - << Date: Thu, 12 Dec 2019 15:43:47 GMT

2.164.3 with the fix (commit d0a16f2)

with CSRF: OK
without CSRF: OK (same Crumb test failing as without the fix)

2.190.3 with the fix (commit d0a16f2)

with CSRF: sessionId is not passed. This is strange I remember this worked with 2.176.3. New tests required
without CSRF: OK (same Crumb test failing as without the fix)

cdancy · 2019-12-12T19:30:08Z

@tbouffard let me know if you need any help here. Would be great to have this in and "just work" across the board.

tbouffard · 2019-12-13T09:33:43Z

@cdancy I have finally figured out the cause of the failure with Jenkins 2.176.2+, see
5d70101, so this should be ok now on Jenkins 2.190.3 with CSRF protection enabled

Can you run the tests on your side to ensure I didn't miss anything with my own testing?
I can also rebase the PR on the latest master commit to make Travis run integration tests and then update the Jenkins version to the latest available LTS.

cdancy · 2019-12-13T14:14:21Z

@tbouffard yes please rebase off of master and I'll run things as here as well.

Always pass the JSESSIONID in addition to the crumb See https://jenkins.io/security/advisory/2019-07-17/#SECURITY-626 Closes cdancy#67

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

This avoids passing empty cookie in requestq

Fix regression introduced by 9d991e2

tbouffard · 2019-12-13T14:27:48Z

Rebase done. If Travis tests pass, I will bump the jenkins version used in the Docker container to the latest one (2.190.3)

cdancy

LGTM

cdancy · 2019-12-16T16:50:01Z

@tbouffard thanks for the fix/enhancement!

cdancy · 2019-12-16T17:17:36Z

@tbouffard release v0.0.22 has been released with this change.

tbouffard · 2019-12-17T13:06:30Z

@cdancy Great and thanks for the support. Very helpful to make this contribution complete. I hope to be quicker next time 😄

cdancy · 2019-12-17T14:35:03Z

@tbouffard no worries. We all have lives and day-jobs to attend to. Just happy for the contribution either way.

cdancy reviewed Oct 10, 2019

View reviewed changes

src/main/java/com/cdancy/jenkins/rest/domain/crumb/Crumb.java Show resolved Hide resolved

cdancy reviewed Oct 10, 2019

View reviewed changes

This was referenced Oct 25, 2019

[Question] jenkins instance requirements for integration tests #72

Closed

POST API calls '403 No valid crumb was included in the request' #67

Closed

cdancy assigned martinda and cdancy Dec 12, 2019

cdancy added the bug label Dec 12, 2019

tbouffard marked this pull request as ready for review December 13, 2019 09:24

tbouffard added 5 commits December 13, 2019 15:23

fix crumb issue with Jenkins 2.176.2+/2.186+

a377d2d

Always pass the JSESSIONID in addition to the crumb See https://jenkins.io/security/advisory/2019-07-17/#SECURITY-626 Closes cdancy#67

defect: make it work when there is no JSESSIONID

b468c87

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

refactor: do not send cookie if not available

ece39ce

This avoids passing empty cookie in requestq

refactor: introduce constant for the JSESSIONID cookie

68950f6

Correctly pass the JSESSIONID cookie

0fe36fb

Fix regression introduced by 9d991e2

tbouffard force-pushed the issue_crumb_and_sessionId_with_Jenkins_2.176.2+ branch from 5d70101 to 0fe36fb Compare December 13, 2019 14:24

Dockerfile: bump Jenkins to 2.190.3

d963969

cdancy self-requested a review December 16, 2019 16:49

cdancy approved these changes Dec 16, 2019

View reviewed changes

cdancy merged commit 6077aa4 into cdancy:master Dec 16, 2019

cdancy added this to the v0.0.22 milestone Dec 16, 2019

tbouffard deleted the issue_crumb_and_sessionId_with_Jenkins_2.176.2+ branch December 17, 2019 13:06

ignite1771 mentioned this pull request Jan 13, 2021

Necessary crumb support after Jenkins 2.176.2+ jenkinsci/java-client-api#477

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix crumb issue with Jenkins 2.176.2+/2.186+ #70

fix crumb issue with Jenkins 2.176.2+/2.186+ #70

tbouffard commented Oct 10, 2019 •

edited

Loading

cdancy Oct 10, 2019

tbouffard Oct 15, 2019

cdancy Oct 16, 2019

tbouffard Dec 12, 2019

tbouffard commented Oct 11, 2019 •

edited

Loading

cdancy commented Oct 11, 2019

cdancy commented Oct 31, 2019

tbouffard commented Oct 31, 2019 •

edited

Loading

tbouffard commented Dec 12, 2019

cdancy commented Dec 12, 2019

tbouffard commented Dec 13, 2019 •

edited

Loading

cdancy commented Dec 13, 2019

tbouffard commented Dec 13, 2019

cdancy left a comment

cdancy commented Dec 16, 2019

cdancy commented Dec 16, 2019

tbouffard commented Dec 17, 2019

cdancy commented Dec 17, 2019

fix crumb issue with Jenkins 2.176.2+/2.186+ #70

fix crumb issue with Jenkins 2.176.2+/2.186+ #70

Conversation

tbouffard commented Oct 10, 2019 • edited Loading

cdancy Oct 10, 2019

Choose a reason for hiding this comment

tbouffard Oct 15, 2019

Choose a reason for hiding this comment

cdancy Oct 16, 2019

Choose a reason for hiding this comment

tbouffard Dec 12, 2019

Choose a reason for hiding this comment

tbouffard commented Oct 11, 2019 • edited Loading

cdancy commented Oct 11, 2019

cdancy commented Oct 31, 2019

tbouffard commented Oct 31, 2019 • edited Loading

tbouffard commented Dec 12, 2019

cdancy commented Dec 12, 2019

tbouffard commented Dec 13, 2019 • edited Loading

cdancy commented Dec 13, 2019

tbouffard commented Dec 13, 2019

cdancy left a comment

Choose a reason for hiding this comment

cdancy commented Dec 16, 2019

cdancy commented Dec 16, 2019

tbouffard commented Dec 17, 2019

cdancy commented Dec 17, 2019

tbouffard commented Oct 10, 2019 •

edited

Loading

tbouffard commented Oct 11, 2019 •

edited

Loading

tbouffard commented Oct 31, 2019 •

edited

Loading

tbouffard commented Dec 13, 2019 •

edited

Loading