Skip to content

Make KeyVaultJcaProvider can work when keyvault-uri is not set. #22488

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.

Sign up for GitHub

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

Merged
chenrujun merged 1 commit into from
Jun 29, 2021
Merged

Make KeyVaultJcaProvider can work when keyvault-uri is not set. #22488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 21 additions & 38 deletions ...curity-keyvault-jca/src/test/java/com/azure/security/keyvault/jca/KeyVaultClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,81 +28,64 @@ public class KeyVaultClientTest {
private static final String KEY_VAULT_TEST_URI_US = "https://fake.vault.usgovcloudapi.net/";
private static final String KEY_VAULT_TEST_URI_DE = "https://fake.vault.microsoftazure.de/";

private KeyVaultClient kvClient;
private KeyVaultClient keyVaultClient;

/**
* Test initialization of keyVaultBaseUri and aadAuthenticationUrl.
*
*/
@Test
public void testInitializationOfGlobalURI() {
kvClient = new KeyVaultClient(KEY_VAULT_TEST_URI_GLOBAL, null);
Assertions.assertEquals(kvClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_GLOBAL);
Assertions.assertEquals(kvClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_GLOBAL);
keyVaultClient = new KeyVaultClient(KEY_VAULT_TEST_URI_GLOBAL, null);
Assertions.assertEquals(keyVaultClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_GLOBAL);
Assertions.assertEquals(keyVaultClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_GLOBAL);
}

@Test
public void testInitializationOfCNURI() {
kvClient = new KeyVaultClient(KEY_VAULT_TEST_URI_CN, null);
Assertions.assertEquals(kvClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_CN);
Assertions.assertEquals(kvClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_CN);
keyVaultClient = new KeyVaultClient(KEY_VAULT_TEST_URI_CN, null);
Assertions.assertEquals(keyVaultClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_CN);
Assertions.assertEquals(keyVaultClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_CN);
}

@Test
public void testInitializationOfUSURI() {
kvClient = new KeyVaultClient(KEY_VAULT_TEST_URI_US, null);
Assertions.assertEquals(kvClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_US);
Assertions.assertEquals(kvClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_US);
keyVaultClient = new KeyVaultClient(KEY_VAULT_TEST_URI_US, null);
Assertions.assertEquals(keyVaultClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_US);
Assertions.assertEquals(keyVaultClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_US);
}

@Test
public void testInitializationOfDEURI() {
kvClient = new KeyVaultClient(KEY_VAULT_TEST_URI_DE, null);
Assertions.assertEquals(kvClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_DE);
Assertions.assertEquals(kvClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_DE);
keyVaultClient = new KeyVaultClient(KEY_VAULT_TEST_URI_DE, null);
Assertions.assertEquals(keyVaultClient.getKeyVaultBaseUri(), KEY_VAULT_BASE_URI_DE);
Assertions.assertEquals(keyVaultClient.getAadAuthenticationUrl(), AAD_LOGIN_URI_DE);
}

@Test
@Disabled
public void testGetAliases() {
String tenantId = System.getProperty("azure.keyvault.tenant-id");
String clientId = System.getProperty("azure.keyvault.client-id");
String clientSecret = System.getProperty("azure.keyvault.client-secret");
String keyVaultUri = System.getProperty("azure.keyvault.uri");
KeyVaultClient keyVaultClient = new KeyVaultClient(
keyVaultUri, System.getProperty("azure.keyvault.aad-authentication-url"),
tenantId,
clientId,
clientSecret);
List<String> result = keyVaultClient.getAliases();
List<String> result = getKeyVaultClient().getAliases();
assertNotNull(result);
}

@Test
@Disabled
public void testGetCertificate() {
String tenantId = System.getProperty("azure.keyvault.tenant-id");
String clientId = System.getProperty("azure.keyvault.client-id");
String clientSecret = System.getProperty("azure.keyvault.client-secret");
String keyVaultUri = System.getProperty("azure.keyvault.uri");
KeyVaultClient keyVaultClient = new KeyVaultClient(
keyVaultUri, System.getProperty("azure.keyvault.aad-authentication-url"),
tenantId,
clientId,
clientSecret);
Certificate certificate = keyVaultClient.getCertificate("myalias");
Certificate certificate = getKeyVaultClient().getCertificate("myalias");
assertNotNull(certificate);
}

@Test
@Disabled
public void testGetKey() {
assertNull(getKeyVaultClient().getKey("myalias", null));
}

private KeyVaultClient getKeyVaultClient() {
String keyVaultUri = System.getProperty("azure.keyvault.uri");
String tenantId = System.getProperty("azure.keyvault.tenant-id");
String clientId = System.getProperty("azure.keyvault.client-id");
String clientSecret = System.getProperty("azure.keyvault.client-secret");
String keyVaultUri = System.getProperty("azure.keyvault.uri");
KeyVaultClient keyVaultClient = new KeyVaultClient(
keyVaultUri, System.getProperty("azure.keyvault.aad-authentication-url"), tenantId, clientId, clientSecret);
assertNull(keyVaultClient.getKey("myalias", null));
return new KeyVaultClient(keyVaultUri, tenantId, clientId, clientSecret);
}
}
42 changes: 42 additions & 0 deletions ...java/com/azure/spring/security/keyvault/certificates/starter/AzureCertPathProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
package com.azure.spring.security.keyvault.certificates.starter;

import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.EnableConfigurationProperties;

/**
* This is used to generate spring-configuration-metadata.json
*
* @see <a href="https://docs.spring.io/spring-boot/docs/current/reference/html/appendix-configuration-metadata.html">Metadata</a>
*/
@EnableConfigurationProperties({ AzureCertPathProperties.class })
@ConfigurationProperties("azure.cert-path")
Copy link
Member

@stliu stliu Jun 24, 2021

Choose a reason for hiding this comment

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

we should discuss this property name, azure.cert-path.custom seems not a clear name to me

Copy link
Author

@chenrujun chenrujun Jun 25, 2021

Choose a reason for hiding this comment

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

Current property name is the conclusion discussed with @gavinfish
Refs: #21611 (comment)

public class AzureCertPathProperties {

/**
* The path to put custom certificates
*/
private String custom;

/**
* The path to put well-known certificates
*/
private String wellKnown;

public String getCustom() {
return custom;
}

public String getWellKnown() {
return wellKnown;
}

public void setCustom(String custom) {
this.custom = custom;
}

public void setWellKnown(String wellKnown) {
this.wellKnown = wellKnown;
}
}
4 changes: 2 additions & 2 deletions ...tificates/starter/KeyVaultProperties.java → ...ates/starter/AzureKeyVaultProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
*
* @see <a href="https://docs.spring.io/spring-boot/docs/current/reference/html/appendix-configuration-metadata.html">Metadata</a>
*/
@EnableConfigurationProperties({ KeyVaultProperties.class })
@EnableConfigurationProperties({ AzureKeyVaultProperties.class })
@ConfigurationProperties("azure.keyvault")
public class KeyVaultProperties {
public class AzureKeyVaultProperties {
/**
* The URI to the Azure Key Vault used
*/
Expand Down
5 changes: 0 additions & 5 deletions .../security/keyvault/certificates/starter/KeyVaultCertificatesEnvironmentPostProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ public class KeyVaultCertificatesEnvironmentPostProcessor implements Environment
@Override
public void postProcessEnvironment(ConfigurableEnvironment environment, SpringApplication application) {

if (environment.getProperty("azure.keyvault.uri") == null) {
Copy link
Member

@stliu stliu Jun 24, 2021

Choose a reason for hiding this comment

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

is there any side effect with this remove?

Copy link
Author

@chenrujun chenrujun Jun 25, 2021

Choose a reason for hiding this comment

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

No side effect, I think.

return;
}

putEnvironmentPropertyToSystemProperty(environment, "azure.keyvault.aad-authentication-url");
Copy link
Member

@stliu stliu Jun 24, 2021

Choose a reason for hiding this comment

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

I'd suggest to add a warning message in log to warn users still having this property configed

Copy link
Author

@chenrujun chenrujun Jun 25, 2021

Choose a reason for hiding this comment

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

Maybe it's not necessary, because:

  1. If customer use azure-spring-boot-starter-keyvault-certificates, azure.keyvault.aad-authentication-url never exist in KeyVaultProperties.java, so customer never know this property exists.

Refs: https://github.com/Azure/azure-sdk-for-java/commits/7989020420f8354b9f6c1597d68c06acddab63d8/sdk/spring/azure-spring-boot-starter-keyvault-certificates/src/main/java/com/azure/spring/security/keyvault/certificates/starter/KeyVaultProperties.java

  1. If customer use azure-security-keyvault-jca, this already written in changelog:

Refs: https://github.com/Azure/azure-sdk-for-java/blob/7989020420f8354b9f6c1597d68c06acddab63d8/sdk/keyvault/azure-security-keyvault-jca/CHANGELOG.md#breaking-changes

putEnvironmentPropertyToSystemProperty(environment, "azure.keyvault.uri");
putEnvironmentPropertyToSystemProperty(environment, "azure.keyvault.tenant-id");
putEnvironmentPropertyToSystemProperty(environment, "azure.keyvault.client-id");
Expand Down