Skip to content

Security-2401 Fix credentials leakage to Splunk#17

Closed
pyieh wants to merge 7 commits into
jenkinsci:masterfrom
pyieh:bugfix/SECURITY-2401
Closed

Security-2401 Fix credentials leakage to Splunk#17
pyieh wants to merge 7 commits into
jenkinsci:masterfrom
pyieh:bugfix/SECURITY-2401

Conversation

@pyieh

@pyieh pyieh commented Jul 21, 2021

Copy link
Copy Markdown

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's LabelConsoleLineStream was 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 the FilterOutputStream creation that received unmasked data, with a new TeeConsoleLogStorageFactory which creates a TeeConsoleLogStorage object that delegates file log storage an underlying implentation of LogStorage while 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. TeeConsoleLogStorage delegates the actual file logging to an underlying LogStorage implementation, in this case FileLogStorage, and is only in charge of tee-ing the build console output to Splunk.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

pyieh added 2 commits July 20, 2021 17:16
…(FileLogStorage) to send build console output to Splunk as it's building the log file
@pyieh
pyieh marked this pull request as draft July 21, 2021 00:28
@pyieh

pyieh commented Jul 27, 2021

Copy link
Copy Markdown
Author

@jglick Have you gotten a chance to take a look?

@jglick jglick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am afraid I do not have time to help much.

Comment on lines +37 to +38
Code copied over from
https://github.com/jenkinsci/workflow-api-plugin/blob/master/src/main/java/org/jenkinsci/plugins/workflow/log/FileLogStorage.java

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not believe you need to do this. Try delegating to FileLogStorage instead.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I said delegating to, not extending.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not everyone using FileLogStorage would necessarily want that

Fine, forBuild can decide on a case-by-case basis.

make FileLogStorage more 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dwnusbaum dwnusbaum Jul 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
    }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need a TeeTaskListener and TeePrintStream due to the APIs involved rather than a TeeOutputStream

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.

@pyieh
pyieh marked this pull request as ready for review July 31, 2021 07:44
@pyieh
pyieh requested review from dwnusbaum and jglick July 31, 2021 07:44

@ramapalani ramapalani left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Helper class that delegates BuildListener functionality to underlying TaskListener
*/
public class TeeBuildListener implements BuildListener, Closeable, SerializableOnlyOverRemoting {
private final OutputStream out;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you make this a transient field. This will also clear spotbugs warning at lines 31, 37

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated. And added some FB warning suppressions, as we're handling the re-instantiation of fields using the Replacement class anyways.

@ddewhurst444

Copy link
Copy Markdown

@jglick @fengxx Can we please get another review of this PR to try and get it merged? This bug creates a security risk for users of this plugin.

@fengxx

fengxx commented Aug 23, 2021

Copy link
Copy Markdown

jenkinsci/workflow-api-plugin#166 should fix the issue.
I added jira comment for this PR and copied it here in case you can not view it

  1. LogStorage is exclusive, the first loaded plugin wins. If there is another plugin implemented LogStorage with higher priority, it will not work
  2. Please try to keep backward compatibility when make changes. compatibility issue such as
    • existing jobs using sendSplunkConsoleLog step will send duplicates logs
    • the PR breaks console log view, user can not view individual workflow step logs

@jglick

jglick commented Aug 23, 2021

Copy link
Copy Markdown
Member

@rajivshankar1982

Copy link
Copy Markdown

Hi, is there any timeline on when this PR will be merged and pushed as a release?

@smitaskamat

Copy link
Copy Markdown

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.

@fengxx

fengxx commented Mar 3, 2022

Copy link
Copy Markdown

dependent jenkinsci/workflow-api-plugin#166 is still pending

This pull request was closed.
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.

8 participants