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.
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
Introduce new Secure Message API #389
Conversation
The new API has more obvious naming and should be harder to misuse, with encrypt/decrypt and sign/verify API clearly named and separated. A common mistake with the old API was for users to accidentally using sign/verify API instead of encryption by not providing a private key. New API features more strict checks and prevents this kind of mistakes. Currently the new API is implemented using the old functions (as they are intended to be used). We'll switch the implementations in the next commit and then deprecate the old API.
Actually, no. The old API will be retained as is because it has some peculiar logic for error handling and guessing whether to decrypt or verify. However, it will be deprecated in favor of the new one. |
return themis_secure_message_unwrap(private_key, private_key_length, public_key, public_key_length, encrypted_message, encrypted_message_length, message, message_length); | ||
} | ||
|
||
themis_status_t themis_secure_message_sign(const uint8_t* private_key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's time to verify the type of key to avoid misusage incorrect type of key (public/private)? : )
we can do simple check on prefix of key value for known prefixes of private keys:
- rsa private key: https://github.com/cossacklabs/themis/blob/master/src/soter/soter_rsa_key.h#L25
- ec private key: https://github.com/cossacklabs/themis/blob/master/src/soter/soter_ec_key.h#L26
if (memcmp(private_key, RSA_PRIV_KEY_PREF, sizeof(RSA_PRIV_KEY_PREF) != 0 &&
memcmp(private_key, RSA_PRIV_KEY_PREF, sizeof(EC_PRIV_KEY_PREF) != 0) {
return THEMIS_INVALID_PARAMETER;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but better will be to extend api and add some functions that will validate keys: validate_private_key
/validate_public_key
and for now, it will check the only prefix. In the future we can extend these validations with verifying full key's structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think of it... I've added the following utilities for the Rust wrapper to query the key kind and validate its content:
themis/src/wrappers/themis/rust/libthemis-sys/src/wrapper.h
Lines 28 to 41 in 29ebf63
enum themis_key_kind | |
{ | |
THEMIS_KEY_INVALID, | |
THEMIS_KEY_RSA_PRIVATE, | |
THEMIS_KEY_RSA_PUBLIC, | |
THEMIS_KEY_EC_PRIVATE, | |
THEMIS_KEY_EC_PUBLIC, | |
}; | |
/// Checks if the buffer contains a valid Themis key. | |
themis_status_t themis_is_valid_key(const uint8_t *key, size_t length); | |
/// Extracts the presumed key kind from the buffer. | |
enum themis_key_kind themis_get_key_kind(const uint8_t *key, size_t length); |
Can they be of some use? What do you think of moving these functions to the core library (and using them for checks internally as well)? I believe this API may be useful for applications to verify key content independently.
As for Secure Messages, I guess we can even go a bit further and verify that both private and public keys are of ECDSA or RSA flavor. Ideally, we'd also verify that both use the same public parameters, but that's harder to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good checks, I think it's good to move into the core themis library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, good checks, make sense at core lvl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lagovas @vixentael I've moved the key validation routines to Themis core, added some tests for them, added validation to new Secure Message API, and added some early checks to ThemisPP and JsThemis which are most affected by poor error reporting. Please give it a look.
"char" may be signed or unsigned depending on a platform. Thus it is necessary to cast it as "uint8_t" explicitly.
This effectively moves themis_secure_message_{wrap,unwrap} code to these specific functions. However, with changes and simplifications. For example, verification and decryption no longer has to guess the type of the container: we now expect only encrypted containers when decrypting and signed containers for verification. Initially I wanted to reimplement themis_secure_message_{wrap,unwrap} in terms of the new functions, however their behavior has several idiosyncrasies so the old functions are still there with the same implementation. For example, if the provided keys are invalid then in in encrypt/decrypt mode the old API returns THEMIS_INVALID_PARAM but in sign/verify mode it returns THEMIS_FAIL. The new API returns THEMIS_INVALID_PARAM consistently (as it is evident from the tests). Leave a comment in the code for developers so that they don't try to (incorrectly) reimplement, for example, unwrap with decrypt and verify or vice versa.
Actually mark the old functions them with a deprecation attribute. This will produce warnings during compilation which should prompt the users to migrate to the new API. Add an exception to our test code which still needs to test the old API. We treat warnings as errors and don't want to fail the build.
Bring in the updates in ThemisPP, we're gonna need them now to avoid merge conflicts in the future.
C++ methods are already using the new names so just replace the calls with the new ones and we're done here.
Rust methods already use new names so we only have to change the implementation. It's quite repetetive and verbose but meh... we have to call different functions with different interfaces. (It may be possible to avoid some of the copypasta with macros but I doubt they will be more maintainable than this.)
Java binding actually fails to build on CI due to deprecation warnings from the core library so we have to make these changes anyway. Current JNI interface is not flexible enough with a single boolean flag. Replace the flag with an integer action mode, provide and use the constants in Java code. JNI interface is an implementation detail so we can make this change freely.
Easy as with C++: just use the new functions and we're done. All object methods already have proper names.
Regarding language wrappers: this PR will contain only minimal necessary changes for the build to work. Some wrappers tolerate calling deprecated functions from C, these are left untouched and will be updated later. Sensitive wrappers will be taught how to use the new C API, but their language-specific API will be updated later as well. |
Co-Authored-By: ilammy <a.lozovsky@gmail.com>
These functions should be useful for Themis users so make them available. We're going to use them internally as well to validate key inputs where appropriate. Note that in Rust we no longer need to link against Soter because the functions are now exported from Themis. Also, drop the long and detalied rant about pointer alignment. It's not very realistic to do anything about it and it does not cause any trouble yet. I guess we can let it pass until we encounter SIGBUS in the wild. Just assume that the pointers are correctly aligned (and try to keep them aligned in Rust).
Add key validation to the new Secure Message API in Themis Core. Now the functions validate the key checksums and make sure that correct private and public keys are used. This should avoid unexpected behavior when a private key is used instead of a public one or vice versa.
Add validation and human-friendly error messages to ThemisPP if the user passes incorrect keys to constructor.
Add key validation to JavaScript wrapper as well. This is similar to ThemisPP but we use NAN's error reporting instead of C++ exceptions.
THEMIS_CHECK_PARAM(encrypted_message_length!=NULL); | ||
THEMIS_CHECK_PARAM(valid_private_key(private_key, private_key_length)); | ||
THEMIS_CHECK_PARAM(valid_public_key(public_key, public_key_length)); | ||
THEMIS_CHECK_PARAM(matching_key_kinds(private_key, private_key_length, public_key, public_key_length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now library more user friendly than before)
@@ -38,6 +38,8 @@ namespace jsthemis{ | |||
|
|||
static Nan::Persistent<v8::Function> constructor; | |||
|
|||
static bool ValidateKeys(const std::vector<uint8_t>& private_key, const std::vector<uint8_t>& public_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho would be better to move these function to keys.hpp
or something like that and separate from secure message because it is useful for secure session too. and if I will try to find this function in github without search then I try to find some key*.h
file or utils.h
/keygen.h
or something unrelated with specific component.
Plus unobvious why wrapper for this function in jsthemis related with SecureMessage class and not with SecureSession... Better to move it to general jsthemis/themis namespace imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. You also reminded me that Secure Session uses private and public keys as well so we should probably add checks there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that my view of Secure Session may be incorrect. I thought it supports only ECDSA keys, but it may turn out that RSA keys are fine too. I'd like to do changes in Secure Session separately after we know what to expect from the keys.
However, I'll move key generation and validation routines to separate modules in this PR, since we're already touching this API.
All language wrappers actually have a separate module named "keygen" or something like that. However, the core library keeps key generation routines in secure_message.c which is weird. Move key generation and validation into a separate source file at least. C does not have modules per se, but this makes much easier to locate keygen functions.
Put reusable code there and keep business logic of key validation in Secure Message. Later we will be able to reuse the key validation routines for Secure Session.
Put reusable code there and keep business logic of key validation in Secure Message. Later we will be able to reuse the key validation routines for Secure Session.
These checks are now performed by valid_{private,public}_key() calls. We don't need to explicitly check for NULL and empty from now on.
@Lagovas done:
|
The users may actually be interested in the status code returned for key validation so provide a separate function that returns more detailed information about the keys.
FYI: next time let's not mix new secure message API and new key gen functions, now this PR looks complicated to catch up. |
Yes. The key validation function supports all types of keys. In fact, it only validates the Soter container wrapping the key. That's not quite complete check, but it fails for data that is most definitely not a valid key.
Good point. I'll rename the functions.
I should have noticed that about ten commits ago. I'm sorry 😥 |
Check for the key kind in themis_is_valid_key(), it's not just keys using Soter containers. For example, Secure Session messages use them too and they are not keys. Also use the correct tag field instead of the whole struct. Do not cause unnecessary undefined behavior.
Use "asym" as a part of their names because there are three kinds of keys visible for Themis users: - ECDSA asymmetric keys for Secure Message and Secure Session - RSA asymmetric keys for Secure Message (and maybe Secure Session) - symmetric (AES) keys derived from master key for Secure Cell Let's clearly state that we dead with asymmetric keys here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are done here, aren't we?
@vixentael it seems so. Inco-o-omi-i-i-i-i-ing! |
The new API has more obvious naming and should be harder to misuse, with encrypt/decrypt and sign/verify API clearly named and separated.
A common mistake with the old API was for users to accidentally using sign/verify API instead of encryption by not providing a private key. New API features more strict checks and prevents this kind of mistakes.
New Secure Message API:
themis_secure_message_encrypt
themis_secure_message_decrypt
themis_secure_message_sign
themis_secure_message_verify
New key validation API:
themis_is_asym_key_valid
themis_get_asym_key_kind
Deprecated API:
themis_secure_message_wrap
themis_secure_message_encrypt
instead if you have both private and public keythemis_secure_message_sign
instead if you use only private keythemis_secure_message_unwrap
themis_secure_message_decrypt
instead if you have both private and public keythemis_secure_message_verify
instead if you use only public keyTasks:
Add key validation to Secure Session as well(will be done separately)