You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2021/09/15 14:48:49 UTC
[brooklyn-server] 09/11: block security in more places, fix some
logic, add tests; and more sanitizing of secrets
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 <al...@cloudsoftcorp.com>
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");
}