Allow setting ARN role and external ID on catalogs that had none#4019
Allow setting ARN role and external ID on catalogs that had none#4019adutra wants to merge 3 commits intoapache:mainfrom
Conversation
Fixes: apache#3885 Catalogs created without a role ARN (e.g. not using STS) could not be updated to add one, because the validation treated going from null to a non-null value as a forbidden AWS account ID change. The same applied to external IDs. This change relaxes the validation in `validateUpdateCatalogDiffOrThrow()` so that setting a previously-null role ARN or external ID is allowed, while still preventing changes to the AWS account ID or external ID once set. This change also changes the error message from the misleading "Cannot modify Role ARN" (the role name *can* be changed) to the more accurate "Cannot modify AWS account ID".
| if (currentAwsConfig.getAwsAccountId() != null | ||
| && !currentAwsConfig.getAwsAccountId().equals(newAwsConfig.getAwsAccountId())) { |
There was a problem hiding this comment.
This means we allow updating the role A to role B if they are same account ? any reason why
There was a problem hiding this comment.
Yes. This has always been like this, presumably for security reasons (i.e. prevent a malicious client from changing to a role in another AWS account). Maybe @dennishuo or @collado-mike know more.
There was a problem hiding this comment.
I would love to know about the history, because i think malicious client can also point it to a role within the same account but different privledge if they have the access to catalog ? want to understand the attack vector more (not a blocker for this pr)
There was a problem hiding this comment.
Me too 😄 The original discussion #3885 was precisely about that.
|
|
||
| if (!Objects.equals(currentAwsConfig.getAwsAccountId(), newAwsConfig.getAwsAccountId())) { | ||
| // Allow setting a role ARN when none was previously configured, but prevent changing the | ||
| // AWS account ID once it has been set. |
There was a problem hiding this comment.
TBH, I do not see much value in allowing changes from unset to a new roles in some account, but preventing changing to a role in another account.
The security implications are the same for changing from unset to some role as for changing from one account to another, IMHO.
If the new role has more access to the data in storage than the Polaris Administrator considers valid (for whatever reason), and Polaris is able to assume that role with current credentials, it is a mistake in the authorization setup in the new role's account.
This can hardly be a concern when the Polaris owner (who controls Polaris storage access credentials) and the catalog owner is the same person or entity.
I can see this being a concern when the Polaris owner is different from the catalog admin.
Therefore, I'd prefer adding a feature flag to enable any role changes (at the discretion of the Polaris owner). If the flag is not set, let's keep current behaviour. The default should probably be off (current, more restricted behaviour).
WDYT?
There was a problem hiding this comment.
I don't disagree 😄
Since we don't have feedback from the original Polaris authors, let's go with the feature flag approach.
There was a problem hiding this comment.
Thx! I believe the feature flag will allow maintaining the old (current) behaviour when the Polaris owner so wishes, and allow more flexibility in fresh deployments.
3 participants
Fixes: #3885
Catalogs created without a role ARN (e.g. not using STS) could not be updated to add one, because the validation treated going from null to a non-null value as a forbidden AWS account ID change. The same applied to external IDs.
This change relaxes the validation in
validateUpdateCatalogDiffOrThrow()so that setting a previously-null role ARN or external ID is allowed, while still preventing changes to the AWS account ID or external ID once set.This change also changes the error message from the misleading "Cannot modify Role ARN" (the role name can be changed) to the more accurate "Cannot modify AWS account ID".
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)