Cool, thanks! There are already a number of TLS improvements in the pipeline (#5819, #5825) and I've been meaning to add the cert verification, so this is perfect.
FYI, I believe Damien wants tests that make connections to internet servers to go into net_hosted.

In order to check expiration of certs on esp32 the following is required to be set:

CONFIG_MBEDTLS_HAVE_TIME_DATE=y

Defaults to being not set ...

There's a thread about this topic here
https://esp32.com/viewtopic.php?t=7585

https://docs.espressif.com/projects/esp-idf/en/stable/api-reference/kconfig.html#config-mbedtls-have-time-date

Yup, can't check the expiration date of certs without knowing today's date... Note #5973, which switches the RTC to have a linux epoch, which I assume is required for mbedtls to get the correct date... I'll look into the details...

Not that I have to say much in this, but to me it feels uncomfortable all checks being disabled by default.
Especially by passing server_hostname and/or ca_certs I would expect they either being honoured and connection fails if mismatch /or/ them not being accepted, as in, an Exception being thrown when passed as args but respective checks are not enabled.

Not that I have to say much in this, but to me it feels uncomfortable all checks being disabled by default.

I totally agree! And I'm planning on submitting PRs to fix that, but with 13 PRs in the pipeline I'm waiting for something to make progress otherwise the reworking of additional PRs as the prior ones get modified adds a lot of unnecessary work.

WRT to changing the default: what I would like to see is a way to address the root certs issue. I'm not familiar with how root cert stores work but ideally MP would ship with a small set of most common root certs in the flash/filesystem and the ability to add more. This way, you just say "use ssl" and it does what one would expect from a modern secure system.

Now I'm sure reality will step in and the grand vision won't be practical, but then I'd like to work out how close we can get.

what I would like to see is a way to address the root certs issue. I'm not familiar with how root cert stores work but ideally MP would ship with a small set of most common root certs in the flash/filesystem and the ability to add more. This way, you just say "use ssl" and it does what one would expect from a modern secure system.

From my perspective that could be an option in whatever way (compile option, its own package, etc.), but having it there by default I personally wouldn't like.

While it might be "nice" for the folks doing IoT cloud solutions, e.g. connecting to a cloud mqtt broker to not care about the TLS setup and its trust chain, I'm usually rolling out my own trust chain and would expect that the trust anchors I set are the only ones.

First thing I do when setting up a system for a dedicated purpose is kicking out all kinds of CAs from the system and the components shipping their own (Firefox at its best - you have to have them, best you can do is deactivation in a very cumbersome way).

What I'm trying to say is, that the current use of x509 and the so many root CAs systems and users trust automatically is broken by design (DigiNotar, Norton, ..) and I'd rather have my own CA system I can trust, than some defaults I per definition can at least not trust /as much/.

Haha, you're definitely at the paranoid end of the spectrum! Going that far is always an option. My goal is to make things reasonably secure by default, then someone who doesn't want security can turn it off (e.g. no cert validation), and you can replace the ca roots in your systems :-).

@mirko
I agree but "Insecure by default" is what the current implementation does (see the docs https://docs.micropython.org/en/latest/library/ussl.html?highlight=ussl#functions ) and also what cpython is doing https://docs.python.org/3/library/ssl.html#ssl.wrap_socket.

Cpython provides the concept of creating ssl-contexts which can be used to wrap sockets. In order to create a context with sensible defaults there is the function context = ssl.create_default_context().

I just realized that my current implementation moves away from cpython by providing custom flags to cert_reqs. Probably that is not a good idea...

I agree but "Insecure by default"

It's not necessarily the "insecure by default" (though I'm not a big fan of it either), but the "I actually provide and set SSL params like server_hostname and ca_certs but they're being completely ignored until I do /another/ thing.
That other thing, though, is not even obvious or intuitive, but I have to set a certain bit pattern to a (to me) non-descriptive kw named cert_reqs for enabling the functionality I already set.

Obviously (again, maybe only to me) if I set server_hostname I want subject verification and when setting ca_certs I want CA verification.
This, from my point of view, should be definitely ensured.
Or throw an exception in this case (mismatch e.g. server_hostname set, but cert_reqs bitmask hasn't respective flag), which you have to actively catch and ignore, so you can't that easily fall for this pitfall.

@mirko, I totally agree but I'm unsure about how to proceed as cpython's behavior of wrap_socket is exactly that terrifying...

@mirko, I totally agree but I'm unsure about how to proceed as cpython's behavior of wrap_socket is exactly that terrifying...

While this might be good reasoning in other or even most cases I'd definitely vote for this not being one of them.
If staying compatible with CPython has above discussed implications I'm definitely in favour of a sane and potentially incompatible API - especially if that means "not suggesting false security".

Hi, is there any update on this issue? I tried out the code as provided but by this pull but it seems not to work. In the code snippet below despite the fact I provide the correct certificate or one I modify to specifically make it raise an exception it prints out "Website verified".
This lead me to think there still exists an issue. Please correct me if I am wrong.

try:
s2 = ussl.wrap_socket(s, server_hostname=host, ca_certs=test_root_ca, cert_reqs=0xffffff)
print("Website verified")
except OSError:
raise Exception('This should not have raised an error!')
finally:
print('Socket closing!')
s2.close()

What's the test saying which comes with the PR?
For me - although slightly adjusted - this PR works.

@mirko, I am not sure I understand your question. All I did was try out the PR and the result did not raise a exception for a modified ca_cert. Basically any ca_cert used for a host printed out "Website verified". Please elaborate what the you did since it works for you.

@mirko, I am not sure I understand your question. All I did was try out the PR and the result did not raise a exception for a modified ca_cert. Basically any ca_cert used for a host printed out "Website verified". Please elaborate what the you did since it works for you.

Not quite sure what's unclear. What I'm asking is whether the test - which is part of this PR - does not what it is expected to do in your setup: https://github.com/micropython/micropython/blob/59fd31808ff5173258d2c235b9253859a6c462b6/tests/esp32/esp32_test_cert.py

Hi,
For me also, I checked out this PR and I ran the test with an invalid CA certificate.
I would expect that it raises an OSError but it doesn't. Even a certificate which is not correctly encoded (even not Base64) doesn't rais any OSError in the test.

Are there any additional steps to do before the certificate validation will work? Additional PRs?

That's the diff I use on top of the latest release: https://pb.nanl.de/show.php?id=45135&hash=15971716

@chtinnes or @kekimari could you provide a full snippet including the cert so I can test the behaviour?

here is what I get when providing a "faulty cert":

=== import socket
===
=== cert = """
=== -----BEGIN CERTIFICATE-----
=== asdffasdff asdfassdf
=== -----END CERTIFICATE-----
=== """
===
=== host = 'letsencrypt.org'
===
=== i = socket.getaddrinfo(host, 443)
===
=== s = socket.socket()
=== s.connect(i[0][-1])
=== try:
===     s2 = ussl.wrap_socket(s, server_hostname=host, cert_reqs=0xffffff, ca_certs=cert)
=== finally:
===     s.close()
Traceback (most recent call last):
  File "<stdin>", line 18, in <module>
ValueError: invalid cert

I've often found badssl.com useful for testing many various certificate verification scenarios on clients.

@pumelo Nevermind, I got it working now. I think I had somehow screwed up the firmware on my board before. It works fine now.

I am just wondering if it is possible now to send mqtt messages to gcp IoT from micropython firmware or this pr is the only obstacle to this? If so then my question is when this pr will be merged or something hampering the merge?

@dmitrypotenko
You might also get some memory issues for some MQTT brokers with the current firmware version as discussed in #5929.
Besides that (i.e., when using a not so greedy cipher) it works now fine for me. I would also be happy to see this in the master soon, since I consider SSL including certificate validation as a crucial for many use cases.

I am just wondering if it is possible now to send mqtt messages to gcp IoT from micropython firmware or this pr is the only obstacle to this? If so then my question is when this pr will be merged or something hampering the merge?

I know it's possible using Google's code, I've been doing it. What I wonder about is without verification, is it vulnerable to a MitM attack. I am unclear on that but I think it is. Here's Google's code.
https://medium.com/google-cloud/connecting-micropython-devices-to-google-cloud-iot-core-3680e632681e

@dmitrypotenko
You might also get some memory issues for some MQTT brokers with the current firmware version as discussed in #5929.
Besides that (i.e., when using a not so greedy cipher) it works now fine for me. I would also be happy to see this in the master soon, since I consider SSL including certificate validation as a crucial for many use cases.

What would a not so greedy cipher be, do you have example code you could share?

I got it working by the way on 1.15, an ESP32-WROVER-IE (has PSRAM), just hoping to avoid subtle gotchas--such as a greedy cipher. I did a diff against 1.15 and that code is slightly newer, something about error handling. Suppose I will need to diff patch both sets of changes into one new file, something which will have to wait for another day.

Yes, if you don't do CA verification you're vulnerable against MITM attacks (and don't pin to the (fingerprint of the) server cert).
Re memory:
Use ECs instead of RSA to save memory (negotiations are much faster as well), if you can curve25519, otherwise the NIST-P ones - e.g. primeXXXv1/secpXXXr1 - will do as well, assuming they're not backdoored.
Use DER (binary format) instead of PEM (though, doesn't work with cert hierarchies >1), kick out everything you don't need out of the mbedtls build (disable PEM support, disable RSA support, disable PKCS-support, disable server support, enable asymmectric content support and set small but sufficient buffer sizes, disable unused SSL/TLS proto versions, disable unused x509 extensions, disable unused ciphers and hash algos, disable CSR support, ..)

Is anyone going to work on closing this issue out ?
It looks like its been going for some time, and now has branch conflicts to be resolved.

Is anyone going to work on closing this issue out ? It looks like its been going for some time, and now has branch conflicts to be resolved.

If the original author (@pumelo) doesn't disagree / wanna do it him-/herself, I'm happy to clean up the PR a bit, re-submit and push for feedback and merge. @pumelo any stand on this?

@mirko of course, just go ahead. I'll be very happy if you would take this up and finish it.

I created now another pull request based on @pumelo's changes, the only major change from my side was to change the cert_reqs paramater to apply all checks by default.

Thanks for the contribution, and it's good that this has been picked up by others in #8252 and #8854. I'll close this in favour of those PRs.