CID update to RFC 9146 #6264

hannestschofenig · 2022-09-07T15:30:50Z

The DTLS 1.2 CID specification has been published as RFC 9146. This PR updates the implementation to match the RFC content.

Signed-off-by: Hannes Tschofenig hannes.tschofenig@arm.com

Description

The DTLS 1.2 CID specification was published as RFC 9146. This PR updates the implementation to match the RFC content.

There are two compile-time options, namely

Set MBEDTLS_SSL_DTLS_CONNECTION_ID to enable the connection id functionality.
Set MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT to 0 (for the RFC 9146 version) and 1 (for the CID draft version draft-ietf-tls-dtls-connection-id-05).

Status

READY

Requires Backporting

NO. This PR does not require backporting.

Migrations

NO.

There are no changes to the API but enhancements to the API.

Additional comments

Todos

Tests
Documentation
Changelog forgotten here, fixed in Add ChangeLog entry for DTLS Connection ID #6681
Backports not needed

Steps to test or reproduce

The code has been tested against Californium.

The DTLS 1.2 CID specification has been published as RFC 9146. This PR updates the implementation to match the RFC content. Signed-off-by: Hannes Tschofenig <hannes.tschofenig@arm.com>

daverodgman · 2022-09-12T11:12:34Z

Looks like there is a compile issue with TLS 1.3-only configurations

The DTLS 1.2 CID specification has been published as RFC 9146. This PR updates the implementation to match the RFC content. Upstream PR: Mbed-TLS/mbedtls#6264 Signed-off-by: Hannes Tschofenig <hannes.tschofenig@arm.com>

hannestschofenig · 2022-09-21T10:32:48Z

The CI rightly points out that the current Mbed TLS implementation only supports TLS 1.3 and not DTLS 1.3. Hence, if you enable only TLS 1.3 support then this feature is not applicable at this point in time.

carlescufi · 2022-09-21T11:23:34Z

@hannestschofenig does this PR superseed #5061? If so, why not close the older one?

The DTLS 1.2 CID specification has been published as RFC 9146. This PR updates the implementation to match the RFC content. Upstream PR: Mbed-TLS/mbedtls#6264 Signed-off-by: Hannes Tschofenig <hannes.tschofenig@arm.com>

Ensure MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT is unset where MBEDTLS_SSL_DTLS_CONNECTION_ID is unset. Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

boaks · 2022-10-27T07:02:28Z

I updated my local mbedtls to this PR in order to run Eclipse/Californium - mbedtls interoperability tests.

Tests are successful.

Thanks @hannestschofenig !

Ensure MBEDTLS_SSL_DTLS_CONNECTION_ID and MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT are unset when MBEDTLS_SSL_PROTO_DTLS is not set in tls13-only tests. Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

We don't need to have two copies of the test with one of them depending on legacy/compat CID: we can have just one copy, but make sure we run ssl-opt.sh both in a build with standard CID and in a build with legacy/compat - that's the job of all.sh (see next commit). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

CID is now enabled in the default config (as well as full), so it's already tested in numerous all.sh components, not need to add one for that. We need a component for the legacy/compat option though as it's never enabled in existing components. So, keep that one, but adjust the name and fix a typo in a message. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

boaks · 2022-11-25T10:53:29Z

@thomas-fossati

Hi Thomas,

If I remember well, you're the expert for the DTLS 1.2 CID / CBC support.
Would it be possible for you to have a look on it? that would be great and helps a lot.

boaks · 2022-11-25T10:55:18Z

we don't want to waste your time testing versions that don't even pass our own CI.

That's only a short test, so the time spend doesn't matter.

mpg · 2022-11-25T11:00:53Z

I just pushed an updated version that should fix the CI failure. At least the component that was failing on the CI is not passing locally on my machine, hopefully I haven't broken anything else in the process.

I've also force-pushed to edit the commit message of the last 6 commits from Hannes and make the DCO check happy.

The previous HEAD before I force-pushed was 13fe72c - I've edited the messages from the last 6 commits without changing their content, and added 3 new commits on top: 1 to fix the failure, and 2 to address my own feedback about redundant tests. @d3zd3z @hannestschofenig please review!

Feedback has been addressed.

boaks · 2022-11-25T11:14:20Z

With commit 6a543ba

dtls_1_2_cid_cbc.pcapng.gz

it works from my side. My tests are not intensive, they mainly ensure a basic interoperability.

Tested ciphersuites:

"TLS-ECDHE-PSK-WITH-AES-128-CBC-SHA256"
"TLS-PSK-WITH-AES-128-GCM-SHA256"
"TLS-PSK-WITH-AES-256-GCM-SHA384"
"TLS-PSK-WITH-AES-128-CCM-8"
"TLS-PSK-WITH-AES-256-CCM-8"
"TLS-PSK-WITH-AES-128-CCM"
"TLS-PSK-WITH-AES-256-CCM"
"TLS-PSK-WITH-AES-128-CBC-SHA256"
"TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256"
"TLS-ECDHE-ECDSA-WITH-AES-256-GCM-SHA384"
"TLS-ECDHE-ECDSA-WITH-AES-128-CCM-8"
"TLS-ECDHE-ECDSA-WITH-AES-256-CCM-8"
"TLS-ECDHE-ECDSA-WITH-AES-128-CCM"
"TLS-ECDHE-ECDSA-WITH-AES-256-CCM"
"TLS-ECDHE-ECDSA-WITH-AES-256-CBC-SHA"
"TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256"
"TLS-ECDHE-ECDSA-WITH-AES-256-CBC-SHA384"
"TLS-ECDHE-RSA-WITH-AES-128-GCM-SHA256"
"TLS-ECDHE-RSA-WITH-AES-256-GCM-SHA384"
"TLS-ECDHE-RSA-WITH-AES-128-CBC-SHA256"
"TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA"
"TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384"

mpg · 2022-11-25T11:19:31Z

Thanks! Basic interop is already great!

mpg · 2022-11-25T11:27:40Z

I've tested interop of the compat version with previous versions of Mbed TLS. Steps taken:

In a checkout of the development branch, enable MBEDTLS_SSL_DTLS_CONNECTION_ID, build ssl_client2 and ssl_server2 and save them somewhere.
In a checkout of this branch, build in the defaults config, test against the programs built at step 1, and observe numerous failures.
In a checkout of this branch, set MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT to 1, rebuild (with make, you need to run make clean first), test against the programs from step 1 again, and now the tests pass.

Testing against programs from test 1 consisted of:

P_SRV=/path/to/step1/ssl_server2 tests/ssl-opt.sh -f 'Connection ID' 
P_CLI=/path/to/step1/ssl_client2 tests/ssl-opt.sh -f 'Connection ID'

mpg · 2022-11-28T08:47:52Z

CI's good. @hannestschofenig @d3zd3z Please review the latest commits.

d3zd3z

Looks good. I wonder if MMBEDTLS is the yummier version.

mpg · 2022-11-29T08:43:24Z

Aw, we managed to forget the ChangeLog entry. See #6681

The DTLS 1.2 CID specification has been published as RFC 9146. This PR updates the implementation to match the RFC content. Upstream PR: Mbed-TLS/mbedtls#6264 Signed-off-by: Hannes Tschofenig <hannes.tschofenig@arm.com>

Mbed-TLS#6264 was missing a changelog entry. Unfortunately we didn't catch it when preparing the 3.3.0 release. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

CID update to RFC 9146

fd6cca4

The DTLS 1.2 CID specification has been published as RFC 9146. This PR updates the implementation to match the RFC content. Signed-off-by: Hannes Tschofenig <hannes.tschofenig@arm.com>

davidhorstmann-arm added the component-tls label Sep 8, 2022

daverodgman added enhancement needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Sep 12, 2022

daverodgman added this to To Do in Roadmap Board for Mbed TLS via automation Sep 12, 2022

daverodgman added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 12, 2022

plskeggs mentioned this pull request Sep 13, 2022

CID update to RFC 9146 #5061

Closed

2 tasks

daverodgman added this to Mbed TLS 3.3 / 2.28.2 in EPICs for Mbed TLS Oct 3, 2022

daverodgman added priority-high High priority - will be reviewed soon and removed priority-medium Medium priority - this can be reviewed as time permits labels Oct 3, 2022

mpg self-requested a review October 6, 2022 08:14

daverodgman requested a review from d3zd3z October 10, 2022 12:35

boaks mentioned this pull request Oct 12, 2022

DTLS connection ID zephyrproject-rtos/zephyr#23424

Closed

Fix all.sh dependency on DTLS connection ID

1df7070

Ensure MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT is unset where MBEDTLS_SSL_DTLS_CONNECTION_ID is unset. Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-work labels Oct 26, 2022

Fix test dependency on DTLS connection ID

4e0fca3

Ensure MBEDTLS_SSL_DTLS_CONNECTION_ID and MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT are unset when MBEDTLS_SSL_PROTO_DTLS is not set in tls13-only tests. Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman removed the needs-ci Needs to pass CI tests label Oct 28, 2022

mpg added 2 commits November 25, 2022 11:48

mpg dismissed d3zd3z’s stale review via 6a543ba November 25, 2022 10:49

mpg force-pushed the rfc9146_2 branch from 13fe72c to 6a543ba Compare November 25, 2022 10:49

Roadmap Board for Mbed TLS automation moved this from Has Approval to In Development Nov 25, 2022

mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Nov 25, 2022

d3zd3z approved these changes Nov 28, 2022

View reviewed changes

bensze01 moved this from In Development to Has Approval in Roadmap Board for Mbed TLS Nov 28, 2022

mpg approved these changes Nov 29, 2022

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 29, 2022

mpg merged commit ffc330f into Mbed-TLS:development Nov 29, 2022

Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Nov 29, 2022

mpg mentioned this pull request Nov 29, 2022

Add ChangeLog entry for DTLS Connection ID #6681

Merged

boaks mentioned this pull request Nov 29, 2022

net: sockets: tls: Implement DTLS Connection ID socket option zephyrproject-rtos/zephyr#36738

Closed

daverodgman removed this from Done in Roadmap Board for Mbed TLS Apr 2, 2023

daverodgman mentioned this pull request Aug 30, 2023

Update DTLS CID implementation to comply with the "final" draft. #4860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CID update to RFC 9146 #6264

CID update to RFC 9146 #6264

hannestschofenig commented Sep 7, 2022 •

edited by mpg

Loading

daverodgman commented Sep 12, 2022

hannestschofenig commented Sep 21, 2022

carlescufi commented Sep 21, 2022

boaks commented Oct 27, 2022

boaks commented Nov 25, 2022

boaks commented Nov 25, 2022

mpg commented Nov 25, 2022

boaks commented Nov 25, 2022 •

edited

Loading

mpg commented Nov 25, 2022

mpg commented Nov 25, 2022

mpg commented Nov 28, 2022

d3zd3z left a comment

mpg commented Nov 29, 2022

CID update to RFC 9146 #6264

CID update to RFC 9146 #6264

Conversation

hannestschofenig commented Sep 7, 2022 • edited by mpg Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

daverodgman commented Sep 12, 2022

hannestschofenig commented Sep 21, 2022

carlescufi commented Sep 21, 2022

boaks commented Oct 27, 2022

boaks commented Nov 25, 2022

boaks commented Nov 25, 2022

mpg commented Nov 25, 2022

boaks commented Nov 25, 2022 • edited Loading

mpg commented Nov 25, 2022

mpg commented Nov 25, 2022

mpg commented Nov 28, 2022

d3zd3z left a comment

Choose a reason for hiding this comment

mpg commented Nov 29, 2022

hannestschofenig commented Sep 7, 2022 •

edited by mpg

Loading

boaks commented Nov 25, 2022 •

edited

Loading