Use new Secure Message API in PyThemis #401
Conversation
Straightforward changes to use new C API of Secure Message. We just need to call different functions and that's it. Keep Python method naming the same, we'll update it later separately.
Previous implementation allowed to (ab)use SecureMessage class in sign/verify mode by not specifying one of the keys. It is not possible now since we're using the new C API. Now we require both public and private key to be specified at Secure Message construction. Update the tests to verify the new requirement and add more error checking to constructor to ensure that both keys are provided and that they have correct kinds.
| self.private_key, len(self.private_key), | ||
| self.peer_public_key, len(self.peer_public_key), | ||
| message, len(message), plain_message, byref(plain_message_length)) | ||
| if res != THEMIS_CODES.SUCCESS: | ||
| raise ThemisError(res, "Secure Message failed decrypting") | ||
| raise ThemisError(res, "Secure Message failed to decrypt") | ||
| return string_at(plain_message, plain_message_length.value) | ||
|
|
||
|
|
||
| def ssign(private_key, message): | ||
| encrypted_message_length = c_int(0) |
There was a problem hiding this comment.
can you add private_key length check and error message here as well? I think it would be nice addition to this PR
There was a problem hiding this comment.
Yeah, it makes sense to check keys for sign/verify mode as well. I'll add the checks.
| encrypted_message, byref(encrypted_message_length)) | ||
| if res != THEMIS_CODES.SUCCESS: | ||
| raise ThemisError(res, "Secure Message failed singing") | ||
| raise ThemisError(res, "Secure Message failed to sign") | ||
| return string_at(encrypted_message, encrypted_message_length.value) | ||
|
|
||
|
|
||
| def sverify(public_key, message): | ||
| plain_message_length = c_int(0) |
| EC_PUBLIC = 4 | ||
|
|
||
|
|
||
| def _public_key(key): |
There was a problem hiding this comment.
better to make this function public to allow use it for apps
There was a problem hiding this comment.
and better to rename to something like validate_public_key because _public_key looks like getter function
There was a problem hiding this comment.
Mmm... I'm not against it but I think there's a better way.
These two functions are effectively internal helpers which implement a particular check specifically for Secure Message needs. However, the user may have different needs that that. I think that for general use it would be better to export C functions themis_is_valid_asym_key and themis_get_asym_key_kind which provide a bit more information about the keys. Then the user could implement their of is_valid_public_key function the way they want.
There was a problem hiding this comment.
will be great. but not only is_valid_public_key, I think is_valid_private_key will be useful for users too.
There was a problem hiding this comment.
I think it's a good idea overall, but let's do it later if you don't mind: #409
Check key kinds in sign/verify mode as well and produce user-friendly error messages. Update the tests to expect ThemisError now if the key is None. Previously this failed on len() method call, now we check the type correctly.
Straightforward changes to use new C API of Secure Message. We just need to call different functions and that's it.
Keep Python method naming the same, we'll update it later separately.
Previous implementation allowed to (ab)use SecureMessage class in sign/verify mode by not specifying one of the keys. It is not possible now since we're using the new C API. Now we require both public and private key to be specified at Secure Message construction.
Update the tests to verify the new requirement and add more error checking to constructor to ensure that both keys are provided and that they have correct kinds.