Skip to content
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.

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

Added support for Secure Key Release #23276

Merged
merged 10 commits into from
Aug 9, 2021
Merged

Conversation

vcolin7
Copy link
Member

@vcolin7 vcolin7 commented Jul 31, 2021

Will add tests and recordings soon.

…d required models for this new operations; modified other related models as well. Updated CHANGELOG.
@ghost ghost added the KeyVault label Jul 31, 2021
@vcolin7 vcolin7 self-assigned this Jul 31, 2021
@vcolin7 vcolin7 added this to the [2021] August milestone Jul 31, 2021
@@ -2,6 +2,9 @@

## 4.4.0-beta.2 (Unreleased)

### Features Added
- Added `Exportable` and `ReleasePolicy` to `CreateKeyOptions`, `ImportKeyOptions`, and `KeyProperties` to support Secure Key Release for Key Vault and Managed HSM.
Copy link
Member

Choose a reason for hiding this comment

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

Have you been able to get SKR working on AKV? It works on HSM, but when I try to create an exportable key -- on a premium SKU vault -- I get back this exception:
(BadParameter) AKV.SKR.1001: The exportable attribute is only supported with premium vaults and API version >= '7.3-preview'.
(This may be something to bring up with the service team, since my request met both the error response's criteria and still failed.)

Copy link
Member Author

@vcolin7 vcolin7 Aug 6, 2021

Choose a reason for hiding this comment

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

I think @maorleger or @heaths brought this up last week. I am currently writing some tests but I suspect I will see the same error message. I'll let you know what I actually get from the service.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked about this directly with @mccoyp, but I'm putting it here for the record. Azure Key vault does not support releasing keys as of this moment but it's in the roadmap. Support for this feature is only available for Managed HSMs.

…a required constructor parameter on `KeyReleasePolicy`.
- Added an empty constructor to fix a serialization issue in KeyReleasePolicy.
- Changed `vaultBaseUrl` to `url` in `KeyService.release()`.
- Added ``.setReleasePolicy()` to `CreateEcKetOptions` and `CreateOctKetOptions`.
}

/**
* Set the {@link OffsetDateTime notBefore} UTC time.
* Set the key size.
Copy link
Member

Choose a reason for hiding this comment

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

No "in bits" like the other classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@MethodSource("getTestParameters")
public void releaseKey(HttpClient httpClient, KeyServiceVersion serviceVersion) {
// TODO: Remove assumption once Key Vault allows for creating exportable keys.
Assumptions.assumeTrue(isManagedHsmTest);
Copy link
Member

Choose a reason for hiding this comment

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

This is client-side validation and you should let the service error. We don't want tight coupling between SDK and service (it's in our guidelines). Only in rare cases - like null checks for required URL path parameters - do we do client-side validation of service client methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do let the service error when it comes down to the public APIs. I added this to a test case just to avoid having it break the live tests. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I recommend it, yes. If/when the service does change - especially unexpectedly - you'll know when it doesn't match the recordings, assuming you validate your request bodies like we do in .NET by default.

@vcolin7 vcolin7 enabled auto-merge (squash) August 9, 2021 23:23
@vcolin7 vcolin7 merged commit a804ef1 into Azure:main Aug 9, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-java that referenced this pull request Apr 17, 2023
Create new version 2022-09-30-preview for MySQL (Azure#23276)

* add base folder 2022-09-30-preview from 2021-12-01-preview

* update version and add new file backupandexport.json

* update readme

* add common-types

* fix readme

* copy 2021-12-01-preview folder to init 2022-09-30-preview

* update version, readme and add new api in FlexibleServers.json

* fix spell check

* fix linter

* fix linter

* fix linter for flexible server

* add location in header to fix linter

* add base for serviceoperation

* update api version

* add new api for service operation

* update readme

* fix model validation

* fix prettier

* update to v5 for apiVersionParameter and resourceGroupName

* fix linter

* fix lro errorResponse

* enableScaling and revert configuration

* fix enableStatusEnum

* sync backupExport

* update readme

* remove service operation

* update backups

* udpate backup name

* fix final-state-via

* fix lro option

* revert replication role

* use cloudError

* Update readme.python.md

* fix customer word

* add default value for EnableStatusEnum

* add logondisk

* fix description

---------

Co-authored-by: Yuchao Yan <yuchaoyan@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants