Skip to content

Conversation

@jglick
Copy link
Member

@jglick jglick commented Oct 18, 2018

Studying JENKINS-54133. Safe to merge, but does not do anything at the moment.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very helpful to have a test that demonstrates the crux of the issue, thanks!

@jglick
Copy link
Member Author

jglick commented Oct 29, 2018

The smart approach feels terribly hacky.

Perhaps. But consider the ant example, and suppose that we are generating a gigantic log from an Ant build (such as would happen when you type ant in the NetBeans monorepo, for example) and logs are being streamed from an agent to some external sink. Using solely a ConsoleAnnotatorFactory is not really practical because these apply globally to all parts of all builds, and this is matching a rather generic pattern like the three lines consisting of target names in

…
jar-ml:

netbeans:
  [genlist] Generating Auto Update information for org.openide.awt
   [repeat] 
   [repeat] all-api.progress:
     [echo] Building platform/api.progress...

init-tasks:
…

that is only identifiable as “Ant target names” due to the context that this is coming from a process running Ant and not something completely unrelated. So if we go the ConsoleNote route, we need to emit these notes from the agent side. But we cannot trust the agent JVM to sign notes in general, because they are often (though not in this case) written to render instance-supplied HTML or other XSS-oriented data, and we assume that any agent JVM might have been compromised.

So, unless a very different approach is taken, we need to have the master JVM create and sign the notes in advance, then serve them to the agent callable.

@jglick jglick changed the title [JENKINS-54133] Reproduced a minimal variant of this scenario in a test [JENKINS-54133] Document remote console note behavior in test Oct 29, 2018
@dwnusbaum
Copy link
Member

I agree that pre-generating the notes makes sense given the constraints, it just seems like the API was not really designed to support this use case cleanly. Given that it's the best option in some cases, I think updating the documentation like you did will be very helpful in case it comes up for anyone in the future.

@jglick
Copy link
Member Author

jglick commented Oct 30, 2018

[It] seems like the API was not really designed to support this use case cleanly

Indeed. It is a bit hard to say what the API was designed for. See jenkinsci/jenkins@2d7e0d9. There is a more detailed discussion in SECURITY-382.

Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, and I'm always happy to have more testcases -- especially when they include explanations and are easy to read like this one!

@svanoort svanoort merged commit 1c58490 into jenkinsci:master Oct 30, 2018
@jglick jglick deleted the remoteConsoleNotes-JENKINS-54133 branch October 31, 2018 15:32
jglick added a commit to jglick/jenkins that referenced this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants