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

Conversation

@abayer
Copy link
Member

commented Oct 31, 2016

JENKINS-24805

This still needs tests - I just want to make sure the approach is right.

cc @reviewbybees esp @jglick

[JENKINS-24805] First work on masking secrets in freestyle logs.
This still needs tests - I just want to make sure the approach is right.
@jglick
Copy link
Member

left a comment

Looks right overall; minor mistakes. Can pick up functional tests from previous PRs.

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

* @param build A non-null build.
* @return A compiled {@link Pattern} from the build's secret values, if the build has any.
*/
public static @Nullable Pattern getPatternForBuild(@Nonnull AbstractBuild<?, ?> build) {

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Use @CheckForNull.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Done.

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

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

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

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

final

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

d'oh! Fixed.

return new Environment() {
@Override public void buildEnvVars(Map<String,String> env) {
for (MultiBinding.MultiEnvironment e : m) {
env.putAll(e.getValues());
secretsForBuild.get(build).addAll(e.getValues().values());

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Better to add this above, as soon as we have called bind, so we do not have to assume buildEnvVars has been called.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Ah, ok, you'd mentioned something about doing it here.

@@ -86,6 +133,52 @@ protected Object readResolve() {
return this;
}

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

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Need not be Serializable.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Whoops. Done.

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

private static final long serialVersionUID = 1;

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Ditto.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Done.

this.charsetName = Charsets.UTF_8.name();
}
return this;
}

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

Unnecessary, delete.

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Done.


private static final long serialVersionUID = 1;

private String charsetName;

This comment has been minimized.

Copy link
@jglick

jglick Oct 31, 2016

Member

final

This comment has been minimized.

Copy link
@abayer

abayer Oct 31, 2016

Author Member

Done.

@abayer abayer changed the title [JENKINS-24805] First work on masking secrets in freestyle logs. [JENKINS-24805] Start masking secrets in freestyle logs. Oct 31, 2016

@abayer

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2016

@jglick - updated, added test from #19.

@reviewbybees

This comment has been minimized.

Copy link

commented Oct 31, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick
Copy link
Member

left a comment

Pretty close.

}
b.append(Pattern.quote(secret));
}
return Secret.fromString(b.toString());

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.

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

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.

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.

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.

@jglick
jglick approved these changes Oct 31, 2016
Copy link
Member

left a comment

🐝

f.getBuildersList().add(new Shell("echo $PASS_1"));
f.getBuildWrappersList().add(wrapper);

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.

@jglick
jglick approved these changes Oct 31, 2016
Copy link
Member

left a comment

re🐝

@aheritier

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

LGTM 🐝

@jglick jglick merged commit d2bcf97 into jenkinsci:master Oct 31, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.