Skip to content
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

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

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
1741680
Draft OAuth authz code flow
sfc-gh-dheyman Nov 30, 2024
195ff36
Refactor
sfc-gh-dheyman Nov 30, 2024
a3dad01
Implement full flow
sfc-gh-dheyman Dec 2, 2024
0982aaf
Add wiremock test
sfc-gh-dheyman Dec 2, 2024
4acd8df
refactored
sfc-gh-dheyman Dec 3, 2024
214c98c
Add test scenarios
sfc-gh-dheyman Dec 3, 2024
dc55937
Added support for Okta oauth authorization code flow
sfc-gh-dheyman Dec 4, 2024
857d8a2
Add logs
sfc-gh-dheyman Dec 4, 2024
dc578a7
Merge branch 'master' into oauth-code-flow
sfc-gh-dheyman Dec 4, 2024
4662dfc
format
sfc-gh-dheyman Dec 4, 2024
c110329
Merge branch 'oauth-code-flow' of github.com:snowflakedb/snowflake-jd…
sfc-gh-dheyman Dec 4, 2024
06b0e24
reformat
sfc-gh-dheyman Dec 4, 2024
86d61ef
Fix shading
sfc-gh-dheyman Dec 4, 2024
c8de884
Extracted wiremock config to files
sfc-gh-dheyman Dec 4, 2024
77a9ce0
linkage checker
sfc-gh-dheyman Dec 5, 2024
9365dbf
CR suggestions applied
sfc-gh-dheyman Dec 5, 2024
971ba0d
CR suggestions applied
sfc-gh-dheyman Dec 5, 2024
538d61b
CR
sfc-gh-dheyman Dec 5, 2024
c8af6a8
Reformat
sfc-gh-dheyman Dec 5, 2024
b1b7854
Fix attempt at flaky wiremock tests
sfc-gh-dheyman Dec 5, 2024
26e67d6
Add internal api
sfc-gh-dheyman Dec 5, 2024
e32e81b
Merge branch 'master' of github.com:snowflakedb/snowflake-jdbc into o…
sfc-gh-dheyman Dec 6, 2024
e346a58
SNOW-1831099: OAuth Client Credentials Flow Implementation (#1993)
sfc-gh-dheyman Dec 10, 2024
05565c7
Merge branch 'master' of github.com:snowflakedb/snowflake-jdbc into o…
sfc-gh-dheyman Dec 10, 2024
1e4971c
Added copyright
sfc-gh-dheyman Dec 10, 2024
e8dc943
Refactor
sfc-gh-dheyman Dec 10, 2024
a45009a
SNOW-1825471: PAT authentication support (#1995)
sfc-gh-dheyman Dec 12, 2024
862b250
Add response from redirect server
sfc-gh-dheyman Dec 12, 2024
0312556
SNOW-1831103/SNOW-1853435: Refresh token & OAuth tokens caching suppo…
sfc-gh-dheyman Dec 20, 2024
53a307b
Merge branch 'master' into oauth-code-flow
sfc-gh-dheyman Dec 20, 2024
db9949c
Remove unnecessary import from AbstractDriverIT
sfc-gh-dheyman Dec 20, 2024
2b3328f
Add authorization code redirect request handler & tests
sfc-gh-dheyman Jan 2, 2025
cb7fad0
Refactor log
sfc-gh-dheyman Jan 2, 2025
a073c6b
CR suggestions applied
sfc-gh-dheyman Jan 8, 2025
5c9d8eb
Remove import
sfc-gh-dheyman Jan 8, 2025
935acb9
Add fixes
sfc-gh-dheyman Jan 10, 2025
cdff04e
Remove obsolete config
sfc-gh-dheyman Jan 10, 2025
fa3761b
Merge branch 'master' into oauth-code-flow
sfc-gh-dheyman Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion FIPS/scripts/check_content.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ set -o pipefail

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null && pwd )"

if jar tvf $DIR/../target/snowflake-jdbc-fips.jar | awk '{print $8}' | grep -v -E "/$" | grep -v -E "^(net|com)/snowflake" | grep -v -E "(com|net)/\$" | grep -v -E "^META-INF" | grep -v -E "^mozilla" | grep -v -E "^com/sun/jna" | grep -v com/sun/ | grep -v mime.types | grep -v -E "^com/github/luben/zstd/" | grep -v -E "^aix/" | grep -v -E "^darwin/" | grep -v -E "^freebsd/" | grep -v -E "^linux/" | grep -v -E "^win/"; then
if jar tvf $DIR/../target/snowflake-jdbc-fips.jar | awk '{print $8}' | grep -v -E "/$" | grep -v -E "^(net|com)/snowflake" | grep -v -E "(com|net)/\$" | grep -v -E "^META-INF" | grep -v -E "^iso3166_" | grep -v -E "^mozilla" | grep -v -E "^com/sun/jna" | grep -v com/sun/ | grep -v mime.types | grep -v -E "^com/github/luben/zstd/" | grep -v -E "^aix/" | grep -v -E "^darwin/" | grep -v -E "^freebsd/" | grep -v -E "^linux/" | grep -v -E "^win/"; then
echo "[ERROR] JDBC jar includes class not under the snowflake namespace"
exit 1
fi
2 changes: 1 addition & 1 deletion ci/scripts/check_content.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ set -o pipefail

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null && pwd )"

if jar tvf $DIR/../../target/snowflake-jdbc${package_modifier}.jar | awk '{print $8}' | grep -v -E "/$" | grep -v -E "^(net|com)/snowflake" | grep -v -E "(com|net)/\$" | grep -v -E "^META-INF" | grep -v -E "^mozilla" | grep -v -E "^com/sun/jna" | grep -v com/sun/ | grep -v mime.types | grep -v -E "^com/github/luben/zstd/" | grep -v -E "^aix/" | grep -v -E "^darwin/" | grep -v -E "^freebsd/" | grep -v -E "^linux/" | grep -v -E "^win/"; then
if jar tvf snowflake-jdbc.jar | awk '{print $8}' | grep -v -E "/$" | grep -v -E "^(net|com)/snowflake" | grep -v -E "(com|net)/\$" | grep -v -E "^META-INF" | grep -v -E "^iso3166_" | grep -v -E "^mozilla" | grep -v -E "^com/sun/jna" | grep -v com/sun/ | grep -v mime.types | grep -v -E "^com/github/luben/zstd/" | grep -v -E "^aix/" | grep -v -E "^darwin/" | grep -v -E "^freebsd/" | grep -v -E "^linux/" | grep -v -E "^win/"; then
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
echo "[ERROR] JDBC jar includes class not under the snowflake namespace"
exit 1
fi
Expand Down
16 changes: 13 additions & 3 deletions parent-pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<apache.httpcore.version>4.4.16</apache.httpcore.version>
<zstd-jni.version>1.5.6-5</zstd-jni.version>
<arrow.version>17.0.0</arrow.version>
<asm.version>9.3</asm.version>
<asm.version>9.6</asm.version>
<avro.version>1.8.1</avro.version>
<awaitility.version>4.2.0</awaitility.version>
<awssdk.version>1.12.655</awssdk.version>
Expand Down Expand Up @@ -60,7 +60,7 @@
<javax.servlet.version>3.1.0</javax.servlet.version>
<jna.version>5.13.0</jna.version>
<joda.time.version>2.8.1</joda.time.version>
<json.smart.version>2.4.9</json.smart.version>
<json.smart.version>2.5.1</json.smart.version>
<junit4.version>4.13.2</junit4.version>
<junit.version>5.11.1</junit.version>
<junit.platform.version>1.11.1</junit.platform.version>
Expand All @@ -69,7 +69,8 @@
<metrics.version>2.2.0</metrics.version>
<mockito.version>4.11.0</mockito.version>
<netty.version>4.1.115.Final</netty.version>
<nimbusds.version>9.37.3</nimbusds.version>
<nimbusds.version>9.40</nimbusds.version>
<nimbusds.oauth2.version>11.20.1</nimbusds.oauth2.version>
<opencensus.version>0.31.1</opencensus.version>
<plexus.container.version>1.0-alpha-9-stable-1</plexus.container.version>
<plexus.utils.version>3.4.2</plexus.utils.version>
Expand Down Expand Up @@ -218,6 +219,11 @@
<artifactId>nimbus-jose-jwt</artifactId>
<version>${nimbusds.version}</version>
</dependency>
<dependency>
<groupId>com.nimbusds</groupId>
<artifactId>oauth2-oidc-sdk</artifactId>
sfc-gh-dheyman marked this conversation as resolved.
Show resolved Hide resolved
<version>${nimbusds.oauth2.version}</version>
</dependency>
<dependency>
<groupId>com.yammer.metrics</groupId>
<artifactId>metrics-core</artifactId>
Expand Down Expand Up @@ -645,6 +651,10 @@
<groupId>com.nimbusds</groupId>
<artifactId>nimbus-jose-jwt</artifactId>
</dependency>
<dependency>
<groupId>com.nimbusds</groupId>
<artifactId>oauth2-oidc-sdk</artifactId>
</dependency>
<dependency>
<groupId>com.yammer.metrics</groupId>
<artifactId>metrics-core</artifactId>
Expand Down
78 changes: 70 additions & 8 deletions src/main/java/net/snowflake/client/core/SFLoginInput.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,20 @@ public class SFLoginInput {
private boolean enableClientStoreTemporaryCredential;
private boolean enableClientRequestMfaToken;

// OAuth
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
private String redirectUri;
private String clientId;
private String clientSecret;
private String externalAuthorizationUrl;
private String externalTokenRequestUrl;
private String scope;

private Duration browserResponseTimeout;

// Additional headers to add for Snowsight.
Map<String, String> additionalHttpHeadersForSnowsight;

SFLoginInput() {}
public SFLoginInput() {}
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved

Duration getBrowserResponseTimeout() {
return browserResponseTimeout;
Expand All @@ -74,7 +82,7 @@ public String getServerUrl() {
return serverUrl;
}

SFLoginInput setServerUrl(String serverUrl) {
public SFLoginInput setServerUrl(String serverUrl) {
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
this.serverUrl = serverUrl;
return this;
}
Expand Down Expand Up @@ -160,7 +168,7 @@ public SFLoginInput setAccountName(String accountName) {
return this;
}

int getLoginTimeout() {
public int getLoginTimeout() {
return loginTimeout;
}

Expand All @@ -184,7 +192,7 @@ SFLoginInput setRetryTimeout(int retryTimeout) {
return this;
}

int getAuthTimeout() {
public int getAuthTimeout() {
sfc-gh-astachowski marked this conversation as resolved.
Show resolved Hide resolved
return authTimeout;
}

Expand Down Expand Up @@ -238,11 +246,11 @@ SFLoginInput setConnectionTimeout(Duration connectionTimeout) {
return this;
}

int getSocketTimeoutInMillis() {
public int getSocketTimeoutInMillis() {
return (int) socketTimeout.toMillis();
}

SFLoginInput setSocketTimeout(Duration socketTimeout) {
public SFLoginInput setSocketTimeout(Duration socketTimeout) {
this.socketTimeout = socketTimeout;
return this;
}
Expand Down Expand Up @@ -388,11 +396,11 @@ SFLoginInput setOCSPMode(OCSPMode ocspMode) {
return this;
}

HttpClientSettingsKey getHttpClientSettingsKey() {
public HttpClientSettingsKey getHttpClientSettingsKey() {
return httpClientKey;
}

SFLoginInput setHttpClientSettingsKey(HttpClientSettingsKey key) {
public SFLoginInput setHttpClientSettingsKey(HttpClientSettingsKey key) {
this.httpClientKey = key;
return this;
}
Expand All @@ -417,6 +425,60 @@ SFLoginInput setDisableSamlURLCheck(boolean disableSamlURLCheck) {
return this;
}

public String getRedirectUri() {
return redirectUri;
}

public SFLoginInput setRedirectUri(String redirectUri) {
this.redirectUri = redirectUri;
return this;
}

public String getClientId() {
return clientId;
}

public SFLoginInput setClientId(String clientId) {
this.clientId = clientId;
return this;
}

public String getClientSecret() {
return clientSecret;
}

public SFLoginInput setClientSecret(String clientSecret) {
this.clientSecret = clientSecret;
return this;
}

public String getExternalAuthorizationUrl() {
return externalAuthorizationUrl;
}

public SFLoginInput setExternalAuthorizationUrl(String externalAuthorizationUrl) {
this.externalAuthorizationUrl = externalAuthorizationUrl;
return this;
}

public String getExternalTokenRequestUrl() {
return externalTokenRequestUrl;
}

public SFLoginInput setExternalTokenRequestUrl(String externalTokenRequestUrl) {
this.externalTokenRequestUrl = externalTokenRequestUrl;
return this;
}

public String getScope() {
return scope;
}

public SFLoginInput setScope(String scope) {
this.scope = scope;
return this;
}

Map<String, String> getAdditionalHttpHeadersForSnowsight() {
return additionalHttpHeadersForSnowsight;
}
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/net/snowflake/client/core/SFSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,14 @@ public synchronized void open() throws SFException, SnowflakeSQLException {
.setSessionParameters(sessionParametersMap)
.setPrivateKey((PrivateKey) connectionPropertiesMap.get(SFSessionProperty.PRIVATE_KEY))
.setPrivateKeyFile((String) connectionPropertiesMap.get(SFSessionProperty.PRIVATE_KEY_FILE))
.setClientId((String) connectionPropertiesMap.get(SFSessionProperty.CLIENT_ID))
.setClientSecret((String) connectionPropertiesMap.get(SFSessionProperty.CLIENT_SECRET))
.setRedirectUri((String) connectionPropertiesMap.get(SFSessionProperty.OAUTH_REDIRECT_URI))
.setScope((String) connectionPropertiesMap.get(SFSessionProperty.OAUTH_SCOPE))
.setExternalAuthorizationUrl(
(String) connectionPropertiesMap.get(SFSessionProperty.EXTERNAL_AUTHORIZATION_URL))
.setExternalTokenRequestUrl(
(String) connectionPropertiesMap.get(SFSessionProperty.EXTERNAL_TOKEN_REQUEST_URL))
.setPrivateKeyBase64(
(String) connectionPropertiesMap.get(SFSessionProperty.PRIVATE_KEY_BASE64))
.setPrivateKeyPwd(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oauthClientId, oauthClientSecret etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CLIENT_SECRET("clientSecret", false, String.class),
OAUTH_SCOPE("scope", false, String.class),
EXTERNAL_AUTHORIZATION_URL("externalAuthorizationUrl", false, String.class),
EXTERNAL_TOKEN_REQUEST_URL("externalTokenRequestUrl", false, String.class),
WAREHOUSE("warehouse", false, String.class),
LOGIN_TIMEOUT("loginTimeout", false, Integer.class),
NETWORK_TIMEOUT("networkTimeout", false, Integer.class),
Expand Down
27 changes: 26 additions & 1 deletion src/main/java/net/snowflake/client/core/SessionUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import net.snowflake.client.core.auth.AuthenticatorType;
import net.snowflake.client.core.auth.ClientAuthnDTO;
import net.snowflake.client.core.auth.ClientAuthnParameter;
import net.snowflake.client.core.auth.oauth.AuthorizationCodeFlowAccessTokenProvider;
import net.snowflake.client.core.auth.oauth.OauthAccessTokenProvider;
import net.snowflake.client.jdbc.ErrorCode;
import net.snowflake.client.jdbc.SnowflakeDriver;
import net.snowflake.client.jdbc.SnowflakeReauthenticationRequest;
Expand Down Expand Up @@ -138,6 +140,8 @@ public class SessionUtil {
public static int DEFAULT_CLIENT_PREFETCH_THREADS = 4;
public static int MIN_CLIENT_CHUNK_SIZE = 48;
public static int MAX_CLIENT_CHUNK_SIZE = 160;
private static final int DEFAULT_BROWSER_AUTHORIZATION_TIMEOUT_SECONDS = 180;
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved

public static Map<String, String> JVM_PARAMS_TO_PARAMS =
Stream.of(
new String[][] {
Expand Down Expand Up @@ -217,8 +221,13 @@ private static AuthenticatorType getAuthenticator(SFLoginInput loginInput) {
.equalsIgnoreCase(AuthenticatorType.EXTERNALBROWSER.name())) {
// SAML 2.0 compliant service/application
return AuthenticatorType.EXTERNALBROWSER;
} else if (loginInput
.getAuthenticator()
.equalsIgnoreCase(AuthenticatorType.OAUTH_AUTHORIZATION_CODE.name())) {
// OAuth authorization code flow authentication
return AuthenticatorType.OAUTH_AUTHORIZATION_CODE;
} else if (loginInput.getAuthenticator().equalsIgnoreCase(AuthenticatorType.OAUTH.name())) {
// OAuth Authentication
// OAuth access code Authentication
return AuthenticatorType.OAUTH;
} else if (loginInput
.getAuthenticator()
Expand Down Expand Up @@ -265,6 +274,22 @@ static SFLoginOutput openSession(
AssertUtil.assertTrue(
loginInput.getLoginTimeout() >= 0, "negative login timeout for opening session");

if (getAuthenticator(loginInput).equals(AuthenticatorType.OAUTH_AUTHORIZATION_CODE)) {
AssertUtil.assertTrue(
loginInput.getClientId() != null,
"passing clientId is required for OAUTH_AUTHORIZATION_CODE_FLOW authentication");
AssertUtil.assertTrue(
loginInput.getClientSecret() != null,
"passing clientSecret is required for OAUTH_AUTHORIZATION_CODE_FLOW authentication");
OauthAccessTokenProvider accessTokenProvider =
new AuthorizationCodeFlowAccessTokenProvider(
new SessionUtilExternalBrowser.DefaultAuthExternalBrowserHandlers(),
DEFAULT_BROWSER_AUTHORIZATION_TIMEOUT_SECONDS);
String oauthAccessToken = accessTokenProvider.getAccessToken(loginInput);
loginInput.setAuthenticator(AuthenticatorType.OAUTH.name());
loginInput.setToken(oauthAccessToken);
}

final AuthenticatorType authenticator = getAuthenticator(loginInput);
if (!authenticator.equals(AuthenticatorType.OAUTH)) {
// OAuth does not require a username
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public interface AuthExternalBrowserHandlers {
void output(String msg);
}

static class DefaultAuthExternalBrowserHandlers implements AuthExternalBrowserHandlers {
public static class DefaultAuthExternalBrowserHandlers implements AuthExternalBrowserHandlers {
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
@Override
public HttpPost build(URI uri) {
return new HttpPost(uri);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,10 @@ public enum AuthenticatorType {
/*
* Authenticator to enable token for regular login with mfa
*/
USERNAME_PASSWORD_MFA
USERNAME_PASSWORD_MFA,

/*
* Authorization code flow with browser popup
*/
OAUTH_AUTHORIZATION_CODE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a couple options:

  1. AUTHENTICATOR=OAUTH plus OAUTH_FLOW={TOKEN | AUTHORIZATION_CODE | CLIENT_CREDENTIALS }, where TOKEN is our existing default.
  2. 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.

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ public ClientAuthnDTO(Map<String, Object> data, @Nullable String inFlightCtx) {
this.inFlightCtx = inFlightCtx;
}

/** Required by Jackson */
// Required by Jackson
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
public Map<String, Object> getData() {
return data;
}

/** Required by Jackson */
// Required by Jackson
public String getInFlightCtx() {
return inFlightCtx;
}
Expand Down
Loading
Loading