Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
|
| crypto_client = client.get_cryptography_client(name, key_version=version) | ||
| return crypto_client.sign(SignatureAlgorithm(algorithm), digest.encode('utf-8')) | ||
| return crypto_client.sign(SignatureAlgorithm(algorithm), | ||
| hash_func(base64.b64decode(digest.encode('utf-8'))).digest()) |
There was a problem hiding this comment.
hash before signing as suggested in other language sample: https://learn.microsoft.com/en-us/azure/key-vault/keys/javascript-developer-guide-sign-verify-key#signing-data
There was a problem hiding this comment.
the argument is called digest, it's already a hash of the data. This is computing a hash of hash.
There was a problem hiding this comment.
@evelyn-ys best to confirm what crypto_client.sign() does specifically (does it digest again internall), but re-digesting the digest before calling sign() is definitely unnecessary.
|
|
||
|
|
||
| def sign_key(cmd, client, algorithm, digest, name=None, version=None): | ||
| if '256' in algorithm: |
There was a problem hiding this comment.
This should really be matching a list of precise allowed identifiers for the digest functions that are supported. Someone passing 'sha-224' for example will get a signature using sha-512, and that's not going to be what they want.
There was a problem hiding this comment.
All the options for algorithm are ES256, ES256K, ES384, ES512, PS256, PS384, PS512, RS256, RS384, RS512, so it's either sha256, sha384 or sha512
There was a problem hiding this comment.
Ok, but this list is presumably checked elsewhere, and may get extended in the future, at which point this can become unsafe. Something like:
signing_algo_to_digest_algo = {
"ES256": hashlib.sha256,
...
}
hash_func = signing_algo_to_digest_algo[algorithm]
would be clearer and reduce the chance that the function can be used in an unsafe manner.
|
|
||
| def verify_key(cmd, client, algorithm, digest, signature, name=None, version=None): | ||
| import base64 | ||
| if '256' in algorithm: |
There was a problem hiding this comment.
Same as above, this is worse because it has the potential to create confusion about what's being verified. It's really important to have a precise mapping.
| crypto_client = client.get_cryptography_client(name, key_version=version) | ||
| return crypto_client.verify(SignatureAlgorithm(algorithm), | ||
| digest.encode('utf-8'), | ||
| hash_func(base64.b64decode(digest.encode('utf-8'))).digest(), |
There was a problem hiding this comment.
The extra round of digest is going to be an unwelcome surprise for users, who won't expect it, if they try to verify using their own code. There needs to be a testcase that checks that signatures created using the REST API directly can be verified with this call.
There was a problem hiding this comment.
I agree that CLI shouldn't make extra undocumented logic above REST API. Will revert additional hash
|
the patch applied cleanly, I tested OK with |
5 participants
Related command
az keyvault key sign/verifyDescription
Fix #27631, #28027
--digestnow accepts base64 encoded stringTesting Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.