eric-maynard commented on code in PR #1305: URL: https://github.com/apache/polaris/pull/1305#discussion_r2036265816
########## polaris-core/src/main/java/org/apache/polaris/core/connection/AuthenticationParametersDpo.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.connection; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import jakarta.annotation.Nonnull; +import java.util.Map; +import org.apache.polaris.core.admin.model.AuthenticationParameters; +import org.apache.polaris.core.admin.model.BearerAuthenticationParameters; +import org.apache.polaris.core.admin.model.OAuthClientCredentialsParameters; +import org.apache.polaris.core.secrets.UserSecretReference; + +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "authenticationType", visible = true) +@JsonSubTypes({ + @JsonSubTypes.Type(value = OAuthClientCredentialsParametersDpo.class, name = "OAUTH"), + @JsonSubTypes.Type(value = BearerAuthenticationParametersDpo.class, name = "BEARER"), +}) +public abstract class AuthenticationParametersDpo implements IcebergCatalogPropertiesProvider { + + public static final String INLINE_CLIENT_SECRET_REFERENCE_KEY = "inlineClientSecretReference"; + public static final String INLINE_BEARER_TOKEN_REFERENCE_KEY = "inlineBearerTokenReference"; + + @JsonProperty(value = "authenticationType") + private final AuthenticationType authenticationType; + + public AuthenticationParametersDpo( + @JsonProperty(value = "authenticationType", required = true) @Nonnull + AuthenticationType authenticationType) { + this.authenticationType = authenticationType; + } + + public @Nonnull AuthenticationType getAuthenticationType() { + return authenticationType; + } + + public abstract AuthenticationParameters asAuthenticationParametersModel(); + + public static AuthenticationParametersDpo fromAuthenticationParametersModelWithSecrets( + AuthenticationParameters authenticationParameters, + Map<String, UserSecretReference> secretReferences) { + AuthenticationParametersDpo config = null; Review Comment: nit: `final AuthenticationParametersDpo config` ########## polaris-core/src/main/java/org/apache/polaris/core/connection/ConnectionConfigInfoDpo.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.connection; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.annotation.Nonnull; +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URL; +import java.util.Map; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.admin.model.ConnectionConfigInfo; +import org.apache.polaris.core.admin.model.IcebergRestConnectionConfigInfo; +import org.apache.polaris.core.secrets.UserSecretReference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "connectionType", visible = true) +@JsonSubTypes({ + @JsonSubTypes.Type(value = IcebergRestConnectionConfigInfoDpo.class, name = "ICEBERG_REST"), +}) +public abstract class ConnectionConfigInfoDpo implements IcebergCatalogPropertiesProvider { + private static final Logger logger = LoggerFactory.getLogger(ConnectionConfigInfoDpo.class); + + // The type of the connection + private final ConnectionType connectionType; + + // The URI of the remote catalog + private final String uri; + + // The authentication parameters for the connection + private final AuthenticationParametersDpo authenticationParameters; + + public ConnectionConfigInfoDpo( + @JsonProperty(value = "connectionType", required = true) @Nonnull + ConnectionType connectionType, + @JsonProperty(value = "uri", required = true) @Nonnull String uri, + @JsonProperty(value = "authenticationParameters", required = true) @Nonnull + AuthenticationParametersDpo authenticationParameters) { + this(connectionType, uri, authenticationParameters, true); + } + + protected ConnectionConfigInfoDpo( + @Nonnull ConnectionType connectionType, + @Nonnull String uri, + @Nonnull AuthenticationParametersDpo authenticationParameters, + boolean validateUri) { + this.connectionType = connectionType; + this.uri = uri; + this.authenticationParameters = authenticationParameters; + if (validateUri) { + validateUri(uri); + } + } + + public ConnectionType getConnectionType() { + return connectionType; + } + + public String getUri() { + return uri; + } + + public AuthenticationParametersDpo getAuthenticationParameters() { + return authenticationParameters; + } + + private static final ObjectMapper DEFAULT_MAPPER; + + static { + DEFAULT_MAPPER = new ObjectMapper(); + DEFAULT_MAPPER.setSerializationInclusion(JsonInclude.Include.NON_NULL); + DEFAULT_MAPPER.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + } + + public String serialize() { + try { + return DEFAULT_MAPPER.writeValueAsString(this); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + public static ConnectionConfigInfoDpo deserialize( Review Comment: nit: @Nullable ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ########## @@ -383,7 +397,13 @@ public PolarisResolvedPathWrapper getResolvedPath( if (resolvedPath == null) { return null; } - if (resolvedPath.getRawLeafEntity() != null + // In the case of a passthrough facade, we may have only resolved part of the parent path + // in which case the subtype wouldn't match; return the path anyways in this case. + // + // TODO: Reconcile how we'll handle "TABLE_NOT_FOUND" or "VIEW_NOT_FOUND" semantics Review Comment: In what case do we not just forward the return code from the remote catalog? ########## polaris-core/src/main/java/org/apache/polaris/core/connection/AuthenticationType.java: ########## @@ -0,0 +1,24 @@ +/* + * 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.connection; + +public enum AuthenticationType { Review Comment: For the sake of wire/serialized compatibility perhaps we should specify the int values here like we do for some other enums ########## polaris-core/src/main/java/org/apache/polaris/core/connection/IcebergCatalogPropertiesProvider.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.connection; + +import jakarta.annotation.Nonnull; +import java.util.Map; +import org.apache.polaris.core.secrets.UserSecretsManager; + +public interface IcebergCatalogPropertiesProvider { + @Nonnull + Map<String, String> asIcebergCatalogProperties(UserSecretsManager secretsManager); Review Comment: I wonder why this is not just part of the functionality of `UserSecretsManager`... can you add more color here? ########## polaris-core/src/main/java/org/apache/polaris/core/connection/IcebergCatalogPropertiesProvider.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.connection; + +import jakarta.annotation.Nonnull; +import java.util.Map; +import org.apache.polaris.core.secrets.UserSecretsManager; + +public interface IcebergCatalogPropertiesProvider { + @Nonnull + Map<String, String> asIcebergCatalogProperties(UserSecretsManager secretsManager); Review Comment: maybe in the javadoc :) ########## polaris-core/src/main/java/org/apache/polaris/core/secrets/UserSecretsManager.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.secrets; + +import jakarta.annotation.Nonnull; +import org.apache.polaris.core.entity.PolarisEntity; + +/** + * Manages secrets specified by users of the Polaris API, either directly or as an intermediary + * layer between Polaris and external secret-management systems. Such secrets are distinct from + * "service-level" secrets that pertain to the Polaris service itself which would be more statically + * configured system-wide. In contrast, user-owned secrets are handled dynamically as part of + * runtime API requests. + */ +public interface UserSecretsManager { + /** + * Persist the {@code secret} under a new URN {@code secretUrn} and return a {@code + * UserSecretReference} that can subsequently be used by this same UserSecretsManager to retrieve + * the original secret. The {@code forEntity} is provided for an implementation to optionally + * extract other identifying metadata such as entity type, name, etc., to store alongside the + * remotely stored secret to facilitate operational management of the secrets outside of the core Review Comment: If it's not expected that the USM will always persist the `PolarisEntity`, can we get a version of this method that doesn't take one? ########## polaris-core/src/main/java/org/apache/polaris/core/secrets/UserSecretReference.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.secrets; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.MoreObjects; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; + +/** + * Represents a "wrapped reference" to a user-owned secret that holds an identifier to retrieve + * possibly remotely-stored secret material, along with an open-ended "referencePayload" that is + * specific to an implementation of the secret storage and which is needed "unwrap" the actual + * secret in combination with whatever is stored in the remote secrets storage. Review Comment: If the URN is the "identifier" referred to in `UserSecretsManager` but we also pass along a`Map<String, String>` everywhere, do we actually need the URN? Since we're not specifying how the `UserSecretReference`'s URN & `referencePayload` are meant to be treated... i.e. it seems like I could satisfy the existing interfaces with a working implementation that totally ignores the `urn` field. ########## polaris-core/src/main/java/org/apache/polaris/core/connection/ConnectionType.java: ########## @@ -0,0 +1,23 @@ +/* + * 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.connection; + +public enum ConnectionType { Review Comment: ditto the other comment about enums ########## polaris-core/src/main/java/org/apache/polaris/core/connection/ConnectionConfigInfoDpo.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.connection; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.annotation.Nonnull; +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URL; +import java.util.Map; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.admin.model.ConnectionConfigInfo; +import org.apache.polaris.core.admin.model.IcebergRestConnectionConfigInfo; +import org.apache.polaris.core.secrets.UserSecretReference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "connectionType", visible = true) +@JsonSubTypes({ + @JsonSubTypes.Type(value = IcebergRestConnectionConfigInfoDpo.class, name = "ICEBERG_REST"), +}) +public abstract class ConnectionConfigInfoDpo implements IcebergCatalogPropertiesProvider { Review Comment: There are some missing javadocs ########## polaris-core/src/main/java/org/apache/polaris/core/secrets/UserSecretReference.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.secrets; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.MoreObjects; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; + +/** + * Represents a "wrapped reference" to a user-owned secret that holds an identifier to retrieve + * possibly remotely-stored secret material, along with an open-ended "referencePayload" that is + * specific to an implementation of the secret storage and which is needed "unwrap" the actual + * secret in combination with whatever is stored in the remote secrets storage. + * + * <p>Example scenarios: + * + * <p>If an implementation simply stores secrets directly in the secrets manager, the + * referencePayload may be empty and "unwrapping" would be a simple identity/no-op transformation. + * + * <p>If tampering or corruption of secrets in the secrets manager presents a unique threat, an + * implementation may use the referencePayload to ensure data integrity of the secret by storing a + * checksum or hash of the stored secret. + * + * <p>If the system must protect against independent exfiltration/attacks on a dedicated secrets + * manager and the core persistence database, the referencePayload may be used to coordinate + * secondary encryption keys such that the original secret can only be fully "unwrapped" given both + * the stored "secret material" as well as the referencePayload and any associated keys used for + * encryption. + */ +public class UserSecretReference { + @JsonProperty(value = "urn") + private final String urn; + + @JsonProperty(value = "referencePayload") + private final Map<String, String> referencePayload; + + public UserSecretReference( + @JsonProperty(value = "urn", required = true) @Nonnull String urn, + @JsonProperty(value = "referencePayload") @Nullable Map<String, String> referencePayload) { + this.urn = urn; + this.referencePayload = Objects.requireNonNullElse(referencePayload, new HashMap<>()); + } + + public @Nonnull String getUrn() { + return urn; + } + + public @Nonnull Map<String, String> getReferencePayload() { + return referencePayload; + } + + @Override + public int hashCode() { + return Objects.hashCode(getUrn()) ^ Objects.hashCode(getReferencePayload()); Review Comment: Maybe just use `MoreObjects.hashCode` since we use the lib elsewhere here ########## polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java: ########## @@ -190,6 +202,21 @@ public Catalog.TypeEnum getCatalogType() { .orElse(null); } + public boolean isPassthroughFacade() { Review Comment: It seems like we are piggybacking on the existing EXTERNAL concept even though the semantics are very different. For example, it'll be invalid for a passthrough catalog to receive a notification right? ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ########## @@ -270,8 +319,8 @@ public LoadTableResponse createTableDirect(Namespace namespace, CreateTableReque .getResolvedReferenceCatalogEntity() .getResolvedLeafEntity() .getEntity()); - if (isExternal(catalog)) { - throw new BadRequestException("Cannot create table on external catalogs."); + if (isStaticFacade(catalog)) { Review Comment: Oh are we just getting rid of the current concept of external catalogs altogether? ########## polaris-core/src/main/java/org/apache/polaris/core/secrets/UserSecretsManager.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.secrets; + +import jakarta.annotation.Nonnull; +import org.apache.polaris.core.entity.PolarisEntity; + +/** + * Manages secrets specified by users of the Polaris API, either directly or as an intermediary + * layer between Polaris and external secret-management systems. Such secrets are distinct from + * "service-level" secrets that pertain to the Polaris service itself which would be more statically + * configured system-wide. In contrast, user-owned secrets are handled dynamically as part of + * runtime API requests. + */ +public interface UserSecretsManager { + /** + * Persist the {@code secret} under a new URN {@code secretUrn} and return a {@code + * UserSecretReference} that can subsequently be used by this same UserSecretsManager to retrieve + * the original secret. The {@code forEntity} is provided for an implementation to optionally + * extract other identifying metadata such as entity type, name, etc., to store alongside the + * remotely stored secret to facilitate operational management of the secrets outside of the core + * Polaris service (for example, to perform garbage-collection if the Polaris service fails to + * delete managed secrets in the external system when associated entities are deleted. + * + * @param secret The secret to store + * @param forEntity The PolarisEntity that is associated with the secret + * @return A reference object that can be used to retrieve the secret which is safe to store in + * its entirety within a persisted PolarisEntity + */ + @Nonnull + UserSecretReference writeSecret(@Nonnull String secret, @Nonnull PolarisEntity forEntity); + + /** + * Retrieve a secret using the {@code secretReference}. + * + * @param secretReference Identifier and any associated payload used for retrieving the secret Review Comment: I thought the USR itself was the identifier, can you add a bit more detail here? What is `any associated payload`? -- 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]
