henrib commented on code in PR #6458: URL: https://github.com/apache/hive/pull/6458#discussion_r4327962865
########## standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/IcebergVendedCredentialProvider.java: ########## @@ -0,0 +1,179 @@ +/* + * 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.iceberg.rest; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.metastore.credential.CompositeVendedCredentialProvider; +import org.apache.hadoop.hive.metastore.credential.StorageAccessRequest; +import org.apache.hadoop.hive.metastore.credential.StorageOperation; +import org.apache.hadoop.hive.metastore.credential.VendedCredentialProvider; +import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils; +import org.apache.hadoop.hive.ql.metadata.HiveException; +import org.apache.hadoop.hive.ql.metadata.HiveUtils; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAccessControlException; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthorizer; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzContext; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzPluginException; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzSessionContext; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveMetastoreClientFactoryImpl; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveOperationType; +import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.rest.credentials.Credential; +import org.apache.iceberg.rest.credentials.ImmutableCredential; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.Collections; +import java.util.EnumSet; +import java.util.List; +import java.util.Set; +import java.util.function.Supplier; + +/** + * This class provides vended credentials for Iceberg. + */ +public class IcebergVendedCredentialProvider { + private final String catalog; + private final VendedCredentialProvider vendedCredentialProvider; + private final Supplier<HiveAuthorizer> authorizerSupplier; + + public IcebergVendedCredentialProvider(Configuration conf) { + this.catalog = MetaStoreUtils.getDefaultCatalog(conf); + this.vendedCredentialProvider = new CompositeVendedCredentialProvider(conf); + this.authorizerSupplier = () -> { + try { + final var hiveConf = HiveConf.cloneConf(conf); + final var authorizerFactory = HiveUtils.getAuthorizerFactory(hiveConf, + HiveConf.ConfVars.HIVE_AUTHORIZATION_MANAGER); + if (authorizerFactory == null) { + throw new IllegalStateException("Iceberg's Credential Vending requires Hive's Authorization Manager"); + } + + final var authenticator = HiveUtils.getAuthenticator(hiveConf, + HiveConf.ConfVars.HIVE_METASTORE_AUTHENTICATOR_MANAGER); + authenticator.setConf(hiveConf); + + final var authzContextBuilder = new HiveAuthzSessionContext.Builder(); + authzContextBuilder.setClientType(HiveAuthzSessionContext.CLIENT_TYPE.HIVEMETASTORE); + authzContextBuilder.setSessionString("IcebergRESTCatalog"); + return authorizerFactory.createHiveAuthorizer( + new HiveMetastoreClientFactoryImpl(hiveConf), hiveConf, authenticator, authzContextBuilder.build()); + } catch (HiveException e) { + throw new IllegalStateException("Failed to initialize Hive authorizer for Iceberg credential vending", e); + } + }; + } + + @VisibleForTesting + IcebergVendedCredentialProvider(String catalog, VendedCredentialProvider vendedCredentialProvider, + Supplier<HiveAuthorizer> authorizerSupplier) { + this.catalog = catalog; + this.vendedCredentialProvider = vendedCredentialProvider; + this.authorizerSupplier = authorizerSupplier; + } + + /** + * Vends credentials for the given table identifier. + * + * @param identifier the table identifier + * @param location the table location + * @return the vended credentials + */ + public List<Credential> vend(TableIdentifier identifier, String location) { + final String username; + try { + username = UserGroupInformation.getCurrentUser().getShortUserName(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + final var allowedOperations = resolveAllowedOperations(identifier); + if (allowedOperations.isEmpty()) { + return Collections.emptyList(); + } + final var request = new StorageAccessRequest(new Path(location), allowedOperations); + return vendedCredentialProvider.vend(username, Collections.singletonList(request)).stream() + .map(credential -> ImmutableCredential.builder() + .prefix(credential.prefix().toString()).config(credential.credentials()).build()) + .map(x -> (Credential) x) + .toList(); + } + + private Set<StorageOperation> resolveAllowedOperations(TableIdentifier identifier) { + Preconditions.checkArgument(identifier.namespace().levels().length == 1); + final var database = identifier.namespace().level(0); + final var table = identifier.name(); + + final var authorizer = authorizerSupplier.get(); + final var allowedOperations = EnumSet.noneOf(StorageOperation.class); + if (isReadable(authorizer, database, table)) { + allowedOperations.add(StorageOperation.LIST); + allowedOperations.add(StorageOperation.READ); + } + if (isWritable(authorizer, database, table)) { + allowedOperations.add(StorageOperation.CREATE); + allowedOperations.add(StorageOperation.DELETE); + } + + return allowedOperations; + } + + // Check if the user has the SELECT permission + private boolean isReadable(HiveAuthorizer authorizer, String database, String table) { + final var object = new HivePrivilegeObject( + HivePrivilegeObject.HivePrivilegeObjectType.TABLE_OR_VIEW, + catalog, + database, + table + ); + return isAllowed(authorizer, Collections.singletonList(object), Collections.emptyList()); + } + + // Check if the user has the INSERT INTO permission + private boolean isWritable(HiveAuthorizer authorizer, String database, String table) { + final var object = new HivePrivilegeObject( + HivePrivilegeObject.HivePrivilegeObjectType.TABLE_OR_VIEW, + catalog, + database, + table, + null, + null, + HivePrivilegeObject.HivePrivObjectActionType.INSERT, + null); + return isAllowed(authorizer, Collections.emptyList(), Collections.singletonList(object)); + } + + private boolean isAllowed(HiveAuthorizer authorizer, List<HivePrivilegeObject> input, + List<HivePrivilegeObject> output) { + var context = new HiveAuthzContext.Builder().build(); + try { + authorizer.checkPrivileges(HiveOperationType.QUERY, input, output, context); Review Comment: +1 on making this a single lookup. Ranger already returns all of a user's privileges on a resource in one call (`getHiveResourceACLs`/`showPrivileges`), and the result can be cached per (user, table) for a short time. That matters because `loadTable` is the hot path and can now trigger two or three `checkPrivileges` round trips. Could the privilege lookup ("which of read/write does this user have") sit behind a small seam that can be replaced or wrapped with a cache, while the row-filter/column-masking refusal stays fixed in the facade and can't be bypassed? That way a faster lookup can't silently drop the fine-grained access check. ########## standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java: ########## @@ -468,6 +485,33 @@ private LoadViewResponse registerView(Map<String, String> vars, Object body) { return CatalogHandlers.registerView(asViewCatalog, namespace, request); } + private LoadTableResponse withCredentials( + Set<AccessDelegationMode> accessDelegationModes, + TableIdentifier ident, + LoadTableResponse response) { + if (credentialProvider == null) { + return response; + } + + if (accessDelegationModes.contains(AccessDelegationMode.VENDED_CREDENTIALS)) { + return withVendedCredentials(ident, response); + } + + if (accessDelegationModes.contains(AccessDelegationMode.REMOTE_SIGNING)) { + LOG.warn("Remote signing is not supported. Ignoring..."); + } + + return response; + } + + private LoadTableResponse withVendedCredentials(TableIdentifier ident, LoadTableResponse response) { + final var credentials = credentialProvider.vend(catalogName, ident, response.tableMetadata()); Review Comment: `vend(...)` is given `response.tableMetadata()`, so the storage request is derived from whatever metadata is returned to the client. That works as long as the client and the provider see the same location, but some deployments put a gateway in front of storage. For example, an Ozone table stored as `ofs://…` can be exposed to REST clients as `s3a://…` through S3 Gateway, with the metadata rewritten in the response. In that case the provider has to scope the credential against the stored location, while the credential's `prefix` has to be the address the client actually uses. Could we: 1. derive the `StorageAccessRequest` from the catalog's table/metadata before any response-side transformation, and 2. document on `VendedStorageCredential` that `prefix` is the client-facing prefix and may differ from `StorageAccessRequest.location()`? Nothing in the current code prevents (2), but making it part of the SPI contract would let gateway-style providers rely on it. ########## standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/IcebergAuthorizer.java: ########## @@ -234,6 +239,36 @@ void validateDropTablePurge(String catalogName, TableIdentifier identifier, Stri } } + /** + * Translates Hive privileges into storage operations. + * + * @param catalogName the catalog name + * @param identifier the table identifier + * @param columnNames the column names + * @return a set of acceptable storage operations + * @throws IllegalStateException if the authorization plugin fails + */ + Set<StorageOperation> resolveAllowedStorageOperations(String catalogName, TableIdentifier identifier, + List<String> columnNames) { + Preconditions.checkArgument(identifier.namespace().levels().length == 1); + final var database = identifier.namespace().level(0); + final var table = identifier.name(); + + final var authorizer = authorizerSupplier.get(); + if (authorizer == null) { + LOG.info("No pre-event listener is configured, skipping credential-vending authorization"); + return EnumSet.allOf(StorageOperation.class); Review Comment: When no pre-event listener/authorizer is configured, `resolveAllowedStorageOperations` returns `EnumSet.allOf(StorageOperation.class)`, so turning on vended credentials without authorization hands out read-write storage credentials to every authenticated caller. Skipping metadata-level checks in that setup is a reasonable compatibility default, but handing out credentials is a much bigger grant. Could this return `EnumSet.noneOf(...)` by default, with an explicit opt-in (e.g. `...vended-credentials.allow-without-authorizer=true`) for test or dev setups? ########## standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/credential/VendedCredentialProvider.java: ########## @@ -0,0 +1,47 @@ +/* + * 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.hadoop.hive.metastore.credential; + +import org.apache.hadoop.classification.InterfaceStability; + +import java.util.List; + +/** + * A credential-vending service. + */ [email protected] +public interface VendedCredentialProvider { + /** + * Checks whether this provider supports the given access request. + * + * @param request the access request + * @return true if this provider supports the given access request + */ + boolean supports(StorageAccessRequest request); + + /** + * Vends credentials for the given access. + * + * @param username the authenticated username + * @param accessRequests the vending requests + * @return a list of vended credentials + */ + List<VendedStorageCredential> vend(String username, List<StorageAccessRequest> accessRequests); Review Comment: Since the SPI is `@Unstable`, now is the cheapest time to widen it. A `String` short name is enough for STS role-session naming, but token-exchange style providers (Knox IDBroker, Azure on-behalf-of, OAuth token exchange) may need the full principal, the groups, or the caller's inbound token. Would a small context record, e.g. `VendingContext(UserGroupInformation ugi, String catalog, TableIdentifier ident)`, work? It also lets a provider log or audit against the table identity rather than just a path. Related: `IcebergVendedCredentialProvider` uses `getShortUserName()`, while the authorizer side resolves the user from the UGI. It would be good to make sure both sides agree on which user name they use. -- 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]
