SSL certificate verification #8854

daviesian · 2022-07-03T20:42:34Z

This PR is an improved and extended version of #8252, which adds support for the cert_reqs and ca_certs arguments to ssl.wrap_socket. PR #8252 only verifies the server certificate when doing mutual TLS, but this version verifies the server certificate whenever one is provided and cert_reqs is set appropriately. This is implemented in the first commit - ~~6dd27ee~~ 91d30da, relying heavily on #8252 (thanks @Carglglz!).

This PR also verifies certificate time validity on the RP2 port, making use of the RTC. If cert_reqs is set to ssl.CERT_REQUIRED, certificates will only be verified successfully if they are valid at the current time. This is implemented in the second commit - ~~e306e2a~~ 1664939.

Of course, this requires the RTC to be set correctly, so this PR also includes an implementation of the ntptime module, copied and modified from the ESP8266 port. This is implemented in the third commit - ~~b97a342~~ 4b4843f. Before making SSL connections, developers should call ntptime.settime() or set the RTC to the correct time by some other means.

I'm not expecting this PR to be directly merged as-is, but would welcome feedback on the wisdom (or otherwise) of the various choices I've proposed. Namely:

Moving ca_certs handling logic out one level, so it happens whenever the ca_certs argument is provided. @Carglglz - does this sound sensible?
Unconditionally adding MBEDTLS_BASE64_C and MBEDTLS_PEM_PARSE_C to the RP2 port, making it easy for developers to provide ca_certs.
Always verifying the valid/expired status of server certificates based on the RTC. With this implementation, there is no way to verify everything apart from the time on a certificate, which feels correct to me. A certificate is either valid or it isn't.

If this is broadly acceptable then I'd be happy to polish this PR into something suitable for merging.

codecov-commenter · 2022-07-03T21:01:24Z

Codecov Report

Merging #8854 (0f6bd96) into master (4cf9928) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8854      +/-   ##
==========================================
+ Coverage   98.34%   98.37%   +0.02%     
==========================================
  Files         156      156              
  Lines       20237    20281      +44     
==========================================
+ Hits        19902    19951      +49     
+ Misses        335      330       -5

Impacted Files	Coverage Δ
extmod/modussl_mbedtls.c	`86.06% <ø> (ø)`
py/map.c	`99.46% <0.00%> (-0.54%)`	⬇️
extmod/vfs_posix_file.c	`92.13% <0.00%> (-0.42%)`	⬇️
py/obj.c	`97.77% <0.00%> (-0.06%)`	⬇️
py/modio.c	`95.58% <0.00%> (ø)`
py/compile.c	`99.69% <0.00%> (ø)`
py/objtype.c	`100.00% <0.00%> (ø)`
py/objcomplex.c	`100.00% <0.00%> (ø)`
ports/unix/mpconfigport.h	`100.00% <0.00%> (ø)`
py/runtime.c	`99.71% <0.00%> (+<0.01%)`	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

iabdalkader · 2022-07-04T10:45:58Z

This is great, thanks for this! One comment though, I think ntptime.py should be removed from esp port and put in a common place (ex /extmod), this way it's easy to reuse this from stm32 and other ports.

daviesian · 2022-07-04T10:48:37Z

Great idea, I'll do that and update the PR!

daviesian · 2022-07-04T11:18:32Z

Actually, it's not quite that simple, because the different boards each use a different epoch (the RP2 uses 1970-1-1, the ESP8266 uses 2000-1-1). I could easily create a per-board module called something like system_epoch so ntptime could do from system_epoch import EPOCH, but that feels messy. @iabdalkader can you think of a cleaner solution?

iabdalkader · 2022-07-04T12:33:35Z

I'm not sure, maybe just define the epoch in time modules ? For example in

diff --git a/ports/rp2/modutime.c b/ports/rp2/modutime.c
index ac291c893..312381b14 100644
--- a/ports/rp2/modutime.c
+++ b/ports/rp2/modutime.c
@@ -117,6 +117,11 @@ STATIC const mp_rom_map_elem_t mp_module_time_globals_table[] = {
     { MP_ROM_QSTR(MP_QSTR_ticks_cpu), MP_ROM_PTR(&mp_utime_ticks_cpu_obj) },
     { MP_ROM_QSTR(MP_QSTR_ticks_add), MP_ROM_PTR(&mp_utime_ticks_add_obj) },
     { MP_ROM_QSTR(MP_QSTR_ticks_diff), MP_ROM_PTR(&mp_utime_ticks_diff_obj) },
+    #if MICROPY_EPOCH_IS_1970
+    { MP_ROM_QSTR(MP_QSTR_epoch), MP_ROM_INT(1970) },
+    #else
+    { MP_ROM_QSTR(MP_QSTR_epoch), MP_ROM_INT(2000) },
+    #endif
 };

Alternatively, could also just add an arg ntptime.time ?

diff --git a/ports/esp8266/modules/ntptime.py b/ports/esp8266/modules/ntptime.py
index dd07e46f1..cddc96a4d 100644
--- a/ports/esp8266/modules/ntptime.py
+++ b/ports/esp8266/modules/ntptime.py
@@ -14,7 +14,7 @@ NTP_DELTA = 3155673600
 host = "pool.ntp.org"

-def time():
+def time(epoch = 1970):
     NTP_QUERY = bytearray(48)
     NTP_QUERY[0] = 0x1B
     addr = socket.getaddrinfo(host, 123)[0][-1]
@@ -26,7 +26,7 @@ def time():
     finally:
         s.close()
     val = struct.unpack("!I", msg[40:44])[0]
-    return val - NTP_DELTA
+    return val - (NTP_DELTA if epoch = 1970 else NTP_DELTA_2000)

Carglglz · 2022-07-04T21:58:43Z

@daviesian Nice to see that this gets some attention 👍🏼

PR #8252 only verifies the server certificate when doing mutual TLS, but this version verifies the server certificate whenever one is provided and cert_reqs is set appropriately. This is implemented in the first commit - 6dd27ee,

Moving ca_certs handling logic out one level, so it happens whenever the ca_certs argument is provided. @Carglglz - does this sound sensible?

Yes probably is better to move it out one level, it makes sense, I don't know why I didn't that in the first place...

RTC check for expiration date is a nice addition as well as PEM format compatibility.
Adding this to other ports would be nice too. 👍🏼

IMO I would convert this to a draft and then split this in 2 or 3 PRs which may be easier to review/discuss/merge , one PR could be just rp2 port and the other esp/stm32/rest... 🤔

daviesian · 2022-07-06T16:06:01Z

Thanks @iabdalkader and @Carglglz for your comments and suggestions. I have now updated the branch to address the issue with the duplicated ntptime module in the cleanest way I could manage. You'll see that ntptime is now sourced from the extmod/ntptime_py directory, with an optional ntptime.epoch_override module that should be added in ports that have MICROPY_EPOCH_IS_1970 defined. You can see an example of that in ports/rp2/netmodules/ntptime/epoch_override.py.

I tried a bunch of different options for overriding the epoch in some ports, and this seemed the cleanest. I have added a README file in the common ntptime package in extmod, so future developers should find it fairly easily.

Recap

This PR is an improved version of #8252, which adds full TLS certificate verification to the RP2 port, and factors out the ntptime module for use in multiple ports. The checks are now passing, so I think this is good to go. Once we've agreed the right approach, I'll be happy to create additional PRs to enable this TLS validation functionality on the other ports, as suggested by @Carglglz.

@dpgeorge, are you happy with this?

dpgeorge · 2022-07-08T13:42:19Z

Yes this looks really good!

(See other previous attempt #4419.)

extmod/modussl_mbedtls.c

extmod/ntptime_py/ntptime/__init__.py

ports/esp8266/boards/GENERIC_512K/manifest.py

daviesian · 2022-07-08T20:43:47Z

@dpgeorge Thanks a lot for your very sensible suggestions, I've pushed an updated implementation. Is there anything further you'd like me to address?

extmod/ntptime.py

ports/esp8266/boards/GENERIC_512K/manifest.py

extmod/modussl_mbedtls.c

dpgeorge · 2022-07-12T01:29:14Z

ports/rp2/mbedtls/mbedtls_config.h

@@ -94,6 +94,8 @@
 #define MBEDTLS_X509_CRT_PARSE_C
 #define MBEDTLS_X509_USE_C

+#define MBEDTLS_BASE64_C
+#define MBEDTLS_PEM_PARSE_C


please split this change into the rp2 commit (so that there's on commit that just touches extmod/modussl_mbedtls.c)

Thanks.

But another point of discussion is whether enabling PEM parsing is a good idea. If it's enabled here then it sets a precedence that other ports should follow, otherwise writing portable code is hard (eg using a PEM in code on rp2 won't then work on an stm32 board).

PEM parsing adds a lot of unnecessary overhead (code size and RAM and time), it's much simpler to use DER format. PEM is essentially base-64 encoded DER with header/footer data. So to parse PEM means allocating a chunk of memory to hold the DER and then base-64 decoding the PEM into that buffer, and then using the DER (eg parsing it).

I think for MicroPython we should standardise on using (and documenting how to use) DER format only. I'd be interested to hear your thoughts on this.

In order not to hold up this PR any longer, would be good not enable PEM here, and instead discuss it more widely.

@daviesian any thoughts on this point? My suggestion is to not add these 2 lines in this PR.

@daviesian ping

Sorry for missing this! You're quite right that this doesn't really belong here, although I do think I would be in favour of including it by default. Removed!

FYI @dpgeorge if cadata is in DER format it can contain only one certificate, so in other words without PEM parsing enabled (and PEM data) this function can't load a cert chain (probably should use mbedtls_x509_crt_parse_der instead and load one DER at a time).

* For certificates in PEM encoding, this may be a concatenation * of multiple certificates; for DER encoding, the buffer must * comprise exactly one certificate. * int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, size_t buflen );

dpgeorge · 2022-07-12T01:34:42Z

I'm inclined to merge #8252 which does implement the core ca_certs and cert_reqs arguments to wrap_socket(). Because that PR was first and to give attribution to @Carglglz . There's also #5998 but #8252 is an improved version of #5998.

This PR could then contribute 1) ntptime generalisation; 2) rp2 improvements.

daviesian · 2022-07-12T08:34:03Z

That sounds like an excellent plan - I would have based this PR off that one if PRs-on-PRs didn't make my head explode. I'll watch for that one being merged, then rebase this one. Thanks!

dpgeorge · 2022-07-20T06:47:57Z

@daviesian can you please rebase on master now that cert_reqs and cadata are implemented?

daviesian · 2022-07-21T20:28:53Z

@dpgeorge Sure thing, this is now rebased on master. Just shout if you'd like any further tweaks, otherwise I think this is ready to merge.

iabdalkader · 2022-07-23T07:13:08Z

Can these changes be propagated to other ports, especially the stm32 ? It will save more PRs and reviews later.

daviesian · 2022-07-23T09:10:09Z

That would be great, but I don't have knowledge of, or access to, any other hardware. Once this is merged, it should be as simple as implementing the time hook in this commit for other ports, so the PR/review process should be pretty minimal for those.

daviesian · 2022-08-03T21:16:02Z

@dpgeorge Is there anything else needed here before merging? It would be great to get this into master!

dpgeorge · 2022-08-04T01:26:59Z

Is there anything else needed here before merging?

See inline comment above.

The ntptime module was previously only included in the ESP8266 port. This commit factors that module out into the extmod directory.

daviesian · 2022-08-04T08:32:46Z

Sorry for missing the inline comment - I clearly need to improve my Github Notification settings! Anyway, I've now removed those unnecessary #defines.

dpgeorge · 2022-08-05T14:14:26Z

Sorry for missing the inline comment

Not a problem. GitHub has a lot of notifications, and it's easy to miss them.

But thanks for updating. Rebased and merged in b560b9f through 1bf2fd0

* This is a reimplementation of micropython#8854 for the stm32 port.

…n-main Translations update from Hosted Weblate

daviesian changed the title ~~Ssl certificate verification~~ SSL certificate verification Jul 3, 2022

dpgeorge added extmod Relates to extmod/ directory in source port-rp2 labels Jul 5, 2022

daviesian force-pushed the ssl-certificate-verification branch 6 times, most recently from b952c18 to 8ebd8a8 Compare July 6, 2022 15:53

dpgeorge mentioned this pull request Jul 8, 2022

esp32: Add ca_cert to ussl #8874

Closed

dpgeorge reviewed Jul 8, 2022

View reviewed changes

extmod/modussl_mbedtls.c Show resolved Hide resolved

extmod/modussl_mbedtls.c Outdated Show resolved Hide resolved

extmod/ntptime_py/ntptime/__init__.py Outdated Show resolved Hide resolved

ports/esp8266/boards/GENERIC_512K/manifest.py Show resolved Hide resolved

daviesian force-pushed the ssl-certificate-verification branch 2 times, most recently from 9805565 to 4b4843f Compare July 8, 2022 20:34

daviesian requested a review from dpgeorge July 11, 2022 09:33

dpgeorge reviewed Jul 12, 2022

View reviewed changes

extmod/ntptime.py Outdated Show resolved Hide resolved

ports/esp8266/boards/GENERIC_512K/manifest.py Show resolved Hide resolved

extmod/modussl_mbedtls.c Show resolved Hide resolved

dpgeorge reviewed Jul 12, 2022

View reviewed changes

This was referenced Jul 12, 2022

extmod/modussl_mbedtls.c: Add missing args and constants to ssl module. #8252

Closed

esp32: Add ca_cert to ussl #5998

Closed

daviesian force-pushed the ssl-certificate-verification branch from 4b4843f to 46de59e Compare July 12, 2022 10:31

dpgeorge mentioned this pull request Jul 16, 2022

RFC: change ussl to implement SSLContext #8915

Closed

lurch mentioned this pull request Jul 20, 2022

Clarify HTTPS note in "Connecting to the Internet with Raspberry Pi Pico W" raspberrypi/pico-feedback#264

Open

daviesian force-pushed the ssl-certificate-verification branch 3 times, most recently from 741c248 to 15021cc Compare July 21, 2022 19:52

daviesian added 3 commits August 4, 2022 09:28

rp2/mbedtls: Enable certificate validity time validation.

882a00d

extmod/ntptime.py: Factor out ntptime module.

5c13fbc

The ntptime module was previously only included in the ESP8266 port. This commit factors that module out into the extmod directory.

extmod/modussl_mbedtls.c: More sensible default debug log level.

0f6bd96

daviesian force-pushed the ssl-certificate-verification branch from 54feed2 to 0f6bd96 Compare August 4, 2022 08:28

dpgeorge closed this Aug 5, 2022

iabdalkader added a commit to iabdalkader/micropython that referenced this pull request Oct 13, 2022

ports/stm32: Enable mbedtls certificate time validation.

2e2de54

* This is a reimplementation of micropython#8854 for the stm32 port.

iabdalkader added a commit to iabdalkader/micropython that referenced this pull request Oct 17, 2022

ports/stm32: Enable mbedtls certificate time validation.

1c276a1

* This is a reimplementation of micropython#8854 for the stm32 port.

iabdalkader added a commit to iabdalkader/micropython that referenced this pull request Oct 17, 2022

ports/stm32: Enable mbedtls certificate time validation.

4d4c3a4

* This is a reimplementation of micropython#8854 for the stm32 port.

iabdalkader added a commit to iabdalkader/micropython that referenced this pull request Oct 17, 2022

ports/stm32: Enable mbedtls certificate time validation.

c4740b2

* This is a reimplementation of micropython#8854 for the stm32 port.

iabdalkader mentioned this pull request Oct 17, 2022

Mbedtls certificate time validation and board-config fragments. #9658

Closed

iabdalkader added a commit to iabdalkader/micropython that referenced this pull request Oct 17, 2022

ports/stm32: Enable mbedtls certificate time validation.

00007ff

* This is a reimplementation of micropython#8854 for the stm32 port.

iabdalkader added a commit to iabdalkader/micropython that referenced this pull request Oct 25, 2022

ports/stm32: Enable mbedtls certificate time validation.

d337a9d

* This is a reimplementation of micropython#8854 for the stm32 port.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 30, 2024

Merge pull request micropython#8854 from weblate/weblate-circuitpytho…

677c194

…n-main Translations update from Hosted Weblate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSL certificate verification #8854

SSL certificate verification #8854

daviesian commented Jul 3, 2022 •

edited

Loading

codecov-commenter commented Jul 3, 2022 •

edited

Loading

iabdalkader commented Jul 4, 2022 •

edited

Loading

daviesian commented Jul 4, 2022

daviesian commented Jul 4, 2022

iabdalkader commented Jul 4, 2022 •

edited

Loading

Carglglz commented Jul 4, 2022

daviesian commented Jul 6, 2022

dpgeorge commented Jul 8, 2022

daviesian commented Jul 8, 2022 •

edited

Loading

dpgeorge Jul 12, 2022

daviesian Jul 21, 2022

dpgeorge Jul 22, 2022

dpgeorge Jul 26, 2022

dpgeorge Aug 4, 2022

daviesian Aug 4, 2022

iabdalkader Oct 14, 2022

dpgeorge commented Jul 12, 2022

daviesian commented Jul 12, 2022

dpgeorge commented Jul 20, 2022

daviesian commented Jul 21, 2022

iabdalkader commented Jul 23, 2022

daviesian commented Jul 23, 2022

daviesian commented Aug 3, 2022

dpgeorge commented Aug 4, 2022

daviesian commented Aug 4, 2022

dpgeorge commented Aug 5, 2022

SSL certificate verification #8854

SSL certificate verification #8854

Conversation

daviesian commented Jul 3, 2022 • edited Loading

codecov-commenter commented Jul 3, 2022 • edited Loading

Codecov Report

iabdalkader commented Jul 4, 2022 • edited Loading

daviesian commented Jul 4, 2022

daviesian commented Jul 4, 2022

iabdalkader commented Jul 4, 2022 • edited Loading

Carglglz commented Jul 4, 2022

daviesian commented Jul 6, 2022

dpgeorge commented Jul 8, 2022

daviesian commented Jul 8, 2022 • edited Loading

dpgeorge Jul 12, 2022

Choose a reason for hiding this comment

daviesian Jul 21, 2022

Choose a reason for hiding this comment

dpgeorge Jul 22, 2022

Choose a reason for hiding this comment

dpgeorge Jul 26, 2022

Choose a reason for hiding this comment

dpgeorge Aug 4, 2022

Choose a reason for hiding this comment

daviesian Aug 4, 2022

Choose a reason for hiding this comment

iabdalkader Oct 14, 2022

Choose a reason for hiding this comment

dpgeorge commented Jul 12, 2022

daviesian commented Jul 12, 2022

dpgeorge commented Jul 20, 2022

daviesian commented Jul 21, 2022

iabdalkader commented Jul 23, 2022

daviesian commented Jul 23, 2022

daviesian commented Aug 3, 2022

dpgeorge commented Aug 4, 2022

daviesian commented Aug 4, 2022

dpgeorge commented Aug 5, 2022

daviesian commented Jul 3, 2022 •

edited

Loading

codecov-commenter commented Jul 3, 2022 •

edited

Loading

iabdalkader commented Jul 4, 2022 •

edited

Loading

iabdalkader commented Jul 4, 2022 •

edited

Loading

daviesian commented Jul 8, 2022 •

edited

Loading