smaheshwar-pltr commented on code in PR #17155:
URL: https://github.com/apache/iceberg/pull/17155#discussion_r4262431663
##########
open-api/rest-catalog-open-api.py:
##########
@@ -791,10 +791,44 @@ class Sha256QueryLocal(Action):
action: Literal['sha-256-query-local']
+class KeyManagementCredential(BaseModel):
+ """
+ Provider-specific credential config for accessing one or more KMS keys
required by an encrypted
+ table operation.
+
+ The key-management provider is advertised in catalog configuration, such
as `encryption.kms-type`
+ returned from `/v1/config`. The `config` map contains provider-specific
properties for the
+ selected key-management provider.
Review Comment:
I see this mentioned on the design doc, just pointing out though that the
alternative might be putting the provider type here instead i.e. on the
`KeyManagementCredential`, which would maybe be similar to how
`StorageCredential` has `prefix` - that would also make sense to me.
(I guess an implementation detail that diverges from #13225 is that with
this property catalog-wide for all clients, we should maybe initialise KMS
lazily instead of eagerly)
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -4248,6 +4300,13 @@ components:
Credentials for ADLS / GCS / S3 / ... are provided through the
`storage-credentials` field.
Clients must first check whether the respective credentials exist in
the `storage-credentials` field before checking the `config` for credentials.
+ ## Key Management Credentials
+
+ Credentials for KMS / key-management systems are provided through the
`key-management-credentials` field.
+ Clients must first check whether KMS credentials exist in the
`key-management-credentials`
+ field before checking the `config` map for KMS credential
configuration.
Review Comment:
Hmm curious if we need the `config` fallback (I read this as table's
config)? For storage credentials, the `config` existed before we had
`storage-credentials` so perhaps we don't need this here. I guess there could
also be conflicts between the two if referring to table config e.g.
`gcs.oauth2.token` is used for both IIRC. Maybe I misunderstand though.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3665,6 +3676,43 @@ components:
additionalProperties:
type: string
+ KeyManagementCredential:
+ type: object
+ description: |
+ Provider-specific credential config for accessing one or more KMS keys
required by an encrypted
+ table operation.
+
+ The key-management provider is advertised in catalog configuration,
such as `encryption.kms-type`
+ returned from `/v1/config`. The `config` map contains
provider-specific properties for the
+ selected key-management provider.
+
+ Catalogs that return `key-management-credentials` for an operation
must include credentials for all
+ KMS key IDs required by table encryption metadata. Clients should
select the credential config by
+ matching KMS key identifiers referenced by table encryption metadata,
such as
+ `EncryptedKey.encrypted-by-id`, against `kms-key-ids`.
+
+ Credential configs should use provider-specific expiration mechanisms
where available and should
+ be scoped to the minimum required KMS operations and listed KMS key
IDs where the provider
+ supports it. Clients must not persist credentials beyond any
provider-specific expiration.
+ required:
+ - kms-key-ids
+ - config
+ properties:
+ kms-key-ids:
+ type: array
+ description: |
+ KMS key identifiers for which the credential config is relevant.
+
+ Clients should match these values against KMS key identifiers
referenced by table encryption
+ metadata, such as `EncryptedKey.encrypted-by-id`.
+ items:
+ type: string
+ config:
+ type: object
+ description: Provider-specific credential configuration for
accessing the listed KMS key IDs.
+ additionalProperties:
+ type: string
Review Comment:
(Minor but just noting there is
https://github.com/apache/iceberg/blob/6a5480a25995ad7a62bc1b7a09443a2a237d722c/open-api/rest-catalog-open-api.yaml#L4290-L4296
for `LoadTableResult` though just for AWS interestingly I believe)
##########
docs/docs/rest-catalog.md:
##########
@@ -83,6 +83,7 @@ at connection time through
| `rest-page-size` | null | The page size to
use when listing namespaces, tables, or other paginated resources.
|
| `namespace-separator` | `%1F` | The separator
character used for namespace levels when communicating with the REST server.
|
| `scan-planning-mode` | `client` | Controls where
scan planning is performed. Supported values: `client` (client-side planning),
`server` (server-side planning). Can be overridden per-table by the server in
LoadTableResponse. |
+| `encryption.kms-type` | null | KMS provider
type used for encrypted tables. Clients may configure this value, but REST
catalog servers may advertise it through `/v1/config`; server `overrides` take
precedence over client configuration. When using `key-management-credentials`,
clients should use the final catalog configuration value after applying server
defaults and overrides. The value is an open enum with initial values `aws`,
`gcp`, and `azure`; catalogs may return additional values when the catalog and
client agree on KMS client resolution and credential config. |
Review Comment:
Nit, with https://github.com/apache/iceberg/pull/18210 there's now `aliyun`.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -4160,6 +4171,43 @@ components:
type: string
const: "sha-256-query-local"
+ KeyManagementCredential:
+ type: object
+ description: |
+ Provider-specific credential config for accessing one or more KMS keys
required by an encrypted
+ table operation.
+
+ The key-management provider is advertised in catalog configuration,
such as `encryption.kms-type`
+ returned from `/v1/config`. The `config` map contains
provider-specific properties for the
+ selected key-management provider.
+
+ Catalogs that return `key-management-credentials` for an operation
must include credentials for all
+ KMS key IDs required by table encryption metadata. Clients should
select the credential config by
+ matching KMS key identifiers referenced by table encryption metadata,
such as
+ `encrypted-by-id` fields in table encryption metadata, against
`kms-key-ids`.
Review Comment:
Hmm on create, there are no `encryption-keys` yet, so there's no
`encrypted-by-id` to match at all, no?. But in this case, the client still
needs the KMS key to wrap the first key encryption key. And that'd usually be
`encryption.key-id` table property itself so you'd match on that I'd imagine.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -4248,6 +4300,13 @@ components:
Credentials for ADLS / GCS / S3 / ... are provided through the
`storage-credentials` field.
Clients must first check whether the respective credentials exist in
the `storage-credentials` field before checking the `config` for credentials.
+ ## Key Management Credentials
+
+ Credentials for KMS / key-management systems are provided through the
`key-management-credentials` field.
+ Clients must first check whether KMS credentials exist in the
`key-management-credentials`
+ field before checking the `config` map for KMS credential
configuration.
+ Clients must use `key-management-credentials`, if present,
consistently with the REST access-delegation credential sourcing rules.
Review Comment:
Nit, I think this is referring to the catalog-side SHOULDs which to me is a
bit unclear, might be worth stating the behaviour directly which I understand
to be something like:
> When `key-management-credentials` contains a credential for a KMS key ID,
clients must use it for that key instead of locally configured KMS credentials.
but I'm not too opinionated
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -4169,6 +4217,10 @@ components:
type: array
items:
$ref: '#/components/schemas/StorageCredential'
+ key-management-credentials:
Review Comment:
Assuming clients refresh KMS creds through the `loadCredentials` endpoint,
that endpoint doesn't currently take
`X-Iceberg-Access-Delegation` so the catalog won't be able to tell I think?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2157,9 +2157,19 @@ components:
to supply access via any or none of the requested mechanisms.
- Specific properties and handling for `vended-credentials` and
`remote-signing`
+ Specific properties and handling for `vended-credentials`,
`remote-signing`, and `kms-vended-credentials`
are documented in the `LoadTableResult` schema section of this spec
document.
+ `kms-vended-credentials` does not subsume `vended-credentials`.
Clients that support both storage
+ and KMS credential vending should request both mechanisms for
encrypted table operations.
Review Comment:
Minor, on "for encrypted table operations", my understanding is that clients
might not know a table is encrypted until they load it though? Maybe just a
wording thing
--
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]
