Repository navigation
Conversation
reneme
force-pushed
the
dtls13/cipher_state
branch
5 times, most recently
from
October 8, 2026 14:40
23f7ad1 to
b78a90e
Compare
Open
reneme
force-pushed
the
dtls13/cipher_state
branch
3 times, most recently
from
October 8, 2026 20:11
fdf7c50 to
a55f4f3
Compare
Cipher_State becomes a (non-instantiable) base class that implements the (D)TLS 1.3 key schedule (RFC 9846 7.1). Record protection is moved into a new TLS_Cipher_State subclass. The base class's public API remains unchanged apart from hosting neither protect_record() nor deprotect_record() anymore. Those are provided by the concrete implementations, currently TLS only. Co-Authored-By: Amos Treiber <amos.treiber@rohde-schwarz.com>
All cryptographic aspects that belong to one generation of traffic keys (AEAD instance, IV, sequence number, traffic secret and, for handshake epochs, the Finished key) are now captured in the internal structure Cipher_State::Epoch. Previously, those were individual members of Cipher_State. The base class now accesses the epochs through a few virtual methods and leaves their management to the concrete subclasses. TLS_Cipher_State keeps one current epoch per direction and simply rolls it over whenever new traffic secrets are derived. Co-Authored-By: Amos Treiber <amos.treiber@rohde-schwarz.com>
The low-level aspects of protecting and deprotecting a record are moved out of TLS_Cipher_State::protect_record() and ::deprotect_record() into protected static helpers of Cipher_State that operate on a given Epoch. This is done to allow re-using these aspects in an upcoming DTLS variant. Co-Authored-By: Amos Treiber <amos.treiber@rohde-schwarz.com>
reneme
force-pushed
the
dtls13/cipher_state
branch
from
October 9, 2026 05:44
a55f4f3 to
73b74c8
Compare
randombit
reviewed
Oct 10, 2026
|
|
||
| const auto hkdf_label = | ||
| concat<secure_vector<uint8_t>>(store_be(static_cast<uint16_t>(length)), | ||
| store_be(static_cast<uint8_t>(m_expansion_label_prefix.size() + label.size())), |
Owner
There was a problem hiding this comment.
This drops the check on the label lengths (was BOTAN_ARG_CHECK(prefix.size() + label.size() <= 255, "label too large");)
| } | ||
|
|
||
| uint64_t Cipher_State::records_decrypted_with_current_key() const { | ||
| return latest_read_epoch().sequence_number; |
Owner
There was a problem hiding this comment.
I think this causes a half-closed connection to fail; peer sends close_notify alert, we remove the read epoch, then the send path checks this value [to see if it should try to request a peer KeyUpdate] which then fails with an exception.
| std::optional<Epoch> m_read_epoch; | ||
| }; | ||
|
|
||
| inline TLS_Cipher_State* as_tls_cipher_state(Cipher_State* cs) { |
Owner
There was a problem hiding this comment.
This introduces a dynamic cast call per record and I'm not sure there is any need for it - couldn't the record layer just hold the correct type directly?
| } | ||
|
|
||
| BOTAN_ASSERT_NOMSG((m_encrypt == nullptr) == (m_decrypt == nullptr)); | ||
| BOTAN_ASSERT_NOMSG(has_write_epoch() == has_read_epoch()); |
Owner
There was a problem hiding this comment.
This is analagous to the earlier assert, but different in the case of a half-close
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes
TLS::Cipher_Statean abstract base class that implements TLS' key schedule and provides low-level building blocks for record (de)protection. The base class is derived byTLS::TLS_Cipher_Statethat adds TLS-specific record (de)protection based on that.Also, this patch introduces an
Epochstruct that holds the unidirectional cryptographic infrastructure for generations of send/recv traffic keys. NaturallyTLS_Cipher_Statejust handles singleton Epochs, but the upcomingDTLS_Cipher_State(#5000) will maintain historic Epochs for handling retransmissions and out-of-order record delivery.The agents did a decent job to split this up into reviewable commits:
TLS_Cipher_State. The *.cpp has some large hunks, but that's essentially just moving code around.Epochstructure