feat: add blapi feature to bypass PKCS#11 in RecordProtection

[larseggert]

Add --features blapi which replaces RecordProtection's hot path from PK11_AEADOp (PKCS#11 → softoken → freebl) to direct freebl calls (AES_AEAD, ChaCha20Poly1305_Encrypt/Decrypt), eliminating the substantial sftk_SessionFromHandle mutex and hash-table overheads.

Benchmark: mozilla/neqo#3601 (comment)

[github-actions]

Preliminary review placeholder — full review with inline comments follows.

[Copilot]

Pull request overview

Adds an optional blapi feature that swaps RecordProtection’s AEAD hot path from PKCS#11 (PK11_AEADOp) to direct NSS freebl primitives (AES-GCM / ChaCha20-Poly1305), aiming to reduce session/mutex/hash-table overhead.

Changes:

• Introduces src/freebl.rs with committed FFI bindings to freebl AEAD primitives and scoped pointer wrappers.
• Adds a new RecordProtection implementation under --features blapi that derives keys/IVs and calls freebl directly.
• Adds a shared xor_nonce() helper and wires in the new Cargo feature + build linking behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/lib.rs	Conditionally exposes the new internal freebl module when blapi is enabled.
src/freebl.rs	Adds FFI declarations for freebl AEAD APIs + scoped destructor wrappers.
src/aead.rs	Adds a blapi-gated RecordProtection implementation and refactors nonce XOR logic.
Cargo.toml	Declares the new blapi feature flag.
build.rs	Adds linkage to a freebl static library intended to support the new freebl calls.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Summary

Clean, well-structured performance optimization that bypasses the PKCS#11 session layer for RecordProtection AEAD operations via direct freebl calls. The approach is sound — the blapi feature is opt-in, correctly gated with cfg, and the implementation handles all three TLS 1.3 cipher suites (AES-128-GCM, AES-256-GCM, ChaCha20-Poly1305).

The xor_nonce extraction is a nice deduplication that makes the nonce construction reusable across both the blapi and non-blapi paths.

Blocking

• build.rs: unconditional static=freebl linkage — Both dynamic_link() and pkg_config() emit the freebl link directive regardless of whether blapi is enabled. This will break all dynamic-link builds on systems that don't have libfreebl.a installed (the common case with packaged NSS). Must be gated on CARGO_FEATURE_BLAPI.

Should address

• No test coverage for the blapi path — The existing tests exercise Aead (PKCS#11 path) and the disable-encryption stub. There's no validation that the freebl bypass produces correct ciphertext or round-trips. A single cfg-gated encrypt/decrypt roundtrip test (ideally against a known vector) would catch FFI signature or parameter-ordering bugs — which are the highest-risk class of error in this change.

Minor

• Feature documentation for blapi in Cargo.toml.
• Structural size assertion on CK_GCM_MESSAGE_PARAMS.
• Pointer derivation style consistency in encrypt.

Thread safety: both the blapi and original RecordProtection are !Send + !Sync (via scoped_ptr!), which is the correct behavior for non-thread-safe freebl/softoken contexts.

[github-actions]

Adding inline comments.

[github-actions]

Summary

Sound approach — bypassing the PKCS#11 session layer for a well-scoped hot path (AEAD encrypt/decrypt) with an opt-in feature flag is clean and low-risk. The FFI signatures match NSS's blapi.h, the scoped_ptr! wrappers handle context lifetimes correctly, and the xor_nonce extraction is a nice DRY improvement shared by both paths.

All prior review feedback has been addressed in the latest commit: build.rs linkage is now feature-gated, Cargo.toml documents the FIPS implication, the CK_GCM_MESSAGE_PARAMS size assertion is in place, the encrypt pointer derivation uses the single-as_mut_ptr pattern, and CI runs cargo test --features blapi.

New observations (see inline comments)

• pkg_config() + static=freebl: The pkg_config code path (system-packaged NSS) links static=freebl, but distro packages typically don't ship libfreebl.a. This will break --features blapi for consumers not building NSS from source.
• blapi + disable-encryption: Enabling both features silently drops blapi in favour of the null AEAD. A compile_error! would make this explicit rather than a silent no-op.
• CI powerset scope: cargo hack --feature-powerset runs on every matrix entry; gating on one toolchain would cut CI time without losing coverage.

Not flagged (verified correct)

• AES-GCM uses separate encrypt/decrypt contexts; ChaCha20 correctly shares one (stateless per-message nonce).
• Key derivation goes through SSL_HkdfExpandLabelWithMech (same function the PKCS#11 path uses internally), so the derived key material is identical — only the AEAD context differs.
• In-place encrypt/decrypt pointer arithmetic is within bounds (verified against the pre-checks).
• Thread safety: blapi RecordProtection is !Send + !Sync via scoped_ptr!, matching the non-blapi variant.

[martinthomson]

A few preliminary comments.

[github-actions]

Summary

Clean, well-scoped performance optimization. The approach is sound: an opt-in feature flag that replaces the PKCS#11 session layer (PK11_AEADOp → sftk_SessionFromHandle + mutex) with direct freebl calls for the three TLS 1.3 cipher suites. The FFI signatures match NSS's blapi.h, the scoped_ptr! wrappers handle context lifetimes correctly (and keep RecordProtection !Send + !Sync), and key derivation goes through the same SSL_HkdfExpandLabelWithMech as the PKCS#11 path, ensuring identical key material.

The xor_nonce extraction is a good DRY improvement shared across all paths.

Prior feedback addressed

Build.rs linkage is now feature-gated, Cargo.toml documents the FIPS implication, the CK_GCM_MESSAGE_PARAMS size assertion is in place, encrypt uses the single-as_mut_ptr derivation pattern, and CI runs cargo test --features blapi. The integration test in tests/aead.rs exercises RecordProtection with known TLS vectors (including expected ciphertext bytes), which validates correctness of the blapi path — a key concern from earlier reviews.

Remaining items (see inline comments)

• blapi + disable-encryption interaction: silently drops blapi with no compile-time signal. A compile_error! would make this explicit.
• pkg_config() + static=freebl: system-packaged NSS typically doesn't ship libfreebl.a; this will break --features blapi for consumers not building NSS from source.
• Code duplication: as @martinthomson noted, the blapi and non-blapi RecordProtection modules share significant structure (bounds checks, nonce construction, method signatures). Refactoring to share the outer method logic — differing only in the crypto dispatch — would reduce the maintenance surface. Context specialization (single-direction AES context) would be a natural part of that refactoring.
• CI powerset scope: cargo hack --feature-powerset on a single toolchain would cut CI time without losing coverage.

[github-actions]

Summary

Sound, well-scoped optimization. Bypassing the PKCS#11 session layer (PK11_AEADOp → sftk_SessionFromHandle + mutex) for an opt-in hot path is a clean approach. The FFI signatures match NSS's blapi.h, unsafe blocks have correct bounds checks and documented safety contracts, and the scoped_ptr! wrappers keep RecordProtection correctly !Send + !Sync. The xor_nonce extraction is a nice DRY improvement shared across all paths.

Key derivation correctly goes through SSL_HkdfExpandLabelWithMech (same function the PKCS#11 path uses internally), so the derived key material is identical — only the AEAD dispatch differs. The compile_error! for blapi + disable-encryption, feature-gated linkage in build.rs (static loader for standalone, dylib=freebl3 for pkg-config), and cargo hack --mutually-exclusive-features are all well done.

Remaining items

• Code sharing (see inline comments): @martinthomson's structural feedback about sharing code between the blapi and non-blapi modules and direction specialization is still open and would reduce the maintenance surface significantly. The two RecordProtection implementations share bounds checks, nonce construction, method signatures, and error handling — differing only in the crypto dispatch.

• Test coverage gap (see inline comment on tests/aead.rs): The integration tests only exercise AES-128-GCM. The ChaCha20-Poly1305 branch in aead_op (different FFI function, different parameter layout — notably tagOut/tagIn as a separate argument vs. embedded in CK_GCM_MESSAGE_PARAMS) is completely untested through the blapi path.

• API contract change: The blapi RecordProtection is uni-directional (runtime error on direction mismatch), unlike the non-blapi variant. This is fine for neqo's usage but should be documented since the public type signature is unchanged.

Verified correct (not flagged)

• In-place encrypt/decrypt pointer arithmetic is within bounds (checked against the pre-conditions).
• Tag pointer derivation uses a single as_mut_ptr() + offset, avoiding aliasing issues.
• AES_CreateContext copies the key material, so the temporary key_data() buffer can safely drop.
• The CK_GCM_MESSAGE_PARAMS size assertion correctly skips Windows (LLP64 mixed pointer/ulong widths); #[repr(C)] handles the layout on all platforms.

[github-actions]

Summary

Sound, well-scoped optimization. The approach — an opt-in feature flag that replaces the PKCS#11 session layer with direct freebl calls for the three TLS 1.3 AEAD cipher suites — is clean and correctly implemented. The FFI signatures match NSS's blapi.h, the unsafe blocks have accurate safety contracts and correct bounds checks, and the scoped_ptr! wrappers keep RecordProtection correctly !Send + !Sync.

Prior review feedback has been addressed well: build.rs linkage is feature-gated (with the correct static=freebl vs dylib=freebl3 split between standalone and pkg-config paths), compile_error! guards against blapi + disable-encryption, CI runs cargo test --features blapi, and the cargo hack --mutually-exclusive-features flag is in place. The known-ciphertext test in aead_encrypt_decrypt implicitly validates the blapi AES-128-GCM path when run under --features blapi, and the new roundtrip_aes256/roundtrip_chacha20 tests cover the remaining cipher suites.

Verified correct (not flagged)

• In-place encrypt/decrypt pointer arithmetic is within bounds (verified against the pre-condition checks). Tag pointer derivation uses the single as_mut_ptr() + offset pattern, avoiding aliasing issues.
• AES_CreateContext copies the key material, so the temporary key_data() buffer can safely drop without invalidating the context.
• AES-GCM correctly uses separate encrypt/decrypt contexts; ChaCha20 correctly shares one (stateless per-message nonce).
• Key derivation goes through SSL_HkdfExpandLabelWithMech (same as the PKCS#11 path), ensuring identical key material.
• The CK_GCM_MESSAGE_PARAMS size assertion correctly skips Windows (LLP64 mixed pointer/ulong widths); #[repr(C)] handles the layout on all platforms.
• The xor_nonce extraction is a clean DRY improvement; Aead::make_nonce's refactored semantics are equivalent to the original in-place XOR.

Remaining design concerns

• Code sharing: @martinthomson's feedback on reducing duplication between the blapi and non-blapi RecordProtection modules is the main open structural concern. See inline comment for a concrete suggestion.
• Direction specialization: @martinthomson's suggestion to avoid constructing the unused AES context (since callers are exclusively encrypt or decrypt) would reduce memory and construction cost.
• Key material lifecycle: See inline comment on src/aead.rs:474 — the blapi path exposes raw key bytes in Rust heap memory that isn't zeroed on drop, unlike the PKCS#11 path.

[larseggert]

@martinthomson could you take another look?

[martinthomson]

This looks pretty good now.

Just one thing before landing: confirm that flipping NSS into FIPS mode doesn't cause this whole thing to fail badly. It would be a shame if that were the case. I'm concerned that we don't see much FIPS-enabled instances of Firefox, but those will continuously fail to establish a QUIC connection, because we will fail to handshake. They will fall back to h2 and so the problem is hidden, but there will be tons of failed QUIC attempts in the process.

(If it does work out poorly, then we might be able to salvage something by making the code switch between the freebl and PKCS#11 APIs at runtime.)

[github-actions]

Well-scoped, opt-in performance optimization that cleanly replaces the PKCS#11 session layer with direct freebl calls for TLS 1.3 AEAD. The approach is sound: feature-gated, compile_error! guards mutual exclusion with disable-encryption, and the RecordProtectionOps trait extraction enforces API consistency across all three backends at compile time.

The latest commit adds FIPS-mode integration tests, addressing @martinthomson's final concern. The test design (separate binary via aead_fips.rs, graceful skip on non-certified builds) is appropriate.

Inline comments flag one technical soundness issue: the ChaChaOpFn type alias uses *mut c_uchar for the tag parameter, but ChaCha20Poly1305_Open expects *const c_uchar — calling through the wrong function pointer type is technically UB. See inline for fix options.

Prior reviewer feedback on code deduplication between backends and bool → Mode in chacha20_poly1305_op remains open but non-blocking per the existing approval.

[github-actions]

Summary

Well-scoped, opt-in performance optimization. The approach — bypassing the PKCS#11 session layer (PK11_AEADOp → sftk_SessionFromHandle + mutex) with direct freebl calls via FREEBL_GetVector() — is clean and correctly implemented for all three TLS 1.3 AEAD cipher suites.

What works well:

• The RecordProtectionOps trait extraction enforces compile-time API consistency across all three backends (recprot.rs, recprot_blapi.rs, recprot_null.rs).
• Feature gating is thorough: compile_error! for mutual exclusion with disable-encryption, cargo hack --mutually-exclusive-features in CI, and the FREEBL_GetVector() length check at runtime.
• The expand_label_buf<const N: usize> helper and split_tag extraction are nice DRY improvements shared across backends.
• The FIPS-mode integration test (aead_fips.rs) directly addresses @martinthomson's final concern. The separate-binary approach via tests/ and graceful skip on non-certified builds is appropriate.

Remaining:

• See inline comment on ChaChaOpFn — the type alias uses *mut c_uchar for the tag parameter, but ChaCha20Poly1305_Open expects *const. This is a soundness issue worth fixing before merge.
• @martinthomson's structural feedback on code sharing between backends is still open. The two RecordProtection implementations share bounds checks, nonce construction, and error handling — differing only in the crypto dispatch. Could be a follow-up.

[github-actions]

Summary

Good performance optimization — bypassing the PKCS#11 session layer for the AEAD hot path is well-motivated and the architecture is clean: RecordProtectionOps trait for compile-time backend contract enforcement, compile_error! for mutually exclusive features, const-generic expand_label_buf, and the FREEBL_GetVector() approach for portable symbol resolution.

The refactoring of RecordProtection from bidirectional (two contexts) to unidirectional (one context + Mode) is a nice simplification that applies across all backends. CI coverage via cargo-hack --feature-powerset and --mutually-exclusive-features is thorough.

Blocking

• FREEBLVectorPartial soundness: &*ptr is created before validating v.length, which is UB if the installed NSS has a shorter vector than the Rust struct expects. See inline comment for fix.
• ChaChaOpFn type mismatch: The *mut c_uchar tag parameter doesn't match ChaCha20Poly1305_Open's const unsigned char *tagIn. This was flagged in prior reviews and is still present.

Non-blocking observations

• The RecordProtectionOps trait + Mode parameter are breaking API changes, appropriately covered by the 0.11 → 0.12 version bump.
• @martinthomson's feedback on reducing code duplication between recprot.rs and recprot_blapi.rs (the four RecordProtectionOps method bodies are structurally identical, differing only in the aead_op dispatch) is worth revisiting — a generic-over-crypto-backend approach could halve the AEAD method code.
• key_data() extracts raw key bytes to the Rust heap without zeroing on drop. For neqo's non-FIPS use case this is acceptable, but worth documenting as a known trade-off if this crate is used in more sensitive contexts.