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

[Key Vault Keys] Add new algorithms #11380

Merged
merged 10 commits into from
Sep 24, 2020

Conversation

sadasant
Copy link
Contributor

@sadasant sadasant commented Sep 21, 2020

This PR only adds the new algorithms from v7.2.

I'll add the tests as part of another PR. I'll keep track of that through this issue: #11396

Closes: #11260

@sadasant sadasant self-assigned this Sep 21, 2020
@ghost ghost added the KeyVault label Sep 21, 2020
@sadasant sadasant marked this pull request as ready for review September 22, 2020 14:01
@@ -737,33 +742,3 @@ export class CryptographyClient {
}
}
}

/**
* Options for {@link encrypt}.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved these from this file to the models file. It made more sense to me.

@@ -137,3 +154,52 @@ export interface VerifyResult {
*/
keyID?: string;
}

/**
* Common optional properties for encrypt, decrypt, wrap and unwrap.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved these from this file to the models file. It made more sense to me.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Need to rename a member, but otherwise LGTM.

@@ -184,6 +184,13 @@ export type KeyCurveName = "P-256" | "P-384" | "P-521" | "P-256K";
// @public
export type KeyOperation = "encrypt" | "decrypt" | "sign" | "verify" | "wrapKey" | "unwrapKey" | "import";

// @public
export interface KeyOperationsOptions extends CryptographyOptions {
aad?: Uint8Array;
Copy link
Member

Choose a reason for hiding this comment

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

Should be additionalAuthenticatedData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

additionalAuthenticatedData is way better! Thank you!

@sadasant
Copy link
Contributor Author

@heaths thank you!

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Two small comments

@@ -107,6 +107,7 @@ describe("CryptographyClient (all decrypts happen remotely)", () => {
const hash = createHash("sha256");
hash.update(signatureValue);
const digest = hash.digest();
console.log({ digest });
Copy link
Member

Choose a reason for hiding this comment

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

this seems like something not meant to be left in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my bad, thank you!

@@ -148,6 +153,10 @@ export class CryptographyClient {
}
}

// Renaming parameters

requestOptions.aad = options.additionalAuthenticatedData;
Copy link
Member

Choose a reason for hiding this comment

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

why do we have to do this? Feels like a swagger transform could make the generated client take the better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the change in two parts:

Please let me know how it looks 🌞

Copy link
Member

Choose a reason for hiding this comment

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

Nice! LGTM

@sadasant sadasant merged commit 3c96491 into Azure:master Sep 24, 2020
@sadasant sadasant deleted the keyvault-keys/fix11260 branch September 24, 2020 19:42
deyaaeldeen pushed a commit to deyaaeldeen/azure-sdk-for-js that referenced this pull request Sep 25, 2020
* [Key Vault Keys] Add new algorithms

* seems like this was necessary. Not sure how I didnt catch it before

* Renamed aad as additionalAuthenticatedData

* formatting

* this seems better

* API changes after recent feedback

* lint fix

* swagger property rename WIP

* generated changes

* removed console.log
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.

[Key Vault Keys] Add new algorithms, remote only
3 participants