Copilot commented on code in PR #12366:
URL: https://github.com/apache/gravitino/pull/12366#discussion_r3733024878
##########
core/src/main/java/org/apache/gravitino/secret/SecretManager.java:
##########
@@ -63,20 +65,86 @@ public SecretProviderRegistry getRegistry() {
return registry;
}
+ /**
+ * Ensures each property key appears at most once across {@code properties},
{@code
+ * secretBindings}, and {@code secretReferences}.
+ *
+ * <p>{@code null} maps are treated as empty.
+ *
+ * @param properties entity properties from the create request (may be null)
+ * @param secretBindings property key → write-through binding (may be null)
+ * @param secretReferences property key → secret locator (may be null)
+ */
+ public void checkSecretKeys(
+ @Nullable Map<String, String> properties,
+ @Nullable Map<String, SecretBinding> secretBindings,
+ @Nullable Map<String, SecretReference> secretReferences) {
+ Set<String> keys = new HashSet<>();
+ int count = 0;
+ if (properties != null) {
+ keys.addAll(properties.keySet());
+ count += properties.size();
+ }
Review Comment:
Create-time `properties` are never checked for raw secret URNs. A caller can
therefore submit another entity's URN as (for example) `jdbc-password`;
`createBaseCatalog` later resolves it and gives the plaintext to the connector,
bypassing the typed reference path and any ownership validation. Reject
matching secret URN values in the request properties before assembling
server-generated URNs.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -699,6 +715,7 @@ public void testConnection(
}
Map<String, String> mergedConfig = buildCatalogConf(provider,
properties);
+ Map<String, String> plaintextConfig =
secretManager.toPlaintextProperties(mergedConfig);
Review Comment:
`testConnection` receives caller-controlled properties but now resolves any
matching Gravitino secret URN. A caller who knows another entity's URN can
combine it with an attacker-controlled connection endpoint and cause the
connector to send that secret externally. Do not resolve raw URNs on this
request path; only resolve URNs loaded from trusted persisted metadata or add
an authorized typed-secret flow.
##########
core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java:
##########
@@ -77,9 +79,13 @@ public Catalog createCatalog(
Catalog.Type type,
String provider,
String comment,
- Map<String, String> properties)
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchMetalakeException, CatalogAlreadyExistsException {
- Catalog catalog = dispatcher.createCatalog(ident, type, provider, comment,
properties);
+ Catalog catalog =
+ dispatcher.createCatalog(
+ ident, type, provider, comment, properties, secretBindings,
secretReferences);
Review Comment:
The delegated create writes the new secrets before returning. If owner or
future-grant setup then fails, the catch block drops the catalog, but
`dropCatalog` does not delete these write-through secrets, leaving orphaned
secret material after a failed create. Include secret rollback in this
post-hook rollback path (or make drop clean the catalog's secret URNs).
##########
clients/client-python/gravitino/api/secret.py:
##########
@@ -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.
+
+from dataclasses import dataclass, field
+from typing import Dict
+
+
+@dataclass
+class SecretBinding:
+ """Write-through secret binding: provider instance name plus plaintext."""
+
+ provider: str
+ plaintext: str
+
+ def __repr__(self) -> str:
+ return f"SecretBinding(provider={self.provider!r}, plaintext=***)"
+
+
+@dataclass
+class SecretReference:
+ """External secret locator: provider instance name plus provider-specific
attributes."""
+
+ provider: str
+ attributes: Dict[str, str] = field(default_factory=dict)
+
+ def __post_init__(self):
+ if not self.attributes:
+ raise ValueError("attributes must not be null or empty")
Review Comment:
This rejects the documented valid case where `attributes` is omitted or
empty. It also conflicts with the field's `default_factory=dict`, so
`SecretReference(provider="...")` always raises. Reject only `None` and allow
an empty dictionary.
##########
common/src/main/java/org/apache/gravitino/dto/secret/SecretBindingDTO.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.gravitino.dto.secret;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.ToString;
+import org.apache.gravitino.secret.SecretBinding;
+
+/** Data transfer object for a write-through {@link SecretBinding}. */
+@Getter
+@EqualsAndHashCode
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@AllArgsConstructor
+@Builder(setterPrefix = "with")
+@ToString(exclude = "plaintext")
+public class SecretBindingDTO {
+
+ @JsonProperty("provider")
+ private String provider;
+
+ @JsonProperty("plaintext")
+ private String plaintext;
+
+ /**
+ * Converts this DTO to a {@link SecretBinding}.
+ *
+ * @return the secret binding
+ */
+ public SecretBinding toSecretBinding() {
+ return new SecretBinding(provider, plaintext);
+ }
+
+ /**
+ * Creates a DTO from a {@link SecretBinding}.
+ *
+ * @param binding the secret binding
+ * @return the secret binding DTO
+ */
+ public static SecretBindingDTO fromSecretBinding(SecretBinding binding) {
+ return new SecretBindingDTO(binding.provider(), binding.plaintext());
+ }
+
+ /**
+ * Converts a property-key map of DTOs to {@link SecretBinding}s.
+ *
+ * @param dtos property key → binding DTO; {@code null} or empty returns an
empty map
+ * @return property key → binding (never {@code null})
+ */
+ public static Map<String, SecretBinding> toSecretBindings(
+ @Nullable Map<String, SecretBindingDTO> dtos) {
+ if (dtos == null || dtos.isEmpty()) {
+ return ImmutableMap.of();
+ }
+ ImmutableMap.Builder<String, SecretBinding> bindings =
ImmutableMap.builder();
+ for (Map.Entry<String, SecretBindingDTO> entry : dtos.entrySet()) {
+ bindings.put(entry.getKey(), entry.getValue().toSecretBinding());
+ }
Review Comment:
A JSON map entry such as `"secretBindings": {"password": null}` is
dereferenced here and throws `NullPointerException`, which the REST handlers
treat as an internal error rather than a bad request. Validate null DTO values
and throw `IllegalArgumentException` with the offending key.
##########
common/src/main/java/org/apache/gravitino/dto/secret/SecretReferenceDTO.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.gravitino.dto.secret;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.ToString;
+import org.apache.gravitino.secret.SecretReference;
+
+/** Data transfer object for an external {@link SecretReference}. */
+@Getter
+@EqualsAndHashCode
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@AllArgsConstructor
+@Builder(setterPrefix = "with")
+@ToString
+public class SecretReferenceDTO {
+
+ @JsonProperty("provider")
+ private String provider;
+
+ @Builder.Default
+ @JsonProperty("attributes")
+ private Map<String, String> attributes = ImmutableMap.of();
+
+ /**
+ * Converts this DTO to a {@link SecretReference}.
+ *
+ * @return the secret reference
+ */
+ public SecretReference toSecretReference() {
+ Map<String, String> attrs = attributes == null ? ImmutableMap.of() :
attributes;
+ return new SecretReference(provider, attrs);
Review Comment:
Omitted or empty `attributes` are normalized to an empty map here, but
`SecretReference` currently rejects empty maps. Consequently the REST shape
advertised by this PR fails during conversion whenever `attributes` is omitted
or `{}`. Update the API model to accept a non-null empty map and adjust its
tests/Javadoc accordingly.
##########
docs/open-api/filesets.yaml:
##########
@@ -346,6 +346,22 @@ components:
default: {}
additionalProperties:
type: string
+ secretBindings:
+ type: object
+ description: >
+ Optional map of property key to write-through binding. Persisted
value
+ becomes a URN.
+ nullable: true
+ additionalProperties:
+ $ref: "./secrets.yaml#/components/schemas/SecretBinding"
Review Comment:
The fileset REST contract is documented here, but the Python
`FilesetCreateRequest`, `create_fileset`, and
`create_multiple_location_fileset` paths were not extended with these fields,
unlike the catalog and schema paths. Thus the PR's stated Python-client support
is incomplete: Python users cannot create filesets with either secret map.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -101,84 +106,99 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
* @throws SchemaAlreadyExistsException If a schema with the same identifier
already exists.
*/
@Override
- public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
+ public Schema createSchema(
+ NameIdentifier ident,
+ String comment,
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchCatalogException, SchemaAlreadyExistsException {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ long uid = idGenerator.nextId();
+ Map<String, String> entityProperties =
SecretPropertyUtils.copyEntityProperties(properties);
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, entityProperties, "schema", uid, secretBindings,
secretReferences);
Review Comment:
There is no schema-operation test for the new secret-aware create path;
current schema tests only update mock arity. Add tests that verify URN
persistence/hidden output and that a connector create failure invokes rollback,
because this rollback boundary differs from the catalog path.
##########
core/src/main/java/org/apache/gravitino/catalog/FilesetOperationDispatcher.java:
##########
@@ -143,45 +148,58 @@ public Fileset createMultipleLocationFileset(
String comment,
Fileset.Type type,
Map<String, String> storageLocations,
- Map<String, String> properties)
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchSchemaException, FilesetAlreadyExistsException {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ long uid = idGenerator.nextId();
+ Map<String, String> entityProperties =
SecretPropertyUtils.copyEntityProperties(properties);
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, entityProperties, "fileset", uid, secretBindings,
secretReferences);
Review Comment:
The fileset create implementation and rollback path are untested; existing
fileset tests only update Mockito argument counts. Add a create test with
bindings/references plus a failing connector test to verify URN persistence,
hidden output, and rollback.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -593,12 +596,20 @@ public Catalog createCatalog(
Catalog.Type type,
String provider,
String comment,
- Map<String, String> properties)
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchMetalakeException, CatalogAlreadyExistsException {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
- Map<String, String> mergedConfig = buildCatalogConf(provider, properties);
+ final Map<String, String> mergedConfig = new
HashMap<>(buildCatalogConf(provider, properties));
long uid = idGenerator.nextId();
+
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, mergedConfig, "catalog", uid, secretBindings,
secretReferences);
+ secretManager.writeSecrets(secretBindings, secretUrns);
Review Comment:
No catalog-operation test exercises this new create path: the added secret
tests cover `SecretManager` helpers, while existing catalog tests never call
`createCatalog` with secret maps. Add coverage that verifies only the URN is
persisted, the connector receives plaintext, the response hides the key, and a
downstream create failure rolls back the write.
##########
common/src/main/java/org/apache/gravitino/dto/secret/SecretReferenceDTO.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.gravitino.dto.secret;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.ToString;
+import org.apache.gravitino.secret.SecretReference;
+
+/** Data transfer object for an external {@link SecretReference}. */
+@Getter
+@EqualsAndHashCode
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+@AllArgsConstructor
+@Builder(setterPrefix = "with")
+@ToString
+public class SecretReferenceDTO {
+
+ @JsonProperty("provider")
+ private String provider;
+
+ @Builder.Default
+ @JsonProperty("attributes")
+ private Map<String, String> attributes = ImmutableMap.of();
+
+ /**
+ * Converts this DTO to a {@link SecretReference}.
+ *
+ * @return the secret reference
+ */
+ public SecretReference toSecretReference() {
+ Map<String, String> attrs = attributes == null ? ImmutableMap.of() :
attributes;
+ return new SecretReference(provider, attrs);
+ }
+
+ /**
+ * Creates a DTO from a {@link SecretReference}.
+ *
+ * @param reference the secret reference
+ * @return the secret reference DTO
+ */
+ public static SecretReferenceDTO fromSecretReference(SecretReference
reference) {
+ return new SecretReferenceDTO(reference.provider(),
reference.attributes());
+ }
+
+ /**
+ * Converts a property-key map of DTOs to {@link SecretReference}s.
+ *
+ * @param dtos property key → reference DTO; {@code null} or empty returns
an empty map
+ * @return property key → reference (never {@code null})
+ */
+ public static Map<String, SecretReference> toSecretReferences(
+ @Nullable Map<String, SecretReferenceDTO> dtos) {
+ if (dtos == null || dtos.isEmpty()) {
+ return ImmutableMap.of();
+ }
+ ImmutableMap.Builder<String, SecretReference> references =
ImmutableMap.builder();
+ for (Map.Entry<String, SecretReferenceDTO> entry : dtos.entrySet()) {
+ references.put(entry.getKey(), entry.getValue().toSecretReference());
+ }
Review Comment:
A JSON map entry such as `"secretReferences": {"password": null}` is
dereferenced here and throws `NullPointerException`, producing a 500 response
for malformed client input. Validate null DTO values and raise
`IllegalArgumentException` with the offending key.
--
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]
