This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit 638627f9ad976561e69521f1dd3bc5f09aa7771a Author: Alex Heneveld <[email protected]> AuthorDate: Wed Sep 15 09:49:24 2021 +0100 block security in more places, fix some logic, add tests; and more sanitizing of secrets --- .../BrooklynComponentTemplateResolver.java | 8 +++++ .../creation/BrooklynEntityDecorationResolver.java | 4 +++ .../spi/creation/CampTypePlanTransformer.java | 34 +----------------- .../brooklyn/camp/brooklyn/ConfigYamlTest.java | 42 ++++++++++++++++++++++ .../brooklyn/catalog/CatalogYamlEntityTest.java | 31 +++++++++++++++- .../catalog/internal/BasicBrooklynCatalog.java | 3 +- .../org/apache/brooklyn/core/config/Sanitizer.java | 5 +-- .../core/typereg/AbstractTypePlanTransformer.java | 37 ++++++++++++++++++- .../apache/brooklyn/core/config/SanitizerTest.java | 14 +++----- .../rest/resources/EffectorUtilsRestTest.java | 2 +- 10 files changed, 132 insertions(+), 48 deletions(-) diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java index 018e293..f8c9844 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java @@ -63,6 +63,7 @@ import org.apache.brooklyn.core.mgmt.EntityManagementUtils; import org.apache.brooklyn.core.mgmt.ManagementContextInjectable; import org.apache.brooklyn.core.mgmt.classloading.JavaBrooklynClassLoadingContext; import org.apache.brooklyn.core.resolve.entity.EntitySpecResolver; +import org.apache.brooklyn.core.typereg.AbstractTypePlanTransformer; import org.apache.brooklyn.core.typereg.RegisteredTypeLoadingContexts; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; @@ -283,6 +284,13 @@ public class BrooklynComponentTemplateResolver { new BrooklynEntityDecorationResolver.TagsResolver(yamlLoader).decorate(spec, attrs, encounteredRegisteredTypeIds); configureEntityConfig(spec, encounteredRegisteredTypeIds); + + // check security. we probably used the catalog resolver which will have delegated to the transformer; + // but if we used the java resolver or one of the others, then: + // - we won't have all the right source tags, but that's okay + // - depth will come from the containing transformer and so be ignored here (which is fine, none of them should have children) + // - secure fields won't be scanned - need to fix that; just check again here on the spec, and above on the spec decorations + AbstractTypePlanTransformer.checkSecuritySensitiveFields(spec); } @SuppressWarnings({ "unchecked", "rawtypes" }) diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynEntityDecorationResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynEntityDecorationResolver.java index eb89faa..438e9f4 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynEntityDecorationResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynEntityDecorationResolver.java @@ -45,6 +45,7 @@ import org.apache.brooklyn.core.entity.BrooklynConfigKeys; import org.apache.brooklyn.core.mgmt.BrooklynTags; import org.apache.brooklyn.core.objs.BasicSpecParameter; import org.apache.brooklyn.core.resolve.jackson.BeanWithTypeUtils; +import org.apache.brooklyn.core.typereg.AbstractTypePlanTransformer; import org.apache.brooklyn.core.typereg.RegisteredTypeLoadingContexts; import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.util.collections.MutableList; @@ -148,6 +149,9 @@ public abstract class BrooklynEntityDecorationResolver<DT> { spec = classFactory.apply((Class<DTInterface>)type).parameters(BasicSpecParameter.fromClass(mgmt, type)); } spec.configure(decoLoader.getConfigMap()); + + AbstractTypePlanTransformer.checkSecuritySensitiveFields(spec); + return spec; } diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampTypePlanTransformer.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampTypePlanTransformer.java index 09c3834..84717fc 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampTypePlanTransformer.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampTypePlanTransformer.java @@ -109,10 +109,7 @@ public class CampTypePlanTransformer extends AbstractTypePlanTransformer { @Override protected AbstractBrooklynObjectSpec<?, ?> createSpec(RegisteredType type, RegisteredTypeLoadingContext context) throws Exception { try { - return decorateWithCommonTags( - checkSecuritySensitiveFields( - new CampResolver(mgmt, type, context).createSpec() - ), + return decorateWithCommonTags(new CampResolver(mgmt, type, context).createSpec(), type, null, null, prevHeadSpecSummary -> "Based on "+prevHeadSpecSummary); } catch (Exception e) { @@ -140,35 +137,6 @@ public class CampTypePlanTransformer extends AbstractTypePlanTransformer { } } - @Beta - public static AbstractBrooklynObjectSpec<?,?> checkSecuritySensitiveFields(AbstractBrooklynObjectSpec<?,?> spec) { - if (Sanitizer.isSensitiveFieldsPlaintextBlocked()) { - // if blocking plaintext values, check them before instantiating - Predicate<Object> predicate = Sanitizer.IS_SECRET_PREDICATE; - spec.getConfig().forEach( (key,val) -> failOnInsecureValueForSensitiveNamedField(predicate, key.getName(), val) ); - spec.getFlags().forEach( (key,val) -> failOnInsecureValueForSensitiveNamedField(predicate, key, val) ); - } - return spec; - } - - public static void failOnInsecureValueForSensitiveNamedField(Predicate<Object> tokens, String key, Object val) { - if (val instanceof BrooklynDslDeferredSupplier || val==null) { - // value allowed; key is irrelevant - return; - } - if (!tokens.apply(key)) { - // not a sensitive named key - return; - } - - // sensitive named key - if (val instanceof String || Boxing.isPrimitiveOrBoxedClass(val.getClass()) || val instanceof Number) { - // value - throw new IllegalStateException("Insecure value supplied for '"+key+"'; external suppliers must be used here"); - } - // complex values allowed - } - @Override protected Object createBean(RegisteredType type, RegisteredTypeLoadingContext context) throws Exception { // beans not supported by this? diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java index 645155c..90d2feb 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java @@ -18,10 +18,15 @@ */ package org.apache.brooklyn.camp.brooklyn; +import com.google.common.annotations.Beta; import java.util.Map; +import org.apache.brooklyn.core.config.Sanitizer; +import org.apache.brooklyn.core.internal.BrooklynProperties; +import org.apache.brooklyn.core.server.BrooklynServerConfig; import org.apache.brooklyn.util.internal.BrooklynSystemProperties; import org.apache.brooklyn.util.text.StringEscapes.JavaStringEscapes; import org.apache.brooklyn.util.yaml.Yamls; +import org.testng.Assert; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; @@ -552,4 +557,41 @@ public class ConfigYamlTest extends AbstractYamlTest { e -> e.toString().contains("1.5")); } + @Test + public void testSensitiveConfigFailsIfConfigured() throws Exception { + Asserts.assertFailsWith(() -> { + return withSensitiveFieldsBlocked(() -> { + String yaml = Joiner.on("\n").join( + "services:", + "- type: org.apache.brooklyn.core.test.entity.TestEntity", + " brooklyn.config:", + " secret1: myval"); + + return createStartWaitAndLogApplication(yaml); + }); + }, e -> { + Asserts.expectedFailureContainsIgnoreCase(e, "secret1"); + Asserts.expectedFailureDoesNotContain(e, "myval"); + return true; + }); + } + + @Beta + public static <T> T withSensitiveFieldsBlocked(Callable<T> r) throws Exception { + String oldValue = + //((BrooklynProperties) mgmt().getConfig()).put(BrooklynServerConfig.SENSITIVE_FIELDS_PLAINTEXT_BLOCKED, true); + System.setProperty(BrooklynServerConfig.SENSITIVE_FIELDS_PLAINTEXT_BLOCKED.getName(), "true"); + Sanitizer.getSensitiveFieldsTokens(true); + Assert.assertTrue( Sanitizer.isSensitiveFieldsPlaintextBlocked() ); + + try { + + return r.call(); + + } finally { + //((BrooklynProperties) mgmt().getConfig()).put(BrooklynServerConfig.SENSITIVE_FIELDS_PLAINTEXT_BLOCKED, oldValue); + System.setProperty(BrooklynServerConfig.SENSITIVE_FIELDS_PLAINTEXT_BLOCKED.getName(), oldValue!=null ? oldValue : ""); + Sanitizer.getSensitiveFieldsTokens(true); + } + } } diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java index a33329d..ae01172 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java @@ -24,10 +24,13 @@ import java.util.Collection; import java.util.Set; import org.apache.brooklyn.api.objs.BrooklynObject; +import org.apache.brooklyn.camp.brooklyn.ConfigYamlTest; import org.apache.brooklyn.core.entity.Dumper; import org.apache.brooklyn.core.mgmt.BrooklynTags; import org.apache.brooklyn.core.mgmt.BrooklynTags.SpecSummary; import org.apache.brooklyn.util.collections.MutableSet; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; @@ -66,7 +69,9 @@ import com.google.common.collect.Iterables; public class CatalogYamlEntityTest extends AbstractYamlTest { protected static final String TEST_VERSION_SNAPSHOT = TEST_VERSION + "-SNAPSHOT"; - + + private static final Logger LOG = LoggerFactory.getLogger(CatalogYamlEntityTest.class); + @Test public void testAddCatalogItemVerySimple() throws Exception { String symbolicName = "my.catalog.app.id.load"; @@ -204,6 +209,30 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { } @Test + public void testAddCatalogItemWithPlaintextValueForSensitiveNamedFieldBlocked() throws Exception { + String id = "sensitive-fields-1"; + Asserts.assertFailsWith(() -> { + return ConfigYamlTest.withSensitiveFieldsBlocked(() -> { + addCatalogItems( + "brooklyn.catalog:", + " name: " + id, + " itemType: entity", + " item:", + " type: "+ BasicEntity.class.getName(), + " brooklyn.config:", + " secret1: myval"); + return null; + }); + }, e -> { + LOG.info("Got error (as expected): "+e); + Asserts.expectedFailureContainsIgnoreCase(e, "secret1"); + Asserts.expectedFailureDoesNotContain(e, "myval"); + return true; + }); + } + + + @Test public void testLaunchApplicationReferencingCatalog() throws Exception { String symbolicName = "myitem"; addCatalogEntity(IdAndVersion.of(symbolicName, TEST_VERSION), TestEntity.class.getName()); diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index 71098f3..d5ad8b4 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -53,6 +53,7 @@ import org.apache.brooklyn.api.typereg.RegisteredTypeLoadingContext; import org.apache.brooklyn.core.catalog.CatalogPredicates; import org.apache.brooklyn.core.catalog.internal.CatalogClasspathDo.CatalogScanningModes; import org.apache.brooklyn.core.config.ConfigUtils; +import org.apache.brooklyn.core.config.Sanitizer; import org.apache.brooklyn.core.mgmt.BrooklynTags; import org.apache.brooklyn.core.mgmt.classloading.OsgiBrooklynClassLoadingContext; import org.apache.brooklyn.core.mgmt.ha.OsgiBundleInstallationResult; @@ -1736,7 +1737,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } // often in tests we don't have osgi and so it acts as follows - log.debug("Catalog load, adding registered types to "+mgmt+": "+catalogYaml); + log.debug("Catalog load, adding registered types to "+mgmt+": "+ Sanitizer.sanitizeMultilineString(catalogYaml)); if (result==null) result = MutableMap.of(); collectCatalogItemsFromCatalogBomRoot("unbundled catalog definition", catalogYaml, null, null, result, false, MutableMap.of(), 0, forceUpdate, true); diff --git a/core/src/main/java/org/apache/brooklyn/core/config/Sanitizer.java b/core/src/main/java/org/apache/brooklyn/core/config/Sanitizer.java index 07881dc..515f069 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/Sanitizer.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/Sanitizer.java @@ -68,7 +68,8 @@ public final class Sanitizer { private static long LAST_SENSITIVE_FIELDS_CACHE_MILLIS = 60*1000; private static final void refreshProperties(Boolean refresh) { - if (Boolean.FALSE.equals(refresh) || LAST_SENSITIVE_FIELDS_LOAD_TIME + LAST_SENSITIVE_FIELDS_CACHE_MILLIS > System.currentTimeMillis()) { + if (Boolean.FALSE.equals(refresh) || + (refresh==null && (LAST_SENSITIVE_FIELDS_LOAD_TIME + LAST_SENSITIVE_FIELDS_CACHE_MILLIS > System.currentTimeMillis()))) { return; } synchronized (Sanitizer.class) { @@ -98,7 +99,7 @@ public final class Sanitizer { } if (plaintextBlocked==null) { - StringSystemProperty plaintextSP = new StringSystemProperty(SENSITIVE_FIELDS_TOKENS.getName()); + StringSystemProperty plaintextSP = new StringSystemProperty(SENSITIVE_FIELDS_PLAINTEXT_BLOCKED.getName()); if (plaintextSP.isNonEmpty()) { plaintextBlocked = TypeCoercions.coerce(plaintextSP.getValue(), SENSITIVE_FIELDS_PLAINTEXT_BLOCKED.getTypeToken()); } diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java b/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java index 97832b7..2716770 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java @@ -18,7 +18,8 @@ */ package org.apache.brooklyn.core.typereg; -import com.google.common.reflect.TypeToken; +import com.google.common.annotations.Beta; +import com.google.common.base.Predicate; import java.util.List; import java.util.function.Function; import java.util.function.Supplier; @@ -29,12 +30,15 @@ import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.api.typereg.RegisteredTypeLoadingContext; import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog; +import org.apache.brooklyn.core.config.Sanitizer; import org.apache.brooklyn.core.mgmt.BrooklynTags; import org.apache.brooklyn.core.mgmt.BrooklynTags.SpecSummary; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; +import org.apache.brooklyn.util.core.task.DeferredSupplier; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; +import org.apache.brooklyn.util.javalang.Boxing; import org.apache.brooklyn.util.javalang.JavaClassNames; import org.apache.brooklyn.util.text.Strings; import org.slf4j.Logger; @@ -206,6 +210,8 @@ public abstract class AbstractTypePlanTransformer implements BrooklynTypePlanTra addDepthTagsWhereMissing( ((EntitySpec<?>)spec).getChildren(), 1 ); } + checkSecuritySensitiveFields(spec); + return spec; } @@ -219,4 +225,33 @@ public abstract class AbstractTypePlanTransformer implements BrooklynTypePlanTra }); } + @Beta + public static AbstractBrooklynObjectSpec<?,?> checkSecuritySensitiveFields(AbstractBrooklynObjectSpec<?,?> spec) { + if (Sanitizer.isSensitiveFieldsPlaintextBlocked()) { + // if blocking plaintext values, check them before instantiating + Predicate<Object> predicate = Sanitizer.IS_SECRET_PREDICATE; + spec.getConfig().forEach( (key,val) -> failOnInsecureValueForSensitiveNamedField(predicate, key.getName(), val) ); + spec.getFlags().forEach( (key,val) -> failOnInsecureValueForSensitiveNamedField(predicate, key, val) ); + } + return spec; + } + + public static void failOnInsecureValueForSensitiveNamedField(Predicate<Object> tokens, String key, Object val) { + if (val instanceof DeferredSupplier || val==null) { + // value allowed; key is irrelevant + return; + } + if (!tokens.apply(key)) { + // not a sensitive named key + return; + } + + // sensitive named key + if (val instanceof String || Boxing.isPrimitiveOrBoxedClass(val.getClass()) || val instanceof Number) { + // value + throw new IllegalStateException("Insecure value supplied for '"+key+"'; external suppliers must be used here"); + } + // complex values allowed + } + } diff --git a/core/src/test/java/org/apache/brooklyn/core/config/SanitizerTest.java b/core/src/test/java/org/apache/brooklyn/core/config/SanitizerTest.java index b697e95..17c2215 100644 --- a/core/src/test/java/org/apache/brooklyn/core/config/SanitizerTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/config/SanitizerTest.java @@ -18,21 +18,17 @@ */ package org.apache.brooklyn.core.config; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; import java.io.ByteArrayInputStream; -import org.apache.brooklyn.util.stream.Streams; -import org.apache.brooklyn.util.text.Strings; -import static org.testng.Assert.assertEquals; - import java.util.Map; - import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.config.ConfigBag; +import org.apache.brooklyn.util.stream.Streams; +import org.apache.brooklyn.util.text.Strings; +import static org.testng.Assert.assertEquals; import org.testng.annotations.Test; -import com.google.common.base.Functions; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; - public class SanitizerTest { @Test diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/EffectorUtilsRestTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/EffectorUtilsRestTest.java index d7157fd..1ef0c34 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/EffectorUtilsRestTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/EffectorUtilsRestTest.java @@ -89,7 +89,7 @@ public class EffectorUtilsRestTest extends BrooklynAppUnitTestSupport { assertEquals( summary.getDescription(), "Invoking effector resetPassword on "+TestEntityWithEffectors.class.getSimpleName()+":"+entity.getId().substring(0,4) - +" with parameters {"+sensitiveField1+"=xxxxxxxx, "+sensitiveField2+"=xxxxxxxx}", + +" with parameters {"+sensitiveField1+"=<suppressed> (MD5 hash: 5E70B271), "+sensitiveField2+"=<suppressed> (MD5 hash: 81DC9BDB)}", "Task summary must hide sensitive parameters"); }
