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 @@
8false1.32
+ 3.612.19.06.14.81.10.0
@@ -96,6 +97,7 @@
org.jenkins-ci.plugins.workflowworkflow-support
+ ${workflow-support-plugin.version}org.jenkins-ci.plugins.workflow
@@ -104,7 +106,6 @@
org.jenkins-ci.pluginsscript-security
- 1.75org.jenkins-ci.plugins
@@ -135,6 +136,7 @@
org.jenkins-ci.plugins.workflowworkflow-support
+ ${workflow-support-plugin.version}teststest
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 extends Step> 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