[KeyVault] - Add support for Key Rotation #16593

maorleger · 2021-07-27T23:59:18Z

What

Add getKeyRotationPolicy, rotateKey, and updateKeyRotationPolicy to KeyClient
Model Key Rotation policy similarly to Certificate Policy
Add rotate permission to the arm template when deploying keyvault

Why

As part of 7.3 the KV crew is adding support for Key Rotation which we will need to model in our SDK. It is currently in preview
but is planned for 7.3. As such, we have a few methods to model here:

Updating a key's rotation policy
Fetching a key's rotation policy
Rotating a key on-demand which will create a new version of the key

In addition, since it requires the /keys/rotate permission, we need to add that to the list of permissions in our test resources.

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

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

maorleger · 2021-07-29T18:12:35Z

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

+  ): Promise<KeyRotationPolicy | undefined> {
+    return withTrace("getRotationPolicy", options, async () => {
+      const policy = await this.client.getKeyRotationPolicy(this.vaultUrl, name);
+      if (policy.id) {


I did ask the service team about what's expected here. Right now, this will return

{ id: undefined, lifetimeActions: [], attributes: null }

And I wonder if it should 404 or 204 with no content. Depending on what the service team responds with we can decide what the client library should do. But for now, I made the decision to return undefined in that case.

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

maorleger · 2021-08-03T20:03:26Z

sdk/keyvault/keyvault-keys/swagger/README.md

@@ -35,3 +35,16 @@ directive:
    transform: >
      $["x-ms-client-name"] = "authenticationTag"
 ```
+
+### Update swagger enum values for LifetimeActionsType to reflect what the service actually returns


Calling out this change - I wanted to unblock myself but we do have a thread going to make the enum values lowercase if possible, or at least update the x-ms-enum swagger to reflect what the service actually returns

check-enforcer · 2021-08-03T20:13: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

heaths

We should more closely match what we do for certificates for consistency. It may be good to leave the "KeyRotationPolicy" prefix to disambiguate, but the design is currently very different from certificates and could be made a little closer. After all, the service team was effectively porting the feature from certs to keys.

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

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

heaths

Better, thank you.

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

heaths · 2021-08-05T22:50:22Z

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

+// @public
+export interface KeyRotationLifetimeAction {
+    action: KeyRotationPolicyAction;
+    timeAfterCreate?: string;


I see these names match the swagger, which is probably fine. There's not quite the analog to our certificate's LifetimeActions that use a date; however, I would suggest a swagger change that these use the ISO 8601 duration format, which is very different than ISO 8601 date formats used elsewhere in this and many other SDKs.

That said, I think for idiomatic reasons we should consider various languages' / frameworks' TimeSpan (.NET) structs; though, I believe @JeffreyRichter had some problem with this as far as Go goes. At the very least, we'll want to document our APIs that this is the ISO 8601 duration format e.g., "P90D" for 90 days, and probably even link to it.

Swagger should not have any 8601 string durations in it. We want number in units (seconds, minutes, hours). How this number is represented in various programming languages is fine; .NET can use a Timespan initialized from the number in the proper units. FYI: Go has a time.Duration type too but it is just a thin wrapper over an integer.

To Jeffrey's point (sorry - I was misremembering his concern was about the swagger, not the SDK), certificates already has a precedent of requiring a number of days: days_before_expiry (though, should be camelCase - this was before my time).

Thank you for the feedback! @jlichwa - certificates swagger already uses numbers to represent triggers for certificate policy - is this something we can model similarly for key rotation policies?

Right now the keys swagger defines timeAfterCreate and timeBeforeExpiry as strings representing durations - is that something that can be changed to number in units like we do in certificates?

Thanks in advance

/cc @herveyw-msft

We purposely deviated from Certificates which were defined long time ago and we did want to repeat same mistakes. Keys model is more flexible with ability to easily define time span in any units (initially we just support years, months, days but later we may extend it to hours, minutes....
Engineers found that ISO 8601 is a standard for TimeSpan - we same in Service BUS https://docs.microsoft.com/en-us/rest/api/servicebus/preview/queues/get#sbqueue

It does look like we have some precedence for using durations. At least in JS we have autoDeleteOnIdle in service bus and monitor query's timespan parameter; however, we should address @JeffreyRichter's guidance. So I propose:

Merging this PR once we have support for rotate permission in arm template (so that we can run live tests against the service) edit: it might already be supported, I will check today

Setting aside some time next week (week of 8/23) when folks are back in the office to discuss the guidelines here - I'd love to get to resolution specifically for Key Rotation and I'll make issues based on the outcome. I can schedule something for next week

cc @AlexGhiondea

sadasant

Looks clean from the code perspective! Make sure to get the Key Vault crew aligned on the public API.

@returns

- sort package.json new import - rename the options interfaces to match the method name - remove the @returns from js-doc. I like @returns, but its inconsistent with our other methods - remove the "erros when fetching deleted key" test - deleting keys is too long an operation, making this an expensive test of service side. We already have a errors when fetching non-existent key.

…add changelog

maorleger · 2021-08-18T15:27:42Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2021-08-18T15:27:54Z

Azure Pipelines successfully started running 1 pipeline(s).

## What - Add `getKeyRotationPolicy`, `rotateKey`, and `updateKeyRotationPolicy` to KeyClient - Model Key Rotation policy similarly to Certificate Policy - Add `rotate` permission to the arm template when deploying keyvault ## Why As part of 7.3 the KV crew is adding support for Key Rotation which we will need to model in our SDK. It is currently in preview but is planned for 7.3. As such, we have a few methods to model here: - Updating a key's rotation policy - Fetching a key's rotation policy - Rotating a key on-demand which will create a new version of the key In addition, since it requires the `/keys/rotate` permission, we need to add that to the list of permissions in our test resources. ## Callout As of 8/18/21 the model for rotation policy contains ISO8601 durations which we are modeling as string similar to our other libraries. That might change at the swagger level or it might change at the SDK level in the near future (decision TBD)

This reverts commit 44a435c.

ghost added the KeyVault label Jul 27, 2021

maorleger commented Jul 28, 2021

View reviewed changes

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

maorleger requested review from mccoyp, vcolin7, heaths and sadasant July 28, 2021 00:29

maorleger commented Jul 29, 2021

View reviewed changes

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

maorleger commented Jul 29, 2021

View reviewed changes

maorleger commented Aug 3, 2021

View reviewed changes

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

maorleger commented Aug 3, 2021

View reviewed changes

heaths requested changes Aug 3, 2021

View reviewed changes

maorleger mentioned this pull request Aug 4, 2021

Consider changing prettier config to set trailing commas #16757

Closed

heaths approved these changes Aug 4, 2021

View reviewed changes

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

maorleger force-pushed the keyvault-rotate branch from f2bb3db to 8e33c13 Compare August 4, 2021 17:11

maorleger commented Aug 4, 2021

View reviewed changes

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

maorleger marked this pull request as ready for review August 4, 2021 20:57

maorleger requested review from bterlson, chradek, deyaaeldeen, mikeharder, praveenkuttappan, witemple-msft and xirzec as code owners August 4, 2021 20:57

maorleger changed the title ~~[KeyVault] - Initial draft of Key Rotation~~ [KeyVault] - Add support for Key Rotation Aug 4, 2021

heaths approved these changes Aug 5, 2021

View reviewed changes

sadasant approved these changes Aug 16, 2021

View reviewed changes

maorleger added 3 commits August 18, 2021 08:20

Add key rotation to keyvault keys

733eada

squash, rebase, rush install, update api

b9c5d91

maorleger added 10 commits August 18, 2021 08:21

update api

666a41a

fixup! update api

14dea56

setKeyRotationPolicy -> updateKeyRotationPolicy

25e344a

inline convertLifetimeActions since it's only used in one place, and …

91b82d6

…add changelog

mark action as required

0b427e7

remove only

962817d

add rotate to arm template key permission

0049a36

update the recordings

59f0266

update changelog after rebase

a267daf

post rebase rush update

637ffed

maorleger force-pushed the keyvault-rotate branch from 4f51cdf to 637ffed Compare August 18, 2021 15:24

maorleger merged commit 44a435c into Azure:main Aug 18, 2021

maorleger deleted the keyvault-rotate branch August 18, 2021 16:43

maorleger mentioned this pull request Aug 18, 2021

[Key Vault] Add support for Key Rotation #16420

Closed

maorleger added a commit to maorleger/azure-sdk-for-js that referenced this pull request Aug 26, 2021

Revert "[KeyVault] - Add support for Key Rotation (Azure#16593)"

8da717a

This reverts commit 44a435c.

This was referenced Sep 14, 2021

[Key Vault] Add support for Key Rotation Azure/azure-sdk-for-python#19840

Closed

[Key Vault] Add support for Key Rotation Azure/azure-sdk-for-java#22993

Closed

[Key Vault] Add support for Key Rotation Azure/azure-sdk-for-net#22685

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KeyVault] - Add support for Key Rotation #16593

[KeyVault] - Add support for Key Rotation #16593

maorleger commented Jul 27, 2021 •

edited

Loading

maorleger Jul 29, 2021 •

edited

Loading

maorleger Aug 3, 2021

check-enforcer bot commented Aug 3, 2021

heaths left a comment

heaths left a comment

heaths Aug 5, 2021

JeffreyRichter Aug 5, 2021

heaths Aug 5, 2021

maorleger Aug 5, 2021

heaths Aug 5, 2021

jlichwa Aug 16, 2021

maorleger Aug 18, 2021 •

edited

Loading

sadasant left a comment

maorleger commented Aug 18, 2021

azure-pipelines bot commented Aug 18, 2021

[KeyVault] - Add support for Key Rotation #16593

[KeyVault] - Add support for Key Rotation #16593

Conversation

maorleger commented Jul 27, 2021 • edited Loading

What

Why

maorleger Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

maorleger Aug 3, 2021

Choose a reason for hiding this comment

check-enforcer bot commented Aug 3, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

heaths left a comment

Choose a reason for hiding this comment

heaths left a comment

Choose a reason for hiding this comment

heaths Aug 5, 2021

Choose a reason for hiding this comment

JeffreyRichter Aug 5, 2021

Choose a reason for hiding this comment

heaths Aug 5, 2021

Choose a reason for hiding this comment

maorleger Aug 5, 2021

Choose a reason for hiding this comment

heaths Aug 5, 2021

Choose a reason for hiding this comment

jlichwa Aug 16, 2021

Choose a reason for hiding this comment

maorleger Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

maorleger commented Aug 18, 2021

azure-pipelines bot commented Aug 18, 2021

maorleger commented Jul 27, 2021 •

edited

Loading

maorleger Jul 29, 2021 •

edited

Loading

maorleger Aug 18, 2021 •

edited

Loading