Skip to content
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

Add CryptoInterface library #6961

Merged
merged 13 commits into from
Apr 29, 2020
Merged

Add CryptoInterface library #6961

merged 13 commits into from
Apr 29, 2020

Conversation

aerlon
Copy link
Contributor

@aerlon aerlon commented Dec 29, 2019

  • Add CryptoInterface library

  • Add TypeConversion core files.

A library providing an easy-to-use frontend for the cryptographic functionality of BearSSL. Created as an offshoot from the code of the new mesh library PR, based on discussions here.

@aerlon aerlon mentioned this pull request Dec 29, 2019
7 tasks
- Make HelloCrypto.ino stylish.

- Include assert.h in CryptoInterface.cpp.
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome commenting in the code. I've done a quick once-over, seems like a good start!

cores/esp8266/TypeConversion.cpp Outdated Show resolved Hide resolved
cores/esp8266/TypeConversion.cpp Outdated Show resolved Hide resolved
cores/esp8266/TypeConversion.cpp Outdated Show resolved Hide resolved
cores/esp8266/TypeConversion.h Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.cpp Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.cpp Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.cpp Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than minor comments already made, I have two thoughts:

  1. If it makes sense to use this from the core, then it should be moved to core files. If it's a library, it's off limits for the core, because the core can't be dependent on a library.
  2. I envisioned a different structure, i.e.: have interface objects within the namespace, one for each algorithm, with usage similar to the base64 class: one class for sha1, one for sha256, one for md5, etc. For example:
class md5
{
  String hash(blah);
  void *hmac(blah);
  void *hmacCT(blah);
...
};
class sha1
{
  String hash(blah);
  void *hmac(blah);
  void *hmacCT(blah);
...
};
...

If it makes sense, each class could hold context for successive calls, or if it doesn't, the methods could be static.
But, I don't want to delay moving forward for this, so if you want to leave that for another PR it's ok. The choice is yours.
However, please put everything in namespace experimental to not get tied down with maintaining compatibility for the moment while we get around to reworking that.

Other things that could be unified behind this CryptoInterface in the future:
crc16, unify crc32 use, move base64, implement base58, unify md5 use.

libraries/CryptoInterface/src/CryptoInterface.h Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.h Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.h Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the suggested code style (tests/astyle_core.conf) for new code..

- Add deprecated attribute to SHA1 and MD5 hashes.

- Remove _warningsEnabled since this has been replaced by the deprecated attribute.

- Prefix all getters with "get".

- Move all CryptoInterface functionality to the experimental namespace.

- Change formatting of core files.

- Improve comments.
@aerlon
Copy link
Contributor Author

aerlon commented Jan 6, 2020

Thank you for the feedback. I've corrected most issues and depending on what you think of the remaining ones this PR may be merge ready.

@dirkmueller Ah, so there is a separate style for core files and example files. Good to know.

@earlephilhower Thanks! I think there's actually more comments than code in this library, but then again it is supposed to be a frontend.

@devyte I should probably finish rewriting the mesh library before starting to rewrite this, so I've put the entire CryptoInterface in namespace experimental for now.

@devyte devyte self-requested a review April 3, 2020 20:49
@devyte devyte self-assigned this Apr 18, 2020
@devyte
Copy link
Collaborator

devyte commented Apr 18, 2020

@aerlon I think this needs some restructuring, and probably moving out of a library and into the core, but given that it's in namespace experimental we can merge it as-is and repolish later.

@devyte devyte added this to the 2.7.0 milestone Apr 18, 2020
cores/esp8266/TypeConversion.cpp Outdated Show resolved Hide resolved
cores/esp8266/TypeConversion.cpp Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.h Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.h Outdated Show resolved Hide resolved
libraries/CryptoInterface/src/CryptoInterface.h Outdated Show resolved Hide resolved
* Defaults to nullptr.
* @param aadLength The length of the aad array in bytes. Defaults to 0.
*/
void chacha20Poly1305Encrypt(void *data, const size_t dataLength, const void *key, const void *keySalt, const size_t keySaltLength, void *resultingNonce, void *resultingTag, const void *aad = nullptr, const size_t aadLength = 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. ChaCha is fast on the 8266, but AES is also widely used and abstracting one more layer into an encryption major class seems more logical to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is now an object with encrypt and decrypt methods. Since free standing objects were requested no further abstraction has been done.

libraries/CryptoInterface/src/CryptoInterface.cpp Outdated Show resolved Hide resolved
aerlon and others added 4 commits April 26, 2020 22:01
- Remove delay in setup() from HelloCrypto example since it does not seem to be required to prevent missing initial Serial prints.

- Mark type conversion functions as big endian.

- Update keywords.txt.
- Create ESP.random functions in the core based on the defaultNonceGenerator code, and use these in defaultNonceGenerator.

- Rename CryptoInterface to esp8266::Crypto and move all functionality to the core.

- Remove need to #include <bearssl/bearssl.h> in the Crypto header file by changing br_hkdf_context to ::br_hkdf_context.

- Restyle code files for core usage.
@earlephilhower earlephilhower self-requested a review April 28, 2020 18:51
- Rename namespace Crypto to namespace crypto.
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some details:

  • TypeConversion should be wrapped by namespace experimental instead of namespace esp8266
  • Code that makes use of this should have a using namespace experimental::crypto at the top to ease use.

But we can fix those details in a subsequent PR.
Approving! And well done.

cores/esp8266/Crypto.h Outdated Show resolved Hide resolved
}

void loop() {
// This serves only to demonstrate the library use. See the header file for a full list of functions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up here can do something like:

using namespace experimental::crypto;

Then further down you can use directly, e.g.:

...
getNonceGenerator()(...);
...
SHA256::hash(...);
...
SHA256::hmac(...);

etc. This eases use and reduces the long fully qualified names. It also allows easy remapping of namespaces, because you just change the namespace import at the top instead of having to change each fully qualified name through the code.

@devyte devyte merged commit 3c9a75f into esp8266:master Apr 29, 2020
@aerlon aerlon mentioned this pull request Apr 29, 2020
@aerlon
Copy link
Contributor Author

aerlon commented Apr 29, 2020

@devyte Thanks. The issues you mentioned are fixed in PR #7252.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants