Skip to content

Commit

Permalink
[JENKINS-25737, JENKINS-41955] - Rework the broken PasswordParameterV…
Browse files Browse the repository at this point in the history 
…alue handling approach
  • Loading branch information
@oleg-nenashev
oleg-nenashev committed Mar 16, 2017
1 parent 391f3a9 commit ffd1d9b
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 74 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/michelin/cio/hudson/plugins/maskpasswords/MaskPasswordsBuildWrapper.java
View file
Expand Up @@ -129,7 +129,7 @@ public ConsoleLogFilter createLoggerDecorator(Run<?, ?> build) {
ParametersAction params = build.getAction(ParametersAction.class);
if(params != null) {
for(ParameterValue param : params) {
if(config.isMasked(param.getClass().getName())) {
if(config.isMasked(param, param.getClass().getName())) {
EnvVars env = new EnvVars();
param.buildEnvironment(build, env);
String password = env.get(param.getName());
Expand Down
204 changes: 138 additions & 66 deletions src/main/java/com/michelin/cio/hudson/plugins/maskpasswords/MaskPasswordsConfig.java
View file
Expand Up @@ -39,12 +39,15 @@
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import javax.annotation.concurrent.GuardedBy;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
Expand All @@ -68,12 +71,23 @@ public class MaskPasswordsConfig {
* Contains the set of {@link ParameterDefinition}s whose value must be
* masked in builds' console.
*/
@GuardedBy("this")
private Set<String> maskPasswordsParamDefClasses;
/**
* Contains the set of {@link ParameterValue}s whose value must be masked in
* builds' console.
*/
private transient Set<String> maskPasswordsParamValueClasses;
@Nonnull
@GuardedBy("this")
private final transient Set<String> paramValueCache_maskedClasses = new HashSet<String>();

/**
* Cache of values, which are not subjects for masking.
*/
@Nonnull
@GuardedBy("this")
private final transient Set<String> paramValueCache_nonMaskedClasses = new HashSet<String>();

/**
* Users can define name/password pairs at the global level to share common
* passwords with several jobs.
Expand Down Expand Up @@ -148,19 +162,29 @@ public void addGlobalVarMaskRegex(VarMaskRegex varMaskRegex) {
* to the list of parameters which will prevent the rebuild
* action to be enabled for a build
*/
public void addMaskedPasswordParameterDefinition(String className) {
public synchronized void addMaskedPasswordParameterDefinition(String className) {
maskPasswordsParamDefClasses.add(className);
// Maybe is it masked now
paramValueCache_nonMaskedClasses.clear();
}

public void setGlobalVarEnabledGlobally(boolean state) {
globalVarEnableGlobally = state;
}

public void clear() {
public synchronized void clear() {
maskPasswordsParamDefClasses.clear();
getGlobalVarPasswordPairsList().clear();
getGlobalVarMaskRegexesList().clear();
globalVarEnableGlobally = false;

// Drop caches
invalidatePasswordValueClassCaches();
}

public synchronized void invalidatePasswordValueClassCaches() {
paramValueCache_maskedClasses.clear();
paramValueCache_nonMaskedClasses.clear();
}

public static MaskPasswordsConfig getInstance() {
Expand Down Expand Up @@ -272,83 +296,131 @@ public boolean isEnabledGlobally() {
return globalVarEnableGlobally;
}

/**
* Check if the parameter value class needs to be masked
* @deprecated There is a high risk of false-negatives. Use {@link #isMasked(hudson.model.ParameterValue, java.lang.String)} at least
* @param paramValueClassName Class name of the {@link ParameterValue}
* @return {@code true} if the parameter value should be masked.
* {@code false} if the plugin is not sure, may be false-negative
*/
@Deprecated
public synchronized boolean isMasked(final @Nonnull String paramValueClassName) {
return isMasked(null, paramValueClassName);
}

/**
* Returns true if the specified parameter value class name corresponds to
* a parameter definition class name selected in Jenkins' main
* configuration screen.
* @param value Parameter value. Without it there is a high risk of false negatives.
* @param paramValueClassName Class name of the {@link ParameterValue} class implementation
* @return {@code true} if the parameter value should be masked.
* {@code false} if the plugin is not sure, may be false-negative especially if the value is {@code null}.
* @since TODO
*/
public synchronized boolean isMasked(String paramValueClassName) {
try {
// do we need to build the set of parameter values which must be
// masked?
if(maskPasswordsParamValueClasses == null) {
maskPasswordsParamValueClasses = new LinkedHashSet<String>();

// The only way to find parameter definition/parameter value
// couples is to reflect the methods of parameter definition
// classes which instantiate the parameter value.
// This means that this algorithm expects that the developers do
// clearly redefine the return type when implementing parameter
// definitions/values.
for(String paramDefClassName: maskPasswordsParamDefClasses) {
final Class paramDefClass = Jenkins.getActiveInstance().getPluginManager().uberClassLoader.loadClass(paramDefClassName);

List<Method> methods = new ArrayList<Method>() {{
// ParameterDefinition.getDefaultParameterValue()
try {
add(paramDefClass.getMethod("getDefaultParameterValue"));
} catch(RuntimeException e) {
LOGGER.log(Level.INFO, "No getDefaultParameterValue(String) method for {0}", paramDefClass);
}
// ParameterDefinition.createValue(String)
// This method does not exist anymore, but many classes still use it
try {
add(paramDefClass.getMethod("createValue", String.class));
} catch(RuntimeException e) {
LOGGER.log(Level.INFO, "No createValue(String) method for {0}", paramDefClass);
}
// ParameterDefinition.createValue(org.kohsuke.stapler.StaplerRequest, net.sf.json.JSONObject)
try {
add(paramDefClass.getMethod("createValue", StaplerRequest.class, JSONObject.class));
} catch (RuntimeException e) {
LOGGER.log(Level.INFO, "No createValue(StaplerRequest, JSONObject) method for {0}", paramDefClass);
}
// ParameterDefinition.createValue(org.kohsuke.stapler.StaplerRequest)
try {
add(paramDefClass.getMethod("createValue", StaplerRequest.class));
} catch (RuntimeException e) {
LOGGER.log(Level.INFO, "No createValue(StaplerRequest) method for {0}", paramDefClass);
}
// ParameterDefinition.createValue(CLICommand command, String value)
try {
add(paramDefClass.getMethod("createValue", CLICommand.class, String.class));
} catch(RuntimeException e) {
LOGGER.log(Level.INFO, "No createValue(CLICommand, String) method for {0}", paramDefClass);
}
}};

// If the class defines any method, we consider it as a ParameterDefinition.
// This parameter definition may produce any ParameterValue, and effectively we cannot guarantee the
// value is a type or a subtype of the requested parameter.
if(!methods.isEmpty()) {
maskPasswordsParamValueClasses.add(paramValueClassName);
}
}
public boolean isMasked(final @CheckForNull ParameterValue value,
final @Nonnull String paramValueClassName) {

// We always mask sensitive variables, the configuration does not matter in such case
if (value != null && value.isSensitive()) {
return true;
}

synchronized(this) {
// Check if the value is in the cache
if (paramValueCache_maskedClasses.contains(paramValueClassName)) {
return true;
}
if (paramValueCache_nonMaskedClasses.contains(paramValueClassName)) {
return false;
}

// Now guess
boolean guessSo = guessIfShouldMask(paramValueClassName);
if (guessSo) {
// We are pretty sure it requires masking
paramValueCache_maskedClasses.add(paramValueClassName);
return true;
} else {
// It does not require masking, but we are not so sure
// The warning will be printed each time the cache is invalidated due to whatever reason
LOGGER.log(Level.WARNING, "Identified the {0} class as a ParameterValue class, which does not require masking. It may be false-negative", paramValueClassName);
paramValueCache_nonMaskedClasses.add(paramValueClassName);
return false;
}
}
catch(Exception e) {
LOGGER.log(Level.WARNING, "Error while initializing Mask Passwords: " + e);
return false;
}

//TODO: add support of specifying masked parameter values byt the... parameter value classs name. So obvious, yeah?
/**
* Tries to guess if the parameter value class should be masked.
* @param paramValueClassName Parameter value class name
* @return {@code true} if we are sure that the class has to be masked
* {@code false} otherwise, there is a risk of false negative due to the presumptions.
*/
/*package*/ synchronized boolean guessIfShouldMask(final @Nonnull String paramValueClassName) {
// The only way to find parameter definition/parameter value
// couples is to reflect the methods of parameter definition
// classes which instantiate the parameter value.
// This means that this algorithm expects that the developers do
// clearly redefine the return type when implementing parameter
// definitions/values.
for(String paramDefClassName: maskPasswordsParamDefClasses) {
final Class<?> paramDefClass;
try {
paramDefClass = Jenkins.getActiveInstance().getPluginManager().uberClassLoader.loadClass(paramDefClassName);
} catch (ClassNotFoundException ex) {
LOGGER.log(Level.WARNING, "Cannot check ParamDef for masking " + paramDefClassName, ex);
continue;
}

tryProcessMethod(paramDefClass, "getDefaultParameterValue", true);
tryProcessMethod(paramDefClass, "createValue", true, StaplerRequest.class, JSONObject.class);
tryProcessMethod(paramDefClass, "createValue", true, StaplerRequest.class);
tryProcessMethod(paramDefClass, "createValue", true, CLICommand.class, String.class);
// This custom implementation is not a part of the API, but let's try it
tryProcessMethod(paramDefClass, "createValue", false, String.class);

// If the parameter value class has been added to the cache, exit
if (paramValueCache_maskedClasses.contains(paramValueClassName)) {
return true;
}
}

return maskPasswordsParamValueClasses.contains(paramValueClassName);
return false;
}

/**
* Processes the methods in the {@link ParameterValue} class and caches all ParameterValue implementations as ones requiring masking.
* @param clazz Class
* @param methodName Method name
* @param parameterTypes Parameters
*/
private synchronized void tryProcessMethod(Class<?> clazz, String methodName, boolean expectedToExist, Class<?> ... parameterTypes) {
try {
Method method = clazz.getMethod(methodName, parameterTypes);
Class<?> returnType = method.getReturnType();
// We do not veto the the root class
if (ParameterValue.class.isAssignableFrom(returnType)) {
if (!ParameterValue.class.equals(returnType)) {
// Add this class to the cache
paramValueCache_maskedClasses.add(returnType.getName());
}
}
} catch (Exception ex) {
Level logLevel = expectedToExist ? Level.INFO : Level.CONFIG;
if (LOGGER.isLoggable(logLevel)) {
String methodSpec = String.format("%s(%s)", methodName, StringUtils.join(parameterTypes, ","));
LOGGER.log(logLevel, "No method {0} for class {1}", new Object[] {methodSpec, clazz});
}
}
}

/**
* Returns true if the specified parameter definition class name has been
* selected in Hudson's/Jenkins' main configuration screen.
* selected in Jenkins main configuration screen.
*/
public boolean isSelected(String paramDefClassName) {
public synchronized boolean isSelected(String paramDefClassName) {
return maskPasswordsParamDefClasses.contains(paramDefClassName);
}

Expand Down
6 changes: 0 additions & 6 deletions src/main/java/com/michelin/cio/hudson/plugins/passwordparam/PasswordParameterDefinition.java
View file
Expand Up @@ -46,12 +46,6 @@ public ParameterValue createValue(Secret password) {
return value;
}

public ParameterValue createValue(String password) {
PasswordParameterValue value = new PasswordParameterValue(getName(), password, getDescription());
value.setDescription(getDescription());
return value;
}

@Override
public ParameterValue createValue(StaplerRequest req) {
String[] value = req.getParameterValues(getName());
Expand Down
15 changes: 14 additions & 1 deletion src/test/java/com/michelin/cio/hudson/plugins/passwordparam/PasswordParameterTest.java
View file
Expand Up @@ -31,15 +31,17 @@
import hudson.model.Cause;
import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.model.ParameterDefinition;
import hudson.model.ParameterValue;
import hudson.model.ParametersDefinitionProperty;
import hudson.model.Result;
import hudson.util.Secret;
import java.io.IOException;
import java.util.Collections;
import org.junit.Assert;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runners.model.Statement;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
Expand All @@ -64,6 +66,17 @@ public void shouldMaskPasswordParameterClassByDefault() {
MaskPasswordsConfig.getInstance().isMasked(PasswordParameterValue.class.getName()));
}

@Test
@Issue("JENKINS-41955")
public void shouldMaskPasswordParameterValueByDefault() {
PasswordParameterDefinition d = new PasswordParameterDefinition("FOO", "BAR");
ParameterValue created = d.createValue(Secret.fromString("hello"));

// We pass the non-existent class name in order to ensure that the Value metadata check is enough
Assert.assertTrue( PasswordParameterValue.class + " must be masked by default",
MaskPasswordsConfig.getInstance().isMasked(created, "nonExistent"));
}

@Test
@Issue("JENKINS-41955")
public void passwordParameterShouldBeMaskedInFreestyleProject() throws Exception {
Expand Down

0 comments on commit ffd1d9b

Please sign in to comment.