You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2016/06/10 13:23:25 UTC
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
GitHub user nakomis opened a pull request:
https://github.com/apache/brooklyn-server/pull/194
Prevents premature resolving of external config
If external config is resolved too early, it can lead to resolved values being persisted (security flaw). Also, changes to the external config supplier are not reflected when the config is subsequently read
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nakomis/brooklyn-server fix/external-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/194.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #194
----
commit 9185b32e94fa396b8648096309a3fa8c92bd5c4a
Author: Martin Harris <gi...@nakomis.com>
Date: 2016-06-10T13:21:40Z
Prevents premature resolving of external config
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #194: Prevents premature resolving of external config
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/194
LGTM, minor comments, including test and naming
let's give it a day for @aledsage @andreaturli @neykov to comment on names/future and see whether to do in this PR but happy to merge as is
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r66627553
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/JcloudsLocationExternalConfigYamlTest.java ---
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.camp.brooklyn;
+
+import static org.testng.Assert.assertEquals;
+
+import java.io.File;
+import java.io.StringReader;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.camp.brooklyn.ExternalConfigYamlTest.MyExternalConfigSupplier;
+import org.apache.brooklyn.camp.brooklyn.ExternalConfigYamlTest.MyExternalConfigSupplierWithoutMapArg;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.entity.StartableApplication;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import org.apache.brooklyn.core.mgmt.rebind.RebindTestUtils;
+import org.apache.brooklyn.core.objs.BrooklynObjectInternal;
+import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess;
+import org.apache.brooklyn.location.jclouds.JcloudsLocation;
+import org.apache.brooklyn.util.core.internal.ssh.SshTool;
+import org.apache.brooklyn.util.core.task.DeferredSupplier;
+import org.apache.brooklyn.util.guava.Maybe;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+
+// also see ExternalConfigYamlTest
+public class JcloudsLocationExternalConfigYamlTest extends AbstractYamlRebindTest {
+
+ private static final Logger log = LoggerFactory.getLogger(ExternalConfigYamlTest.class);
+
+ private static final ConfigKey<String> MY_CONFIG_KEY = ConfigKeys.newStringConfigKey("my.config.key");
+
+ /** @return A started management context */
+ protected LocalManagementContext createOrigManagementContext() {
+ return RebindTestUtils.managementContextBuilder(mementoDir, classLoader)
+ .properties(externalProps())
+ .persistPeriodMillis(getPersistPeriodMillis())
+ .forLive(useLiveManagementContext())
+ .emptyCatalog(useEmptyCatalog())
+ .buildStarted();
+ }
+
+ /** @return An unstarted management context using the specified mementoDir (or default if null) */
+ protected LocalManagementContext createNewManagementContext(File mementoDir) {
+ if (mementoDir==null) mementoDir = this.mementoDir;
+ return RebindTestUtils.managementContextBuilder(mementoDir, classLoader)
+ .properties(externalProps())
+ .forLive(useLiveManagementContext())
+ .emptyCatalog(useEmptyCatalog())
+ .buildUnstarted();
+ }
+
+ private BrooklynProperties externalProps() {
+ BrooklynProperties props = BrooklynProperties.Factory.newDefault();
+ props.put("brooklyn.external.myprovider", MyExternalConfigSupplier.class.getName());
+ props.put("brooklyn.external.myprovider.mykey", "myval");
+ props.put("brooklyn.external.myproviderWithoutMapArg", MyExternalConfigSupplierWithoutMapArg.class.getName());
+ return props;
+ }
+
+ @Test(groups="Live")
+ public void testJcloudsInheritanceAndPasswordSecret() throws Exception {
+ String yaml = Joiner.on("\n").join(
+ "services:",
+ "- type: "+EmptySoftwareProcess.class.getName(),
+ "location:",
+ " jclouds:aws-ec2:",
+ " password: $brooklyn:external(\"myprovider\", \"mykey\")",
+ " my.config.key: $brooklyn:external(\"myprovider\", \"mykey\")");
+
+ origApp = (StartableApplication) createAndStartApplication(new StringReader(yaml));
+
+ Entity entity = Iterables.getOnlyElement( origApp.getChildren() );
+ Location l = Iterables.getOnlyElement( entity.getLocations() );
+ log.info("Location: "+l);
+ assertEquals(l.config().get(MY_CONFIG_KEY), "myval");
+
+ Maybe<Object> rawConfig = ((BrooklynObjectInternal.ConfigurationSupportInternal)l.config()).getRaw(MY_CONFIG_KEY);
+ log.info("Raw config: "+rawConfig);
+ Assert.assertTrue(rawConfig.isPresentAndNonNull());
+ Assert.assertTrue(rawConfig.get() instanceof DeferredSupplier, "Expected deferred raw value; got "+rawConfig.get());
+
+ rawConfig = ((BrooklynObjectInternal.ConfigurationSupportInternal)l.config()).getRaw(SshTool.PROP_PASSWORD);
+ log.info("Raw config password: "+rawConfig);
+ Assert.assertTrue(rawConfig.isPresentAndNonNull());
+ Assert.assertTrue(rawConfig.get() instanceof DeferredSupplier, "Expected deferred raw value; got "+rawConfig.get());
+ }
+
+ @Test(groups="Live")
+ public void testProvisioningPropertyInheritance() throws Exception {
+ String yaml = Joiner.on("\n").join(
+ "services:",
+ "- type: "+EmptySoftwareProcess.class.getName(),
+ " provisioning.properties:",
+ " password: $brooklyn:external(\"myprovider\", \"mykey\")",
+ // note that these 2 do not get transferred -- see below
+ " simple: 42",
+ " my.config.key: $brooklyn:external(\"myprovider\", \"mykey\")",
+ "location: aws-ec2");
+
+ origApp = (StartableApplication) createAndStartApplication(new StringReader(yaml));
+
+ Entity entity = Iterables.getOnlyElement( origApp.getChildren() );
+ Location l = Iterables.getOnlyElement( entity.getLocations() );
+ log.info("Location: "+l);
+ assertEquals(l.config().get(JcloudsLocation.PASSWORD), "myval");
+
+ Maybe<Object> rawConfig = ((BrooklynObjectInternal.ConfigurationSupportInternal)l.config()).getRaw(ConfigKeys.newStringConfigKey("password"));
+ log.info("Raw config password: "+rawConfig);
+ Assert.assertTrue(rawConfig.isPresentAndNonNull());
+ Assert.assertTrue(rawConfig.get() instanceof DeferredSupplier, "Expected deferred raw value; got "+rawConfig.get());
+
+ // these are null as only recognised provisioning properties are transmitted by jclouds
--- End diff --
this is an ugly inconsistency between how jclouds handles `obtain(flags)` and others (eg localhost, see the test in the other class) which i had never noticed
@nakomis @aledsage @andreaturli @neykov wdyt, should unrecognised flags passed in `obtain(...)` *(a)* be passed through and set on the provisioned machine location, as in localhost, or *(b)* be dropped (optionally with warning or failure), as in jclouds ?
(not necessarily for this PR, this is just where we've noticed it)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r66626931
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/JcloudsLocationExternalConfigYamlTest.java ---
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.camp.brooklyn;
+
+import static org.testng.Assert.assertEquals;
+
+import java.io.File;
+import java.io.StringReader;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.location.Location;
+import org.apache.brooklyn.camp.brooklyn.ExternalConfigYamlTest.MyExternalConfigSupplier;
+import org.apache.brooklyn.camp.brooklyn.ExternalConfigYamlTest.MyExternalConfigSupplierWithoutMapArg;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.entity.StartableApplication;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import org.apache.brooklyn.core.mgmt.rebind.RebindTestUtils;
+import org.apache.brooklyn.core.objs.BrooklynObjectInternal;
+import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess;
+import org.apache.brooklyn.location.jclouds.JcloudsLocation;
+import org.apache.brooklyn.util.core.internal.ssh.SshTool;
+import org.apache.brooklyn.util.core.task.DeferredSupplier;
+import org.apache.brooklyn.util.guava.Maybe;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+
+// also see ExternalConfigYamlTest
+public class JcloudsLocationExternalConfigYamlTest extends AbstractYamlRebindTest {
+
+ private static final Logger log = LoggerFactory.getLogger(ExternalConfigYamlTest.class);
+
+ private static final ConfigKey<String> MY_CONFIG_KEY = ConfigKeys.newStringConfigKey("my.config.key");
+
+ /** @return A started management context */
+ protected LocalManagementContext createOrigManagementContext() {
--- End diff --
i think in 0.10.0-SNAPSHOT the superclass has a `createBrooklynProperties()` method you could extend instead, removing the `create*ManagementContext(...)` overrides
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #194: Prevents premature resolving of external config
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the issue:
https://github.com/apache/brooklyn-server/pull/194
This is in addition to the changes in https://github.com/apache/brooklyn-server/pull/193
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r66628347
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java ---
@@ -367,6 +367,22 @@ public Object getStringKey(String key) {
}
}
+ /** Puts into this bag the raw value for the given key in the given bag, if it was present. */
+ @Beta
+ public <T> ConfigBag copyRaw(ConfigBag source, ConfigKey<T> key) {
--- End diff --
would `copy(...)` be a better name? `Raw` makes it feel ugly when actually i think this is quite a nice pattern for dealing with transferring values without resolving them
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #194: Prevents premature resolving of external config
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/194
+1 @neykov we could have `coerced()` and `future()` methods with `get()` a shorthand for the former
we still have the question what should `obtain(...)` do with unrecognised flags -- current behaviour is inconsistent. address that another day.
merging, thanks @nakomis .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #194: Prevents premature resolving of external config
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/194
I'm fine with merging the proposed changes, but I think it's papering over a broken abstraction. We can't hide the fact that we have deferred values in the configuration so better expose it into the API and make use of it.
I propose that longer term we add methods that return something similar to `ListenableFuture` (even for already resolved values). Then use the existing `get` methods as a shorthand when we will make immediate use of the value. More complex scenarios will push the futures around. This will even allow having chains of dependent values so that when one changes all dependents change as well - for example having a live `$brooklyn:formatString` updating as the underlying sensors update.
This still leaves the case of complex objects - we are currently resolving their parts ad-hoc (for example maps). Could we improve our parser so it knows that an object contains futures and wrap the object itself in a future?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/194
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r66628716
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -2422,7 +2437,7 @@ protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService
.configure(WinRmMachineLocation.WINRM_CONFIG_PORT, sshHostAndPort.isPresent() ? sshHostAndPort.get().getPort() : node.getLoginPort())
.configure("user", getUser(setup))
.configure(WinRmMachineLocation.USER, setup.get(USER))
- .configure(WinRmMachineLocation.PASSWORD, setup.get(PASSWORD))
+ .configure(ConfigBag.newInstance().copyRaw(setup, PASSWORD, WinRmMachineLocation.PASSWORD).getAllConfigRaw())
--- End diff --
this changes the spec configuration to set `password` as a flag instead of a config key, and it feels ugly
as @nakomis noted to me offline the WinRM impl doesn't create a user so this field is probably actually being ignored, and even if not the flag/config change should result in the same WinRM machine location configuration
but if we do start doing this more (which seems likely if we want to prevent resolution) we probably want to introduce something cleaner such as
configureCopyKey(ConfigBag, ConfigKey<?>)
configureCopyKeys(ConfigBag, ConfigKey<?>, ConfigKey<?>, ...ConfigKey<?>)
configureCopyKeyAs(ConfigBag, ConfigKey<?> sourceName, ConfigKey<?> targetName)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r67360931
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -2422,7 +2437,7 @@ protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService
.configure(WinRmMachineLocation.WINRM_CONFIG_PORT, sshHostAndPort.isPresent() ? sshHostAndPort.get().getPort() : node.getLoginPort())
.configure("user", getUser(setup))
.configure(WinRmMachineLocation.USER, setup.get(USER))
- .configure(WinRmMachineLocation.PASSWORD, setup.get(PASSWORD))
+ .configure(ConfigBag.newInstance().copyRaw(setup, PASSWORD, WinRmMachineLocation.PASSWORD).getAllConfigRaw())
--- End diff --
This would require adding a dependency on brooklyn-core to brooklyn-api, which would create a circular dependency
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r66627723
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java ---
@@ -367,6 +367,22 @@ public Object getStringKey(String key) {
}
}
+ /** Puts into this bag the raw value for the given key in the given bag, if it was present. */
+ @Beta
+ public <T> ConfigBag copyRaw(ConfigBag source, ConfigKey<T> key) {
--- End diff --
this is the significant api addition
it feels right to me but wdyt @aledsage ?
also @nakomis we should probably add a test for this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #194: Prevents premature resolving of external ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/194#discussion_r66629162
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java ---
@@ -367,6 +367,22 @@ public Object getStringKey(String key) {
}
}
+ /** Puts into this bag the raw value for the given key in the given bag, if it was present. */
+ @Beta
+ public <T> ConfigBag copyRaw(ConfigBag source, ConfigKey<T> key) {
--- End diff --
or in line with the suggestion on `configureCopy...` below
copyKey(ConfigBag, ConfigKey<?>)
copyKeys(ConfigBag, ConfigKey<?>, ConfigKey<?>, ...ConfigKey<?>)
copyKeyAs(ConfigBag, ConfigKey<?> sourceName, ConfigKey<?> targetName)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #194: Prevents premature resolving of external config
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the issue:
https://github.com/apache/brooklyn-server/pull/194
PR comments addressed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---