Skip to content

ECC support for Key Vault SDK#3577

Closed
msfcolombo wants to merge 3 commits intoAzure:psSdkJson6from
msfcolombo:psSdkJson6
Closed

ECC support for Key Vault SDK#3577
msfcolombo wants to merge 3 commits intoAzure:psSdkJson6from
msfcolombo:psSdkJson6

Conversation

@msfcolombo
Copy link
Copy Markdown

@msfcolombo msfcolombo commented Aug 11, 2017

Description

  • Added Elliptic Curve Cryptography fields and types to JsonWebKey classes.
  • Added support to ECDSA algorithms to Key Vault core. The following curves are supported: P-256, P-384, P-521 and SECP256K1.
  • Miscellaneous cleanups and enhancements.
  • Merged PR with swagger spec changes.

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@msftclas
Copy link
Copy Markdown

@msfcolombo,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@shahabhijeet
Copy link
Copy Markdown
Contributor

@msfcolombo
This PR is good to go once someone approves from key vault team (e.g. Harvey/Rick Randal)

/// The cancellation token.
/// </param>
public static async Task<KeyBundle> CreateKeyAsync(this IKeyVaultClient operations, string vaultBaseUrl, string keyName, string kty, int? keySize = default(int?), IList<string> keyOps = default(IList<string>), KeyAttributes keyAttributes = default(KeyAttributes), IDictionary<string, string> tags = default(IDictionary<string, string>), CancellationToken cancellationToken = default(CancellationToken))
public static async Task<KeyBundle> CreateKeyAsync(this IKeyVaultClient operations, string vaultBaseUrl, string keyName, string kty, string curve = default(string), int? keySize = default(int?), IList<string> keyOps = default(IList<string>), KeyAttributes keyAttributes = default(KeyAttributes), IDictionary<string, string> tags = default(IDictionary<string, string>), CancellationToken cancellationToken = default(CancellationToken))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a breaking API change for both code and existing binaries. Do we need to make a breaking change for this?

Copy link
Copy Markdown
Author

@msfcolombo msfcolombo Aug 16, 2017

Choose a reason for hiding this comment

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

A complete solution for this would be using a structure for key creation parameters. Otherwise every new key feature would cause a breaking change. But in order to avoid breaking this time, we would need Autorest to generate an overload. I will play with Swagger and Autorest to check if I can make it generate method overloads. If I can't find a solution quickly, I will change minor version and we will have to break.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@JeffSimmer I restored compatibility. Please take a look commit 296dbdb.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, I think the NewKeyParameters class is much nicer from a versioning perspective. Thanks!

… versions.

-Restored order of create key parameters.
-Added a new method for creating keys which takes a structure as parameter and is more future-proof.
-Renamed a few fields and types to make more readable and intuitive.
-Added create/sign/verify tests cases for ECC keys.
@shahabhijeet
Copy link
Copy Markdown
Contributor

@msfcolombo need signoff from your team (Rich or Hervey)

-JsonWebKey does not automatically validate itself during deserialization; this prevents cryptic or out-of-context error messages.
-JsonWebKey is now sealed and stores JSON properties that are not part of schema; this allows the class to support future changes to the protocol.
-Verification logic was moved to the new type JsonWebKeyVerifier, which provides rich features such as plugability for new key types.
-Fixed letter case on informal texts such as assembly title.
-Bumped some versions to reflect changes and compatibility breaks.
@shahabhijeet
Copy link
Copy Markdown
Contributor

@msfcolombo this PR will be closed by EOW. You can always reopen again.
This PR needs an approval from a KeyVault team members I have mentioned above.

@msfcolombo
Copy link
Copy Markdown
Author

Closing for now. Will reopen again after I get internal sign-off.

@msfcolombo msfcolombo closed this Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants