Conversation
jesup
requested review from
KershawChang,
larseggert,
martinthomson and
mxinden
as code owners
There was a problem hiding this comment.
Pull request overview
Adds export_keying_material support (RFC 5705/8446) across the neqo stack, enabling WebTransport's ExportKeyingMaterial functionality by wrapping NSS's SSL_ExportKeyingMaterial.
Changes:
- Added
SSL_ExportKeyingMaterialFFI binding andexport_keying_materialmethod onSecretAgentinneqo-crypto - Exposed the method through
Connection(transport) andHttp3Client(HTTP/3) layers - Added comprehensive tests at both the crypto and transport layers
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| neqo-crypto/bindings/bindings.toml | Added SSL_ExportKeyingMaterial to FFI bindings |
| neqo-crypto/src/err.rs | Added InvalidState error variant |
| neqo-crypto/src/agent.rs | Core implementation wrapping NSS's export keying material API |
| neqo-crypto/tests/agent.rs | Tests for the crypto-layer export keying material |
| neqo-transport/src/connection/mod.rs | Passthrough method on Connection |
| neqo-transport/src/connection/tests/handshake.rs | Transport-layer tests |
| neqo-http3/src/connection_client.rs | Passthrough method on Http3Client |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3464 +/- ##
=======================================
Coverage 94.85% 94.85%
=======================================
Files 114 114
Lines 37931 37931
Branches 37931 37931
=======================================
Hits 35979 35979
Misses 1252 1252
Partials 700 700
Flags with carried forward coverage won't be shown. Click here to find out more.
|
larseggert
left a comment
larseggert
left a comment
There was a problem hiding this comment.
LGTM, but has clippy and format issues. You could also be DRYer in the tests.
|
@jesup once this passes CI, can you rebase the stacked patches? I committed some changes to |
|
Note: I have some changes based on @martinthomson 's comments in PR #3428 which I'll be adding |
jesup
force-pushed
the
users/jesup/exportkey
branch
2 times, most recently
from
187bd7c to
cdf24cd
Compare
|
@jesup last commits included |
jesup
force-pushed
the
users/jesup/exportkey
branch
from
cdf24cd to
feb57f8
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds ExportKeyingMaterial support across the neqo stack (crypto → transport → HTTP/3), with the HTTP/3 layer implementing the WebTransport session-scoped context construction from draft-ietf-webtrans-http3-15 §4.8. Clean layering, good test coverage across all three tiers, and the unsafe FFI block is sound.
Blocking issue: The Session ID in the WebTransport exporter context is encoded as a fixed 8-byte big-endian integer (to_be_bytes()), but the spec requires a QUIC variable-length integer ((i) notation). This will produce wrong keying material and break interop — see inline comment with suggested fix using the existing Encoder.
Regarding previously raised review feedback: @larseggert's concerns about QUIC connection state checking (now guarded with self.state.connected()), HTTP/3 test coverage (now added), and zero-length edge case testing (now added) have all been addressed since the earlier review round.
|
This PR is part of a stack of 13 bookmarks:
Created with jj-stack |
jesup
force-pushed
the
users/jesup/exportkey
branch
from
11b152a to
847996d
Compare
There was a problem hiding this comment.
Clean layered implementation of TLS keying material export (crypto → transport → HTTP/3), with the HTTP/3 layer adding the WebTransport session-scoped context construction from draft-ietf-webtrans-http3-15 §4.8. The unsafe FFI block is sound — pointer lifetimes are tied to the input slices, lengths are validated via c_uint::try_from, and the is_connected guard prevents calling into NSS on an incomplete handshake. Good test coverage across all three layers.
Blocking: The session ID varint encoding issue already flagged — to_be_bytes() produces a fixed 8-byte encoding instead of the QUIC variable-length integer required by the spec's (i) notation. This will break interop. The suggested Encoder-based fix in that thread is the right approach.
Also still open from prior review rounds:
- RFC reference:
RFC 9846→RFC 8446inagent.rsdoc comment - Error doc:
# Errorssection inwebtransport_export_keying_materialconflatesInvalidInputandTransport - Zero-length guard: tests at all three layers assert
out_len == 0fails, but this relies on NSS internals — add an explicit guard
Previously raised and now resolved: @larseggert's feedback on QUIC connection state checking (now guarded), HTTP/3 test coverage (now added), zero-length test coverage (now added), and the Copilot .expect() suggestions (now applied).
jesup
force-pushed
the
users/jesup/exportkey
branch
from
847996d to
cf85be7
Compare
There was a problem hiding this comment.
Clean layered implementation — the FFI binding in neqo-crypto, the transport passthrough, and the WebTransport context construction in neqo-http3 are well-structured. The unsafe block is sound: pointer lifetimes are scoped to input slices, lengths are validated via c_uint::try_from, and the is_connected/connected guards prevent calling into NSS on incomplete handshakes. Test coverage across all three layers is good.
Blocking: The session ID varint encoding in webtransport_export_keying_material — to_be_bytes() produces a fixed 8-byte encoding instead of the QUIC variable-length integer required by the spec's (i) notation. This will produce incorrect keying material and break interop with spec-compliant peers.
Architectural note: Http3ServerHandler exposes several webtransport_* methods but not webtransport_export_keying_material. The spec requires both endpoints to derive keying material. This may be intentional scoping for this PR, but worth tracking — without the server-side method, the WT exporter can't be fully integration-tested at the HTTP/3 layer (client-server material agreement).
jesup
force-pushed
the
users/jesup/exportkey
branch
from
cf85be7 to
f826e0a
Compare
There was a problem hiding this comment.
Good layered design — the TLS exporter in neqo-crypto, transport passthrough, and the WebTransport session-scoped context construction in neqo-http3 are cleanly separated. Test coverage across all three layers is solid, including the session-ID scoping test and boundary checks.
Blocking: The unsafe FFI call in SecretAgent::export_keying_material references two undefined variables (ctx_ptr, ctx_len) and a non-existent type (PR_Bool). This appears to be a partial application of the simplification suggested in prior reviews — the variable-binding block was removed but the call site wasn't updated. This won't compile. See inline comment with suggested fix.
Most issues raised in prior review rounds (varint encoding, RFC reference, error docs, zero-length guard, session-ID variation test) have been addressed in this revision.
jesup
force-pushed
the
users/jesup/exportkey
branch
from
f826e0a to
68ba245
Compare
There was a problem hiding this comment.
Clean layered implementation of TLS keying material export (RFC 8446 §7.5) through neqo-crypto → neqo-transport → neqo-http3, with the HTTP/3 layer constructing the WebTransport session-scoped context per draft-ietf-webtrans-http3-15 §4.8. The unsafe FFI block is sound — pointer lifetimes are tied to input slices, lengths are validated via c_uint::try_from, and the is_connected/connected guards prevent calling into NSS before handshake completion. Test coverage across all three layers is solid, including the session-ID scoping property, boundary checks, and client-server agreement.
Blocking: context::as_ptr() uses :: instead of . — this won't compile (see inline comment).
Most issues raised in prior review rounds (varint encoding, RFC reference, zero-length guard, error docs, session-ID variation test, always-true has_context) have been addressed. The server-side webtransport_export_keying_material on Http3ServerHandler is still missing — the spec requires both endpoints to derive keying material — but that may be intentional scoping for this PR.
jesup
force-pushed
the
users/jesup/exportkey
branch
from
68ba245 to
afd9b26
Compare
jesup
force-pushed
the
users/jesup/exportkey
branch
2 times, most recently
from
6c20b8b to
b76faba
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds export_keying_material at the QUIC transport layer and webtransport_export_keying_material at the HTTP/3 layer, implementing the WebTransport keying material exporter per draft-ietf-webtrans-http3-15 §4.8. The validate_wt_session refactoring to extract common session validation is clean and well-motivated.
Spec correctness: The ExportKeyingMaterialContext encoding (QUIC varint session ID, 8-bit length-prefixed label and context, "EXPORTER-WebTransport" TLS exporter label) is correct per the spec. The 255-byte limit on label/context correctly follows from the 8-bit length fields.
Blocking issue: The Cargo.lock change (removal of the nss-rs git source line) indicates a local path override was active during lock file generation. The underlying Agent::export_keying_material method called at neqo-transport/src/connection/mod.rs:922 must exist in the nss-rs dependency — this PR cannot be merged until the required nss-rs changes are published upstream and the lock file is regenerated against the git source. This was flagged in a prior review and is still present.
Test coverage: Good — covers idempotency, label/context/session-id/output-length variation, 255/256-byte boundaries, zero-length output rejection, invalid session, pre-handshake rejection, and client-server agreement at the transport layer.
Follow-up considerations (non-blocking):
Http3Serverdoes not exposewebtransport_export_keying_material. If the spec requires server-side export (it does — both endpoints should be able to derive keying material), a follow-up PR should add the server counterpart.- The transport-level
export_keying_materialAPI accepts arbitrary labels without enforcing the RFC 8446 §7.5"EXPORTER-"prefix requirement (see inline comment).
jesup
force-pushed
the
users/jesup/exportkey
branch
from
b76faba to
aeb6595
Compare
| @@ -900,7 +900,6 @@ dependencies = [ | |||
| [[package]] | |||
| name = "nss-rs" | |||
| version = "0.8.0" | |||
There was a problem hiding this comment.
Caution
The source line for nss-rs was removed, which makes the lockfile inconsistent with Cargo.toml (which specifies git = "https://github.com/mozilla/nss-rs" at rev = "0.8.0"). Without a source, Cargo treats this as a path dependency and cargo build --locked will fail.
Additionally, export_keying_material does not appear to exist in mozilla/nss-rs at the referenced revision — the transport layer's self.crypto.tls().export_keying_material(...) call won't compile against the published nss-rs 0.8.0.
This needs to be resolved before merge: either a corresponding nss-rs PR needs to land first and the rev bumped, or this Cargo.lock change needs to be reverted and the nss dependency updated to point at a revision that includes the method.
| } | ||
| if out.is_empty() { | ||
| return Err(Error::InvalidInput); | ||
| } |
There was a problem hiding this comment.
Note
This out.is_empty() check is duplicated at the HTTP/3 layer (connection_client.rs:963). Since the transport method is public API and should be self-contained, keeping it here makes sense — but the HTTP/3 layer check is then redundant. Consider removing the one in webtransport_export_keying_material to keep validation in one place.
| /// # Errors | ||
| /// | ||
| /// Returns `Error::InvalidStreamId` if `session_id` does not | ||
| /// correspond to an active WebTransport session, | ||
| /// `Error::InvalidInput` if the label or context exceed 255 | ||
| /// bytes, or `Error::Transport` if the connection is not ready or | ||
| /// the TLS export fails. |
There was a problem hiding this comment.
Note
The # Errors section is slightly inaccurate. When label or context exceed 255 bytes, it returns Error::InvalidInput (from the u8::try_from calls), not Error::Transport. And empty out also yields InvalidInput. Consider:
| /// # Errors | |
| /// | |
| /// Returns `Error::InvalidStreamId` if `session_id` does not | |
| /// correspond to an active WebTransport session, | |
| /// `Error::InvalidInput` if the label or context exceed 255 | |
| /// bytes, or `Error::Transport` if the connection is not ready or | |
| /// the TLS export fails. |
/// # Errors
///
/// Returns `Error::InvalidStreamId` if `session_id` does not
/// correspond to an active WebTransport session,
/// `Error::InvalidInput` if the label or context exceed 255
/// bytes or `out` is empty, or `Error::Transport` if the TLS
/// export fails.
| let long_label = vec![0u8; 256]; | ||
| assert!( | ||
| client | ||
| .webtransport_export_keying_material(session_id, &long_label, b"ctx", &mut [0u8; 32]) | ||
| .is_err() | ||
| ); |
There was a problem hiding this comment.
Tip
The boundary tests (label_too_long, zero_length, invalid_session) only assert .is_err() without verifying the error variant. Matching on the expected variant would catch regressions where the function fails for the wrong reason:
| let long_label = vec![0u8; 256]; | |
| assert!( | |
| client | |
| .webtransport_export_keying_material(session_id, &long_label, b"ctx", &mut [0u8; 32]) | |
| .is_err() | |
| ); |
let long_label = vec![0u8; 256];
assert!(matches!(
client
.webtransport_export_keying_material(session_id, &long_label, b"ctx", &mut [0u8; 32]),
Err(neqo_http3::Error::InvalidInput)
));
>
> Same applies to the other `.is_err()` assertions in the adjacent tests.
| #[test] | ||
| fn wt_export_keying_material() { | ||
| let (mut client, mut server) = connect(); | ||
| let wt_session = create_wt_session(&mut client, &mut server); | ||
| let session_id = wt_session.stream_id(); | ||
|
|
||
| let mut result = [0u8; 32]; | ||
| client | ||
| .webtransport_export_keying_material(session_id, b"my-label", b"my-context", &mut result) | ||
| .unwrap(); |
There was a problem hiding this comment.
Warning
Good test coverage on the client side. However, there's no test verifying that client and server derive identical WebTransport keying material for the same session — which is the fundamental protocol correctness property of the exporter (draft-ietf-webtrans-http3-15 §4.8). The transport-layer test export_keying_material_same_both_sides covers the raw TLS exporter, but the WebTransport context construction could diverge between endpoints.
This would require Http3Server to also expose webtransport_export_keying_material (which it currently doesn't). Even if server-side support is deferred to a follow-up, a tracking issue would help.
There was a problem hiding this comment.
Clean, well-layered addition of ExportKeyingMaterial at the transport and HTTP/3 layers. The validate_wt_session extraction is a nice refactor, the context struct encoding correctly implements draft-ietf-webtrans-http3-15 §4.8 (varint session ID, 8-bit length-prefixed label and context), and the test coverage is solid — including the critical session-scoping property.
Blocking: The Cargo.lock has the source line for nss-rs removed, and export_keying_material does not exist in mozilla/nss-rs at the referenced revision. The PR won't build against the declared dependency. This needs a companion nss-rs change landed first.
Non-blocking observations (detailed in inline comments):
# Errorsdoc is slightly inaccurate regarding which error variants are returned for which failure modesout.is_empty()is checked at both transport and HTTP/3 layers; one is redundant- Boundary tests use
.is_err()without matching the expected error variant - No server-side
webtransport_export_keying_materialyet — worth tracking for completeness
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Martin Thomson <mt@lowentropy.net>
jesup
force-pushed
the
users/jesup/exportkey
branch
from
aeb6595 to
7880eee
Compare
Failed Interop TestsNone ❓ All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as clientNone ❓ neqo-pr as serverNone ❓ Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as clientNone ❓ neqo-pr as serverNone ❓ |
| /// Returns `Error::InvalidStreamId` if `session_id` does not | ||
| /// correspond to an active WebTransport session, | ||
| /// `Error::InvalidInput` if the label or context exceed 255 | ||
| /// bytes, or `Error::Transport` if the connection is not ready or | ||
| /// the TLS export fails. |
There was a problem hiding this comment.
[!NOTE]
The # Errors section is missing the empty out buffer case — Error::InvalidInput is also returned when out.is_empty() (line 963).
| /// Returns `Error::InvalidStreamId` if `session_id` does not | |
| /// correspond to an active WebTransport session, | |
| /// `Error::InvalidInput` if the label or context exceed 255 | |
| /// bytes, or `Error::Transport` if the connection is not ready or | |
| /// the TLS export fails. | |
| /// Returns `Error::InvalidStreamId` if `session_id` does not | |
| /// correspond to an active WebTransport session, | |
| /// `Error::InvalidInput` if `out` is empty or `label`/`context` | |
| /// exceed 255 bytes, or `Error::Transport` if the connection is | |
| /// not ready or the TLS export fails. |
| .export_keying_material(b"EXPORTER-WebTransport", &[], &mut []) | ||
| .is_err() | ||
| ); | ||
| } |
There was a problem hiding this comment.
[!NOTE]
Good coverage of the pre-handshake and zero-length cases. Consider also testing:
- Export during Closing/Draining (should succeed per the
state.closing()guard). - Export after the connection is fully Closed (should fail with
NotConnected).
This would exercise the state boundary that connected() || closing() introduces and document the intended behavior for callers.
| @@ -900,7 +900,6 @@ dependencies = [ | |||
| [[package]] | |||
| name = "nss-rs" | |||
| version = "0.8.0" | |||
There was a problem hiding this comment.
[!CAUTION]
The source line for nss-rs was removed, meaning this lockfile was regenerated against a local path override rather than the upstream git source in Cargo.toml. This will cause cargo build --locked to fail in CI. The lockfile needs to be regenerated once the upstream nss-rs includes export_keying_material.
There was a problem hiding this comment.
Summary
This PR adds export_keying_material at the QUIC transport layer and webtransport_export_keying_material at the HTTP/3 layer, implementing draft-ietf-webtrans-http3-15 §4.8. The ExportKeyingMaterialContext encoding (QUIC varint session ID + 8-bit length-prefixed label/context) correctly matches the spec's wire format. The validate_wt_session extraction from webtransport_create_stream_local is a clean refactoring.
Blocking: The Cargo.lock has lost its source line for nss-rs, indicating it was built against a local path override. This must be fixed before merge (already labeled needs-rebase).
Architectural notes:
Http3Serverhas no correspondingwebtransport_export_keying_material. Per the spec, both endpoints must be able to derive keying material. This could be a follow-up PR, but should be tracked.- The transport-level tests verify client-server agreement (
export_keying_material_same_both_sides), but the WT-layer tests only exercise the client. Once the server-side API exists, a WT-layer agreement test would close this gap.
Test coverage is solid: determinism, label/context/session/length variation, boundary conditions (255 vs 256 bytes), zero-length output, and invalid session IDs are all covered.
No description provided.