Skip to content
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

[TLS 1.3] Extend CredentialsManager for upcoming TLS 1.3 PSK #3622

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Jul 11, 2023

This attempts to step away from the generic Credentials_Manager::psk() method that is parameterized with type and context. Instead, the class now has dedicated getter-methods for the specific keys it manages.

Before After
type:"tls-server", context:"session-ticket" Credentials_Manager::session_ticket_key()
type:"tls-server", context:"dtls-cookie-secret" Credentials_Manager::dtls_cookie_secret()
type"tls-server"/"tls-client", context:"my.host.name" Credentials_Manager::find_preshared_keys()

The TLS 1.2 implementation keeps using the ::psk() method as before -- so existing applications that override it will continue to work as before. We think, dedicated methods for specific artefacts is much more on-par with the other TLS dependency classes (e.g. Callbacks and Policy). Additionally, its easier to understand the API like that.

New applications can make use of the added default implementation of ::psk() that orchestrates the new dedicated methods as shown in the table above. With Botan 4.0 the ::psk() method should probably disappear entirely. (And Credentials_Manager should perhaps move into the TLS namespace.)

The new methods find_preshared_keys() and choose_preshared_key() take into account that one can offer multiple PSKs in a TLS 1.3 Client Hello. The server then gets to choose which PSK it wants to negotiate with.

Given that TLS 1.2 pre-negotiates a singular PSK identity prior to actually requesting the actual PSK-value, it can benefit from the same find_preshared_keys() (through the legacy psk()). If exactly one PSK identity is requested, it should be guaranteed that at most one PSK is returned by find_preshared_keys().

The actual TLS 1.3 PSK implementation will come in a follow-up.

@reneme reneme mentioned this pull request Jul 11, 2023
Copy link
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 for tomorrow.

src/fuzzer/tls_client.cpp Show resolved Hide resolved
src/fuzzer/tls_server.cpp Show resolved Hide resolved
src/lib/tls/credentials_manager.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls13/tls_psk_identity_13.h Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 11, 2023

Coverage Status

coverage: 91.706% (-0.03%) from 91.735% when pulling c575c31 on Rohde-Schwarz:tls13/creds_mgr_for_psk into 054e13f on randombit:master.

This introduces new explicit methods for the session ticket key, the
DTLS hello cookie and a more generic PSK search method.

The existing psk() method now has a default implementation that
orchestrates the new methods via the `context` and `type` parameters
just like before.

Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
Copy link
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.

LGTM


// New applications should use the appropriate credentials methods. This is a
// retrofit of the behaviour before Botan 3.2.0 and will be removed in a
// future major release.
Copy link
Owner

Choose a reason for hiding this comment

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

We should note this in doc/deprecated.rst

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: c575c31

Please merge at will, if the text in doc/deprecated.rst is sufficient.

@randombit randombit merged commit 65b7548 into randombit:master Jul 18, 2023
32 of 33 checks passed
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