CID update to RFC 9146

[hannestschofenig]

Description

The DTLS 1.2 CID specification is finally close to publication and will be released as RFC 9146. This PR updates the implementation to match the RFC content.

Here is the CID RFC in the RFC Editor Queue: https://www.rfc-editor.org/authors/rfc9146.html

There are now two compile-time options in this PR, namely

• MBEDTLS_SSL_DTLS_CONNECTION_ID_LEGACY (for the CID draft version draft-ietf-tls-dtls-connection-id-05), and
• MBEDTLS_SSL_DTLS_CONNECTION_ID (for the RFC 9146 version).
  These two options can only be used mutuallly exclusive.

Status

IN DEVELOPMENT

Requires Backporting

This PR does not require backporting.

Migrations

There is NO API change.

Additional comments

IMPORTANT: For those who used the temporary CID implementation in Mbed TLS please note that there were late changes to the specification that are backwards-compatibility-breaking. Two aspects have changed:

• IANA assigned a different value to the CID extension. The Mbed TLS implementation so far used an experimental value.
• The MAC computation and the additional data computation changed.

Todos

• [] Tests
• Documentation
• Changelog updated

Steps to test or reproduce

I have tested ssl_client2 against Californium.

[hannestschofenig]

The ssl_suite needs to be updated to cover the modified additional data calculation defined in RFC 9146.

[mpg]

Thanks @hannestschofenig for this PR and @boaks for your review!

Unfortunately, I don't have time to actually review the PR now, but I wanted to point out something: there are people with large-scale deployments of the experimental version. For them, a flag day where all clients and servers have to be updated at the same time is probably not an option. So I think we probably want to keep supporting the draft version that we had for some time on the server side.

I haven't fully thought out the design, but I think it could look like, MBEDTLS_SSL_DTLS_CONNECTION_ID (enabled by default) gives the RFC version, and a new option MBEDTLS_SSL_DTLS_CONNECTION_ID_DRAFT_COMPAT (disabled by default) controls support for the current draft version in addition to the RFC version. When receiving a ClientHello, we can treat them as separate extensions: the draft-compat one is 254 and the RFC 54, so the serverknows whether it's dealing with a legacy client and can treat records accordingly. That is, with a compat-enabled server there would be three possible states: no connection ID, draft-compat CID, and standard CID, and as many code paths.

That way, the migration path for existing deployments is much more realistic:

1. Upgrade all servers, enabling the compat option.
2. Gradually upgrade the clients.
3. When all clients are upgraded, disable the compat option on the server.

Wdyt? I know this adds complexity, sorry, but I'm afraid it's complexity we need.

[boaks]

@mpg

That's the way, how Eclipse/Californium supports migration and backwards compatibility.

The code-point 254 was never draft nor IANA nor anything else then just a random number.
In my opinion MBEDTLS_SSL_DTLS_CONNECTION_ID_LEGACY would match therefore better.
(For Eclipse/Californium I use the term DTLS_CONNECTION_ID_DEPRECATED, but mainly for supporting the code point 53 (IANA) with the old MAC definition.)

[mpg]

Good point, we shouldn't put DRAFT in the name of the legacy/compat option, as the code point we're using was never in the draft or anything.

Good to know you used the same strategy to support migration!

[boaks]

Let me add:
many people in the lwm2m and coap world are waiting for this feature, some of them for the feature compatible to (my) Eclipse/Californium.
With that, I see an additional choice:

• one definition for the RFC
• one definition for the legacy mode
• one definition to support both

FMPOV, the first two should be possible without investing too much time. That helps people, who want to make their experience with DTLS CID. The third one requires then more work, but will not hold up others from using this feature. I'm not sure, even the "people with large-scale deployments of the experimental version" may setup a second endpoint an migrate the devices to that with a firmware-update. But sure, that depends more on what they consider to be a solution.

[mpg]

@hannestschofenig There are a couple CI failures. Unfortunately the full results are not public yet, but the Travis part is. I suggest you first fix the issues found by Travis and then if there are extra failures and you don't have access to the Jenkins logs, feel free to ask and I'll post the relevant parts of the logs.

Can you fix those issues and implement the suggested option for facilitating migration? Thanks!

[mpg]

PS: we also require commit messages to include a Signed-off-by: line as documented in CONTRIBUTING.md.

[boaks]

@mpg

On the one side you're too short in time to do a review this (rather small) PR.
On the other side you request a PR with "legacy migration support", which affects additionally more code and will therefore be larger.

Wouldn't it be much better to focus in a first step on a "RFC 9146" and "legacy" mode by compile time switches?

That was mainly my proposal in order to balance the time requirements in developing and reviewing.
The later step, to offer "legacy migration" will come with additional changes, and makes the threshold to get it merged, higher.

(In Eclipse/Californium I did it also exactly in that way. First, just add the new implementation, compliant to RFC9146, then, in a second step, at the "deprecated" support.)

FMPOV, systems, which are already deployed and using the old or legacy definitions don't benefit that much from migrating to the new definitions. It provides mainly "cryptographic hygiene", but when those systems consider to migrate is more left to those systems and I guess, it will be considered to be rolled out only with other major updates, when they had enough time, to test such a major update in deep.
But new systems and new deployments will benefit from using the "final" definition, because that eliminates the need to migrate later.

(The update in the RFC mainly addressed an attack (malicious cid injection in the server hello), which is in my opinion, usually discovered by the handshake MAC in the Finish, even if the records MAC may succeed. See long discussion in the IETF TLS mailing list .)

[mpg]

@boaks Good point! Sorry, I'm afraid I missed your proposal the first time, thanks for mentioning it again. That is indeed a better option enabling more incremental progress.

So, in terms of compile time options one could still have just two of them: one to enable the current behaviour, and one to enable the RFC 9146 implementation. As a first step, those two options would be mutually exclusive (enforced in check_config.h) so that you'd have to choose one or the other (or none) at compile time. This step is quite simple and leads to a releasable state.

Then, as a second step (and perhaps only if there's enough demand), we can add the ability to enable both options at compile-time and negotiate them as part of the handshake server-side. But we don't need that upfront indeed.

@hannestschofenig I withdraw my request for dynamic negotiation, but please keep the old behaviour as a compile-time alternative, as removing it entirely is not acceptable at this point.

[hannestschofenig]

@plskeggs I rebased the code to the development branch. Thanks for pointing this out.

[boaks]

@mpg
@daverodgman

FMPOV, this PR is "LGTM".

You both have requested changes, so would it be possible for you to spend some time in review the changes?
It's 2 weeks ago, that Hannes updated it towards the "development-branch". I'm afraid, it will be a endless loop, if reviews takes longer.

[boaks]

@hannestschofenig

I hope the reviews will be done soon. Afterwards I would propose, that the commits are squashed and you add the missing "Signed-off-by: Author Name authoremail@example.com" to the resulting commit.

Or are there any reasons, not to squash after the reviews?

[gilles-peskine-arm]

@boaks What's reviewed is what we merge, including the commit history. We want the commit history to accurately reflect the development and the review, and we want it to be helpful to readers (one thing at a time), so we never squash, and we avoid rebasing during review unless there's a compelling reason (e.g. merge conflict, or commit that doesn't make sense on its own such as forgetting to add a new file).

[gilles-peskine-arm]

The DCO check is failing. Please add signoff lines to all commit messages before we can (re)start reviewing.

[boaks]

What's reviewed is what we merge ...

For work, which is done over such a long period, this will hardly work. Nor do I see, that the most are interested in the history of the withdrawn stuff. At least I prefer to focus on the final outcome and the proper documentation of that.

Anyway, this is not my PR, so proceed as you prefer.

[plskeggs]

@hannestschofenig -- is it safe to assume that this PR is superseded by #6264?

[hannestschofenig]

I created the new PR to deal with the Signed-off-by issue. The new PR also doesn't carry the entire commit history because there was a lot of "forwards-and-backwards" in the design.

[boaks]

@hannestschofenig

Thanks a lot for all the work an the big step ahead now!

[mpg]

Closing, superseded by #6264