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

extmod/modussl_mbedtls: Wire in support for DTLS #10062

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

damz
Copy link
Contributor

@damz damz commented Nov 23, 2022

What is this PR trying to solve?

This PR enables support for DTLS (i.e. TLS over datagram transport protocols like UDP). While support for DTLS is absent in cpython, it is worth supporting it in micropython because it is the basis of the ubiquitous CoAP protocol, used in many IOT projects. See #5270

How does this PR solve the problem?

A new set of "protocols" are added to SSLContext:

  • ssl.PROTOCOL_DTLS_CLIENT
  • ssl.PROTOCOL_DTLS_SERVER

They act as you expect. If one of these is set, the library assumes that the underlying socket is a datagram-like socket (i.e. UDP or similar).

Implement our own timer callbacks as the out of the box implementation relies on gettimeofday().

TODO: To fully support asyncio for DTLS socket, we will need to return a readable or writable event in poll(MP_STREAM_POLL, ...) if _mbedtls_timing_get_delay(self) >= 1. This is left for future work so as not to interfere with #9871.

@damz
Copy link
Contributor Author

damz commented Nov 23, 2022

Note that I only enabled it on the esp32 port so far. The size increase of the micropython.elf file on that port is 8.68 kB. Do we think it is reasonable to enable on all mbedtls-based ports?

@damz damz force-pushed the pr/dtls branch 2 times, most recently from fa2ad97 to ba0772f Compare November 25, 2022 15:49
@@ -187,7 +230,7 @@ STATIC mp_obj_ssl_socket_t *socket_new(mp_obj_t sock, struct ssl_args *args) {

ret = mbedtls_ssl_config_defaults(&o->conf,
args->server_side.u_bool ? MBEDTLS_SSL_IS_SERVER : MBEDTLS_SSL_IS_CLIENT,
MBEDTLS_SSL_TRANSPORT_STREAM,
args->dtls.u_bool ? MBEDTLS_SSL_TRANSPORT_DATAGRAM : MBEDTLS_SSL_TRANSPORT_STREAM,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to check the underlying socket type (SOCK_DGRAM) instead of adding a new variable for dtls?

Choose a reason for hiding this comment

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

Hey @stigbjorlykke after looking into this a bit, I don't believe that it is possible. DTLS lives on top of UDP, so the SOCK_DGRAM type doesn't have that context needed.

@keenanjohnson
Copy link

keenanjohnson commented Apr 19, 2023

Hey all! I would really like to see this change go through and I'm happy to help jump in and push this through. @stigbjorlykke is your comment a requirement before we merge this or is it a suggestion we are ok with accepting for now?

Are there any other changes needed by the reviewers?

@stigbjorlykke
Copy link
Contributor

@stigbjorlykke is your comment a requirement before we merge this or is it a suggestion we are ok with accepting for now?

This is not a requirement, it's a suggestion for a better API. The socket already know it's type, and adding an extra (superfluous) variable makes it easier to write faulty code. I have no idea how easy it is to check this, which is why I ask.

@keenanjohnson
Copy link

I've looked into the comment and I don't believe that the socket does know it's DTLS type, so I believe the extra variable is required.

Are you looking at an additional set of the defined SOCK_DGRAM types @stigbjorlykke?

Any help to get this merged as soon as possible would really help out our project :)

Thanks!

@mynameisvasco
Copy link

Any update on this?

Copy link

codecov bot commented Jan 7, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (42eab32) 98.36% compared to head (ddb064f) 98.33%.

Files Patch % Lines
extmod/modssl_mbedtls.c 63.15% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10062      +/-   ##
==========================================
- Coverage   98.36%   98.33%   -0.04%     
==========================================
  Files         159      159              
  Lines       21088    21106      +18     
==========================================
+ Hits        20743    20754      +11     
- Misses        345      352       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jan 7, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +8720 +1.079% standard[incl +192(data)]
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@damz damz force-pushed the pr/dtls branch 3 times, most recently from b81b204 to 69aefcf Compare January 7, 2024 04:43
Signed-off-by: Damien Tournoud <damien@platform.sh>
@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