-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
CID update to RFC 9146 #5061
CID update to RFC 9146 #5061
Conversation
The ssl_suite needs to be updated to cover the modified additional data calculation defined in RFC 9146. |
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, That way, the migration path for existing deployments is much more realistic:
Wdyt? I know this adds complexity, sorry, but I'm afraid it's complexity we need. |
That's the way, how Eclipse/Californium supports migration and backwards compatibility. The code-point |
Good point, we shouldn't put Good to know you used the same strategy to support migration! |
Let me add:
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. |
@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! |
PS: we also require commit messages to include a |
On the one side you're too short in time to do a review this (rather small) PR. 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. (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. (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 .) |
@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 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. |
@plskeggs I rebased the code to the development branch. Thanks for pointing this out. |
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? |
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? |
@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). |
The DCO check is failing. Please add signoff lines to all commit messages before we can (re)start reviewing. |
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. |
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
-Backported hannestschofenig's Mbed-TLS/mbedtls#5061 to mbed TLS 3.0. This makes DTLS CID support compliant with RFC-9146. ref: NCSDK-15193 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no> Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no> (cherry picked from commit cf39070)
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
@hannestschofenig -- is it safe to assume that this PR is superseded by #6264? |
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. |
Thanks a lot for all the work an the big step ahead now! |
Closing, superseded by #6264 |
-Backported hannestschofenig's Mbed-TLS/mbedtls#5061 to mbed TLS 3.0. This makes DTLS CID support compliant with RFC-9146. ref: NCSDK-15193 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no> Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no> (cherry picked from commit cf39070) (cherry picked from commit 96bd272)
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
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:
Todos
Steps to test or reproduce
I have tested ssl_client2 against Californium.