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.

Sign up for GitHub

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

Jump to bottom

key_client: supply GetRandomBytes API. #1271

Merged
merged 8 commits into from
May 9, 2023
Merged

key_client: supply GetRandomBytes API. #1271

merged 8 commits into from
May 9, 2023

Conversation

LykxSassinator
Copy link
Contributor

@LykxSassinator LykxSassinator commented Apr 26, 2023 •
edited
Loading

Signed-off-by: Lucasliang nkcs_lykx@hotmail.com

In this pr:

  • The ManagedHsmClient has been supplied for accessing managed HSM resources.
  • Supply GetRandomBytes API to ManagedHsmClient for generating random values.
  • Supply GetRandomBytes API to KeyClient.

@LykxSassinator
managed_hsm_client: add ManagedHsmClient and supply GetRandomBytes AP…
940dc78 
…I for it.

Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>

In this pr:
* The ManagedHsmClient has been supplied for accessing managed HSM resources.
* Supply GetRandomBytes API to ManagedHsmClient for generating random values.
heaths
heaths requested changes Apr 26, 2023
View reviewed changes
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.

This should be added to the KeyClient reflecting what we do in other languages. Don't validate REST methods apart from any URI path parameters, and let the service return a service error as implemented; thus, KeyClient would return an error reflecting the service's HTTP 400.

@LykxSassinator
Copy link
Contributor Author

LykxSassinator commented Apr 27, 2023 •
edited
Loading

This should be added to the KeyClient reflecting what we do in other languages.

FYI, several things are a little bit wired to me, triggering me to divide GetRandomBytes into ManagedHsmClient:

  • The GetRandomBytes API belongs to ManagedHsm, from the official doc, but
    image
  • Moreover, I've made the related tests and found that there exists errors if I support a vaultUrl not a mangedHsmUrl, that:

ErrorDetails { code: Some("Unauthorized"), message: Some("Found token with aud=https://vault.azure.net, but expected aud=https://managedhsm.azure.net

These phenomenon showed me that GetRandomBytes is not an API belongings to KeyClient.

@LykxSassinator
Revoke ManagedHsmClient and move GetRandomBytes into KeyClient.
b92d768 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator
Copy link
Contributor Author

LykxSassinator commented Apr 27, 2023 •
edited
Loading

/cc @heaths Done for it. FYI, I accept to make it consistent with implementations in other languages. Thx.

@LykxSassinator LykxSassinator changed the title managed_hsm_client: add ManagedHsmClient and supply GetRandomBytes API. key_client: supply GetRandomBytes API. Apr 27, 2023
@LykxSassinator
Modify the return type for GetRandomBytes.
a7bf99c 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator
Polish annotations.
982cb4e 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator
Polish annotations again.
28ef19c 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
heaths
heaths requested changes Apr 27, 2023
View reviewed changes
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.

Much better, thank you. One more change, though.

There are a number of APIs that don't work for both Key Vault and Managed HSM, but I'm one of the owners for the SDK and the design of both the service REST APIs and the SDKs are such that developers could test most functionality with the relatively cheap Key Vault service but deploy against a Managed HSM. Few key operations are not supported for both, including /rng.

sdk/security_keyvault/src/keys/models.rs Outdated Show resolved Hide resolved
@LykxSassinator
Revoke the modification on the return type of GetRandomBytesResult.
a13c48a 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
heaths
heaths requested changes May 1, 2023
View reviewed changes
sdk/security_keyvault/src/keys/models.rs Outdated Show resolved Hide resolved
@LykxSassinator
Remove unnecessary serialization of GetRandomBytesResult.
bfa5c4e 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator LykxSassinator requested a review from heaths May 4, 2023 03:43
heaths
heaths approved these changes May 4, 2023
View reviewed changes
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.

Thank you!

sdk/security_keyvault/src/keys/models.rs
deserialize_with = "deser_base64"
)]
/// `value` is encoded as a base64url string.
#[serde(rename = "value", deserialize_with = "deser_base64")]
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see we're already doing that with this function. I forgot it was locally implemented.

@LykxSassinator
Polish clippy format errors.
17ff3c5 
Signed-off-by: Lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator
Copy link
Contributor Author

LykxSassinator commented May 5, 2023 •
edited
Loading

/cc @demoray PTAL, thx. If there is no new comments on this pr, please help me to merge it.

@LykxSassinator LykxSassinator requested a review from heaths May 9, 2023 03:10
@LykxSassinator
Copy link
Contributor Author

@heaths If there was no other advices or comments, could u help me to merge it? thx.

@demoray demoray merged commit 56c32bb into Azure:main May 9, 2023
8 checks passed
@demoray
Copy link
Contributor

demoray commented May 9, 2023

Thanks for the contributions!

@LykxSassinator LykxSassinator mentioned this pull request May 17, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heaths heaths Awaiting requested review from heaths heaths is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LykxSassinator @demoray @heaths