-
Notifications
You must be signed in to change notification settings - Fork 28
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
Cryptographic function interface #46
Conversation
I'm still dealing with some issues when running the unit tests... They pass just fine individually, but when run together, cause erratic behavior -- Since we're time crunched now, I've requested your review even before I trace down this bug as discussed in the meeting. I imagine the fix won't require significant code changes, so reviewing the interface & refactor as it is now for any changes that you think are necessary should be mostly valid & worthwhile. |
** Global Variables | ||
*/ | ||
// Cryptography Interface | ||
static CryptographyInterfaceStruct cryptography_if_struct; |
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.
Should these globals be here, or should they be declared in the crypto_interface.c ? --Per J.Lucas
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.
It's not a global actually, it's a static (local/module) variable... Sorry, I didn't update the comment here, but I did in the libgcrypt interface file:
https://github.com/nasa/CryptoLib/pull/46/files#diff-8455637f5a51288fb98808edae34ef7ba4faf362e80d728810678ec2701f1399R51
This is the local struct that contains all of the function implementations for the cryptography interface. It is defined with different function pointers depending on which interface is being used, and returned to the core application. This should be here and is really central to how the interface layer is implemented, IE, via these locally defined structs with different function pointers.
CryptographyInterface get_cryptography_interface_libgcrypt(void) | ||
{ | ||
cryptography_if_struct.cryptography_config = cryptography_config; | ||
cryptography_if_struct.cryptography_init = cryptography_init; | ||
cryptography_if_struct.get_ek_ring = get_ek_ring; | ||
cryptography_if_struct.cryptography_shutdown = cryptography_shutdown; | ||
cryptography_if_struct.cryptography_encrypt = cryptography_encrypt; | ||
cryptography_if_struct.cryptography_decrypt = cryptography_decrypt; | ||
cryptography_if_struct.cryptography_authenticate = cryptography_authenticate; | ||
cryptography_if_struct.cryptography_validate_authentication = cryptography_validate_authentication; | ||
cryptography_if_struct.cryptography_aead_encrypt = cryptography_aead_encrypt; | ||
cryptography_if_struct.cryptography_aead_decrypt = cryptography_aead_decrypt; | ||
return &cryptography_if_struct; |
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.
Should we distinguish these function names between the different types? eg kmc_cryptography_config
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.
We could do that, but I think that would actually make working with the code base somewhat trickier... If we ever need to update the interface, we generally need to update it in all the places where the functions are defined -- if they have different names, search and replace won't work, and you'll need to manually map out what goes to what in your IDE.
Having a unique name here doesn't add much, just knowing that the functions are static (IE locally defined in this code file) should be enough of an indication about what's going on.
src/src_main/crypto_tc.c
Outdated
aad_len, // Length of AAD | ||
(sa_ptr->est==1), | ||
(sa_ptr->ast==1), | ||
(sa_ptr->ast==1) |
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.
Remove duplicated pointer
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.
Should be (sa_ptr->aad==1) ? With aad instead of ast?
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 I might agree with you, these three parameters refer to the following booleans in the interface function:
uint8_t decrypt_bool, uint8_t authenticate_bool,
uint8_t aad_bool)
The question is, will you ever provide AAD, but not authenticate? Or Authenticate, but not provide AAD? If the answer is no to both questions, then we can combine those both into a single authenticate_bool.
For some reason during my refactor, I thought I ran into one such case:
https://github.com/nasa/CryptoLib/pull/46/files#diff-680e5a3e45c21aa3a202f9dcff50de85c32aad656eaae798a6052c7495c7df59R106
But that may be incorrect (there was also the issue with it doing authenticate then encrypt, instead of encrypt then authenticate). I will investigate some more.
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.
For now, I think it's OK to keep the interface function as having different controls for authentication vs AAD related calls... In the case of TC, the presence of AAD is determined by the AST flag in the SA... I don't know about the TM & key management cases, as linked above, it seems to vary, but that logic may be wrong. For the purposes of this initial interface implementation, it should be OK to keep it separate, as it is now.
src/src_main/crypto_tc.c
Outdated
aad_len, // length of AAD | ||
(sa_ptr->est==1), | ||
(sa_ptr->ast==1), | ||
(sa_ptr->ast==1) |
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.
Same question as line 460
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 this is ok. As suggested on slack, any changes, due to time constraints can be put in as issues for later.
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 like the interface. Simple and straightforward. I will followup with our guys about additional testing.
No description provided.