-
Notifications
You must be signed in to change notification settings - Fork 358
CDAP-21265 : Add support for PKCE #16199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright © 2024 Cask Data, Inc. | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
| * use this file except in compliance with the License. You may obtain a copy of | ||
| * the License at | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| * | ||
| */ | ||
|
|
||
| package io.cdap.cdap.datapipeline.oauth; | ||
|
|
||
| /** | ||
| * The type of OAuth 2.0 authorization flow to use. | ||
| */ | ||
| public enum AuthType { | ||
| /** | ||
| * Standard OAuth 2.0 Authorization Code flow. | ||
| */ | ||
| STANDARD, | ||
|
|
||
| /** | ||
| * OAuth 2.0 Authorization Code flow with Proof Key for Code Exchange (PKCE). | ||
| * Provides enhanced security by dynamically generating a code challenge and verifier. | ||
| */ | ||
| PKCE | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,13 +46,16 @@ public class OAuthStore { | |
| private static final String TOKEN_REFRESH_URL_COL = "tokenrefreshurl"; | ||
| private static final String CREDENTIAL_ENCODING_STRATEGY_COL = "credentialencodingstrategy"; | ||
| private static final String USER_AGENT_COL = "useragent"; | ||
| private static final String AUTH_TYPE_COL = "authtype"; | ||
| private static final String CLIENT_CREDS_KEY_PREFIX = "oauthclientcreds"; | ||
| private static final String ACCESS_TOKEN_KEY_PREFIX = "oauthaccesstoken"; | ||
| private static final String REFRESH_TOKEN_KEY_PREFIX = "oauthrefreshtoken"; | ||
| private static final String PKCE_KEY_PREFIX = "oauth-pkce"; | ||
| private static final Gson GSON = new Gson(); | ||
| private final TransactionRunner transactionRunner; | ||
| private final SecureStore secureStore; | ||
| private final SecureStoreManager secureStoreManager; | ||
| private final long codeVerifierTTL; | ||
|
|
||
| public static final StructuredTableId TABLE_ID = new StructuredTableId("oauth"); | ||
| public static final StructuredTableSpecification TABLE_SPEC = new StructuredTableSpecification.Builder() | ||
|
|
@@ -61,17 +64,20 @@ public class OAuthStore { | |
| Fields.stringType(LOGIN_URL_COL), | ||
| Fields.stringType(TOKEN_REFRESH_URL_COL), | ||
| Fields.stringType(CREDENTIAL_ENCODING_STRATEGY_COL), | ||
| Fields.stringType(USER_AGENT_COL)) | ||
| Fields.stringType(USER_AGENT_COL), | ||
| Fields.stringType(AUTH_TYPE_COL)) | ||
| .withPrimaryKeys(OAUTH_PROVIDER_COL) | ||
| .build(); | ||
|
|
||
| public OAuthStore( | ||
| TransactionRunner transactionRunner, | ||
| SecureStore secureStore, | ||
| SecureStoreManager secureStoreManager) { | ||
| SecureStoreManager secureStoreManager, | ||
| long codeVerifierTTL) { | ||
| this.transactionRunner = transactionRunner; | ||
| this.secureStore = secureStore; | ||
| this.secureStoreManager = secureStoreManager; | ||
| this.codeVerifierTTL = codeVerifierTTL; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -299,6 +305,10 @@ private static String getAccessTokenKey(String oauthProvider, String credentialI | |
| return String.format("%s-%s-%s", ACCESS_TOKEN_KEY_PREFIX, oauthProvider.toLowerCase(), credentialId.toLowerCase()); | ||
| } | ||
|
|
||
| private static String getPKCEKey(String oauthProvider, String state) { | ||
| return String.format("%s-%s-%s", PKCE_KEY_PREFIX, oauthProvider.toLowerCase(), state.toLowerCase()); | ||
| } | ||
|
|
||
| private static List<Field<?>> getKey(String name) { | ||
| List<Field<?>> keyFields = new ArrayList<>(1); | ||
| keyFields.add(Fields.stringField(OAUTH_PROVIDER_COL, name)); | ||
|
|
@@ -311,6 +321,7 @@ private static OAuthProvider fromRow(StructuredRow row, OAuthClientCredentials c | |
| String tokenRefreshURL = row.getString(TOKEN_REFRESH_URL_COL); | ||
| String credentialEncodingStrategy = row.getString(CREDENTIAL_ENCODING_STRATEGY_COL); | ||
| String userAgent = row.getString(USER_AGENT_COL); | ||
| String authTypeStr = row.getString(AUTH_TYPE_COL); | ||
|
|
||
| return OAuthProvider.newBuilder() | ||
| .withName(name) | ||
|
|
@@ -321,6 +332,7 @@ private static OAuthProvider fromRow(StructuredRow row, OAuthClientCredentials c | |
| Optional.ofNullable(credentialEncodingStrategy) | ||
| .map(OAuthProvider.CredentialEncodingStrategy::valueOf).orElse(null)) | ||
| .withUserAgent(userAgent) | ||
| .withAuthType(authTypeStr != null ? AuthType.valueOf(authTypeStr) : AuthType.STANDARD) | ||
| .build(); | ||
| } | ||
|
|
||
|
|
@@ -333,6 +345,36 @@ private static List<Field<?>> getRow(OAuthProvider oauthProvider) { | |
| CREDENTIAL_ENCODING_STRATEGY_COL, | ||
| oauthProvider.getCredentialEncodingStrategy().toString())); | ||
| fields.add(Fields.stringField(USER_AGENT_COL, oauthProvider.getUserAgent())); | ||
| fields.add(Fields.stringField(AUTH_TYPE_COL, oauthProvider.getAuthType().toString())); | ||
| return fields; | ||
| } | ||
|
|
||
| public void writePKCECodeVerifier(String provider, String state, String codeVerifier) throws Exception { | ||
| String key = getPKCEKey(provider, state); | ||
| secureStoreManager.put(NamespaceId.SYSTEM.getNamespace(), key, codeVerifier, | ||
| "PKCE Code Verifier", Collections.emptyMap(), codeVerifierTTL); | ||
| } | ||
|
|
||
| public String getPKCECodeVerifier(String provider, String state) throws OAuthStoreException { | ||
| String key = getPKCEKey(provider, state); | ||
| try { | ||
| return new String(secureStore.getData(NamespaceId.SYSTEM.getNamespace(), key), StandardCharsets.UTF_8); | ||
| } catch (IOException e) { | ||
| throw new OAuthStoreException("Failed to read PKCE code verifier from secure storage", e); | ||
| } catch (Exception e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catch specific exception?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Can be simplified as catch (IOException | Exception e) {
throw new OAuthStoreException("Failed to read PKCE code verifier from secure storage", e);
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we cannot : there's compilation issue : |
||
| throw new OAuthStoreException("PKCE code verifier not found or expired for state or an " | ||
| + "unexpected error reading from secure store: ", e); | ||
| } | ||
| } | ||
|
|
||
| public void deletePKCECodeVerifier(String provider, String state) throws OAuthStoreException { | ||
| String key = getPKCEKey(provider, state); | ||
| try { | ||
| secureStoreManager.delete(NamespaceId.SYSTEM.getNamespace(), key); | ||
| } catch (IOException e) { | ||
| throw new OAuthStoreException("Failed to delete PKCE code verifier from secure storage", e); | ||
| } catch (Exception e) { | ||
|
vsethi09 marked this conversation as resolved.
|
||
| // Ignore if not found | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check if the error was not found.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not safe to assume that any exception caught here would be of type Is it safe to ignore the exception and not propagate it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For delete : For any read operation error, we are catching IOexception. But for any other exception yes we should catch .. There is a UNCLEAN solution like : I don't think we should throw exception for NOT FOUND and break the flow. Moreover , it's a delete op.
Let me know your thoughts. |
||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
authTypefield is annotated with@Nullable, but the constructor guarantees it will never be null by defaulting toAuthType.STANDARDif a null value is passed. To avoid confusion and accurately reflect the class invariants, the@Nullableannotation should be removed from the field.