DTLS 1.3: use the "dtls13" HKDF label prefix (RFC 9147 5.9), fixes interop with BoringSSL - #1
Closed
JonathanLennox wants to merge 36 commits into
Closed
JonathanLennox wants to merge 36 commits into
JonathanLennox wants to merge 36 commits into
Conversation
…atagrams Adds the ACK-driven reliable handshake of RFC 9147 sections 5.8 and 7, and packs handshake flights into as few datagrams as the MTU allows. The packing applies to DTLS 1.2 as well as 1.3, since that is what github bcgit#1487 asks for: on the existing aggregated-handshake test a client flight went from 5 datagrams to 3 for the same 1279 bytes. Only handshake records are packed, and a non-handshake record flushes the buffer before it leaves, so write order is preserved; that matters for the implicit change_cipher_spec, which must not overtake the flight it follows. The reliable handshake registers each written fragment against the record number that carried it, retires fragments when an ACK arrives, retransmits only what is outstanding, and emits ACKs on the RFC 9147 7.1 triggers. Inbound ACKs are filtered by the epoch of the record carrying them, and both ACK emission and the accumulated record-number list are bounded by what fits in one datagram. DTLS 1.3 still cannot be negotiated, so none of the 1.3 paths are reachable yet. DTLSReassembler.contributeFragment changed from void to boolean and gained acceptsFragment and getNextExpectedOffset; the admission predicate is unchanged, and DTLS 1.2 behaviour is unaffected. Two test helpers, MinimalHandshakeAggregator and ServerHandshakeDropper, decided what to do by inspecting only the first record of a datagram, which was exact only while each datagram carried one record. Both now walk every record. relates to github bcgit#1468. closes github bcgit#1487.
…relates to github bcgit#1468.
…relates to github bcgit#1468.
…t, relates to github bcgit#1468.
…overage, relates to github bcgit#1468.
…s retransmission, relates to github bcgit#1468.
…on, relates to github bcgit#1468.
…etention, relates to github bcgit#1468.
…, relates to github bcgit#1468.
…seq, relates to github bcgit#1468. The re-review of the previous fixes found two comments claiming more than the code does and two test assertions weaker than their messages. The notification comment said the call was placed exactly as TlsServerProtocol does it. The version gating is mirrored; the ordering relative to the other TlsServer callbacks is not, and cannot be, because DTLS selects the version in generateServerHello, by which point establishClientSigAlgs and processClientExtensions have already run. Say both, rather than claiming a wholesale mirror. DTLSVerifier produces a DTLSRequest for any ClientHello carrying a cookie it has verified, so it need not have sent the HelloVerifyRequest during that call. The invariant is that the ClientHello arrived through the cookie exchange. The server-refusal test asserted the internal_error the client receives, which is not specific to this cause, so the harness now records what the server itself threw and the test asserts that diagnostic. The harness also stops printing a stack trace for a server abort a test expects, which otherwise makes a passing test look like a failing one. Counting datagrams that carry a ClientHello cannot tell a new message from a retransmission of the first, so the scripted transport records each ClientHello's message_seq and the test requires it to advance to 1, per RFC 6347 4.2.2.
…ke, relates to github bcgit#1468. TlsUtils.initCipher and the TlsAEADCipher constructor both read getSecurityParametersHandshake(), which AbstractTlsContext.handshakeComplete nulls, so neither could build a cipher once a connection was established. A DTLS 1.3 key update (RFC 9147 4.6.3) has to do exactly that. Add TlsCryptoParameters.getSecurityParameters(), mirroring AbstractTlsContext.getSecurityParameters(): the handshake parameters while a handshake is in progress, the connection parameters otherwise. Read the parameters through it at those two sites. TLS over TCP and DTLS 1.2 are unchanged. handshakeComplete moves the single SecurityParameters instance from the handshake field to the connection field; while the handshake field is non-null, getSecurityParameters() returns that same object, so the new accessor is identical to the old one for every caller that runs during a handshake. Every production caller of initCipher is a handshake path in TlsClientProtocol, TlsServerProtocol, DTLSClientProtocol and DTLSServerProtocol, including renegotiation, which reopens handshake parameters via handshakeBeginning first. TLS 1.3 key update over TCP does not come through here at all: it rekeys the existing cipher in place through RecordStream's rekeyDecoder/rekeyEncoder. The change is therefore purely a widening: a call that used to throw NullPointerException now resolves. The full TLS, DTLS and JSSE suites were run against it. TlsImplUtils.calculateKeyBlock, which still reads the handshake parameters, is unreachable post-handshake: the TLS 1.3 branch of the TlsAEADCipher constructor returns before it.
…sors, relates to github bcgit#1468. Review of the preceding commit found two defects in it. TlsCryptoParameters.getSecurityParameters() delegated straight to the context, bypassing this class's own public, non-final accessors. This repository already contains subclasses constructed with a null context that override only the accessors they need, which is what a third party stubbing a cipher would write; such a subclass was silently bypassed, and one with a null context threw where it previously worked. Tls13NullCipher still read the handshake security parameters, so the same post-handshake limitation remained one class over. The RFC 9150 integrity-only suites reach it through create13NullCipher, are accepted by getPRFAlgorithm for any 1.3 version with no DTLS exclusion, and are exposed by the JSSE provider, so a DTLS 1.3 key update would have thrown whenever one was negotiated.
…thub bcgit#1468. processDTLS13Record probed exactly two candidates, the current read epoch and the epoch retained for retransmission. That is right while those are the only epochs the record layer holds, but it has no notion of recency, so it cannot express RFC 9147 4.2.2's rule that a record whose low two epoch bits match more than one held epoch belongs to the most recent of them - and it has nowhere to put the pre-update read epoch that RFC 9147 8 requires be retained across a key update. Replace the probe with one ordered collection of live read epochs, iterated most recent first, so that 4.2.2's rule is the iteration order rather than a special case. getEpochForRetransmit resolves through the same collection, so the send and receive sides cannot disagree about which epochs are held. The collection is bounded: at most one read epoch is retained across a key update, and retainReadEpoch asserts it. Section 8 forbids sending at a new epoch before the peer's KeyUpdate is acknowledged, so a second update cannot begin while the first is retained; a set that grew without that bound would be a memory-growth denial of service driven by peer-controlled key updates. No behaviour changes here. The retained slot is always empty until key update lands, and the plaintext epoch that now appears in the collection was already rejected a few lines later by the DTLSCiphertext cipher check.
…github bcgit#1468. RFC 9147 8 forbids sending under a new epoch until the peer has acknowledged the KeyUpdate, while updating the local traffic secret destroys the secret the current write epoch was keyed from, so the new epoch must be built at the moment the KeyUpdate is generated and then held unused. Neither existing mechanism can hold it. enablePendingEpochWrite installs the pending epoch the instant it is asked to, and commitPendingEpochIfCurrent retires the old epoch only once both directions have reached the pending one; a key update advances exactly one direction, so it satisfies neither. A shared pending epoch would also force one epoch number on both directions, which is wrong once each direction advances on its own key updates - so the number here is derived from the write epoch alone rather than reusing initPendingEpoch's. Add pendingWriteEpoch with derivePendingWriteEpoch to build and hold it and installPendingWriteEpoch to make it the write epoch. The held epoch is deliberately not resolvable by getEpochForRetransmit: nothing has been sent under it, so there is nothing to retransmit under it. A second derivation while one is held is refused, matching 5.8.4's ban on a second KeyUpdate while one is outstanding. No behaviour changes here. Nothing calls either method yet, so the handshake's epoch progression is untouched; key update itself follows.
… github bcgit#1468. DTLSReliableHandshake.finish() unregistered the record layer's ACK listener outright, with a comment that post-handshake ACKs belong to the post-handshake state machines. Nothing owned them, so an ACK or a handshake message arriving after the handshake was decoded and dropped. DTLS13PostHandshake is that owner. It receives every handshake record that arrives at epoch 3 or above (RFC 9147 6.1), reassembles it with DTLSReassembler - a NewSessionTicket routinely exceeds an MTU, so refusing fragments would be an interoperability failure our own peers could never show - dispatches by message type, and acknowledges the record. NewSessionTicket is received, acknowledged and discarded: resumption and pre-shared keys are not supported here. This implements the receiver's acknowledgement obligation, not the sender's no-send-before-acknowledgement rule; the sending state machines of RFC 9147 5.8.4 arrive with key update. Two rules are easy to get backwards and are deliberate here: - RFC 9147 7 requires a post-handshake record to be acknowledged even when the message it carries is discarded as a duplicate, which is the opposite of the handshake-time rule. During the handshake the flight's own quarter timer will ACK anyway; after it the peer is waiting on exactly this ACK and would otherwise retransmit for as long as its state machine runs. The handshake-time test is untouched. - The RFC 9147 5.8.1 final-flight hook expires with the retransmit timeout at twice the MSL. This owner does not: a KeyUpdate may arrive at any point in a connection's life, and an owner that expired with that timeout would work in every test that finishes in milliseconds and die on exactly the long-lived connections it exists for. Both handlers are offered the record and partition it by epoch. The message_seq counters are carried across the handshake boundary rather than restarted, and a KeyUpdate arriving before the handshake completes is now fatal with unexpected_message (RFC 8446 4.6.3) instead of being dropped quietly.
…old one on first decrypt, relates to github bcgit#1468. DTLS13PostHandshake validated, counted and acknowledged a received KeyUpdate but did nothing with it, so an acknowledged key update left the peer writing under keys the connection could not read. This closes that. This implements the RECEIVER half of RFC 9147 8, whose trigger is the first successful DECRYPTION at the new epoch - not the sender's half, whose trigger is an acknowledgement of its own KeyUpdate. An ACK is not a decryption, and nothing here reads one. updatePeerReadEpoch is the whole of a received key update's effect on the read side, and is one operation on purpose. update13TrafficSecretPeer destroys the secret it replaces, so a cipher not built from the updated secret at that moment can never be built from the superseded one afterwards, and the epoch retained a line later would be keyed from material that no longer exists. Everything that can refuse the update is therefore checked before the secret is touched, and the construction is not deferred. Note that building a cipher after the handshake no longer throws now that the security parameters resolve to the connection ones, so that call cannot be relied on to object if it is reached in the wrong order; only the order protects this. The pre-update epoch is released at the point RFC 9147 8 names - a record has just decrypted at the current read epoch - and before the record is dispatched. That ordering is load-bearing: our acknowledgement is what frees the peer to send at its new epoch and it need not send anything else first, so a KeyUpdate may legitimately be the first thing to arrive there, and a later release would make that look like a second update while one was still retained. One that really is that is refused with unexpected_message rather than being allowed to drop keys records are still arriving under or to grow the retained set at the peer's discretion. An update_requested is recorded as a pending obligation rather than answered from the receive path, which is what TlsProtocol.receive13KeyUpdate does too. Over DTLS there is a second reason: RFC 9147 5.8.4 makes a KeyUpdate a single-flight message with its own retransmit timer and section 8 forbids sending under the new epoch until it is acknowledged, so the answer belongs to the sending state machine - which is also the only side that can apply section 8's override of RFC 8446 at the epoch limit. Three things inherited from the epoch commits first become live here: - installPendingWriteEpoch now advances currentEpoch with the write epoch. Until a key update both directions share one DTLSEpoch, so the superseded number stays resolvable through the other direction and nothing is stale; once both have moved and the retained read epoch is released, nothing holds that epoch but a currentEpoch left behind, and getEpochForRetransmit falls back to it - a released epoch resolving to stale keys and a used sequence number. - The superseded write epoch is retained by nothing and released there. 5.8.4's sending state machine has had its ACK by then, and section 7 requires a post-handshake ACK at the highest available sending epoch, so nothing is ever sent under it again. Only the read side needs retaining. - nextEpoch is the epoch-number overflow guard, used by both directions' key update. Section 8's own cap is 2^48-1 for a sender and unenforceable for a receiver; neither binds before the int a DTLSEpoch holds its epoch in, and a silent wrap would give a negative epoch or one aliasing an epoch already held. commitPendingEpochIfCurrent is left alone: its both-directions test is an invariant a key update never reaches, not one it falsifies, and that is now documented rather than assumed. The paired-record-layer harness set prfCryptoHashAlgorithm without prfHashLength, which production sets in the same place. It could build a cipher but not update a traffic secret, which no test before this one did.
…elates to github bcgit#1468. This is the SENDING half of RFC 9147 section 8. Its trigger is the acknowledgement: "implementations MUST NOT send records with the new keys or send a new KeyUpdate until the previous KeyUpdate has been acknowledged". The receiving half, whose trigger is the first successful decryption at the new epoch, landed separately and is not touched here. An ACK is not a decryption. The state machine is the one RFC 9147 5.8.4 describes for a single-flight post-handshake message: derive the next write epoch and hold it, send the KeyUpdate at the old epoch, wait for an ACK, retransmit until it arrives, and install the epoch only then. It is driven from the receive path, not the send path, because a peer that has just sent a KeyUpdate may have nothing further to send - and section 8 has left it unable to use the epoch it wants. A send-driven timer would stall exactly that peer. A key update starts by itself once the write epoch's sequence number reaches the threshold RecordStream.needsKeyUpdate applies over TLS, and a second one is refused while one is outstanding (5.8.4). Two acknowledgement defects that stalled this are fixed with it, both from applying a handshake-time rule after the handshake. RFC 9147 7 says "During the handshake, ACK records MUST be sent with an epoch which is equal to or higher than the record which is being acknowledged", but "After the handshake, implementations MUST use the highest available sending epoch" - with no floor. The two directions' epochs advance on their own key updates, so a peer that has updated its sending keys while we have not both sends its KeyUpdate above our epoch and acknowledges ours from below. sendAck refused the first and the ACK record-number filter discarded the second, each leaving a KeyUpdate unacknowledged and its sender retransmitting for as long as its state machine runs. Both rules now apply during the handshake only; the filter additionally still applies to an unprotected ACK at epoch 0 after it, which is the threat it was written for. An epoch built from the peer's updated traffic secret is now marked as such and is no longer resolvable for writing. A DTLSEpoch carries one cipher and one sequence number counter for both directions, so writing at the peer's epoch would encrypt under the peer's key at sequence numbers the peer has already used. Nothing reaches it today, but the two directions' epoch numbers coincide as a matter of course, so it would be reached by number collision rather than by anything obviously wrong.
…elates to github bcgit#1468. The whole-branch review returned zero Critical findings; these are its six Important and four Minor ones, applied in one pass. No protocol behaviour changes. The substantive one is that the peerKeyed guard was correct but its reasoning was not, and the reasoning is what a maintainer would use to decide whether the guard is still needed. TlsUtils.initCipher builds a cipher for BOTH directions: TlsAEADCipher's TLS 1.3 constructor calls rekeyCipher once for the decrypt side, keyed from the peer's traffic secret, and once for the encrypt side, keyed from the local one. updatePeerReadEpoch updates only the peer's secret, so the epoch it builds carries an encrypt side keyed identically to the current write epoch's, paired with a sequence number counter starting at zero. Writing at such an epoch therefore does not produce records the peer cannot read, as the comments claimed: it produces records under the same AEAD key at nonces the current write epoch has already used - nonce reuse under a live key, silent, and for GCM enough to recover the authentication key. Both comments now say that, and record the structural cause: updatePeerReadEpoch builds a full bidirectional cipher when it needs only the read direction, so it leaves a correctly-keyed encryptor in an object that must never encrypt. Also: - Disclose the one deliberate specification deviation. RFC 8446 4.6.3 has a peer that receives 'update_requested' send its own KeyUpdate before its next Application Data record; RFC 9147 5.8.4 forbids sending one while an earlier one is unacknowledged. 9147 is followed and the obligation is deferred past application data. checkKeyUpdateBeforeSend now names both rules, which wins and why, handleMessage's 8446 quote points at it, and a new test exercises the deferral and its discharge from the wire. - Take getLiveReadEpochs off the per-record receive path. Both directions now resolve by walking one private ordering of the four epoch slots in place, instead of building a Vector and scanning it for every received unified-header record; getLiveReadEpochs is retained as the test-facing view over the same ordering. - Synchronize DTLS13PostHandshake. DTLSRecordLayer supports concurrent send and receive and reaches this object from both, so its mutable state needed guarding (not a key-confusion case: that was checked and is unreachable). No call site holds the record layer's write lock, so the added monitor cannot invert against it. - Make sendKeyUpdate's "armed last" comment true by construction: the flight tracker is now reset and registered before the latch that speaks for it, rather than relying on isComplete() being false on an empty tracker. - Minors: remove the unused getNextSendSeq; say that releaseRetainedReadEpoch is not a zeroisation guarantee and often not even the last reference; qualify getLiveReadEpochs's javadoc to the unified-header path; rename getRecordLayerForTest to getRecordLayer with the package-private contract stated; trim the end-to-end key update pump deadlines.
…r, relates to github bcgit#1468. The scoped re-review of the previous commit found that its corrected comment was still false in one reachable state, and that state is the one the new test creates. deriveNextWriteEpoch advances the local traffic secret when a KeyUpdate is sent, while installPendingWriteEpoch swaps the epoch in only when the acknowledgement arrives. So a peer read epoch derived while our own KeyUpdate is outstanding carries an encrypt side keyed like the PENDING write epoch, not the current one. The conclusion is unchanged, since the key is live either way and both sequence counters start at zero, but the sentence a maintainer would reason from was wrong, and checking it against the outstanding-KeyUpdate case would have suggested the guard was over-cautious. The lock order between DTLS13PostHandshake's monitor and the record layer's write lock was held by construction and documented nowhere. It is now stated at both ends: the monitor is taken first, and a call into that class from inside a write-lock block would invert the order. Two test comments named mutations that no longer do what they claim. The epoch ordering moved out of the test-facing view into getLiveReadEpoch, so reversing the view proves nothing; and relaxing only the first of the two guards on a second KeyUpdate fails on the exception raised by the second, not on the datagram count. The acknowledgement probe's settle period had been trimmed along with the key update deadlines it does not share. Its claim is a count of records that did not arrive, so the margin is restored under its own name.
…le (RFC 9147 section 5.9) instead of TLS 1.3's "tls13 ", so DTLS 1.3 traffic keys, record number keys, Finished keys and exporters interoperate with other implementations, relates to github bcgit#1468.
This was referenced Sep 14, 2026
mondain
force-pushed
the
dtls13/4-post-handshake
branch
from
September 15, 2026 16:48
b022ef2 to
9d43a57
Compare
Owner
|
[AI] This change is now in the series, so I'm closing this PR rather than merging it. The label prefix is a key-schedule fix that parts 1 to 3 need as much as part 4, so I cherry-picked the commit into part 1 ( One adjustment was needed: as written,
Because the branches were rewritten, GitHub now shows this PR against a base it no longer shares history with, which is why its diff looks enormous. |
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
2 participants
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.
Fixes the one thing that stopped this series from interoperating with a non-BC peer: the DTLS 1.3 key schedule was derived with TLS 1.3's HKDF-Expand-Label prefix.
RFC 9147 section 5.9:
TlsCryptoUtils.hkdfExpandLabelhardcodes"tls13 ", and nothing in the series changed that, so every DTLS 1.3 secret came out differently from what any other implementation derives: the handshake and application traffic secrets, the recordkey/iv, thesnrecord number key, thefinishedkey,traffic upd, and the RFC 5705 exporter that DTLS-SRTP depends on. It is exactly the class of defect the PR descriptions flag: BC-to-BC cannot see it, because both peers make the same mistake and agree with each other.How it showed up
Jitsi Videobridge, built on
dtls13/4-post-handshake, as DTLS server against Chrome (BoringSSL) as DTLS client, with the bridge offering DTLS 1.3 and X25519MLKEM768:TlsTimeoutException: Handshake timed out.With this change the same bridge completes the handshake with Chrome in about 100 ms:
Negotiated DTLS version DTLS 1.3, key exchange group X25519MLKEM768, and SRTP media flows in both directions, which also confirms the exporter now agrees.The change
TlsCryptoUtils.hkdfExpandLabelgains an overload with anisDTLSparameter selecting the prefix. The existing five-argument method keeps its TLS 1.3 behaviour unchanged (it delegates withfalse), so no TLS caller is affected.TlsUtils.deriveSecret,calculateFinishedHMAC,update13TrafficSecret(each taking it fromSecurityParameters.getNegotiatedVersion()),calculatePSKBinder/OfferedPsks.encodeBinders(explicit parameter;TlsClientProtocolpassesfalse, the server side reads the context),TlsAEADCipher.setup13Cipher(its existingisDTLSv13),Tls13NullCipher.setupHmac, and the exporter inAbstractTlsContext.exportKeyingMaterial13.TlsSecret.deriveUsingPRFfor thetls13_hkdf_*PRF algorithms is deliberately left on the TLS prefix; it is not on the DTLS 1.3 path.Test
DTLS13KeyScheduleLabelTestcomputes HKDF-Expand-Label independently, from the RFC 5869 HKDF-Expand definition withjavax.cryptoHMAC-SHA256 rather than through any BC TLS code, and asserts thedtls13form forkey,iv,sn,s hs traffic,finishedandexporter, that thetls13form is unchanged, that the two prefixes yield different keys, and that the five-argument overload is the TLS form. It is the DTLS 1.3 analogue of the byte-level transcript test in part 3: pinned against an external computation, not against another BC peer.tlsmodule: 896 tests, no failures;checkstyleMainclean;tls/src/main/javastays within the Java 1.4/1.5 constraints (no new APIs).Disclosure
This work was produced with generative-AI assistance, per the contributing guidelines. The RFC text, the packet capture analysis, the change and the test were reviewed.
Relates to bcgit#1468, bcgit#2441, bcgit#2442.