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

[JENKINS-24805] Start masking secrets in freestyle logs. #28

Merged
merged 4 commits into from Oct 31, 2016
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -38,12 +38,15 @@
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.Serializable;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
import javax.annotation.Nonnull;

import hudson.util.Secret;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.credentialsbinding.impl.CredentialNotFoundException;
import org.kohsuke.stapler.DataBoundConstructor;
@@ -138,4 +141,19 @@ public Unbinder getUnbinder() {
return (BindingDescriptor<C>) super.getDescriptor();
}

/**
* Utility method for turning a collection of secret strings into a {@link Secret}.
* @param secrets A collection of secret strings
* @return A {@link Secret} generated from that collection.
*/
public static Secret getSecretForStrings(Collection<String> secrets) {

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Best to make this @Restricted(NoExternalUse.class), or move it somewhere in the impl package and make it package access.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Done.

StringBuilder b = new StringBuilder();
for (String secret : secrets) {
if (b.length() > 0) {
b.append('|');
}
b.append(Pattern.quote(secret));
}
return Secret.fromString(b.toString());

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

No need to return Secret here; return Pattern. It is only BindingStep which cares about wrapping that in a Secret, for serialization purposes.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Yeah, but as you may note, I reuse this in both BindingStep and SecretBuildWrapper. =)

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

And only BindingStep needs it to be a Secret. SecretBuildWrapper just throws that away and recompiles the Pattern—a waste, and confusing.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Gotcha - fixing up.

}
}
@@ -136,14 +136,7 @@
private String charsetName;

Filter(Collection<String> secrets, String charsetName) {
StringBuilder b = new StringBuilder();
for (String secret : secrets) {
if (b.length() > 0) {
b.append('|');
}
b.append(Pattern.quote(secret));
}
pattern = Secret.fromString(b.toString());
pattern = MultiBinding.getSecretForStrings(secrets);
this.charsetName = charsetName;
}

@@ -26,38 +26,88 @@

import hudson.Extension;
import hudson.Launcher;
import hudson.console.ConsoleLogFilter;
import hudson.console.LineTransformationOutputStream;
import hudson.model.AbstractBuild;
import hudson.model.AbstractProject;
import hudson.model.BuildListener;
import hudson.model.Run;
import hudson.tasks.BuildWrapper;
import hudson.tasks.BuildWrapperDescriptor;
import java.io.IOException;
import java.io.ObjectStreamException;
import java.io.OutputStream;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.WeakHashMap;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.jenkinsci.plugins.credentialsbinding.MultiBinding;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

@SuppressWarnings({"rawtypes", "unchecked"}) // inherited from BuildWrapper
public class SecretBuildWrapper extends BuildWrapper {

private /*almost final*/ List<? extends MultiBinding<?>> bindings;

private final static Map<AbstractBuild<?, ?>, Collection<String>> secretsForBuild = new WeakHashMap<AbstractBuild<?, ?>, Collection<String>>();

/**
* Gets the {@link Pattern} for the secret values for a given build, if that build has secrets defined. If not, return
* null.
* @param build A non-null build.
* @return A compiled {@link Pattern} from the build's secret values, if the build has any.
*/
public static @CheckForNull Pattern getPatternForBuild(@Nonnull AbstractBuild<?, ?> build) {
if (secretsForBuild.containsKey(build)) {
return Pattern.compile(MultiBinding.getSecretForStrings(secretsForBuild.get(build)).getPlainText());
} else {
return null;
}
}

@DataBoundConstructor public SecretBuildWrapper(List<? extends MultiBinding<?>> bindings) {
this.bindings = bindings == null ? Collections.<MultiBinding<?>>emptyList() : bindings;
}

public List<? extends MultiBinding<?>> getBindings() {
return bindings;
}

@Override
public OutputStream decorateLogger(AbstractBuild build, OutputStream logger) throws IOException, InterruptedException, Run.RunnerAbortedException {
if (!bindings.isEmpty()) {
return new Filter(build.getCharset().name()).decorateLogger(build, logger);
} else {
return logger;

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

I guess…there is no use case for adding the wrapper with an empty list of bindings, though, so we could just skip this check.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Ok. I was worried about what would happen with the empty bindings case (that informed my conditional addition below as well).

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

I suppose it would just work as a degenerate case without special effort, but if you are worried you should test it.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Good point - turned out to be busted, so I've got a new commit with test+fix incoming.

}
}

@Override public Environment setUp(AbstractBuild build, final Launcher launcher, BuildListener listener) throws IOException, InterruptedException {
final List<MultiBinding.MultiEnvironment> m = new ArrayList<MultiBinding.MultiEnvironment>();

Set<String> secrets = new HashSet<String>();

for (MultiBinding binding : bindings) {
m.add(binding.bind(build, build.getWorkspace(), launcher, listener));
MultiBinding.MultiEnvironment e = binding.bind(build, build.getWorkspace(), launcher, listener);
m.add(e);
secrets.addAll(e.getValues().values());
}

if (!secrets.isEmpty()) {

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Simpler to just add it unconditionally.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Fixing.

secretsForBuild.put(build, secrets);
}

return new Environment() {
@Override public void buildEnvVars(Map<String,String> env) {
for (MultiBinding.MultiEnvironment e : m) {
@@ -68,6 +118,7 @@
for (MultiBinding.MultiEnvironment e : m) {
e.getUnbinder().unbind(build, build.getWorkspace(), launcher, listener);
}
secretsForBuild.remove(build);
return true;
}
};
@@ -86,6 +137,42 @@ protected Object readResolve() {
return this;
}

/** Similar to {@code MaskPasswordsOutputStream}. */
private static final class Filter extends ConsoleLogFilter {

private final String charsetName;

Filter(String charsetName) {
this.charsetName = charsetName;
}

@Override public OutputStream decorateLogger(final AbstractBuild build, final OutputStream logger) throws IOException, InterruptedException {
return new LineTransformationOutputStream() {
Pattern p;

@Override protected void eol(byte[] b, int len) throws IOException {
if (p == null) {
p = getPatternForBuild(build);
}

if (p != null) {
Matcher m = p.matcher(new String(b, 0, len, charsetName));
if (m.find()) {
logger.write(m.replaceAll("****").getBytes(charsetName));
} else {
// Avoid byte → char → byte conversion unless we are actually doing something.
logger.write(b, 0, len);
}
} else {
// Avoid byte → char → byte conversion unless we are actually doing something.
logger.write(b, 0, len);
}
}
};
}

}

@Extension public static class DescriptorImpl extends BuildWrapperDescriptor {

@Override public boolean isApplicable(AbstractProject<?, ?> item) {
@@ -0,0 +1,81 @@
/*
* The MIT License
*
* Copyright 2016 CloudBees inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package org.jenkinsci.plugins.credentialsbinding.impl;

import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.domains.Domain;
import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.model.Item;
import hudson.tasks.Shell;
import hudson.util.Secret;
import org.jenkinsci.plugins.credentialsbinding.MultiBinding;
import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;

import java.util.Collections;

public class SecretBuildWrapperTest {

@Rule public JenkinsRule r = new JenkinsRule();

@Issue("JENKINS-24805")
@Test public void maskingFreeStyleSecrets() throws Exception {
String credentialsId_1 = "creds_1";
String username_1 = "s3cr3t";
String password_1 = "p4ss";
StringCredentialsImpl c_1 = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId_1, "sample1", Secret.fromString(password_1));
String credentialsId_2 = "creds_2";
String username_2 = "s3cr3t0";
String password_2 = "p4ss" + "EvenLonger";
StringCredentialsImpl c_2 = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId_2, "sample2", Secret.fromString(password_2));

CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c_1);
CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c_2);

SecretBuildWrapper bw_1 = new SecretBuildWrapper(Collections.<MultiBinding<?>>singletonList(new StringBinding("PASS_1", credentialsId_1)));
SecretBuildWrapper bw_2 = new SecretBuildWrapper(Collections.<MultiBinding<?>>singletonList(new StringBinding("PASS_2", credentialsId_2)));

FreeStyleProject f = r.createFreeStyleProject();

f.setConcurrentBuild(true);
f.getBuildersList().add(new Shell("echo $PASS_1"));
f.getBuildersList().add(new Shell("echo $PASS_2"));
f.getBuildWrappersList().add(bw_1);
f.getBuildWrappersList().add(bw_2);

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Not very realistic, since you can in fact only add one of each kind of BuildWrapper to a given Project. Better to create just one wrapper, with a two-element list of bindings.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Okiedokie.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Actually, removed the second one entirely since that's a separate issue.


r.configRoundtrip((Item)f);

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

BTW the cast is unnecessary as of some sufficiently recent jenkins-test-harness ~ plugin-pom.


FreeStyleBuild b = r.buildAndAssertSuccess(f);
r.assertLogNotContains(password_1, b);
r.assertLogNotContains(password_2, b);
r.assertLogContains("echo ****", b);
}

}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.