CDAP-21265 : Add support for PKCE by sahusanket · Pull Request #16199 · cdapio/cdap

sahusanket · 2026-08-12T22:25:25Z

Title:

feat: Add PKCE support for OAuth providers

Description:

This PR introduces Proof Key for Code Exchange (PKCE) support for OAuth providers in CDAP data pipelines.

Key Changes:

PKCE Flow Support: Updates the OAuthStore schema to include authType (expanding the table spec to 6 columns) to properly identify and route PKCE-enabled providers.
Secure State & TTLs: Adds endpoints to generate the code_challenge and securely store the code_verifier with a configurable time-to-live (oauth.pkce.code.verifier.ttl.sec) via CDAP system preferences.

TESTING:

Tested the following cases for a secure store as GCP SECRET MANAGER and the default CDAP secure store.

1. PKCE Flow (AuthType = PKCE)

Tests specifically targeting the Proof Key for Code Exchange logic and the new 10-minute Secure Store TTL.

Phase	Test Scenario	Action	Expected Result
Config	Provider Creation	Call `PutOAuthProvider` with `authType = PKCE`.	Provider is saved successfully with PKCE enabled.
Auth URL	URL Generation	Generate Auth URL for the PKCE provider.	URL contains `code_challenge` and `code_challenge_method=S256`. A `code_verifier` is generated internally.
Storage	Verifier Persistence	Check Secure Store after Auth URL generation.	The `code_verifier` is saved under the system namespace using the `state` as the key, with a 10-minute TTL.
256 SHA check	`code_challenge = BASE64URL-ENCODE(SHA256(ASCII(code_verifier)))`	Take the `code_challenge` from step 2, and `code_verifier` from step 3 and Verify.	It should match perfectly.
Callback	Valid Exchange Payload	Trigger the OAuth callback (mocking the IdP).	The POST body to `tokenRefreshURL` contains `grant_type=authorization_code`, `code`, and the retrieved `code_verifier`.
Storage	Missing Verifier	Check Secure Store after successful callback.	The `code_verifier` should be deleted.
TTL	Expiration (Timeout)	Trigger Auth URL generation, wait > 10 minutes, then attempt Token Exchange callback.	Secure Store throws NotFound/null. Token exchange fails gracefully.
TTL	Success (Within window)	Trigger Auth URL generation, attempt callback within 10 minutes.	The `code_verifier` is retrieved successfully and token exchange proceeds.

2. Standard Flow (AuthType = STANDARD or default)

Tests to ensure existing integrations without PKCE continue to work perfectly.

Phase	Test Scenario	Action	Expected Result
Config	Provider Creation	Call `PutOAuthProvider` with `authType = STANDARD` (or missing).	Provider is saved successfully without PKCE flags.
Auth URL	URL Generation	Generate Auth URL for the `STANDARD` provider.	URL does not contain `code_challenge` or `code_challenge_method`.
Storage	No Verifier Persistence	Check Secure Store after Auth URL generation.	Secure Store is not hit; no `code_verifier` is stored.
Callback	Valid Exchange Payload	Trigger the OAuth callback (mocking the IdP).	The POST body to `tokenRefreshURL` contains `grant_type=authorization_code` and `code`, but no `code_verifier`.

Note:
PKCE support is currently introduced for internal use. The time-to-live (TTL) auto-cleanup for code_verifiers relies on a Secure Store implementation that natively supports TTL (such as GCP Secret Manager).

If the default Secure Store implementation is used, unused code_verifiers will not be automatically cleaned up upon expiration. However, this is entirely harmless from a security perspective, and the resulting storage footprint is negligible.
Follow up Jira : https://cdap.atlassian.net/browse/CDAP-21266

gemini-code-assist

Code Review

This pull request introduces support for Proof Key for Code Exchange (PKCE) OAuth authentication by updating OAuthProvider, OAuthStore, and OAuthHandler to handle the PKCE flow, including generating, storing, and verifying code challenges and verifiers. The review feedback highlights several key improvements: removing a redundant @nullable annotation on the authType field, avoiding swallowing exceptions blindly when retrieving the code verifier, reusing a static SecureRandom instance for efficiency, and validating the user-supplied state parameter to prevent potential key injection or path traversal vulnerabilities.

gemini-code-assist · 2026-08-12T22:27:47Z

+  @Nullable
+  private final AuthType authType;


The authType field is annotated with @Nullable, but the constructor guarantees it will never be null by defaulting to AuthType.STANDARD if a null value is passed. To avoid confusion and accurately reflect the class invariants, the @Nullable annotation should be removed from the field.

Suggested change

@Nullable

private final AuthType authType;

private final AuthType authType;

gemini-code-assist · 2026-08-12T22:27:48Z

+  @javax.annotation.Nullable
+  public String getPKCECodeVerifier(String provider, String state) {
+    String key = getPKCEKey(provider, state);
+    try {
+      return new String(secureStore.getData(NamespaceId.SYSTEM.getNamespace(), key), java.nio.charset.StandardCharsets.UTF_8);
+    } catch (Exception e) {
+      return null;
+    }
+  }


Catching Exception blindly and returning null swallows all potential errors, including critical secure store misconfigurations, network issues, or permission errors. This makes troubleshooting extremely difficult because any system failure will be silently treated as an 'invalid or expired state'. Consider logging the exception or catching a more specific 'not found' exception if available.

gemini-code-assist · 2026-08-12T22:27:48Z

+  private String generateCodeVerifier() {
+    SecureRandom sr = new SecureRandom();
+    byte[] codeVerifierBytes = new byte[32];
+    sr.nextBytes(codeVerifierBytes);
+    return Base64.getUrlEncoder().withoutPadding().encodeToString(codeVerifierBytes);
+  }


Instantiating SecureRandom on every method call is inefficient and can lead to performance overhead or entropy source exhaustion. It is recommended to reuse a single, thread-safe SecureRandom instance as a static final class member.

Suggested change

private String generateCodeVerifier() {

SecureRandom sr = new SecureRandom();

byte[] codeVerifierBytes = new byte[32];

sr.nextBytes(codeVerifierBytes);

return Base64.getUrlEncoder().withoutPadding().encodeToString(codeVerifierBytes);

}

private static final SecureRandom SECURE_RANDOM = new SecureRandom();

private String generateCodeVerifier() {

byte[] codeVerifierBytes = new byte[32];

SECURE_RANDOM.nextBytes(codeVerifierBytes);

return Base64.getUrlEncoder().withoutPadding().encodeToString(codeVerifierBytes);

}

Please fix.

gemini-code-assist · 2026-08-12T22:27:48Z

+        if (state == null || state.isEmpty()) {
+          throw new OAuthServiceException(java.net.HttpURLConnection.HTTP_BAD_REQUEST, "State is required for PKCE authentication");
+        }


The state parameter is used directly to construct the secure store key. Since the state is user-supplied in the callback request, a malicious user could potentially pass a state containing path traversal characters (e.g., ../) or other special characters, which might cause unexpected behavior or key injection in the underlying secure store implementation. Since the state is generated as a UUID, you should validate that the incoming state only contains safe characters (e.g., alphanumeric and hyphens) before using it.

Suggested change

if (state == null || state.isEmpty()) {

throw new OAuthServiceException(java.net.HttpURLConnection.HTTP_BAD_REQUEST, "State is required for PKCE authentication");

}

if (state == null || state.isEmpty() || !state.matches("^[a-zA-Z0-9-]+$")) {

throw new OAuthServiceException(java.net.HttpURLConnection.HTTP_BAD_REQUEST, "State is required and must be valid for PKCE authentication");

}

vsethi09 · 2026-08-13T06:18:09Z

    return userAgent;
  }

+  public enum AuthType {


Please add comments to explain what these Auth Types are.

vsethi09 · 2026-08-13T06:21:58Z

    return fields;
  }
+
+  public void storePKCECodeVerifier(String provider, String state, String codeVerifier) throws Exception {


supernit: use write or put, instead of store to keep the naming consistent with other methods.

vsethi09 · 2026-08-13T06:22:26Z

+        "PKCE Code Verifier", Collections.emptyMap(), codeVerifierTTL);
+  }
+
+  @javax.annotation.Nullable


Can be imported?

vsethi09 · 2026-08-13T06:22:49Z

+  public String getPKCECodeVerifier(String provider, String state) {
+    String key = getPKCEKey(provider, state);
+    try {
+      return new String(secureStore.getData(NamespaceId.SYSTEM.getNamespace(), key), java.nio.charset.StandardCharsets.UTF_8);


can be imported?

vsethi09 · 2026-08-13T06:22:59Z

+    String key = getPKCEKey(provider, state);
+    try {
+      return new String(secureStore.getData(NamespaceId.SYSTEM.getNamespace(), key), java.nio.charset.StandardCharsets.UTF_8);
+    } catch (Exception e) {


Catch specific exception?

Done
In interface > Exception : if the specified namespace or name does not exist

nit: Can be simplified as

catch (IOException | Exception e) { throw new OAuthStoreException("Failed to read PKCE code verifier from secure storage", e); }

Actually we cannot : there's compilation issue : Types in multi-catch must be disjoint: 'java. io. IOException' is a subclass of 'java. lang. Exception'

vsethi09 · 2026-08-13T06:23:37Z

+    try {
+      secureStoreManager.delete(NamespaceId.SYSTEM.getNamespace(), key);
+    } catch (Exception e) {
+      // Ignore if not found


Check if the error was not found.

The NotFoundException and SecureKeyNotFoundException both are a part of cdap-common which not a dependency in this module.

Is it not safe to assume that any exception caught here would be of type NotFoundException or SecureKeyNotFoundException? There could be other kind of Exceptions here, like RuntimeException.

Is it safe to ignore the exception and not propagate it?

For delete :
Yes , NotFoundException or SecureKeyNotFoundException is Safe to ignore here.

For any read operation error, we are catching IOexception.

But for any other exception yes we should catch ..

There is a UNCLEAN solution like :

catch (Exception e) { if (e.getClass().getName().endsWith("NotFoundException")) { // Ignore if already deleted return; } throw new OAuthStoreException("Failed to delete PKCE code verifier from secure storage", e); }

I don't think we should throw exception for NOT FOUND and break the flow.

Moreover , it's a delete op.

the PKCECodeVerifier anyway should be written with TTL , even if for some runtime it is missed, it will be cleaned up Eventually [ for GCP secret manager ]

Let me know your thoughts.

vsethi09 · 2026-08-13T06:24:38Z

+    try {
+      return new String(secureStore.getData(NamespaceId.SYSTEM.getNamespace(), key), java.nio.charset.StandardCharsets.UTF_8);
+    } catch (Exception e) {
+      return null;


Just curious, why is null returned here?

Was done to handle it at OauthHandler level to have a better error message for the user but it's not clean .
Removed it.

vsethi09 · 2026-08-13T06:25:46Z

    return userAgent;
  }

+  public enum AuthType {


Can be moved to a separate file as the scope of this enum is not limited to this class. It is referenced in other java packages.

vsethi09 · 2026-08-13T06:27:07Z

+    Map<String, String> prefs = null;
+    try {
+      prefs = context.getPreferencesForNamespace(NamespaceId.SYSTEM.getNamespace(), true);
+    } catch (Exception e) {


catch specific exception

Unreadability of Preferences should not stop the start up of Studio service.
But yes, added catch for the mentioned / known exceptions.

vsethi09 · 2026-08-13T06:27:42Z

-    this.oauthStore = new OAuthStore(context, context, context.getAdmin());
+
+    this.oauthStore = new OAuthStore(context, context, context.getAdmin(),
+        getPrefOrDefault(prefs, PREF_PKCE_CODE_VERIFIER_TTL, 900));


define const for default value for better visibility

vsethi09 · 2026-08-13T06:30:07Z

+        try {
+          oauthStore.storePKCECodeVerifier(provider, state, codeVerifier);
+        } catch (Exception e) {
+          throw new OAuthServiceException(java.net.HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to store PKCE code verifier", e);


Can be imported?

vsethi09 · 2026-08-13T06:52:25Z

    }
  }
+
+  private long getPrefOrDefault(java.util.Map<String, String> prefs, String key, long defaultValue) {


Can be imported?

vsethi09 · 2026-08-13T07:22:09Z


  @Override
  public void initialize(SystemHttpServiceContext context) throws Exception {
+    Map<String, String> prefs = null;


Initialise empty map, so that null checks are not needed at multiple places.

vsethi09 · 2026-08-13T17:35:22Z

+
+  private String generateCodeChallenge(String codeVerifier) throws OAuthServiceException {
+    try {
+      MessageDigest md = java.security.MessageDigest.getInstance("SHA-256");


Can be imported?

sonarqubecloud · 2026-08-14T09:53:28Z

Quality Gate failed

Failed conditions
42.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

sahusanket requested a review from vsethi09 August 12, 2026 22:25

sahusanket self-assigned this Aug 12, 2026

sahusanket added the build Triggers github actions build label Aug 12, 2026

gemini-code-assist Bot reviewed Aug 12, 2026

View reviewed changes

vsethi09 reviewed Aug 13, 2026

View reviewed changes

Comment thread ...ap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java

vsethi09 reviewed Aug 13, 2026

View reviewed changes

Comment thread ...ap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthServiceTest.java

vsethi09 reviewed Aug 13, 2026

View reviewed changes

vsethi09 requested changes Aug 13, 2026

View reviewed changes

sahusanket force-pushed the CDAP-21265_pkce_oath branch 2 times, most recently from 544b0a1 to e964c6d Compare August 13, 2026 13:33

vsethi09 reviewed Aug 13, 2026

View reviewed changes

vsethi09 requested changes Aug 13, 2026

View reviewed changes

CDAP-21261 : Add support for Refresh Token Rotation

edf1662

sahusanket force-pushed the CDAP-21265_pkce_oath branch from e964c6d to edf1662 Compare August 14, 2026 07:53

	@Nullable
	private final AuthType authType;
	private final AuthType authType;

Uh oh!

Conversation

sahusanket commented Aug 12, 2026

Title:

Description:

TESTING:

1. PKCE Flow (AuthType = PKCE)

2. Standard Flow (AuthType = STANDARD or default)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sahusanket Aug 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 14, 2026

Quality Gate failed

Uh oh!

Reviewers

Assignees

sahusanket Aug 13, 2026 •

edited

Loading