Skip to content
This repository was archived by the owner on May 14, 2025. It is now read-only.

Commit 20b19cf

Browse files
tzolovtzolov
committed
Incorrect Stream properties sanitizing
Resolves #2196 for 1.5.0 branch - Parse/process Stream's DSL using the java API (e.g. StreamDefinition, StreamAppDefinition) instead of incomplete regular expressions. - Fix the affected tests
1 parent cc58c2e commit 20b19cf

File tree

5 files changed

+55
-76
lines changed

5 files changed

+55
-76
lines changed

‎spring-cloud-dataflow-server-core/src/main/java/org/springframework/cloud/dataflow/server/controller/StreamDefinitionController.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ public StreamDefinitionResource toResource(StreamDefinition stream) {
247247
@Override
248248
public StreamDefinitionResource instantiateResource(StreamDefinition stream) {
249249
final StreamDefinitionResource resource = new StreamDefinitionResource(stream.getName(),
250-
ArgumentSanitizer.sanitizeStream(stream.getDslText()));
250+
new ArgumentSanitizer().sanitizeStream(stream));
251251
DeploymentState deploymentState = streamDeploymentStates.get(stream);
252252
if (deploymentState != null) {
253253
final DeploymentStateResource deploymentStateResource = ControllerUtils

‎spring-cloud-dataflow-server-core/src/main/java/org/springframework/cloud/dataflow/server/controller/StreamDeploymentController.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ public StreamDeploymentResource instantiateResource(StreamDeployment streamDeplo
184184
deploymentProperties = streamDeployment.getDeploymentProperties();
185185
}
186186
return new StreamDeploymentResource(streamDeployment.getStreamName(),
187-
ArgumentSanitizer.sanitizeStream(this.dslText), deploymentProperties, this.status);
187+
new ArgumentSanitizer().sanitizeStream(new StreamDefinition(streamDeployment.getStreamName(), this.dslText)), deploymentProperties, this.status);
188188
}
189189

190190
private boolean canDisplayDeploymentProperties() {

‎spring-cloud-dataflow-server-core/src/main/java/org/springframework/cloud/dataflow/server/controller/support/ArgumentSanitizer.java

Lines changed: 25 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,14 @@
1616

1717
package org.springframework.cloud.dataflow.server.controller.support;
1818

19-
import java.util.regex.Matcher;
19+
import java.util.List;
20+
import java.util.Map;
2021
import java.util.regex.Pattern;
22+
import java.util.stream.Collectors;
2123

22-
import org.springframework.util.Assert;
23-
import org.springframework.util.StringUtils;
24+
import org.springframework.cloud.dataflow.core.StreamAppDefinition;
25+
import org.springframework.cloud.dataflow.core.StreamDefinition;
26+
import org.springframework.cloud.dataflow.core.StreamDefinitionToDslConverter;
2427

2528
/**
2629
* Sanitizes potentially sensitive keys for a specific command line arg.
@@ -34,24 +37,17 @@ public class ArgumentSanitizer {
3437

3538
private static final String[] KEYS_TO_SANITIZE = { "password", "secret", "key", "token", ".*credentials.*",
3639
"vcap_services" };
37-
//used to find the passwords embedded in a stream definition
38-
private static Pattern passwordParameterPatternForStreams = Pattern.compile(
39-
//Search for the -- characters then look for unicode letters
40-
"(?i)(--[\\p{Z}]*[\\p{L}.]*[\\p{Pd}]*("
41-
//that match the following strings from the KEYS_TO_SANITIZE array
42-
+ StringUtils.arrayToDelimitedString(KEYS_TO_SANITIZE, "|")
43-
//Following the equal sign (group1) accept any number of unicode characters(letters, open punctuation, close punctuation etc) for the value to be sanitized for group 3.
44-
+ ")[\\p{L}-]*[\\p{Z}]*=[\\p{Z}]*)((\"[\\p{L}-|\\p{Pd}|\\p{Ps}|\\p{Pe}|\\p{Pc}|\\p{S}|\\p{N}|\\p{Z}]*\")|([\\p{N}|\\p{L}-|\\p{Po}|\\p{Pc}|\\p{S}]*))",
45-
Pattern.UNICODE_CASE);
46-
4740

4841
private Pattern[] keysToSanitize;
4942

43+
private StreamDefinitionToDslConverter dslConverter;
44+
5045
public ArgumentSanitizer() {
5146
this.keysToSanitize = new Pattern[KEYS_TO_SANITIZE.length];
5247
for (int i = 0; i < keysToSanitize.length; i++) {
5348
this.keysToSanitize[i] = getPattern(KEYS_TO_SANITIZE[i]);
5449
}
50+
this.dslConverter = new StreamDefinitionToDslConverter();
5551
}
5652

5753
private Pattern getPattern(String value) {
@@ -107,38 +103,24 @@ public String sanitize(String key, String value) {
107103
}
108104

109105
/**
110-
* Redacts sensitive values in a stream.
106+
* Redacts sensitive property values in a stream.
111107
*
112-
* @param definition the definition to sanitize
113-
* @return Stream definition that has sensitive data redacted.
108+
* @param streamDefinition the stream definition to sanitize
109+
* @return Stream definition text that has sensitive data redacted.
114110
*/
115-
public static String sanitizeStream(String definition) {
116-
Assert.hasText(definition, "definition must not be null nor empty");
117-
final StringBuffer output = new StringBuffer();
118-
final Matcher matcher = passwordParameterPatternForStreams.matcher(definition);
119-
while (matcher.find()) {
120-
String passwordValue = matcher.group(3);
121-
122-
String maskedPasswordValue;
123-
boolean isPipeAppended = false;
124-
boolean isNameChannelAppended = false;
125-
if (passwordValue.endsWith("|")) {
126-
isPipeAppended = true;
127-
}
128-
if (passwordValue.endsWith(">")) {
129-
isNameChannelAppended = true;
130-
}
131-
maskedPasswordValue = REDACTION_STRING;
132-
if (isPipeAppended) {
133-
maskedPasswordValue = maskedPasswordValue + " |";
134-
}
135-
if (isNameChannelAppended) {
136-
maskedPasswordValue = maskedPasswordValue + " >";
137-
}
138-
matcher.appendReplacement(output, matcher.group(1) + maskedPasswordValue);
139-
}
140-
matcher.appendTail(output);
141-
return output.toString();
111+
public String sanitizeStream(StreamDefinition streamDefinition) {
112+
List<StreamAppDefinition> sanitizedAppDefinitions = streamDefinition.getAppDefinitions().stream()
113+
.map(app -> StreamAppDefinition.Builder
114+
.from(app)
115+
.setProperties(this.sanitizeProperties(app.getProperties()))
116+
.build(streamDefinition.getName())
117+
).collect(Collectors.toList());
118+
119+
return this.dslConverter.toDsl(sanitizedAppDefinitions);
142120
}
143121

122+
private Map<String, String> sanitizeProperties(Map<String, String> properties) {
123+
return properties.entrySet().stream()
124+
.collect(Collectors.toMap(e -> e.getKey(), e -> this.sanitize(e.getKey(), e.getValue())));
125+
}
144126
}

‎spring-cloud-dataflow-server-core/src/test/java/org/springframework/cloud/dataflow/server/controller/StreamControllerTests.java

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030

3131
import org.springframework.beans.factory.annotation.Autowired;
3232
import org.springframework.boot.test.context.SpringBootTest;
33-
import org.springframework.cloud.dataflow.configuration.metadata.ApplicationConfigurationMetadataResolver;
3433
import org.springframework.cloud.dataflow.core.BindingPropertyKeys;
3534
import org.springframework.cloud.dataflow.core.StreamAppDefinition;
3635
import org.springframework.cloud.dataflow.core.StreamDefinition;
@@ -40,7 +39,6 @@
4039
import org.springframework.cloud.dataflow.server.repository.DeploymentIdRepository;
4140
import org.springframework.cloud.dataflow.server.repository.DeploymentKey;
4241
import org.springframework.cloud.dataflow.server.repository.StreamDefinitionRepository;
43-
import org.springframework.cloud.dataflow.server.service.StreamService;
4442
import org.springframework.cloud.deployer.resource.maven.MavenResource;
4543
import org.springframework.cloud.deployer.spi.app.AppDeployer;
4644
import org.springframework.cloud.deployer.spi.app.AppInstanceStatus;
@@ -95,9 +93,6 @@ public class StreamControllerTests {
9593
@Autowired
9694
private DeploymentIdRepository deploymentIdRepository;
9795

98-
@Autowired
99-
private ApplicationConfigurationMetadataResolver metadataResolver;
100-
10196
private MockMvc mockMvc;
10297

10398
@Autowired
@@ -109,9 +104,6 @@ public class StreamControllerTests {
109104
@Autowired
110105
private CommonApplicationProperties appsProperties;
111106

112-
@Autowired
113-
private StreamService defaultStreamService;
114-
115107
@Before
116108
public void setupMocks() {
117109
this.mockMvc = MockMvcBuilders.webAppContextSetup(wac)
@@ -261,8 +253,8 @@ public void testFindRelatedAndNestedStreams() throws Exception {
261253
response = mockMvc
262254
.perform(get("/streams/definitions/myStream5/related?nested=true").accept(MediaType.APPLICATION_JSON))
263255
.andReturn().getResponse().getContentAsString();
264-
assertTrue(response.contains(":myStream5.time > log --password=******"));
265-
assertTrue(response.contains("time | log --secret=******"));
256+
assertTrue(response.contains(":myStream5.time > log --password='******'"));
257+
assertTrue(response.contains("time | log --secret='******'"));
266258
assertTrue(response.contains("\"totalElements\":2"));
267259

268260
String response2 = mockMvc.perform(
@@ -313,7 +305,7 @@ public void testFindAll() throws Exception {
313305
.param("deploy", "false"))
314306
.andExpect(status().isCreated());
315307
mockMvc.perform(post("/streams/definitions").param("name", "timelogDoubleTick")
316-
.param("definition", "time --format=\"YYYY MM DD\" | log")
308+
.param("definition", "a: time --format=\"YYYY MM DD\" | log")
317309
.param("deploy", "false")).andExpect(status().isCreated());
318310
mockMvc.perform(post("/streams/definitions/").param("name", "twoPassword")
319311
.param("definition", "time --password='foo'| log --password=bar")
@@ -332,8 +324,8 @@ public void testFindAll() throws Exception {
332324
.perform(get("/streams/definitions/").accept(MediaType.APPLICATION_JSON))
333325
.andReturn().getResponse().getContentAsString();
334326

335-
assertTrue(response.contains("time --password=****** | log"));
336-
assertTrue(response.contains("time --foo=bar| log"));
327+
assertTrue(response.contains("time --password='******' | log"));
328+
assertTrue(response.contains("time --foo=bar | log"));
337329

338330
assertTrue(response.contains(":myStream1.time > log"));
339331
assertTrue(response.contains("time | log"));
@@ -346,19 +338,18 @@ public void testFindAll() throws Exception {
346338
assertTrue(response.contains(":myStream2 > log"));
347339
assertTrue(response.contains(":myStream3 > log"));
348340
assertTrue(response.contains("time --format='YYYY MM DD' | log"));
349-
assertTrue(response.contains("time --format=\\\"YYYY MM DD\\\" | log"));
350-
assertTrue(response.contains("time --password=****** | log --password=******"));
351-
System.out.println(response);
352-
assertTrue(response.contains("time --password=****** > :foobar"));
353-
assertTrue(response.contains("time --password=****** --arg=foo | log"));
354-
assertTrue(response.contains("time --password=****** --arg=bar | log"));
341+
assertTrue(response.contains("a: time --format='YYYY MM DD' | log"));
342+
assertTrue(response.contains("time --password='******' | log --password='******'"));
343+
assertTrue(response.contains("time --password='******' > :foobar"));
344+
assertTrue(response.contains("time --password='******' --arg=foo | log"));
345+
assertTrue(response.contains("time --password='******' --arg=bar | log"));
355346

356347
assertTrue(response.contains("\"totalElements\":15"));
357348

358349
}
359350

360351
@Test
361-
public void testSaveInvalidAppDefintions() throws Exception {
352+
public void testSaveInvalidAppDefinitions() throws Exception {
362353
mockMvc.perform(post("/streams/definitions/").param("name", "myStream").param("definition", "foo | bar")
363354
.accept(MediaType.APPLICATION_JSON)).andDo(print()).andExpect(status().isBadRequest())
364355
.andExpect(jsonPath("$[0].logref", is("InvalidStreamDefinitionException")))
@@ -369,7 +360,7 @@ public void testSaveInvalidAppDefintions() throws Exception {
369360
}
370361

371362
@Test
372-
public void testSaveInvalidAppDefintionsDueToParseException() throws Exception {
363+
public void testSaveInvalidAppDefinitionsDueToParseException() throws Exception {
373364
mockMvc.perform(post("/streams/definitions/").param("name", "myStream")
374365
.param("definition", "foo --.spring.cloud.stream.metrics.properties=spring* | bar")
375366
.accept(MediaType.APPLICATION_JSON)).andDo(print()).andExpect(status().isBadRequest())
@@ -619,7 +610,7 @@ public void testDisplaySingleStreamWithRedaction() throws Exception {
619610
when(appDeployer.status("myStream.log")).thenReturn(status);
620611
mockMvc.perform(get("/streams/definitions/myStream").accept(MediaType.APPLICATION_JSON))
621612
.andExpect(status().isOk()).andExpect(content().json("{name: \"myStream\"}"))
622-
.andExpect(content().json("{dslText: \"time --secret=****** | log\"}"));
613+
.andExpect(content().json("{dslText: \"time --secret='******' | log\"}"));
623614
}
624615

625616
@Test

‎spring-cloud-dataflow-server-core/src/test/java/org/springframework/cloud/dataflow/server/support/ArgumentSanitizerTest.java

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.junit.Before;
2121
import org.junit.Test;
2222

23+
import org.springframework.cloud.dataflow.core.StreamDefinition;
2324
import org.springframework.cloud.dataflow.server.controller.support.ArgumentSanitizer;
2425

2526
/**
@@ -56,18 +57,23 @@ public void testMultipartProperty() {
5657

5758
@Test
5859
public void testHierarchicalPropertyNames() {
59-
Assert.assertEquals("time --password=****** | log", ArgumentSanitizer.sanitizeStream("time --password=bar | log"));
60+
Assert.assertEquals("time --password='******' | log",
61+
sanitizer.sanitizeStream(new StreamDefinition("stream", "time --password=bar | log")));
6062
}
6163

6264
@Test
6365
public void testStreamMatcherWithHyphenDotChar() {
64-
Assert.assertEquals("twitterstream --twitter.credentials.consumer-key=****** --twitter.credentials.consumer-secret=****** "
65-
+ "--twitter.credentials.access-token=****** --twitter.credentials.access-token-secret=****** | filter --expression=#jsonPath(payload,'$.lang')=='en' | "
66-
+ "twitter-sentiment --vocabulary=http://dl.bintray.com/test --model-fetch=output/test --model=http://dl.bintray.com/test | "
67-
+ "field-value-counter --field-name=sentiment --name=sentiment", ArgumentSanitizer.sanitizeStream("twitterstream "
68-
+ "--twitter.credentials.consumer-key=dadadfaf --twitter.credentials.consumer-secret=dadfdasfdads "
69-
+ "--twitter.credentials.access-token=58849055-dfdae --twitter.credentials.access-token-secret=deteegdssa4466 | filter --expression=#jsonPath(payload,'$.lang')=='en' | "
70-
+ "twitter-sentiment --vocabulary=http://dl.bintray.com/test --model-fetch=output/test --model=http://dl.bintray.com/test | "
71-
+ "field-value-counter --field-name=sentiment --name=sentiment"));
66+
Assert.assertEquals("twitterstream --twitter.credentials.access-token-secret='******' "
67+
+ "--twitter.credentials.access-token='******' --twitter.credentials.consumer-secret='******' "
68+
+ "--twitter.credentials.consumer-key='******' | "
69+
+ "filter --expression=#jsonPath(payload,'$.lang')=='en' | "
70+
+ "twitter-sentiment --vocabulary=http://dl.bintray.com/test --model-fetch=output/test "
71+
+ "--model=http://dl.bintray.com/test | field-value-counter --field-name=sentiment --name=sentiment",
72+
sanitizer.sanitizeStream(new StreamDefinition("stream", "twitterstream "
73+
+ "--twitter.credentials.consumer-key=dadadfaf --twitter.credentials.consumer-secret=dadfdasfdads "
74+
+ "--twitter.credentials.access-token=58849055-dfdae "
75+
+ "--twitter.credentials.access-token-secret=deteegdssa4466 | filter --expression=#jsonPath(payload,'$.lang')=='en' | "
76+
+ "twitter-sentiment --vocabulary=http://dl.bintray.com/test --model-fetch=output/test --model=http://dl.bintray.com/test | "
77+
+ "field-value-counter --field-name=sentiment --name=sentiment")));
7278
}
7379
}

0 commit comments

Comments
 (0)