Skip to content

Commit 3d2afad

Browse files
maheshpmaheshp
committed
Pluggable tasks secure configurations are encrypted before saving in config.xml #903
* Pluggable tasks with secure configurations were saved as plain text in config.xml, this fix is to store them as encrypted value
1 parent a51bc0d commit 3d2afad

File tree

22 files changed

+258
-95
lines changed

22 files changed

+258
-95
lines changed

‎common/test/unit/com/thoughtworks/go/domain/config/ConfigurationPropertyTest.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public void shouldGetEncryptValueWhenConstructedAsSecure() throws InvalidCipherT
8181
goCipher);
8282
property.handleSecureValueConfiguration(true);
8383
assertThat(property.isSecure(), is(true));
84-
assertThat(property.getEncryptedValue().getValue(), is(encryptedText));
84+
assertThat(property.getEncryptedValue(), is(encryptedText));
8585
assertThat(property.getConfigurationKey().getName(), is("secureKey"));
8686
assertThat(property.getConfigurationValue(), is(nullValue()));
8787
}
@@ -94,7 +94,7 @@ public void shouldNotEncryptWhenWhenConstructedAsNotSecure() throws InvalidCiphe
9494
assertThat(property.isSecure(), is(false));
9595
assertThat(property.getConfigurationKey().getName(), is("secureKey"));
9696
assertThat(property.getConfigurationValue().getValue(), is("secureValue"));
97-
assertThat(property.getEncryptedValue(), is(nullValue()));
97+
assertThat(property.getEncryptedConfigurationValue(), is(nullValue()));
9898
}
9999

100100
@Test
@@ -105,16 +105,16 @@ public void shouldNotClearEncryptedValueWhenWhenNewValueNotProvided() throws Inv
105105
assertThat(property.isSecure(), is(true));
106106
assertThat(property.getConfigurationKey().getName(), is("secureKey"));
107107
assertThat(property.getConfigurationValue(), is(nullValue()));
108-
assertThat(property.getEncryptedValue(), is(notNullValue()));
109-
assertThat(property.getEncryptedValue().getValue(), is("secureValue"));
108+
assertThat(property.getEncryptedConfigurationValue(), is(notNullValue()));
109+
assertThat(property.getEncryptedValue(), is("secureValue"));
110110
}
111111

112112
@Test
113113
public void shouldSetEmptyEncryptedValueWhenValueIsEmptyAndSecure() throws Exception {
114114
GoCipher goCipher = mock(GoCipher.class);
115115
ConfigurationProperty property = new ConfigurationProperty(new ConfigurationKey("secureKey"), new ConfigurationValue(""), new EncryptedConfigurationValue("old"), goCipher);
116116
property.handleSecureValueConfiguration(true);
117-
assertThat(property.getEncryptedValue().getValue(), is(""));
117+
assertThat(property.getEncryptedValue(), is(""));
118118
verify(cipher, never()).decrypt(anyString());
119119
}
120120

@@ -175,7 +175,7 @@ public void shouldInitializeConfigValueToBlankWhenBothValueAndEncryptedValueIsNu
175175
assertThat(configurationProperty.getConfigurationKey().getName(), is("key"));
176176
assertThat(configurationProperty.getConfigurationValue(), is(notNullValue()));
177177
assertThat(configurationProperty.getConfigurationValue().getValue(), is(""));
178-
assertThat(configurationProperty.getEncryptedValue(), is(nullValue()));
178+
assertThat(configurationProperty.getEncryptedConfigurationValue(), is(nullValue()));
179179
Method initializeMethod = ReflectionUtils.findMethod(ConfigurationProperty.class, "initialize");
180180
assertThat(initializeMethod.getAnnotation(PostConstruct.class), is(notNullValue()));
181181
}
@@ -204,7 +204,7 @@ public boolean isSecure(String key) {
204204

205205
assertThat(configurationProperty.getConfigurationKey().getName(), is(secureKey));
206206
assertThat(configurationProperty.getConfigurationValue(), is(nullValue()));
207-
assertThat(configurationProperty.getEncryptedValue().getValue(), is(encryptedValue));
207+
assertThat(configurationProperty.getEncryptedValue(), is(encryptedValue));
208208
}
209209

210210
@Test
@@ -231,7 +231,7 @@ public boolean isSecure(String key) {
231231

232232
assertThat(configurationProperty.getConfigurationKey().getName(), is(secureKey));
233233
assertThat(configurationProperty.getConfigurationValue(), is(nullValue()));
234-
assertThat(configurationProperty.getEncryptedValue().getValue(), is("encryptedValue"));
234+
assertThat(configurationProperty.getEncryptedValue(), is("encryptedValue"));
235235
}
236236

237237
@Test
@@ -249,7 +249,7 @@ public void shouldSetConfigAttributesWhenMetadataIsNotPassedInMap() throws Excep
249249

250250
assertThat(configurationProperty.getConfigurationKey().getName(), is("fooKey"));
251251
assertThat(configurationProperty.getConfigurationValue().getValue(), is("fooValue"));
252-
assertThat(configurationProperty.getEncryptedValue(), is(nullValue()));
252+
assertThat(configurationProperty.getEncryptedConfigurationValue(), is(nullValue()));
253253
}
254254

255255
@Test

‎common/test/unit/com/thoughtworks/go/domain/packagerepository/PackageDefinitionTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,10 +225,10 @@ public void shouldSetConfigAttributes() throws Exception {
225225
assertThat(definition.getName(), is("package-name"));
226226
assertThat(definition.getConfiguration().size(), is(3));
227227
assertThat(definition.getConfiguration().getProperty("key1").getConfigurationValue().getValue(), is("value1"));
228-
assertThat(definition.getConfiguration().getProperty("key1").getEncryptedValue(), is(nullValue()));
229-
assertThat(definition.getConfiguration().getProperty("key2").getEncryptedValue().getValue(), is(encryptedValue));
228+
assertThat(definition.getConfiguration().getProperty("key1").getEncryptedConfigurationValue(), is(nullValue()));
229+
assertThat(definition.getConfiguration().getProperty("key2").getEncryptedValue(), is(encryptedValue));
230230
assertThat(definition.getConfiguration().getProperty("key2").getConfigurationValue(), is(nullValue()));
231-
assertThat(definition.getConfiguration().getProperty("key3").getEncryptedValue().getValue(), is("encrypted-value"));
231+
assertThat(definition.getConfiguration().getProperty("key3").getEncryptedValue(), is("encrypted-value"));
232232
assertThat(definition.getConfiguration().getProperty("key3").getConfigurationValue(), is(nullValue()));
233233
}
234234

‎common/test/unit/com/thoughtworks/go/domain/packagerepository/PackageRepositoryTest.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -164,16 +164,16 @@ public void shouldMakeConfigurationSecureBasedOnMetadata() throws Exception {
164164

165165
//assert package properties
166166
assertThat(secureProperty.isSecure(), is(true));
167-
assertThat(secureProperty.getEncryptedValue(), is(notNullValue()));
168-
assertThat(secureProperty.getEncryptedValue().getValue(), is(goCipher.encrypt("value1")));
167+
assertThat(secureProperty.getEncryptedConfigurationValue(), is(notNullValue()));
168+
assertThat(secureProperty.getEncryptedValue(), is(goCipher.encrypt("value1")));
169169

170170
assertThat(nonSecureProperty.isSecure(), is(false));
171171
assertThat(nonSecureProperty.getValue(), is("value2"));
172172

173173
//assert repository properties
174174
assertThat(secureRepoProperty.isSecure(), is(true));
175-
assertThat(secureRepoProperty.getEncryptedValue(), is(notNullValue()));
176-
assertThat(secureRepoProperty.getEncryptedValue().getValue(), is(goCipher.encrypt("value1")));
175+
assertThat(secureRepoProperty.getEncryptedConfigurationValue(), is(notNullValue()));
176+
assertThat(secureRepoProperty.getEncryptedValue(), is(goCipher.encrypt("value1")));
177177

178178
assertThat(nonSecureRepoProperty.isSecure(), is(false));
179179
assertThat(nonSecureRepoProperty.getValue(), is("value2"));
@@ -193,17 +193,17 @@ public void shouldNotUpdateSecurePropertyWhenPluginIsMissing() {
193193

194194
packageRepository.applyPackagePluginMetadata();
195195

196-
assertThat(secureProperty.getEncryptedValue(), is(notNullValue()));
196+
assertThat(secureProperty.getEncryptedConfigurationValue(), is(notNullValue()));
197197
assertThat(secureProperty.getConfigurationValue(), is(nullValue()));
198198

199199
assertThat(nonSecureProperty.getConfigurationValue(), is(notNullValue()));
200-
assertThat(nonSecureProperty.getEncryptedValue(), is(nullValue()));
200+
assertThat(nonSecureProperty.getEncryptedConfigurationValue(), is(nullValue()));
201201

202-
assertThat(secureRepoProperty.getEncryptedValue(), is(notNullValue()));
202+
assertThat(secureRepoProperty.getEncryptedConfigurationValue(), is(notNullValue()));
203203
assertThat(secureRepoProperty.getConfigurationValue(), is(nullValue()));
204204

205205
assertThat(nonSecureRepoProperty.getConfigurationValue(), is(notNullValue()));
206-
assertThat(nonSecureRepoProperty.getEncryptedValue(), is(nullValue()));
206+
assertThat(nonSecureRepoProperty.getEncryptedConfigurationValue(), is(nullValue()));
207207
}
208208

209209
@Test
@@ -244,11 +244,11 @@ public void shouldSetConfigAttributesAsAvailable() throws Exception {
244244
assertThat(packageRepository.getConfiguration().get(1).getConfigurationValue().getValue(), is(username.value));
245245

246246
assertThat(packageRepository.getConfiguration().get(2).getConfigurationKey().getName(), is(password.name));
247-
assertThat(packageRepository.getConfiguration().get(2).getEncryptedValue().getValue(), is(new GoCipher().encrypt(password.value)));
247+
assertThat(packageRepository.getConfiguration().get(2).getEncryptedValue(), is(new GoCipher().encrypt(password.value)));
248248
assertThat(packageRepository.getConfiguration().get(2).getConfigurationValue(), is(nullValue()));
249249

250250
assertThat(packageRepository.getConfiguration().get(3).getConfigurationKey().getName(), is(secureKeyNotChanged.name));
251-
assertThat(packageRepository.getConfiguration().get(3).getEncryptedValue().getValue(), is(oldEncryptedValue));
251+
assertThat(packageRepository.getConfiguration().get(3).getEncryptedValue(), is(oldEncryptedValue));
252252
assertThat(packageRepository.getConfiguration().get(3).getConfigurationValue(), is(nullValue()));
253253

254254
assertSame(packageRepository.getPackages(), packages);

‎config/config-api/src/com/thoughtworks/go/config/pluggabletask/PluggableTask.java‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import com.thoughtworks.go.plugin.api.task.TaskConfigProperty;
3535
import com.thoughtworks.go.util.ListUtil;
3636

37+
import javax.annotation.PostConstruct;
3738
import java.util.ArrayList;
3839
import java.util.HashMap;
3940
import java.util.List;
@@ -88,10 +89,12 @@ protected void setTaskConfigAttributes(Map attributes) {
8889
for (Property property : taskConfig.list()) {
8990
String key = property.getKey();
9091
if (attributes.containsKey(key)) {
92+
Boolean isSecure = property.getOption(Property.SECURE);
9193
if (configuration.getProperty(key) == null) {
92-
configuration.addNewConfiguration(property.getKey(), property.getOption(Property.SECURE));
94+
configuration.addNewConfiguration(property.getKey(), isSecure);
9395
}
9496
configuration.getProperty(key).setConfigurationValue(new ConfigurationValue((String) attributes.get(key)));
97+
configuration.getProperty(key).handleSecureValueConfiguration(isSecure);
9598
}
9699
}
97100
}
@@ -109,32 +112,41 @@ public TaskConfig toTaskConfig() {
109112
public void addConfigurations(List<ConfigurationProperty> configurations) {
110113
ConfigurationPropertyBuilder builder = new ConfigurationPropertyBuilder();
111114
for (ConfigurationProperty property : configurations) {
112-
String configKey = property.getConfigurationKey() != null ? property.getConfigKeyName() : null;
113-
String encryptedValue = property.getEncryptedValue() != null ? property.getEncryptedValue().getValue() : null;
114-
String configValue = property.getConfigurationValue() != null ? property.getConfigValue() : null;
115115

116-
TaskPreference taskPreference = taskPreference();
117-
if(isValidPluginConfiguration(configKey, taskPreference)) {
118-
configuration.add(builder.create(configKey, configValue, encryptedValue, pluginConfigurationFor(configKey, taskPreference).getOption(Property.SECURE)));
119-
} else
120-
{
116+
if(isValidPluginConfiguration(property.getConfigKeyName())) {
117+
configuration.add(builder.create(property.getConfigKeyName(), property.getConfigValue(), property.getEncryptedValue(),
118+
pluginConfigurationFor(property.getConfigKeyName()).getOption(Property.SECURE)));
119+
} else {
121120
configuration.add(property);
122121
}
123122
}
124123
}
125124

126-
private boolean isValidPluginConfiguration(String configKey, TaskPreference taskPreference) {
127-
return taskPreference != null && pluginConfigurationFor(configKey, taskPreference) != null;
125+
private boolean isValidPluginConfiguration(String configKey) {
126+
return taskPreference() != null && pluginConfigurationFor(configKey) != null;
128127
}
129128

130-
private Property pluginConfigurationFor(String configKey, TaskPreference taskPreference) {
131-
return taskPreference.getConfig().get(configKey);
129+
private Property pluginConfigurationFor(String configKey) {
130+
TaskPreference taskPreference = taskPreference();
131+
return taskPreference != null ? taskPreference.getConfig().get(configKey) : null;
132132
}
133133

134134
private TaskPreference taskPreference() {
135135
return PluggableTaskConfigStore.store().preferenceFor(pluginConfiguration.getId());
136136
}
137137

138+
@PostConstruct
139+
public void applyPluginMetadata() {
140+
if (taskPreference() != null) {
141+
for (ConfigurationProperty configurationProperty : configuration) {
142+
if (isValidPluginConfiguration(configurationProperty.getConfigKeyName())) {
143+
Boolean isSecure = pluginConfigurationFor(configurationProperty.getConfigKeyName()).getOption(Property.SECURE);
144+
configurationProperty.handleSecureValueConfiguration(isSecure);
145+
}
146+
}
147+
}
148+
}
149+
138150
@Override
139151
protected void validateTask(ValidationContext validationContext) {
140152
}
@@ -190,7 +202,7 @@ public Map<String, Map<String, String>> configAsMap() {
190202
Map<String, Map<String, String>> configMap = new HashMap<>();
191203
for (ConfigurationProperty property : configuration) {
192204
Map<String, String> mapValue = new HashMap<>();
193-
mapValue.put(VALUE_KEY, property.getConfigValue());
205+
mapValue.put(VALUE_KEY, property.getValue());
194206
if (!property.errors().isEmpty()) {
195207
mapValue.put(ERRORS_KEY, ListUtil.join(property.errors().getAll()));
196208
}

‎config/config-api/src/com/thoughtworks/go/domain/config/Configuration.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ public void clearEmptyConfigurations() {
111111
List<ConfigurationProperty> propertiesToRemove = new ArrayList<>();
112112
for (ConfigurationProperty configurationProperty : this) {
113113
ConfigurationValue configurationValue = configurationProperty.getConfigurationValue();
114-
EncryptedConfigurationValue encryptedValue = configurationProperty.getEncryptedValue();
114+
EncryptedConfigurationValue encryptedValue = configurationProperty.getEncryptedConfigurationValue();
115115

116116
if (StringUtil.isBlank(configurationProperty.getValue()) && (configurationValue == null || configurationValue.errors().isEmpty()) && (encryptedValue == null || encryptedValue.errors().isEmpty())) {
117117
propertiesToRemove.add(configurationProperty);

‎config/config-api/src/com/thoughtworks/go/domain/config/ConfigurationProperty.java‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public String forFingerprint() {
147147
return format("%s=%s", configurationKey.getName(), getValue());
148148
}
149149

150-
public EncryptedConfigurationValue getEncryptedValue() {
150+
public EncryptedConfigurationValue getEncryptedConfigurationValue() {
151151
return encryptedValue;
152152
}
153153

@@ -206,15 +206,15 @@ public void addError(String fieldName, String message) {
206206

207207
public void addErrorAgainstConfigurationValue(String message) {
208208
if (isSecure()) {
209-
getEncryptedValue().errors().add("value", message);
209+
getEncryptedConfigurationValue().errors().add("value", message);
210210
} else {
211211
getConfigurationValue().errors().add("value", message);
212212
}
213213
}
214214

215215
public boolean doesNotHaveErrorsAgainstConfigurationValue() {
216216
if (isSecure()) {
217-
List<String> errorsOnValue = getEncryptedValue().errors().getAllOn("value");
217+
List<String> errorsOnValue = getEncryptedConfigurationValue().errors().getAllOn("value");
218218
return errorsOnValue == null || errorsOnValue.isEmpty();
219219
} else {
220220
List<String> errorsOnValue = getConfigurationValue().errors().getAllOn("value");
@@ -278,11 +278,15 @@ public void validateKeyUniqueness(HashMap<String, ConfigurationProperty> map, St
278278
}
279279

280280
public String getConfigKeyName() {
281-
return configurationKey.getName();
281+
return configurationKey != null ? configurationKey.getName() : null;
282282
}
283283

284284
public String getConfigValue() {
285-
return configurationValue.getValue();
285+
return configurationValue != null ? configurationValue.getValue() : null;
286+
}
287+
288+
public String getEncryptedValue() {
289+
return encryptedValue != null ? encryptedValue.getValue() : null;
286290
}
287291

288292
public String getDisplayValue() {

‎config/config-api/src/com/thoughtworks/go/domain/scm/SCM.java‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -135,18 +135,14 @@ public void setConfiguration(Configuration configuration) {
135135
public void addConfigurations(List<ConfigurationProperty> configurations) {
136136
ConfigurationPropertyBuilder builder = new ConfigurationPropertyBuilder();
137137
for (ConfigurationProperty property : configurations) {
138-
String configKey = property.getConfigurationKey() != null ? property.getConfigKeyName() : null;
139-
String encryptedValue = property.getEncryptedValue() != null ? property.getEncryptedValue().getValue() : null;
140-
String configValue = property.getConfigurationValue() != null ? property.getConfigValue() : null;
141-
142138
SCMConfigurations scmConfigurations = SCMMetadataStore.getInstance().getConfigurationMetadata(getPluginId());
143-
if (isValidPluginConfiguration(configKey, scmConfigurations)) {
144-
configuration.add(builder.create(configKey, configValue, encryptedValue, scmConfigurationFor(configKey, scmConfigurations).getOption(SCMConfiguration.SECURE)));
139+
if (isValidPluginConfiguration(property.getConfigKeyName(), scmConfigurations)) {
140+
configuration.add(builder.create(property.getConfigKeyName(), property.getConfigValue(), property.getEncryptedValue(),
141+
scmConfigurationFor(property.getConfigKeyName(), scmConfigurations).getOption(SCMConfiguration.SECURE)));
145142
}
146143
else {
147144
configuration.add(property);
148145
}
149-
150146
}
151147
}
152148

0 commit comments

Comments
 (0)