Skip to content

Commit

Permalink
Validate secret lookup response for unwanted secrets. (gocd#6099)
Browse files Browse the repository at this point in the history 
- Error out with `SecretResolutionFailureException` if plugin sends
  additional secret params in lookup secret response.

> Response with addtional secret params
```
requestBody = ["key1", "key2"]

responseBody = [
	{key: "key1", value="value-1"},
	{key: "key2", value="value-2"},
	{key: "key3", value="value-3"}
]

This will throw `SecretResolutionFailureException` with message -

Expected plugin to resolve secret(s) `key1, key2` but plugin sent addition secret param(s) `key3`.

```

- Error out with `SecretResolutionFailureException` if plugin fails to
  resolve one or more secret params as part of lookup secret response
  and sends partially resolved secret pamars in response.

>  Response with partially resolved secret params
```
requestBody = ["key1", "key2", "key3"]

responseBody = [
	{key: "key1", value="value-1"}
]

This will throw `SecretResolutionFailureException` with message -

Expected plugin to resolve secret(s) `key1, key2, key3` but plugin failed resolve secret param(s) `key2, key3`. Please make sure that secret with same name exist in your secret management tool.
```
  • Loading branch information
@bdpiprava
bdpiprava authored Apr 9, 2019
1 parent d52d487 commit eb2a9b4
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 26 deletions.
17 changes: 17 additions & 0 deletions ...n/java/com/thoughtworks/go/plugin/access/exceptions/SecretResolutionFailureException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,25 @@

package com.thoughtworks.go.plugin.access.exceptions;

import java.util.Set;

import static java.lang.String.format;

public class SecretResolutionFailureException extends RuntimeException {
public SecretResolutionFailureException(String message) {
super(message);
}

public static SecretResolutionFailureException withMissingSecretParams(Set<String> secretsToResolve, Set<String> missingSecrets) {
return new SecretResolutionFailureException(format("Expected plugin to resolve secret(s) `%s` but plugin failed resolve secret param(s) `%s`. Please make sure that secret with same name exist in your secret management tool.", csv(secretsToResolve), csv(missingSecrets)));
}


public static SecretResolutionFailureException withUnwantedSecretParams(Set<String> secretsToResolve, Set<String> unwantedSecrets) {
return new SecretResolutionFailureException(format("Expected plugin to resolve secret(s) `%s` but plugin sent addition secret param(s) `%s`.", csv(secretsToResolve), csv(unwantedSecrets)));
}

private static String csv(Set<String> secretsToResolve) {
return String.join(", ", secretsToResolve);
}
}
18 changes: 17 additions & 1 deletion ...ugin-access/src/main/java/com/thoughtworks/go/plugin/access/secrets/SecretsExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,19 @@
import com.thoughtworks.go.plugin.access.ExtensionsRegistry;
import com.thoughtworks.go.plugin.access.PluginRequestHelper;
import com.thoughtworks.go.plugin.access.common.AbstractExtension;
import com.thoughtworks.go.plugin.access.exceptions.SecretResolutionFailureException;
import com.thoughtworks.go.plugin.access.secrets.v1.SecretsExtensionV1;
import com.thoughtworks.go.plugin.api.response.validation.ValidationResult;
import com.thoughtworks.go.plugin.domain.common.Image;
import com.thoughtworks.go.plugin.domain.common.PluginConfiguration;
import com.thoughtworks.go.plugin.domain.secrets.Secret;
import com.thoughtworks.go.plugin.infra.PluginManager;
import org.apache.commons.collections4.SetUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

import java.util.*;
import java.util.stream.Collectors;

import static com.thoughtworks.go.plugin.domain.common.PluginConstants.SECRETS_EXTENSION;

Expand Down Expand Up @@ -73,7 +76,20 @@ public ValidationResult validateSecretsConfig(final String pluginId, final Map<S
}

public List<Secret> lookupSecrets(String pluginId, SecretConfig secretConfig, Set<String> keys) {
return getVersionedSecretsExtension(pluginId).lookupSecrets(pluginId, secretConfig, keys);
final List<Secret> secrets = getVersionedSecretsExtension(pluginId).lookupSecrets(pluginId, secretConfig, keys);
final Set<String> resolvedSecrets = secrets.stream().map(Secret::getKey).collect(Collectors.toSet());

final Set<String> additionalSecretsInResponse = SetUtils.difference(resolvedSecrets, keys).toSet();
if (!additionalSecretsInResponse.isEmpty()) {
throw SecretResolutionFailureException.withUnwantedSecretParams(keys, additionalSecretsInResponse);
}

if (resolvedSecrets.containsAll(keys)) {
return secrets;
}

final Set<String> missingSecrets = SetUtils.disjunction(resolvedSecrets, keys).toSet();
throw SecretResolutionFailureException.withMissingSecretParams(keys, missingSecrets);
}

protected VersionedSecretsExtension getVersionedSecretsExtension(String pluginId) {
Expand Down
95 changes: 71 additions & 24 deletions ...-access/src/test/java/com/thoughtworks/go/plugin/access/secrets/SecretsExtensionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,25 @@
import com.thoughtworks.go.config.SecretConfig;
import com.thoughtworks.go.plugin.access.ExtensionsRegistry;
import com.thoughtworks.go.plugin.access.common.AbstractExtension;
import com.thoughtworks.go.plugin.access.exceptions.SecretResolutionFailureException;
import com.thoughtworks.go.plugin.access.secrets.v1.SecretsExtensionV1;
import com.thoughtworks.go.plugin.domain.secrets.Secret;
import com.thoughtworks.go.plugin.infra.PluginManager;
import com.thoughtworks.go.plugin.infra.plugininfo.GoPluginDescriptor;
import com.thoughtworks.go.util.ReflectionUtil;
import org.junit.Before;
import org.junit.Test;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

import java.util.*;

import static com.thoughtworks.go.plugin.access.secrets.SecretsExtension.SUPPORTED_VERSIONS;
import static com.thoughtworks.go.plugin.domain.common.PluginConstants.SECRETS_EXTENSION;
import static java.util.Arrays.asList;
import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.mockito.Mockito.*;

public class SecretsExtensionTest {
Expand All @@ -43,8 +47,8 @@ public class SecretsExtensionTest {
protected GoPluginDescriptor descriptor;
protected SecretsExtension extension;

@Before
public void setUp() throws Exception {
@BeforeEach
void setUp() {
pluginManager = mock(PluginManager.class);
extensionsRegistry = mock(ExtensionsRegistry.class);
descriptor = mock(GoPluginDescriptor.class);
Expand All @@ -56,21 +60,21 @@ public void setUp() throws Exception {
}

@Test
public void shouldHaveVersionedSecretsExtensionForAllSupportedVersions() {
void shouldHaveVersionedSecretsExtensionForAllSupportedVersions() {
for (String supportedVersion : SUPPORTED_VERSIONS) {
final String message = String.format("Must define versioned extension class for %s extension with version %s", SECRETS_EXTENSION, supportedVersion);

when(pluginManager.resolveExtensionVersion(PLUGIN_ID, SECRETS_EXTENSION, SUPPORTED_VERSIONS)).thenReturn(supportedVersion);

final VersionedSecretsExtension extension = this.extension.getVersionedSecretsExtension(PLUGIN_ID);

assertNotNull(message, extension);
assertThat(ReflectionUtil.getField(extension, "VERSION"), is(supportedVersion));
assertThat(extension).as(message).isNotNull();
assertThat(ReflectionUtil.getField(extension, "VERSION")).isEqualTo(supportedVersion);
}
}

@Test
public void getIcon_shouldDelegateToVersionedExtension() {
void getIcon_shouldDelegateToVersionedExtension() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
Expand All @@ -83,7 +87,7 @@ public void getIcon_shouldDelegateToVersionedExtension() {
}

@Test
public void getSecretsConfigMetadata_shouldDelegateToVersionedExtension() {
void getSecretsConfigMetadata_shouldDelegateToVersionedExtension() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
Expand All @@ -96,7 +100,7 @@ public void getSecretsConfigMetadata_shouldDelegateToVersionedExtension() {
}

@Test
public void getSecretsConfigView_shouldDelegateToVersionedExtension() {
void getSecretsConfigView_shouldDelegateToVersionedExtension() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
Expand All @@ -109,7 +113,7 @@ public void getSecretsConfigView_shouldDelegateToVersionedExtension() {
}

@Test
public void validateSecretsConfig_shouldDelegateToVersionedExtension() {
void validateSecretsConfig_shouldDelegateToVersionedExtension() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
Expand All @@ -122,22 +126,65 @@ public void validateSecretsConfig_shouldDelegateToVersionedExtension() {
verify(secretsExtensionV1).validateSecretsConfig(PLUGIN_ID, configuration);
}

@Test
public void lookupSecrets_shouldDelegateToVersionedExtension() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
Set<String> keys = new HashSet<>(asList("key1", "key2"));
@Nested
class lookupSecrets {
@Test
void shouldDelegateToVersionedExtension() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
Set<String> keys = new HashSet<>(asList("key1", "key2"));

when(pluginManager.resolveExtensionVersion(PLUGIN_ID, SECRETS_EXTENSION, SUPPORTED_VERSIONS)).thenReturn(SecretsExtensionV1.VERSION);
when(secretsExtensionV1.lookupSecrets(PLUGIN_ID, new SecretConfig(), keys)).thenReturn(Arrays.asList(
new Secret("key1", "value-1"),
new Secret("key2", "value-2")
));

final List<Secret> secrets = extension.lookupSecrets(PLUGIN_ID, new SecretConfig(), keys);

assertThat(secrets).hasSize(2)
.contains(new Secret("key1", "value-1"), new Secret("key2", "value-2"));
verify(secretsExtensionV1).lookupSecrets(PLUGIN_ID, new SecretConfig(), keys);
}

when(pluginManager.resolveExtensionVersion(PLUGIN_ID, SECRETS_EXTENSION, SUPPORTED_VERSIONS)).thenReturn(SecretsExtensionV1.VERSION);
@Test
void shouldBombIfResolvedSecretContainsAdditionalSecrets() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
final Set<String> secretsToLookup = new HashSet<>(asList("key1", "key2"));

when(pluginManager.resolveExtensionVersion(PLUGIN_ID, SECRETS_EXTENSION, SUPPORTED_VERSIONS)).thenReturn(SecretsExtensionV1.VERSION);
when(secretsExtensionV1.lookupSecrets(PLUGIN_ID, new SecretConfig(), secretsToLookup)).thenReturn(Arrays.asList(
new Secret("key1", "value-1"),
new Secret("key2", "value-2"),
new Secret("key3", "value-3")
));

assertThatCode(() -> extension.lookupSecrets(PLUGIN_ID, new SecretConfig(), secretsToLookup))
.isInstanceOf(SecretResolutionFailureException.class)
.hasMessage("Expected plugin to resolve secret(s) `key1, key2` but plugin sent addition secret param(s) `key3`.");
}

this.extension.lookupSecrets(PLUGIN_ID, new SecretConfig(), keys);
@Test
void shouldBombWhenPluginReturnsPartiallyResolvedSecretParams() {
SecretsExtensionV1 secretsExtensionV1 = mock(SecretsExtensionV1.class);
Map<String, VersionedSecretsExtension> secretsExtensionMap = singletonMap("1.0", secretsExtensionV1);
extension = new SecretsExtension(pluginManager, extensionsRegistry, secretsExtensionMap);
final Set<String> secretsToLookup = new HashSet<>(asList("key1", "key2", "key3"));

verify(secretsExtensionV1).lookupSecrets(PLUGIN_ID, new SecretConfig(), keys);
when(pluginManager.resolveExtensionVersion(PLUGIN_ID, SECRETS_EXTENSION, SUPPORTED_VERSIONS)).thenReturn(SecretsExtensionV1.VERSION);
when(secretsExtensionV1.lookupSecrets(PLUGIN_ID, new SecretConfig(), secretsToLookup)).thenReturn(singletonList(new Secret("key1", "value-1")));

assertThatCode(() -> extension.lookupSecrets(PLUGIN_ID, new SecretConfig(), secretsToLookup))
.isInstanceOf(SecretResolutionFailureException.class)
.hasMessage("Expected plugin to resolve secret(s) `key1, key2, key3` but plugin failed resolve secret param(s) `key2, key3`. Please make sure that secret with same name exist in your secret management tool.");
}
}

@Test
public void shouldExtendAbstractExtension() {
assertTrue(new SecretsExtension(pluginManager, extensionsRegistry) instanceof AbstractExtension);
void shouldExtendAbstractExtension() {
assertThat(new SecretsExtension(pluginManager, extensionsRegistry) instanceof AbstractExtension).isTrue();
}
}
1 change: 0 additions & 1 deletion server/src/main/java/com/thoughtworks/go/server/service/SecretParamResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ private BiConsumer<String, SecretParams> lookupAndUpdateSecretParamsValue() {
List<Secret> resolvedSecrets = secretsExtension.lookupSecrets(secretConfig.getPluginId(), secretConfig, secretParamMap.keySet());
LOGGER.debug("Resolved secret size '{}'", resolvedSecrets.size());


LOGGER.debug("Updating secret params '{}' with values.", secretParamMap.keySet());
resolvedSecrets.forEach(assignValue(secretParamMap));
LOGGER.debug("Secret params '{}' updated with values.", secretParamMap.keySet());
Expand Down

0 comments on commit eb2a9b4

Please sign in to comment.