From de5e69b5e85bd5a89960e65f3a634256b24b7af5 Mon Sep 17 00:00:00 2001 From: Carroll Chiou Date: Thu, 5 Nov 2020 22:11:11 -0700 Subject: [PATCH] [JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings (#370) --- CHANGELOG.md | 8 + pom.xml | 4 +- .../workflow/cps/ContextVariableSet.java | 1 + .../plugins/workflow/cps/CpsStepContext.java | 2 +- .../jenkinsci/plugins/workflow/cps/DSL.java | 238 +++++++++++++----- .../cps/actions/ArgumentsActionImpl.java | 145 ++--------- .../cps/view/InterpolatedSecretsAction.java | 121 +++++++++ .../InterpolatedSecretsAction/summary.jelly | 38 +++ .../plugins/workflow/cps/DSLTest.java | 184 ++++++++++++++ .../workflow/cps/ParamsVariableTest.java | 4 +- .../cps/actions/ArgumentsActionImplTest.java | 77 +++--- 11 files changed, 613 insertions(+), 209 deletions(-) create mode 100644 src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java create mode 100644 src/main/resources/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction/summary.jelly diff --git a/CHANGELOG.md b/CHANGELOG.md index 01a68067b..289dba700 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ ## Changelog +### 2.85 + +Release date: 2020-11-05 + +* Improvement: Add warnings when secrets are used with Groovy String interpolation. ([JENKINS-63254](https://issues.jenkins-ci.org/browse/JENKINS-63254)) +* Fix: Allow masking of secret variables that use the same name as system variables. ([JENKINS-47101](https://issues.jenkins-ci.org/browse/JENKINS-47101)) +* Fix: Throw an error when a step that requires a body has no body. ([PR #370](https://github.com/jenkinsci/workflow-cps-plugin/pull/370)) + ### 2.84 Release date: 2020-10-30 diff --git a/pom.xml b/pom.xml index cbed3dec7..a07addba6 100644 --- a/pom.xml +++ b/pom.xml @@ -69,6 +69,7 @@ 8 false 1.32 + 3.6 12.19.0 6.14.8 1.10.0 @@ -96,6 +97,7 @@ org.jenkins-ci.plugins.workflow workflow-support + ${workflow-support-plugin.version} org.jenkins-ci.plugins.workflow @@ -104,7 +106,6 @@ org.jenkins-ci.plugins script-security - 1.75 org.jenkins-ci.plugins @@ -135,6 +136,7 @@ org.jenkins-ci.plugins.workflow workflow-support + ${workflow-support-plugin.version} tests test diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/ContextVariableSet.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/ContextVariableSet.java index b35966310..1275dffb8 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/ContextVariableSet.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/ContextVariableSet.java @@ -81,6 +81,7 @@ private static final class DynamicContextQuery { } } + @FunctionalInterface interface ThrowingSupplier { T get() throws IOException; } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java index fe1dcacc2..51a64e770 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java @@ -155,7 +155,7 @@ public class CpsStepContext extends DefaultStepContext { // TODO add XStream cla private transient List bodyInvokers = new ArrayList<>(); /** - * While {@link CpsStepContext} has not received teh response, maintains the body closure. + * While {@link CpsStepContext} has not received the response, maintains the body closure. * * This is the implicit closure block passed to the step invocation. */ diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java index 46b6c79e7..a930b0a7b 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java @@ -26,11 +26,13 @@ import com.cloudbees.groovy.cps.Continuable; import com.cloudbees.groovy.cps.Outcome; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import groovy.lang.Closure; import groovy.lang.GString; import groovy.lang.GroovyObject; import groovy.lang.GroovyObjectSupport; import groovy.lang.GroovyRuntimeException; +import hudson.AbortException; import hudson.EnvVars; import hudson.ExtensionList; import hudson.Util; @@ -76,23 +78,35 @@ import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*; import org.jenkinsci.plugins.workflow.cps.steps.LoadStep; import org.jenkinsci.plugins.workflow.cps.steps.ParallelStep; +import org.jenkinsci.plugins.workflow.cps.view.InterpolatedSecretsAction; import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner; import org.jenkinsci.plugins.workflow.flow.StepListener; import org.jenkinsci.plugins.workflow.graph.FlowNode; import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback; +import org.jenkinsci.plugins.workflow.steps.EnvironmentExpander; import org.jenkinsci.plugins.workflow.steps.Step; import org.jenkinsci.plugins.workflow.steps.StepContext; import org.jenkinsci.plugins.workflow.steps.StepDescriptor; import org.jenkinsci.plugins.workflow.steps.StepExecution; import org.jvnet.hudson.annotation_indexer.Index; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.ClassDescriptor; import org.kohsuke.stapler.NoStaplerConstructorException; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + /** * Calls {@link Step}s and other DSL objects. */ @PersistIn(PROGRAM) public class DSL extends GroovyObjectSupport implements Serializable { + @SuppressFBWarnings("MS_SHOULD_BE_FINAL") // Used to control warning behavior of unsafe Groovy interpolation + @Restricted(NoExternalUse.class) + public static String UNSAFE_GROOVY_INTERPOLATION = System.getProperty(DSL.class.getName() + ".UNSAFE_GROOVY_INTERPOLATION", "warn"); + private final FlowExecutionOwner handle; private transient CpsFlowExecution exec; private transient Map functions; @@ -227,23 +241,40 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) { boolean hack = d instanceof ParallelStep.DescriptorImpl || d instanceof LoadStep.DescriptorImpl; if (ps.body == null && !hack) { - an = new StepAtomNode(exec, d, thread.head.get()); + if (!(d.getClass().getName().equals("org.jenkinsci.plugins.workflow.support.steps.StageStep$DescriptorImpl")) + && d.takesImplicitBlockArgument()) { + throw new IllegalStateException(String.format("%s step must be called with a body", name)); + } else { + an = new StepAtomNode(exec, d, thread.head.get()); + } } else { an = new StepStartNode(exec, d, thread.head.get()); } - // Ensure ArgumentsAction is attached before we notify even synchronous listeners: final CpsStepContext context = new CpsStepContext(d, thread, handle, an, ps.body); + EnvVars allEnv = null; + Set sensitiveVariables = Collections.emptySet(); + try { + allEnv = context.get(EnvVars.class); + EnvironmentExpander envExpander = context.get(EnvironmentExpander.class); + if (envExpander != null) { + sensitiveVariables = new HashSet<>(envExpander.getSensitiveVariables()); + } + } catch (IOException | InterruptedException e) { + LOGGER.log(Level.WARNING, "Unable to retrieve environment variables", e); + } + // Ensure ArgumentsAction is attached before we notify even synchronous listeners: + ArgumentsActionImpl argumentsAction = null; try { // No point storing empty arguments, and ParallelStep is a special case where we can't store its closure arguments if (ps.namedArgs != null && !(ps.namedArgs.isEmpty()) && isKeepStepArguments() && !(d instanceof ParallelStep.DescriptorImpl)) { // Get the environment variables to find ones that might be credentials bindings Computer comp = context.get(Computer.class); - EnvVars allEnv = new EnvVars(context.get(EnvVars.class)); if (comp != null && allEnv != null) { allEnv.entrySet().removeAll(comp.getEnvironment().entrySet()); } - an.addAction(new ArgumentsActionImpl(ps.namedArgs, allEnv)); + argumentsAction = new ArgumentsActionImpl(ps.namedArgs, allEnv, sensitiveVariables); + an.addAction(argumentsAction); } } catch (Exception e) { // Avoid breaking execution because we can't store some sort of crazy Step argument @@ -257,8 +288,10 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) { boolean sync; ClassLoader originalLoader = Thread.currentThread().getContextClassLoader(); try { + TaskListener listener = context.get(TaskListener.class); + logInterpolationWarnings(name, argumentsAction, an.getId(), ps.interpolatedStrings, allEnv, sensitiveVariables, listener); if (unreportedAmbiguousFunctions.remove(name)) { - reportAmbiguousStepInvocation(context, d); + reportAmbiguousStepInvocation(context, d, listener); } d.checkContextAvailability(context); Thread.currentThread().setContextClassLoader(CpsVmExecutorService.ORIGINAL_CONTEXT_CLASS_LOADER.get()); @@ -266,7 +299,7 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) { s = d.newInstance(ps.namedArgs); } else { DescribableModel stepModel = DescribableModel.of(d.clazz); - s = stepModel.instantiate(ps.namedArgs, context.get(TaskListener.class)); + s = stepModel.instantiate(ps.namedArgs, listener); } // Persist the node - block start and end nodes do their own persistence. @@ -327,6 +360,53 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) { } } + private void logInterpolationWarnings(String stepName, @CheckForNull ArgumentsActionImpl argumentsAction, String nodeId, Set interpolatedStrings, @CheckForNull EnvVars envVars, @Nonnull Set sensitiveVariables, TaskListener listener) throws IOException, InterruptedException { + if (UNSAFE_GROOVY_INTERPOLATION.equals("ignore")) { + return; + } + boolean shouldFail; + if (UNSAFE_GROOVY_INTERPOLATION.equals("fail")) { + shouldFail = true; + } else { + shouldFail = false; + } + + if (argumentsAction == null || interpolatedStrings.isEmpty() || envVars == null || envVars.isEmpty() || sensitiveVariables.isEmpty()) { + return; + } + + List scanResults = sensitiveVariables.stream() + .filter(e -> interpolatedStrings.stream().anyMatch(g -> g.contains(envVars.get(e)))) + .collect(Collectors.toList()); + + if (scanResults != null && !scanResults.isEmpty()) { + String warningType; + if (shouldFail) { + warningType = "Error"; + } else { + warningType = "Warning"; + } + String warning = String.format("%s: A secret was passed to \"%s\" using Groovy String interpolation, which is insecure.%n\t\t Affected argument(s) used the following variable(s): %s%n\t\t See https://jenkins.io/redirect/groovy-string-interpolation for details.", + warningType, stepName, scanResults.toString()); + FlowExecutionOwner owner = exec.getOwner(); + if (owner != null && owner.getExecutable() instanceof Run) { + InterpolatedSecretsAction runReport = ((Run) owner.getExecutable()).getAction(InterpolatedSecretsAction.class); + if (runReport == null) { + runReport = new InterpolatedSecretsAction(); + ((Run) owner.getExecutable()).addAction(runReport); + } + runReport.record(stepName, scanResults); + } else { + LOGGER.log(Level.FINE, "Unable to generate Interpolated Secrets Report"); + } + if (shouldFail) { + throw new AbortException(warning); + } else { + listener.getLogger().println(warning); + } + } + } + private static String loadSoleArgumentKey(StepDescriptor d) { try { String[] names = new ClassDescriptor(d.clazz).loadConstructorParamNames(); @@ -354,7 +434,7 @@ protected Object invokeDescribable(String symbol, Object _args) { // The only time a closure is valid is when the resulting Describable is immediately executed via a meta-step NamedArgsAndClosure args = parseArgs(_args, metaStep!=null && metaStep.takesImplicitBlockArgument(), - UninstantiatedDescribable.ANONYMOUS_KEY, singleArgumentOnly); + UninstantiatedDescribable.ANONYMOUS_KEY, singleArgumentOnly, new HashSet<>()); UninstantiatedDescribable ud = new UninstantiatedDescribable(symbol, null, args.namedArgs); if (metaStep==null) { @@ -367,12 +447,15 @@ protected Object invokeDescribable(String symbol, Object _args) { // also note that in this case 'd' is not trustworthy, as depending on // where this UninstantiatedDescribable is ultimately used, the symbol // might be resolved with a specific type. - return ud; + + // we are returning the NamedArgsAndClosure instead of the UninstantiatedDescribable in order to preserve + // the discovered interpolated strings that are stored in the NamedArgsAndClosure. + args.uninstantiatedDescribable = ud; + return args; } else { Descriptor d = SymbolLookup.get().findDescriptor((Class)(metaStep.getMetaStepArgumentType()), symbol); try { // execute this Describable through a meta-step - // split args between MetaStep (represented by mm) and Describable (represented by dm) DescribableModel mm = DescribableModel.of(metaStep.clazz); DescribableModel dm = DescribableModel.of(d.clazz); @@ -414,34 +497,28 @@ protected Object invokeDescribable(String symbol, Object _args) { ud = new UninstantiatedDescribable(symbol, null, dargs); margs.put(p.getName(),ud); - return invokeStep(metaStep, symbol, new NamedArgsAndClosure(margs, args.body)); + return invokeStep(metaStep, symbol, new NamedArgsAndClosure(margs, args.body, args.interpolatedStrings)); } catch (Exception e) { throw new IllegalArgumentException("Failed to prepare "+symbol+" step",e); } } } - private void reportAmbiguousStepInvocation(CpsStepContext context, StepDescriptor d) { - Exception e = null; - try { - TaskListener listener = context.get(TaskListener.class); - if (listener != null) { - List ambiguousClassNames = StepDescriptor.all().stream() - .filter(sd -> sd.getFunctionName().equals(d.getFunctionName())) - .map(sd -> sd.clazz.getName()) - .collect(Collectors.toList()); - String message = String.format("Warning: Invoking ambiguous Pipeline Step ‘%1$s’ (%2$s). " + - "‘%1$s’ could refer to any of the following steps: %3$s. " + - "You can invoke steps by class name instead to avoid ambiguity. " + - "For example: steps.'%2$s'(...)", - d.getFunctionName(), d.clazz.getName(), ambiguousClassNames); - listener.getLogger().println(message); - return; - } - } catch (InterruptedException | IOException temp) { - e = temp; - } - LOGGER.log(Level.FINE, "Unable to report ambiguous step invocation for: " + d.getFunctionName(), e); + private void reportAmbiguousStepInvocation(CpsStepContext context, StepDescriptor d, @Nullable TaskListener listener) { + if (listener != null) { + List ambiguousClassNames = StepDescriptor.all().stream() + .filter(sd -> sd.getFunctionName().equals(d.getFunctionName())) + .map(sd -> sd.clazz.getName()) + .collect(Collectors.toList()); + String message = String.format("Warning: Invoking ambiguous Pipeline Step ‘%1$s’ (%2$s). " + + "‘%1$s’ could refer to any of the following steps: %3$s. " + + "You can invoke steps by class name instead to avoid ambiguity. " + + "For example: steps.'%2$s'(...)", + d.getFunctionName(), d.clazz.getName(), ambiguousClassNames); + listener.getLogger().println(message); + return; + } + LOGGER.log(Level.FINE, "Unable to report ambiguous step invocation for: " + d.getFunctionName()); } /** Returns the capacity we need to allocate for a HashMap so it will hold all elements without needing to resize. */ @@ -455,20 +532,60 @@ private static int preallocatedHashmapCapacity(int elementsToHold) { } } - static class NamedArgsAndClosure { - final Map namedArgs; + /** + * This class holds the argument map and optional body of the step that is to be invoked. + * + *

Some steps have complex argument types (e.g. `checkout` takes {@link hudson.scm.SCM}). When user use symbol-based + * syntax with those arguments, an instance of this class is created as the result of {@link DSL#invokeDescribable(String, Object)}. + * The instance is returned to the Groovy program so that it can be passed to {@link DSL#invokeStep(StepDescriptor, String, Object)} + * later, so it must implement {@link Serializable}. + */ + static class NamedArgsAndClosure implements Serializable { final Map namedArgs; final Closure body; + final List msgs; + final Set interpolatedStrings; + // UninstantiatedDescribable is set when the associated symbol is being built as the parameter of a step to be invoked. + UninstantiatedDescribable uninstantiatedDescribable = null; - private NamedArgsAndClosure(Map namedArgs, Closure body) { + private NamedArgsAndClosure(Map namedArgs, Closure body, @Nonnull Set foundInterpolatedStrings) { this.namedArgs = new LinkedHashMap<>(preallocatedHashmapCapacity(namedArgs.size())); this.body = body; + this.msgs = new ArrayList<>(); + this.interpolatedStrings = new HashSet<>(foundInterpolatedStrings); + namedArgs = (Map) collectInterpolatedStrings(namedArgs, interpolatedStrings); for (Map.Entry entry : namedArgs.entrySet()) { String k = entry.getKey().toString().intern(); // coerces GString and more - Object v = flattenGString(entry.getValue()); + Object v = flattenGString(entry.getValue(), interpolatedStrings); this.namedArgs.put(k, v); } } + + /** + * Recursively search argument values for instances of {@link NamedArgsAndClosure} and convert them to {@link UninstantiatedDescribable}. + * These instances were created in {@link DSL#invokeDescribable(String, Object)} for symbols with no meta-step. + * Gathers all the interpolated strings from each instance of {@link NamedArgsAndClosure}. + */ + private static Object collectInterpolatedStrings(Object argValue, Set interpolatedStrings) { + if (argValue instanceof NamedArgsAndClosure) { + interpolatedStrings.addAll(((NamedArgsAndClosure) argValue).interpolatedStrings); + return ((NamedArgsAndClosure) argValue).uninstantiatedDescribable; + } else if (argValue instanceof Map) { + Map r = new LinkedHashMap<>(preallocatedHashmapCapacity(((Map) argValue).size())); + for (Map.Entry e : ((Map) argValue).entrySet()) { + r.put(e.getKey(), collectInterpolatedStrings(e.getValue(), interpolatedStrings)); + } + return r; + } else if (argValue instanceof List) { + List r = new ArrayList<>(); + for (int i = 0; i < ((List) argValue).size(); i++) { + r.add(collectInterpolatedStrings(((List) argValue).get(i), interpolatedStrings)); + } + return r; + } else { + return argValue; + } + } } /** @@ -479,28 +596,31 @@ private NamedArgsAndClosure(Map namedArgs, Closure body) { * For reference, Groovy does {@linkplain ReflectionCache#getCachedClass ReflectionCache.getCachedClass(types[i]).}{@linkplain CachedClass#coerceArgument coerceArgument(a)}. * Note that {@link DescribableModel#instantiate} would also handle {@link GString} in {@code coerce}, * but better to do it here in the Groovy-specific code so we do not need to rely on that. + * Record all instances of interpolated Groovy strings. We can check this collection later to see if sensitive variables were used. * @return {@code v} or an equivalent with all {@link GString}s flattened, including in nested {@link List}s or {@link Map}s */ - private static Object flattenGString(Object v) { + private static Object flattenGString(Object v, @Nonnull Set interpolatedStrings) { if (v instanceof GString) { - return v.toString(); + String flattened = v.toString(); + interpolatedStrings.add(flattened); + return flattened; } else if (v instanceof List) { boolean mutated = false; List r = new ArrayList<>(); for (Object o : ((List) v)) { - Object o2 = flattenGString(o); + Object o2 = flattenGString(o, interpolatedStrings); mutated |= o != o2; r.add(o2); } return mutated ? r : v; } else if (v instanceof Map) { boolean mutated = false; - Map r = new LinkedHashMap<>(preallocatedHashmapCapacity(((Map) v).size())); - for (Map.Entry e : ((Map) v).entrySet()) { + Map r = new LinkedHashMap<>(preallocatedHashmapCapacity(((Map) v).size())); + for (Map.Entry e : ((Map) v).entrySet()) { Object k = e.getKey(); - Object k2 = flattenGString(k); + Object k2 = flattenGString(k, interpolatedStrings); Object o = e.getValue(); - Object o2 = flattenGString(o); + Object o2 = flattenGString(o, interpolatedStrings); mutated |= k != k2 || o != o2; r.put(k2, o2); } @@ -518,12 +638,12 @@ static NamedArgsAndClosure parseArgs(Object arg, StepDescriptor d) { if (singleArgumentOnly) { // Can fetch the one argument we need DescribableParameter dp = stepModel.getSoleRequiredParameter(); String paramName = (dp != null) ? dp.getName() : null; - return parseArgs(arg, d.takesImplicitBlockArgument(), paramName, singleArgumentOnly); + return parseArgs(arg, d.takesImplicitBlockArgument(), paramName, singleArgumentOnly, new HashSet<>()); } } catch (NoStaplerConstructorException e) { // Ignore steps without databound constructors and treat them as normal. } - return parseArgs(arg,d.takesImplicitBlockArgument(), loadSoleArgumentKey(d), singleArgumentOnly); + return parseArgs(arg,d.takesImplicitBlockArgument(), loadSoleArgumentKey(d), singleArgumentOnly, new HashSet<>()); } /** @@ -548,19 +668,25 @@ static NamedArgsAndClosure parseArgs(Object arg, StepDescriptor d) { * @param soleArgumentKey * If the context in which this method call happens allow implicit sole default argument, specify its name. * If null, the call must be with names arguments. + * @param interpolatedStrings + * The collection of interpolated Groovy strings. */ - static NamedArgsAndClosure parseArgs(Object arg, boolean expectsBlock, String soleArgumentKey, boolean singleRequiredArg) { - if (arg instanceof NamedArgsAndClosure) + static NamedArgsAndClosure parseArgs(Object arg, boolean expectsBlock, String soleArgumentKey, boolean singleRequiredArg, @Nonnull Set interpolatedStrings) { + if (arg instanceof NamedArgsAndClosure) { return (NamedArgsAndClosure) arg; - if (arg instanceof Map) // TODO is this clause actually used? - return new NamedArgsAndClosure((Map) arg, null); - if (arg instanceof Closure && expectsBlock) - return new NamedArgsAndClosure(Collections.emptyMap(),(Closure)arg); + } + if (arg instanceof Map) { // TODO is this clause actually used? + return new NamedArgsAndClosure((Map) arg, null, interpolatedStrings); + } + if (arg instanceof Closure && expectsBlock) { + return new NamedArgsAndClosure(Collections.emptyMap(),(Closure)arg, interpolatedStrings); + } if (arg instanceof Object[]) {// this is how Groovy appears to pack argument list into one Object for invokeMethod List a = Arrays.asList((Object[])arg); - if (a.size()==0) - return new NamedArgsAndClosure(Collections.emptyMap(),null); + if (a.size()==0) { + return new NamedArgsAndClosure(Collections.emptyMap(), null, interpolatedStrings); + } Closure c=null; @@ -575,21 +701,21 @@ static NamedArgsAndClosure parseArgs(Object arg, boolean expectsBlock, String so if (!singleRequiredArg || (soleArgumentKey != null && mapArg.size() == 1 && mapArg.containsKey(soleArgumentKey))) { // this is how Groovy passes in Map - return new NamedArgsAndClosure(mapArg, c); + return new NamedArgsAndClosure(mapArg, c, interpolatedStrings); } } switch (a.size()) { case 0: - return new NamedArgsAndClosure(Collections.emptyMap(),c); + return new NamedArgsAndClosure(Collections.emptyMap(),c, interpolatedStrings); case 1: - return new NamedArgsAndClosure(singleParam(soleArgumentKey, a.get(0)), c); + return new NamedArgsAndClosure(singleParam(soleArgumentKey, a.get(0)), c, interpolatedStrings); default: throw new IllegalArgumentException("Expected named arguments but got "+a); } } - return new NamedArgsAndClosure(singleParam(soleArgumentKey, arg), null); + return new NamedArgsAndClosure(singleParam(soleArgumentKey, arg), null, interpolatedStrings); } private static Map singleParam(String soleArgumentKey, Object arg) { if (soleArgumentKey != null) { diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImpl.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImpl.java index 20ba11216..9ba61ee36 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImpl.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImpl.java @@ -47,14 +47,13 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; -import java.util.regex.Pattern; - /** * Implements {@link ArgumentsAction} by storing step arguments, with sanitization. @@ -66,47 +65,39 @@ public class ArgumentsActionImpl extends ArgumentsAction { @CheckForNull private Map arguments; + private final Set sensitiveVariables; boolean isUnmodifiedBySanitization = true; private static final Logger LOGGER = Logger.getLogger(ArgumentsActionImpl.class.getName()); - public ArgumentsActionImpl(@Nonnull Map stepArguments, @CheckForNull EnvVars env) { + public ArgumentsActionImpl(@Nonnull Map stepArguments, @CheckForNull EnvVars env, @Nonnull Set sensitiveVariables) { + this.sensitiveVariables = new HashSet<>(sensitiveVariables); arguments = serializationCheck(sanitizeMapAndRecordMutation(stepArguments, env)); } /** Create a step, sanitizing strings for secured content */ public ArgumentsActionImpl(@Nonnull Map stepArguments) { - this(stepArguments, new EnvVars()); + this(stepArguments, new EnvVars(), Collections.emptySet()); } /** For testing use only */ - ArgumentsActionImpl(){ + ArgumentsActionImpl(@Nonnull Set sensitiveVariables){ this.isUnmodifiedBySanitization = false; this.arguments = Collections.emptyMap(); + this.sensitiveVariables = sensitiveVariables; } - /** See if sensitive environment variable content is in a string */ - public static boolean isStringSafe(@CheckForNull String input, @CheckForNull EnvVars variables, @Nonnull Set safeEnvVariables) { - if (input == null || variables == null || variables.size() == 0) { - return true; + /** See if sensitive environment variable content is in a string and replace the content with its associated variable name, otherwise return string unmodified*/ + public static String replaceSensitiveVariables(@Nonnull String input, @CheckForNull EnvVars variables, @Nonnull Set sensitiveVariables) { + if (variables == null || variables.size() == 0 || sensitiveVariables.size() ==0) { + return input; } - StringBuilder pattern = new StringBuilder(); - int count = 0; - for (Map.Entry ent : variables.entrySet()) { - String val = ent.getValue(); - if (val == null || val.isEmpty() || safeEnvVariables.contains(ent.getKey())) { // Skip values that are safe - continue; - } - if (count > 0) { - pattern.append('|'); - } - pattern.append(Pattern.quote(val)); - count++; + String modded = input; + for (String sensitive : sensitiveVariables) { + modded = modded.replace(variables.get(sensitive), "${" + sensitive + "}"); } - return (count > 0) - ? !Pattern.compile(pattern.toString()).matcher(input).find() - : true; + return modded; } /** Restrict stored arguments to a reasonable subset of types so we don't retain totally arbitrary objects @@ -138,98 +129,6 @@ boolean isStorableType(Object ob) { return c.isPrimitive() || (c.isArray() && !(c.getComponentType().isPrimitive())); // Primitive arrays are not legal here } - /** Normal environment variables, as opposed to ones that might come from credentials bindings */ - private static final HashSet SAFE_ENVIRONMENT_VARIABLES = new HashSet<>(Arrays.asList( - // Pipeline/Jenkins variables in normal builds - "BRANCH_NAME", - "BUILD_DISPLAY_NAME", - "BUILD_ID", - "BUILD_NUMBER", - "BUILD_TAG", - "BUILD_URL", - "CHANGE_AUTHOR", - "CHANGE_AUTHOR_DISPLAY_NAME", - "CHANGE_AUTHOR_EMAIL", - "CHANGE_ID", - "CHANGE_TARGET", - "CHANGE_TITLE", - "CHANGE_URL", - "EXECUTOR_NUMBER", - "HUDSON_COOKIE", - "HUDSON_HOME", - "HUDSON_SERVER_COOKIE", - "HUDSON_URL", - "JENKINS_HOME", - "JENKINS_SERVER_COOKIE", - "JENKINS_URL", - "JOB_BASE_NAME", - "JOB_NAME", - "JOB_URL", - "NODE_LABELS", - "NODE_NAME", - "STAGE_NAME", - "WORKSPACE", - - // Normal system variables for posix environments - "HOME", - "LANG", - "LOGNAME", - "MAIL", - "NLSPATH", - "PATH", - "PWD", - "SHELL", - "SHLVL", - "TERM", - "USER", - "XFILESEARCHPATH", - - // Windows system variables - "ALLUSERSPROFILE", - "APPDATA", - "CD", - "ClientName", - "CMDEXTVERSION", - "CMDCMDLINE", - "CommonProgramFiles", - "COMPUTERNAME", - "COMSPEC", - "DATE", - "ERRORLEVEL", - "HighestNumaNodeNumber", - "HOMEDRIVE", - "HOMEPATH", - "LOCALAPPDATA", - "LOGONSERVER", - "NUMBER_OF_PROCESSORS", - "OS", - "PATHEXT", - "PROCESSOR_ARCHITECTURE", - "PROCESSOR_ARCHITEW6432", - "PROCESSOR_IDENTIFIER", - "PROCESSOR_LEVEL", - "PROCESSOR_REVISION", - "ProgramW6432", - "ProgramData", - "ProgramFiles", - "ProgramFiles (x86)", - "PROMPT", - "PSModulePath", - "Public", - "RANDOM", - "%SessionName%", - "SYSTEMDRIVE", - "SYSTEMROOT", - "TEMP", "TMP", - "TIME", - "UserDnsDomain", - "USERDOMAIN", - "USERDOMAIN_roamingprofile", -// "USERNAME", // Not whitelisted because this is a likely variable name for credentials binding - "USERPROFILE", - "WINDIR" - )); - /** * Sanitize a list recursively */ @@ -279,7 +178,7 @@ Object sanitizeArrayAndRecordMutation(@Nonnull Object[] objects, @CheckForNull E /** Recursively sanitize a single object by: * - Exploding {@link Step}s and {@link UninstantiatedDescribable}s into their Maps to sanitize - * - Removing unsafe strings using {@link #isStringSafe(String, EnvVars, Set)} and replace with {@link NotStoredReason#MASKED_VALUE} + * - Removing unsafe strings using {@link #replaceSensitiveVariables(String, EnvVars, Set)} and replace with the variable name * - Removing oversized objects using {@link #isOversized(Object)} and replacing with {@link NotStoredReason#OVERSIZE_VALUE} * While making an effort not to retain needless copies of objects and to re-use originals where possible * (including the Step or UninstantiatedDescribable) @@ -331,9 +230,12 @@ Object sanitizeObjectAndRecordMutation(@CheckForNull Object o, @CheckForNull Env this.isUnmodifiedBySanitization = true; return NotStoredReason.UNSERIALIZABLE; } - } else if (modded instanceof String && vars != null && !vars.isEmpty() && !isStringSafe((String)modded, vars, SAFE_ENVIRONMENT_VARIABLES)) { - this.isUnmodifiedBySanitization = false; - return NotStoredReason.MASKED_VALUE; + } else if (modded instanceof String && vars != null && !vars.isEmpty()) { + String replaced = replaceSensitiveVariables((String)modded, vars, sensitiveVariables); + if (!replaced.equals(modded)) { + this.isUnmodifiedBySanitization = false; + } + return replaced; } if (modded != tempVal) { @@ -391,12 +293,11 @@ Map serializationCheck(@Nonnull Map arguments) { @Nonnull Map sanitizeMapAndRecordMutation(@Nonnull Map mapContents, @CheckForNull EnvVars variables) { // Package scoped so we can test it directly - HashMap output = Maps.newHashMapWithExpectedSize(mapContents.size()); + LinkedHashMap output = new LinkedHashMap<>(mapContents.size()); boolean isMutated = false; for (Map.Entry param : mapContents.entrySet()) { Object modded = sanitizeObjectAndRecordMutation(param.getValue(), variables); - if (modded != param.getValue()) { // Sanitization stripped out some values, so we need to store the mutated object output.put(param.getKey(), modded); diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java new file mode 100644 index 000000000..c2261f68a --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction.java @@ -0,0 +1,121 @@ +/* + * The MIT License + * + * Copyright (c) 2020, 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.workflow.cps.view; + +import hudson.model.Run; +import jenkins.model.RunAction2; +import org.jenkinsci.plugins.structs.describable.DescribableModel; +import org.jenkinsci.plugins.structs.describable.DescribableParameter; +import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable; +import org.jenkinsci.plugins.workflow.actions.ArgumentsAction; +import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner; +import org.jenkinsci.plugins.workflow.graph.FlowNode; +import org.jenkinsci.plugins.workflow.graph.StepNode; +import org.jenkinsci.plugins.workflow.steps.StepDescriptor; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.kohsuke.stapler.export.Exported; +import org.kohsuke.stapler.export.ExportedBean; + +import javax.annotation.Nonnull; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * Action to generate the UI report for watched environment variables + */ +@Restricted(NoExternalUse.class) +@ExportedBean +public class InterpolatedSecretsAction implements RunAction2 { + + private List interpolatedWarnings = new ArrayList<>(); + private transient Run run; + + public String getIconFileName() { + return null; + } + + public String getDisplayName() { + return null; + } + + public String getUrlName() { + return null; + } + + public void record(@Nonnull String stepName, @Nonnull List interpolatedVariables) { + interpolatedWarnings.add(new InterpolatedWarnings(stepName, interpolatedVariables)); + } + + @Exported + public List getWarnings() { + return interpolatedWarnings; + } + + public boolean hasWarnings() { + if (interpolatedWarnings.isEmpty()) { + return false; + } else { + return true; + } + } + + public boolean isInProgress() { + return run.isBuilding(); + } + + @Override + public void onAttached(Run run) { + this.run = run; + } + + @Override + public void onLoad(Run run) { + this.run = run; + } + + @ExportedBean + public static class InterpolatedWarnings { + final String stepName; + final List interpolatedVariables; + + InterpolatedWarnings(@Nonnull String stepName, @Nonnull List interpolatedVariables) { + this.stepName = stepName; + this.interpolatedVariables = interpolatedVariables; + } + + @Exported + public String getStepName() { + return stepName; + } + + @Exported + public List getInterpolatedVariables() { + return interpolatedVariables; + } + } +} diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction/summary.jelly b/src/main/resources/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction/summary.jelly new file mode 100644 index 000000000..0ece94ec1 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/workflow/cps/view/InterpolatedSecretsAction/summary.jelly @@ -0,0 +1,38 @@ + + + + + + + ${%The following steps that have been detected may have insecure interpolation of sensitive variables} + (${%click here for an explanation}): + + (${%in progress}) + + +
    + +
  • ${warning.stepName}: ${warning.interpolatedVariables}
    • +     +
+ + + \ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java index 9ec99a78c..fc522a79e 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java @@ -24,16 +24,26 @@ package org.jenkinsci.plugins.workflow.cps; +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl; +import hudson.Functions; import hudson.model.AbstractDescribableImpl; import hudson.model.Descriptor; import hudson.model.Result; + +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; import static org.hamcrest.Matchers.containsString; +import org.hamcrest.MatcherAssert; +import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable; import org.jenkinsci.plugins.workflow.actions.ArgumentsAction; +import org.jenkinsci.plugins.workflow.cps.view.InterpolatedSecretsAction; import org.jenkinsci.plugins.workflow.graph.FlowNode; import org.jenkinsci.plugins.workflow.graphanalysis.LinearScanner; import org.jenkinsci.plugins.workflow.graphanalysis.NodeStepTypePredicate; @@ -49,6 +59,9 @@ import org.jenkinsci.plugins.workflow.testMetaStep.AmbiguousEchoLowerStep; import org.jenkinsci.plugins.workflow.testMetaStep.AmbiguousEchoUpperStep; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; import static org.junit.Assert.*; import org.junit.Assert; @@ -420,6 +433,177 @@ public void namedSoleParamForStep() throws Exception { "'org.jenkinsci.plugins.workflow.steps.SleepStep': comment,units", b); } + @Issue("JENKINS-63254") + @Test public void sensitiveVariableInterpolation() throws Exception { + final String credentialsId = "creds"; + final String username = "bob"; + final String password = "secr3t"; + UsernamePasswordCredentialsImpl c = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", username, password); + CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c); + String shellStep = Functions.isWindows()? "bat" : "sh"; + p.setDefinition(new CpsFlowDefinition("" + + "node {\n" + + "withCredentials([usernamePassword(credentialsId: 'creds', usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD')]) {\n" + + shellStep + " \"echo $PASSWORD\"\n" + + "}\n" + + "}", true)); + WorkflowRun run = r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + r.assertLogContains("Warning: A secret was passed to \""+ shellStep + "\"", run); + r.assertLogContains("Affected argument(s) used the following variable(s): [PASSWORD]", run); + InterpolatedSecretsAction reportAction = run.getAction(InterpolatedSecretsAction.class); + Assert.assertNotNull(reportAction); + List warnings = reportAction.getWarnings(); + MatcherAssert.assertThat(warnings.size(), is(1)); + InterpolatedSecretsAction.InterpolatedWarnings stepWarning = warnings.get(0); + MatcherAssert.assertThat(stepWarning.getStepName(), is(shellStep)); + MatcherAssert.assertThat(stepWarning.getInterpolatedVariables(), is(Arrays.asList("PASSWORD"))); + LinearScanner scan = new LinearScanner(); + FlowNode node = scan.findFirstMatch(run.getExecution().getCurrentHeads().get(0), new NodeStepTypePredicate(shellStep)); + ArgumentsAction argAction = node.getPersistentAction(ArgumentsAction.class); + Assert.assertFalse(argAction.isUnmodifiedArguments()); + MatcherAssert.assertThat(argAction.getArguments().values().iterator().next(), is("echo ${PASSWORD}")); + } + + @Issue("JENKINS-63254") + @Test public void sensitiveVariableInterpolationWithMetaStep() throws Exception { + final String credentialsId = "creds"; + final String username = "bob"; + final String password = "secr3t"; + UsernamePasswordCredentialsImpl c = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", username, password); + CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c); + p.setDefinition(new CpsFlowDefinition("" + + "node {\n" + + "withCredentials([usernamePassword(credentialsId: 'creds', usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD')]) {\n" + + "archiveArtifacts(\"${PASSWORD}\")" + + "}\n" + + "}", true)); + WorkflowRun run = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)); + r.assertLogContains("Warning: A secret was passed to \"archiveArtifacts\"", run); + r.assertLogContains("Affected argument(s) used the following variable(s): [PASSWORD]", run); + InterpolatedSecretsAction reportAction = run.getAction(InterpolatedSecretsAction.class); + Assert.assertNotNull(reportAction); + List warnings = reportAction.getWarnings(); + MatcherAssert.assertThat(warnings.size(), is(1)); + InterpolatedSecretsAction.InterpolatedWarnings stepWarning = warnings.get(0); + MatcherAssert.assertThat(stepWarning.getStepName(), is("archiveArtifacts")); + MatcherAssert.assertThat(stepWarning.getInterpolatedVariables(), is(Arrays.asList("PASSWORD"))); + } + + @Test public void multipleSensitiveVariables() throws Exception { + final String credentialsId = "creds"; + final String username = "bob"; + final String password = "secr3t"; + UsernamePasswordCredentialsImpl c = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", username, password); + CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c); + String shellStep = Functions.isWindows()? "bat" : "sh"; + p.setDefinition(new CpsFlowDefinition("" + + "node {\n" + + "withCredentials([usernamePassword(credentialsId: 'creds', usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD')]) {\n" + + shellStep + " \"echo $PASSWORD $USERNAME $PASSWORD\"\n" + + "}\n" + + "}", true)); + WorkflowRun run = r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + r.assertLogContains("Warning: A secret was passed to \""+ shellStep + "\"", run); + r.assertLogContains("Affected argument(s) used the following variable(s): [PASSWORD, USERNAME]", run); + InterpolatedSecretsAction reportAction = run.getAction(InterpolatedSecretsAction.class); + Assert.assertNotNull(reportAction); + List warnings = reportAction.getWarnings(); + MatcherAssert.assertThat(warnings.size(), is(1)); + InterpolatedSecretsAction.InterpolatedWarnings stepWarning = warnings.get(0); + MatcherAssert.assertThat(stepWarning.getStepName(), is(shellStep)); + MatcherAssert.assertThat(stepWarning.getInterpolatedVariables(), is(Arrays.asList("PASSWORD", "USERNAME"))); + LinearScanner scan = new LinearScanner(); + FlowNode node = scan.findFirstMatch(run.getExecution().getCurrentHeads().get(0), new NodeStepTypePredicate(shellStep)); + ArgumentsAction argAction = node.getPersistentAction(ArgumentsAction.class); + Assert.assertFalse(argAction.isUnmodifiedArguments()); + MatcherAssert.assertThat(argAction.getArguments().values().iterator().next(), is("echo ${PASSWORD} ${USERNAME} ${PASSWORD}")); + } + + @Issue("JENKINS-63254") + @Test public void sensitiveVariableInterpolationWithNestedDescribable() throws Exception { + final String credentialsId = "creds"; + final String username = "bob"; + final String password = "secr3t"; + UsernamePasswordCredentialsImpl c = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", username, password); + CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c); + p.setDefinition(new CpsFlowDefinition("" + + "node {\n" + + "withCredentials([usernamePassword(credentialsId: 'creds', usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD')]) {\n" + + "monomorphWithSymbolStep(monomorphSymbol([firstArg:\"${PASSWORD}\", secondArg:'two']))" + + "}\n" + + "}", true)); + WorkflowRun run = r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + r.assertLogContains("First arg: ****, second arg: two", run); + r.assertLogContains("Warning: A secret was passed to \"monomorphWithSymbolStep\"", run); + r.assertLogContains("Affected argument(s) used the following variable(s): [PASSWORD]", run); + InterpolatedSecretsAction reportAction = run.getAction(InterpolatedSecretsAction.class); + Assert.assertNotNull(reportAction); + List warnings = reportAction.getWarnings(); + MatcherAssert.assertThat(warnings.size(), is(1)); + InterpolatedSecretsAction.InterpolatedWarnings stepWarning = warnings.get(0); + MatcherAssert.assertThat(stepWarning.getStepName(), is("monomorphWithSymbolStep")); +// MatcherAssert.assertThat(stepWarning.getStepName(), is("monomorphWithSymbolStep(data: monomorphSymbol(firstArg: ${PASSWORD}, secondArg: two))")); + MatcherAssert.assertThat(stepWarning.getInterpolatedVariables(), is(Arrays.asList("PASSWORD"))); + LinearScanner scan = new LinearScanner(); + FlowNode node = scan.findFirstMatch(run.getExecution().getCurrentHeads().get(0), new NodeStepTypePredicate("monomorphWithSymbolStep")); + ArgumentsAction argAction = node.getPersistentAction(ArgumentsAction.class); + Assert.assertFalse(argAction.isUnmodifiedArguments()); + Object var = argAction.getArguments().values().iterator().next(); + MatcherAssert.assertThat(var, instanceOf(UninstantiatedDescribable.class)); + MatcherAssert.assertThat(((UninstantiatedDescribable)var).getArguments().toString(), is("{firstArg=${PASSWORD}, secondArg=two}")); + } + + @Issue("JENKINS-63254") + @Test public void complexSensitiveVariableInterpolationWithNestedDescribable() throws Exception { + final String credentialsId = "creds"; + final String username = "bob"; + final String password = "secr3t"; + UsernamePasswordCredentialsImpl c = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", username, password); + CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), c); + p.setDefinition(new CpsFlowDefinition("" + + "node {\n" + + "withCredentials([usernamePassword(credentialsId: 'creds', usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD')]) {\n" + + "monomorphListSymbolStep([monomorphSymbol(firstArg: monomorphWithSymbolStep(monomorphSymbol([firstArg: \"innerFirstArgIs${PASSWORD}\", secondArg: \"innerSecondArgIs${USERNAME}\"])), secondArg: \"hereismy${PASSWORD}\"), monomorphSymbol(firstArg: \"${PASSWORD}\", secondArg: \"${USERNAME}\")])" + + "}\n" + + "}", true)); + WorkflowRun run = r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + r.assertLogContains("Warning: A secret was passed to \"monomorphWithSymbolStep\"", run); + r.assertLogContains("Affected argument(s) used the following variable(s): [PASSWORD, USERNAME]", run); + r.assertLogContains("Warning: A secret was passed to \"monomorphListSymbolStep\"", run); + r.assertLogNotContains("Affected argument(s) used the following variable(s): [PASSWORD]", run); + InterpolatedSecretsAction reportAction = run.getAction(InterpolatedSecretsAction.class); + Assert.assertNotNull(reportAction); + List warnings = reportAction.getWarnings(); + MatcherAssert.assertThat(warnings.size(), is(2)); + InterpolatedSecretsAction.InterpolatedWarnings stepWarning = warnings.get(0); + MatcherAssert.assertThat(stepWarning.getStepName(), is("monomorphWithSymbolStep")); + MatcherAssert.assertThat(stepWarning.getInterpolatedVariables(), equalTo(Arrays.asList("PASSWORD", "USERNAME"))); + InterpolatedSecretsAction.InterpolatedWarnings listStepWarning = warnings.get(1); + MatcherAssert.assertThat(listStepWarning.getStepName(), is("monomorphListSymbolStep")); + MatcherAssert.assertThat(listStepWarning.getInterpolatedVariables(), equalTo(Arrays.asList("PASSWORD", "USERNAME"))); + } + + @Test public void noBodyError() throws Exception { + p.setDefinition((new CpsFlowDefinition("timeout(time: 1, unit: 'SECONDS')", true))); + WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)); + r.assertLogContains("timeout step must be called with a body", b); + } + + @Test public void legacyStage() throws Exception { + p.setDefinition(new CpsFlowDefinition( + "stage(name: 'A');\n" + + "echo('done')", true)); + WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + } + + @Test public void standardStage() throws Exception { + p.setDefinition(new CpsFlowDefinition( + "stage('Build'){\n" + + " echo('building')\n" + + "}\n", true)); + WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + } + public static class CLStep extends Step { public final String name; @DataBoundConstructor public CLStep(String name) {this.name = name;} diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/ParamsVariableTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/ParamsVariableTest.java index 200e8583c..51ee3c7f7 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/ParamsVariableTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/ParamsVariableTest.java @@ -30,6 +30,7 @@ import hudson.model.ParametersDefinitionProperty; import hudson.model.PasswordParameterDefinition; import hudson.model.PasswordParameterValue; +import hudson.model.Result; import hudson.model.StringParameterDefinition; import hudson.model.StringParameterValue; import org.jenkinsci.plugins.workflow.job.WorkflowJob; @@ -65,7 +66,8 @@ public class ParamsVariableTest { WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition("echo(/TEXT=${params.TEXT}/)",true)); p.addProperty(new ParametersDefinitionProperty(new StringParameterDefinition("TEXT", ""))); - r.assertLogContains("TEXT=null", r.assertBuildStatusSuccess(p.scheduleBuild2(0, new ParametersAction(new StringParameterValue("TEXT", /* not possible via UI, but to simulate other ParameterValue impls */null))))); + WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0, new ParametersAction(new StringParameterValue("TEXT", /* not possible via UI, but to simulate other ParameterValue impls */null)))); + r.assertLogContains("Null value not allowed as an environment variable: TEXT", b); } } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImplTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImplTest.java index 3b38d4113..8b1c03260 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImplTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/actions/ArgumentsActionImplTest.java @@ -13,8 +13,13 @@ import hudson.Functions; import hudson.XmlFile; import hudson.model.Action; +import hudson.model.ParametersAction; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.PasswordParameterDefinition; +import hudson.model.PasswordParameterValue; import hudson.tasks.ArtifactArchiver; import org.apache.commons.lang.RandomStringUtils; +import org.hamcrest.MatcherAssert; import org.hamcrest.Matchers; import org.hamcrest.collection.IsMapContaining; import org.jenkinsci.plugins.credentialsbinding.impl.BindingStep; @@ -47,6 +52,7 @@ import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import org.jvnet.hudson.test.BuildWatcher; @@ -156,35 +162,31 @@ public void testStringSafetyTest() throws Exception { String input = "I have a secret p4ssw0rd"; HashMap passwordBinding = new HashMap<>(); passwordBinding.put("mypass", "p4ssw0rd"); - Assert.assertTrue("Input with no variables is safe", ArgumentsActionImpl.isStringSafe(input, new EnvVars(), Collections.EMPTY_SET)); - Assert.assertFalse("Input containing bound value is unsafe", ArgumentsActionImpl.isStringSafe(input, new EnvVars(passwordBinding), Collections.EMPTY_SET)); - - Assert.assertTrue("EnvVars that do not occur are safe", ArgumentsActionImpl.isStringSafe("I have no passwords", new EnvVars(passwordBinding), Collections.EMPTY_SET)); - - HashMap safeBinding = new HashMap<>(); - safeBinding.put("harmless", "secret"); - HashSet safeVars = new HashSet<>(); - safeVars.add("harmless"); - passwordBinding.put("harmless", "secret"); - Assert.assertTrue("Input containing whitelisted bound value is safe", ArgumentsActionImpl.isStringSafe(input, new EnvVars(safeBinding), safeVars)); - Assert.assertFalse("Input containing one safe and one unsafe bound value is unsafe", ArgumentsActionImpl.isStringSafe(input, new EnvVars(passwordBinding), safeVars)); + Set sensitiveVariables = new HashSet<>(); + sensitiveVariables.add("mypass"); + MatcherAssert.assertThat("Input with no variables is safe", ArgumentsActionImpl.replaceSensitiveVariables(input, new EnvVars(), sensitiveVariables), is(input)); + MatcherAssert.assertThat("Input containing bound value is unsafe", ArgumentsActionImpl.replaceSensitiveVariables(input, new EnvVars(passwordBinding), sensitiveVariables), is("I have a secret ${mypass}")); + MatcherAssert.assertThat("EnvVars that do not occur are safe", ArgumentsActionImpl.replaceSensitiveVariables("I have no passwords", new EnvVars(passwordBinding), sensitiveVariables), is("I have no passwords")); } @Test public void testRecursiveSanitizationOfContent() { - int maxLen = ArgumentsActionImpl.getMaxRetainedLength(); - ArgumentsActionImpl impl = new ArgumentsActionImpl(); - EnvVars env = new EnvVars(); String secretUsername = "secretuser"; env.put("USERVARIABLE", secretUsername); // assume secretuser is a bound credential + Set sensitiveVariables = new HashSet<>(); + sensitiveVariables.add("USERVARIABLE"); + + int maxLen = ArgumentsActionImpl.getMaxRetainedLength(); + ArgumentsActionImpl impl = new ArgumentsActionImpl(sensitiveVariables); + char[] oversized = new char[maxLen+10]; Arrays.fill(oversized, 'a'); String oversizedString = new String (oversized); // Simplest masking of secret and oversized value - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, impl.sanitizeObjectAndRecordMutation(secretUsername, env)); + Assert.assertEquals("${USERVARIABLE}", impl.sanitizeObjectAndRecordMutation(secretUsername, env)); Assert.assertFalse(impl.isUnmodifiedArguments()); impl.isUnmodifiedBySanitization = true; @@ -196,12 +198,12 @@ public void testRecursiveSanitizationOfContent() { Step mystep = new EchoStep("I have a "+secretUsername); Map singleSanitization = (Map)(impl.sanitizeObjectAndRecordMutation(mystep, env)); Assert.assertEquals(1, singleSanitization.size()); - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, singleSanitization.get("message")); + Assert.assertEquals("I have a ${USERVARIABLE}", singleSanitization.get("message")); Assert.assertFalse(impl.isUnmodifiedArguments()); impl.isUnmodifiedBySanitization = true; singleSanitization = ((UninstantiatedDescribable) (impl.sanitizeObjectAndRecordMutation(mystep.getDescriptor().uninstantiate(mystep), env))).getArguments(); Assert.assertEquals(1, singleSanitization.size()); - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, singleSanitization.get("message")); + Assert.assertEquals("I have a ${USERVARIABLE}", singleSanitization.get("message")); Assert.assertFalse(impl.isUnmodifiedArguments()); impl.isUnmodifiedBySanitization = true; @@ -210,7 +212,7 @@ public void testRecursiveSanitizationOfContent() { dangerous.put("name", secretUsername); Map sanitizedMap = impl.sanitizeMapAndRecordMutation(dangerous, env); Assert.assertNotEquals(sanitizedMap, dangerous); - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, sanitizedMap.get("name")); + Assert.assertEquals("${USERVARIABLE}", sanitizedMap.get("name")); Assert.assertFalse(impl.isUnmodifiedArguments()); impl.isUnmodifiedBySanitization = true; @@ -223,7 +225,7 @@ public void testRecursiveSanitizationOfContent() { List sanitized = (List)impl.sanitizeListAndRecordMutation(unsanitizedList, env); Assert.assertEquals(3, sanitized.size()); Assert.assertFalse(impl.isUnmodifiedArguments()); - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, sanitized.get(2)); + Assert.assertEquals("${USERVARIABLE}", sanitized.get(2)); impl.isUnmodifiedBySanitization = true; Assert.assertEquals(unsanitizedList, impl.sanitizeObjectAndRecordMutation(unsanitizedList, new EnvVars())); @@ -235,18 +237,20 @@ public void testArraySanitization() { EnvVars env = new EnvVars(); String secretUsername = "IAmA"; env.put("USERVARIABLE", secretUsername); // assume secretuser is a bound credential + Set sensitiveVariables = new HashSet<>(); + sensitiveVariables.add("USERVARIABLE"); HashMap args = new HashMap<>(); args.put("ints", new int[]{1,2,3}); args.put("strings", new String[]{"heh",secretUsername,"lumberjack"}); - ArgumentsActionImpl filtered = new ArgumentsActionImpl(args, env); + ArgumentsActionImpl filtered = new ArgumentsActionImpl(args, env, sensitiveVariables); Map filteredArgs = filtered.getArguments(); Assert.assertEquals(2, filteredArgs.size()); Assert.assertThat(filteredArgs, IsMapContaining.hasEntry("ints", ArgumentsAction.NotStoredReason.UNSERIALIZABLE)); Assert.assertThat(filteredArgs, IsMapContaining.hasKey("strings")); Object[] contents = (Object[])(filteredArgs.get("strings")); - Assert.assertArrayEquals(new Object[]{"heh", ArgumentsAction.NotStoredReason.MASKED_VALUE, "lumberjack"}, (Object[])(filteredArgs.get("strings"))); + Assert.assertArrayEquals(new Object[]{"heh", "${USERVARIABLE}", "lumberjack"}, (Object[])(filteredArgs.get("strings"))); } @Test @@ -280,24 +284,26 @@ public void testBasicCreateAndMask() throws Exception { passwordBinding.put("mypass", "p4ssw0rd"); Map arguments = new HashMap<>(); arguments.put("message", "I have a secret p4ssw0rd"); + Set sensitiveVariables = new HashSet<>(); + sensitiveVariables.add("mypass"); Field maxSizeF = ArgumentsAction.class.getDeclaredField("MAX_RETAINED_LENGTH"); maxSizeF.setAccessible(true); int maxSize = maxSizeF.getInt(null); // Same string, unsanitized - ArgumentsActionImpl argumentsActionImpl = new ArgumentsActionImpl(arguments, new EnvVars()); + ArgumentsActionImpl argumentsActionImpl = new ArgumentsActionImpl(arguments, new EnvVars(), sensitiveVariables); Assert.assertTrue(argumentsActionImpl.isUnmodifiedArguments()); Assert.assertEquals(arguments.get("message"), argumentsActionImpl.getArgumentValueOrReason("message")); Assert.assertEquals(1, argumentsActionImpl.getArguments().size()); Assert.assertEquals("I have a secret p4ssw0rd", argumentsActionImpl.getArguments().get("message")); // Test sanitizing arguments now - argumentsActionImpl = new ArgumentsActionImpl(arguments, new EnvVars(passwordBinding)); + argumentsActionImpl = new ArgumentsActionImpl(arguments, new EnvVars(passwordBinding), sensitiveVariables); Assert.assertFalse(argumentsActionImpl.isUnmodifiedArguments()); - Assert.assertEquals(ArgumentsActionImpl.NotStoredReason.MASKED_VALUE, argumentsActionImpl.getArgumentValueOrReason("message")); + Assert.assertEquals("I have a secret ${mypass}", argumentsActionImpl.getArgumentValueOrReason("message")); Assert.assertEquals(1, argumentsActionImpl.getArguments().size()); - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, argumentsActionImpl.getArguments().get("message")); + Assert.assertEquals("I have a secret ${mypass}", argumentsActionImpl.getArguments().get("message")); // Mask oversized values arguments.clear(); @@ -424,8 +430,7 @@ public void testBasicCredentials() throws Exception { filtered = scanner.filteredNodes(exec, new DescriptorMatchPredicate(EchoStep.DescriptorImpl.class)); for (FlowNode f : filtered) { act = f.getPersistentAction(ArgumentsActionImpl.class); - Assert.assertEquals(ArgumentsAction.NotStoredReason.MASKED_VALUE, act.getArguments().get("message")); - Assert.assertNull(ArgumentsAction.getStepArgumentsAsString(f)); + MatcherAssert.assertThat((String) act.getArguments().get("message"), allOf(not(containsString("bob")), not(containsString("s3cr3t")))); } List allStepped = scanner.filteredNodes(run.getExecution().getCurrentHeads(), FlowScanningUtils.hasActionPredicate(ArgumentsActionImpl.class)); @@ -621,6 +626,22 @@ public void testReallyUnusualStepInstantiations() throws Exception { equalTo(NotStoredReason.UNSERIALIZABLE)); } + @Issue("JENKINS-47101") + @Test public void passwordParametersSanitized() throws Exception { + WorkflowJob p = r.createProject(WorkflowJob.class); + p.addProperty(new ParametersDefinitionProperty( + Arrays.asList(new PasswordParameterDefinition("MYPASSWORD", "mysecret", "description")))); + p.setDefinition(new CpsFlowDefinition("echo(\"$MYPASSWORD\")", true)); + ParametersAction paramsAction = new ParametersAction(Arrays.asList(new PasswordParameterValue("MYPASSWORD", "mysecret"))); + WorkflowRun b = p.scheduleBuild2(0, paramsAction).waitForStart(); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + LinearScanner scan = new LinearScanner(); + FlowNode shNode = scan.findFirstMatch(b.getExecution().getCurrentHeads().get(0), new NodeStepTypePredicate("echo")); + ArgumentsAction args = shNode.getPersistentAction(ArgumentsAction.class); + assertThat(args.isUnmodifiedArguments(), equalTo(false)); + assertThat(args.getArguments(), hasEntry("message", "${MYPASSWORD}")); + } + public static class NopStep extends Step { @DataBoundConstructor public NopStep(Object value) {}