You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/12/14 16:21:34 UTC
[GitHub] [druid] nishantmonu51 opened a new pull request #10677: Add URI based allowPrefexList and denyPrefixList
nishantmonu51 opened a new pull request #10677:
URL: https://github.com/apache/druid/pull/10677
Description
Right now there is no support to restrict druid based ingestion to deny ingestion from specific paths.
This PR adds a new config `druid.ingestion.uri.allowPrefexList` and `druid.ingestion.uri.denyPrefexList` which allows/deny configured URI prefixes from being accessed through druid ingestion.
Default behavior is to allow ingestion from all paths.
This PR has:
[*] been self-reviewed.
[*] added documentation for new or modified features or behaviors.
[*] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-753662705
@gianm : Updated the PR based on your feedback and followed the approach of adding a new method to InputSource `validateAllowDenyPrefixList(InputSourceSecurityConfig)`. Please check
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r554239833
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java
##########
@@ -153,6 +154,9 @@
@JsonIgnore
private final RetryPolicyFactory retryPolicyFactory;
+ @JsonIgnore
+ private final InputSourceSecurityConfig securityConfig;
Review comment:
Why not `InputSourceSecurityConfig.ALLOW_ALL`? The system-wide securityConfig is ignored in DruidInputSource anyway.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java
##########
@@ -1044,17 +1048,21 @@ public IndexIOConfig(
if (firehoseFactory != null && inputFormat != null) {
throw new IAE("Cannot use firehose and inputFormat together. Try using inputSource instead of firehose.");
}
+ if (inputSource != null) {
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
I guess you wanted to validate inputSource in the constructor to fail early. However, this seems possible to cause some issues because it will throw exception on validation failure, which in turn making creating an ioConfig instance failed. For example, suppose you had an ingestion spec stored in the metadata store. Then, for some reason, you wanted to update the security config to forbid some URIs which are in the ingestion spec in the metadata store. The update will break reading ingestion specs from metadata store which happens in the Overlord. It will also unnecessarily perform the duplicate check multiple times during ingestion. I think this should be called somewhere in the code path of ingestion, such as `Task.isReady()`.
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -214,10 +215,23 @@ public boolean needsFormat()
private void cachePathsIfNeeded() throws IOException
{
if (cachedPaths == null) {
- cachedPaths = ImmutableList.copyOf(Preconditions.checkNotNull(getPaths(inputPaths, configuration), "paths"));
+ Collection<Path> paths = Preconditions.checkNotNull(getPaths(inputPaths, configuration), "paths");
+ cachedPaths = ImmutableList.copyOf(paths);
}
}
+ @Override
+ public void validateAllowDenyPrefixList(InputSourceSecurityConfig securityConfig)
+ {
+ try {
+ cachePathsIfNeeded();
Review comment:
I think you won't probably need to cache paths since this will unnecessarily populate the cache in the Overlord.
##########
File path: core/src/main/java/org/apache/druid/data/input/AbstractInputSource.java
##########
@@ -31,13 +31,16 @@
*/
public abstract class AbstractInputSource implements InputSource
{
+ private transient boolean validated = false;
Review comment:
Why `transient`? We don't use `Serializable`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-760395019
@nishantmonu51 sorry to say, but I created a release branch yesterday. As this PR is unlikely backported (it's a feature and we backport only bug fixes since features can have bugs), I'm going to untag 0.21.0 milestone. This PR will be included in the next milestone. Since this PR didn't make it for 0.21.0, I think we cannot get rid of `HttpInputSourceConfig` immediately but keep it as deprecated for a while. I will not call out `HttpInputSourceConfig` in the release notes to discourage people from using it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson edited a comment on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson edited a comment on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-760395019
@nishantmonu51 sorry to say, but I created a release branch yesterday. As this PR is unlikely backported (it's a feature and we backport only bug fixes since features can have bugs), I'm going to untag 0.21.0 milestone. This PR will be included in the next release instead. Since this PR didn't make it for 0.21.0, I think we cannot get rid of `HttpInputSourceConfig` immediately but keep it as deprecated for a while. I will not call out `HttpInputSourceConfig` in the release notes to discourage people from using it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r556771796
##########
File path: extensions-core/azure-extensions/src/test/java/org/apache/druid/data/input/azure/AzureInputSourceTest.java
##########
@@ -214,6 +216,78 @@ public void test_toString_returnsExpectedString()
Assert.assertEquals("AzureInputSource{uris=[], prefixes=[azure://container/blob], objects=[]}", actualToString);
}
+ @Test(expected = IllegalArgumentException.class)
+ public void test_Deny_All()
Review comment:
Is this the same as the `testDenyAll()` test below?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamSamplerSpec.java
##########
@@ -102,7 +106,7 @@ public SamplerResponse sample()
"[spec.ioConfig.inputFormat] is required"
);
}
-
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
nit: this doesn't look harm because neither `RecordSupplierInputSource` nor `FirehoseFactoryToInputSourceAdaptor` do anything in `validateAllowDenyPrefixList()`. However, it looks confusing since `validateAllowDenyPrefixList()` is not called in `RecordSupplier`. I would suggest to delete this line and add it back when we truly support this security config for stream ingestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r551042745
##########
File path: docs/configuration/index.md
##########
@@ -1358,8 +1358,8 @@ You can optionally configure following additional configs to restrict druid inge
|Property|Possible Values|Description|Default|
|--------|---------------|-----------|-------|
-|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
Review comment:
yes, they were never released and just committed to master.
I removed them since having a generic configs that apply would to all inputSources would be easier to configure instead of having source specific configs. .
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
Review comment:
done.
##########
File path: docs/configuration/index.md
##########
@@ -1358,8 +1358,8 @@ You can optionally configure following additional configs to restrict druid inge
|Property|Possible Values|Description|Default|
|--------|---------------|-----------|-------|
-|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
-|`druid.ingestion.http.denyListDomains`|List of domains|Blacklisted domains from which ingestion will NOT be allowed. Only one of allowList or denyList can be set. |empty list|
+|`druid.ingestion.uri.allowPrefixList`|List of URIs|Allowed uris from which ingestion will be allowed. Only one of allowPrefixList or denyPrefixList can be set.|empty list|
Review comment:
added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-757043116
@nishantmonu51 BTW, since we are about to cut a branch for 0.21.0 release (scheduled on the next Monday), do you think we can get this PR merged before the branch cut? I'm asking because this change will introduce an incompatible change after the release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-759700496
@nishantmonu51 I'm going to cut the branch for 0.21.0 by tonight as we are already behind the schedule (it was originally scheduled on the last Monday). I'm not sure whether we can make it or not because of the test failures. In this case, I would rather not rush but schedule this PR for a future release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r554239833
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java
##########
@@ -153,6 +154,9 @@
@JsonIgnore
private final RetryPolicyFactory retryPolicyFactory;
+ @JsonIgnore
+ private final InputSourceSecurityConfig securityConfig;
Review comment:
Why not `InputSourceSecurityConfig.ALLOW_ALL`? The system-wide securityConfig is ignored in DruidInputSource anyway.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java
##########
@@ -1044,17 +1048,21 @@ public IndexIOConfig(
if (firehoseFactory != null && inputFormat != null) {
throw new IAE("Cannot use firehose and inputFormat together. Try using inputSource instead of firehose.");
}
+ if (inputSource != null) {
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
I guess you wanted to validate inputSource in the constructor to fail early. However, this seems possible to cause some issues because it will throw exception on validation failure, which in turn making creating an ioConfig instance failed. For example, suppose you had an ingestion spec stored in the metadata store. Then, for some reason, you wanted to update the security config to forbid some URIs which are in the ingestion spec in the metadata store. The update will break reading ingestion specs from metadata store which happens in the Overlord. It will also unnecessarily perform the duplicate check multiple times during ingestion. I think this should be called somewhere in the code path of ingestion, such as `Task.isReady()`.
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -214,10 +215,23 @@ public boolean needsFormat()
private void cachePathsIfNeeded() throws IOException
{
if (cachedPaths == null) {
- cachedPaths = ImmutableList.copyOf(Preconditions.checkNotNull(getPaths(inputPaths, configuration), "paths"));
+ Collection<Path> paths = Preconditions.checkNotNull(getPaths(inputPaths, configuration), "paths");
+ cachedPaths = ImmutableList.copyOf(paths);
}
}
+ @Override
+ public void validateAllowDenyPrefixList(InputSourceSecurityConfig securityConfig)
+ {
+ try {
+ cachePathsIfNeeded();
Review comment:
I think you won't probably need to cache paths since this will unnecessarily populate the cache in the Overlord.
##########
File path: core/src/main/java/org/apache/druid/data/input/AbstractInputSource.java
##########
@@ -31,13 +31,16 @@
*/
public abstract class AbstractInputSource implements InputSource
{
+ private transient boolean validated = false;
Review comment:
Why `transient`? We don't use `Serializable`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r543549478
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/LocalInputSource.java
##########
@@ -72,15 +75,17 @@ public LocalInputSource(
this.baseDir = baseDir;
this.filter = baseDir != null ? Preconditions.checkNotNull(filter, "filter") : filter;
this.files = files == null ? Collections.emptySet() : files;
-
if (baseDir == null && CollectionUtils.isNullOrEmpty(files)) {
throw new IAE("At least one of baseDir or files should be specified");
}
+ securityConfig.validateFileAccess(baseDir);
+ securityConfig.validateFileAccess(files);
Review comment:
Maybe out of this PR's scope but it could be useful to support pattern based validation for `filter`.
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
+ @JsonProperty
+ private final List<URI> allowPrefexList;
+ @JsonProperty
+ private final List<URI> denyPrefixList;
+
+ @JsonCreator
+ public InputSourceSecurityConfig(
+ @JsonProperty("allowPrefexList") @Nullable List<URI> allowPrefexList,
Review comment:
Could you please fix typo: allowPrefixList in all occurrences including user docs?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-966447063
@jihoonson : I will resolve the conflicts and update the PR this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-966008789
Hi @nishantmonu51, any updates on this PR? I believe this will be useful from security perspective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r556551833
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java
##########
@@ -232,6 +237,10 @@ public boolean isReady(TaskActionClient taskActionClient) throws Exception
throw new UOE("partitionsSpec[%s] is not supported", tuningConfig.getPartitionsSpec().getClass().getName());
}
}
+ InputSource inputSource = getIngestionSchema().getIOConfig().getNonNullInputSource(
+ getIngestionSchema().getDataSchema().getParser()
+ );
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r555825958
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java
##########
@@ -1044,17 +1048,21 @@ public IndexIOConfig(
if (firehoseFactory != null && inputFormat != null) {
throw new IAE("Cannot use firehose and inputFormat together. Try using inputSource instead of firehose.");
}
+ if (inputSource != null) {
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
isReady is also called multiple times, so not sure if that is any better in terms of number of checks,
Just to double check, if i move it to isReady the task will be accepted by the overlord and it will fail when the task is actually tried to be run. I was thinking that if someone is not allowed, we should block the ingestion at that time when the task is submitted at first place. Is that the behavior you are expecting ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r556796308
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamSamplerSpec.java
##########
@@ -102,7 +106,7 @@ public SamplerResponse sample()
"[spec.ioConfig.inputFormat] is required"
);
}
-
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r555809503
##########
File path: extensions-core/hdfs-storage/src/main/java/org/apache/druid/inputsource/hdfs/HdfsInputSource.java
##########
@@ -214,10 +215,23 @@ public boolean needsFormat()
private void cachePathsIfNeeded() throws IOException
{
if (cachedPaths == null) {
- cachedPaths = ImmutableList.copyOf(Preconditions.checkNotNull(getPaths(inputPaths, configuration), "paths"));
+ Collection<Path> paths = Preconditions.checkNotNull(getPaths(inputPaths, configuration), "paths");
+ cachedPaths = ImmutableList.copyOf(paths);
}
}
+ @Override
+ public void validateAllowDenyPrefixList(InputSourceSecurityConfig securityConfig)
+ {
+ try {
+ cachePathsIfNeeded();
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r556796195
##########
File path: extensions-core/azure-extensions/src/test/java/org/apache/druid/data/input/azure/AzureInputSourceTest.java
##########
@@ -214,6 +216,78 @@ public void test_toString_returnsExpectedString()
Assert.assertEquals("AzureInputSource{uris=[], prefixes=[azure://container/blob], objects=[]}", actualToString);
}
+ @Test(expected = IllegalArgumentException.class)
+ public void test_Deny_All()
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-993630835
merged master and resolved conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-759523613
@jihoonson handled review comments, waiting on the CI now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r544148377
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
+ @JsonProperty
+ private final List<URI> allowPrefexList;
+ @JsonProperty
+ private final List<URI> denyPrefixList;
+
+ @JsonCreator
+ public InputSourceSecurityConfig(
+ @JsonProperty("allowPrefexList") @Nullable List<URI> allowPrefexList,
Review comment:
done, test is already there but it also had the typo since i copy pasted it, fixed now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-758695886
@jihoonson thanks for the review, I hv updated the PR, yes, it will be nice to get this in 0.21.0 release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-757043116
@nishantmonu51 BTW, since we are about to cut a branch for 0.21.0 release (scheduled on the next Monday), do you think we can get this PR merged before the branch cut? I'm asking because this change will introduce an incompatible change after the release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r546172186
##########
File path: docs/configuration/index.md
##########
@@ -1358,8 +1358,8 @@ You can optionally configure following additional configs to restrict druid inge
|Property|Possible Values|Description|Default|
|--------|---------------|-----------|-------|
-|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
-|`druid.ingestion.http.denyListDomains`|List of domains|Blacklisted domains from which ingestion will NOT be allowed. Only one of allowList or denyList can be set. |empty list|
+|`druid.ingestion.uri.allowPrefixList`|List of URIs|Allowed uris from which ingestion will be allowed. Only one of allowPrefixList or denyPrefixList can be set.|empty list|
Review comment:
I think we should list out which input sources this applies to, and make sure each one has a test.
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
Review comment:
`final`?
##########
File path: docs/configuration/index.md
##########
@@ -1358,8 +1358,8 @@ You can optionally configure following additional configs to restrict druid inge
|Property|Possible Values|Description|Default|
|--------|---------------|-----------|-------|
-|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
Review comment:
Why remove the domain flavors?
Also, did we ever ship them? If so, we'll need to handle that somehow in the docs or the code itself.
##########
File path: docs/configuration/index.md
##########
@@ -1358,8 +1358,8 @@ You can optionally configure following additional configs to restrict druid inge
|Property|Possible Values|Description|Default|
|--------|---------------|-----------|-------|
-|`druid.ingestion.http.allowListDomains`|List of domains|Allowed domains from which ingestion will be allowed. Only one of allowList or denyList can be set.|empty list|
-|`druid.ingestion.http.denyListDomains`|List of domains|Blacklisted domains from which ingestion will NOT be allowed. Only one of allowList or denyList can be set. |empty list|
+|`druid.ingestion.uri.allowPrefixList`|List of URIs|Allowed uris from which ingestion will be allowed. Only one of allowPrefixList or denyPrefixList can be set.|empty list|
Review comment:
I'd also add some text to the docs explaining two points:
- How does the prefix checking work? (Does it happen on path boundaries? Like will a prefix of `http://example.com/f` match `http://example.com/foo` or `http://example.com/f/b` or both?)
- Pointing out that using "allow" is a much safer method than "deny"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r543744946
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
+ @JsonProperty
+ private final List<URI> allowPrefexList;
+ @JsonProperty
+ private final List<URI> denyPrefixList;
+
+ @JsonCreator
+ public InputSourceSecurityConfig(
+ @JsonProperty("allowPrefexList") @Nullable List<URI> allowPrefexList,
Review comment:
+1 , maybe introduce a serde test that would catch this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r555808142
##########
File path: core/src/main/java/org/apache/druid/data/input/AbstractInputSource.java
##########
@@ -31,13 +31,16 @@
*/
public abstract class AbstractInputSource implements InputSource
{
+ private transient boolean validated = false;
Review comment:
equals tests on base classes were failing without making it transient as they were not including this field in the comparison
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r556275449
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java
##########
@@ -1044,17 +1048,21 @@ public IndexIOConfig(
if (firehoseFactory != null && inputFormat != null) {
throw new IAE("Cannot use firehose and inputFormat together. Try using inputSource instead of firehose.");
}
+ if (inputSource != null) {
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
> Just to double check, if i move it to isReady the task will be accepted by the overlord and it will fail when the task is actually tried to be run. I was thinking that if someone is not allowed, we should block the ingestion at that time when the task is submitted at first place. Is that the behavior you are expecting ?
@nishantmonu51 `Task.isReady()` is actually called as soon as it is issued to the Overlord. When it returns true, the Overlord changes its state from `waiting` to `pending`, and tries to assign it to one of middleManagers. So, I think it will be effectively the same as what you want to achieve.
> isReady is also called multiple times, so not sure if that is any better in terms of number of checks,
Good point. I think it won't be not that expensive, but maybe we can check the `validated` state and avoid further checking if it is already validated?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r556280161
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTask.java
##########
@@ -232,6 +237,10 @@ public boolean isReady(TaskActionClient taskActionClient) throws Exception
throw new UOE("partitionsSpec[%s] is not supported", tuningConfig.getPartitionsSpec().getClass().getName());
}
}
+ InputSource inputSource = getIngestionSchema().getIOConfig().getNonNullInputSource(
+ getIngestionSchema().getDataSchema().getParser()
+ );
+ inputSource.validateAllowDenyPrefixList(securityConfig);
Review comment:
This should be done in `ParallelIndexSupervisorTask.isReady()` as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r546613641
##########
File path: core/src/test/java/org/apache/druid/data/input/impl/InputSourceSecurityConfigTest.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.druid.data.input.impl;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class InputSourceSecurityConfigTest
+{
+ @Test
+ public void testEmptyAllowList()
+ {
+ InputSourceSecurityConfig config = new InputSourceSecurityConfig(Collections.emptyList(), null);
+ Assert.assertFalse(config.isURIAllowed(URI.create("gs://my-bucket/abc")));
+ Assert.assertFalse(config.isURIAllowed(URI.create("s3://my-bucket/abc")));
+ }
+
+ @Test
+ public void testNonSpecifiedAllowDenyList()
+ {
+ InputSourceSecurityConfig config = new InputSourceSecurityConfig(null, null);
+ Assert.assertTrue(config.isURIAllowed(URI.create("gs://my-bucket/abc")));
+ Assert.assertTrue(config.isURIAllowed(URI.create("s3://my-bucket/abc")));
+ }
+
+ @Test
+ public void testAllowList()
Review comment:
I think there should be similar tests but with more than one item on the list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r770233437
##########
File path: docs/configuration/index.md
##########
@@ -1525,6 +1525,19 @@ The amount of direct memory needed by Druid is at least
ensure at least this amount of direct memory is available by providing `-XX:MaxDirectMemorySize=<VALUE>` at the command
line.
+#### Indexer Security Configuration
Review comment:
I find this section confusing because the `Indexer` here means the node type `Indexer not ingestion. Can you move the new docs to [Ingestion Security Configuration](https://github.com/apache/druid/blob/master/docs/configuration/index.md#ingestion-security-configuration)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-966479433
@nishantmonu51 thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r546612421
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
+ @JsonProperty
+ private final List<URI> allowPrefixList;
+ @JsonProperty
+ private final List<URI> denyPrefixList;
+
+ @JsonCreator
+ public InputSourceSecurityConfig(
+ @JsonProperty("allowPrefixList") @Nullable List<URI> allowPrefixList,
+ @JsonProperty("denyPrefixList") @Nullable List<URI> denyPrefixList
+ )
+ {
+ this.allowPrefixList = allowPrefixList;
+ this.denyPrefixList = denyPrefixList;
+ Preconditions.checkArgument(
+ this.denyPrefixList == null || this.allowPrefixList == null,
+ "Can only use one of allowList or blackList"
+ );
+ }
+
+ public List<URI> getallowPrefixList()
+ {
+ return allowPrefixList;
+ }
+
+ public List<URI> getdenyPrefixList()
+ {
+ return denyPrefixList;
+ }
+
+ private static boolean matchesAny(List<URI> domains, URI uri)
+ {
+ URI toMatch = uri.normalize();
+ for (URI prefix : domains) {
+ return toMatch.toString().startsWith(prefix.toString());
Review comment:
should return only if there is a match.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r551042834
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/InputSourceSecurityConfig.java
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.druid.data.input.impl;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.StringUtils;
+
+import javax.annotation.Nullable;
+import java.io.File;
+import java.net.URI;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+public class InputSourceSecurityConfig
+{
+
+ public static InputSourceSecurityConfig ALLOW_ALL = new InputSourceSecurityConfig(null, null);
+ @JsonProperty
+ private final List<URI> allowPrefixList;
+ @JsonProperty
+ private final List<URI> denyPrefixList;
+
+ @JsonCreator
+ public InputSourceSecurityConfig(
+ @JsonProperty("allowPrefixList") @Nullable List<URI> allowPrefixList,
+ @JsonProperty("denyPrefixList") @Nullable List<URI> denyPrefixList
+ )
+ {
+ this.allowPrefixList = allowPrefixList;
+ this.denyPrefixList = denyPrefixList;
+ Preconditions.checkArgument(
+ this.denyPrefixList == null || this.allowPrefixList == null,
+ "Can only use one of allowList or blackList"
+ );
+ }
+
+ public List<URI> getallowPrefixList()
+ {
+ return allowPrefixList;
+ }
+
+ public List<URI> getdenyPrefixList()
+ {
+ return denyPrefixList;
+ }
+
+ private static boolean matchesAny(List<URI> domains, URI uri)
+ {
+ URI toMatch = uri.normalize();
+ for (URI prefix : domains) {
+ return toMatch.toString().startsWith(prefix.toString());
Review comment:
fixed. thanks for catching it up. Added a test as well.
##########
File path: core/src/test/java/org/apache/druid/data/input/impl/InputSourceSecurityConfigTest.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.druid.data.input.impl;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class InputSourceSecurityConfigTest
+{
+ @Test
+ public void testEmptyAllowList()
+ {
+ InputSourceSecurityConfig config = new InputSourceSecurityConfig(Collections.emptyList(), null);
+ Assert.assertFalse(config.isURIAllowed(URI.create("gs://my-bucket/abc")));
+ Assert.assertFalse(config.isURIAllowed(URI.create("s3://my-bucket/abc")));
+ }
+
+ @Test
+ public void testNonSpecifiedAllowDenyList()
+ {
+ InputSourceSecurityConfig config = new InputSourceSecurityConfig(null, null);
+ Assert.assertTrue(config.isURIAllowed(URI.create("gs://my-bucket/abc")));
+ Assert.assertTrue(config.isURIAllowed(URI.create("s3://my-bucket/abc")));
+ }
+
+ @Test
+ public void testAllowList()
Review comment:
added test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nishantmonu51 commented on a change in pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10677:
URL: https://github.com/apache/druid/pull/10677#discussion_r555815427
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java
##########
@@ -153,6 +154,9 @@
@JsonIgnore
private final RetryPolicyFactory retryPolicyFactory;
+ @JsonIgnore
+ private final InputSourceSecurityConfig securityConfig;
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-995452665
> [ERROR] org.apache.druid.inputsource.hdfs.HdfsInputSourceTest$ReaderTest.testAllowPath(org.apache.druid.inputsource.hdfs.HdfsInputSourceTest$ReaderTest)
> [ERROR] Run 1: HdfsInputSourceTest$ReaderTest.testAllowPath:401 » NullPointer inputSourceConf...
> [ERROR] Run 2: HdfsInputSourceTest$ReaderTest.testAllowPath:401 » NullPointer inputSourceConf...
> [ERROR] Run 3: HdfsInputSourceTest$ReaderTest.testAllowPath:401 » NullPointer inputSourceConf...
> [ERROR] Run 4: HdfsInputSourceTest$ReaderTest.testAllowPath:401 » NullPointer inputSourceConf...
> [ERROR] org.apache.druid.metadata.input.InputSourceModuleTest.testHttpInputSourceDefaultConfig(org.apache.druid.metadata.input.InputSourceModuleTest)
> [ERROR] Run 1: InputSourceModuleTest.testHttpInputSourceDefaultConfig:121 » Configuration Gui...
> [ERROR] Run 2: InputSourceModuleTest.testHttpInputSourceDefaultConfig:121 » Configuration Gui...
> [ERROR] Run 3: InputSourceModuleTest.testHttpInputSourceDefaultConfig:121 » Configuration Gui...
> [ERROR] Run 4: InputSourceModuleTest.testHttpInputSourceDefaultConfig:121 » Configuration Gui...
Oh, I missed the CI failure. Those failed jobs seem legit as seen above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on pull request #10677: Add URI based allowPrefexList and denyPrefixList
Posted by GitBox <gi...@apache.org>.
himanshug commented on pull request #10677:
URL: https://github.com/apache/druid/pull/10677#issuecomment-745616949
LGTM overall
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org