SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support #1978

sfc-gh-dheyman · 2024-11-30T22:00:37Z

Overview

SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support

Pre-review self checklist

PR branch is updated with all the changes from master branch
The code is correctly formatted (run mvn -P check-style validate)
New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
The pull request name is prefixed with SNOW-XXXX:
Code is in compliance with internal logging requirements

External contributors - please answer these questions before submitting a pull request. Thanks!

What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Issue: #NNNN
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency or upgrading an existing one
- I am adding new public/protected component not marked with @SnowflakeJdbcInternalApi (note that public/protected methods/fields in classes marked with this annotation are already internal)
Please describe how your code solves the related issue.

Please write a short description of how your code change solves the related issue.

…bc into oauth-code-flow

gitguardian · 2024-12-04T21:27:52Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

ci/scripts/check_content.sh

sfc-gh-dprzybysz · 2024-12-05T08:39:51Z

parent-pom.xml

@@ -218,6 +219,11 @@
        <artifactId>nimbus-jose-jwt</artifactId>
        <version>${nimbusds.version}</version>
      </dependency>
+      <dependency>
+        <groupId>com.nimbusds</groupId>
+        <artifactId>oauth2-oidc-sdk</artifactId>


do we have security review finished for this library?

src/main/java/net/snowflake/client/core/SFLoginInput.java

sfc-gh-dprzybysz · 2024-12-05T08:47:18Z

...main/java/net/snowflake/client/core/auth/oauth/AuthorizationCodeFlowAccessTokenProvider.java

+      TokenRequest request = buildTokenRequest(loginInput, authorizationCode, pkceVerifier);
+      URI requestUri = request.getEndpointURI();
+      logger.debug(
+          "Requesting access token from: " + requestUri.getAuthority() + requestUri.getPath());


please use logger parameters instead of string concatenation, also in other logs

...main/java/net/snowflake/client/core/auth/oauth/AuthorizationCodeFlowAccessTokenProvider.java

src/main/java/net/snowflake/client/core/auth/oauth/TokenResponseDTO.java

sfc-gh-jszczerbinski

Looks good to me in general, I will need a second look once I get some more knowledge about OAuth

sfc-gh-jszczerbinski · 2024-12-05T10:48:33Z

src/main/java/net/snowflake/client/core/SFSessionProperty.java

@@ -29,6 +29,12 @@ public enum SFSessionProperty {
  AUTHENTICATOR("authenticator", false, String.class),
  OKTA_USERNAME("oktausername", false, String.class),
  PRIVATE_KEY("privateKey", false, PrivateKey.class),
+  OAUTH_REDIRECT_URI("redirectUri", false, String.class),
+  CLIENT_ID("clientID", false, String.class),


oauthClientId, oauthClientSecret etc.?

I'd prefer not to introduce new parameters, after all user can use only one authentication type at a time

sfc-gh-jszczerbinski · 2024-12-05T13:54:31Z

src/test/resources/oauth/authorization_code/successful_scenario_mapping.json

+        },
+        "bodyPatterns": [
+          {
+            "contains": "grant_type=authorization_code&code=123&redirect_uri=http%3A%2F%2Flocalhost%3A8001%2Fsnowflake%2Foauth-redirect&code_verifier="


Please verify if we indeed use PKCE

yes, we need to.

src/main/java/net/snowflake/client/core/SFLoginInput.java

sfc-gh-astachowski · 2024-12-05T15:03:15Z

src/test/java/net/snowflake/client/jdbc/OauthAuthorizationCodeFlowLatestIT.java

+  }
+
+  @Test
+  public void browserTimeoutFlowScenario() {


These error tests seem very similar, maybe we could make them parameterized? For keeping descriptive names, see Arguments.argumentSet (in case of @ArgumentProvider).

I'd do it after client credentials flow is added, because I'm not yet sure if these will remain as they are.

…auth-code-flow

sfc-gh-mhofman · 2024-12-09T14:06:49Z

src/main/java/net/snowflake/client/core/auth/AuthenticatorType.java

+  /*
+   * Authorization code flow with browser popup
+   */
+  OAUTH_AUTHORIZATION_CODE


maybe CLIENT_OAUTH would be a good name from this OAuth flows to make it different than OAuth and indicate it's on client side, WDYT? @sfc-gh-dprzybysz @sfc-gh-dheyman @sfc-gh-pfus

I don't like it. CLIENT_OAUTH is too generic. Dawid's name seems better to me.

I would rather change the original OAUTH authenticator to OAUTH_ACCESS_TOKEN or something like that, but I understand it would be a BCR.

@sfc-gh-eworoshow WDYT? did you go through any naming discussion already

I see a couple options:

AUTHENTICATOR=OAUTH plus OAUTH_FLOW={TOKEN | AUTHORIZATION_CODE | CLIENT_CREDENTIALS }, where TOKEN is our existing default.

AUTHENTICATOR={OAUTH | OAUTH_ACCESS_TOKEN, alias of OAUTH | OAUTH_AUTHORIZATION_CODE | ... }.

In both cases I think we should use the "standard" name for the OAuth flow we're implementing.

sfc-gh-pfus · 2024-12-09T14:08:34Z

linkage-checker-exclusion-rules.xml

@@ -14,11 +14,6 @@
        <Source><Package name="com.nimbusds.jose"/></Source>
        <Reason>Optional</Reason>
    </LinkageError>
-    <LinkageError>
-        <Target><Package name="org.brotli.dec"/></Target>


Why do we want to delete these lines?

sfc-gh-pfus · 2024-12-09T14:15:47Z

src/main/java/net/snowflake/client/core/auth/AuthenticatorType.java

+  /*
+   * Authorization code flow with browser popup
+   */
+  OAUTH_AUTHORIZATION_CODE


I don't like it. CLIENT_OAUTH is too generic. Dawid's name seems better to me.

sfc-gh-pfus · 2024-12-09T14:17:44Z

...main/java/net/snowflake/client/core/auth/oauth/AuthorizationCodeFlowAccessTokenProvider.java

+      String code = codeFuture.get(this.browserAuthorizationTimeoutSeconds, TimeUnit.SECONDS);
+      return new AuthorizationCode(code);
+    } catch (Exception e) {
+      if (e instanceof TimeoutException) {


Why not catch (TimeoutException)?

good idea, fixed.

sfc-gh-pfus · 2024-12-09T14:20:42Z

src/main/java/net/snowflake/client/core/auth/oauth/TokenResponseDTO.java

+  private final long expiresIn;
+  private final long refreshTokenExpiresIn;
+
+  @JsonCreator(mode = JsonCreator.Mode.PROPERTIES)


What if some of these properties are missing? Will nulls be used?

sfc-gh-dprzybysz · 2024-12-10T08:55:31Z

src/main/java/net/snowflake/client/core/SFOauthLoginInput.java

@@ -0,0 +1,51 @@
+package net.snowflake.client.core;


let's add license header like in other files

sfc-gh-dprzybysz · 2024-12-10T08:58:06Z

...main/java/net/snowflake/client/core/auth/oauth/AuthorizationCodeFlowAccessTokenProvider.java

    try {
      TokenRequest request = buildTokenRequest(loginInput, authorizationCode, pkceVerifier);
      URI requestUri = request.getEndpointURI();
      logger.debug(
-          "Requesting access token from: " + requestUri.getAuthority() + requestUri.getPath());
+          "Requesting access token from: {}", requestUri.getAuthority() + requestUri.getPath());


you can concatenate in formatted string:

"Requesting access token from: {}{}", requestUri.getAuthority(), requestUri.getPath()

sfc-gh-dprzybysz · 2024-12-10T09:00:25Z

src/main/java/net/snowflake/client/core/auth/oauth/TokenResponseDTO.java


+@SnowflakeJdbcInternalApi


package scope class does not need internal annotation

…auth-code-flow

…rt (#2009)

sfc-gh-dheyman added 2 commits November 30, 2024 22:25

Draft OAuth authz code flow

1741680

Refactor

195ff36

sfc-gh-dheyman force-pushed the oauth-code-flow branch from 119a205 to 195ff36 Compare November 30, 2024 22:07

Implement full flow

a3dad01

sfc-gh-dheyman force-pushed the oauth-code-flow branch from ec6effb to a3dad01 Compare December 2, 2024 17:38

sfc-gh-dheyman added 8 commits December 2, 2024 23:56

Add wiremock test

0982aaf

refactored

4acd8df

Add test scenarios

214c98c

Added support for Okta oauth authorization code flow

dc55937

Add logs

857d8a2

Merge branch 'master' into oauth-code-flow

dc578a7

format

4662dfc

Merge branch 'oauth-code-flow' of github.com:snowflakedb/snowflake-jd…

c110329

…bc into oauth-code-flow

sfc-gh-dheyman marked this pull request as ready for review December 4, 2024 15:12

sfc-gh-dheyman requested a review from a team as a code owner December 4, 2024 15:12

sfc-gh-dheyman changed the title ~~OAuth Authorization Code flow using Nimbus OAuth 2.0 SDK~~ SNOW-1825482: OAuth Authorization Code flow (Snowflake + Okta) Dec 4, 2024

snowflakedb deleted a comment from gitguardian bot Dec 4, 2024

reformat

06b0e24

sfc-gh-dheyman added 2 commits December 4, 2024 22:56

Fix shading

86d61ef

Extracted wiremock config to files

c8de884

sfc-gh-dprzybysz reviewed Dec 5, 2024

View reviewed changes

sfc-gh-dheyman added 3 commits December 5, 2024 10:32

linkage checker

77a9ce0

CR suggestions applied

9365dbf

CR suggestions applied

971ba0d

sfc-gh-dheyman requested review from sfc-gh-dprzybysz and sfc-gh-pfus December 5, 2024 12:27

sfc-gh-dheyman added 2 commits December 5, 2024 13:51

CR

538d61b

Reformat

c8af6a8

sfc-gh-jszczerbinski reviewed Dec 5, 2024

View reviewed changes

Fix attempt at flaky wiremock tests

b1b7854

sfc-gh-astachowski reviewed Dec 5, 2024

View reviewed changes

src/main/java/net/snowflake/client/core/SFLoginInput.java Show resolved Hide resolved

sfc-gh-astachowski reviewed Dec 5, 2024

View reviewed changes

sfc-gh-dheyman added 2 commits December 5, 2024 16:38

Add internal api

26e67d6

Merge branch 'master' of github.com:snowflakedb/snowflake-jdbc into o…

e32e81b

…auth-code-flow

sfc-gh-mhofman reviewed Dec 9, 2024

View reviewed changes

sfc-gh-pfus reviewed Dec 9, 2024

View reviewed changes

sfc-gh-dprzybysz reviewed Dec 10, 2024

View reviewed changes

sfc-gh-dheyman added 4 commits December 10, 2024 15:08

SNOW-1831099: OAuth Client Credentials Flow Implementation (#1993)

e346a58

Merge branch 'master' of github.com:snowflakedb/snowflake-jdbc into o…

05565c7

…auth-code-flow

Added copyright

1e4971c

Refactor

e8dc943

sfc-gh-dheyman changed the title ~~SNOW-1825482: OAuth Authorization Code flow (Snowflake + Okta)~~ SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support Dec 10, 2024

sfc-gh-dheyman added 7 commits December 12, 2024 14:14

SNOW-1825471: PAT authentication support (#1995)

a45009a

Add response from redirect server

862b250

SNOW-1831103/SNOW-1853435: Refresh token & OAuth tokens caching suppo…

0312556

…rt (#2009)

Merge branch 'master' into oauth-code-flow

53a307b

Remove unnecessary import from AbstractDriverIT

db9949c

Add authorization code redirect request handler & tests

2b3328f

Refactor log

cb7fad0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support #1978

SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support #1978

sfc-gh-dheyman commented Nov 30, 2024 •

edited

Loading

gitguardian bot commented Dec 4, 2024 •

edited

Loading

sfc-gh-dprzybysz Dec 5, 2024

sfc-gh-dheyman Dec 5, 2024

sfc-gh-dprzybysz Dec 5, 2024

sfc-gh-dheyman Dec 5, 2024

sfc-gh-jszczerbinski left a comment

sfc-gh-jszczerbinski Dec 5, 2024

sfc-gh-dheyman Dec 5, 2024

sfc-gh-jszczerbinski Dec 5, 2024

sfc-gh-dheyman Dec 5, 2024 •

edited

Loading

sfc-gh-astachowski Dec 5, 2024

sfc-gh-dheyman Dec 5, 2024

sfc-gh-mhofman Dec 9, 2024

sfc-gh-pfus Dec 9, 2024

sfc-gh-dheyman Dec 9, 2024

sfc-gh-mhofman Dec 10, 2024

sfc-gh-eworoshow Dec 10, 2024

sfc-gh-pfus Dec 9, 2024

sfc-gh-dheyman Dec 10, 2024

sfc-gh-pfus Dec 9, 2024

sfc-gh-pfus Dec 9, 2024

sfc-gh-dheyman Dec 10, 2024

sfc-gh-pfus Dec 9, 2024

sfc-gh-dheyman Dec 10, 2024

sfc-gh-dprzybysz Dec 10, 2024

sfc-gh-dheyman Dec 10, 2024

sfc-gh-dprzybysz Dec 10, 2024

sfc-gh-dheyman Dec 10, 2024

sfc-gh-dprzybysz Dec 10, 2024

sfc-gh-dheyman Dec 10, 2024


		@SnowflakeJdbcInternalApi

SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support #1978

Are you sure you want to change the base?

SNOW-1825482: PAT + OAuth Authorization Code + OAuth Client Credentials support #1978

Conversation

sfc-gh-dheyman commented Nov 30, 2024 • edited Loading

Overview

Pre-review self checklist

External contributors - please answer these questions before submitting a pull request. Thanks!

gitguardian bot commented Dec 4, 2024 • edited Loading

️✅ There are no secrets present in this pull request anymore.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-jszczerbinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-dheyman Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-dheyman commented Nov 30, 2024 •

edited

Loading

gitguardian bot commented Dec 4, 2024 •

edited

Loading

sfc-gh-dheyman Dec 5, 2024 •

edited

Loading