KMS changes #1723
KMS changes #1723
Conversation
kms/api-client/asymmetric.py
Outdated
| @@ -46,21 +48,22 @@ def decryptRSA(ciphertext, client, key_path): | |||
| Decrypt a given ciphertext using an 'RSA_DECRYPT_OAEP_2048_SHA256' private | |||
| key stored on Cloud KMS | |||
| """ | |||
| request_body = {'ciphertext': ciphertext.decode()} | |||
There was a problem hiding this comment.
In the encrypt snippet0, the corresponding request field is converted to base64 by the snippet. I believe that needs to happen here as well.
kms.projects().locations().keyRings().cryptoKeys().cryptoKeyVersions().asymmetricDecrypt(name='projects/google.com:rra-kms-quickstart/locations/global/keyRings/foo-ring/cryptoKeys/foo-key/cryptoKeyVersions/1', body={'ciphertext': 'foobar'}).execute()
...
googleapiclient.errors.HttpError: <HttpError 400 when requesting https://cloudkms.googleapis.com/v1/projects/google.com:rra-kms-quickstart/locations/global/keyRings/foo-ring/cryptoKeys/foo-key/cryptoKeyVersions/1:asymmetricDecrypt?alt=json returned "Invalid value at 'ciphertext' (TYPE_BYTES),">
I believe the binary data inputs in the body of each request should always be like "base64.b64encode(data).decode('ascii')". This leads to the return type matching the input type (in this case, plaintext comes from base64.b64decode, which returns bytes). I think it'd be best to also document that the input must be "bytes", or the equivalent python 2 type (which I think is str).
kms/api-client/asymmetric.py
Outdated
| ciphertext = base64.b64encode(ciphertext).decode('utf-8') | ||
| return ciphertext | ||
| ciphertext = public_key.encrypt(plaintext, pad) | ||
| return base64.b64encode(ciphertext) |
There was a problem hiding this comment.
This should return the bytes returned from public_key.encrypt, without base64 encoding it.
In general, treat base64 as a part of the JSON encoding, because the only reason base64 is used is because JSON only supports UTF-8 strings. base64 is a shim to make non-UTF-8 data compatible with JSON. For that reason, it's clearest to keep the base64 encoding/decoding right before/after the actual HTTP call.
There was a problem hiding this comment.
make sense, I moved the encoding into the decrypt request body
kms/api-client/asymmetric_test.py
Outdated
| ciphertext_bytes = sample.encryptRSA(self.message_bytes, | ||
| self.client, | ||
| self.rsaDecrypt) | ||
| ciphertext = ciphertext_bytes.decode('utf-8') | ||
| # ciphertext should be 344 characters with base64 and RSA 2048 | ||
| assert len(ciphertext) == 344, \ | ||
| 'ciphertext should be 344 chars; got {}'.format(len(ciphertext)) | ||
| assert ciphertext[-2:] == '==', 'cipher text should end with ==' |
There was a problem hiding this comment.
This is an odd detail to assert, and I'm surprised the test isn't flaky because of it. The API doesn't guarantee anything about the content of the ciphertext. I'd remove this assertion.
kms/api-client/asymmetric_test.py
Outdated
| self.client, | ||
| self.rsaSign) | ||
| assert success is True, 'RSA verification failed' | ||
| changed_bytes = (self.message+".").encode('utf-8') |
There was a problem hiding this comment.
Nit: alternatively, changed+bytes = message_bytes + b'.'.
There was a problem hiding this comment.
That does look nicer. Changed
6e472c5 to
800ae10
Compare
800ae10 to
e85865b
Compare
kms/api-client/asymmetric.py
Outdated
| """ | ||
| public_key = getAsymmetricPublicKey(client, key_path) | ||
|
|
||
| digest_bytes = hashlib.sha256(message.encode('ascii')).digest() | ||
| digest_bytes = hashlib.sha256(message).digest() | ||
| sig_bytes = base64.b64decode(signature) |
There was a problem hiding this comment.
The signature passed to verifySignature{RSA,EC} should be raw binary. signAsymmetric should b64decode the signature received in the response.
| self.client, | ||
| self.rsaDecrypt) | ||
| assert plaintext_bytes == self.message_bytes | ||
| plaintext = plaintext_bytes.decode('utf-8') |
There was a problem hiding this comment.
Is there much value in checking that plaintext == self.message if you've already checked that plaintext_bytes == self.message_bytes?
There was a problem hiding this comment.
Probably not, but I'd rather err on the side of too many tests, especially when dealing with all these encoding changes
There was a problem hiding this comment.
Most of the time you specify utf-8 for string.encode() and bytes.decode(), except on line 51 where you just assume the default (which is utf-8). Both ways work fine, but this is a minor inconsistency.
There was a problem hiding this comment.
good point, we should probably keep that consistent. Fixed
f3b9b79 to
927e3cc
Compare
use byte parameters instead of strings
use byte parameters instead of strings
implements changes suggested in b/113025362 and b/113025156