gianm commented on code in PR #17774:
URL: https://github.com/apache/druid/pull/17774#discussion_r2027868639
##########
processing/src/test/java/org/apache/druid/query/DataSourceTest.java:
##########
@@ -221,4 +225,53 @@ public void
testMapWithRestriction_onRestrictedDataSource_alwaysThrows()
ISE e3 = Assert.assertThrows(ISE.class, () ->
restrictedDataSource.withPolicies(policyWasNotChecked));
Assert.assertEquals("Missing policy check result for table [table1]",
e3.getMessage());
}
+
+ @Test
+ public void testValidate_onInlineDataSource()
+ {
+ InlineDataSource inlineDataSource =
InlineDataSource.fromIterable(ImmutableList.of(), RowSignature.empty());
+
Assert.assertTrue(inlineDataSource.validate(NoopPolicyEnforcer.instance()));
+ Assert.assertTrue(inlineDataSource.validate(new
RestrictAllTablesPolicyEnforcer(null)));
+ }
+
+ @Test
+ public void testValidate_onRestrictedDataSource()
+ {
+ RestrictedDataSource restrictedDataSource = RestrictedDataSource.create(
+ TableDataSource.create("table1"),
+ RowFilterPolicy.from(new NullFilter("random-column", null))
+ );
+
Assert.assertTrue(restrictedDataSource.validate(NoopPolicyEnforcer.instance()));
+ Assert.assertTrue(restrictedDataSource.validate(new
RestrictAllTablesPolicyEnforcer(null)));
+ // Fail, only NoRestrictionPolicy is allowed.
+ Assert.assertFalse(restrictedDataSource.validate(new
RestrictAllTablesPolicyEnforcer(ImmutableList.of(
+ "NoRestrictionPolicy"))));
+ }
+
+ @Test
+ public void testValidate_onTableDataSource()
+ {
+ TableDataSource tableDataSource = TableDataSource.create("table1");
+ Assert.assertTrue(tableDataSource.validate(NoopPolicyEnforcer.instance()));
+ // Fail, policy must exist on table
+ Assert.assertFalse(tableDataSource.validate(new
RestrictAllTablesPolicyEnforcer(null)));
+ }
+
+ @Test
+ public void testValidate_onUnionDataSource()
Review Comment:
Please include a version of this where there is a `union` and both children
are `restricted`. In this case would `RestrictAllTablesPolicyEnforcer` be able
to pass?
Please also include one with a `join`.
##########
integration-tests/docker/environment-configs/common:
##########
@@ -39,6 +39,7 @@ druid_auth_authenticatorChain=["basic"]
druid_auth_authorizer_basic_type=basic
druid_auth_authorizers=["basic"]
druid_auth_authorizeQueryContextParams=true
+druid_policy_enforcer_type=none
Review Comment:
Let's not include this here, same reason as we shouldn't include in the
default configs.
##########
processing/src/main/java/org/apache/druid/query/policy/PolicyEnforcer.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.query.policy;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.UnstableApi;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.segment.Segment;
+
+/**
+ * A mechanism for enforcing policies on data sources in Druid queries.
+ * <p>
+ * Note: The {@code PolicyEnforcer} is intended to serve as a sanity checker
and not as a primary authorization mechanism.
+ * It should not be used to implement security rules. Instead, it acts as a
last line of defense to verify that
+ * security policies have been implemented correctly and to prevent incorrect
policy usage.
+ * </p>
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes({
+ @JsonSubTypes.Type(value = NoopPolicyEnforcer.class, name = "none"),
+ @JsonSubTypes.Type(value = RestrictAllTablesPolicyEnforcer.class, name =
"restrictAllTables"),
+})
+@UnstableApi
+public interface PolicyEnforcer
+{
+
+ /**
+ * Validate a {@link DataSource} node against the policy enforcer. This
method is called during query planning from
+ * {@link DataSource#validate(PolicyEnforcer)}.
+ * <p>
+ * Don't call this method directly, use {@link
DataSource#validate(PolicyEnforcer)}.
+ *
+ * @param ds data source to validate
+ * @return true if the data source is valid according to the policy
enforcer, false otherwise
+ */
+ boolean validate(DataSource ds, Policy policy);
+
+
+ /**
+ * Validate a {@link Segment} node against the policy enforcer. This method
is called during query planning from
+ * {@link Segment#validate(PolicyEnforcer)}.
+ * <p>
+ * Don't call this method directly, use {@link
Segment#validate(PolicyEnforcer)}.
+ *
+ * @param segment segment to validate
+ * @return true if the segment is valid according to the policy enforcer,
false otherwise
+ */
+ boolean validate(Segment segment, Policy policy);
Review Comment:
I believe a call site for this is missing on realtime tasks. There should be
something in `SinkQuerySegmentWalker`. Please add a test that covers that too.
##########
processing/src/main/java/org/apache/druid/query/policy/PolicyEnforcer.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.query.policy;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.UnstableApi;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.segment.Segment;
+
+/**
+ * A mechanism for enforcing policies on data sources in Druid queries.
+ * <p>
+ * Note: The {@code PolicyEnforcer} is intended to serve as a sanity checker
and not as a primary authorization mechanism.
+ * It should not be used to implement security rules. Instead, it acts as a
last line of defense to verify that
+ * security policies have been implemented correctly and to prevent incorrect
policy usage.
+ * </p>
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes({
+ @JsonSubTypes.Type(value = NoopPolicyEnforcer.class, name = "none"),
+ @JsonSubTypes.Type(value = RestrictAllTablesPolicyEnforcer.class, name =
"restrictAllTables"),
+})
+@UnstableApi
+public interface PolicyEnforcer
+{
+
+ /**
+ * Validate a {@link DataSource} node against the policy enforcer. This
method is called during query planning from
+ * {@link DataSource#validate(PolicyEnforcer)}.
+ * <p>
+ * Don't call this method directly, use {@link
DataSource#validate(PolicyEnforcer)}.
+ *
+ * @param ds data source to validate
+ * @return true if the data source is valid according to the policy
enforcer, false otherwise
+ */
+ boolean validate(DataSource ds, Policy policy);
Review Comment:
I see a few call sites missing for this:
- it isn't being called on realtime tasks (equivalent of `ServerManager`
there is `SinkQuerySegmentWalker`)
- it isn't being called for any MSQ paths-- for this I think it would work
to call it in `BaseLeafFrameProcessorFactory#makeProcessors` right before
creating the `segmentMapFnProcessor`. Actually, you could also move
`validate(Segment)` into the map fn itself, which would simplify enforcement.
All the enforcement logic could be in the `BaseLeafFrameProcessorFactory` class.
Please add tests that cover these.
##########
processing/src/test/java/org/apache/druid/query/policy/RestrictAllTablesPolicyEnforcerTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.query.policy;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.query.RestrictedDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.filter.NullFilter;
+import org.apache.druid.segment.TestHelper;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class RestrictAllTablesPolicyEnforcerTest
+{
+ @Test
+ public void test_serialize() throws Exception
+ {
+ RestrictAllTablesPolicyEnforcer policyEnforcer = new
RestrictAllTablesPolicyEnforcer(null);
+ ObjectMapper jsonMapper = TestHelper.makeJsonMapper();
+
+ String expected =
"{\"type\":\"restrictAllTables\",\"allowedPolicies\":[]}";
+ Assert.assertEquals(expected,
jsonMapper.writeValueAsString(policyEnforcer));
+ }
+
+ @Test
+ public void test_serde_roundTrip() throws Exception
+ {
+ RestrictAllTablesPolicyEnforcer policyEnforcer = new
RestrictAllTablesPolicyEnforcer(null);
+ ObjectMapper jsonMapper = TestHelper.makeJsonMapper();
+ PolicyEnforcer deserialized = jsonMapper.readValue(
+ jsonMapper.writeValueAsString(policyEnforcer),
+ PolicyEnforcer.class
+ );
+ Assert.assertEquals(policyEnforcer, deserialized);
+ }
+
+ @Test
+ public void test_validate() throws Exception
+ {
+ RestrictAllTablesPolicyEnforcer policyEnforcer = new
RestrictAllTablesPolicyEnforcer(null);
+ RowFilterPolicy policy = RowFilterPolicy.from(new NullFilter("some-col",
null));
+ TableDataSource table = TableDataSource.create("table");
+ RestrictedDataSource restricted = RestrictedDataSource.create(table,
policy);
+
+ Assert.assertFalse(policyEnforcer.validate(table));
+ Assert.assertTrue(policyEnforcer.validate(restricted));
Review Comment:
ok, TY
##########
server/src/main/java/org/apache/druid/guice/security/PolicyModule.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.guice.security;
+
+import com.google.inject.Binder;
+import org.apache.druid.guice.JsonConfigProvider;
+import org.apache.druid.initialization.DruidModule;
+import org.apache.druid.query.policy.PolicyEnforcer;
+
+/**
+ * Module for configuring the policy enforcer.
+ */
+public class PolicyModule implements DruidModule
+{
+ @Override
+ public void configure(Binder binder)
+ {
+ JsonConfigProvider.bind(binder, "druid.policy.enforcer",
PolicyEnforcer.class);
Review Comment:
Ah, maybe you had to include the config in the default files because there
is no default here. This is the right place to put a default, you can do it
with `JsonConfigProvider.bindWithDefault`. It doesn't have the same problem as
Jackson's `defaultImpl`: with the default here, if the type is misspelled then
the user will get an error rather than silently using the default impl.
##########
processing/src/main/java/org/apache/druid/query/policy/RestrictAllTablesPolicyEnforcer.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.query.policy;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.RestrictedDataSource;
+import org.reflections.Reflections;
+
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * All tables must be restricted by a policy.
+ */
+public class RestrictAllTablesPolicyEnforcer implements PolicyEnforcer
+{
+ private final ImmutableList<String> allowedPolicies;
+
+ @JsonCreator
+ public RestrictAllTablesPolicyEnforcer(
+ @Nullable @JsonProperty("allowedPolicies") List<String> allowedPolicies
+ )
+ {
+ if (allowedPolicies == null) {
+ this.allowedPolicies = ImmutableList.of();
+ } else {
+ Set<String> policyClazz = new
Reflections("org.apache.druid.query.policy").getSubTypesOf(
Review Comment:
Extensions can't make code changes here, though, because the code here is
core. Extensions could fork it and create a new enforcer, but then it wouldn't
be the same enforcer. So as it is, this code isn't really useful, because it
only works for core policies, and there's only one kind of policy in core
anyway. IMO, it's better to remove this so any class can be provided here.
##########
processing/src/main/java/org/apache/druid/query/policy/PolicyEnforcer.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.query.policy;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.UnstableApi;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.segment.Segment;
+
+/**
+ * A mechanism for enforcing policies on data sources in Druid queries.
+ * <p>
+ * Note: The {@code PolicyEnforcer} is intended to serve as a sanity checker
and not as a primary authorization mechanism.
+ * It should not be used to implement security rules. Instead, it acts as a
last line of defense to verify that
+ * security policies have been implemented correctly and to prevent incorrect
policy usage.
+ * </p>
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes({
+ @JsonSubTypes.Type(value = NoopPolicyEnforcer.class, name = "none"),
+ @JsonSubTypes.Type(value = RestrictAllTablesPolicyEnforcer.class, name =
"restrictAllTables"),
+})
+@UnstableApi
+public interface PolicyEnforcer
+{
+
+ /**
+ * Validate a {@link DataSource} node against the policy enforcer. This
method is called during query planning from
+ * {@link DataSource#validate(PolicyEnforcer)}.
+ * <p>
+ * Don't call this method directly, use {@link
DataSource#validate(PolicyEnforcer)}.
+ *
+ * @param ds data source to validate
+ * @return true if the data source is valid according to the policy
enforcer, false otherwise
+ */
+ boolean validate(DataSource ds, Policy policy);
+
+
+ /**
+ * Validate a {@link Segment} node against the policy enforcer. This method
is called during query planning from
Review Comment:
Similar thought here, the javadoc should specify how it's called and what
the `segment` argument will be.
##########
processing/src/main/java/org/apache/druid/query/policy/PolicyEnforcer.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.query.policy;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.guice.annotations.UnstableApi;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.segment.Segment;
+
+/**
+ * A mechanism for enforcing policies on data sources in Druid queries.
+ * <p>
+ * Note: The {@code PolicyEnforcer} is intended to serve as a sanity checker
and not as a primary authorization mechanism.
+ * It should not be used to implement security rules. Instead, it acts as a
last line of defense to verify that
+ * security policies have been implemented correctly and to prevent incorrect
policy usage.
+ * </p>
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes({
+ @JsonSubTypes.Type(value = NoopPolicyEnforcer.class, name = "none"),
+ @JsonSubTypes.Type(value = RestrictAllTablesPolicyEnforcer.class, name =
"restrictAllTables"),
+})
+@UnstableApi
+public interface PolicyEnforcer
+{
+
+ /**
+ * Validate a {@link DataSource} node against the policy enforcer. This
method is called during query planning from
+ * {@link DataSource#validate(PolicyEnforcer)}.
Review Comment:
The javadoc here should specify how it's called and what the `ds` argument
will be. I think with the new structure, where it is called indirectly through
`DataSource` itself, that means this `validate` method will get called once for
each `restricted` and once for each `table` that is not wrapped under a
`restricted`.
##########
examples/conf/druid/auto/_common/common.runtime.properties:
##########
@@ -151,3 +151,8 @@ druid.lookup.enableLookupSyncOnStartup=false
# Expression processing config
#
druid.expressions.useStrictBooleans=true
+
+#
+# Row security policies
+#
+druid.policy.enforcer.type=none
Review Comment:
Let's not include this in the default configs. It won't be a commonly-set
setting, and we generally just want to put the common ones here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
