[SPARK-57890][CORE] Add core credential types (UserContext, ServiceCredential, UserCredentials) by yadavay-amzn · Pull Request #57140 · apache/spark

yadavay-amzn · 2026-07-08T21:02:33Z

What changes were proposed in this pull request?

This adds the foundational credential types for the OIDC Credential Propagation SPIP (SPARK-57703).

Three types are introduced in org.apache.spark.security (spark-core), all @DeveloperApi:

UserContext : driver-side OIDC identity (principal, issuer, rawToken, issuedAt, expiresAt). It is intentionally not Serializable (driver-only, never transmitted to executors). Its toString() redacts rawToken as [REDACTED], and getRawToken() is annotated @JsonIgnore so the raw token is excluded from Jackson serialization.
ServiceCredential : a short-lived, scoped delegated credential (properties: Map<String, String>, expiresAt). Serializable. Its toString() redacts the property values (keys shown, values [REDACTED]) since they may hold cloud secret keys.
UserCredentials : a per-scheme Map<String, ServiceCredential> bundle. Serializable, with a forScheme(String) lookup returning Optional<ServiceCredential>. It contains no UserContext or raw token.

The types are implemented in Java so they are directly usable from both Scala and Java and from the Java CredentialProvider SPI added in the follow-up (SPARK-57891). All are immutable.

Why are the changes needed?

There are currently no types in Spark to represent an OIDC identity or short-lived delegated credentials for propagation to executors. These are the foundational types the rest of the credential-propagation framework builds on. See the SPIP design document, Appendix A.

Does this PR introduce any user-facing change?

No behavior change. It adds new @DeveloperApi types only.

How was this patch tested?

New JUnit CredentialTypesSuite covering the acceptance criteria: rawToken redaction in toString(), exclusion of rawToken from Jackson serialization, ServiceCredential.toString() value redaction, Java-serialization round-trip for ServiceCredential/UserCredentials (no loss), forScheme present/absent lookup, and expiry checks.

Was this patch authored or co-authored using generative AI tooling?

No.

…edential, UserCredentials)

…st, harden rawToken redaction, case-insensitive forScheme

shrirangmhalgi

LGTM. Reviewed the security surface and API design:

What I verified:

rawToken redaction: toString() prints [REDACTED], @JsonIgnore excludes from Jackson serialization
UserContext non-Serializable: test confirms NotSerializableException (driver-only, never transmitted to executors)
Defensive copies: ServiceCredential(properties) and UserCredentials(credentials) both copy input maps; getters return unmodifiableMap
Case-insensitive forScheme: normalized to Locale.ROOT lowercase at construction
Immutability: all fields final, no mutable state, thread-safe without synchronization
serialVersionUID on both Serializable types
ServiceCredential.toString() redacts property values (keys shown for debuggability, values [REDACTED])
UserCredentials.toString() delegates correctly - HashMap.toString() calls ServiceCredential.toString() per entry
Jackson dependency: jackson-databind already in spark-core pom.xml, no new dependency
Identity uniqueness: equals() includes both principal + issuer (sub is only unique within an issuer)

Overall clean foundation for the credential propagation framework.

sarutak · 2026-07-08T23:39:25Z

+  }
+
+  /** Returns the raw OIDC JWT. This value must never be logged or transmitted to executors. */
+  @JsonIgnore


UserContext is not Serializable and not intended to be written as JSON. Is there any reason to annotate as @JsonIgnore?

You're right, there's no reachable JSON path, so this is just being defensive against a path that doesn't exist.
Removed both annotations.

sarutak · 2026-07-09T00:26:43Z

+   */
+  public UserCredentials(Map<String, ServiceCredential> credentials) {
+    Objects.requireNonNull(credentials, "credentials must not be null");
+    Map<String, ServiceCredential> normalized = new HashMap<>(credentials.size());


The comment says "the last entry (in iteration order) wins" for case-colliding keys, but HashMap's iteration order is non-deterministic, making this behavior effectively unpredictable. For a credential system, silent non-deterministic behavior seems risky.

Since the SPIP design document only specifies "per-scheme Map[String, ServiceCredential]" without requiring last-wins semantics, I'd suggest throwing on collision instead:

String normalized = entry.getKey().toLowerCase(Locale.ROOT); if (normalized.put(normalized, entry.getValue()) != null) { throw new IllegalArgumentException( "Duplicate scheme after case normalization: " + entry.getKey()); }

And updating the Javadoc accordingly:

- If the supplied map contains keys that differ only by case, the last entry (in iteration order) wins. + If the supplied map contains keys that differ only by case, an {@link IllegalArgumentException} is thrown.

This is safer and can always be relaxed to last-wins later (with a LinkedHashMap) if needed.

Agreed, silent non-deterministic last-wins is not the right default for a credential type.

Changed the constructor to throw IllegalArgumentException on a post-normalization collision and updated the Javadoc to match.
Also added testUserCredentialsRejectsCaseCollidingSchemes

sarutak · 2026-07-09T00:28:03Z

+    Objects.requireNonNull(credentials, "credentials must not be null");
+    Map<String, ServiceCredential> normalized = new HashMap<>(credentials.size());
+    for (Map.Entry<String, ServiceCredential> entry : credentials.entrySet()) {
+      normalized.put(entry.getKey().toLowerCase(Locale.ROOT), entry.getValue());


HashMap permits null keys, so if a caller passes a map containing a null key, .toLowerCase() will throw an NPE without a clear message. A brief null check would make the failure more informative:

Objects.requireNonNull(entry.getKey(), "scheme key must not be null");

sarutak · 2026-07-09T00:47:22Z

+      Instant expiresAt) {
+    this.principal = Objects.requireNonNull(principal, "principal must not be null");
+    this.issuer = Objects.requireNonNull(issuer, "issuer must not be null");
+    this.rawToken = Objects.requireNonNull(rawToken, "rawToken must not be null");


I'd suggest excluding rawToken from equals() and hashCode(). My reasoning:

UserContext is driver-only, not used as a map key, and there's no use case for equality comparison beyond testing. Including secret material in equals/hashCode when there's no practical need for it seems unnecessary.

If exact token comparison becomes necessary in the future, a dedicated method (e.g., isIdenticalToken(UserContext)) can be added without breaking existing equality semantics.

For testing, rawToken can be verified separately via assertEquals(expected, ctx.getRawToken()).

What do you think?
If exact token comparison is needed in the future, a dedicated method (e.g.,　isIdenticalToken(UserContext)) can be added without breaking existing equality semantics.

Makes sense - I was considering to remove it while implementing as well.

Excluded rawToken from both equals() and hashCode(). The identity fields (principal, issuer) plus the validity window now define equality.
I also updated the test to assert two contexts differing only by rawToken compare equal.
Happy to add isIdenticalToken(...) later if an exact-token comparison is ever needed.

…g/null scheme keys, exclude rawToken from equals/hashCode

sarutak · 2026-07-09T07:33:21Z

The description says as follows.

This adds the foundational credential types for the OIDC Credential Propagation SPIP (SPARK-57703), task 1 of 3.

It's unclear what "3" refers to here (3 deliverable units in the SPIP? 12 sub-tasks in JIRA?). I'd suggest just dropping it like:

- This adds the foundational credential types for the OIDC Credential Propagation SPIP (SPARK-57703), task 1 of 3.
+ This adds the foundational credential types for the OIDC Credential Propagation SPIP (SPARK-57703).

yadavay-amzn · 2026-07-09T19:02:53Z

I was referring to this PR as the first of the 3 tasks of the SPIP which together provide the foundational credential types and interfaces.
However, I agree that the wording is a bit confusing and ambiguous, updated the PR description to remove that part.

Also re-running the CI jobs, since the failures seem unrelated to the PR changes.

sarutak · 2026-07-09T23:24:31Z

I believe the CI failure is not relevant this change so I'll merge this.

…edential, UserCredentials) ### What changes were proposed in this pull request? This adds the foundational credential types for the OIDC Credential Propagation SPIP ([SPARK-57703](https://issues.apache.org/jira/browse/SPARK-57703)). Three types are introduced in `org.apache.spark.security` (spark-core), all `DeveloperApi`: - `UserContext` : driver-side OIDC identity (`principal`, `issuer`, `rawToken`, `issuedAt`, `expiresAt`). It is intentionally **not** `Serializable` (driver-only, never transmitted to executors). Its `toString()` redacts `rawToken` as `[REDACTED]`, and `getRawToken()` is annotated `JsonIgnore` so the raw token is excluded from Jackson serialization. - `ServiceCredential` : a short-lived, scoped delegated credential (`properties: Map<String, String>`, `expiresAt`). `Serializable`. Its `toString()` redacts the property values (keys shown, values `[REDACTED]`) since they may hold cloud secret keys. - `UserCredentials` : a per-scheme `Map<String, ServiceCredential>` bundle. `Serializable`, with a `forScheme(String)` lookup returning `Optional<ServiceCredential>`. It contains no `UserContext` or raw token. The types are implemented in Java so they are directly usable from both Scala and Java and from the Java `CredentialProvider` SPI added in the follow-up (SPARK-57891). All are immutable. ### Why are the changes needed? There are currently no types in Spark to represent an OIDC identity or short-lived delegated credentials for propagation to executors. These are the foundational types the rest of the credential-propagation framework builds on. See the SPIP design document, Appendix A. ### Does this PR introduce _any_ user-facing change? No behavior change. It adds new `DeveloperApi` types only. ### How was this patch tested? New JUnit `CredentialTypesSuite` covering the acceptance criteria: `rawToken` redaction in `toString()`, exclusion of `rawToken` from Jackson serialization, `ServiceCredential.toString()` value redaction, Java-serialization round-trip for `ServiceCredential`/`UserCredentials` (no loss), `forScheme` present/absent lookup, and expiry checks. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #57140 from yadavay-amzn/SPARK-57890. Authored-by: Anupam Yadav <anupamy030@gmail.com> Signed-off-by: Kousuke Saruta <sarutak@apache.org> (cherry picked from commit 99ad64c) Signed-off-by: Kousuke Saruta <sarutak@apache.org>

sarutak · 2026-07-09T23:28:21Z

Merge Summary:

merged into master 99ad64c
merged into branch-4.x 177b6ce

Posted by merge_spark_pr.py

yadavay-amzn added 2 commits July 8, 2026 19:05

[SPARK-57890][CORE] Add core credential types (UserContext, ServiceCr…

e25c3da

…edential, UserCredentials)

[SPARK-57890][CORE] Address review: @SInCE 4.3.0, non-Serializable te…

4393c2f

…st, harden rawToken redaction, case-insensitive forScheme

shrirangmhalgi approved these changes Jul 8, 2026

View reviewed changes

sarutak self-requested a review July 8, 2026 23:25

sarutak reviewed Jul 9, 2026

View reviewed changes

[SPARK-57890][CORE] Address review: drop @JsonIgnore, reject collidin…

6367350

…g/null scheme keys, exclude rawToken from equals/hashCode

sarutak approved these changes Jul 9, 2026

View reviewed changes

sarutak closed this in 99ad64c Jul 9, 2026

Uh oh!

Conversation

yadavay-amzn commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

shrirangmhalgi left a comment

Choose a reason for hiding this comment

What I verified:

Uh oh!

sarutak Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

yadavay-amzn Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sarutak Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

yadavay-amzn Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sarutak Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

yadavay-amzn Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sarutak Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

yadavay-amzn Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarutak commented Jul 9, 2026

Uh oh!

yadavay-amzn commented Jul 9, 2026

Uh oh!

sarutak commented Jul 9, 2026

Uh oh!

sarutak commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yadavay-amzn commented Jul 8, 2026 •

edited

Loading

yadavay-amzn Jul 9, 2026 •

edited

Loading