diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 25d485bf1d84..04d1a5ffde04 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -166,6 +166,7 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; +import java.util.stream.Collectors; import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import org.apache.commons.io.IOUtils; @@ -2013,21 +2014,66 @@ public List getLoggerNames() { * Used by {@code } so that we send an encrypted value to the client. */ public String getPasswordValue(Object o) { - if (o==null) return null; - if (o instanceof Secret) { - StaplerRequest req = Stapler.getCurrentRequest(); + if (o == null) { + return null; + } + + /* + Return plain value if it's the default value for PasswordParameterDefinition. + This needs to work even when the user doesn't have CONFIGURE permission + */ + if (o.equals(PasswordParameterDefinition.DEFAULT_VALUE)) { + return o.toString(); + } + + /* Mask from Extended Read */ + StaplerRequest req = Stapler.getCurrentRequest(); + if (o instanceof Secret || Secret.BLANK_NONSECRET_PASSWORD_FIELDS_WITHOUT_ITEM_CONFIGURE) { if (req != null) { Item item = req.findAncestorObject(Item.class); if (item != null && !item.hasPermission(Item.CONFIGURE)) { return "********"; } } + } + + /* Return encrypted value if it's a Secret */ + if (o instanceof Secret) { return ((Secret) o).getEncryptedValue(); } - if (getIsUnitTest() && !o.equals(PasswordParameterDefinition.DEFAULT_VALUE)) { - throw new SecurityException("attempted to render plaintext ‘" + o + "’ in password field; use a getter of type Secret instead"); + + /* Log a warning if we're in development mode (core or plugin): There's an f:password backed by a non-Secret */ + if (req != null && (Boolean.getBoolean("hudson.hpi.run") || Boolean.getBoolean("hudson.Main.development"))) { + LOGGER.log(Level.WARNING, () -> " form control in " + getJellyViewsInformationForCurrentRequest() + + " is not backed by hudson.util.Secret. Learn more: https://jenkins.io/redirect/hudson.util.Secret"); + } + + /* Return plain value if it's not a Secret and the escape hatch is set */ + if (!Secret.AUTO_ENCRYPT_PASSWORD_CONTROL) { + return o.toString(); + } + + /* Make it a Secret and return its encrypted value */ + return Secret.fromString(o.toString()).getEncryptedValue(); + } + + private String getJellyViewsInformationForCurrentRequest() { + final Thread thread = Thread.currentThread(); + String threadName = thread.getName(); + + // try to simplify based on org.kohsuke.stapler.jelly.JellyViewScript + // Views are expected to contain a slash and a period, neither as the first char, and the last slash before the first period: Class/view.jelly + // Nested classes use slashes, so we do not expect period before: Class/Nested/view.jelly + String views = Arrays.stream(threadName.split(" ")).filter(part -> { + int slash = part.lastIndexOf("/"); + int firstPeriod = part.indexOf("."); + return slash > 0 && firstPeriod > 0 && slash < firstPeriod; + }).collect(Collectors.joining(" ")); + if (StringUtils.isBlank(views)) { + // fallback to full thread name if there are no apparent views + return threadName; } - return o.toString(); + return views; } public List filterDescriptors(Object context, Iterable descriptors) { diff --git a/core/src/main/java/hudson/model/PasswordParameterDefinition.java b/core/src/main/java/hudson/model/PasswordParameterDefinition.java index ef71d9c51f37..6f5f86fb28e6 100644 --- a/core/src/main/java/hudson/model/PasswordParameterDefinition.java +++ b/core/src/main/java/hudson/model/PasswordParameterDefinition.java @@ -46,12 +46,18 @@ public class PasswordParameterDefinition extends SimpleParameterDefinition { private Secret defaultValue; - @DataBoundConstructor + @Deprecated public PasswordParameterDefinition(String name, String defaultValue, String description) { super(name, description); this.defaultValue = Secret.fromString(defaultValue); } + @DataBoundConstructor + public PasswordParameterDefinition(String name, Secret defaultValueAsSecret, String description) { + super(name, description); + this.defaultValue = defaultValueAsSecret; + } + @Override public ParameterDefinition copyWithDefaultValue(ParameterValue defaultValue) { if (defaultValue instanceof PasswordParameterValue) { diff --git a/core/src/main/java/hudson/model/PasswordParameterValue.java b/core/src/main/java/hudson/model/PasswordParameterValue.java index cfd0004f9c37..6bac6fa59f72 100644 --- a/core/src/main/java/hudson/model/PasswordParameterValue.java +++ b/core/src/main/java/hudson/model/PasswordParameterValue.java @@ -44,12 +44,18 @@ public PasswordParameterValue(String name, String value) { this(name, value, null); } - @DataBoundConstructor + @Deprecated public PasswordParameterValue(String name, String value, String description) { super(name, description); this.value = Secret.fromString(value); } + @DataBoundConstructor + public PasswordParameterValue(String name, Secret value, String description) { + super(name, description); + this.value = value; + } + @Override public void buildEnvironment(Run build, EnvVars env) { String v = Secret.toString(value); diff --git a/core/src/main/java/hudson/util/Secret.java b/core/src/main/java/hudson/util/Secret.java index bbda0d6d6a66..f635c277e3f4 100644 --- a/core/src/main/java/hudson/util/Secret.java +++ b/core/src/main/java/hudson/util/Secret.java @@ -301,6 +301,12 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont private static final long serialVersionUID = 1L; + @Restricted(NoExternalUse.class) + public static final boolean AUTO_ENCRYPT_PASSWORD_CONTROL = SystemProperties.getBoolean(Secret.class.getName() + ".AUTO_ENCRYPT_PASSWORD_CONTROL", true); + + @Restricted(NoExternalUse.class) + public static /* non-final */ boolean BLANK_NONSECRET_PASSWORD_FIELDS_WITHOUT_ITEM_CONFIGURE = SystemProperties.getBoolean(Secret.class.getName() + ".BLANK_NONSECRET_PASSWORD_FIELDS_WITHOUT_ITEM_CONFIGURE", true); + static { Stapler.CONVERT_UTILS.register(new org.apache.commons.beanutils.Converter() { public Secret convert(Class type, Object value) { @@ -310,9 +316,23 @@ public Secret convert(Class type, Object value) { if (value instanceof Secret) { return (Secret) value; } - return Secret.fromString(value.toString()); } }, Secret.class); + if (AUTO_ENCRYPT_PASSWORD_CONTROL) { + Stapler.CONVERT_UTILS.register(new org.apache.commons.beanutils.Converter() { + public String convert(Class type, Object value) { + if (value == null) { + return null; + } + Secret decrypted = Secret.decrypt(value.toString()); + if (decrypted == null) { + return value.toString(); + } else { + return decrypted.getPlainText(); + } + } + }, String.class); + } } } diff --git a/core/src/main/resources/hudson/model/PasswordParameterDefinition/config.jelly b/core/src/main/resources/hudson/model/PasswordParameterDefinition/config.jelly index 8035d1f4f7e1..f934d7535a95 100644 --- a/core/src/main/resources/hudson/model/PasswordParameterDefinition/config.jelly +++ b/core/src/main/resources/hudson/model/PasswordParameterDefinition/config.jelly @@ -29,8 +29,8 @@ THE SOFTWARE. - - + + diff --git a/core/src/main/resources/hudson/model/PasswordParameterValue/value.jelly b/core/src/main/resources/hudson/model/PasswordParameterValue/value.jelly index e4a500773074..100b383154d0 100644 --- a/core/src/main/resources/hudson/model/PasswordParameterValue/value.jelly +++ b/core/src/main/resources/hudson/model/PasswordParameterValue/value.jelly @@ -26,8 +26,9 @@ THE SOFTWARE. - - - + + + + ${%hidden} \ No newline at end of file diff --git a/core/src/main/resources/hudson/model/PasswordParameterValue/value.properties b/core/src/main/resources/hudson/model/PasswordParameterValue/value.properties new file mode 100644 index 000000000000..8824b1a741a0 --- /dev/null +++ b/core/src/main/resources/hudson/model/PasswordParameterValue/value.properties @@ -0,0 +1 @@ +hidden=(password value not shown) diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index f6449d7beef6..5e9ab92d5708 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -24,23 +24,35 @@ package lib.form; import com.gargoylesoftware.htmlunit.Page; +import com.gargoylesoftware.htmlunit.html.DomElement; +import com.gargoylesoftware.htmlunit.html.HtmlHiddenInput; import com.gargoylesoftware.htmlunit.html.HtmlInput; import com.gargoylesoftware.htmlunit.html.HtmlPage; import com.gargoylesoftware.htmlunit.html.HtmlTextInput; +import hudson.FilePath; +import hudson.Launcher; import hudson.cli.CopyJobCommand; import hudson.cli.GetJobCommand; import hudson.model.*; +import hudson.tasks.BuildStep; +import hudson.tasks.BuildStepDescriptor; +import hudson.tasks.Builder; import hudson.util.FormValidation; import hudson.util.Secret; import java.io.ByteArrayOutputStream; +import java.io.IOException; import java.io.PrintStream; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Locale; import java.util.regex.Pattern; +import jenkins.model.GlobalConfiguration; import jenkins.model.Jenkins; +import jenkins.model.TransientActionFactory; import jenkins.security.apitoken.ApiTokenTestHelper; +import jenkins.tasks.SimpleBuildStep; import org.acegisecurity.Authentication; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; @@ -54,10 +66,14 @@ import org.jvnet.hudson.test.MockAuthorizationStrategy; import org.jvnet.hudson.test.TestExtension; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; import org.kohsuke.stapler.Stapler; import org.kohsuke.stapler.StaplerRequest; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; + public class PasswordTest { @Rule @@ -223,4 +239,441 @@ public FormValidation doCheckSecret(@QueryParameter String value) { } } + @Test + public void testBackgroundSecretConversion() throws Exception { + final JenkinsRule.WebClient wc = j.createWebClient(); + j.configRoundtrip(); + // empty default values + assertEquals("", PasswordHolderConfiguration.getInstance().secretWithSecretGetterAndSetter.getPlainText()); + assertEquals("", PasswordHolderConfiguration.getInstance().secretWithStringGetterAndSetter.getPlainText()); + assertEquals("", PasswordHolderConfiguration.getInstance().stringWithSecretGetterAndSetter); + assertEquals("", PasswordHolderConfiguration.getInstance().stringWithStringGetterAndSetter); + + // set some values and expect them to remain after round-trip + final Secret secretWithSecretGetterAndSetter = Secret.fromString("secretWithSecretGetterAndSetter"); + secretWithSecretGetterAndSetter.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + PasswordHolderConfiguration.getInstance().secretWithSecretGetterAndSetter = secretWithSecretGetterAndSetter; + + final Secret secretWithStringGetterAndSetter = Secret.fromString("secretWithStringGetterAndSetter"); + secretWithStringGetterAndSetter.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + PasswordHolderConfiguration.getInstance().secretWithStringGetterAndSetter = secretWithStringGetterAndSetter; + + PasswordHolderConfiguration.getInstance().stringWithSecretGetterAndSetter = "stringWithSecretGetterAndSetter"; + PasswordHolderConfiguration.getInstance().stringWithStringGetterAndSetter = "stringWithStringGetterAndSetter"; + + + final HtmlPage configPage = wc.goTo("configure"); + for (DomElement element : configPage.getElementsByTagName("input")) { + if ("hidden".equals(element.getAttribute("type")) && element.getAttribute("class").contains("complex-password-field")) { + final HtmlHiddenInput input = (HtmlHiddenInput) element; + // assert that all password fields contain encrypted values after we set plain values + assertTrue(input.getValueAttribute().startsWith("{")); + assertTrue(input.getValueAttribute().endsWith("}")); + } + } + + j.configRoundtrip(); + + // confirm round-trip did not change effective values + assertEquals("secretWithSecretGetterAndSetter", PasswordHolderConfiguration.getInstance().secretWithSecretGetterAndSetter.getPlainText()); + assertEquals("secretWithStringGetterAndSetter", PasswordHolderConfiguration.getInstance().secretWithStringGetterAndSetter.getPlainText()); + assertEquals("stringWithSecretGetterAndSetter", PasswordHolderConfiguration.getInstance().stringWithSecretGetterAndSetter); + assertEquals("stringWithStringGetterAndSetter", PasswordHolderConfiguration.getInstance().stringWithStringGetterAndSetter); + + assertEquals(secretWithSecretGetterAndSetter.getEncryptedValue(), PasswordHolderConfiguration.getInstance().secretWithSecretGetterAndSetter.getEncryptedValue()); + + // The following is because the serialized "Secret" value in the form gets decrypted, losing IV, to be passed as String into the setter, to be converted to Secret, getting new IV in #getEncryptedValue call. + assertNotEquals(secretWithStringGetterAndSetter.getEncryptedValue(), PasswordHolderConfiguration.getInstance().secretWithStringGetterAndSetter.getEncryptedValue()); + } + + @TestExtension + public static class PasswordHolderConfiguration extends GlobalConfiguration { + private Secret secretWithStringGetterAndSetter; // the badly implemented secret migration + private Secret secretWithSecretGetterAndSetter; // the old, good case + private String stringWithStringGetterAndSetter; // the trivially wrong case + private String stringWithSecretGetterAndSetter; + + public String getSecretWithStringGetterAndSetter() { + return secretWithStringGetterAndSetter == null ? null : secretWithStringGetterAndSetter.getPlainText(); + } + + public void setSecretWithStringGetterAndSetter(String secretWithStringGetterAndSetter) { + this.secretWithStringGetterAndSetter = Secret.fromString(secretWithStringGetterAndSetter); + } + + public Secret getSecretWithSecretGetterAndSetter() { + return secretWithSecretGetterAndSetter; + } + + public void setSecretWithSecretGetterAndSetter(Secret secretWithSecretGetterAndSetter) { + this.secretWithSecretGetterAndSetter = secretWithSecretGetterAndSetter; + } + + public String getStringWithStringGetterAndSetter() { + return stringWithStringGetterAndSetter; + } + + public void setStringWithStringGetterAndSetter(String stringWithStringGetterAndSetter) { + this.stringWithStringGetterAndSetter = stringWithStringGetterAndSetter; + } + + public Secret getStringWithSecretGetterAndSetter() { + return Secret.fromString(stringWithSecretGetterAndSetter); + } + + public void setStringWithSecretGetterAndSetter(Secret stringWithSecretGetterAndSetter) { + this.stringWithSecretGetterAndSetter = stringWithSecretGetterAndSetter == null? null : stringWithSecretGetterAndSetter.getPlainText(); + } + + public static PasswordHolderConfiguration getInstance() { + return GlobalConfiguration.all().getInstance(PasswordHolderConfiguration.class); + } + } + + @Test + public void testBuildStep() throws Exception { + final FreeStyleProject project = j.createFreeStyleProject(); + project.getBuildersList().add(new PasswordHolderBuildStep()); + project.save(); + assertEquals(1, project.getBuilders().size()); + j.configRoundtrip(project); + + // empty default values after initial form submission + PasswordHolderBuildStep buildStep = (PasswordHolderBuildStep) project.getBuildersList().get(0); + assertNotNull(buildStep); + assertEquals("", buildStep.secretWithSecretGetterSecretSetter.getPlainText()); + assertEquals("", buildStep.secretWithSecretGetterStringSetter.getPlainText()); + assertEquals("", buildStep.secretWithStringGetterSecretSetter.getPlainText()); + assertEquals("", buildStep.secretWithStringGetterStringSetter.getPlainText()); + assertEquals("", buildStep.stringWithSecretGetterSecretSetter); + assertEquals("", buildStep.stringWithSecretGetterStringSetter); + assertEquals("", buildStep.stringWithStringGetterSecretSetter); + assertEquals("", buildStep.stringWithStringGetterStringSetter); + + buildStep = (PasswordHolderBuildStep) project.getBuildersList().get(0); + assertNotNull(buildStep); + + + // set some values and expect them to remain after round-trip + final Secret secretWithSecretGetterSecretSetter = Secret.fromString("secretWithSecretGetterSecretSetter"); + secretWithSecretGetterSecretSetter.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + buildStep.secretWithSecretGetterSecretSetter = secretWithSecretGetterSecretSetter; + + final Secret secretWithStringGetterStringSetter = Secret.fromString("secretWithStringGetterStringSetter"); + secretWithStringGetterStringSetter.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + buildStep.secretWithStringGetterStringSetter = secretWithStringGetterStringSetter; + + final Secret secretWithStringGetterSecretSetter = Secret.fromString("secretWithStringGetterSecretSetter"); + secretWithStringGetterSecretSetter.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + buildStep.secretWithStringGetterSecretSetter = secretWithStringGetterSecretSetter; + + final Secret secretWithSecretGetterStringSetter = Secret.fromString("secretWithSecretGetterStringSetter"); + secretWithSecretGetterStringSetter.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + buildStep.secretWithSecretGetterStringSetter = secretWithSecretGetterStringSetter; + + buildStep.stringWithSecretGetterSecretSetter = "stringWithSecretGetterSecretSetter"; + buildStep.stringWithStringGetterStringSetter = "stringWithStringGetterStringSetter"; + buildStep.stringWithStringGetterSecretSetter = "stringWithStringGetterSecretSetter"; + buildStep.stringWithSecretGetterStringSetter = "stringWithSecretGetterStringSetter"; + + project.save(); + + final HtmlPage configPage = j.createWebClient().goTo(project.getUrl() + "/configure"); + int i = 0; + for (DomElement element : configPage.getElementsByTagName("input")) { + if ("hidden".equals(element.getAttribute("type")) && element.getAttribute("class").contains("complex-password-field")) { + final HtmlHiddenInput input = (HtmlHiddenInput) element; + // assert that all password fields contain encrypted values after we set plain values + assertTrue(input.getValueAttribute().startsWith("{")); + assertTrue(input.getValueAttribute().endsWith("}")); + i++; + } + } + assertTrue(i >= 8); // at least 8 password fields expected on that job config form + + j.configRoundtrip(project); + buildStep = (PasswordHolderBuildStep) project.getBuildersList().get(0); + + // confirm round-trip did not change effective values + assertEquals("secretWithSecretGetterSecretSetter", buildStep.secretWithSecretGetterSecretSetter.getPlainText()); + assertEquals("secretWithStringGetterStringSetter", buildStep.secretWithStringGetterStringSetter.getPlainText()); + assertEquals("secretWithStringGetterSecretSetter", buildStep.secretWithStringGetterSecretSetter.getPlainText()); + assertEquals("secretWithSecretGetterStringSetter", buildStep.secretWithSecretGetterStringSetter.getPlainText()); + + assertEquals("stringWithSecretGetterSecretSetter", buildStep.stringWithSecretGetterSecretSetter); + assertEquals("stringWithStringGetterStringSetter", buildStep.stringWithStringGetterStringSetter); + assertEquals("stringWithStringGetterSecretSetter", buildStep.stringWithStringGetterSecretSetter); + assertEquals("stringWithSecretGetterStringSetter", buildStep.stringWithSecretGetterStringSetter); + + // confirm the Secret getter/setter will not change encrypted value (keeps IV) + assertEquals(secretWithSecretGetterSecretSetter.getEncryptedValue(), buildStep.secretWithSecretGetterSecretSetter.getEncryptedValue()); + + // This depends on implementation; if the Getter returns the plain text (to be re-encrypted by Functions#getPasswordValue), then this won't work, but if they getter returns #getEncrytedValue (as implemented in the build step here), it does + // While clever, would recommend fixing mismatched getters/setters here + assertEquals(secretWithStringGetterSecretSetter.getEncryptedValue(), buildStep.secretWithStringGetterSecretSetter.getEncryptedValue()); + + // This isn't equal because we expect that the code cannot handle an encrypted secret value passed to the setter, so we decrypt it + assertNotEquals(secretWithStringGetterStringSetter.getEncryptedValue(), buildStep.secretWithStringGetterStringSetter.getEncryptedValue()); + } + + public static class PasswordHolderBuildStep extends Builder implements SimpleBuildStep { + + // There are actually more cases than this, but between this and testStringlyTypedSecrets we should be covering everything sufficiently: + // Storage permutations: + // - Secret + // - plain string + // - encrypted string + // Getter permutations: + // - Secret + // - plain string + // - encrypted string + // Setter permutations: + // - Secret + // - String that can handle encrypted values + // - String that cannot handle encrypted values + // + // These last two aren't really all that different, since we decrypt encrypted Strings now. + private Secret secretWithStringGetterStringSetter; // the badly implemented secret migration + private Secret secretWithSecretGetterStringSetter; + private Secret secretWithStringGetterSecretSetter; + private Secret secretWithSecretGetterSecretSetter; // the old, good case + private String stringWithStringGetterStringSetter; // the trivially wrong case + private String stringWithSecretGetterStringSetter; + private String stringWithStringGetterSecretSetter; + private String stringWithSecretGetterSecretSetter; + + @DataBoundConstructor + public PasswordHolderBuildStep() { + // data binding + } + + public String getSecretWithStringGetterStringSetter() { + return secretWithStringGetterStringSetter == null ? null : secretWithStringGetterStringSetter.getEncryptedValue(); // model half-assed migration from String to Secret + } + + @DataBoundSetter + public void setSecretWithStringGetterStringSetter(String secretWithStringGetterStringSetter) { + this.secretWithStringGetterStringSetter = Secret.fromString(secretWithStringGetterStringSetter); + } + + public Secret getSecretWithSecretGetterStringSetter() { + return secretWithSecretGetterStringSetter; + } + + @DataBoundSetter + public void setSecretWithSecretGetterStringSetter(String secretWithSecretGetterStringSetter) { + this.secretWithSecretGetterStringSetter = Secret.fromString(secretWithSecretGetterStringSetter); + } + + public String getSecretWithStringGetterSecretSetter() { + return secretWithStringGetterSecretSetter == null ? null : secretWithStringGetterSecretSetter.getEncryptedValue(); // model half-assed migration from String to Secret + } + + @DataBoundSetter + public void setSecretWithStringGetterSecretSetter(Secret secretWithStringGetterSecretSetter) { + this.secretWithStringGetterSecretSetter = secretWithStringGetterSecretSetter; + } + + public Secret getSecretWithSecretGetterSecretSetter() { + return secretWithSecretGetterSecretSetter; + } + + @DataBoundSetter + public void setSecretWithSecretGetterSecretSetter(Secret secretWithSecretGetterSecretSetter) { + this.secretWithSecretGetterSecretSetter = secretWithSecretGetterSecretSetter; + } + + public String getStringWithStringGetterStringSetter() { + return stringWithStringGetterStringSetter; + } + + @DataBoundSetter + public void setStringWithStringGetterStringSetter(String stringWithStringGetterStringSetter) { + this.stringWithStringGetterStringSetter = stringWithStringGetterStringSetter; + } + + public Secret getStringWithSecretGetterStringSetter() { + return Secret.fromString(stringWithSecretGetterStringSetter); + } + + @DataBoundSetter + public void setStringWithSecretGetterStringSetter(String stringWithSecretGetterStringSetter) { + this.stringWithSecretGetterStringSetter = stringWithSecretGetterStringSetter; + } + + public String getStringWithStringGetterSecretSetter() { + return stringWithStringGetterSecretSetter; + } + + @DataBoundSetter + public void setStringWithStringGetterSecretSetter(Secret stringWithStringGetterSecretSetter) { + this.stringWithStringGetterSecretSetter = stringWithStringGetterSecretSetter == null? null : stringWithStringGetterSecretSetter.getPlainText(); + } + + public Secret getStringWithSecretGetterSecretSetter() { + return Secret.fromString(stringWithSecretGetterSecretSetter); + } + + @DataBoundSetter + public void setStringWithSecretGetterSecretSetter(Secret stringWithSecretGetterSecretSetter) { + this.stringWithSecretGetterSecretSetter = stringWithSecretGetterSecretSetter == null? null : stringWithSecretGetterSecretSetter.getPlainText(); + } + + @Override + public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { + // do nothing + } + + @TestExtension + public static class DescriptorImpl extends BuildStepDescriptor { + + @Override + public boolean isApplicable(Class jobType) { + return jobType == FreeStyleProject.class; + } + } + } + + @Test + public void testStringlyTypedSecrets() throws Exception { + final FreeStyleProject project = j.createFreeStyleProject(); + project.getBuildersList().add(new StringlyTypedSecretsBuilder("")); + project.save(); + assertEquals(1, project.getBuilders().size()); + j.configRoundtrip(project); + + // empty default values after initial form submission + StringlyTypedSecretsBuilder buildStep = (StringlyTypedSecretsBuilder) project.getBuildersList().get(0); + assertNotNull(buildStep); + assertTrue(buildStep.mySecret.startsWith("{")); + assertTrue(buildStep.mySecret.endsWith("}")); + assertTrue(Secret.fromString(buildStep.mySecret).getPlainText().isEmpty()); + + // set a value and expect it to remain after round-trip + final Secret stringlyTypedSecret = Secret.fromString("stringlyTypedSecret"); + stringlyTypedSecret.getEncryptedValue(); // ensure IV is set so the encrypted value is stable + buildStep.mySecret = stringlyTypedSecret.getEncryptedValue(); + + project.save(); + + final HtmlPage configPage = j.createWebClient().goTo(project.getUrl() + "/configure"); + for (DomElement element : configPage.getElementsByTagName("input")) { + if ("hidden".equals(element.getAttribute("type")) && element.getAttribute("class").contains("complex-password-field")) { + final HtmlHiddenInput input = (HtmlHiddenInput) element; + // assert that all password fields contain encrypted values after we set plain values + assertTrue(input.getValueAttribute().startsWith("{")); + assertTrue(input.getValueAttribute().endsWith("}")); + } + } + + j.configRoundtrip(project); + buildStep = (StringlyTypedSecretsBuilder) project.getBuildersList().get(0); + + // confirm round-trip did not change effective values + assertEquals("stringlyTypedSecret", Secret.fromString(buildStep.mySecret).getPlainText()); + + // Unfortunately the constructor parameter will be decrypted transparently now, so this is sort of a minor regression with this enhancement. + // Note that it's not enough to just undo the related changes to core/src/main to try this; as Functions#getPasswordValue will throw a SecurityException during tests only and break the previous assertion. + assertNotEquals(stringlyTypedSecret.getEncryptedValue(), buildStep.mySecret); + } + + public static class StringlyTypedSecretsBuilder extends Builder implements SimpleBuildStep { + + private String mySecret; + + @DataBoundConstructor + public StringlyTypedSecretsBuilder(String mySecret) { + this.mySecret = Secret.fromString(mySecret).getEncryptedValue(); + } + + public String getMySecret() { + return mySecret; + } + + @Override + public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { + // do nothing + } + + @TestExtension + public static class DescriptorImpl extends BuildStepDescriptor { + + @Override + public boolean isApplicable(Class jobType) { + return jobType == FreeStyleProject.class; + } + } + } + + @Test + public void testBlankoutOfStringBackedPasswordFieldWithoutItemConfigure() throws Exception { + FreeStyleProject p = j.createFreeStyleProject(); + JenkinsRule.WebClient wc = j.createWebClient(); + HtmlPage htmlPage = wc.goTo(p.getUrl() + "/passwordFields"); + for (DomElement element : htmlPage.getElementsByTagName("input")) { + if ("hidden".equals(element.getAttribute("type")) && element.getAttribute("class").contains("complex-password-field")) { + final HtmlHiddenInput input = (HtmlHiddenInput) element; + // assert that all password fields contain encrypted values after we set plain values + assertTrue(input.getValueAttribute().startsWith("{")); + assertTrue(input.getValueAttribute().endsWith("}")); + } + } + + final MockAuthorizationStrategy a = new MockAuthorizationStrategy(); + a.grant(Jenkins.READ, Job.READ, Job.EXTENDED_READ).everywhere().toEveryone(); + j.jenkins.setAuthorizationStrategy(a); + + /* Now go to the page without Item/Configure and expect asterisks */ + htmlPage = wc.goTo(p.getUrl() + "/passwordFields"); + for (DomElement element : htmlPage.getElementsByTagName("input")) { + if ("hidden".equals(element.getAttribute("type")) && element.getAttribute("class").contains("complex-password-field")) { + final HtmlHiddenInput input = (HtmlHiddenInput) element; + assertTrue(input.getValueAttribute().equals("********")); + } + } + } + + @TestExtension + public static class FactoryImpl extends TransientActionFactory { + + @Override + public Class type() { + return Job.class; + } + + @Nonnull + @Override + public Collection createFor(@Nonnull Job target) { + return Collections.singletonList(new ActionImpl()); + } + } + + public static class ActionImpl implements Action { + + @CheckForNull + @Override + public String getIconFileName() { + return null; + } + + @CheckForNull + @Override + public String getDisplayName() { + return null; + } + + @CheckForNull + @Override + public String getUrlName() { + return "passwordFields"; + } + + public Secret getSecretPassword() { + return Secret.fromString("secretPassword"); + } + + public String getStringPassword() { + return "stringPassword"; + } + } } diff --git a/test/src/test/resources/lib/form/PasswordTest/ActionImpl/index.jelly b/test/src/test/resources/lib/form/PasswordTest/ActionImpl/index.jelly new file mode 100644 index 000000000000..276caaf17a90 --- /dev/null +++ b/test/src/test/resources/lib/form/PasswordTest/ActionImpl/index.jelly @@ -0,0 +1,11 @@ + + + + + + + + + + + diff --git a/test/src/test/resources/lib/form/PasswordTest/PasswordHolderBuildStep/config.jelly b/test/src/test/resources/lib/form/PasswordTest/PasswordHolderBuildStep/config.jelly new file mode 100644 index 000000000000..2b4743ee5221 --- /dev/null +++ b/test/src/test/resources/lib/form/PasswordTest/PasswordHolderBuildStep/config.jelly @@ -0,0 +1,27 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/src/test/resources/lib/form/PasswordTest/PasswordHolderConfiguration/config.jelly b/test/src/test/resources/lib/form/PasswordTest/PasswordHolderConfiguration/config.jelly new file mode 100644 index 000000000000..cce768c9c486 --- /dev/null +++ b/test/src/test/resources/lib/form/PasswordTest/PasswordHolderConfiguration/config.jelly @@ -0,0 +1,17 @@ + + + + + + + + + + + + + + + + + diff --git a/test/src/test/resources/lib/form/PasswordTest/StringlyTypedSecretsBuilder/config.jelly b/test/src/test/resources/lib/form/PasswordTest/StringlyTypedSecretsBuilder/config.jelly new file mode 100644 index 000000000000..ece5354043d8 --- /dev/null +++ b/test/src/test/resources/lib/form/PasswordTest/StringlyTypedSecretsBuilder/config.jelly @@ -0,0 +1,6 @@ + + + + + +