[JENKINS-18114] Fixing CLI crumbs #3019

jglick · 2017-09-11T20:47:59Z

Amends #2315. The CLI client was already asking for a crumb. But Jenkins was not properly serving X-Jenkins alongside it, so it did not pass it back; but then Jenkins forgave it anyway! Simpler and clearer to just serve the expected header.

Proposed changelog entries

None should be needed.

@reviewbybees

…ctually serve it, from CrumbIssuer.RestrictedApi.

ghost · 2017-09-11T20:59:17Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

daniel-beck · 2017-09-13T06:05:22Z

Test failures seem related.

…tuff better checked by authentication(), and was failing since this fake client was not passing a crumb.

jglick · 2017-09-13T06:24:11Z

CLIActionTest.serveCliActionToAnonymousUserWithoutPermissions was meaningless. But CLIActionTest.authentication was real: the proposed change actually breaks the (admittedly dubious) use case of running CLI commands as anonymous when anonymous has no Overall/Read.

Perhaps we need to return to the crumb exclusion, and just drop the client code to send the crumb, which adds to the round-trip time; is not needed when there is an exclusion; and in master does not even work anyway.

jglick · 2017-09-13T06:28:13Z

(Note that FullDuplexHttpStream.tryToResolveRedirects does not work for an anonymous user without read access either, but this is just a convenience in case you have a noncanonical Jenkins URL.)

daniel-beck · 2017-09-13T06:32:32Z

admittedly dubious

This may be a symptom of a larger possible problem: Unless we want all UnprotectedRootActions to be CrumbExclusions, there probably should be a way for unauthorized users to get a crumb. Searching I now only found JENKINS-31515, but it's still something we should think about.

jglick · 2017-09-13T06:39:18Z

I do not think we can (safely) create a crumb for anonymous users. There is JENKINS-22474 which could allow us to delete most CrumbExclusions for RootActions. Arguably UnprotectedRootActions should also be excluded automatically.

…ccess, and just let the client stop asking for a crumb.

…ned demonstration of the HTTP Duplex transport.

jglick · 2017-09-13T06:56:48Z

cli/src/main/java/hudson/cli/CLI.java

@@ -549,7 +549,7 @@ public boolean verify(String s, SSLSession sslSession) {
                for (Handler h : Logger.getLogger("").getHandlers()) {
                    h.setLevel(level);
                }
-                for (Logger logger : new Logger[] {LOGGER, PlainCLIProtocol.LOGGER, Logger.getLogger("org.apache.sshd")}) { // perhaps also Channel
+                for (Logger logger : new Logger[] {LOGGER, FullDuplexHttpStream.LOGGER, PlainCLIProtocol.LOGGER, Logger.getLogger("org.apache.sshd")}) { // perhaps also Channel


Generally useful.

jglick · 2017-09-13T06:57:52Z

cli/src/main/java/hudson/cli/FullDuplexHttpStream.java

@@ -79,11 +85,10 @@ public FullDuplexHttpStream(URL base, String relativeTarget, String authorizatio

        URL target = new URL(this.base, relativeTarget);

-        CrumbData crumbData = new CrumbData();


As described in PR comments, this did not previously work, and given the existence of an exclusion, was not necessary anyway.

jglick · 2017-09-13T06:59:08Z

core/src/main/java/hudson/security/csrf/CrumbIssuer.java

@@ -193,6 +193,7 @@ public void validateCrumb(StaplerRequest request, String submittedCrumb) {
        }

        @Override public void doXml(StaplerRequest req, StaplerResponse rsp, @QueryParameter String xpath, @QueryParameter String wrapper, @QueryParameter String tree, @QueryParameter int depth) throws IOException, ServletException {
+            setHeaders(rsp);


Makes the crumb code work. No longer necessary for this PR, which deletes the code that called this endpoint from the context of the CLI (would make that code in old clients start working, which is fine but unnecessary); but generally seems wise to keep the behavior of RestrictedApi consistent with Api.

jglick · 2017-09-13T07:00:35Z

test/src/test/java/hudson/cli/CLIActionTest.java

@@ -122,16 +117,6 @@ public void serveCliActionToAnonymousUserWithoutPermissions() throws Exception {
        JenkinsRule.WebClient wc = j.createWebClient();
        // The behavior changed due to SECURITY-192. index page is no longer accessible to anonymous
        wc.assertFails("cli", HttpURLConnection.HTTP_FORBIDDEN);
-        // so we check the access by emulating the CLI connection post request


Actually would pass again given the restored crumb exclusion, but is anyway redundant given more comprehensive, and realistic, tests later added to the CLI system.

jglick · 2017-09-13T07:01:12Z

test/src/test/java/jenkins/util/FullDuplexHttpServiceTest.java

+    public LoggerRule logging = new LoggerRule().record(FullDuplexHttpService.class, Level.FINE).record(FullDuplexHttpStream.class, Level.FINE);
+
+    @Test
+    public void smokes() throws Exception {


Just for fun!

jglick · 2017-09-13T07:05:10Z

test/src/test/java/jenkins/util/FullDuplexHttpServiceTest.java

+
+    @Test
+    public void smokes() throws Exception {
+        logging.record("org.eclipse.jetty", Level.ALL);


I have been investigating failures of HTTP Duplex to work in test cases using Jetty 9.4.x. Symptoms are typically a failure to connect the upload side, as in JENKINS-43666.

(Starting from #2959 which picks up jenkinsci/jenkins-test-harness#63. Seems unrelated to #3011.)

How verbose is this? Intended to stay on ALL?

In practice Jetty logs everything at FINE.

jglick · 2017-09-13T13:22:20Z

@reviewbybees done

ghost · 2017-09-15T15:27:34Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev

🐝 IIUC

* [JENKINS-18114] The CLI client already asks for a crumb; we just to actually serve it, from CrumbIssuer.RestrictedApi. * serveCliActionToAnonymousUserWithoutPermissions() was checking some stuff better checked by authentication(), and was failing since this fake client was not passing a crumb. * Bring back CliCrumbExclusion, needed for anonymous use with no read access, and just let the client stop asking for a crumb. * Added FullDuplexHttpServiceTest; useful to have a simple, self-contained demonstration of the HTTP Duplex transport.

[JENKINS-18114] The CLI client already asks for a crumb; we just to a…

856cd92

…ctually serve it, from CrumbIssuer.RestrictedApi.

jglick requested a review from daniel-beck September 11, 2017 20:47

serveCliActionToAnonymousUserWithoutPermissions() was checking some s…

13b4d74

…tuff better checked by authentication(), and was failing since this fake client was not passing a crumb.

jglick added 2 commits September 13, 2017 02:46

Bring back CliCrumbExclusion, needed for anonymous use with no read a…

587ebbe

…ccess, and just let the client stop asking for a crumb.

Added FullDuplexHttpServiceTest; useful to have a simple, self-contai…

652b392

…ned demonstration of the HTTP Duplex transport.

jglick commented Sep 13, 2017

View reviewed changes

jglick mentioned this pull request Sep 13, 2017

Use a fixed number of selectors and acceptors regardless of CPU count jenkinsci/jenkins-test-harness#69

Merged

daniel-beck approved these changes Sep 13, 2017

View reviewed changes

jglick added the needs-more-reviews Complex change, which would benefit from more eyes label Sep 13, 2017

This was referenced Sep 13, 2017

Make LoggerRule use the nicer formatter developed for support-core jenkinsci/jenkins-test-harness#68

Merged

jenkins-test-harness 2.24 → 2.26 #3024

Merged

oleg-nenashev approved these changes Sep 15, 2017

View reviewed changes

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Sep 15, 2017

oleg-nenashev merged commit 5e84c54 into jenkinsci:master Sep 22, 2017

jglick deleted the CLI-crumb-JENKINS-18114 branch September 25, 2017 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-18114] Fixing CLI crumbs #3019

[JENKINS-18114] Fixing CLI crumbs #3019

jglick commented Sep 11, 2017

ghost commented Sep 11, 2017

daniel-beck commented Sep 13, 2017

jglick commented Sep 13, 2017

jglick commented Sep 13, 2017

daniel-beck commented Sep 13, 2017

jglick commented Sep 13, 2017

jglick Sep 13, 2017

jglick Sep 13, 2017

jglick Sep 13, 2017

jglick Sep 13, 2017

jglick Sep 13, 2017

jglick Sep 13, 2017

jglick Sep 13, 2017

daniel-beck Sep 13, 2017

jglick Sep 15, 2017

jglick commented Sep 13, 2017

ghost commented Sep 15, 2017

oleg-nenashev left a comment

		@@ -79,11 +85,10 @@ public FullDuplexHttpStream(URL base, String relativeTarget, String authorizatio

		URL target = new URL(this.base, relativeTarget);

		CrumbData crumbData = new CrumbData();

[JENKINS-18114] Fixing CLI crumbs #3019

[JENKINS-18114] Fixing CLI crumbs #3019

Conversation

jglick commented Sep 11, 2017

Proposed changelog entries

ghost commented Sep 11, 2017

daniel-beck commented Sep 13, 2017

jglick commented Sep 13, 2017

jglick commented Sep 13, 2017

daniel-beck commented Sep 13, 2017

jglick commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Sep 13, 2017

ghost commented Sep 15, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment