check paths used for shuffle intermediary data manager get and delete by clintropolis · Pull Request #9630 · apache/druid

clintropolis · 2020-04-06T21:49:56Z

Description

This PR fixes an issue spotted by LGTM where strings provided by an HTTP request to the ShuffleResource used by native batch indexing were not checked, allowing paths to be supplied to delete, and to a lesser extent, read files outside of the intended storage directory.

Both of the fix areas are flagged with: https://lgtm.com/rules/5970070/

The added tests would fail prior to this PR and serve to illustrate the bug. I moved logic of DataSchema. validateDatasourceName and RandomIdUtils into druid-core under TaskIdUtils so that this logic could be recycled to ensure that the supervisorTaskId are chill to use.

This PR has:

been self-reviewed.
added unit tests or modified existing tests to cover new code paths.
been tested in a test Druid cluster.

Key changed/added classes in this PR

IntermediaryDataManager
RandomIdUtils -> TaskIdUtils

lgtm-com · 2020-04-06T22:56:58Z

This pull request fixes 2 alerts when merging c009e2e into fc2897d - view on LGTM.com

fixed alerts:

2 for Uncontrolled data used in path expression

…file-path

jihoonson

LGTM

ccaominh · 2020-04-07T02:12:11Z

indexing-service/src/main/java/org/apache/druid/indexing/worker/IntermediaryDataManager.java

  @Nullable
  public File findPartitionFile(String supervisorTaskId, String subTaskId, Interval interval, int partitionId)
  {
+    TaskIdUtils.validateId("supervisorTaskId", supervisorTaskId);


What do you think about moving the validation up the stack to right when the external data enters druid (i.e., in the API resource layer like ShuffleResource.getPartition()/deletePartitions())? For example, in the future, there may be clients of the lower level code that do not need to do the validation.

It seems safer to me to be here where the problem is possible so that any future potential caller wouldn't need to worry about forgetting to do this and avoid the possibility.

lgtm-com · 2020-04-07T11:58:49Z

This pull request fixes 2 alerts when merging d120c28 into 79522f3 - view on LGTM.com

fixed alerts:

2 for Uncontrolled data used in path expression

…apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

…#9630) (#9640) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

…apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

…apache#9630) (#78) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

…apache#9630) (#77) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

…apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

…apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh Conflicts: server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java

…ache#9630)(apache#9666) (#146) * Indexing Service validates externally received taskId (apache#9666) Addresses issues flagged by https://lgtm.com/rules/5970070/ * check paths used for shuffle intermediary data manager get and delete (apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh Conflicts: server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java * Add validation for authenticator and authorizer name (apache#10106) * Add validation for authorizer name * fix deps * add javadocs * Do not use resource filters * Fix BasicAuthenticatorResource as well * Add integration tests * fix test * fix Conflicts: core/src/main/java/org/apache/druid/common/utils/IdUtils.java extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisor.java extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisor.java integration-tests/src/main/java/org/apache/druid/testing/utils/KafkaEventWriter.java integration-tests/src/test/java/org/apache/druid/tests/indexer/AbstractKafkaIndexerTest.java * Add commons-text to server dependencies Co-authored-by: Clint Wylie <cwylie@apache.org>

…apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh Conflicts: server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java

…ache#9630)(apache#9666) (#151) * Indexing Service validates externally received taskId (apache#9666) Addresses issues flagged by https://lgtm.com/rules/5970070/ * check paths used for shuffle intermediary data manager get and delete (apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh Conflicts: server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java * Add validation for authenticator and authorizer name (apache#10106) * Add validation for authorizer name * fix deps * add javadocs * Do not use resource filters * Fix BasicAuthenticatorResource as well * Add integration tests * fix test * fix Conflicts: core/src/main/java/org/apache/druid/common/utils/IdUtils.java core/src/test/java/org/apache/druid/common/utils/IdUtilsTest.java extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaConsumerConfigs.java extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisor.java extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisor.java indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTask.java indexing-service/src/main/java/org/apache/druid/indexing/worker/IntermediaryDataManager.java integration-tests/src/test/java/org/apache/druid/tests/indexer/AbstractKafkaIndexerTest.java server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java * Add commons-text to server dependencies * remove unsupported endpoints from ITBasicAuthConfigurationTest Co-authored-by: Clint Wylie <cwylie@apache.org>

…ache#9630)(apache#9666) (#147) * Indexing Service validates externally received taskId (apache#9666) Addresses issues flagged by https://lgtm.com/rules/5970070/ * check paths used for shuffle intermediary data manager get and delete (apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh Conflicts: server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java * Add validation for authenticator and authorizer name (apache#10106) * Add validation for authorizer name * fix deps * add javadocs * Do not use resource filters * Fix BasicAuthenticatorResource as well * Add integration tests * fix test * fix Conflicts: extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/endpoint/BasicAuthenticatorResource.java extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/endpoint/BasicAuthorizerResource.java extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/CoordinatorBasicAuthenticatorResourceTest.java indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java server/src/test/java/org/apache/druid/segment/indexing/DataSchemaTest.java extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/basic/authorization/endpoint/BasicAuthorizerResourceTest.java indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTask.java * Add commons-text to server dependencies * remove unsupported endpoints from ITBasicAuthConfigurationTest Co-authored-by: Clint Wylie <cwylie@apache.org>

check paths used for shuffle intermediary data manager get and delete

c009e2e

clintropolis added Area - Batch Ingestion Security labels Apr 6, 2020

jihoonson added this to the 0.18.0 milestone Apr 6, 2020

clintropolis added 2 commits April 6, 2020 17:15

Merge remote-tracking branch 'upstream/master' into sanitize-shuffle-…

fc1af65

…file-path

add test

4b014ad

jihoonson approved these changes Apr 7, 2020

View reviewed changes

ccaominh reviewed Apr 7, 2020

View reviewed changes

gianm approved these changes Apr 7, 2020

View reviewed changes

clintropolis added 2 commits April 6, 2020 20:49

newline

6a98cf7

meh

d120c28

jihoonson merged commit d267b1c into apache:master Apr 7, 2020

clintropolis deleted the sanitize-shuffle-file-path branch April 7, 2020 18:52

clintropolis added a commit to clintropolis/druid that referenced this pull request Apr 7, 2020

check paths used for shuffle intermediary data manager get and delete (…

0cca4ea

…apache#9630) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

clintropolis mentioned this pull request Apr 7, 2020

[Backport] check paths used for shuffle intermediary data manager get and delete #9640

Merged

fjy pushed a commit that referenced this pull request Apr 7, 2020

check paths used for shuffle intermediary data manager get and delete (…

b46d06a

…#9630) (#9640) * check paths used for shuffle intermediary data manager get and delete * add test * newline * meh

clintropolis mentioned this pull request Apr 7, 2020

[Backport] check paths used for shuffle intermediary data manager get and delete implydata/druid-public#77

Merged

clintropolis mentioned this pull request Apr 7, 2020

[Backport] check paths used for shuffle intermediary data manager get and delete implydata/druid-public#78

Merged

suneet-s mentioned this pull request Apr 10, 2020

Overlord and IndexingService validates externally received taskId #9666

Merged

suneet-s mentioned this pull request Jul 14, 2020

[0.17.0-iap] User input validation on internal APIs (#10106)(#9630)(#9666) implydata/druid-public#146

Merged

suneet-s mentioned this pull request Jul 14, 2020

[0.16.0-iap] User input validation on internal APIs (#10106)(#9630)(#9666) implydata/druid-public#147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check paths used for shuffle intermediary data manager get and delete#9630

check paths used for shuffle intermediary data manager get and delete#9630
jihoonson merged 5 commits intoapache:masterfrom
clintropolis:sanitize-shuffle-file-path

clintropolis commented Apr 6, 2020 •

edited

Loading

Uh oh!

lgtm-com bot commented Apr 6, 2020

Uh oh!

jihoonson left a comment

Uh oh!

ccaominh Apr 7, 2020

Uh oh!

clintropolis Apr 7, 2020

Uh oh!

lgtm-com bot commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

clintropolis commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

lgtm-com bot commented Apr 6, 2020

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

ccaominh Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

clintropolis Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clintropolis commented Apr 6, 2020 •

edited

Loading