MonkeyCanCode commented on issue #3440:
URL: https://github.com/apache/polaris/issues/3440#issuecomment-3775405520
> Thank you for the clarification. I understand the distinction between
omitting KMS only when KMS is not being used vs. globally disabling KMS logic.
My testing goal here is specifically aligned with the former: ensuring that
when a catalog is intentionally created without any KMS configuration, Polaris
produces only S3‑related permissions in the STS session policy. I performed an
A/B test using two catalogs that are identical except for the presence or
absence of a region setting:
>
> Catalog A (region unset) allowedKmsKeys = [], no currentKmsKey, no region
→ STS policies contain only S3 actions, and INSERT succeeds.
>
> Catalog B (region = us-east-1) allowedKmsKeys = [], no currentKmsKey,
region present → STS policies still include { "Action": ["kms:DescribeKey",
"kms:Decrypt"], "Resource": "arn:aws:kms:us-east-1::key/*" }
>
> even though the catalog does not configure or allow any KMS keys.
>
> This causes the AssumeRole request to fail with: Invalid action:
kms:DescribeKey
>
> which in turn causes a failure when Spark attempts to write data. So the
issue is not that the storage system lacks KMS support, and it is not about
disabling KMS entirely. Rather, the mismatch is:
>
> The catalog is explicitly configured with no KMS (allowedKmsKeys = [], no
currentKmsKey). Yet the generated STS session policy still includes KMS
actions, depending on whether region was set.
>
> From the user perspective, the expectation is: “If I did not configure any
KMS keys in the catalog, then the generated STS policy should not contain any
kms:* actions.”
>
> This matches the intent described in the changelog entry from PR‑3445
(“exclude KMS policies when KMS is not being used for S3”). The current
behavior means that two catalogs with the same KMS configuration behave
differently depending solely on whether a region was provided, which makes the
experience inconsistent for users who rely on AssumeRole to access
S3-compatible storage. A small adjustment to addKmsKeyPolicy(...)—only
attaching a KMS policy when hasCurrentKey || hasAllowedKey should fixes the
issue for both catalog types without modifying any other KMS logic.
As mentioned above, that is mainly for the wildcard KMS policy when it
thinks the target backend is AWS S3 (which in this case, it is using accountid
and region which may not be sufficient to distinguish AWS vs non-AWS S3). But
we may need to check with original contributor on this before we can attempt a
fix if by mistake as the comment in the code is stating wildcard kms policy
should be put if it is an "AWS S3" (this will be covert in the dev mailing list
later tonight as mentioned above).
Do you mind also help to confirm if the second fix is necessary in case we
mixed changes due to alignment in the testing (which you reported without it,
it won't be using the right STS credential but AWS secret/key instead)? Based
on my understanding, without the 2nd fix, table would lost its write
capabilities after a metadata refresh (thus I implicitly added it back). If
preferred, I can submit 2 PRs later tonight then wait for final confirmation
before merge them in assuming approved by the Polaris dev team.
--
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]
