Add keys as a child proxy resource under vaults (AKV's ARM contract). #10643
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[Staging] Swagger Validation Report
️✔️ |
|
Azure Pipelines successfully started running 1 pipeline(s). |
adarce
force-pushed
the
dev-keyvault-Microsoft.KeyVault-2019-09-01
branch
from
391a131 to
0e762ba
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
adarce
force-pushed
the
dev-keyvault-Microsoft.KeyVault-2019-09-01
branch
from
0e762ba to
f8b1dbb
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
adarce
force-pushed
the
dev-keyvault-Microsoft.KeyVault-2019-09-01
branch
from
f8b1dbb to
339a9a9
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
adarce
force-pushed
the
dev-keyvault-Microsoft.KeyVault-2019-09-01
branch
from
339a9a9 to
ad656ce
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
adarce
force-pushed
the
dev-keyvault-Microsoft.KeyVault-2019-09-01
branch
from
ad656ce to
9603e8c
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@heaths , @schaabs , @fengzhou-msft : I see that the required "automation - sdk (SDK Python)" check fails during the Docker container initialization step. What team should this issue be escalated to? |
|
@NullMDR can you help check the automation - sdk failures? |
|
/azp run automation - sdk |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@fengzhou-msft It's because the base branch's CI is not updated. |
|
Thanks for investigating. @fengzhou-msft : Is this issue something that I can fix? If so, how do I do this? |
|
Why are you modifying a GA'd swagger? Can you describe what this PR is for? Is something missing, or are you adding features, which should go into a new service version? |
|
@heaths : I am modifying the most recent stable API version for KeyVault's control plane. This PR is additive (adding new resource types), so as per this ARM documentation for API versions, these non-breaking API changes do not require a new API version. Overall, these new APIs will allow customers to create keys via ARM templates. KeyVault's control plane will be proxying such requests to KeyVault's data plane. By exposing data plane APIs via ARM templates, we enable seamless UX workflows for partner teams such as Azure Disk Encryption. |
|
@RandalliLama can you get someone from the service team to review as well? |
heaths
approved these changes
Sep 2, 2020
There was a problem hiding this comment.
Perhaps it's unnecessary, but we might consider sharing JWK and other related definitions with data-plane. Or perhaps the idea is not to return everything in a response for ARM functions? The defs are already out of sync with what shipped in 7.1 GA and in 7.2 preview.
| @@ -38,6 +38,7 @@ These settings apply only when `--tag=package-2019-09` is specified on the comma | |||
| input-file: | |||
| - Microsoft.KeyVault/stable/2019-09-01/keyvault.json | |||
| - Microsoft.KeyVault/stable/2019-09-01/providers.json | |||
| - Microsoft.KeyVault/stable/2019-09-01/keys.json | |||
There was a problem hiding this comment.
Are you sure that all generators are pointing to this readme.md for configuration? At least for data plane, we point at the JSON files directly. I.e. this should be merged into keyvault.json to work with existing tooling and configuration.
There was a problem hiding this comment.
Thanks for the feedback - this is my first time authoring a swagger spec. I'll look into reverting this and updating keyvault.json instead. (I wanted to avoid updating any existing files and only add new files, so as to avoid any breaking changes.)
| "tags": [ | ||
| "Keys" | ||
| ], | ||
| "operationId": "Keys_ListVersions", |
There was a problem hiding this comment.
Question: what's the use case for this one? ListKeys I can see being used in an ARM function to query for a specific key (must like getting a storage account key), but the versions themselves are, for all intent to the customer, opaque versions. Referring to a specific key (Keys_GetVersion) makes sense to refer to a fixed version, but why support listing them in ARM?
There was a problem hiding this comment.
Good question. The ListKeys/ListKeyVersions APIs are not intended to be used in ARM templates (they are paginated by continuation tokens, which I don't think ARM templates support). Instead, they are intended to be used by UX in the Azure Portal.
The scenario is that a partner team (e.g.: Azure Disk Encryption) will allow customers to provide customer-managed keys by opening a picker control owned by the Key Vault team. This picker will have a dropdown or grid that will list all keys in a given vault, as well as list all key versions for a given key. This picker will call these APIs via ARM instead of directly accessing the data plane because the two code paths use different permission models:
- Direct data plane APIs: Permissions to list keys or key versions are configured either via legacy access policies, or via RBAC DataActions.
- Data plane APIs proxied via control plane: Permissions to list keys or key versions are configured via RBAC Actions, and can bypass firewall.
The reasoning behind using a separate permission model for these new APIs instead of overloading the existing RBAC DataActions:
- Fundamentally different APIs: Although both accomplish the same thing, CreateKey Action is a management operation (typically, a one-time only setup). The CreateKey DataAction is a data operation (used frequently).
- Restricted elevation of privilege: We want users with permissions to create vaults via ARM to implicitly also have permission to indirectly create keys via ARM. This happens because owners/contributors have permissions that match
Microsoft.KeyVault/vaults/*RBAC Actions, which includes the CreateKey Action. However, we don't want these users to automatically be able to create keys directly via the data plane.
| } | ||
| }, | ||
| "definitions": { | ||
| "CloudError": { |
There was a problem hiding this comment.
There should already be an error type from keyvault.json. IF you don't merge this into keyvault.json (recommended), I recommend breaking out the error type into another file and use an external ref (see what I did in the data-plane for KV 7.1) to avoid unnecessarily functional duplication in generated code.
There was a problem hiding this comment.
Same for other shared definitions below.
| }, | ||
| "x-ms-external": true | ||
| }, | ||
| "Attributes": { |
There was a problem hiding this comment.
Should these be imported from the data plane to make sure they stay in sync, or is that necessarily important here as long as the useful data from keys can be queried in ARM functions?
For example, KeyProperties in 7.1 also has recoveryDays and a couple more values for recoveryLevel, IIRC.
| "EC", | ||
| "EC-HSM", | ||
| "RSA", | ||
| "RSA-HSM" |
There was a problem hiding this comment.
Key Vault doesn't currently support "oct".
specification/keyvault/resource-manager/Microsoft.KeyVault/stable/2019-09-01/keys.json
Show resolved
Hide resolved
|
@adarce I could help to update your base branch. Please let me know if you want. |
|
@NullMDR : Yes, please assist. |
I'm willing to refactor things to avoid unnecessary duplication, but I want to avoid making any breaking changes to existing APIs. Also yes: for the ARM APIs, the keys returned from ARM will contain a subset of the properties exposed by keys directly returned from the data plane (e.g.: the full JWK won't be returned in the ARM APIs, so RSA keys won't have their modulus/exponent exposed for example). |
majastrz
reviewed
Sep 4, 2020
| }, | ||
| "KeyCreateOrUpdateParameters": { | ||
| "properties": { | ||
| "tags": { |
There was a problem hiding this comment.
tags [](start = 9, length = 4)
Are keys going to be tracked or proxy resources in ARM?
There was a problem hiding this comment.
The reason I ask is because tags shouldn't really be used on proxy resources.
In reply to: 483360952 [](ancestors = 483360952)
There was a problem hiding this comment.
Yes, keys are going to be proxy resources in ARM, in the same way that secrets are child proxy resources under vaults in ARM. See here for comparison: https://github.com/Azure/azure-rest-api-specs/blob/9603e8c3886893f114df4a2dc844d175c3f2b1c6/specification/keyvault/resource-manager/Microsoft.KeyVault/stable/2019-09-01/secrets.json#L361-L369
Note that Azure Key Vault's data plane supports its own tags for all objects that can be stored in a vault (keys, secrets, certificates, and managed storage account keys).
majastrz
reviewed
Sep 4, 2020
| } | ||
| }, | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.KeyVault/vaults/{vaultName}/keys/{keyName}/versions/{keyVersion}": { | ||
| "get": { |
There was a problem hiding this comment.
get [](start = 7, length = 3)
I don't see a PUT. Does this mean that a PUT on /keys/{name} will automatically create a /keys/{name}/version/{keyVersion} resource in the backend?
|
Hi, @adarce Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected] |
adarce
force-pushed
the
dev-keyvault-Microsoft.KeyVault-2019-09-01
branch
from
538aa14 to
b11b1a9
Compare
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure CLI Extension Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-js - Release
|
azure-sdk-for-go - Release
|
azure-sdk-for-java - Release
|
azure-sdk-for-net - Release
|
azure-sdk-for-python - Release
|
azure-sdk-for-python-track2 - Release
|
azure-resource-manager-schemas - Release
|
Trenton Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
This was referenced Oct 28, 2020
jlichwa
approved these changes
Oct 28, 2020
|
@fengzhou-msft is the one who will have to merge the PR. |
|
@fengzhou-msft : I have rebased my PR onto the latest from master, and all required checks are now passing. Is there anything else I need to do before you can merge the PR? |
giromm2ms
pushed a commit
to giromm2ms/azure-rest-api-specs
that referenced
this pull request
Dec 20, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Please follow the link to find more details on PR review process.