gianm commented on code in PR #17564: URL: https://github.com/apache/druid/pull/17564#discussion_r1889263579
########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,136 @@ +/* + * 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.collect.ImmutableMap; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +public class AuthorizationResult +{ + /** + * Provides unrestricted access to all resources. This should be limited to Druid internal systems or superusers, + * except in cases where ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_ALL = new AuthorizationResult( + true, + null, + Collections.emptyMap(), + null, + null + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + false, + Access.DENIED.getMessage(), + Collections.emptyMap(), + null, + null + ); + + private final boolean isAllowed; + + @Nullable + private final String failureMessage; + + private final Map<String, Optional<DimFilter>> policyFilters; + + @Nullable + private final Set<ResourceAction> sqlResourceActions; + + @Nullable + private final Set<ResourceAction> allResourceActions; + + AuthorizationResult( + boolean isAllowed, + @Nullable String failureMessage, + Map<String, Optional<DimFilter>> policyFilters, + @Nullable Set<ResourceAction> sqlResourceActions, + @Nullable Set<ResourceAction> allResourceActions + ) + { + this.isAllowed = isAllowed; + this.failureMessage = failureMessage; + this.policyFilters = policyFilters; + this.sqlResourceActions = sqlResourceActions; + this.allResourceActions = allResourceActions; + } + + public static AuthorizationResult deny(@Nonnull String failureMessage) + { + return new AuthorizationResult(false, failureMessage, Collections.emptyMap(), null, null); + } + + public static AuthorizationResult allowWithRestriction(Map<String, Optional<DimFilter>> policyFilters) + { + return new AuthorizationResult(true, null, policyFilters, null, null); + } + + public AuthorizationResult withResourceActions( + Set<ResourceAction> sqlResourceActions, + Set<ResourceAction> allResourceActions + ) + { + return new AuthorizationResult( + isAllowed(), + getFailureMessage(), + ImmutableMap.copyOf(getPolicyFilters()), + sqlResourceActions, + allResourceActions + ); + } + + public boolean isAllowed() Review Comment: The current design here has a problem— since `isAllowed()` returns `true` when there are policies, it encourages code to "forget" to check policies. It would be better to use a tri-state enum so callers don't "forget": `ALLOWED`, `DENIED`, `ALLOWED_WITH_POLICIES`. OTOH, if we want to maintain more compatibility for extensions, then we can merge this stuff back into `Access`, and do two methods: `isAllowed()` and `isAllowedWithPolicies()`. When there are policies in play, the first method should return `false` and second should return `true`. That would mean things fail-denied if the caller "forgets" to check the policies. ########## server/src/main/java/org/apache/druid/server/security/Access.java: ########## @@ -21,52 +21,86 @@ import com.google.common.base.Strings; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nullable; +import java.util.Objects; +import java.util.Optional; public class Access { public static final String DEFAULT_ERROR_MESSAGE = "Unauthorized"; + public static final String DEFAULT_AUTHORIZED_MESSAGE = "Authorized"; - public static final Access OK = new Access(true); - public static final Access DENIED = new Access(false); + public static final Access OK = Access.allow(); + public static final Access DENIED = Access.deny(""); private final boolean allowed; private final String message; + // A row-level policy filter on top of table-level read access. It should be empty if there are no policy restrictions + // or if access is requested for an action other than reading the table. + private final Optional<DimFilter> rowFilter; + /** + * @deprecated use {@link #allow()} or {@link #deny(String)} instead + */ + @Deprecated public Access(boolean allowed) { - this(allowed, ""); + this(allowed, "", Optional.empty()); } - public Access(boolean allowed, String message) + Access(boolean allowed, String message, Optional<DimFilter> rowFilter) { this.allowed = allowed; this.message = message; + this.rowFilter = rowFilter; + } + + public static Access allow() + { + return new Access(true, "", Optional.empty()); + } + + public static Access deny(@Nullable String message) + { + return new Access(false, Objects.isNull(message) ? "" : message, Optional.empty()); + } + + public static Access allowWithRestriction(Optional<DimFilter> rowFilter) + { + return new Access(true, "", rowFilter); } public boolean isAllowed() Review Comment: The design has a problem— since `isAllowed()` returns `true` when there are policies, it encourages code to "forget" to check policies. It would be better to use a tri-state enum so callers don't "forget". We can use the same one I mentioned in the comments for `AuthorizationResult`. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.collect.ImmutableMap; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.TrueDimFilter; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; + +public class AuthorizationResult Review Comment: Please also augment this class with Javadocs. ########## server/src/main/java/org/apache/druid/server/security/Access.java: ########## @@ -21,52 +21,86 @@ import com.google.common.base.Strings; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nullable; +import java.util.Objects; +import java.util.Optional; public class Access Review Comment: Please augment this class with Javadocs for all methods. ########## processing/src/main/java/org/apache/druid/query/Query.java: ########## @@ -242,6 +243,16 @@ default String getMostSpecificId() Query<T> withDataSource(DataSource dataSource); + default Query<T> withPolicyRestrictions(Map<String, Optional<DimFilter>> restrictions) + { + return this.withPolicyRestrictions(restrictions, true); + } + + default Query<T> withPolicyRestrictions(Map<String, Optional<DimFilter>> restrictions, boolean enableStrictPolicyCheck) Review Comment: Javadoc please. ########## processing/src/main/java/org/apache/druid/segment/RestrictedSegment.java: ########## @@ -0,0 +1,52 @@ +/* + * 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 javax.annotation.Nullable; + +public class RestrictedSegment extends WrappedSegmentReference Review Comment: Please include javadoc describing the purpose of this class, and how its defensive mechanisms work. The way they should work is something like: that if you call `asCursorFactory` or `as(CursorFactory.class)` (plus perhaps some other small list), restrictions are handled automatically. But if you call `asQueryableIndex` or `as` for something other than that small list, the query gets the unrestricted internal object and it needs to call some method on the `RestrictedSegment` confirming that it applied the restrictions on its own. ########## server/src/main/java/org/apache/druid/server/security/AuthConfig.java: ########## @@ -100,6 +100,9 @@ public AuthConfig() @JsonProperty private final boolean enableInputSourceSecurity; + @JsonProperty + private final boolean enableStrictPolicyCheck; Review Comment: Please include Javadoc about what this means (what exactly is "strict" about it). ########## processing/src/main/java/org/apache/druid/query/DataSource.java: ########## @@ -118,6 +123,27 @@ public interface DataSource */ DataSource withUpdatedDataSource(DataSource newSource); + default DataSource mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters) + { + return mapWithRestriction(rowFilters, true); + } + + /** + * Returns an updated datasource based on the policy restrictions on tables. If this datasource contains no table, no + * changes should occur. + * + * @param rowFilters a mapping of table names to row filters, every table in the datasource tree must have an entry + * @return the updated datasource, with restrictions applied in the datasource tree + */ + default DataSource mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters, boolean enableStrictPolicyCheck) Review Comment: We should design the strict check a bit differently. With this current design, a single literally TRUE filter would pass, but we don't want that. I think it would be better for this to _not_ take `enableStrictPolicyCheck`, but instead for the strict check to happen in `QueryLifecycle` after the query is mapped. That would enable the check to be even stricter: it should really check not just that there is a filter, but also that the filter is actually doing something. To allow for the `druid_internal` or `admin` case, we can bypass the strict check if the user has permission for `STATE READ` (a broad administrative permission). ########## processing/src/main/java/org/apache/druid/query/RestrictedDataSource.java: ########## @@ -0,0 +1,227 @@ +/* + * 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.error.DruidException; +import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.TrueDimFilter; +import org.apache.druid.query.planning.DataSourceAnalysis; +import org.apache.druid.segment.RestrictedSegment; +import org.apache.druid.segment.SegmentReference; +import org.apache.druid.utils.JvmUtils; + +import javax.annotation.Nullable; +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; + @Nullable + private final DimFilter rowFilter; + + @JsonProperty("base") + public TableDataSource getBase() + { + return base; + } + + /** + * Returns true if the row-level filter imposes no restrictions. + */ + public boolean allowAll() + { + return Objects.isNull(rowFilter) || rowFilter.equals(TrueDimFilter.instance()); + } + + @Nullable + @JsonProperty("filter") + public DimFilter getFilter() + { + return rowFilter; + } + + RestrictedDataSource(TableDataSource base, @Nullable DimFilter rowFilter) + { + this.base = base; + this.rowFilter = rowFilter; + } + + @JsonCreator + public static RestrictedDataSource create( + @JsonProperty("base") DataSource base, + @Nullable @JsonProperty("filter") DimFilter rowFilter + ) + { + if (!(base instanceof TableDataSource)) { + throw new IAE("Expected a TableDataSource, got [%s]", base.getClass()); + } + return new RestrictedDataSource((TableDataSource) base, rowFilter); + } + + @Override + public Set<String> getTableNames() + { + return base.getTableNames(); + } + + @Override + public List<DataSource> getChildren() + { + return ImmutableList.of(base); + } + + @Override + public DataSource withChildren(List<DataSource> children) + { + if (children.size() != 1) { + throw new IAE("Expected [1] child, got [%d]", children.size()); + } + + return create(children.get(0), rowFilter); + } + + @Override + public boolean isCacheable(boolean isBroker) + { + return false; + } + + @Override + public boolean isGlobal() + { + return base.isGlobal(); + } + + @Override + public boolean isConcrete() + { + return base.isConcrete(); + } + + @Override + public Function<SegmentReference, SegmentReference> createSegmentMapFunction( + Query query, + AtomicLong cpuTimeAccumulator + ) + { + return JvmUtils.safeAccumulateThreadCpuTime( + cpuTimeAccumulator, + () -> base.createSegmentMapFunction( + query, + cpuTimeAccumulator + ).andThen((segment) -> (new RestrictedSegment(segment, rowFilter))) + ); + } + + @Override + public DataSource withUpdatedDataSource(DataSource newSource) + { + return create(newSource, rowFilter); + } + + @Override + public DataSource mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters, boolean enableStrictPolicyCheck) + { + if (!rowFilters.containsKey(this.base.getName()) && enableStrictPolicyCheck) { + throw DruidException.defensive("Missing row filter for table [%s]", this.base.getName()); + } + + Optional<DimFilter> newFilter = rowFilters.getOrDefault(this.base.getName(), Optional.empty()); + if (!newFilter.isPresent()) { + throw DruidException.defensive( + "No restriction found on table [%s], but had %s before.", + this.base.getName(), + this.rowFilter + ); + } + if (newFilter.get().equals(TrueDimFilter.instance())) { + // The internal druid_system always has a TrueDimFilter, whic can be applied in conjunction with an external user's filter. + return this; + } else if (newFilter.get().equals(rowFilter)) { + // This likely occurs when we perform an authentication check for the same user more than once, which is not ideal. + return this; + } else { + throw new ISE("Incompatible restrictions on [%s]: %s and %s", this.base.getName(), rowFilter, newFilter.get()); + } + } + + @Override + public String toString() + { + try { + return "RestrictedDataSource{" + + "base=" + base + + ", filter='" + rowFilter + '}'; + } + catch (Exception e) { Review Comment: why the try/catch here? It shouldn't be necessary. ########## server/src/main/java/org/apache/druid/server/security/Access.java: ########## @@ -21,52 +21,86 @@ import com.google.common.base.Strings; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nullable; +import java.util.Objects; +import java.util.Optional; public class Access { public static final String DEFAULT_ERROR_MESSAGE = "Unauthorized"; + public static final String DEFAULT_AUTHORIZED_MESSAGE = "Authorized"; - public static final Access OK = new Access(true); - public static final Access DENIED = new Access(false); + public static final Access OK = Access.allow(); + public static final Access DENIED = Access.deny(""); private final boolean allowed; private final String message; + // A row-level policy filter on top of table-level read access. It should be empty if there are no policy restrictions + // or if access is requested for an action other than reading the table. + private final Optional<DimFilter> rowFilter; + /** + * @deprecated use {@link #allow()} or {@link #deny(String)} instead + */ + @Deprecated public Access(boolean allowed) { - this(allowed, ""); + this(allowed, "", Optional.empty()); } - public Access(boolean allowed, String message) + Access(boolean allowed, String message, Optional<DimFilter> rowFilter) { this.allowed = allowed; this.message = message; + this.rowFilter = rowFilter; + } + + public static Access allow() + { + return new Access(true, "", Optional.empty()); + } + + public static Access deny(@Nullable String message) + { + return new Access(false, Objects.isNull(message) ? "" : message, Optional.empty()); Review Comment: You could use `StringUtils.nullToEmptyNonDruidDataString` here. ########## processing/src/main/java/org/apache/druid/query/DataSource.java: ########## @@ -118,6 +123,27 @@ public interface DataSource */ DataSource withUpdatedDataSource(DataSource newSource); + default DataSource mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters) + { + return mapWithRestriction(rowFilters, true); + } + + /** + * Returns an updated datasource based on the policy restrictions on tables. If this datasource contains no table, no + * changes should occur. + * + * @param rowFilters a mapping of table names to row filters, every table in the datasource tree must have an entry Review Comment: This should take `Map<String, Policies>` instead, so other types of policies can be applied in the future without changing the `DataSource` interface. ########## server/src/main/java/org/apache/druid/server/security/Access.java: ########## @@ -21,52 +21,86 @@ import com.google.common.base.Strings; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nullable; +import java.util.Objects; +import java.util.Optional; public class Access { public static final String DEFAULT_ERROR_MESSAGE = "Unauthorized"; + public static final String DEFAULT_AUTHORIZED_MESSAGE = "Authorized"; - public static final Access OK = new Access(true); - public static final Access DENIED = new Access(false); + public static final Access OK = Access.allow(); + public static final Access DENIED = Access.deny(""); private final boolean allowed; private final String message; + // A row-level policy filter on top of table-level read access. It should be empty if there are no policy restrictions + // or if access is requested for an action other than reading the table. + private final Optional<DimFilter> rowFilter; + /** + * @deprecated use {@link #allow()} or {@link #deny(String)} instead + */ + @Deprecated public Access(boolean allowed) { - this(allowed, ""); + this(allowed, "", Optional.empty()); } - public Access(boolean allowed, String message) + Access(boolean allowed, String message, Optional<DimFilter> rowFilter) Review Comment: Let's keep the `public Access(boolean allowed, String message)` constructor for compatibility with extension authorizers. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.collect.ImmutableMap; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.TrueDimFilter; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; + +public class AuthorizationResult Review Comment: The API here for checking if something is allowed is a little weird. Callers need to use `getPermissionErrorMessage` to check if the error message is absent, and if it is absent, callers need to apply the policies. It would be better to have an enum `Authorized` with three possible values: `ALLOW`, `DENY`, `ALLOW_WITH_POLICIES`. It should be returned by a method like `public Authorized getResult()`. Callers should dispatch over the possibilities in a way that makes sense to them. Callers that aren't policy-aware should be written such that they fail-safe. i.e., if the return is anything other than `ALLOW` they should treat it as `DENY`. ########## processing/src/main/java/org/apache/druid/query/Query.java: ########## @@ -242,6 +243,16 @@ default String getMostSpecificId() Query<T> withDataSource(DataSource dataSource); + default Query<T> withPolicyRestrictions(Map<String, Optional<DimFilter>> restrictions) Review Comment: Is this for tests only? If so, to avoid adding too many methods to the `Query` interface, let's skip it and only include ` default Query<T> withPolicyRestrictions(Map<String, Optional<DimFilter>> restrictions, boolean enableStrictPolicyCheck)`. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,136 @@ +/* + * 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.collect.ImmutableMap; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +public class AuthorizationResult +{ + /** + * Provides unrestricted access to all resources. This should be limited to Druid internal systems or superusers, + * except in cases where ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_ALL = new AuthorizationResult( + true, + null, + Collections.emptyMap(), + null, + null + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + false, + Access.DENIED.getMessage(), + Collections.emptyMap(), + null, + null + ); + + private final boolean isAllowed; + + @Nullable + private final String failureMessage; + + private final Map<String, Optional<DimFilter>> policyFilters; Review Comment: It would be good to keep the value as some more general class, like `Map<String, Policies>`, so when we want to add other kinds of policies (such as column restrictions, possibly) they can fit right in without further changes to the `AuthorizationResult` object. ########## server/src/main/java/org/apache/druid/server/security/Access.java: ########## @@ -21,52 +21,86 @@ import com.google.common.base.Strings; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.filter.DimFilter; + +import javax.annotation.Nullable; +import java.util.Objects; +import java.util.Optional; public class Access { public static final String DEFAULT_ERROR_MESSAGE = "Unauthorized"; + public static final String DEFAULT_AUTHORIZED_MESSAGE = "Authorized"; - public static final Access OK = new Access(true); - public static final Access DENIED = new Access(false); + public static final Access OK = Access.allow(); + public static final Access DENIED = Access.deny(""); private final boolean allowed; private final String message; + // A row-level policy filter on top of table-level read access. It should be empty if there are no policy restrictions + // or if access is requested for an action other than reading the table. + private final Optional<DimFilter> rowFilter; Review Comment: I feel like it would be good to keep the value as some more general class, like `Policies` rather than `Optional<DimFilter>`, so when we want to add other kinds of policies (such as column restrictions, possibly) they can fit right in without further changes to the `Access` object. ########## server/src/main/java/org/apache/druid/server/QueryLifecycle.java: ########## @@ -215,10 +219,9 @@ public void initialize(final Query<?> baseQuery) * * @param req HTTP request object of the request. If provided, the auth-related fields in the HTTP request * will be automatically set. - * * @return authorization result */ - public Access authorize(HttpServletRequest req) + public AuthorizationResult authorize(HttpServletRequest req) Review Comment: Because `AuthorizationResult` includes policies, this method signature makes it ambiguous as to whether the caller should apply policies. In this case, the `QueryLifecycle` itself applies the policies, and the caller therefore doesn't need to. We should make that clear somehow. Javadoc could do it, or perhaps returning something other than `AuthorizationResult`. ########## processing/src/main/java/org/apache/druid/query/RestrictedDataSource.java: ########## @@ -0,0 +1,227 @@ +/* + * 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.error.DruidException; +import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.TrueDimFilter; +import org.apache.druid.query.planning.DataSourceAnalysis; +import org.apache.druid.segment.RestrictedSegment; +import org.apache.druid.segment.SegmentReference; +import org.apache.druid.utils.JvmUtils; + +import javax.annotation.Nullable; +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; + @Nullable + private final DimFilter rowFilter; + + @JsonProperty("base") + public TableDataSource getBase() + { + return base; + } + + /** + * Returns true if the row-level filter imposes no restrictions. + */ + public boolean allowAll() + { + return Objects.isNull(rowFilter) || rowFilter.equals(TrueDimFilter.instance()); + } + + @Nullable + @JsonProperty("filter") + public DimFilter getFilter() + { + return rowFilter; + } + + RestrictedDataSource(TableDataSource base, @Nullable DimFilter rowFilter) + { + this.base = base; + this.rowFilter = rowFilter; + } + + @JsonCreator + public static RestrictedDataSource create( + @JsonProperty("base") DataSource base, + @Nullable @JsonProperty("filter") DimFilter rowFilter + ) + { + if (!(base instanceof TableDataSource)) { + throw new IAE("Expected a TableDataSource, got [%s]", base.getClass()); + } + return new RestrictedDataSource((TableDataSource) base, rowFilter); + } + + @Override + public Set<String> getTableNames() + { + return base.getTableNames(); + } + + @Override + public List<DataSource> getChildren() + { + return ImmutableList.of(base); + } + + @Override + public DataSource withChildren(List<DataSource> children) + { + if (children.size() != 1) { + throw new IAE("Expected [1] child, got [%d]", children.size()); + } + + return create(children.get(0), rowFilter); + } + + @Override + public boolean isCacheable(boolean isBroker) + { + return false; + } + + @Override + public boolean isGlobal() + { + return base.isGlobal(); + } + + @Override + public boolean isConcrete() + { + return base.isConcrete(); + } + + @Override + public Function<SegmentReference, SegmentReference> createSegmentMapFunction( + Query query, + AtomicLong cpuTimeAccumulator + ) + { + return JvmUtils.safeAccumulateThreadCpuTime( + cpuTimeAccumulator, + () -> base.createSegmentMapFunction( + query, + cpuTimeAccumulator + ).andThen((segment) -> (new RestrictedSegment(segment, rowFilter))) + ); + } + + @Override + public DataSource withUpdatedDataSource(DataSource newSource) + { + return create(newSource, rowFilter); + } + + @Override + public DataSource mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters, boolean enableStrictPolicyCheck) + { + if (!rowFilters.containsKey(this.base.getName()) && enableStrictPolicyCheck) { + throw DruidException.defensive("Missing row filter for table [%s]", this.base.getName()); + } + + Optional<DimFilter> newFilter = rowFilters.getOrDefault(this.base.getName(), Optional.empty()); + if (!newFilter.isPresent()) { + throw DruidException.defensive( + "No restriction found on table [%s], but had %s before.", + this.base.getName(), + this.rowFilter + ); + } + if (newFilter.get().equals(TrueDimFilter.instance())) { + // The internal druid_system always has a TrueDimFilter, whic can be applied in conjunction with an external user's filter. + return this; + } else if (newFilter.get().equals(rowFilter)) { + // This likely occurs when we perform an authentication check for the same user more than once, which is not ideal. Review Comment: What does "not ideal" mean exactly? When can this happen? If it indicates a bug we should actually throw a `defensive` exception here, not `return this`. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.collect.ImmutableMap; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.filter.TrueDimFilter; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; + +public class AuthorizationResult +{ + /** + * Provides unrestricted access to all resources. This should be limited to Druid internal systems or superusers, + * except in cases where ACL considerations are not a priority. + */ + public static final AuthorizationResult ALLOW_ALL = new AuthorizationResult( + true, + null, + Collections.emptyMap(), + null, + null + ); + + /** + * Provides a default deny access result. + */ + public static final AuthorizationResult DENY = new AuthorizationResult( + false, + Access.DENIED.getMessage(), + Collections.emptyMap(), + null, + null + ); + + private final boolean isAllowed; + + @Nullable + private final String failureMessage; + + private final Map<String, Optional<DimFilter>> policyFilters; + + @Nullable + private final Set<ResourceAction> sqlResourceActions; + + @Nullable + private final Set<ResourceAction> allResourceActions; + + AuthorizationResult( + boolean isAllowed, + @Nullable String failureMessage, + Map<String, Optional<DimFilter>> policyFilters, + @Nullable Set<ResourceAction> sqlResourceActions, + @Nullable Set<ResourceAction> allResourceActions + ) + { + this.isAllowed = isAllowed; + this.failureMessage = failureMessage; + this.policyFilters = policyFilters; + this.sqlResourceActions = sqlResourceActions; + this.allResourceActions = allResourceActions; + } + + public static AuthorizationResult deny(@Nonnull String failureMessage) + { + return new AuthorizationResult(false, failureMessage, Collections.emptyMap(), null, null); + } + + public static AuthorizationResult allowWithRestriction(Map<String, Optional<DimFilter>> policyFilters) + { + return new AuthorizationResult(true, null, policyFilters, null, null); + } + + public AuthorizationResult withResourceActions( Review Comment: IMO it's better to keep these fields out of `AuthorizationResult`. They're only used by the SQL planner, whereas `AuthorizationResult` generally is used more broadly. Adding them to `AuthorizationResult` risks confusing non-SQL callers about the purpose of the fields. The SQL planner should have its own object that wraps `AuthorizationResult` instead. ########## processing/src/main/java/org/apache/druid/query/DataSource.java: ########## @@ -118,6 +123,27 @@ public interface DataSource */ DataSource withUpdatedDataSource(DataSource newSource); + default DataSource mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters) Review Comment: Is this for tests only? If so, to avoid adding too many methods to the `DataSource` interface, let's skip it and only include `mapWithRestriction(Map<String, Optional<DimFilter>> rowFilters, boolean enableStrictPolicyCheck)`. ########## server/src/main/java/org/apache/druid/server/security/AuthorizationUtils.java: ########## @@ -35,29 +37,31 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; /** * Static utility functions for performing authorization checks. */ public class AuthorizationUtils { + static final ImmutableSet<String> RESTRICTION_APPLICABLE_RESOURCE_TYPES = ImmutableSet.of( + ResourceType.DATASOURCE, + ResourceType.VIEW Review Comment: IMO, we should do datasource-only for now, since `VIEW` would be another bundle of stuff to think about: views are resolved in the SQL planner, so the restrictions would need to be applied in a different place. This does bring up a question about the model though. If a user has restricted access to a `DATASOURCE`, should those restrictions apply when the datasource is accessed through a SQL view? My stance is "yes" and I think the way we're doing it will achieve that. Please include some tests for this just to be sure it works as expected. -- 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]
