CVE-2019-10363

@@ -219,6 +219,13 @@
       <scope>test</scope>
     </dependency>
 
+    <dependency>
+      <groupId>io.jenkins.plugins</groupId>
+      <artifactId>sge-cloud-plugin</artifactId>
+      <version>1.22</version>
+      <scope>test</scope>
+    </dependency>
+
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>
       <artifactId>ssh-credentials</artifactId>

@@ -92,7 +92,8 @@ public void testExportSSHCredentials() throws Exception {
 
         String passphrase = sshCredential.getScalarValue("passphrase");
         assertThat(passphrase, not("password"));
-        assertThat(requireNonNull(Secret.decrypt(passphrase)).getPlainText(), is("password"));
+        assertThat(requireNonNull(Secret.decrypt(passphrase), "Failed to decrypt the password from " + passphrase)
+                .getPlainText(), is("password"));
 
         String sshKeyExported = sshCredential.get("privateKeySource")
                 .asMapping()

@@ -0,0 +1,39 @@
+package io.jenkins.plugins.casc;
+
+import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule;
+import java.io.StringWriter;
+import org.apache.commons.io.output.WriterOutputStream;
+import org.jenkinsci.plugins.sge.BatchCloud;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class SGECloudTest {
+
+    @Rule
+    public JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule();
+
+    @Test
+    @Issue("SECURITY-1458")
+    public void shouldNotExportPassword() throws Exception {
+        ConfigurationAsCode casc = ConfigurationAsCode.get();
+        ConfiguratorRegistry registry = ConfiguratorRegistry.get();
+        ConfigurationContext context = new ConfigurationContext(registry);
+
+        final String passwordText = "Hello, world!";
+        BatchCloud cloud = new BatchCloud("testBatchCloud", "whatever",
+                "sge", 5, "sge.acmecorp.com", 8080,
+                "username", passwordText);
+        j.jenkins.clouds.add(cloud);
+
+        StringWriter writer = new StringWriter();
+        casc.export(new WriterOutputStream(writer));
+        String exported = writer.toString();
+        assertThat("Password should not have been exported",
+                exported, not(containsString(passwordText)));
+    }
+}

@@ -223,15 +223,18 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi
             if (o == null) {
                 return null;
             }
+
+            // In Export we sensitive only those values which do not get rendered as secrets
+            boolean shouldBeMasked = isSecret(instance);
             if (multiple) {
                 Sequence seq = new Sequence();
                 if (o.getClass().isArray()) o = Arrays.asList((Object[]) o);
                 for (Object value : (Iterable) o) {
-                    seq.add(c.describe(value, context));
+                    seq.add(_describe(c, context, value, shouldBeMasked));
                 }
                 return seq;
             }
-            return c.describe(o, context);
+            return _describe(c, context, o, shouldBeMasked);
         } catch (Exception | /* Jenkins.getDescriptorOrDie */AssertionError e) {
             // Don't fail the whole export, prefer logging this error
             LOGGER.log(Level.WARNING, "Failed to export", e);
@@ -240,6 +243,25 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi
         }
     }
 
+    /**
+     * Describes a node.
+     * @param c Configurator
+     * @param context Context to be passed
+     * @param value Value
+     * @param shouldBeMasked If {@code true}, the value should be masked in the output.
+     *                       It will be applied to {@link Scalar} nodes only.
+     * @throws Exception export error
+     * @return Node
+     */
+    private CNode _describe(Configurator c, ConfigurationContext context, Object value, boolean shouldBeMasked)
+            throws Exception {
+        CNode node = c.describe(value, context);
+        if (shouldBeMasked && node instanceof Scalar) {
+            ((Scalar)node).sensitive(true);
+        }
+        return node;
+    }
+
     public boolean equals(Owner o1, Owner o2) throws Exception {
         final Object v1 = getValue(o1);
         final Object v2 = getValue(o2);

@@ -63,7 +63,8 @@ public CNode describe(Object instance, ConfigurationContext context) {
             return new Scalar((Boolean) instance);
         }
         if (instance instanceof Secret) {
-            return new Scalar(((Secret) instance).getEncryptedValue());
+            // Secrets are sensitive, but they do not need masking since they are exported in the encrypted form
+            return new Scalar(((Secret) instance).getEncryptedValue()).encrypted(true);
         }
         if (target.isEnum()) {
             return new Scalar((Enum) instance);

@@ -9,10 +9,14 @@
 
 public final class Scalar implements CNode, CharSequence {
 
+    private static final String SECRET_VALUE_STRING = "****";
+
     private String value;
     private Format format;
     private boolean raw;
     private Source source;
+    private boolean sensitive;
+    private boolean encrypted;
 
     public enum Format { STRING, MULTILINESTRING, BOOLEAN, NUMBER, FLOATING }
 
@@ -67,8 +71,52 @@ public Scalar asScalar() {
         return this;
     }
 
+    /**
+     * Gets value of the scalar for export.
+     * @return Value of the scalar if not {@link #isMasked()},
+     *         {@link #SECRET_VALUE_STRING} otherwise.
+     *         Encrypted sensitive data will be returned as is.
+     *
+     */
     public String getValue() {
-        return value;
+        return isMasked() ? SECRET_VALUE_STRING : value;
+    }
+
+    /**
+     * Check whether the scalar value should be masked in the output.
+     * @return {@code true} if the value is masked
+     * @since TODO
+     */
+    public boolean isMasked() {
+        return sensitive && !encrypted;
+    }
+
+    /**
+     * Sets the sensitive flag.
+     * It indicates that the scalar represents a sensitive argument (secret or other restricted data).
+     * @param sensitive value to set
+     * @return Object instance
+     * @since TODO
+     */
+    public Scalar sensitive(boolean sensitive) {
+        this.sensitive = sensitive;
+        return this;
+    }
+
+    /**
+     * Indicates that the data is encrypted and hence safe to be exported.
+     * @param encrypted Value to set
+     * @return Object instance
+     * @since TODO
+     */
+    public Scalar encrypted(boolean encrypted) {
+        this.encrypted = encrypted;
+        return this;
+    }
+
+    @Override
+    public boolean isSensitiveData() {
+        return sensitive;
     }
 
     @NonNull
@@ -116,5 +164,7 @@ private Scalar(Scalar it) {
         this.format = it.format;
         this.raw = it.raw;
         this.source = it.source;
+        this.sensitive = it.sensitive;
+        this.encrypted = it.encrypted;
     }
 }

@@ -0,0 +1,117 @@
+package io.jenkins.plugins.casc.yaml;
+
+
+import hudson.util.Secret;
+import io.jenkins.plugins.casc.ConfigurationAsCode;
+import io.jenkins.plugins.casc.ConfigurationContext;
+import io.jenkins.plugins.casc.ConfiguratorRegistry;
+import io.jenkins.plugins.casc.impl.configurators.DataBoundConfigurator;
+import io.jenkins.plugins.casc.model.CNode;
+import io.jenkins.plugins.casc.snakeyaml.error.YAMLException;
+import io.jenkins.plugins.casc.snakeyaml.nodes.Node;
+import java.io.IOException;
+import java.io.StringWriter;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.kohsuke.stapler.DataBoundConstructor;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Contains tests for particular export cases.
+ */
+public class ExportTest {
+
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    @Test
+    public void shouldNotExportValuesWithSecretGetters() throws Exception {
+        DataBoundConfigurator<DataBoundSecret> c = new DataBoundConfigurator<>(DataBoundSecret.class);
+        String res = export(c, new DataBoundSecret("test"));
+        assertThat(res, not(containsString("test")));
+    }
+
+    @Test
+    @Issue("SECURITY-1458")
+    public void shouldNotExportValuesWithSecretFields() throws Exception {
+        DataBoundConfigurator<DataBoundSecretField> c = new DataBoundConfigurator<>(DataBoundSecretField.class);
+        String res = export(c, new DataBoundSecretField("test"));
+        assertThat(res, not(containsString("test")));
+    }
+
+    @Test
+    @Issue("SECURITY-1458")
+    public void shouldNotExportValuesWithSecretConstructors() throws Exception {
+        DataBoundConfigurator<DataBoundSecretConstructor> c = new DataBoundConfigurator<>(DataBoundSecretConstructor.class);
+        String res = export(c, new DataBoundSecretConstructor(Secret.fromString("test")));
+        // TODO: uncomment after https://github.com/jenkinsci-cert/configuration-as-code-plugin/pull/5
+        // assertThat(res, not(containsString("test")));
+    }
+
+    public <T> String export(DataBoundConfigurator<T> configurator, T object) throws Exception {
+        ConfigurationAsCode casc = ConfigurationAsCode.get();
+        ConfiguratorRegistry registry = ConfiguratorRegistry.get();
+        ConfigurationContext context = new ConfigurationContext(registry);
+
+        final CNode config = configurator.describe(object, context);
+        final Node valueNode = casc.toYaml(config);
+
+        try (StringWriter writer = new StringWriter()) {
+            ConfigurationAsCode.serializeYamlNode(valueNode, writer);
+            return writer.toString();
+        } catch (IOException e) {
+            throw new YAMLException(e);
+        }
+    }
+
+    public static class DataBoundSecret {
+
+        Secret mySecretValue;
+
+        @DataBoundConstructor
+        public DataBoundSecret(String mySecretValue) {
+            this.mySecretValue = Secret.fromString(mySecretValue);
+        }
+
+        public Secret getMySecretValue() {
+            return mySecretValue;
+        }
+    }
+
+    public static class DataBoundSecretField {
+
+        Secret mySecretValue;
+
+        @DataBoundConstructor
+        public DataBoundSecretField(String mySecretValue) {
+            this.mySecretValue = Secret.fromString(mySecretValue);
+        }
+
+        public String getMySecretValue() {
+            return mySecretValue.getPlainText();
+        }
+    }
+
+    /**
+     * Example of a safe persistency to the disk when JCasC cannot discover the field.
+     */
+    public static class DataBoundSecretConstructor {
+
+        Secret mySecretValueField;
+
+        @DataBoundConstructor
+        public DataBoundSecretConstructor(Secret mySecretValue) {
+            this.mySecretValueField = mySecretValue;
+        }
+
+        public String getMySecretValue() {
+            return mySecretValueField.getPlainText();
+        }
+    }
+
+}