Add a createOctKey convenience method and relevant options. #13522

maorleger · 2021-02-02T01:05:40Z

What

Add createOctKey method to support creating keys of type oct and oct-HSM
Add createOctKeyOptions supporting hsm (keySize is already supported at the createKeyOptions interface)

Why

This adds the same convenience layer we have for createEcKey, createRsaKey, etc....

Callouts

Decided to use hsm to be compatible with the APIs we already shipped. Feel free to let me know if you want this to be hardwareProtected at the cost of being inconsistent with our other createKeyOptions.

resolves #12457

maorleger · 2021-02-02T16:45:45Z

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

+  /**
+   * Whether to create a hardware-protected key in a hardware security module (HSM).
+   */
+  hsm?: boolean;


I went with hsm because we already use hsm on createEcKeyOptions and createRsaKeyOptions - but feel free to let me know if we should break tradition here to use hardwareProtected

sadasant

Love it! Thank you.

check-enforcer · 2021-02-02T22:29:00Z

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-02-02T22:45:28Z

/check-enforcer override

build passes, but there's a known issue with check enforcer

heaths · 2021-02-02T23:08:13Z

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

@@ -42,6 +42,11 @@ export interface CreateKeyOptions extends coreHttp.OperationOptions {
    };
 }

+// @public
+export interface CreateOctKeyOptions extends CreateKeyOptions {
+    hsm?: boolean;


This should've been hardwareProtected, but I see you shipped previously with hsm so that's fine. I guess this was missed in unification.

What about keySize here? That's valid as well. See https://github.com/Azure/azure-sdk-for-net/blob/f8aabfa3cc1b0a7ae2b6a1c111ea96416b3a4f4d/sdk/keyvault/Azure.Security.KeyVault.Keys/src/CreateOctKeyOptions.cs#L27. In .NET we have optional "parameters" as read-write properties instead of constructor params (reserved for required "parameters" and their properties are typically read-only).

Opened #13545 for the second issue.

Agree on using hardwareProtected but yea I kept it hsm to be consistent across our APIs (see this comment: Add a createOctKey convenience method and relevant options. #13522 (comment))

I was wondering as well, but noticed we already offer keySize in CreateKeyOptions which this extends. See here:

azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/keysModels.ts

Line 297 in d476db9

keySize?: number;

That change was before my time, but when I used this new options bag (CreateOctKeyOptions) in sample code keySize was available to me. Do you think I should move it to a few specific Create<KeyType>Options classes instead?

Interesting. In the PR, I noticed that createRsaKeyOptions also defines it at

azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/keysModels.ts

Line 351 in d476db9

keySize?: number;

, which is what threw me. Probably can't change it now, but keySize probably shouldn't have been defined on the base; practically, though, I doubt it would never not be a parameter.

Yea, not sure why it's overridden - might be a miss. But, since we can't remove createKeyOptions.keySize now maybe the thing to do is to remove the overrides since it's just debt at this point? I can reuse and update #13545 to reflect that.

We define `keySize` in `createRsaKeyOptions` but we don't have to since it's already defined in `createKeyOptions` which this inherits from. Having it defined in two places adds confusion (see #13522 (comment) as an example). This commit just removes the redefined parameter and updates the base parameter's doc comment to be clearer. Fixes #13589

Remove readonly for keyOps and keySize (Azure#13522)

maorleger requested review from bterlson, chradek, sophiajt, sadasant and xirzec as code owners February 2, 2021 01:05

ghost added the KeyVault label Feb 2, 2021

maorleger requested a review from heaths February 2, 2021 16:42

maorleger commented Feb 2, 2021

View reviewed changes

sadasant approved these changes Feb 2, 2021

View reviewed changes

maorleger added 4 commits February 2, 2021 14:23

Add a createOctKey convenience method and relevant options.

8fb0c4c

use known value

80df24d

lint

06d8ca6

CHANGELOG

3c11811

maorleger force-pushed the create-oct-key-options branch from d489191 to 3c11811 Compare February 2, 2021 22:23

maorleger merged commit d476db9 into Azure:master Feb 2, 2021

maorleger deleted the create-oct-key-options branch February 2, 2021 22:46

heaths reviewed Feb 2, 2021

View reviewed changes

heaths mentioned this pull request Feb 2, 2021

CreateOctKeyOptions required keySize parameter #13545

Closed

This was referenced Feb 4, 2021

[KeyVault] - Tech debt cleanup #13589

Closed

Remove keySize override from createRsaKeyOptions #13684

Merged

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Mar 23, 2021

CodeGen from PR 13522 in Azure/azure-rest-api-specs

61cc62a

Remove readonly for keyOps and keySize (Azure#13522)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a createOctKey convenience method and relevant options. #13522

Add a createOctKey convenience method and relevant options. #13522

maorleger commented Feb 2, 2021

maorleger Feb 2, 2021

sadasant left a comment

check-enforcer bot commented Feb 2, 2021

maorleger commented Feb 2, 2021 •

edited

Loading

heaths Feb 2, 2021

heaths Feb 2, 2021

maorleger Feb 2, 2021 •

edited

Loading

heaths Feb 3, 2021

maorleger Feb 3, 2021 •

edited

Loading

Add a createOctKey convenience method and relevant options. #13522

Add a createOctKey convenience method and relevant options. #13522

Conversation

maorleger commented Feb 2, 2021

What

Why

Callouts

maorleger Feb 2, 2021

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

check-enforcer bot commented Feb 2, 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 commented Feb 2, 2021 • edited Loading

heaths Feb 2, 2021

Choose a reason for hiding this comment

heaths Feb 2, 2021

Choose a reason for hiding this comment

maorleger Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

heaths Feb 3, 2021

Choose a reason for hiding this comment

maorleger Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

maorleger commented Feb 2, 2021 •

edited

Loading

maorleger Feb 2, 2021 •

edited

Loading

maorleger Feb 3, 2021 •

edited

Loading