Skip to content

Conversation

@mfil
Copy link
Contributor

@mfil mfil commented Jul 23, 2024

Description

This pull request implements the TLS-Exporter feature as defined in RFC 8446, Section 7.5 and RFC 5705.

TLS-Exporter allows the client and server to extract additional shared symmetric keys from the SSL context by inputting a label and a desired length for the key.

Currently, it is possible for library users to implement TLS-Exporter in TLS 1.2 by using mbedtls_ssl_set_export_keys_cb() to obtain the master secret and then calculate mbedtls_ssl_tls_prf(). It is not currently possible to do this for TLS 1.3. This pull request adds the function mbedtls_ssl_export_keying_material() to implement TLS-Exporter in the library for both TLS 1.2 and 1.3.

I have marked this pull request as "Draft" because I have not yet added any automated tests, and I would like some help in figuring out what would be the best way to add them. I have added options to ssl_client2 and ssl_server2 to print out the derived symmetric keys on the command line. I have checked that when connecting openssl s_client (with the -keymatexport option) to ssl_server2, they both produce the same key.

I have added a test for the TLS 1.3 Exporter. I could not find test vectors online, so I have taken the "exp master" key from RFC 8448 and used an online HMAC-SHA256 calculator to calculate the expected result. Additionally, I have added options to ssl_client2 and ssl_server2 to print out the derived symmetric keys on the command line. I have checked that when connecting openssl s_client (with the -keymatexport option) to ssl_server2, they both export the same key.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog provided
  • development PR provided
  • framework PR not required
  • 3.6 PR provided [Backport 3.6] Implement TLS-Exporter #9469
  • 2.28 PR not required because: TLS 1.3 is only experimental in this version, and the user can implement TLS-Exporter in TLS 1.2
  • tests provided

@mfil mfil marked this pull request as draft July 23, 2024 12:41
@mfil mfil force-pushed the feature/implement_tls_exporter branch from 7de124f to d71386c Compare July 25, 2024 14:18
@mfil
Copy link
Contributor Author

mfil commented Jul 25, 2024

Sorry for the force-push. I forgot to sign off my latest commit, and there's no way to fix that with more commits.

I can close and reopen this merge request, but first I'd like to ask for some help with implementing unit tests.

@mfil mfil marked this pull request as ready for review August 9, 2024 18:06
@mfil mfil changed the title Draft: Implement TLS-Exporter Implement TLS-Exporter Aug 9, 2024
@mfil mfil force-pushed the feature/implement_tls_exporter branch from 8dd570e to 46e761e Compare August 12, 2024 09:22
@mfil mfil mentioned this pull request Aug 12, 2024
5 tasks
@gowthamsk-arm gowthamsk-arm added priority-medium Medium priority - this can be reviewed as time permits 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-m Estimated task size: medium (~1w) component-tls13 needs-ci Needs to pass CI tests labels Aug 13, 2024
@mfil mfil force-pushed the feature/implement_tls_exporter branch from a9085ea to b2bb98f Compare August 14, 2024 14:47
@davidhorstmann-arm
Copy link
Contributor

@mfil Thanks for this very succinct and well-documented PR. @waleed-elmelegy-arm and I plan to do reviews over the next couple of months so we can try to get this merged.

In the meantime, there are some merge conflicts (I think from the 3.6 release). Would you be able to resolve these? We aren't planning much work in this area of the code for a while, so it will be unlikely to bitrot again.

@mfil
Copy link
Contributor Author

mfil commented Sep 20, 2024

@mfil Thanks for this very succinct and well-documented PR. @waleed-elmelegy-arm and I plan to do reviews over the next couple of months so we can try to get this merged.

In the meantime, there are some merge conflicts (I think from the 3.6 release). Would you be able to resolve these? We aren't planning much work in this area of the code for a while, so it will be unlikely to bitrot again.

I've rebased onto develop, fixed some mistakes in the comments, and force-pushed. (Let me know if you prefer opening a new pull request.)

I'll wait for reviews before changing the 3.6 backport pull request, ok?

@davidhorstmann-arm
Copy link
Contributor

I'll wait for reviews before changing the 3.6 backport pull request, ok?

That sounds fine to me, thanks! I'll start the CI again, I seem to remember it failed the last time with a build error in some configurations. If you need a hand reproducing it let me know.

@mfil
Copy link
Contributor Author

mfil commented Sep 20, 2024

I'll wait for reviews before changing the 3.6 backport pull request, ok?

That sounds fine to me, thanks! I'll start the CI again, I seem to remember it failed the last time with a build error in some configurations. If you need a hand reproducing it let me know.

Most of them make sense to me.

In all_u16-check_names, it complains that mbedtls_ssl_export_keying_material isn't declared in a header, but it's right there in include/mbedtls/ssl.h, so I don't understand that.

@mfil
Copy link
Contributor Author

mfil commented Sep 21, 2024

@davidhorstmann-arm I think I fixed everything. Can you re-run the CI?

@gilles-peskine-arm
Copy link
Contributor

I've started a CI run.

@mfil
Copy link
Contributor Author

mfil commented Sep 22, 2024

I've started a CI run.

Thank you!

I'm stuck on getting the last tests to succeed. The TLS 1.2 Exporter needs client_random and server_random. If I'm seeing this correctly, if MBEDTLS_SSL_CONTEXT_SERIALIZATION is not defined, then these go away when the handshake is done.

Would it be a problem to always have a randbytes field in struct mbedtls_ssl_transform?

@tom-cosgrove-arm
Copy link
Contributor

I've started the CI on the updated PR

@mfil
Copy link
Contributor Author

mfil commented Sep 22, 2024

Ok, the code style check is happy now, that still leaves my problem I mentioned above.

@mfil mfil force-pushed the feature/implement_tls_exporter branch from b789256 to 8cd4456 Compare October 18, 2024 13:45
@mfil
Copy link
Contributor Author

mfil commented Oct 18, 2024

I've fixed the merge conflict and added a commit that should fix the remaining CI failures. Please re-run the CI @davidhorstmann-arm @gilles-peskine-arm

@waleed-elmelegy-arm waleed-elmelegy-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Apr 14, 2025
@gilles-peskine-arm
Copy link
Contributor

There's a 3.6 backport but it isn't up to date. @mfil @davidhorstmann-arm @waleed-elmelegy-arm Is there still interest in the 3.6 backport? Given the size of the code now, is it still ok for a long-term support branch?

@gilles-peskine-arm gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Apr 15, 2025
@gilles-peskine-arm
Copy link
Contributor

Having checked internally, it seems that we would still like to include this in Mbed TLS 3.6. @mfil Can you please update #9469 ? Since it hasn't been reviewed yet, please rebase it on top of the current head of mbedtls-3.6, and update the patch to the version that's approved for the development branch. If any differences are needed in 3.6, please briefly explain what's changed, so we know what to expect in git range-diff.

@mfil
Copy link
Contributor Author

mfil commented Apr 15, 2025

The backport pull request is ready!

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Approving the change from 1a1ec2f to dba07e1

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Apr 16, 2025
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@davidhorstmann-arm davidhorstmann-arm 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, needs-backports Backports are missing or are pending review and approval. needs-ci Needs to pass CI tests labels Apr 17, 2025
@davidhorstmann-arm
Copy link
Contributor

@mfil Thanks for all your work on this. Everything looks fine and the backport is approved, so I'll add it to the merge queue!

@davidhorstmann-arm davidhorstmann-arm added this pull request to the merge queue Apr 17, 2025
Merged via the queue into Mbed-TLS:development with commit 232da48 Apr 17, 2025
4 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Non-roadmap pull requests Apr 17, 2025
@Neustradamus
Copy link

Thanks @mfil for your PR and @davidhorstmann-arm, @gilles-peskine-arm, @tom-cosgrove-arm, @waleed-elmelegy-arm, @gowthamsk-arm: I have discovered this PR, it follows a part of my ticket here:

So "tls-exporter" is now supported but "tls-unique" and "tls-server-end-point" are always missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports component-tls13 enhancement priority-medium Medium priority - this can be reviewed as time permits size-m Estimated task size: medium (~1w)

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants