add keyvault decrypt #333
add keyvault decrypt #333
Conversation
|
|
There was a problem hiding this comment.
Thank you for the contributoin. While we do want idiomatic code (naming conventions, usage patterns, etc.) we also want names to be consistent with existing Azure SDKs, such as the .NET SDKs I referenced a few times here, though a mix of Java and Python SDKs might be easier, but the main difference you'll see is that .NET uses PascalCase while Rust uses snake_case as you have done mostly.
sdk/key_vault/src/key.rs
Outdated
| #[derive(Debug, Deserialize, Getters)] | ||
| #[getset(get = "pub")] | ||
| pub struct DecryptResult { | ||
| aad: Option<String>, |
There was a problem hiding this comment.
This should be additional_authenticated_data. See https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/keyvault/Azure.Security.KeyVault.Keys/api/Azure.Security.KeyVault.Keys.netstandard2.0.cs for names, adjusting for idiomatic naming conventions, of course. We want Azure SDKs to be first and foremost idiomatic, but consistent second and naming is one of them.
This also shouldn't be defined on DecryptResult. It's passed into DecryptParameters (as it should be called). Specifically, see the classes at https://github.com/Azure/azure-sdk-for-net/blob/fe8e623c8ef31810a4712fb12fec4ecbc019859f/sdk/keyvault/Azure.Security.KeyVault.Keys/api/Azure.Security.KeyVault.Keys.netstandard2.0.cs#L395-L422.
There was a problem hiding this comment.
I was looking at https://docs.microsoft.com/en-us/rest/api/keyvault/decrypt/decrypt#keyoperationresult
and the key operation result contain these field. I guess the azure sdk for .NET is more precise
There was a problem hiding this comment.
Yeah, we did change a few property names mainly to avoid abbreviations (we have guidelines against that). Abbreviations are great for a transform protocol but not always an SDK, or so the thinking goes. The SDK does also change the shape a bit for some things you haven't touched here, too (so not an issue with this PR).
sdk/key_vault/src/key.rs
Outdated
| #[getset(get = "pub")] | ||
| pub struct DecryptResult { | ||
| aad: Option<String>, | ||
| iv: Option<String>, |
There was a problem hiding this comment.
Even though aad and iv shouldn't be defined here, they are base64url-encoded and should be exposed as binary, i.e. Option<Vec<u8>>
|
|
||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum EncryptionAlgorithm { |
There was a problem hiding this comment.
Please keep consistent casing. If it's idiomatic for all-uppercase characters, do so throughout. See https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/EncryptionAlgorithm.cs for how .NET did this. IIRC, Java is all uppercase since that is idiomatic for Java statics.
There was a problem hiding this comment.
I will follow this as well. But I was looking at the SignatureAlgorithms above, which doesn't follow the .NET.
There was a problem hiding this comment.
At this point we're not concerned with breaking changes - in fact, there will be a lot more to come - so you could make SignatureAlgorithms match the guidance or ignore for now and I'll clean it up later.
sdk/key_vault/src/key.rs
Outdated
| value: &str, | ||
| aad: Option<&str>, | ||
| iv: Option<&str>, | ||
| tag: Option<&str>, |
There was a problem hiding this comment.
These should be put in DecryptParameters. See https://github.com/Azure/azure-sdk-for-net/blob/fe8e623c8ef31810a4712fb12fec4ecbc019859f/sdk/keyvault/Azure.Security.KeyVault.Keys/api/Azure.Security.KeyVault.Keys.netstandard2.0.cs#L358 and https://github.com/Azure/azure-sdk-for-net/blob/fe8e623c8ef31810a4712fb12fec4ecbc019859f/sdk/keyvault/Azure.Security.KeyVault.Keys/api/Azure.Security.KeyVault.Keys.netstandard2.0.cs#L395-L415.
sdk/key_vault/src/key.rs
Outdated
| &mut self, | ||
| algorithm: EncryptionAlgorithm, | ||
| key_name: &str, | ||
| key_version: &str, |
There was a problem hiding this comment.
Even in DecryptParameters, version (not key_version) is optional despite what the swagger says (path variables can't be optional, but they handle this correctly on the server.
sdk/key_vault/src/key.rs
Outdated
| request_body.insert("alg".to_owned(), Value::String(algorithm.to_string())); | ||
| request_body.insert("value".to_owned(), Value::String(value.to_owned())); | ||
|
|
||
| if let Some(aad) = aad { |
There was a problem hiding this comment.
These members should be Vec<u8> (or Option<Vec<u8>>) and base64url-encoded here - don't make the user do it in their own code, which is inconsistent with our other Azure SDKs.
|
I tried to look into the net, java, python, js sdk to get inspiration. |
sdk/key_vault/src/key.rs
Outdated
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum DecryptParametersEncryption { | ||
| Rsa15(Rsa15Parameters), |
There was a problem hiding this comment.
I think this could all be simplified to families. RSA will only ever take the ciphertext and algorithm, while AES GCM will take the ciphertext, aad, and tag, and AES CBC the ciphertext and iv. Would it be possible to simplify these? Overall, though, I appreciate the attention to detail since the parameters do take different arguments.
There was a problem hiding this comment.
I simplified but had to add an error in the process, in order to make sure the params can't be created with the wrong alg. Let me know if that's ok, or even if there is a better way
|
updated with latest remarks, waiting for review |
There was a problem hiding this comment.
This looks great! Thank you. So much cleaner. Only one other change because inputs to encrypt and decrypt are different - at least for AES GCM. I'd just name all the structs *DecryptParameters. A couple may be duplicated for encryption, but should the models - or at least parameters to construct them - change in the future, we don't incur any breaking changes. I'll get it merged then.
Nothing too fancy here, but I really quickly need the decrypt function for the vault