Security-2401 Fix credentials leakage to Splunk#17
Conversation
…(FileLogStorage) to send build console output to Splunk as it's building the log file
|
@jglick Have you gotten a chance to take a look? |
jglick
left a comment
There was a problem hiding this comment.
I am afraid I do not have time to help much.
| Code copied over from | ||
| https://github.com/jenkinsci/workflow-api-plugin/blob/master/src/main/java/org/jenkinsci/plugins/workflow/log/FileLogStorage.java |
There was a problem hiding this comment.
I do not believe you need to do this. Try delegating to FileLogStorage instead.
There was a problem hiding this comment.
@jglick I originally tried just extending the FileLogStorage class, however the problem I faced was that I need access to the openStorages to override the forFile(File) method, but openStorages is private.
There was a problem hiding this comment.
And the feature isn't something that is the responsibility of FileLogStorage so implementing it as part of FileLogStorage doesn't make sense either, which is why I was trying to create a separate class that was responsible for this "piping data to Splunk" feature.
There was a problem hiding this comment.
I said delegating to, not extending.
There was a problem hiding this comment.
The reason we don't delegate this feature to FileLogStorage is because this feature of "piping data to Splunk" isn't really a responsibility of FileLogStorage, and not everyone using FileLogStorage would necessarily want that.
The other way I can think without duplicating code is to make FileLogStorage more dynamic such that we can update just a few of its methods(?)
The key changes we need are adding a source field, and overriding the overallListener() and nodeListener() methods to take a TeeOutputStream built on top of a IndexOutputStream instead of simply an IndexOutputStream. This is what's responsible for piping the data to Splunk.
There was a problem hiding this comment.
not everyone using
FileLogStoragewould necessarily want that
Fine, forBuild can decide on a case-by-case basis.
make
FileLogStoragemore dynamic such that we can update just a few of its methods
Again, I do not think you need that. Just call FileLogStorage.forFile, and have your impl delegate overallLog & stepLog directly, and overallListener & nodeListener using a TeeOutputStream. The stock log file will get used for reading and writing, but you also can send all text to a Splunk sink. Could be made a bit easier to do this with some convenience APIs in workflow-api but should not be necessary.
There was a problem hiding this comment.
(https://en.wikipedia.org/wiki/Delegation_pattern in case the term was not clear.)
There was a problem hiding this comment.
TeeOutputStream
I looked into this a while back for something else, but I think you need a TeeTaskListener and TeePrintStream due to the APIs involved rather than a TeeOutputStream. Anyways, here is an untested sketch of an implementation of LogStorage that delegates to another implementation of LogStorage while also copying all writes to a specified BuildListener in case it is useful. You would use this along with your own implementation of LogStorageFactory to make Pipeline builds use your instance of DelegatingLogStorage for logging.
package org.jenkinsci.plugins.workflow.log;
import hudson.console.AnnotatedLargeText;
import hudson.model.BuildListener;
import hudson.model.TaskListener;
import java.io.File;
import java.io.IOException;
import java.io.PrintStream;
import java.util.Locale;
import javax.annotation.Nonnull;
import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
/**
* An implementation of {@LogStorage} that delegates all reads to a specified {@LogStorage} delegate while
* sending writes to both the delegate and another implementation of {@link BuildListener}.
*/
public class DelegatingLogStorage implements LogStorage {
private final LogStorage delegate;
private final BuildListener teeListener;
private DelegatingLogStorage(LogStorage delegate, BuildListener teeListener) {
this.delegate = delegate;
this.teeListener = teeListener;
}
@Override
public BuildListener overallListener() throws IOException, InterruptedException {
return new TeeBuildListener(delegate.overallListener(), teeListener);
}
@Override
public TaskListener nodeListener(FlowNode node) throws IOException, InterruptedException {
return new TeeBuildListener(delegate.nodeListener(node), teeListener);
}
@Override
public AnnotatedLargeText<FlowExecutionOwner.Executable> overallLog(FlowExecutionOwner.Executable build, boolean complete) {
return delegate.overallLog(build, complete);
}
@Override
public AnnotatedLargeText<FlowNode> stepLog(FlowNode node, boolean complete) {
return delegate.stepLog(node, complete);
}
@Override
public File getLogFile(@Nonnull FlowExecutionOwner.Executable build, boolean complete) {
return delegate.getLogFile(build, complete);
}
/* TODO: How should getCharset be implemented? */
private static class TeeBuildListener implements BuildListener {
private final PrintStream teeStream;
public TeeBuildListener(TaskListener main, TaskListener branch) {
teeStream = new TeePrintStream(main.getLogger(), branch.getLogger());
}
@Override
public PrintStream getLogger() {
return teeStream;
}
}
private static class TeePrintStream extends PrintStream {
private final PrintStream branch;
public TeePrintStream(PrintStream main, PrintStream branch) {
super(main);
this.branch = branch;
}
@Override
public boolean checkError() {
return super.checkError() || branch.checkError();
}
@Override
public PrintStream format(String format, Object... args) {
super.format(format, args);
branch.format(format, args);
return this;
}
@Override
public PrintStream format(Locale l, String format, Object... args) {
super.format(l, format, args);
branch.format(l, format, args);
return this;
}
@Override
public void print(boolean b) {
super.print(b);
branch.print(b);
}
@Override
public void print(char c) {
super.print(c);
branch.print(c);
}
@Override
public void print(int i) {
super.print(i);
branch.print(i);
}
@Override
public void print(long l) {
super.print(l);
branch.print(l);
}
@Override
public void print(float f) {
super.print(f);
branch.print(f);
}
@Override
public void print(double d) {
super.print(d);
branch.print(d);
}
@Override
public void print(char[] s) {
super.print(s);
branch.print(s);
}
@Override
public void print(String s) {
super.print(s);
branch.print(s);
}
@Override
public void print(Object o) {
super.print(o);
branch.print(o);
}
@Override
public void println() {
super.println();
branch.println();
}
@Override
public void println(boolean b) {
super.println(b);
branch.println(b);
}
@Override
public void println(char c) {
super.println(c);
branch.println(c);
}
@Override
public void println(int i) {
super.println(i);
branch.println(i);
}
@Override
public void println(long l) {
super.println(l);
branch.println(l);
}
@Override
public void println(float f) {
super.println(f);
branch.println(f);
}
@Override
public void println(double d) {
super.println(d);
branch.println(d);
}
@Override
public void println(char[] s) {
super.println(s);
branch.println(s);
}
@Override
public void println(String s) {
super.println(s);
branch.println(s);
}
@Override
public void println(Object o) {
super.println(o);
branch.println(o);
}
@Override
public void write(int b) {
super.write(b);
branch.write(b);
}
@Override
public void write(byte[] buf, int off, int length) {
super.write(buf, off, length);
branch.write(buf, off, length);
}
@Override
public void close() {
super.close();
branch.close();
}
@Override
public void flush() {
super.flush();
branch.flush();
}
}
}
(maybe you can use TeeOutputStream with an implementation of TeeBuildListener like the following, but I was not sure about it from an encoding standpoint):
private static class TeeBuildListener implements BuildListener {
private final PrintStream teeStream;
public TeeBuildListener(TaskListener main, TaskListener branch) {
teeStream = new PrintStream(new TeeOutputStream(main.getLogger(), branch.getLogger()), false);
}
@Override
public PrintStream getLogger() {
return teeStream;
}
}
There was a problem hiding this comment.
I think you need a
TeeTaskListenerandTeePrintStreamdue to the APIs involved rather than aTeeOutputStream
AFAIK you can use TeeOutputStream and just call a PrintStream constructor. There is no actual need to delegate individual PrintStream methods.
I was not sure about it from an encoding standpoint
Pipeline enforces UTF-8 (it is only traditional job types that vary encoding by agent) so you just need to pass a constant encoding arg.
…only responsible for sending data to Splunk
| Helper class that delegates BuildListener functionality to underlying TaskListener | ||
| */ | ||
| public class TeeBuildListener implements BuildListener, Closeable, SerializableOnlyOverRemoting { | ||
| private final OutputStream out; |
There was a problem hiding this comment.
Can you make this a transient field. This will also clear spotbugs warning at lines 31, 37
There was a problem hiding this comment.
Updated. And added some FB warning suppressions, as we're handling the re-instantiation of fields using the Replacement class anyways.
|
jenkinsci/workflow-api-plugin#166 should fix the issue.
|
|
|
Hi, is there any timeline on when this PR will be merged and pushed as a release? |
|
Very interested in a fix for this issue. Credentials leaking into Splunk in plain text make this plugin un-usable. A fix for this issue would be greatly appreciated. |
|
dependent jenkinsci/workflow-api-plugin#166 is still pending |
Fix for secret leakage issues documented in SECURITY-2401 where secrets in the build console output that were being masked by plugins, in our usecase the credentials plugin, were still showing up as unmasked in Splunk.
Underlying issue is the instantiation order of
FilterOutputStreams, which manipulate a stream of data as it comes, in this case the build console. The issue is that in pipeline jobs, the Splunk plugin'sLabelConsoleLineStreamwas getting access to the output before the credentials binding plugin was able to mask the credentials.My change replaces
SplunkTaskListenerDecoratorFactory, which was the root trigger of theFilterOutputStreamcreation that received unmasked data, with a newTeeConsoleLogStorageFactorywhich creates aTeeConsoleLogStorageobject that delegates file log storage an underlying implentation ofLogStoragewhile only being in charge of sending the build console output, which is now fully masked, to Splunk.LogStorage is a new API documented here that overhauls how pipeline build logs are created and stored.
TeeConsoleLogStoragedelegates the actual file logging to an underlyingLogStorageimplementation, in this caseFileLogStorage, and is only in charge of tee-ing the build console output to Splunk.