gianm commented on code in PR #17564: URL: https://github.com/apache/druid/pull/17564#discussion_r1909251825
########## processing/src/test/java/org/apache/druid/query/policy/RowFilterPolicyTest.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.google.common.collect.ImmutableList; +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.EqualityFilter; +import org.apache.druid.query.filter.Filter; +import org.apache.druid.segment.CursorBuildSpec; +import org.apache.druid.segment.column.ColumnType; +import org.apache.druid.segment.filter.AndFilter; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class RowFilterPolicyTest Review Comment: Also include a serde test (round trip through JSON) and equals test (using `EqualsVerifier`). ########## processing/src/main/java/org/apache/druid/query/policy/Policy.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.segment.CursorBuildSpec; + +/** + * Extensible interface for a granular-level (e.x. row filter) restriction on read-table access. Implementations must be + * Jackson-serializable. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") +@JsonSubTypes({ + @JsonSubTypes.Type(value = RowFilterPolicy.class, name = "row"), + @JsonSubTypes.Type(value = NoRestrictionPolicy.class, name = "noRestriction") +}) +public interface Policy Review Comment: Please add `@UnstableApi` to this interface to emphasize that it might change as we evolve it. ########## sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java: ########## @@ -2184,7 +2184,7 @@ private TestHttpStatement( @Override protected void authorize( DruidPlanner planner, - Function<Set<ResourceAction>, Access> authorizer Review Comment: Please add some tests that use Policies to this class. ########## processing/src/main/java/org/apache/druid/query/policy/RowFilterPolicy.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.collect.ImmutableList; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.Filter; +import org.apache.druid.segment.CursorBuildSpec; +import org.apache.druid.segment.filter.AndFilter; + +import javax.annotation.Nonnull; +import java.util.Objects; + +/** + * Represents a basic row filter policy restriction. + */ +public class RowFilterPolicy implements Policy +{ + @JsonProperty("rowFilter") + private final DimFilter rowFilter; + + @JsonCreator + RowFilterPolicy(@Nonnull @JsonProperty("rowFilter") DimFilter rowFilter) + { + this.rowFilter = rowFilter; Review Comment: verify here that `rowFilter` is nonnull, using `Preconditions.checkNotNull` ########## processing/src/main/java/org/apache/druid/query/policy/NoRestrictionPolicy.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.JsonCreator; +import org.apache.druid.segment.CursorBuildSpec; + +/** + * Represents a special kind of policy restriction, indicating that this table is restricted, but doesn't impose any restriction + * to a user. + */ +public class NoRestrictionPolicy implements Policy +{ + public static final NoRestrictionPolicy INSTANCE = new NoRestrictionPolicy(); + + @JsonCreator Review Comment: for singletons, you can do a `public static NoRestrictionPolicy instance()` method and annotate that with `@JsonCreator`. Then Jackson will return the singleton rather than creating a new instance with the constructor. ########## processing/src/main/java/org/apache/druid/query/DataSource.java: ########## @@ -118,6 +123,26 @@ public interface DataSource */ DataSource withUpdatedDataSource(DataSource newSource); + /** + * Returns the query with an updated datasource based on the policy restrictions on tables. + * <p> + * If this datasource contains no table, no changes should occur. + * + * @param policyMap a mapping of table names to policy restrictions + * @return the updated datasource, with restrictions applied in the datasource tree + * @throws IllegalStateException when mapping a RestrictedDataSource, unless the table has a NoRestrictionPolicy in + * the policyMap (used by druid-internal). Missing policy or adding a + * non-NoRestrictionPolicy to RestrictedDataSource would throw. + */ + default DataSource mapWithRestriction(Map<String, Optional<Policy>> policyMap) Review Comment: IMO `withPolicies` is a better name. ########## processing/src/main/java/org/apache/druid/segment/RestrictedSegment.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.segment; + +import org.apache.druid.query.policy.NoRestrictionPolicy; +import org.apache.druid.query.policy.Policy; +import org.apache.druid.timeline.SegmentId; +import org.joda.time.Interval; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.io.Closeable; +import java.io.IOException; +import java.util.Optional; + +/** + * A wrapped {@link SegmentReference} with a {@link Policy} restriction. The policy must be applied when querying the + * wrapped segment, e.x {@link #asCursorFactory()} returns a policy-enforced {@link RestrictedCursorFactory}. The policy + * and wrapped SegmentReference (i.e. delegate) can't be accessed directly. + * <p> + * There's a backdoor to get the wrapped SegmentReference through {@code as(BypassRestrictedSegment.class)}, returning + * a policy-aware (but not enforced) {@link BypassRestrictedSegment} instance. + */ +public class RestrictedSegment implements SegmentReference +{ + protected final SegmentReference delegate; + protected final Policy policy; + + public RestrictedSegment( + SegmentReference delegate, + Policy policy + ) + { + this.delegate = delegate; + this.policy = policy; + } + + @Override + public Optional<Closeable> acquireReferences() + { + return delegate.acquireReferences(); + } + + @Override + public SegmentId getId() + { + return delegate.getId(); + } + + @Override + public Interval getDataInterval() + { + return delegate.getDataInterval(); + } + + @Override + public CursorFactory asCursorFactory() + { + return new RestrictedCursorFactory(delegate.asCursorFactory(), policy); + } + + @Nullable + @Override + public QueryableIndex asQueryableIndex() + { + return null; + } + + @Nullable + @Override + public <T> T as(@Nonnull Class<T> clazz) + { + if (CursorFactory.class.equals(clazz)) { + return (T) asCursorFactory(); + } else if (QueryableIndex.class.equals(clazz)) { + return null; + } else if (TimeBoundaryInspector.class.equals(clazz)) { + return (T) WrappedTimeBoundaryInspector.create(delegate.as(TimeBoundaryInspector.class)); + } else if (TopNOptimizationInspector.class.equals(clazz)) { + return (T) new SimpleTopNOptimizationInspector(policy instanceof NoRestrictionPolicy); + } else if (BypassRestrictedSegment.class.equals(clazz)) { + // A backdoor solution to get the wrapped segment, effectively bypassing the policy. + return (T) new BypassRestrictedSegment(delegate, policy); Review Comment: Please add `BypassRestrictedSegment` to the javadoc for `Segment#as`. It should have all the interfaces that core segments could support. ########## processing/src/main/java/org/apache/druid/query/policy/Policy.java: ########## @@ -0,0 +1,81 @@ +/* + * 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.segment.CursorBuildSpec; + +/** + * Extensible interface for a granular-level (e.x. row filter) restriction on read-table access. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") +@JsonSubTypes({ + @JsonSubTypes.Type(value = RowFilterPolicy.class, name = "row"), + @JsonSubTypes.Type(value = NoRestrictionPolicy.class, name = "noRestriction") +}) +public interface Policy +{ + /** + * Apply this policy to a {@link CursorBuildSpec} to seamlessly enforce policies for cursor-based queries. The + * application must encapsulate 100% of the requirements of this policy. + */ + CursorBuildSpec visit(CursorBuildSpec spec); + + /** + * Defines how strict we want to enforce the policy on tables during query execution process. + * <ol> + * <li>{@code APPLY_WHEN_APPLICABLE}, the most basic level, restriction is applied whenever seen fit. + * <li>{@code POLICY_CHECKED_ON_ALL_TABLES_ALLOW_EMPTY}, every table must have been checked on the policy. + * <li>{@code POLICY_CHECKED_ON_ALL_TABLES_POLICY_MUST_EXIST}, every table must have a policy when requests come from external users. + * </ol> + */ + enum TablePolicySecurityLevel Review Comment: Let's please remove this and do something similar in a follow up PR. I think some more iteration will be useful on how strictness works, and it'll be easier to that stuff in its own PR. ########## processing/src/main/java/org/apache/druid/segment/BypassRestrictedSegment.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.segment; + +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.policy.Policy; + +/** + * A wrapped {@link SegmentReference} with a {@link DimFilter} restriction, and the policy restriction can be bypassed. Review Comment: This javadoc isn't really clear. The phrases "with restriction", "can be bypassed", and in the next line "some methods", make it sound like the restriction is applied sometimes. But it's actually never applied. It's the responsibility of the query engine to apply it. Also, the javadoc mentions `DimFilter` restriction, but it's really any policy that can be here. Better wording would be: > A wrapped `{@link SegmentReference}` containing a `{@link Policy}` that is not applied. It is up to the caller to apply the policy. Obtained from a `{@link RestrictedSegment}`. > > This class is useful when a query engine needs direct access to interfaces that cannot have policies applied transparently. For example, `{@link RestrictedSegment}` returns `null` for `{@link #asQueryableIndex}` because it cannot apply policies transparently to a `{@link QueryableIndex}`. To use one, a query engine needs to obtain a `BypassRestrictedSegment` and apply the policies itself. ########## processing/src/main/java/org/apache/druid/query/policy/RowFilterPolicy.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.collect.ImmutableList; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.Filter; +import org.apache.druid.segment.CursorBuildSpec; +import org.apache.druid.segment.filter.AndFilter; + +import javax.annotation.Nonnull; +import java.util.Objects; + +/** + * Represents a basic row filter policy restriction. + */ +public class RowFilterPolicy implements Policy +{ + @JsonProperty("rowFilter") + private final DimFilter rowFilter; Review Comment: typically, rather than having Jackson use the `private` fields directly, we create a getter and annotate the getter with `@JsonProperty`. Please make that change here. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,235 @@ +/* + * 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.server.security; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import org.apache.druid.error.DruidException; +import org.apache.druid.query.policy.NoRestrictionPolicy; +import org.apache.druid.query.policy.Policy; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +/** + * Represents the outcoming of performing authorization check on required resource accesses on a query or http requests. + * It contains: + * <ul> + * <li>a boolean allow or deny access results for checking permissions on a list of resource actions. + * <li>a failure message if deny access. It's null when access is allowed. + * <li>a map of table name with optional {@link Policy} restriction. An empty value means there's no restriction + * enforced on the table. + * </ul> + */ +public class AuthorizationResult +{ + /** + * Provides access with no restrictions to all resources.This should be limited to Druid internal systems or + * superusers, except in cases where granular ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_NO_RESTRICTION = new AuthorizationResult( + PERMISSION.ALLOW_NO_RESTRICTION, + null, + Collections.emptyMap() + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + PERMISSION.DENY, + Access.DENIED.getMessage(), + Collections.emptyMap() + ); + + enum PERMISSION + { + ALLOW_NO_RESTRICTION, + ALLOW_WITH_RESTRICTION, + DENY + } + + private final PERMISSION permission; Review Comment: Should be `Permission` (style) ########## processing/src/main/java/org/apache/druid/segment/RestrictedSegment.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.segment; + +import org.apache.druid.query.policy.NoRestrictionPolicy; +import org.apache.druid.query.policy.Policy; +import org.apache.druid.timeline.SegmentId; +import org.joda.time.Interval; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.io.Closeable; +import java.io.IOException; +import java.util.Optional; + +/** + * A wrapped {@link SegmentReference} with a {@link Policy} restriction. The policy must be applied when querying the Review Comment: "The policy must be applied when querying" makes it sound like it's the caller's responsibility to apply the policy. But it's not; the policy is applied transparently. Better wording would emphasize that the `RestrictedSegment` does apply policies transparently, except in just one case: if you call `as(BypassRestrictedSegment.class)`, the resulting object does not apply policies. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,235 @@ +/* + * 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.server.security; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import org.apache.druid.error.DruidException; +import org.apache.druid.query.policy.NoRestrictionPolicy; +import org.apache.druid.query.policy.Policy; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +/** + * Represents the outcoming of performing authorization check on required resource accesses on a query or http requests. + * It contains: + * <ul> + * <li>a boolean allow or deny access results for checking permissions on a list of resource actions. + * <li>a failure message if deny access. It's null when access is allowed. + * <li>a map of table name with optional {@link Policy} restriction. An empty value means there's no restriction + * enforced on the table. + * </ul> + */ +public class AuthorizationResult +{ + /** + * Provides access with no restrictions to all resources.This should be limited to Druid internal systems or + * superusers, except in cases where granular ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_NO_RESTRICTION = new AuthorizationResult( + PERMISSION.ALLOW_NO_RESTRICTION, + null, + Collections.emptyMap() + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + PERMISSION.DENY, + Access.DENIED.getMessage(), + Collections.emptyMap() + ); + + enum PERMISSION + { + ALLOW_NO_RESTRICTION, + ALLOW_WITH_RESTRICTION, + DENY + } + + private final PERMISSION permission; + + @Nullable + private final String failureMessage; + + private final Map<String, Optional<Policy>> policyRestrictions; + + AuthorizationResult( + PERMISSION permission, + @Nullable String failureMessage, + Map<String, Optional<Policy>> policyRestrictions + ) + { + this.permission = permission; + this.failureMessage = failureMessage; + this.policyRestrictions = policyRestrictions; + + // sanity check + switch (permission) { + case DENY: + validateFailureMessageIsSet(); + validatePolicyRestrictionEmpty(); + return; + case ALLOW_WITH_RESTRICTION: + validateFailureMessageNull(); + validatePolicyRestrictionNonEmpty(); + return; + case ALLOW_NO_RESTRICTION: + validateFailureMessageNull(); + validatePolicyRestrictionEmpty(); + return; + default: + throw DruidException.defensive("unreachable"); + } + } + + public static AuthorizationResult deny(@Nonnull String failureMessage) + { + return new AuthorizationResult(PERMISSION.DENY, failureMessage, Collections.emptyMap()); + } + + public static AuthorizationResult allowWithRestriction(Map<String, Optional<Policy>> policyRestrictions) + { + if (policyRestrictions.isEmpty()) { + return ALLOW_NO_RESTRICTION; + } + return new AuthorizationResult(PERMISSION.ALLOW_WITH_RESTRICTION, null, policyRestrictions); + } + + /** + * Returns true if user has basic access. Review Comment: The javadoc should define what "basic access" is, since it's not obvious. Although, the term seems a little odd anyway: it seems like it means "allowed with or without restrictions". To me "basic" doesn't seem like the right word for that. I don't really know what is the right word. The method is only used in one place (`QueryLifecycle`); perhaps the thing to do is update that place to check the `permission` directly using `getPermission()`. ########## server/src/main/java/org/apache/druid/segment/metadata/SegmentMetadataQuerySegmentWalker.java: ########## @@ -142,7 +142,11 @@ private <T> Sequence<T> run( final TimelineLookup<String, SegmentLoadInfo> timelineLookup = timelineConverter.apply(timeline); QueryToolChest<T, Query<T>> toolChest = conglomerate.getToolChest(query); - Set<Pair<SegmentDescriptor, SegmentLoadInfo>> segmentAndServers = computeSegmentsToQuery(timelineLookup, query, toolChest); Review Comment: no reason to make changes in this file, I think? If the changes are 100% formatting changes then let's not include them. (it's ok to include formatting changes if there are also substantive changes in the same file) ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,235 @@ +/* + * 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.server.security; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import org.apache.druid.error.DruidException; +import org.apache.druid.query.policy.NoRestrictionPolicy; +import org.apache.druid.query.policy.Policy; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +/** + * Represents the outcoming of performing authorization check on required resource accesses on a query or http requests. + * It contains: + * <ul> + * <li>a boolean allow or deny access results for checking permissions on a list of resource actions. + * <li>a failure message if deny access. It's null when access is allowed. + * <li>a map of table name with optional {@link Policy} restriction. An empty value means there's no restriction + * enforced on the table. + * </ul> + */ +public class AuthorizationResult +{ + /** + * Provides access with no restrictions to all resources.This should be limited to Druid internal systems or + * superusers, except in cases where granular ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_NO_RESTRICTION = new AuthorizationResult( + PERMISSION.ALLOW_NO_RESTRICTION, + null, + Collections.emptyMap() + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + PERMISSION.DENY, + Access.DENIED.getMessage(), + Collections.emptyMap() + ); + + enum PERMISSION + { + ALLOW_NO_RESTRICTION, + ALLOW_WITH_RESTRICTION, + DENY + } + + private final PERMISSION permission; + + @Nullable + private final String failureMessage; + + private final Map<String, Optional<Policy>> policyRestrictions; + + AuthorizationResult( + PERMISSION permission, + @Nullable String failureMessage, + Map<String, Optional<Policy>> policyRestrictions + ) + { + this.permission = permission; + this.failureMessage = failureMessage; + this.policyRestrictions = policyRestrictions; + + // sanity check + switch (permission) { + case DENY: + validateFailureMessageIsSet(); + validatePolicyRestrictionEmpty(); + return; + case ALLOW_WITH_RESTRICTION: + validateFailureMessageNull(); + validatePolicyRestrictionNonEmpty(); + return; + case ALLOW_NO_RESTRICTION: + validateFailureMessageNull(); + validatePolicyRestrictionEmpty(); + return; + default: + throw DruidException.defensive("unreachable"); + } + } + + public static AuthorizationResult deny(@Nonnull String failureMessage) + { + return new AuthorizationResult(PERMISSION.DENY, failureMessage, Collections.emptyMap()); + } + + public static AuthorizationResult allowWithRestriction(Map<String, Optional<Policy>> policyRestrictions) + { + if (policyRestrictions.isEmpty()) { + return ALLOW_NO_RESTRICTION; + } + return new AuthorizationResult(PERMISSION.ALLOW_WITH_RESTRICTION, null, policyRestrictions); + } + + /** + * Returns true if user has basic access. + */ + public boolean allowBasicAccess() + { + return PERMISSION.ALLOW_NO_RESTRICTION.equals(permission) || PERMISSION.ALLOW_WITH_RESTRICTION.equals(permission); + } + + /** + * Returns true if user has all required permission, and the policy restrictions indicates one of the following: + * <li> no policy found + * <li> the user has a no-restriction policy + */ + public boolean allowAccessWithNoRestriction() + { + return PERMISSION.ALLOW_NO_RESTRICTION.equals(permission) || (PERMISSION.ALLOW_WITH_RESTRICTION.equals(permission) + && policyRestrictions.values() + .stream() + .map(p -> p.orElse(null)) + .filter(Objects::nonNull) // Can be replaced by Optional::stream after java 11 + .allMatch(p -> (p instanceof NoRestrictionPolicy))); + } + + /** + * Returns an error string if the AuthorizationResult doesn't permit all requried access. + */ + public String getErrorMessage() + { + switch (permission) { + case DENY: + return Objects.requireNonNull(failureMessage); + case ALLOW_WITH_RESTRICTION: + if (!allowAccessWithNoRestriction()) { + return Access.DEFAULT_ERROR_MESSAGE; + } + default: + throw DruidException.defensive("unreachable"); + } + } + + public Map<String, Optional<Policy>> getPolicy() Review Comment: 3 notes: - It would be clearer to name this `getPolicyMap()`. - Please include javadoc, since it's not obvious what the keys are. - The value should be a `List<Policy>` rather than `Optional<Policy>`. ########## processing/src/main/java/org/apache/druid/segment/RestrictedCursorFactory.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.segment; + +import org.apache.druid.query.policy.Policy; +import org.apache.druid.segment.column.ColumnCapabilities; +import org.apache.druid.segment.column.RowSignature; + +import javax.annotation.Nullable; + +public class RestrictedCursorFactory implements CursorFactory Review Comment: Javadoc please. Link back to `RestrictedSegment` and say this is used to transparently apply policies to cursors. ########## processing/src/main/java/org/apache/druid/query/policy/RowFilterPolicy.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.collect.ImmutableList; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.Filter; +import org.apache.druid.segment.CursorBuildSpec; +import org.apache.druid.segment.filter.AndFilter; + +import javax.annotation.Nonnull; +import java.util.Objects; + +/** + * Represents a basic row filter policy restriction. + */ +public class RowFilterPolicy implements Policy +{ + @JsonProperty("rowFilter") + private final DimFilter rowFilter; + + @JsonCreator + RowFilterPolicy(@Nonnull @JsonProperty("rowFilter") DimFilter rowFilter) + { + this.rowFilter = rowFilter; + } + + public static RowFilterPolicy from(@Nonnull DimFilter rowFilter) + { + return new RowFilterPolicy(rowFilter); + } + + @Override + public CursorBuildSpec visit(CursorBuildSpec spec) + { + CursorBuildSpec.CursorBuildSpecBuilder builder = CursorBuildSpec.builder(spec); + final Filter filter = spec.getFilter(); + final Filter policyFilter = this.rowFilter.toFilter(); + + final Filter newFilter = filter == null ? policyFilter : new AndFilter(ImmutableList.of(policyFilter, filter)); Review Comment: use `Filters.and`, which will merge this with pre-existing top-level `AndFilter` if any is present. If you do this, you don't need to have a null check here either, because `Filters.and` handles it. ########## processing/src/main/java/org/apache/druid/segment/BypassRestrictedSegment.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.segment; + +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.policy.Policy; + +/** + * A wrapped {@link SegmentReference} with a {@link DimFilter} restriction, and the policy restriction can be bypassed. + * <p> + * In some methods, such as {@link #as(Class)}, {@link #asQueryableIndex()}, and {@link #asCursorFactory()}, the policy + * is ignored. + */ +class BypassRestrictedSegment extends RestrictedSegment Review Comment: It's better if this doesn't extend `RestrictedSegment`, since it doesn't actually apply restrictions. It should be its own standalone class. ########## processing/src/main/java/org/apache/druid/query/metadata/metadata/SegmentMetadataQuery.java: ########## @@ -74,7 +74,7 @@ public static AnalysisType fromString(String name) @Override public byte[] getCacheKey() { - return new byte[] {(byte) this.ordinal()}; Review Comment: no reason to make changes in this file, I think? If the changes are 100% formatting changes then let's not include them. (it's ok to include formatting changes if there are also substantive changes in the same file) ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,235 @@ +/* + * 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.server.security; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import org.apache.druid.error.DruidException; +import org.apache.druid.query.policy.NoRestrictionPolicy; +import org.apache.druid.query.policy.Policy; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +/** + * Represents the outcoming of performing authorization check on required resource accesses on a query or http requests. + * It contains: + * <ul> + * <li>a boolean allow or deny access results for checking permissions on a list of resource actions. + * <li>a failure message if deny access. It's null when access is allowed. + * <li>a map of table name with optional {@link Policy} restriction. An empty value means there's no restriction + * enforced on the table. + * </ul> + */ +public class AuthorizationResult +{ + /** + * Provides access with no restrictions to all resources.This should be limited to Druid internal systems or + * superusers, except in cases where granular ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_NO_RESTRICTION = new AuthorizationResult( + PERMISSION.ALLOW_NO_RESTRICTION, + null, + Collections.emptyMap() + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + PERMISSION.DENY, + Access.DENIED.getMessage(), + Collections.emptyMap() + ); + + enum PERMISSION + { + ALLOW_NO_RESTRICTION, + ALLOW_WITH_RESTRICTION, + DENY + } + + private final PERMISSION permission; + + @Nullable + private final String failureMessage; + + private final Map<String, Optional<Policy>> policyRestrictions; + + AuthorizationResult( + PERMISSION permission, + @Nullable String failureMessage, + Map<String, Optional<Policy>> policyRestrictions + ) + { + this.permission = permission; + this.failureMessage = failureMessage; + this.policyRestrictions = policyRestrictions; + + // sanity check + switch (permission) { + case DENY: + validateFailureMessageIsSet(); + validatePolicyRestrictionEmpty(); + return; + case ALLOW_WITH_RESTRICTION: + validateFailureMessageNull(); + validatePolicyRestrictionNonEmpty(); + return; + case ALLOW_NO_RESTRICTION: + validateFailureMessageNull(); + validatePolicyRestrictionEmpty(); + return; + default: + throw DruidException.defensive("unreachable"); + } + } + + public static AuthorizationResult deny(@Nonnull String failureMessage) + { + return new AuthorizationResult(PERMISSION.DENY, failureMessage, Collections.emptyMap()); + } + + public static AuthorizationResult allowWithRestriction(Map<String, Optional<Policy>> policyRestrictions) + { + if (policyRestrictions.isEmpty()) { + return ALLOW_NO_RESTRICTION; + } + return new AuthorizationResult(PERMISSION.ALLOW_WITH_RESTRICTION, null, policyRestrictions); + } + + /** + * Returns true if user has basic access. + */ + public boolean allowBasicAccess() + { + return PERMISSION.ALLOW_NO_RESTRICTION.equals(permission) || PERMISSION.ALLOW_WITH_RESTRICTION.equals(permission); + } + + /** + * Returns true if user has all required permission, and the policy restrictions indicates one of the following: + * <li> no policy found + * <li> the user has a no-restriction policy + */ + public boolean allowAccessWithNoRestriction() + { + return PERMISSION.ALLOW_NO_RESTRICTION.equals(permission) || (PERMISSION.ALLOW_WITH_RESTRICTION.equals(permission) + && policyRestrictions.values() + .stream() + .map(p -> p.orElse(null)) + .filter(Objects::nonNull) // Can be replaced by Optional::stream after java 11 Review Comment: we do require Java 11 now, so this can be written using Java 11 functions. ########## server/src/main/java/org/apache/druid/server/QueryLifecycle.java: ########## @@ -155,10 +162,6 @@ public <T> QueryResponse<T> runSimple( final QueryResponse<T> queryResponse; try { preAuthorized(authenticationResult, authorizationResult); - if (!authorizationResult.isAllowed()) { Review Comment: Why remove this? What happens now if a request is not authorized? ########## processing/src/main/java/org/apache/druid/query/RestrictedDataSource.java: ########## @@ -0,0 +1,212 @@ +/* + * 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; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.collect.ImmutableList; +import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.query.planning.DataSourceAnalysis; +import org.apache.druid.query.policy.Policy; +import org.apache.druid.segment.RestrictedSegment; +import org.apache.druid.segment.SegmentReference; +import org.apache.druid.utils.JvmUtils; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Function; + +/** + * Reperesents a TableDataSource with row-level restriction. + * <p> + * A RestrictedDataSource means the base TableDataSource has restriction imposed. A table without any restriction should + * never be transformed to a RestrictedDataSource. Druid internal system and admin users would have a null rowFilter, + * while external users would have a rowFilter based on the applied restriction. + */ +public class RestrictedDataSource implements DataSource +{ + private final TableDataSource base; + private final Policy policy; Review Comment: Policies may come from multiple sources, and all need to be combined together by Authorizers. So, this either needs to be `List<Policy>` or we need to have an `AndPolicy` that can wrap multiple policies. IMO, `List<Policy>` is cleaner since it makes it easier to write code that combines policies and code that walks through policies. With a `List`, that's just adding elements to a list or iterating a list. With `Policy`, combining two policies would require creating a new `AndPolicy` (and checking the input policies to see if they are already `AndPolicy`; & if so, flattening them.) Walking through policies would require an `instanceof AndPolicy` check. So my vote is to go with `List<Policy>` 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]
