WIP - Security Hardening: Guess sensitive attributes by name in addition to API checks

plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java

@@ -50,6 +50,15 @@ public class Attribute<Owner, Type> {
         // Nop
     };
 
+    // TODO: Make the list configurable via system property
+    // TODO: Move secret-related logic to SecretsHelper
+    private static final Set<String> SUSPECTED_SECRET_ATTRIBUTE_NAMES_AND_SUFFIXES =
+            Collections.unmodifiableSet(new HashSet<>(
+                    Arrays.asList("password", "passphrase", "passwd", "pwd",
+                            "secret", "secretkey", "privatekey",
+                            "token", "accesstoken", "apitoken")));
+
+
     //TODO: Concurrent cache?
     //private static final HashMap<Class, Boolean> SECRET_ATTRIBUTE_CACHE =
     //        new HashMap<>();
@@ -262,8 +271,15 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi
     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);
+        if (node instanceof Scalar) {
+            Scalar scalar = (Scalar)node;
+            if (shouldBeMasked) {
+                scalar.sensitive(true);
+                scalar.encrypted(this.getter.encrypted());
+            }
+            if (type == Secret.class) {
+                scalar.encrypted(true);
+            }
         }
         return node;
     }
@@ -295,6 +311,14 @@ public interface Setter<O,T> {
     @FunctionalInterface
     public interface Getter<O,T> {
         T getValue(O target) throws Exception;
+
+        /**
+         * Checks whether getter returns encrypted value.
+         * @return {@code true} if {@link #getValue(Object)} returns encrypted value instead of the original value.
+         *         {@code false} will be returned for {@link Secret} getters since they do not encrypt the value on their own
+         * @since TODO
+         */
+        default boolean encrypted() {return false;}
     }
 
     @CheckForNull
@@ -342,6 +366,16 @@ public static boolean calculateIfSecret(@CheckForNull Class<?> targetClass, @Non
             return true;
         }
 
+        // try attributes and suffixes
+        String lowerCaseFieldName = fieldName.toLowerCase();
+        for (String suffix : SUSPECTED_SECRET_ATTRIBUTE_NAMES_AND_SUFFIXES) {
+            if (lowerCaseFieldName.endsWith(suffix)) {
+                LOGGER.log(Level.FINER, "Attribute {0} is secret, because '{1}' is in the list of common Secret attribute names and suffixes",
+                        new Object[]{fieldName, suffix});
+                return true;
+            }
+        }
+
         if (targetClass == null) {
             LOGGER.log(Level.FINER, "Attribute {0} is assumed to be non-secret, because there is no class instance in the call. " +
                             "This call is used only for fast-fetch caching, and the result may be adjusted later",

plugin/src/main/java/io/jenkins/plugins/casc/core/ProxyConfigurator.java

@@ -58,7 +58,7 @@ protected ProxyConfiguration instance(Mapping mapping, ConfigurationContext cont
                         .getter(ProxyConfiguration::getUserName)
                         .setter(noop()),
                 new Attribute<ProxyConfiguration, String>("password", String.class)
-                        .getter(ProxyConfiguration::getEncryptedPassword)
+                        .getter(ProxyPasswordGetter.INSTANCE)
                         .setter(noop()),
                 new Attribute<ProxyConfiguration, String>("noProxyHost", String.class)
                         .getter(config -> config.noProxyHost)
@@ -128,4 +128,19 @@ public void setTestUrl(String testUrl) {
             this.testUrl = testUrl;
         }
     }
+
+    static class ProxyPasswordGetter implements Attribute.Getter<ProxyConfiguration, String> {
+
+        private static final ProxyPasswordGetter INSTANCE = new ProxyPasswordGetter();
+
+        @Override
+        public String getValue(ProxyConfiguration target) throws Exception {
+            return target.getEncryptedPassword();
+        }
+
+        @Override
+        public boolean encrypted() {
+            return true;
+        }
+    }
 }

plugin/src/main/java/io/jenkins/plugins/casc/model/Scalar.java

@@ -94,23 +94,25 @@ public boolean isMasked() {
     /**
      * Sets the sensitive flag.
      * It indicates that the scalar represents a sensitive argument (secret or other restricted data).
+     * Sensitive flag cannot be reset to {@code false}.
      * @param sensitive value to set
      * @return Object instance
      * @since 1.25
      */
     public Scalar sensitive(boolean sensitive) {
-        this.sensitive = sensitive;
+        this.sensitive = this.sensitive || sensitive;
         return this;
     }
 
     /**
      * Indicates that the data is encrypted and hence safe to be exported.
+     * Encrypted flag cannot be reset to {@code false}.
      * @param encrypted Value to set
      * @return Object instance
      * @since 1.25
      */
     public Scalar encrypted(boolean encrypted) {
-        this.encrypted = encrypted;
+        this.encrypted = this.encrypted || encrypted;
         return this;
     }
 

plugin/src/test/java/io/jenkins/plugins/casc/AttributeTest.java

@@ -41,6 +41,7 @@ public void checkCommonSecretPatterns() {
         assertFieldIsSecret(SecretFromPublicField.class, "secretField");
         assertFieldIsSecret(SecretFromPrivateField.class, "secretField");
 
+        assertFieldIsSecret(SecretFromImpliedAttributeName.class, "password");
         assertFieldIsSecret(SecretRenamedFieldFithSecretConstructor.class, "mySecretValueField");
     }
 
@@ -61,6 +62,8 @@ public void checkCommonSecretPatternsForOverrides() {
         assertFieldIsSecret(SecretFromPublicField2.class, "secretField");
         assertFieldIsSecret(SecretFromPrivateField2.class, "secretField");
         assertFieldIsSecret(SecretFromPrivateField3.class, "secretField");
+
+        assertFieldIsSecret(SecretFromImpliedAttributeName2.class, "password");
     }
 
     @Test
@@ -69,6 +72,25 @@ public void checkNonSecretPatterns() {
         assertFieldIsNotSecret(NonSecretField.class, "passwordPath");
     }
 
+    @Test
+    public void shouldConsiderSuffixes() {
+        assertFieldIsSecret(null, "secretKey");
+        assertFieldIsSecret(null, "mySecretKey");
+        assertFieldIsSecret(null, "myPwd");
+        assertFieldIsSecret(null, "superSecretPassword");
+
+        // Examples of false positives
+        assertFieldIsSecret(null, "pathToSecretKey");
+        assertFieldIsSecret(null, "foretoken"); // https://en.wiktionary.org/wiki/foretoken
+    }
+
+    @Test
+    public void shouldNotConsiderSuffixesInTheMiddle() {
+        assertFieldIsNotSecret(null, "passwordPath");
+        assertFieldIsNotSecret(null, "passwordFile");
+        assertFieldIsNotSecret(null, "tokenSource");
+    }
+
     public static void assertFieldIsSecret(Class<?> clazz, String fieldName) {
         String displayName = clazz != null ? (clazz.getName() + "#" + fieldName) : fieldName;
         assertTrue("Field is not secret: " + displayName,
@@ -159,6 +181,24 @@ public SecretFromPrivateField3(String secret) {
         }
     }
 
+    public static class SecretFromImpliedAttributeName {
+
+        public String password;
+
+        @DataBoundConstructor
+        public SecretFromImpliedAttributeName(String password) {
+            this.password = password;
+        }
+    }
+
+    public static class SecretFromImpliedAttributeName2 extends SecretFromImpliedAttributeName {
+
+        @DataBoundConstructor
+        public SecretFromImpliedAttributeName2(String password) {
+            super(password);
+        }
+    }
+
     public static class NonSecretField {
 
         public String passwordPath;