collado-mike commented on code in PR #389:
URL: https://github.com/apache/polaris/pull/389#discussion_r1950073614
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialProperty.java:
##########
@@ -23,6 +23,8 @@ public enum PolarisCredentialProperty {
AWS_KEY_ID(String.class, "s3.access-key-id", "the aws access key id"),
AWS_SECRET_KEY(String.class, "s3.secret-access-key", "the aws access key
secret"),
AWS_TOKEN(String.class, "s3.session-token", "the aws scoped access token"),
+ AWS_ENDPOINT(String.class, "s3.endpoint", "the aws s3 endpoint"),
+ AWS_PATH_STYLE_ACCESS(Boolean.class, "s3.path-style-access", "the aws s3
path style access"),
Review Comment:
Typically, these are catalog-level properties, not credentials. I think it's
best to avoid overloading the credential providers with the need to populate
these configuration properties.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/s3compatible/S3CompatibleStorageConfigurationInfo.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.polaris.core.storage.s3compatible;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.MoreObjects;
+import java.util.List;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Polaris Storage Configuration information for an S3 Compatible solution,
MinIO, Ceph, Dell ECS...
+ */
+public class S3CompatibleStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
+
+ // 5 is the approximate max allowed locations for the size of AccessPolicy
when LIST is required
+ // for allowed read and write locations for subscoping creds.
+ @JsonIgnore private static final int MAX_ALLOWED_LOCATIONS = 5;
+ private @NotNull String s3Endpoint;
+ private @Nullable String s3CredentialsCatalogAccessKeyId;
+ private @Nullable String s3CredentialsCatalogSecretAccessKey;
+ private @Nullable Boolean s3PathStyleAccess;
+ private @NotNull Boolean skipCredentialSubscopingIndirection;
+ private @Nullable String s3CredentialsClientAccessKeyId;
+ private @Nullable String s3CredentialsClientSecretAccessKey;
Review Comment:
What is the distinction between `s3CredentialsCatalogAccessKeyId` and
`s3CredentialsClientAccessKeyId`? Rather than having users set the names of
environment variables, can't we use AWS profiles? That's a heck of a lot safer
than allowing users to put the name of _any_ environment variable and read it
directly from the server.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * 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.polaris.core.storage.s3compatible;
+
+import jakarta.annotation.Nonnull;
+import java.net.URI;
+import java.util.EnumMap;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.storage.InMemoryStorageIntegration;
+import org.apache.polaris.core.storage.PolarisCredentialProperty;
+import org.apache.polaris.core.storage.StorageUtil;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import software.amazon.awssdk.policybuilder.iam.IamConditionOperator;
+import software.amazon.awssdk.policybuilder.iam.IamEffect;
+import software.amazon.awssdk.policybuilder.iam.IamPolicy;
+import software.amazon.awssdk.policybuilder.iam.IamResource;
+import software.amazon.awssdk.policybuilder.iam.IamStatement;
+import software.amazon.awssdk.services.sts.StsClient;
+import software.amazon.awssdk.services.sts.StsClientBuilder;
+import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
+import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
+
+/** S3 compatible implementation of PolarisStorageIntegration */
+public class S3CompatibleCredentialsStorageIntegration
+ extends InMemoryStorageIntegration<S3CompatibleStorageConfigurationInfo> {
+ private static final Logger LOGGER =
+ LoggerFactory.getLogger(S3CompatibleCredentialsStorageIntegration.class);
+ private StsClient stsClient;
+ private String caI, caS; // catalogAccessKeyId & catalogSecretAccessKey
+ private String clI, clS; // clientAccessKeyId & clientSecretAccessKey
+
+ public S3CompatibleCredentialsStorageIntegration() {
+ super(S3CompatibleCredentialsStorageIntegration.class.getName());
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds(
+ @NotNull PolarisDiagnostics diagnostics,
+ @NotNull S3CompatibleStorageConfigurationInfo storageConfig,
+ boolean allowListOperation,
+ @NotNull Set<String> allowedReadLocations,
+ @NotNull Set<String> allowedWriteLocations) {
+
+ caI = System.getenv(storageConfig.getS3CredentialsCatalogAccessKeyId());
+ caS =
System.getenv(storageConfig.getS3CredentialsCatalogSecretAccessKey());
+
+ EnumMap<PolarisCredentialProperty, String> propertiesMap =
+ new EnumMap<>(PolarisCredentialProperty.class);
+ propertiesMap.put(PolarisCredentialProperty.AWS_ENDPOINT,
storageConfig.getS3Endpoint());
+ propertiesMap.put(
+ PolarisCredentialProperty.AWS_PATH_STYLE_ACCESS,
+ storageConfig.getS3PathStyleAccess().toString());
+ if (storageConfig.getS3Region() != null) {
+ propertiesMap.put(PolarisCredentialProperty.CLIENT_REGION,
storageConfig.getS3Region());
+ }
+
+ if (storageConfig.getSkipCredentialSubscopingIndirection() == true) {
+ LOGGER.debug("S3Compatible - skipCredentialSubscopingIndirection !");
+ clI = System.getenv(storageConfig.getS3CredentialsClientAccessKeyId());
+ clS =
System.getenv(storageConfig.getS3CredentialsClientSecretAccessKey());
Review Comment:
This is not safe. Now, if I have privilege to create a catalog, I can simply
set this `skipCredentialSubcopingIndirection` flag and I immediately get access
to the raw credentials used to assume any role for any other catalog in the
service?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.polaris.core.storage.s3compatible;
+
+import java.net.URI;
+import java.util.EnumMap;
+import java.util.Set;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.storage.InMemoryStorageIntegration;
+import org.apache.polaris.core.storage.PolarisCredentialProperty;
+import
org.apache.polaris.core.storage.s3compatible.S3CompatibleStorageConfigurationInfo.CredsCatalogAndClientStrategyEnum;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import software.amazon.awssdk.regions.Region;
+import software.amazon.awssdk.services.sts.StsClient;
+import software.amazon.awssdk.services.sts.StsClientBuilder;
+import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
+import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
+
+/** Credential vendor that supports generating */
+public class S3CompatibleCredentialsStorageIntegration
+ extends InMemoryStorageIntegration<S3CompatibleStorageConfigurationInfo> {
+ private static final Logger LOGGER =
+ LoggerFactory.getLogger(S3CompatibleCredentialsStorageIntegration.class);
+ private StsClient stsClient;
+
+ public S3CompatibleCredentialsStorageIntegration() {
+ super(S3CompatibleCredentialsStorageIntegration.class.getName());
+ }
+
+ public void createStsClient(S3CompatibleStorageConfigurationInfo
s3storageConfig) {
+
+ LOGGER.debug("S3Compatible - createStsClient()");
+ StsClientBuilder stsBuilder =
software.amazon.awssdk.services.sts.StsClient.builder();
+ stsBuilder.region(
+ Region
+ .US_WEST_1); // default region to avoid bug, because most (all?)
S3 compatible softwares
+ // do not care about regions
+ stsBuilder.endpointOverride(
+ URI.create(
+ s3storageConfig
+ .getS3Endpoint())); // S3Compatible - AWS STS endpoint is
unique and different from
+ // the S3 Endpoint
+ stsBuilder.credentialsProvider(
+ StaticCredentialsProvider.create(
+ AwsBasicCredentials.create(
+ s3storageConfig.getS3CredentialsCatalogAccessKeyId(),
+ s3storageConfig.getS3CredentialsCatalogSecretAccessKey())));
+ this.stsClient = stsBuilder.build();
+ LOGGER.debug("S3Compatible - stsClient successfully built");
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds(
+ @NotNull PolarisDiagnostics diagnostics,
+ @NotNull S3CompatibleStorageConfigurationInfo storageConfig,
+ boolean allowListOperation,
+ @NotNull Set<String> allowedReadLocations,
+ @NotNull Set<String> allowedWriteLocations) {
+
+ LOGGER.debug("S3Compatible - getSubscopedCreds - applying credential
strategy");
+ EnumMap<PolarisCredentialProperty, String> propertiesMap =
+ new EnumMap<>(PolarisCredentialProperty.class);
+ propertiesMap.put(PolarisCredentialProperty.AWS_ENDPOINT,
storageConfig.getS3Endpoint());
+ propertiesMap.put(
+ PolarisCredentialProperty.AWS_PATH_STYLE_ACCESS,
+ storageConfig.getS3PathStyleAccess().toString());
Review Comment:
I don't understand why you need them in this class at all. The catalog
properties are used to initialize the FileIO in `BasePolarisCatalog`. The
properties from this class are appended to those properties.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * 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.polaris.core.storage.s3compatible;
+
+import jakarta.annotation.Nonnull;
+import java.net.URI;
+import java.util.EnumMap;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.storage.InMemoryStorageIntegration;
+import org.apache.polaris.core.storage.PolarisCredentialProperty;
+import org.apache.polaris.core.storage.StorageUtil;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import software.amazon.awssdk.policybuilder.iam.IamConditionOperator;
+import software.amazon.awssdk.policybuilder.iam.IamEffect;
+import software.amazon.awssdk.policybuilder.iam.IamPolicy;
+import software.amazon.awssdk.policybuilder.iam.IamResource;
+import software.amazon.awssdk.policybuilder.iam.IamStatement;
+import software.amazon.awssdk.services.sts.StsClient;
+import software.amazon.awssdk.services.sts.StsClientBuilder;
+import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
+import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
+
+/** S3 compatible implementation of PolarisStorageIntegration */
+public class S3CompatibleCredentialsStorageIntegration
+ extends InMemoryStorageIntegration<S3CompatibleStorageConfigurationInfo> {
+ private static final Logger LOGGER =
+ LoggerFactory.getLogger(S3CompatibleCredentialsStorageIntegration.class);
+ private StsClient stsClient;
+ private String caI, caS; // catalogAccessKeyId & catalogSecretAccessKey
+ private String clI, clS; // clientAccessKeyId & clientSecretAccessKey
+
+ public S3CompatibleCredentialsStorageIntegration() {
+ super(S3CompatibleCredentialsStorageIntegration.class.getName());
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds(
+ @NotNull PolarisDiagnostics diagnostics,
+ @NotNull S3CompatibleStorageConfigurationInfo storageConfig,
+ boolean allowListOperation,
+ @NotNull Set<String> allowedReadLocations,
+ @NotNull Set<String> allowedWriteLocations) {
+
+ caI = System.getenv(storageConfig.getS3CredentialsCatalogAccessKeyId());
+ caS =
System.getenv(storageConfig.getS3CredentialsCatalogSecretAccessKey());
+
+ EnumMap<PolarisCredentialProperty, String> propertiesMap =
+ new EnumMap<>(PolarisCredentialProperty.class);
+ propertiesMap.put(PolarisCredentialProperty.AWS_ENDPOINT,
storageConfig.getS3Endpoint());
+ propertiesMap.put(
+ PolarisCredentialProperty.AWS_PATH_STYLE_ACCESS,
+ storageConfig.getS3PathStyleAccess().toString());
+ if (storageConfig.getS3Region() != null) {
+ propertiesMap.put(PolarisCredentialProperty.CLIENT_REGION,
storageConfig.getS3Region());
+ }
+
+ if (storageConfig.getSkipCredentialSubscopingIndirection() == true) {
+ LOGGER.debug("S3Compatible - skipCredentialSubscopingIndirection !");
+ clI = System.getenv(storageConfig.getS3CredentialsClientAccessKeyId());
+ clS =
System.getenv(storageConfig.getS3CredentialsClientSecretAccessKey());
+ if (clI != null && clS != null) {
+ propertiesMap.put(PolarisCredentialProperty.AWS_KEY_ID, clI);
+ propertiesMap.put(PolarisCredentialProperty.AWS_SECRET_KEY, clS);
+ } else {
+ propertiesMap.put(PolarisCredentialProperty.AWS_KEY_ID, caI);
+ propertiesMap.put(PolarisCredentialProperty.AWS_SECRET_KEY, caS);
+ }
+ } else {
+ LOGGER.debug("S3Compatible - assumeRole !");
+ createStsClient(storageConfig);
+ AssumeRoleResponse response =
+ stsClient.assumeRole(
+ AssumeRoleRequest.builder()
+ .roleSessionName("PolarisCredentialsSTS")
+ .roleArn(
+ (storageConfig.getS3RoleArn() == null) ? "" :
storageConfig.getS3RoleArn())
+ .policy(
+ policyString(allowListOperation, allowedReadLocations,
allowedWriteLocations)
+ .toJson())
+ .build());
+
+ propertiesMap.put(PolarisCredentialProperty.AWS_KEY_ID,
response.credentials().accessKeyId());
+ propertiesMap.put(
+ PolarisCredentialProperty.AWS_SECRET_KEY,
response.credentials().secretAccessKey());
+ propertiesMap.put(PolarisCredentialProperty.AWS_TOKEN,
response.credentials().sessionToken());
+ LOGGER.debug(
+ "S3Compatible - assumeRole - Token Expiration at : {}",
+ response.credentials().expiration().toString());
+ }
+
+ return propertiesMap;
+ }
+
+ public void createStsClient(S3CompatibleStorageConfigurationInfo
storageConfig) {
+ LOGGER.debug("S3Compatible - createStsClient()");
+ try {
+ StsClientBuilder stsBuilder =
software.amazon.awssdk.services.sts.StsClient.builder();
+ stsBuilder.endpointOverride(URI.create(storageConfig.getS3Endpoint()));
Review Comment:
If I read this right, this is the only code that really needs to deviate
from the default S3 credential vending process, right? We could realistically
refactor the default STS client to be provided by a factory that takes in the
storage config. Then we don't need all this duplication
--
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]
