Improve Access Delegation Mode Selection Algorithm#3750
Improve Access Delegation Mode Selection Algorithm#3750iting0321 wants to merge 5 commits intoapache:mainfrom
Conversation
|
Hi @iting0321 :
I'm not sure Polaris supports remote signing yet 🤔 @adutra : WDYT? |
adutra
left a comment
There was a problem hiding this comment.
Thanks @iting0321 for this nice improvement!
I had something similar in my PR for remote signing #2280.
...e/service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationModeResolver.java
Outdated
Show resolved
Hide resolved
...e/service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationModeResolver.java
Outdated
Show resolved
Hide resolved
...e/service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationModeResolver.java
Outdated
Show resolved
Hide resolved
| Boolean.TRUE.equals( | ||
| realmConfig.getConfig( | ||
| FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION, catalogEntity)); |
There was a problem hiding this comment.
- Contrary to the javadocs, this method cannot return null since there is a default value (false).
- The
SKIP_CREDENTIAL_SUBSCOPING_INDIRECTIONis not overridable at catalog level.
| Boolean.TRUE.equals( | |
| realmConfig.getConfig( | |
| FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION, catalogEntity)); | |
| boolean skipCredentialSubscoping = | |
| realmConfig.getConfig(FeatureConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION); |
...e/service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationModeResolver.java
Outdated
Show resolved
Hide resolved
| catalogEntity.getName()); | ||
| return REMOTE_SIGNING; | ||
| } | ||
|
|
There was a problem hiding this comment.
We are missing one important check here:
boolean credentialSubscopingAuthorized =
baseCatalog instanceof IcebergCatalog
|| realmConfig.getConfig(
ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING, catalogEntity);
if (!credentialSubscopingAuthorized) {
LOGGER.debug(
"Credential subscoping is not authorized for catalog {}, selecting REMOTE_SIGNING",
catalogEntity.getName());
return REMOTE_SIGNING;
}
.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
Outdated
Show resolved
Hide resolved
Polaris does not support remote signing yet indeed, but the changes in this PR go in the right direction, and imho, can be adopted before remote signing lands. |
|
Improving access delegation mode selection before remote signing is fully supports sounds reasonable to me. Please consider this check, though: That is to say, it might be best to do this validation only after the final mode resolution 🤔 |
I think this check is fine; as you noted, we don't support anything else than I see @iting0321's work in this PR as a way to proactively prepare Polaris for remote signing – without actually making remote signing possible. |
| EnumSet<AccessDelegationMode> requestedModes) { | ||
| if (requestedModes.isEmpty()) { | ||
| return requestedModes; | ||
| resolvedAccessDelegationMode = AccessDelegationMode.UNKNOWN; |
There was a problem hiding this comment.
Not for this PR, but I've always thought we should rename this mode to NONE, wdyt?
There was a problem hiding this comment.
I think NONE is more precise, since it clearly communicates that no delegation mode was requested or selected. I’m happy to add it in a follow-up PR.
There was a problem hiding this comment.
The original idea behind AccessDelegationMode.UNKNOWN was for it to represent cases when the client declared something unparseable. Cf. AccessDelegationMode.fromProtocolValuesList()
If we want to definitely resolve to one specific AccessDelegationMode why don't we feed raw protocol strings to AccessDelegationModeResolver instead?
| * Resolves the optimal {@link AccessDelegationMode} to use based on the client's requested modes | ||
| * and the catalog's capabilities. | ||
| * | ||
| * <p>The selection algorithm is: |
There was a problem hiding this comment.
I think the rest of the javadocs starting from this line are rather for the implementation, not for the interface.
There was a problem hiding this comment.
Thanks for your feedback. I just removed it.
| return delegationModes; | ||
| // Parse the modes from the header - validation will happen after mode resolution | ||
| // in IcebergCatalogHandler.resolveAccessDelegationModes() | ||
| return AccessDelegationMode.fromProtocolValuesList(accessDelegationMode); |
There was a problem hiding this comment.
We need to reject REMOTE_SIGNING explicitly until it's available, otherwise we could introduce regressions, wdyt?
| return AccessDelegationMode.fromProtocolValuesList(accessDelegationMode); | |
| var accessDelegationMode = AccessDelegationMode.fromProtocolValuesList(accessDelegationMode); | |
| // TODO remove when remote signing is implemented | |
| Preconditions.checkArgument( | |
| accessDelegationMode != REMOTE_SIGNING, | |
| "Unsupported access delegation mode: %s", | |
| accessDelegationMode); |
There was a problem hiding this comment.
Yes, I think we should reject REMOTE_SIGNING explicitly for now.
Since the resolver may accept it but the feature isn’t implemented, clients could see unexpected behavior. Rejecting unsupported modes in the adapter keeps the flow clean and avoids failures later on.
| "Credential subscoping is skipped for this realm, selecting REMOTE_SIGNING"); | ||
| return REMOTE_SIGNING; | ||
| } | ||
|
|
There was a problem hiding this comment.
We are still missing this check:
There was a problem hiding this comment.
Sorry, I missed that earlier. I just pushed a commit adding the missing check and the corresponding unit test in AccessDelegationModeResolverTest.java
Thanks for pointing it out!
36b24df to
1bf8c18
Compare
…lection based on catalog capabilities
- Remove resolveToSet() method and use single AccessDelegationMode - Store resolved mode as field for later retrieval - Fix SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION config access - Update log message to reflect realm-level scope for credential subscoping - Turn AccessDelegationModeResolver into an interface + CDI bean
1bf8c18 to
9d0af9b
Compare
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for working on this, @iting0321 !
I agree with @adutra that is makes sense to add a resolver for access delegation modes. This PR seems to be moving in the right direction.
|
|
||
| private EnumSet<AccessDelegationMode> parseAccessDelegationModes(String accessDelegationMode) { | ||
| EnumSet<AccessDelegationMode> delegationModes = | ||
| // Parse the modes from the header - validation will happen after mode resolution |
There was a problem hiding this comment.
nit: header - nothing in this case binds to any "headers"... The first part of this comment looks a bit confusing to me.
| // TODO remove when remote signing is implemented | ||
| Preconditions.checkArgument( | ||
| delegationModes.isEmpty() || delegationModes.contains(VENDED_CREDENTIALS), | ||
| !modes.contains(REMOTE_SIGNING), |
There was a problem hiding this comment.
This assertion might be too strict. When Polaris supports VENDED_CREDENTIALS but does not support REMOTE_SIGNING, I do not see any hard in the client requesting both, as long as the resolution algorithm selects the supported more (i.e. VENDED_CREDENTIALS in the current state of the code).
| * | ||
| * @return The resolved access delegation mode, or null if not yet resolved | ||
| */ | ||
| protected AccessDelegationMode getResolvedAccessDelegationMode() { |
There was a problem hiding this comment.
This method is not used. Is it worth adding?
| EnumSet<AccessDelegationMode> requestedModes) { | ||
| if (requestedModes.isEmpty()) { | ||
| return requestedModes; | ||
| resolvedAccessDelegationMode = AccessDelegationMode.UNKNOWN; |
There was a problem hiding this comment.
The original idea behind AccessDelegationMode.UNKNOWN was for it to represent cases when the client declared something unparseable. Cf. AccessDelegationMode.fromProtocolValuesList()
If we want to definitely resolve to one specific AccessDelegationMode why don't we feed raw protocol strings to AccessDelegationModeResolver instead?
| LOGGER.warn( | ||
| "Unknown access delegation mode combination: {}, defaulting to VENDED_CREDENTIALS", | ||
| requestedModes); | ||
| return VENDED_CREDENTIALS; |
There was a problem hiding this comment.
I'm not sure about defaulting to VENDED_CREDENTIALS. The client may have a legitimate reason not to ask for VENDED_CREDENTIALS (e.g. due to risk of credential exposure), so forcing sensitive data into the response may not be advisable.
If we want to do this, I'd propose to add a feature flag (with default "off").
| // For external/federated catalogs, check if ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING is enabled. | ||
| boolean credentialSubscopingAuthorized = | ||
| !catalogEntity.isExternal() | ||
| || configurationStore.getConfiguration( |
| } | ||
|
|
||
| CatalogEntity catalogEntity = getResolvedCatalogEntity(); | ||
| resolvedAccessDelegationMode = accessDelegationModeResolver().resolve(requestedModes, catalogEntity); |
There was a problem hiding this comment.
Is the resolvedAccessDelegationMode field really necessary? It does not appear to be read at all (only written). Do we expect more that one "resolve" call per API request?
Summary
This PR introduces an improved algorithm for selecting the optimal
AccessDelegationModewhen clients request bothVENDED_CREDENTIALSandREMOTE_SIGNING. The new implementation considers actual catalog capabilities (STS availability) rather than using simple heuristics.Problem
Previously, when a client requested both
VENDED_CREDENTIALSandREMOTE_SIGNINGmodes via theX-Iceberg-Access-Delegationheader, Polaris would always preferVENDED_CREDENTIALS.However, this could lead to failures when:
In these cases,
REMOTE_SIGNINGwould be the better choice, but the original algorithm had no way to make this determination.Solution
Introduced
AccessDelegationModeResolver- a dedicated component that intelligently selects the optimal access delegation mode based on:X-Iceberg-Access-DelegationheaderAwsStorageConfigurationInfo.getStsUnavailable()flagSKIP_CREDENTIAL_SUBSCOPING_INDIRECTIONsettingHow to Test
Related Issues
#3090 - Improve access delegation mode selection algorithm
Checklist
AccessDelegationModeResolverTestwith 15 unit testsCHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)