From fc942ee10d86649db9e9b8ce3dc0a04ea23439ce Mon Sep 17 00:00:00 2001 From: Arun Mahadevan Date: Tue, 7 Aug 2018 12:13:54 -0700 Subject: [PATCH] STORM-3184: Mask the plaintext passwords from the logs Introduce a `Password` config annotation and use it to mark configs that are sensitive and mask the values while logging. --- .../storm/common/AbstractAutoCreds.java | 4 ++- .../clj/org/apache/storm/daemon/nimbus.clj | 6 ++-- .../clj/org/apache/storm/daemon/worker.clj | 6 ++-- .../src/jvm/org/apache/storm/Config.java | 9 ++++++ .../storm/daemon/supervisor/Supervisor.java | 2 +- .../org/apache/storm/utils/ConfigUtils.java | 31 +++++++++++++++++++ .../ConfigValidationAnnotations.java | 7 +++++ .../apache/storm/utils/ConfigUtilsTest.java | 12 +++++++ 8 files changed, 69 insertions(+), 8 deletions(-) diff --git a/external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java b/external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java index 7b2fc2db0b7..2ce99aa3fdd 100644 --- a/external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java +++ b/external/storm-autocreds/src/main/java/org/apache/storm/common/AbstractAutoCreds.java @@ -28,6 +28,7 @@ import org.apache.storm.security.INimbusCredentialPlugin; import org.apache.storm.security.auth.IAutoCredentials; import org.apache.storm.security.auth.ICredentialsRenewer; +import org.apache.storm.utils.ConfigUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -149,7 +150,8 @@ protected Set> getCredentials(Map cred protected void fillHadoopConfiguration(Map topoConf, String configKey, Configuration configuration) { Map config = (Map) topoConf.get(configKey); - LOG.info("TopoConf {}, got config {}, for configKey {}", topoConf, config, configKey); + LOG.info("TopoConf {}, got config {}, for configKey {}", ConfigUtils.maskPasswords(topoConf), + ConfigUtils.maskPasswords(config), configKey); if (config != null) { List resourcesToLoad = new ArrayList<>(); for (Map.Entry entry : config.entrySet()) { diff --git a/storm-core/src/clj/org/apache/storm/daemon/nimbus.clj b/storm-core/src/clj/org/apache/storm/daemon/nimbus.clj index 850867ecd5b..fc89ac4b818 100644 --- a/storm-core/src/clj/org/apache/storm/daemon/nimbus.clj +++ b/storm-core/src/clj/org/apache/storm/daemon/nimbus.clj @@ -61,7 +61,7 @@ (:use [org.apache.storm.daemon common]) (:use [org.apache.storm config]) (:import [org.apache.zookeeper data.ACL ZooDefs$Ids ZooDefs$Perms]) - (:import [org.apache.storm.utils VersionInfo Time] + (:import [org.apache.storm.utils VersionInfo Time ConfigUtils] (org.apache.storm.metric ClusterMetricsConsumerExecutor) (org.apache.storm.metric.api IClusterMetricsConsumer$ClusterInfo DataPoint IClusterMetricsConsumer$SupervisorInfo) (org.apache.storm Config) @@ -1748,7 +1748,7 @@ " (storm-" (.get_storm_version topology) " JDK-" (.get_jdk_version topology) ") with conf " - (redact-value storm-conf STORM-ZOOKEEPER-TOPOLOGY-AUTH-PAYLOAD)) + (redact-value (ConfigUtils/maskPasswords storm-conf) STORM-ZOOKEEPER-TOPOLOGY-AUTH-PAYLOAD)) ;; lock protects against multiple topologies being submitted at once and ;; cleanup thread killing topology in b/w assignment and starting the topology (locking (:submit-lock nimbus) @@ -2463,7 +2463,7 @@ (defserverfn service-handler [conf inimbus] (.prepare inimbus conf (master-inimbus-dir conf)) - (log-message "Starting Nimbus with conf " conf) + (log-message "Starting Nimbus with conf " (ConfigUtils/maskPasswords conf)) (let [nimbus (nimbus-data conf inimbus) blob-store (:blob-store nimbus)] (.prepare ^org.apache.storm.nimbus.ITopologyValidator (:validator nimbus) conf) diff --git a/storm-core/src/clj/org/apache/storm/daemon/worker.clj b/storm-core/src/clj/org/apache/storm/daemon/worker.clj index 859a7352fa7..13daa10a4cf 100644 --- a/storm-core/src/clj/org/apache/storm/daemon/worker.clj +++ b/storm-core/src/clj/org/apache/storm/daemon/worker.clj @@ -25,7 +25,7 @@ (:import [java.util.concurrent Executors] [org.apache.storm.hooks IWorkerHook BaseWorkerHook]) (:import [java.util ArrayList HashMap]) - (:import [org.apache.storm.utils Utils TransferDrainer ThriftTopologyUtils WorkerBackpressureThread DisruptorQueue]) + (:import [org.apache.storm.utils Utils TransferDrainer ThriftTopologyUtils WorkerBackpressureThread DisruptorQueue ConfigUtils]) (:import [org.apache.storm.grouping LoadMapping]) (:import [org.apache.storm.messaging TransportFactory]) (:import [org.apache.storm.messaging TaskMessage IContext IConnection ConnectionWithStatus ConnectionWithStatus$Status]) @@ -604,7 +604,7 @@ ;; should guarantee this consistency (defserverfn mk-worker [conf shared-mq-context storm-id assignment-id port worker-id] (log-message "Launching worker for " storm-id " on " assignment-id ":" port " with id " worker-id - " and conf " conf) + " and conf " (ConfigUtils/maskPasswords conf)) ;; create an empty list to store deserialized hooks (def deserialized-hooks (java.util.ArrayList.)) (if-not (local-mode? conf) @@ -778,7 +778,7 @@ (schedule-recurring (:reset-log-levels-timer worker) 0 (conf WORKER-LOG-LEVEL-RESET-POLL-SECS) (fn [] (reset-log-levels latest-log-config))) (schedule-recurring (:refresh-active-timer worker) 0 (conf TASK-REFRESH-POLL-SECS) (partial refresh-storm-active worker)) - (log-message "Worker has topology config " (redact-value (:storm-conf worker) STORM-ZOOKEEPER-TOPOLOGY-AUTH-PAYLOAD)) + (log-message "Worker has topology config " (redact-value (ConfigUtils/maskPasswords (:storm-conf worker)) STORM-ZOOKEEPER-TOPOLOGY-AUTH-PAYLOAD)) (log-message "Worker " worker-id " for storm " storm-id " on " assignment-id ":" port " has finished loading") ret )))))) diff --git a/storm-core/src/jvm/org/apache/storm/Config.java b/storm-core/src/jvm/org/apache/storm/Config.java index 628c9ff3ef2..fc9fb556b0b 100644 --- a/storm-core/src/jvm/org/apache/storm/Config.java +++ b/storm-core/src/jvm/org/apache/storm/Config.java @@ -812,6 +812,7 @@ public class Config extends HashMap { * Password for the keystore for HTTPS for Storm Logviewer */ @isString + @Password public static final String LOGVIEWER_HTTPS_KEYSTORE_PASSWORD = "logviewer.https.keystore.password"; /** @@ -825,6 +826,7 @@ public class Config extends HashMap { * Password to the private key in the keystore for setting up HTTPS (SSL). */ @isString + @Password public static final String LOGVIEWER_HTTPS_KEY_PASSWORD = "logviewer.https.key.password"; /** @@ -837,6 +839,7 @@ public class Config extends HashMap { * Password for the truststore for HTTPS for Storm Logviewer */ @isString + @Password public static final String LOGVIEWER_HTTPS_TRUSTSTORE_PASSWORD = "logviewer.https.truststore.password"; /** @@ -915,6 +918,7 @@ public class Config extends HashMap { * Password to the keystore used by Storm UI for setting up HTTPS (SSL). */ @isString + @Password public static final String UI_HTTPS_KEYSTORE_PASSWORD = "ui.https.keystore.password"; /** @@ -928,6 +932,7 @@ public class Config extends HashMap { * Password to the private key in the keystore for setting up HTTPS (SSL). */ @isString + @Password public static final String UI_HTTPS_KEY_PASSWORD = "ui.https.key.password"; /** @@ -940,6 +945,7 @@ public class Config extends HashMap { * Password to the truststore used by Storm UI setting up HTTPS (SSL). */ @isString + @Password public static final String UI_HTTPS_TRUSTSTORE_PASSWORD = "ui.https.truststore.password"; /** @@ -1041,6 +1047,7 @@ public class Config extends HashMap { * Password to the keystore used by Storm DRPC for setting up HTTPS (SSL). */ @isString + @Password public static final String DRPC_HTTPS_KEYSTORE_PASSWORD = "drpc.https.keystore.password"; /** @@ -1054,6 +1061,7 @@ public class Config extends HashMap { * Password to the private key in the keystore for setting up HTTPS (SSL). */ @isString + @Password public static final String DRPC_HTTPS_KEY_PASSWORD = "drpc.https.key.password"; /** @@ -1066,6 +1074,7 @@ public class Config extends HashMap { * Password to the truststore used by Storm DRPC setting up HTTPS (SSL). */ @isString + @Password public static final String DRPC_HTTPS_TRUSTSTORE_PASSWORD = "drpc.https.truststore.password"; /** diff --git a/storm-core/src/jvm/org/apache/storm/daemon/supervisor/Supervisor.java b/storm-core/src/jvm/org/apache/storm/daemon/supervisor/Supervisor.java index c305a72038e..2b10da93e73 100644 --- a/storm-core/src/jvm/org/apache/storm/daemon/supervisor/Supervisor.java +++ b/storm-core/src/jvm/org/apache/storm/daemon/supervisor/Supervisor.java @@ -192,7 +192,7 @@ EventManager getEventManger() { * Launch the supervisor */ public void launch() throws Exception { - LOG.info("Starting Supervisor with conf {}", conf); + LOG.info("Starting Supervisor with conf {}", ConfigUtils.maskPasswords(conf)); String path = ConfigUtils.supervisorTmpDir(conf); FileUtils.cleanDirectory(new File(path)); diff --git a/storm-core/src/jvm/org/apache/storm/utils/ConfigUtils.java b/storm-core/src/jvm/org/apache/storm/utils/ConfigUtils.java index 76176b4d438..a7c32c1ec1f 100644 --- a/storm-core/src/jvm/org/apache/storm/utils/ConfigUtils.java +++ b/storm-core/src/jvm/org/apache/storm/utils/ConfigUtils.java @@ -18,16 +18,19 @@ package org.apache.storm.utils; +import com.google.common.collect.Maps; import org.apache.commons.io.FileUtils; import org.apache.storm.Config; import org.apache.storm.daemon.supervisor.AdvancedFSOps; import org.apache.storm.generated.StormTopology; import org.apache.storm.validation.ConfigValidation; +import org.apache.storm.validation.ConfigValidationAnnotations; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.File; import java.io.IOException; +import java.lang.annotation.Annotation; import java.lang.reflect.Field; import java.net.URLEncoder; import java.util.ArrayList; @@ -48,6 +51,34 @@ public class ConfigUtils { public final static String NIMBUS_DO_NOT_REASSIGN = "NIMBUS-DO-NOT-REASSIGN"; public static final String FILE_SEPARATOR = File.separator; + private static final Set passwordConfigKeys = new HashSet<>(); + + static { + for (Field field : Config.class.getFields()) { + for (Annotation annotation : field.getAnnotations()) { + boolean isPassword = annotation.annotationType().getName().equals( + ConfigValidationAnnotations.Password.class.getName()); + if (isPassword) { + try { + passwordConfigKeys.add((String) field.get(null)); + } catch (IllegalAccessException e) { + // ignore + } + } + } + } + } + + public static Map maskPasswords(final Map conf) { + Maps.EntryTransformer maskPasswords = + new Maps.EntryTransformer() { + public Object transformEntry(String key, Object value) { + return passwordConfigKeys.contains(key) ? "*****" : value; + } + }; + return Maps.transformEntries(conf, maskPasswords); + } + // A singleton instance allows us to mock delegated static methods in our // tests by subclassing. private static ConfigUtils _instance = new ConfigUtils(); diff --git a/storm-core/src/jvm/org/apache/storm/validation/ConfigValidationAnnotations.java b/storm-core/src/jvm/org/apache/storm/validation/ConfigValidationAnnotations.java index b770f34c8a6..8a54a94a897 100644 --- a/storm-core/src/jvm/org/apache/storm/validation/ConfigValidationAnnotations.java +++ b/storm-core/src/jvm/org/apache/storm/validation/ConfigValidationAnnotations.java @@ -214,5 +214,12 @@ public static class ValidatorParams { public @interface CustomValidator { Class validatorClass(); } + + @Retention(RetentionPolicy.RUNTIME) + @Target(ElementType.FIELD) + public @interface Password { + Class validatorClass() default ConfigValidation.NotNullValidator.class; + } + } diff --git a/storm-core/test/jvm/org/apache/storm/utils/ConfigUtilsTest.java b/storm-core/test/jvm/org/apache/storm/utils/ConfigUtilsTest.java index 6f5caf217fd..ccc17c9a50b 100644 --- a/storm-core/test/jvm/org/apache/storm/utils/ConfigUtilsTest.java +++ b/storm-core/test/jvm/org/apache/storm/utils/ConfigUtilsTest.java @@ -95,4 +95,16 @@ public void getValueAsList_nonStringList() { Map map = mockMap(key, values); Assert.assertEquals(expectedValue, ConfigUtils.getValueAsList(key, map)); } + + @Test + public void testMaskPasswords() { + Map conf = new HashMap<>(); + conf.put(Config.LOGVIEWER_HTTPS_KEY_PASSWORD, "pass1"); + conf.put(Config.TOPOLOGY_MESSAGE_TIMEOUT_SECS, 100); + Map result = ConfigUtils.maskPasswords(conf); + Assert.assertEquals("*****", result.get(Config.LOGVIEWER_HTTPS_KEY_PASSWORD)); + Assert.assertEquals(100, result.get(Config.TOPOLOGY_MESSAGE_TIMEOUT_SECS)); + } + + } \ No newline at end of file