End to end TLS SSL

[mnriem]

No description provided.

[JonathanGiles]

I've only had a very brief few moments to look through this but I wanted to share the first round of feedback now rather than wait until a complete review could be completed.

[JonathanGiles]

It appears as if this dependency is being used in a shaded fashion, alongside other libraries. I would be happier if we didn't need these dependencies at all though. From a quick look through the code it doesn't appear to be baked in deeply, mainly showing up it seems in the LegacyRestClient.

[mnriem]

The dependencies of the JCA provider are shaded to make sure the provider is self-contained and can be used in any Java application

[JonathanGiles]

Yep, understood. My comment was mainly focused on whether the httpclient is necessary in the first place? It doesn't appear to be from a cursory glance.

[JonathanGiles]

Not a huge fan of allowing this dependency - would love to see this removed if at all possible.

[JonathanGiles]

Don't skip this check.

[JonathanGiles]

Don't skip this check.

[JonathanGiles]

Don't skip this check.

[schaabs]

Is this intended to work against clouds other than Azure Public Cloud? If so this would need to be configurable.

[schaabs]

Certificates downloaded from Key Vault through the get secret API might be in PKCS12 or PEM format. These can be differentiated by the by the contentType of the secret bundle.

[vcolin7]

Many of my comments and suggestions can be pushed to a later release, but there are a few that I think should be at least discussed before we go ahead with merging.

Additionally, there are still a couple missing changes to make to files involved with CI to properly set everything up for the release. Please refer this PR and this PR. The first one shows how to make an exclusion for some README issues and how to include a new library on the code coverage runs. The second one shows how to add checkstyle suppressions (when they are justified) and how to include a new package on the build pipeline (you have already performed this step).

Finally, I think we should consider refactoring some classes into a models package and possibly a couple others in the future.

[vcolin7]

nit; Small details.

Suggested change

	KeyVaultJcaProvider provider = new KeyVaultJcaProvider();
	Security.addProvider(provider);
	KeyStore ks = KeyStore.getInstance("AzureKeyVault");
	KeyVaultLoadStoreParameter parameter = new KeyVaultLoadStoreParameter(
	System.getProperty("azure.keyvault.uri"),
	System.getProperty("azure.tenant.id"),
	System.getProperty("azure.client.id"),
	System.getProperty("azure.client.secret"));
	ks.load(parameter);
	SSLContext sslContext = SSLContexts
	.custom()
	.loadTrustMaterial(ks, new TrustSelfSignedStrategy())
	.build();
	SSLConnectionSocketFactory sslSocketFactory = SSLConnectionSocketFactoryBuilder
	.create()
	.setSslContext(sslContext)
	.setHostnameVerifier((hostname, session) -> {
	return true;
	})
	.build();
	PoolingHttpClientConnectionManager cm = PoolingHttpClientConnectionManagerBuilder
	.create()
	.setSSLSocketFactory(sslSocketFactory)
	.build();
	String result = null;
	try ( CloseableHttpClient client = HttpClients.custom().setConnectionManager(cm).build()) {
	HttpGet httpGet = new HttpGet("https://localhost:8766");
	HttpClientResponseHandler<String> responseHandler = (ClassicHttpResponse response) -> {
	int status = response.getCode();
	String result1 = "Not success";
	if (status == 204) {
	result1 = "Success";
	}
	return result1;
	};
	result = client.execute(httpGet, responseHandler);
	} catch (IOException ioe) {
	ioe.printStackTrace();
	}
	KeyVaultJcaProvider provider = new KeyVaultJcaProvider();
	Security.addProvider(provider);
	KeyStore ks = KeyStore.getInstance("AzureKeyVault");
	KeyVaultLoadStoreParameter parameter = new KeyVaultLoadStoreParameter(
	System.getProperty("azure.keyvault.uri"),
	System.getProperty("azure.tenant.id"),
	System.getProperty("azure.client.id"),
	System.getProperty("azure.client.secret"));
	ks.load(parameter);
	SSLContext sslContext = SSLContexts
	.custom()
	.loadTrustMaterial(ks, new TrustSelfSignedStrategy())
	.build();
	SSLConnectionSocketFactory sslSocketFactory = SSLConnectionSocketFactoryBuilder
	.create()
	.setSslContext(sslContext)
	.setHostnameVerifier((hostname, session) -> {
	return true;
	})
	.build();
	PoolingHttpClientConnectionManager cm = PoolingHttpClientConnectionManagerBuilder
	.create()
	.setSSLSocketFactory(sslSocketFactory)
	.build();
	String result = null;
	try (CloseableHttpClient client = HttpClients.custom().setConnectionManager(cm).build()) {
	HttpGet httpGet = new HttpGet("https://localhost:8766");
	HttpClientResponseHandler<String> responseHandler = (ClassicHttpResponse response) -> {
	int status = response.getCode();
	String result1 = "Not success";
	if (status == 204) {
	result1 = "Success";
	}
	return result1;
	};
	result = client.execute(httpGet, responseHandler);
	} catch (IOException ioe) {
	ioe.printStackTrace();
	}

[vcolin7]

I noticed the Key Vault Secrets Spring Boot Starter does not make use of the Revapi plugin. Should we consider adding it there?

[vcolin7]

I noticed that the all classes that are part of the Key Vault Secrets Spring Boot Starter live in the azure-spring-boot folder under the package com.azure.spring.keyvault, should this be there as well?

[mnriem]

@chenrujun @saragluna Can you answer this question?

[vcolin7]

Maybe we can use dummy-value or something else instead (I don't have a better suggestion at the moment). As it stands right now, it is a little ambiguous if what does not matter is the value or the property at all.

Suggested change

	server.ssl.key-store-password=doesnotmatter
	server.ssl.key-password=doesnotmatter
	server.ssl.trust-store-password=doesnotmatter
	server.ssl.key-store-password=dummy-value
	server.ssl.key-password=dummy-value
	server.ssl.trust-store-password=dummy-value

[vcolin7]

Approving based on conversations with @JonathanGiles and @mnriem. Suggested changes and improvements will target future releases.

Suggested changes and improvements will target future releases.

[g2vinay]

This will work on IMDS supported Azure Platforms only.
For e.g This won't work in App Service.

[yiliuTo]

Hi, the case of App Service has been handled separately. And according to this doc, requesting access tokens via Azure resources VM Extension Endpoint is also not encouraged. Thus are there any other cases that are not covered? Thanks

[g2vinay]

There's a newer 2019 API version available, which uses IDENTITY_ENDPOINT and IDENTITY_HEADER env vars.

[yiliuTo]

Hi @g2vinay , I found the following information that when using Linux Consumption, 2017 API version is required. Thus is it a better choice that we remain use of 2017 API version to support more cases?