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

AES implementation without a TLS or SSL library #7512

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

Conversation

nickovs
Copy link
Contributor

@nickovs nickovs commented Jul 8, 2021

Currently ucryptolib only provides an AES implementation if one of the SSL or TLS libraries is included. This means that AES is unavailable on ports that don't support a full TLS stack and, on those ports that do have a TLS stack, it is necessary to have the entire stack in order to support AES. There is already a fallback implementation of SHA256 for uhashlib; this PR provides a fallback implementation of AES for ports where no TLS stack is included.

The AES implementation here was built from scratch with a view to being portable, and to optimising runtime memory needs over code size and code size over pure performance (although the performance is in fact pretty good). It has been tested against the test vectors in NIST Special Publication 800-38A as well as passing the AES tests in tests/extmod/ucryptolib_*.py.

The code has been tested on the Unix (macOS), esp32 and rp2 ports; it should probably be tested on other ports before merging. Since the rp2 port does not currently have any AES implementation, the PR enables the new implementation on that port by default.

@codecov-commenter
Copy link

Codecov Report

Merging #7512 (48d5fdd) into master (e10a044) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7512      +/-   ##
==========================================
- Coverage   98.29%   98.28%   -0.01%     
==========================================
  Files         154      154              
  Lines       19989    19989              
==========================================
- Hits        19648    19647       -1     
- Misses        341      342       +1     
Impacted Files Coverage Δ
extmod/moducryptolib.c 92.23% <100.00%> (+0.07%) ⬆️
py/bc.c 88.65% <0.00%> (-1.04%) ⬇️
py/obj.c 96.82% <0.00%> (ø)
py/objtype.c 100.00% <0.00%> (ø)
py/runtime.c 99.38% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e10a044...48d5fdd. Read the comment docs.

@nickovs nickovs force-pushed the native_aes branch 2 times, most recently from 52171e7 to 713069d Compare July 8, 2021 19:41
@jimmo
Copy link
Member

jimmo commented Jul 8, 2021

This means that AES is unavailable on ports that don't support a full TLS stack and, on those ports that do have a TLS stack, it is necessary to have the entire stack in order to support AES.

Thanks @nickovs

Just curious what the above means... is it not possible to just use (say) mbedtls' AES (or SHA) algorithms without linking in any other parts of the TLS stack.

I agree it's unfortunate to require the whole mbedtls submodule just so rp2 can have crypto (but this doesn't seem too much of a burden?).

@nickovs
Copy link
Contributor Author

nickovs commented Jul 8, 2021

@jimmo It may well be possible to alter the build system to pick and choose which bits of mbedtls get used, but there seems to be a fair amount of internal dependency. Coupled with the fact that right now neither mbedtls nor axTLS have been ported to run on the rp2 version of micropython I thought it would be helpful to have a stand-alone implementation.

@iabdalkader
Copy link
Contributor

Hello, your PR seems to be making changes to the submodules lib/mbedtls, lib/pico-sdk and lib/tinyusb, I don't think this is intentional.

@nickovs
Copy link
Contributor Author

nickovs commented Aug 12, 2021

@iabdalkader I think I have fixed the problem with the submodules. For some reason my master branch had its submodules out of sync with upstream.

@dpgeorge
Copy link
Member

Oh no, I'm very sorry @nickovs that I completely lost track of this PR! And then I went ahead and added ucryptolib support to rp2 using mbedtls, see 86e6744 ...

What you have here is nice and clean, a good addition for ports that don't have a full TLS library. Do you think this is still useful and worth adding?

It's actually pretty easy to include mbedtls in the build now (either Makefile based or CMakeLists.txt), although it is rather large, it added about 14k to the rp2 firmware to enable ucryptolib.

@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

6 participants