Correct the handling of access delegation mode #211

dimas-b · 2024-08-26T16:55:59Z

Description

Iceberg REST API spec defines two possible values for the X-Iceberg-Access-Delegation:

vended-credentials
remote-signing

This change fixes Polaris docs to use the vended-credentials value instead of previous (incorrect) value of true and makes the REST API service validate the X-Iceberg-Access-Delegation header submitted by the client against the spec.

Backward-compatibility with old clients using the value of true for access delegation is maintained.

Detailed changes:

Add AccessDelegationMode enum to parse and represent specified
delegation modes in java.
Update IcebergCatalogAdapter to validate that if access
delegation is supported by the client, that "vended-credentials"
is accepted ("remote-signing" is not supported by the server).
Interpret the single header value of true as vended-credentials.
Remove unused method parameters.
Re-generate docs

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Existing and new unit tests

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation

collado-mike · 2024-08-26T17:06:28Z

polaris-service/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java

+  private EnumSet<AccessDelegationMode> parseAccessDelegationModes(String accessDelegationMode) {
+    EnumSet<AccessDelegationMode> delegationModes =
+        AccessDelegationMode.fromProtocolValuesList(accessDelegationMode);
+    if (!delegationModes.isEmpty() && !delegationModes.contains(VENDED_CREDENTIALS)) {


For backward compatibility, can we support "true" for the time being?

We could, but is it critical enough to justify carrying this baggage forward? Note that true is an invalid header value according to the Iceberg REST Catalog spec and Polaris is not released yet.

It seems like there will be some need to reconcile "strict intent" vs "hints" better in the future, since right now a lot of the Iceberg spec is open-ended about interpretation -- for example, the spec currently says:

The server may choose to supply access via any or none of the requested mechanisms.

This also relates to apache/iceberg#10359 in that some behaviors currently can only "guess" at the caller's intent.

I agree that any hard-coding of "true" is probably not good baggage to carry forward. I would say though that extrapolating from #77 and how the Tabular "demo REST docker image" provides hooks for directly returning the server-wide application credentials in credential-vending, maybe it's worth providing some extensibility here.

What if we extract a server-level configuration parameter or two:

SUPPORTED_ACCESS_DELEGATION_MODES=vended-credentials,...

ERROR_ON_FAILED_NEGOTIATION_FOR_ACCESS_DELEGATION_MODE=true

The first one would just be a set-intersection between the list of modes the client wants and the set that the server claims to accept. I suppose the later mapping of declared modes to access-delegation impl will be something to add in the future once we actually need to plumb it through to PolarisCatalogHandlerWrapper to do different things for different modes.

The second one will dictate whether the server should be in "strict" mode and return an error response, or instead be permissive and just log a warning (or if there was a way to return helpful warnings in Iceberg REST responses that'd be great).

To the last point I wonder if we should also think of proposing some standard scaffolding for "warning notices" in REST responses that client-libraries can start to support to display to a caller in whatever way is most natural for the client-library. I know cloud providers have gone through this exercise before so that individual service providers can have a standard way to start providing deprecation notices, etc.

The server may choose to supply access via any or none of the requested mechanisms.

My reading of this is that if the client specifies some access modes via the header, the server must use zero, one or more of them. The server may not use a delegation mode that the client did not request (for the client may not be able to handle it).

In this PR, if the client does not request any delegation mode, the server will not use "vended credentials" (same as before). If the client sends "vended credentials" and something else, the server will use "vended credentials" (same as before).

If the client requests true - that will lead to an error response (change in behaviour).

If the client requests only remote signing - that will lead to an error response (change in behaviour). Previously, the server would still use "vended credentials", so this is a bug fix, IMHO.

I'm not against adding config for more lenient processing, but given that there's no released Polaris version that supported old values, I think we can use simpler and stricter validation logic. It is easier to relax conditions after a release than tighten them if we have to.

After some offline thinking, I will add backward compatibility support for true presently.

backward compatibility added

Makes sense, I think that's a decent tradeoff for now since "true" was unfortunately exposed through docs for awhile so this would facilitate migrating some initial proofs-of-concept for now.

I agree with having strict validation in general, and agree erroring on unsupported remote-signing or any other invalid values for now is a good idea since it's easier to relax such behaviors than tighten in the future.

polaris-service/src/test/java/org/apache/polaris/service/catalog/AccessDelegationModeTest.java

polaris-service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationMode.java

polaris-service/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java

snazy

Thanks @dimas-b !

dennishuo

LGTM

flyrain · 2024-08-27T18:38:44Z

polaris-service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationMode.java

+    if (protocolValues.trim().toLowerCase(Locale.ROOT).equals("true")) {
+      return EnumSet.of(VENDED_CREDENTIALS);
+    }


This isn't a blocker for this PR, but I think we need a plan to deprecate it.

RussellSpitzer · 2024-08-29T03:39:52Z

@dimas-b Sorry about this but could you please rebase? Please ping me when you do so I can merge

* Add `AccessDelegationMode` enum to parse and represent specified delegation modes in java. * Update `IcebergCatalogAdapter` to validate that if access delegation is supported by the client, that "vended-credentials" is accepted ("remote-signing" is not supported by the server). * Remove unused method parameters.

dimas-b · 2024-08-29T20:00:18Z

@RussellSpitzer : rebased :)

Manually resolved conflicts in the first commit
Re-ran docs/build for the last commit

Following up on apache/polaris#211

dimas-b requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau and jackye1995 as code owners August 26, 2024 16:56

collado-mike reviewed Aug 26, 2024

View reviewed changes

flyrain reviewed Aug 27, 2024

View reviewed changes

polaris-service/src/main/java/org/apache/polaris/service/catalog/AccessDelegationMode.java Outdated Show resolved Hide resolved

polaris-service/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java Outdated Show resolved Hide resolved

dimas-b force-pushed the access-delegation-mode branch from b341f14 to c3224fd Compare August 27, 2024 00:45

snazy previously approved these changes Aug 27, 2024

View reviewed changes

dimas-b dismissed snazy’s stale review via f61ff40 August 27, 2024 18:13

dimas-b force-pushed the access-delegation-mode branch from c3224fd to f61ff40 Compare August 27, 2024 18:13

dennishuo approved these changes Aug 27, 2024

View reviewed changes

flyrain reviewed Aug 27, 2024

View reviewed changes

flyrain approved these changes Aug 27, 2024

View reviewed changes

RussellSpitzer previously approved these changes Aug 28, 2024

View reviewed changes

dimas-b added 5 commits August 29, 2024 15:52

review: final / private

c22533e

review: Preconditions.checkArgument

a8ccf57

review: support old true value for access delegation mode

0744757

Re-generate docs

f946b27

dimas-b dismissed RussellSpitzer’s stale review via f946b27 August 29, 2024 19:58

dimas-b force-pushed the access-delegation-mode branch from f61ff40 to f946b27 Compare August 29, 2024 19:58

RussellSpitzer enabled auto-merge (squash) September 3, 2024 21:10

RussellSpitzer approved these changes Sep 4, 2024

View reviewed changes

RussellSpitzer merged commit ed4c0cb into apache:main Sep 4, 2024
3 checks passed

dimas-b deleted the access-delegation-mode branch September 4, 2024 15:58

dimas-b added a commit to dimas-b/polaris-site that referenced this pull request Sep 6, 2024

Fix X-Iceberg-Access-Delegation example

6eb90ec

Following up on apache/polaris#211

eric-maynard mentioned this pull request Sep 25, 2024

[BUG] Incorrect usage of the X-Iceberg-Access-Delegation header #146

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct the handling of access delegation mode #211

Correct the handling of access delegation mode #211

dimas-b commented Aug 26, 2024 •

edited

Loading

collado-mike Aug 26, 2024

dimas-b Aug 26, 2024 •

edited

Loading

dennishuo Aug 26, 2024

dimas-b Aug 26, 2024

dimas-b Aug 27, 2024

dimas-b Aug 27, 2024

dennishuo Aug 27, 2024

snazy left a comment

dennishuo left a comment

flyrain Aug 27, 2024

RussellSpitzer commented Aug 29, 2024

dimas-b commented Aug 29, 2024 •

edited

Loading

Correct the handling of access delegation mode #211

Correct the handling of access delegation mode #211

Conversation

dimas-b commented Aug 26, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

collado-mike Aug 26, 2024

Choose a reason for hiding this comment

dimas-b Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

dennishuo Aug 26, 2024

Choose a reason for hiding this comment

dimas-b Aug 26, 2024

Choose a reason for hiding this comment

dimas-b Aug 27, 2024

Choose a reason for hiding this comment

dimas-b Aug 27, 2024

Choose a reason for hiding this comment

dennishuo Aug 27, 2024

Choose a reason for hiding this comment

snazy left a comment

Choose a reason for hiding this comment

dennishuo left a comment

Choose a reason for hiding this comment

flyrain Aug 27, 2024

Choose a reason for hiding this comment

RussellSpitzer commented Aug 29, 2024

dimas-b commented Aug 29, 2024 • edited Loading

dimas-b commented Aug 26, 2024 •

edited

Loading

dimas-b Aug 26, 2024 •

edited

Loading

dimas-b commented Aug 29, 2024 •

edited

Loading