Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SECURITY-3075] getAggregateSecretPattern to fail when run inside agent JVM #260

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

jglick
Copy link
Member

@jglick jglick commented Jul 26, 2023

Cleans up after #239, especially relevant now given jenkinsci/workflow-durable-task-step-plugin#323.

@jglick jglick added the bug label Jul 26, 2023
@jglick jglick requested review from timja, daniel-beck and a team July 26, 2023 17:36
@jglick
Copy link
Member Author

jglick commented Jul 26, 2023

🤔 Given jenkinsci/workflow-api-plugin#290 I should probably write a test to make sure that the ISE is not just caught and ignored.

@jglick jglick marked this pull request as draft July 26, 2023 18:06
Comment on lines +63 to +65
/* Not currently ensured:
r.assertLogNotContains("s3cr3t", b);
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

At first I thought I might have getAggregateSecretPattern log a warning but return Pattern.compile(".+") to guarantee that the secret (along with everything else) is masked. But this just seemed confusing, and the stack trace would be shown only in the agent log, not the controller log nor build console. So I settled on just throwing the exception, which results in a warning in the agent log but also a clear error in the build log:

Started
[Pipeline] Start of Pipeline
[Pipeline] node
Running on remote in /…/agent-work-dirs/remote/workspace/p
[Pipeline] {
[Pipeline] isUnix
[Pipeline] sh
java.lang.IllegalStateException: Not running on the Jenkins controller JVM
	at jenkins.util.JenkinsJVM.checkJenkinsJVM(JenkinsJVM.java:46)
	at org.jenkinsci.plugins.credentialsbinding.masking.SecretPatterns.getAggregateSecretPattern(SecretPatterns.java:57)
	at org.jenkinsci.plugins.credentialsbinding.masking.SecretPatternsTest$BadMasker.decorate(SecretPatternsTest.java:73)
	at org.jenkinsci.plugins.workflow.log.TaskListenerDecorator.decorateAll(TaskListenerDecorator.java:231)
	at org.jenkinsci.plugins.workflow.log.TaskListenerDecorator$DecoratedTaskListener.getLogger(TaskListenerDecorator.java:269)
	at org.jenkinsci.plugins.workflow.log.TaskListenerDecorator$CloseableTaskListener.getLogger(TaskListenerDecorator.java:306)
	at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$Execution$NewlineSafeTaskListener.getLogger(DurableTaskStep.java:458)
	at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$HandlerImpl.output(DurableTaskStep.java:723)
	at org.jenkinsci.plugins.durabletask.FileMonitoringTask$Watcher.run(FileMonitoringTask.java:611)
	at jenkins.security.ImpersonatingScheduledExecutorService$1.run(ImpersonatingScheduledExecutorService.java:67)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
+ echo do not look at s3cr3t please
do not look at s3cr3t please
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
Finished: SUCCESS

This makes it obvious to both end users and the plugin developer that something is broken in the plugin, so it should lead to the bad masking actually getting fixed.

Comment on lines +73 to +74
Pattern pattern = SecretPatterns.getAggregateSecretPattern(Set.of("s3cr3t"));
return new SecretPatterns.MaskingOutputStream(logger, () -> pattern, "UTF-8");
Copy link
Member Author

Choose a reason for hiding this comment

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

As opposed to

Suggested change
Pattern pattern = SecretPatterns.getAggregateSecretPattern(Set.of("s3cr3t"));
return new SecretPatterns.MaskingOutputStream(logger, () -> pattern, "UTF-8");
return new SecretPatterns.MaskingOutputStream(logger, () -> SecretPatterns.getAggregateSecretPattern(Set.of("s3cr3t")), "UTF-8");

as in the code prior to jenkinsci/azure-keyvault-plugin@f46b7fa for example. In this case the exception is thrown from a different spot and handled differently:

Started
[Pipeline] Start of Pipeline
[Pipeline] node
Running on remote in /…/agent-work-dirs/remote/workspace/p
[Pipeline] {
[Pipeline] isUnix
[Pipeline] sh
java.lang.IllegalStateException: Not running on the Jenkins controller JVM
	at jenkins.util.JenkinsJVM.checkJenkinsJVM(JenkinsJVM.java:46)
	at org.jenkinsci.plugins.credentialsbinding.masking.SecretPatterns.getAggregateSecretPattern(SecretPatterns.java:57)
	at org.jenkinsci.plugins.credentialsbinding.masking.SecretPatternsTest$BadMasker.lambda$decorate$0(SecretPatternsTest.java:73)
	at org.jenkinsci.plugins.credentialsbinding.masking.SecretPatterns$MaskingOutputStream.eol(SecretPatterns.java:93)
	at hudson.console.LineTransformationOutputStream.eol(LineTransformationOutputStream.java:61)
	at hudson.console.LineTransformationOutputStream.write(LineTransformationOutputStream.java:57)
	at java.base/java.io.PrintStream.write(PrintStream.java:528)
	at java.base/java.io.FilterOutputStream.write(FilterOutputStream.java:87)
	at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$Execution$NewlineSafeTaskListener$1.write(DurableTaskStep.java:462)
	at java.base/java.io.FilterOutputStream.write(FilterOutputStream.java:137)
	at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$Execution$NewlineSafeTaskListener$1.write(DurableTaskStep.java:466)
	at org.apache.commons.io.IOUtils.copyLarge(IOUtils.java:1310)
	at org.apache.commons.io.IOUtils.copy(IOUtils.java:978)
	at org.apache.commons.io.IOUtils.copyLarge(IOUtils.java:1282)
	at org.apache.commons.io.IOUtils.copy(IOUtils.java:953)
	at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$HandlerImpl.output(DurableTaskStep.java:737)
	at org.jenkinsci.plugins.durabletask.FileMonitoringTask$Watcher.run(FileMonitoringTask.java:611)
	at jenkins.security.ImpersonatingScheduledExecutorService$1.run(ImpersonatingScheduledExecutorService.java:67)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)

with the agent printing a warning

WARNING: giving up on watching …

@jglick jglick marked this pull request as ready for review July 27, 2023 19:42
@jglick jglick merged commit 861c06d into jenkinsci:master Jul 27, 2023
@jglick jglick deleted the SECURITY-3075-strict branch July 27, 2023 22:14
@basil
Copy link
Member

basil commented Aug 22, 2023

Caused jenkinsci/hashicorp-vault-plugin#311 and JENKINS-71788.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants