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

Support for AES GCM mode #6389

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Support for AES GCM mode #6389

wants to merge 7 commits into from

Conversation

nickovs
Copy link
Contributor

@nickovs nickovs commented Aug 30, 2020

This PR implements support for AES using Galois Counter Mode, a mode for Authenticated Encryption with Additional Data (AEAD) that protects not only the confidentiality of the plaintext but the integrity of both the plaintext and optional additional data. AESGCM is widely used in internet protocols, is included included in many standards and is supported by the cryptography CPython library.

This implementation currently only supports systems that use mbedTLS for cryptography and not axTLS. This is due to axTLS both not including the necessary code and being effectively a dead project from a development point of view.

The new functionality is enabled by setting the MICROPY_PY_UCRYPTOLIB_GCM flag in the mpconfigport.h header. The PR sets this by default for the esp32 build (since it builds with mbedTLS already and the size increase is modest).

The PR also includes documentation for the new class and methods.

Note that the function signatures for the encrypt() and decrypt() methods differ from the ones for the basic aes class. This is because AEAD operations both require more inputs and have different semantics to the underlying block cipher operations. This is also why the functionality was implemented as a new class rather than a new mode on the existing class.

Copy link
Contributor

@Jongy Jongy left a comment

Choose a reason for hiding this comment

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

Nice PR :) I'd like to see MicroPython getting support for such new features.

Left you some comments on the implementation. Should also add some tests - can base them on existing crypto tests at tests/extmod/ucryptolib_*.

extmod/moducryptolib.c Outdated Show resolved Hide resolved
ports/esp32/mpconfigport.h Show resolved Hide resolved
extmod/moducryptolib.c Outdated Show resolved Hide resolved
extmod/moducryptolib.c Outdated Show resolved Hide resolved
extmod/moducryptolib.c Show resolved Hide resolved
extmod/moducryptolib.c Outdated Show resolved Hide resolved
extmod/moducryptolib.c Outdated Show resolved Hide resolved
extmod/moducryptolib.c Outdated Show resolved Hide resolved
extmod/moducryptolib.c Outdated Show resolved Hide resolved
extmod/moducryptolib.c Show resolved Hide resolved
@nickovs
Copy link
Contributor Author

nickovs commented Sep 11, 2020

On the subject of tests, I wrote some, but currently the tests are run on the Unix build and the Unix build doesn't pass the standard tests when built with mbedTLS, and this only runs with mbedTLS. The axTLS project seems to be dead, so we should probably switch the Unix build over to mbedTLS. At some point when I have time I will fix the standard tests to work with mbedTLS and at that time I'll expand that test coverage to cover this too, but at the moment any tests that I write for this won't be able to run under the existing test build setup.

@nickovs
Copy link
Contributor Author

nickovs commented Oct 22, 2020

Is there any consensus on merging this? The Travis failure is a Zephyr docker image issue, which is unrelated to the PR, and the 0.004% reduction in coverage seems to be a sampling error since the changes are to do with things that are untouched by this PR.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request May 16, 2022
@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants