feat: remove Rc<RefCell<Context>> from hp::Key

[larseggert]

Replace Aes(Rc<RefCell<Context>>) with Aes { ctx: Option<Context>, key: SymKey }, eliminating the RefCell::borrow_mut overhead on every call to Key::mask.

PK11_CloneContext is not supported for AES-ECB contexts, so Clone stores the SymKey (via PK11_ReferenceSymKey, O(1)) and sets ctx to None. The context is recreated lazily on the first call to mask after a clone, surfacing any NSS error there rather than panicking in Clone.

mask now takes &mut self; neqo will need a corresponding update.

[github-actions]

Dismissing empty pending review to resubmit with inline comments.

[Copilot]

Pull request overview

This PR refactors QUIC header-protection key handling (hp::Key) to avoid Rc<RefCell<Context>> by storing the AES SymKey and (re)creating the AES-ECB PK11Context lazily after cloning, which removes RefCell::borrow_mut overhead on each mask() call.

Changes:

• Replace Key::Aes(Rc<RefCell<Context>>) with Key::Aes { ctx: Option<Context>, key: SymKey } and implement a custom Clone that references the SymKey and resets ctx.
• Change Key::mask to take &mut self and lazily initialize the AES context when ctx is None.
• Update the HP tests to use mutable keys when calling mask().

Reviewed changes

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

File	Description
src/hp.rs	Refactors AES HP key storage/clone behavior and makes mask() mutable with lazy AES context initialization.
tests/hp.rs	Adjusts tests to accommodate mask(&mut self, ...) and validates cloned keys still produce identical masks.

---

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

[github-actions]

Clean, well-motivated change. Replacing Rc<RefCell<Context>> with owned Option<Context> + SymKey is the right trade-off — it removes interior-mutability overhead, eliminates the RefCell::borrow_mut on every mask call, and makes the API honestly &mut self. The lazy context recreation via make_aes_ctx is correct, and the switch from SECItemBorrowed::wrap(&ZERO[..0]) to SECItemBorrowed::make_empty() is a nice cleanup.

No correctness or security issues found. Two minor style suggestions inline (neither blocking). One additional note:

Re @Copilot's observation: since Key is a pub enum with public variants, callers can construct Key::Aes { ctx: None, .. } directly. The doc on line 60 ("ctx is None only between clone() and the first call to mask") is therefore not an enforceable invariant. Consider rewording to "mask lazily initializes ctx when it is None" to match the actual guarantee. Making the variants #[non_exhaustive] would harden this further, though it may be overkill for the current consumer base.

[github-actions]

Well-motivated change. Replacing Rc<RefCell<Context>> with owned Option<Context> + SymKey is correct: it removes interior-mutability overhead, honestly expresses the &mut self requirement, and the lazy context recreation via make_aes_ctx is sound. The version bump to 0.10.0 is appropriate for the &self → &mut self breaking change on mask.

Verified:

• Thread safety: Context and SymKey are !Send/!Sync (raw pointers from scoped_ptr!), same as the old Rc-based type — no auto-trait regression.
• Clone correctness: SymKey::clone uses PK11_ReferenceSymKey (O(1) ref-count bump); Context intentionally has no Clone, so lazy recreation is the right approach.
• Error semantics: If make_aes_ctx fails after clone, ctx stays None and the next mask call will retry — correct behaviour for transient NSS errors.
• SAFETY: The "Fixes" commit adopted the earlier suggestion to ground the unsafe argument in aliasing (Deref yields a pointer copy, &mut self guarantees exclusion) rather than AES-ECB statefulness. This is the right framing.

Two minor style suggestions inline (neither blocking). The Option::insert cleanup from the prior review round looks good.

[github-actions]

Clean, well-motivated change. Replacing Rc<RefCell<Context>> with owned Option<Context> + SymKey is the right trade-off: it removes interior-mutability overhead, makes the &mut self requirement honest, and the lazy context recreation via make_aes_ctx is sound. All feedback from prior review rounds has been addressed (SAFETY comment rewording, Option::insert, doc trimming, #[non_exhaustive]).

A few notes on the overall shape:

• #[non_exhaustive] is an additional API break beyond the &self → &mut self change on mask. It prevents external construction and exhaustive destructuring of both variants. This is the right call for a crypto API (prevents constructing a Key::Aes with arbitrary state), but the PR description only mentions the mask signature change. Worth calling out in release notes since downstream code that pattern-matches on Key::Chacha(k) will also need updating.

• Version bump to 0.10.0 is appropriate for the breaking changes.

• Test coverage is adequate: hp_test exercises the normal path, lazy-init-after-clone path, and continued use of the original key post-clone, across all three cipher suites.

Two minor doc/style suggestions inline — neither blocking.

[martinthomson]

I'm going to suggest that this attempt to protect the clone() implementation is an error. Instead, I suggest that you remove the Option around the context and make a fallible clone method. We only invoke the clone in one place in neqo and that could be replaced by a fallible function.

Then, you can construct the context when creating the thing. And when duplicating it. That one place in next() where clone() is called can be replaced with an invocation of the fallible duplication function. Then the mask function doesn't need &mut self and we're good.

[martinthomson]

Much nicer.