Skip to content

[TLS 1.3] PSK Support#3618

Merged
reneme merged 5 commits intorandombit:masterfrom
Rohde-Schwarz:tls13/psk
Aug 7, 2023
Merged

[TLS 1.3] PSK Support#3618
reneme merged 5 commits intorandombit:masterfrom
Rohde-Schwarz:tls13/psk

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Jul 6, 2023

Pull Request Dependencies

Description

This adds support for externally provisioned PSKs in TLS 1.3. Note that currently, only the PSK_DHE_KE mode is implemented. PSK_KE is left as future-work, same with 0-RTT/early data support.

External PSKs are provided by deriving the Credentials_Manager and overriding at least the find_preshared_keys() method. For fine-grained control, applications should additionally override choose_preshared_key().

Note that the low-level key derivation machinery for PSK support was already implemented due to the existing TLS 1.3 resumption support. Most of this pull request is focussing on managing the offering and choosing of the PSK(s) during the early negotiation.

Tests

Unfortunately, BoGo doesn't come with PSK tests for TLS 1.3 (yet?). We added an integration test in test_cli.py that performs a PSK-based handshake between a botan-cli client and server. Additionally we sucessfully confirmed compatibility with openssl s_client and gnutls-cli both in TLS 1.2 and 1.3.

Copy link
Copy Markdown
Collaborator Author

@reneme reneme left a comment

Choose a reason for hiding this comment

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

Self-review. We'll address those issues (+ the currently failing CI) today. External PSK should then be finished from our side.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 11, 2023

Coverage Status

coverage: 91.732% (+0.04%) from 91.691% when pulling 35180a8 on Rohde-Schwarz:tls13/psk into 65b7548 on randombit:master.

@reneme reneme force-pushed the tls13/psk branch 2 times, most recently from 2201d0b to 4c5d0bf Compare July 12, 2023 07:14
@reneme reneme marked this pull request as ready for review July 12, 2023 07:17
Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Jul 18, 2023

Rebased after #3622 was merged. As mentioned in the PR description, most of this patch adds details to manage PSKs and let them co-exist with sessions during the early stages of the handshake. Along with the introduction of the public API Channel::external_psk_identity() this might be somewhat hard to follow. Should we spend the time and split this into two or three more focused commits?

Copy link
Copy Markdown
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

Looks very good.

I'd like to see at least one C++ test since IIUC we are currently only testing this functionality with the cli, which seems not great.

@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Jul 26, 2023

I'd like to see at least one C++ test since IIUC we are currently only testing this functionality with the cli, which seems not great.

Mhm, that bugged me as well. Especially, since we're just doing a round-trip test in CLI against our own implementation. If we break something along the way, chances are that we would stay compatible with ourselves but not with the outside world. 😨

Unfortunately, neither BoGo nor RFC8448 provide tests for externally provided PSK. And just implementing another round-trip test in C++ didn't seem like a big win to me.

Suggestion: Let's generate our own RFC8448-ish test vector and freeze it in a Text_Based_Test. This current state is manually cross-tested against OpenSSL and gnutls at least. I'll look into that.

@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Jul 26, 2023

Suggestion: Let's generate our own RFC8448-ish test vector and freeze it in a Text_Based_Test. This current state is manually cross-tested against OpenSSL and gnutls at least. I'll look into that.

Done. This worked surprisingly smoothly by hacking the CLI-based roundtrip test we had implemented already.

For future reference:

  1. Fix the RNG seed to make things deterministic ./botan tls_server/tls_client --rng-type=drbg --drbg-seed=<some fixed seed>
  2. Configure the roundtrip test to what we need (mostly setting up appropriate policies for client and server)
  3. Print the relevant information in the C++ TLS implementation
  4. Spend some nerves on debugging and formatting it for test_tls_rfc8448.cpp and building a test flow for it

I've created a temporary branch with the hacked bits. This might come in handy if we want to repeat this exercise for PSK_KE (w/o ephemeral key).

@reneme reneme requested a review from randombit July 26, 2023 12:58
@randombit
Copy link
Copy Markdown
Owner

Thanks for adding the new tests @reneme

@reneme reneme merged commit 702b9f6 into randombit:master Aug 7, 2023
@reneme reneme deleted the tls13/psk branch August 7, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants