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

Upgrading to new Key Vault (and MSAL) libraries #1375

Closed
wants to merge 46 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
f4555d6
First pass at porting to new Key Vault
JonathanGiles Jun 24, 2020
892eb2b
Everything now compiles, but I'm sure there are numerous bugs, and te…
JonathanGiles Jun 25, 2020
088d2f7
Beginning to re-enable some disabled unit tests
JonathanGiles Jun 25, 2020
30e7fdf
Add requirement for a tenant ID to be set to run tests
JonathanGiles Jun 25, 2020
2d62262
Minor tidy ups
JonathanGiles Jun 30, 2020
a6b75cd
Upgrade gradle file to include equivalent dependencies as in pom.xml,…
JonathanGiles Jun 30, 2020
5e37322
add -DtenantID=$(tenantID)
peterbae Jul 3, 2020
1df8828
Merge pull request #1 from peterbae/keyvault-upgrade
JonathanGiles Jul 5, 2020
8c9a944
Updating APIs to propagate tenantId. Still I am sure the tests will f…
JonathanGiles Jul 6, 2020
f7f12d9
Merge branch 'keyvault-upgrade' of github.com:JonathanGiles/mssql-jdb…
JonathanGiles Jul 6, 2020
cc0ac7f
Updating method call to add missing tenantId
JonathanGiles Jul 6, 2020
86fc00c
Wiring in vaultBaseUrl env var lookup for Key Vault
JonathanGiles Jul 6, 2020
8fec202
Debugging key path format (hopefully)
JonathanGiles Jul 7, 2020
9f44f0f
Clean up algorithm code
JonathanGiles Jul 7, 2020
dab428f
Adding validation for vaultBaseURL
JonathanGiles Jul 7, 2020
398c2d8
Fixes for Key and Crypto client
srnagar Aug 12, 2020
1b363f7
Merge pull request #2 from srnagar/keyvault-upgrade
JonathanGiles Aug 12, 2020
948939f
Merge from master
srnagar Aug 12, 2020
6d014b5
Merge pull request #3 from srnagar/keyvault-upgrade
JonathanGiles Aug 12, 2020
dcf2f79
Fix compilation errors after merging from master
srnagar Aug 12, 2020
62a83e4
Merge pull request #4 from srnagar/keyvault-upgrade
JonathanGiles Aug 12, 2020
73d160f
Add tenantID to pipeline yml
srnagar Aug 13, 2020
fba2a23
Merge pull request #5 from srnagar/keyvault-upgrade
JonathanGiles Aug 13, 2020
f006a63
Fix unit test
srnagar Aug 13, 2020
b2db031
Merge pull request #6 from srnagar/keyvault-upgrade
JonathanGiles Aug 13, 2020
47a7b94
Address PR comments
srnagar Aug 18, 2020
9b1cac0
Merge remote-tracking branch 'upstream/dev' into keyvault-upgrade
srnagar Aug 18, 2020
fcf327e
Merge pull request #7 from srnagar/keyvault-upgrade
JonathanGiles Aug 18, 2020
23b7f6a
Remove tenant Id and fix adal issues
srnagar Aug 18, 2020
4121bc5
Merge pull request #8 from srnagar/keyvault-upgrade
JonathanGiles Aug 18, 2020
37c676d
Fix test failures
srnagar Aug 18, 2020
8e8517b
Merge pull request #9 from srnagar/keyvault-upgrade
JonathanGiles Aug 18, 2020
ee3b5c8
Use cached key and crypto clients
srnagar Aug 19, 2020
48c6dac
Merge pull request #10 from srnagar/keyvault-upgrade
JonathanGiles Aug 19, 2020
6baad02
Update license header and cleanup pipeline builder
srnagar Aug 20, 2020
a053426
Merge pull request #11 from srnagar/keyvault-upgrade
JonathanGiles Aug 20, 2020
0a3cb0f
Fix unit tests
srnagar Aug 20, 2020
684ecce
Merge pull request #12 from srnagar/keyvault-upgrade
JonathanGiles Aug 20, 2020
e4206e1
Java 8 compatibility
srnagar Aug 24, 2020
d7c2305
Merge pull request #13 from srnagar/keyvault-upgrade
JonathanGiles Aug 24, 2020
be1d739
Change Set.of to Collections.singleton()
srnagar Aug 24, 2020
1423d93
Remote tenant id from JDBC tests
srnagar Aug 24, 2020
e077ffe
Merge pull request #14 from srnagar/keyvault-upgrade
JonathanGiles Aug 24, 2020
f749361
Merge remote-tracking branch 'upstream/dev' into keyvault-upgrade
srnagar Aug 24, 2020
b08233c
Merge from dev
srnagar Aug 24, 2020
49b802e
Merge pull request #15 from srnagar/keyvault-upgrade
JonathanGiles Aug 25, 2020
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
10 changes: 5 additions & 5 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ jobs:
displayName: 'Maven build jre14'
inputs:
mavenPomFile: 'pom.xml'
goals: 'clean dependency:purge-local-repository -Dmssql_jdbc_test_connection_properties=jdbc:sqlserver://$(Target_SQL)$(server_domain);$(database);$(user);$(password); install -Pjre14 -DuserNTLM=$(userNTLM) -DpasswordNTLM=$(passwordNTLM) -DdomainNTLM=$(domainNTLM) -DexcludedGroups=$(Ex_Groups) -Dpkcs12_truststore_password=$(pkcs12_truststore_password) -Dpkcs12_truststore=$(pkcs12_truststore.secureFilePath)
-DapplicationClientID=$(applicationClientID) -DapplicationKey=$(applicationKey) -DkeyID=$(keyID) -DwindowsKeyPath=$(windowsKeyPath) -DenclaveAttestationUrl=$(enclaveAttestationUrl) -DenclaveAttestationProtocol=$(enclaveAttestationProtocol) -DenclaveServer=$(enclaveServer)'
goals: 'clean dependency:purge-local-repository -Dmssql_jdbc_test_connection_properties=jdbc:sqlserver://$(Target_SQL)$(server_domain);$(database);$(user);$(password); install -Pjre14 -DuserNTLM=$(userNTLM) -DpasswordNTLM=$(passwordNTLM) -DdomainNTLM=$(domainNTLM) -DexcludedGroups=$(Ex_Groups) -Dpkcs12_truststore_password=$(pkcs12_truststore_password) -Dpkcs12_truststore=$(pkcs12_truststore.secureFilePath)
-DapplicationClientID=$(applicationClientID) -DapplicationKey=$(applicationKey) -DkeyID=$(keyID) -DwindowsKeyPath=$(windowsKeyPath) -DenclaveAttestationUrl=$(enclaveAttestationUrl) -DenclaveAttestationProtocol=$(enclaveAttestationProtocol) -DenclaveServer=$(enclaveServer) -DtenantID=$(tenantID)'
Copy link
Contributor

Choose a reason for hiding this comment

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

tenantID not necessary anymore

testResultsFiles: '**/TEST-*.xml'
testRunTitle: 'Maven build jre14'
javaHomeOption: Path
Expand All @@ -49,7 +49,7 @@ jobs:
inputs:
mavenPomFile: 'pom.xml'
goals: 'clean dependency:purge-local-repository -Dmssql_jdbc_test_connection_properties=jdbc:sqlserver://$(Target_SQL)$(server_domain);$(database);$(user);$(password); install -Pjre11 -DuserNTLM=$(userNTLM) -DpasswordNTLM=$(passwordNTLM) -DdomainNTLM=$(domainNTLM) -DexcludedGroups=$(Ex_Groups) -Dpkcs12_truststore_password=$(pkcs12_truststore_password) -Dpkcs12_truststore=$(pkcs12_truststore.secureFilePath)
-DapplicationClientID=$(applicationClientID) -DapplicationKey=$(applicationKey) -DkeyID=$(keyID) -DwindowsKeyPath=$(windowsKeyPath) -DenclaveAttestationUrl=$(enclaveAttestationUrl) -DenclaveAttestationProtocol=$(enclaveAttestationProtocol) -DenclaveServer=$(enclaveServer)'
-DapplicationClientID=$(applicationClientID) -DapplicationKey=$(applicationKey) -DkeyID=$(keyID) -DwindowsKeyPath=$(windowsKeyPath) -DenclaveAttestationUrl=$(enclaveAttestationUrl) -DenclaveAttestationProtocol=$(enclaveAttestationProtocol) -DenclaveServer=$(enclaveServer) -DtenantID=$(tenantID)'
testResultsFiles: '**/TEST-*.xml'
testRunTitle: 'Maven build jre11'
javaHomeOption: Path
Expand All @@ -58,8 +58,8 @@ jobs:
displayName: 'Maven build jre8'
inputs:
mavenPomFile: 'pom.xml'
goals: 'clean dependency:purge-local-repository -Dmssql_jdbc_test_connection_properties=jdbc:sqlserver://$(Target_SQL)$(server_domain);$(database);$(user);$(password); install -Pjre8 -DuserNTLM=$(userNTLM) -DpasswordNTLM=$(passwordNTLM) -DdomainNTLM=$(domainNTLM) -DexcludedGroups=$(Ex_Groups) -Dpkcs12_truststore_password=$(pkcs12_truststore_password) -Dpkcs12_truststore=$(pkcs12_truststore.secureFilePath)
-DapplicationClientID=$(applicationClientID) -DapplicationKey=$(applicationKey) -DkeyID=$(keyID) -DwindowsKeyPath=$(windowsKeyPath) -DenclaveAttestationUrl=$(enclaveAttestationUrl) -DenclaveAttestationProtocol=$(enclaveAttestationProtocol) -DenclaveServer=$(enclaveServer)'
goals: 'clean dependency:purge-local-repository -Dmssql_jdbc_test_connection_properties=jdbc:sqlserver://$(Target_SQL)$(server_domain);$(database);$(user);$(password); install -Pjre8 -DuserNTLM=$(userNTLM) -DpasswordNTLM=$(passwordNTLM) -DdomainNTLM=$(domainNTLM) -DexcludedGroups=$(Ex_Groups) -Dpkcs12_truststore_password=$(pkcs12_truststore_password) -Dpkcs12_truststore=$(pkcs12_truststore.secureFilePath)
-DapplicationClientID=$(applicationClientID) -DapplicationKey=$(applicationKey) -DkeyID=$(keyID) -DwindowsKeyPath=$(windowsKeyPath) -DenclaveAttestationUrl=$(enclaveAttestationUrl) -DenclaveAttestationProtocol=$(enclaveAttestationProtocol) -DenclaveServer=$(enclaveServer) -DtenantID=$(tenantID)'
testResultsFiles: '**/TEST-*.xml'
testRunTitle: 'Maven build jre8'
javaHomeOption: Path
Expand Down
21 changes: 9 additions & 12 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,12 @@ repositories {
dependencies {
compile 'org.osgi:org.osgi.core:6.0.0',
'org.osgi:org.osgi.compendium:5.0.0'
compileOnly 'com.microsoft.azure:azure-keyvault:1.2.2',
'com.microsoft.azure:azure-keyvault-webkey:1.2.1',
'com.microsoft.rest:client-runtime:1.7.0',
'com.microsoft.azure:adal4j:1.6.4',
compileOnly 'com.azure:azure-security-keyvault-keys:4.2.0',
'com.azure:azure-identity:1.1.0',
'org.antlr:antlr4-runtime:4.7.2',
'com.google.code.gson:gson:2.8.6',
'org.bouncycastle:bcprov-jdk15on:1.64',
'org.bouncycastle:bcpkix-jdk15on:1.64'
'org.bouncycastle:bcprov-jdk15on:1.65',
'org.bouncycastle:bcpkix-jdk15on:1.65'
testCompile 'org.junit.platform:junit-platform-console:1.5.2',
'org.junit.platform:junit-platform-commons:1.5.2',
'org.junit.platform:junit-platform-engine:1.5.2',
Expand All @@ -127,15 +125,14 @@ dependencies {
'org.junit.jupiter:junit-jupiter-api:5.6.0',
'org.junit.jupiter:junit-jupiter-engine:5.6.0',
'org.junit.jupiter:junit-jupiter-params:5.6.0',
'com.zaxxer:HikariCP:3.4.1',
'org.apache.commons:commons-dbcp2:2.6.0',
'com.zaxxer:HikariCP:3.4.2',
'org.apache.commons:commons-dbcp2:2.7.0',
'org.slf4j:slf4j-nop:1.7.29',
'org.antlr:antlr4-runtime:4.7.2',
'org.eclipse.gemini.blueprint:gemini-blueprint-mock:2.1.0.RELEASE',
'com.google.code.gson:gson:2.8.6',
'org.bouncycastle:bcprov-jdk15on:1.64',
'com.microsoft.azure:adal4j:1.6.4',
'com.microsoft.azure:azure-keyvault:1.2.2',
'com.microsoft.azure:azure-keyvault-webkey:1.2.1',
'org.bouncycastle:bcprov-jdk15on:1.65',
'com.azure:azure-security-keyvault-keys:4.2.0',
'com.azure:azure-identity:1.1.0',
'com.h2database:h2:1.4.200'
}
23 changes: 7 additions & 16 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,8 @@
<releaseExt>-preview</releaseExt>

<!-- Driver Dependencies -->
<azure.keyvault.version>1.2.4</azure.keyvault.version>
<azure.adal4j.version>1.6.5</azure.adal4j.version>
<rest.client.version>1.7.4</rest.client.version>
<azure.keyvault.version>4.1.4</azure.keyvault.version>
<azure.identity.version>1.0.7</azure.identity.version>
<osgi.core.version>6.0.0</osgi.core.version>
<osgi.comp.version>5.0.0</osgi.comp.version>
<antlr.runtime.version>4.7.2</antlr.runtime.version>
Expand All @@ -86,22 +85,14 @@

<dependencies>
<dependency>
<groupId>com.microsoft.azure</groupId>
<artifactId>azure-keyvault</artifactId>
<groupId>com.azure</groupId>
<artifactId>azure-security-keyvault-keys</artifactId>
<version>${azure.keyvault.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>com.microsoft.azure</groupId>
<artifactId>adal4j</artifactId>
<version>${azure.adal4j.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>com.microsoft.rest</groupId>
<artifactId>client-runtime</artifactId>
<version>${rest.client.version}</version>
<optional>true</optional>
<groupId>com.azure</groupId>
<artifactId>azure-identity</artifactId>
<version>${azure.identity.version}</version>
</dependency>

<!-- dependencies for ANTLR -->
Expand Down
159 changes: 95 additions & 64 deletions src/main/java/com/microsoft/sqlserver/jdbc/KeyVaultCredential.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,86 +5,117 @@

package com.microsoft.sqlserver.jdbc;

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;

import com.microsoft.aad.adal4j.AuthenticationContext;
import com.microsoft.aad.adal4j.AuthenticationResult;
import com.microsoft.aad.adal4j.ClientCredential;
import com.microsoft.azure.keyvault.authentication.KeyVaultCredentials;

import com.azure.core.annotation.Immutable;
import com.azure.core.credential.AccessToken;
import com.azure.core.credential.TokenCredential;
import com.azure.core.credential.TokenRequestContext;
import com.azure.core.util.logging.ClientLogger;
import com.microsoft.aad.msal4j.ClientCredentialFactory;
import com.microsoft.aad.msal4j.ClientCredentialParameters;
import com.microsoft.aad.msal4j.ConfidentialClientApplication;
import com.microsoft.aad.msal4j.IAuthenticationResult;
import com.microsoft.aad.msal4j.IClientCredential;
import com.microsoft.aad.msal4j.SilentParameters;
import java.net.MalformedURLException;
import java.time.OffsetDateTime;
import java.time.ZoneOffset;
import java.util.HashSet;
import java.util.Objects;
import java.util.concurrent.CompletableFuture;
import reactor.core.publisher.Mono;

/**
*
* An implementation of ServiceClientCredentials that supports automatic bearer token refresh.
*
* An AAD credential that acquires a token with a client secret for an AAD application.
*/
class KeyVaultCredential extends KeyVaultCredentials {

SQLServerKeyVaultAuthenticationCallback authenticationCallback = null;
String clientId = null;
String clientKey = null;
String accessToken = null;
@Immutable
class KeyVaultCredential implements TokenCredential {
private final ClientLogger logger = new ClientLogger(KeyVaultCredential.class);
private final String clientId;
private final String clientSecret;
private String authorization;
private ConfidentialClientApplication confidentialClientApplication;

KeyVaultCredential(String clientId) throws SQLServerException {
/**
* Creates a KeyVaultCredential with the given identity client options.
*
* @param clientId the client ID of the application
* @param clientSecret the secret value of the AAD application.
*/
KeyVaultCredential(String clientId, String clientSecret) {
Objects.requireNonNull(clientSecret, "'clientSecret' cannot be null.");
Objects.requireNonNull(clientSecret, "'clientId' cannot be null.");
this.clientId = clientId;
this.clientSecret = clientSecret;
}

KeyVaultCredential() {}

KeyVaultCredential(String clientId, String clientKey) {
this.clientId = clientId;
this.clientKey = clientKey;
@Override
public Mono<AccessToken> getToken(TokenRequestContext request) {
return authenticateWithConfidentialClientCache(request)
.onErrorResume(t -> Mono.empty())
.switchIfEmpty(Mono.defer(() -> authenticateWithConfidentialClient(request)));
}

KeyVaultCredential(SQLServerKeyVaultAuthenticationCallback authenticationCallback) {
this.authenticationCallback = authenticationCallback;
public KeyVaultCredential setAuthorization(String authorization) {
if (this.authorization != null && this.authorization.equals(authorization)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

our coding standard: null != this.authorization as it prevents accidental assignment

return this;
}
this.authorization = authorization;
confidentialClientApplication = getConfidentialClientApplication();
return this;
}

public String doAuthenticate(String authorization, String resource, String scope) {
String accessToken = null;
if (null == authenticationCallback) {
if (null == clientKey) {
try {
SqlFedAuthToken token = SQLServerSecurityUtility.getMSIAuthToken(resource, clientId);
accessToken = (null != token) ? token.accessToken : null;
} catch (Exception e) {
throw new RuntimeException(e);
}
} else {
AuthenticationResult token = getAccessTokenFromClientCredentials(authorization, resource, clientId,
clientKey);
accessToken = token.getAccessToken();
}
private ConfidentialClientApplication getConfidentialClientApplication() {
if (clientId == null) {
throw logger.logExceptionAsError(new IllegalArgumentException(
"A non-null value for client ID must be provided for user authentication."));
}

if (authorization == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

null == authorization (see above)

throw logger.logExceptionAsError(new IllegalArgumentException(
"A non-null value for authorization must be provided for user authentication."));
}

IClientCredential credential;
if (clientSecret != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

null != clientSecret (see above)

credential = ClientCredentialFactory.create(clientSecret);
} else {
accessToken = authenticationCallback.getAccessToken(authorization, resource, scope);
throw logger.logExceptionAsError(
new IllegalArgumentException("Must provide client secret."));
}
ConfidentialClientApplication.Builder applicationBuilder =
ConfidentialClientApplication.builder(clientId, credential);
try {
applicationBuilder = applicationBuilder.authority(authorization);
} catch (MalformedURLException e) {
throw logger.logExceptionAsWarning(new IllegalStateException(e));
}
return accessToken;
return applicationBuilder.build();
}

private static AuthenticationResult getAccessTokenFromClientCredentials(String authorization, String resource,
String clientId, String clientKey) {
AuthenticationContext context = null;
AuthenticationResult result = null;
ExecutorService service = null;
try {
service = Executors.newFixedThreadPool(1);
context = new AuthenticationContext(authorization, false, service);
ClientCredential credentials = new ClientCredential(clientId, clientKey);
Future<AuthenticationResult> future = context.acquireToken(resource, credentials, null);
result = future.get();
} catch (Exception e) {
throw new RuntimeException(e);
} finally {
if (null != service) {
service.shutdown();
private Mono<AccessToken> authenticateWithConfidentialClientCache(TokenRequestContext request) {
return Mono.fromFuture(() -> {
SilentParameters.SilentParametersBuilder parametersBuilder = SilentParameters
.builder(new HashSet<>(request.getScopes()));
try {
return confidentialClientApplication.acquireTokenSilently(parametersBuilder.build());
} catch (MalformedURLException e) {
return getFailedCompletableFuture(logger.logExceptionAsError(new RuntimeException(e)));
}
}
}).map(ar -> new AccessToken(ar.accessToken(),
OffsetDateTime.ofInstant(ar.expiresOnDate().toInstant(), ZoneOffset.UTC)))
.filter(t -> !t.isExpired());
}

if (null == result) {
throw new RuntimeException("authentication result was null");
}
return result;
private CompletableFuture<IAuthenticationResult> getFailedCompletableFuture(Exception e) {
CompletableFuture<IAuthenticationResult> completableFuture = new CompletableFuture<>();
completableFuture.completeExceptionally(e);
return completableFuture;
}

private Mono<AccessToken> authenticateWithConfidentialClient(TokenRequestContext request) {
return Mono.fromFuture(() -> confidentialClientApplication
.acquireToken(ClientCredentialParameters.builder(new HashSet<>(request.getScopes())).build()))
.map(ar -> new AccessToken(ar.accessToken(),
OffsetDateTime.ofInstant(ar.expiresOnDate().toInstant(), ZoneOffset.UTC)));
}
}
Loading