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

[KeyVault] - Address archboard review feedback #18319

Merged
merged 6 commits into from
Oct 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sdk/keyvault/keyvault-keys/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@

### Breaking Changes

- `KeyClient.getRandomBytes` will now return the generated bytes directly instead of wrapping them in a `RandomBytes` model.
- Since it's no longer used, `RandomBytes` has been removed from the public API.
- `KeyReleasePolicy.data` has been renamed to `KeyReleasePolicy.encodedPolicy`.
- `KeyClient.releaseKey`'s `target` parameter has been renamed to `targetAttestationToken`.
maorleger marked this conversation as resolved.
Show resolved Hide resolved

### Bugs Fixed

### Other Changes
Expand Down
11 changes: 3 additions & 8 deletions sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,13 @@ export class KeyClient {
getDeletedKey(name: string, options?: GetDeletedKeyOptions): Promise<DeletedKey>;
getKey(name: string, options?: GetKeyOptions): Promise<KeyVaultKey>;
getKeyRotationPolicy(name: string, options?: GetKeyRotationPolicyOptions): Promise<KeyRotationPolicy>;
getRandomBytes(count: number, options?: GetRandomBytesOptions): Promise<RandomBytes>;
getRandomBytes(count: number, options?: GetRandomBytesOptions): Promise<Uint8Array>;
importKey(name: string, key: JsonWebKey_2, options?: ImportKeyOptions): Promise<KeyVaultKey>;
listDeletedKeys(options?: ListDeletedKeysOptions): PagedAsyncIterableIterator<DeletedKey>;
listPropertiesOfKeys(options?: ListPropertiesOfKeysOptions): PagedAsyncIterableIterator<KeyProperties>;
listPropertiesOfKeyVersions(name: string, options?: ListPropertiesOfKeyVersionsOptions): PagedAsyncIterableIterator<KeyProperties>;
purgeDeletedKey(name: string, options?: PurgeDeletedKeyOptions): Promise<void>;
releaseKey(name: string, target: string, options?: ReleaseKeyOptions): Promise<ReleaseKeyResult>;
releaseKey(name: string, targetAttestationToken: string, options?: ReleaseKeyOptions): Promise<ReleaseKeyResult>;
restoreKeyBackup(backup: Uint8Array, options?: RestoreKeyBackupOptions): Promise<KeyVaultKey>;
rotateKey(name: string, options?: RotateKeyOptions): Promise<KeyVaultKey>;
updateKeyProperties(name: string, keyVersion: string, options?: UpdateKeyPropertiesOptions): Promise<KeyVaultKey>;
Expand Down Expand Up @@ -300,7 +300,7 @@ export interface KeyProperties {
// @public
export interface KeyReleasePolicy {
contentType?: string;
data?: Uint8Array;
encodedPolicy?: Uint8Array;
}

// @public
Expand Down Expand Up @@ -464,11 +464,6 @@ export { PollOperationState }
export interface PurgeDeletedKeyOptions extends coreHttp.OperationOptions {
}

// @public
export interface RandomBytes {
bytes: Uint8Array;
}

// @public
export interface ReleaseKeyOptions extends coreHttp.OperationOptions {
algorithm?: KeyExportEncryptionAlgorithm;
Expand Down
2 changes: 1 addition & 1 deletion sdk/keyvault/keyvault-keys/src/generated/models/index.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 5 additions & 7 deletions sdk/keyvault/keyvault-keys/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ import {
ReleaseKeyResult,
KeyReleasePolicy,
KeyExportEncryptionAlgorithm,
RandomBytes,
GetCryptographyClientOptions,
RotateKeyOptions,
UpdateKeyRotationPolicyOptions,
Expand Down Expand Up @@ -153,7 +152,6 @@ export {
GetDeletedKeyOptions,
GetKeyOptions,
GetRandomBytesOptions,
RandomBytes,
ImportKeyOptions,
JsonWebKey,
KeyCurveName,
Expand Down Expand Up @@ -784,10 +782,10 @@ export class KeyClient {
* @param count - The number of bytes to generate between 1 and 128 inclusive.
* @param options - The optional parameters.
*/
public getRandomBytes(count: number, options: GetRandomBytesOptions = {}): Promise<RandomBytes> {
public getRandomBytes(count: number, options: GetRandomBytesOptions = {}): Promise<Uint8Array> {
return withTrace("getRandomBytes", options, async (updatedOptions) => {
const response = await this.client.getRandomBytes(this.vaultUrl, count, updatedOptions);
return { bytes: response.value! };
return response.value!;
});
}

Expand Down Expand Up @@ -822,12 +820,12 @@ export class KeyClient {
* ```
*
* @param name - The name of the key.
* @param target - The attestation assertion for the target of the key release.
* @param targetAttestationToken - The attestation assertion for the target of the key release.
* @param options - The optional parameters.
*/
public releaseKey(
name: string,
target: string,
targetAttestationToken: string,
options: ReleaseKeyOptions = {}
): Promise<ReleaseKeyResult> {
return withTrace("releaseKey", options, async (updatedOptions) => {
Expand All @@ -836,7 +834,7 @@ export class KeyClient {
this.vaultUrl,
name,
options?.version || "",
target,
targetAttestationToken,
{
enc: algorithm,
nonce,
Expand Down
10 changes: 1 addition & 9 deletions sdk/keyvault/keyvault-keys/src/keysModels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ export interface KeyReleasePolicy {
contentType?: string;

/** Blob encoding the policy rules under which the key can be released. */
data?: Uint8Array;
encodedPolicy?: Uint8Array;
}

/**
Expand Down Expand Up @@ -596,14 +596,6 @@ export enum KnownKeyExportEncryptionAlgorithm {
export type KeyExportEncryptionAlgorithm = string;
/* eslint-enable tsdoc/syntax */

/**
* Result of the {@link KeyClient.getRandomBytes} operation.
*/
export interface RandomBytes {
/** The random bytes returned by the service. */
bytes: Uint8Array;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

much better 👏

/**
* Options for {@link KeyClient.getCryptographyClient}.
*/
Expand Down
12 changes: 10 additions & 2 deletions sdk/keyvault/keyvault-keys/swagger/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ directive:

### Update swagger enum values for LifetimeActionsType to reflect what the service actually returns

There is an ongoing thread about changing the swagger or returning lowercase values for enum values.

```yaml
directive:
- from: swagger-document
Expand All @@ -48,3 +46,13 @@ directive:
$.values[0].value = "Rotate";
$.values[1].value = "Notify";
```

### Rename KeyReleasePolicy.data to KeyReleasePolicy.encodedPolicy

```yaml
directive:
Copy link
Member Author

Choose a reason for hiding this comment

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

Fun fact - this was entirely generated by GitHub Copilot 😄

- from: swagger-document
where: $.definitions.KeyReleasePolicy.properties.data
transform: >
$["x-ms-client-name"] = "encodedPolicy";
```
28 changes: 15 additions & 13 deletions sdk/keyvault/keyvault-keys/test/public/keyClient.hsm.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ onVersions({ minVer: "7.2" }).describe(
onVersions({ minVer: "7.3-preview" }).describe("getRandomBytes", () => {
it("can return the required number of bytes", async () => {
const result = await hsmClient.getRandomBytes(10);
assert.exists(result.bytes);
assert.equal(result.bytes.length, 10);
assert.exists(result);
assert.equal(result.length, 10);
});

it("returns an error when bytes is out of range", async () => {
Expand Down Expand Up @@ -106,13 +106,13 @@ onVersions({ minVer: "7.2" }).describe(
const keyName = recorder.getUniqueName("exportkey");
const createdKey = await hsmClient.createKey(keyName, "RSA", {
exportable: true,
releasePolicy: { data: encodedReleasePolicy },
releasePolicy: { encodedPolicy: encodedReleasePolicy },
keyOps: ["encrypt", "decrypt"]
});

assert.exists(createdKey.properties.releasePolicy?.data);
assert.exists(createdKey.properties.releasePolicy?.encodedPolicy);
assert.isNotEmpty(
JSON.parse(uint8ArrayToString(createdKey.properties.releasePolicy!.data!))
JSON.parse(uint8ArrayToString(createdKey.properties.releasePolicy!.encodedPolicy!))
);
assert.isTrue(createdKey.properties.exportable);
const releaseResult = await hsmClient.releaseKey(keyName, attestation);
Expand All @@ -125,12 +125,12 @@ onVersions({ minVer: "7.2" }).describe(

const importedKey = await hsmClient.importKey(keyName, createRsaKey(), {
exportable: true,
releasePolicy: { data: encodedReleasePolicy }
releasePolicy: { encodedPolicy: encodedReleasePolicy }
});

assert.exists(importedKey.properties.releasePolicy?.data);
assert.exists(importedKey.properties.releasePolicy?.encodedPolicy);
assert.isNotEmpty(
JSON.parse(uint8ArrayToString(importedKey.properties.releasePolicy!.data!))
JSON.parse(uint8ArrayToString(importedKey.properties.releasePolicy!.encodedPolicy!))
);
const releaseResult = await hsmClient.releaseKey(keyName, attestation, {
version: importedKey.properties.version,
Expand All @@ -145,7 +145,7 @@ onVersions({ minVer: "7.2" }).describe(
const keyName = recorder.getUniqueName("exportkey");
const createdKey = await hsmClient.createKey(keyName, "RSA", {
exportable: true,
releasePolicy: { data: encodedReleasePolicy },
releasePolicy: { encodedPolicy: encodedReleasePolicy },
keyOps: ["encrypt", "decrypt"]
});

Expand All @@ -165,12 +165,12 @@ onVersions({ minVer: "7.2" }).describe(
version: "1.0"
};
const updatedKey = await hsmClient.updateKeyProperties(createdKey.name, {
releasePolicy: { data: stringToUint8Array(JSON.stringify(newReleasePolicy)) }
releasePolicy: { encodedPolicy: stringToUint8Array(JSON.stringify(newReleasePolicy)) }
});

assert.exists(updatedKey.properties.releasePolicy?.data);
assert.exists(updatedKey.properties.releasePolicy?.encodedPolicy);
const decodedReleasePolicy = JSON.parse(
uint8ArrayToString(updatedKey.properties.releasePolicy!.data!)
uint8ArrayToString(updatedKey.properties.releasePolicy!.encodedPolicy!)
);

// Note: the service will parse the policy and return a different shape, for example: { "claim": "sdk-test", "equals": "false" } in this test.
Expand All @@ -188,7 +188,9 @@ onVersions({ minVer: "7.2" }).describe(
it("errors when a key has a release policy but is not exportable", async () => {
const keyName = recorder.getUniqueName("policynonexportable");
await assert.isRejected(
hsmClient.createRsaKey(keyName, { releasePolicy: { data: encodedReleasePolicy } }),
hsmClient.createRsaKey(keyName, {
releasePolicy: { encodedPolicy: encodedReleasePolicy }
}),
/exportable/i
);
});
Expand Down