Skip to content

[TLS 1.3] Use a KEM interface in the Callbacks#3608

Merged
reneme merged 2 commits intorandombit:masterfrom
Rohde-Schwarz:tls13/kem_callbacks
Jul 3, 2023
Merged

[TLS 1.3] Use a KEM interface in the Callbacks#3608
reneme merged 2 commits intorandombit:masterfrom
Rohde-Schwarz:tls13/kem_callbacks

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Jun 28, 2023

This refactors the TLS 1.3 asymmetric key establishment to use a KEM-like interface. I.e. TLS::Callbacks now has three new methods:

  • tls_kem_generate_key()
  • tls_kem_encapsulate()
  • tls_kem_decapsulate()

Their default implementations simply use the existing tls_generate_ephemeral_key() and tls_ephemeral_key_agreement(). So "legacy" code based on Botan 3.0 won't break. With that, we gain the flexibility to implement the TLS 1.3 key establishment with PQ KEMs or hybrid approaches.

TLS 1.2 stays with the existing KEX-based callbacks mentioned above. We're not planning on contributing a KEM-based key establishment for TLS 1.2 anyway.

Future Work

Frankly, the internal implementation of the Key_Share extension is a mess. I'll look into cleaning this up independently.

Perhaps we might want to look into migrating TLS 1.2 to those callbacks as well and deprecate the old KEX-based callbacks entirely.

@reneme reneme requested a review from randombit June 28, 2023 12:09
For that, three new TLS::Callbacks have been added. For backward
compatibility they are implemented in terms of the existing key
agreement callbacks. This will provide the flexibility to add hybrid PQ
KEMs to the TLS 1.3 handshake

Co-Authored-By: Fabian Albert fabian.albert@rohde-schwarz.com
@reneme reneme force-pushed the tls13/kem_callbacks branch from 7e51956 to 8224afd Compare June 29, 2023 09:37
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 29, 2023

Coverage Status

coverage: 91.735% (-0.006%) from 91.741% when pulling ccb0e42 on Rohde-Schwarz:tls13/kem_callbacks into 73ad31a on randombit:master.

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 good to me, thanks. Left a few minor comments. Fine to merge after addressing.

struct Encapsulation_Result {
std::vector<uint8_t> encapsulated_bytes;
secure_vector<uint8_t> shared_secret;
};
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd prefer this be a class with a constructor, private members, and accessors. Specifically then, given an Encapsulation_Result, one can know that we never eg modify the encapsulated bytes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

At first we just wanted to use a std::pair but went for this simple struct to be able to name the members of the returned tuple in tls_kem_encapsulate.

Though, thinking about it, we could add a PK_KEM_Encryptor::encrypt() override that also uses this struct instead of the currently used out-params. Having this as a "full blown" class might be more worthwhile.

Maybe as a follow-up?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah having something like this be the return type from the basic KEM operations would be quite reasonable, the current KEM approach of having the user pass two references is a little confusing. Main concern would be this is public API and whatever we ships in 3.1.0 (July 10th) we're mostly stuck with. If you think you would have time to look at this before then, fine to merge as is

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See here for a suggestion to generalize this result struct: #3611

@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Jul 3, 2023

Merging for better overview in #3609.

@reneme reneme merged commit cf8d8a6 into randombit:master Jul 3, 2023
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