[KeyVault] - Add support for Secure Key Release #16150

maorleger · 2021-07-01T22:43:08Z

What

Add exportable attribute and releasePolicy to KV Keys
Add releaseKey method and all of its models
Add AZURE_KEYVAULT_ATTESTATION_URI as a static resource

Why

The MHSM service is adding support for SKR in 7.3, as such we need to implement it in our client libraries.

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md

sdk/keyvault/keyvault-keys/test/public/keyClient.hsm.spec.ts

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md

check-enforcer · 2021-07-01T23:30:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

maorleger · 2021-07-02T14:40:38Z

sdk/keyvault/keyvault-keys/test/public/keyClient.hsm.spec.ts

@@ -71,5 +72,82 @@ onVersions({ minVer: "7.2" }).describe(
        );
      });
    });
+
+    onVersions({ minVer: "7.3-preview" }).describe("releaseKey", () => {


To explain this test setup here:

with the service team's help I setup a mock service on Azure App Service that pretends to be an attestation service and returns mock data.

The managed HSM requires an attestation service that supports OIDC OpenID Connect Discovery. The minimal requirements for us are:

It should be reachable by the MHSM

It should respond to https://server.com/.well-known/openid-configuration with the jwks_uri

the jwks_uri value should be reachable by the MHSM

The attestation we provided must have the right information

The last bit is where it gets interesting - we need to use the same keys to sign the JWT as the ones returned by jwks_uri.

So this should be something that is stood up by live test resources if we want to test this live. You can see what my mock service returns here:

https://skrattestation.azurewebsites.net/generate-test-token -> used by us to get an attestation signed JWT
https://skrattestation.azurewebsites.net/keys -> used by the MHSM to get the key used for signing the token
https://skrattestation.azurewebsites.net/.well-known/openid-configuration -> tells the MHSM where to go to get key

I'll investigate ways to make this part of our test resource deployment, but it's also possible to use this today to record tests for any language

heaths · 2021-07-06T18:11:58Z

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md

@@ -228,6 +232,7 @@ export class KeyClient {
    listPropertiesOfKeys(options?: ListPropertiesOfKeysOptions): PagedAsyncIterableIterator<KeyProperties>;
    listPropertiesOfKeyVersions(name: string, options?: ListPropertiesOfKeyVersionsOptions): PagedAsyncIterableIterator<KeyProperties>;
    purgeDeletedKey(name: string, options?: PurgeDeletedKeyOptions): Promise<void>;
+    releaseKey(name: string, version: string, target: string, options?: ReleaseKeyOptions): Promise<ReleaseKeyResult>;


IIRC, the target is the environment they want to target. We might use environment instead, but talk with Hervey.

Or maybe even targetEnvironment. Would be nice to align with the swagger, even if it means changing that (it's in preview).

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md

sdk/keyvault/test-resources.json

sdk/keyvault/keyvault-keys/src/index.ts

heaths · 2021-07-19T22:59:29Z

sdk/keyvault/keyvault-keys/src/keysModels.ts

+  nonce?: string;
+
+  /** The {@link KeyExportEncryptionAlgorithm} to for protecting the exported key material. */
+  algorithm?: KeyExportEncryptionAlgorithm;


I like the name, but should we have the service team change the x-ms-client-name? https://github.com/Azure/azure-rest-api-specs/blob/e7682aa897902920f3a95b2f358b6f7729d18666/specification/keyvault/data-plane/Microsoft.KeyVault/preview/7.3-preview/keys.json#L1856

I think what is in swagger now - which changing won't change the wire format - is confusing with our existing EncryptionAlgorithm we have in cryptography.

/cc @lusitanian

/cc @herveyw-msft

sdk/keyvault/keyvault-keys/src/keysModels.ts

sadasant · 2021-07-26T20:22:02Z

sdk/keyvault/keyvault-keys/src/keysModels.ts

+  RsaAesKeyWrap384 = "RSA_AES_KEY_WRAP_384"
+}
+
+/* eslint-disable tsdoc/syntax */


Is this necessary? Isn't there a way to make it look nice while this stays enabled?

I tried various ways to make our tsdoc linter happy - it's not happy with the escaping of the newlines on lines 583-585 but they are necessary to display newlines correctly in VSCode. This is also how they are generated in the generated code so I feel it's reasonable to have them here. I was not able to get newlines rendered correctly without this but I am happy to make an issue to investigate that further

I'm convinced to leave it as you have it here :)

sadasant

Looks good to me! Please get approval from the Key Vault crew as well.

ghost added the KeyVault label Jul 1, 2021

maorleger force-pushed the keyvault-key-release branch from 86510f0 to 5594d60 Compare July 1, 2021 22:49

maorleger commented Jul 1, 2021

View reviewed changes

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md Show resolved Hide resolved

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md Show resolved Hide resolved

sdk/keyvault/keyvault-keys/test/public/keyClient.hsm.spec.ts Outdated Show resolved Hide resolved

maorleger commented Jul 1, 2021

View reviewed changes

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md Show resolved Hide resolved

maorleger commented Jul 2, 2021

View reviewed changes

maorleger force-pushed the keyvault-key-release branch from 9e4a451 to 6c51ff8 Compare July 2, 2021 15:03

heaths reviewed Jul 6, 2021

View reviewed changes

maorleger added 16 commits July 13, 2021 08:52

wip: live working implementation

f2348ad

update api

c139550

move releasepolicy to properties, start import key work

299191f

import key, add known algos, include algo in result

ca11f14

cleanup

35ec817

fetch jwt dynamically

f387e4a

add doc comments

dea185d

remove todos now that they are in PR

960d0b0

set soft delete retention

f11ced3

re-record

a0baf84

lint fixes

d16658d

suppress incorrect lint warning

d2b9c7b

add new properties to deletedKey test

b2a2daf

update attestation url

6ae1113

rebase main and add changelog

cedd9c3

add comment about issue for shared attestation resource

6ff928c

maorleger force-pushed the keyvault-key-release branch from 528b17e to 6ff928c Compare July 13, 2021 16:35

maorleger marked this pull request as ready for review July 13, 2021 16:35

maorleger requested review from bterlson, chradek, sadasant and xirzec as code owners July 13, 2021 16:35

maorleger added 6 commits July 15, 2021 17:45

add error cases test

72228f7

recordings

bb82e91

move attestation url to env vars

208762f

update recordings, use env from recorder, and fix test values

33c6df7

fix tests

497adfc

lint

01d932a

heaths reviewed Jul 19, 2021

View reviewed changes

sdk/keyvault/test-resources.json Outdated Show resolved Hide resolved

heaths reviewed Jul 19, 2021

View reviewed changes

sdk/keyvault/test-resources.json Outdated Show resolved Hide resolved

heaths reviewed Jul 19, 2021

View reviewed changes

sdk/keyvault/keyvault-keys/src/index.ts Outdated Show resolved Hide resolved

heaths reviewed Jul 19, 2021

View reviewed changes

sdk/keyvault/keyvault-keys/src/keysModels.ts Outdated Show resolved Hide resolved

maorleger added 5 commits July 20, 2021 08:35

feedback

ab0cca6

update doc-comment

9da60ba

update keysModel

8ee57e3

only return value

7a8743d

update recordings and env var to match dotnet

dd5ad08

sadasant reviewed Jul 26, 2021

View reviewed changes

sadasant approved these changes Jul 26, 2021

View reviewed changes

maorleger merged commit 6f36795 into Azure:main Jul 27, 2021

maorleger deleted the keyvault-key-release branch July 27, 2021 17:09

maorleger linked an issue Jul 27, 2021 that may be closed by this pull request

Add support for Secure Key Release from Key Vault #16086

Closed

maorleger mentioned this pull request Jul 27, 2021

Add support for Secure Key Release from Key Vault #16086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KeyVault] - Add support for Secure Key Release #16150

[KeyVault] - Add support for Secure Key Release #16150

maorleger commented Jul 1, 2021 •

edited

Loading

check-enforcer bot commented Jul 1, 2021

maorleger Jul 2, 2021 •

edited

Loading

heaths Jul 6, 2021

heaths Jul 15, 2021

heaths Jul 19, 2021

heaths Jul 19, 2021

sadasant Jul 26, 2021

maorleger Jul 26, 2021 •

edited

Loading

sadasant Jul 26, 2021

sadasant left a comment

[KeyVault] - Add support for Secure Key Release #16150

[KeyVault] - Add support for Secure Key Release #16150

Conversation

maorleger commented Jul 1, 2021 • edited Loading

What

Why

check-enforcer bot commented Jul 1, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

maorleger Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

heaths Jul 6, 2021

Choose a reason for hiding this comment

heaths Jul 15, 2021

Choose a reason for hiding this comment

heaths Jul 19, 2021

Choose a reason for hiding this comment

heaths Jul 19, 2021

Choose a reason for hiding this comment

sadasant Jul 26, 2021

Choose a reason for hiding this comment

maorleger Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

sadasant Jul 26, 2021

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

maorleger commented Jul 1, 2021 •

edited

Loading

maorleger Jul 2, 2021 •

edited

Loading

maorleger Jul 26, 2021 •

edited

Loading