feat(s2n-quic-dc): Add UPS replay to path secret map#3089
Conversation
serdarhas
force-pushed
the
serdarsh/dc-ups-emission
branch
from
7257b38 to
58dc96f
Compare
serdarhas
force-pushed
the
serdarsh/dc-ups-emission
branch
from
c0fb146 to
eb72730
Compare
I'm confused, doesn't this PR implement the persistence logic? |
| let rehandshake_period = Duration::from_secs(3600 * 24); | ||
|
|
||
| let persistence = persistence_directory.and_then(|dir| { | ||
| match super::persistence::PersistenceStore::new(dir) { |
There was a problem hiding this comment.
Should we be enforcing that each Map uses a different directory for persistence?
| C: 'static + time::Clock + Send + Sync, | ||
| S: event::Subscriber, | ||
| { | ||
| let signer = self.signer.unwrap_or_else(stateless_reset::Signer::random); |
There was a problem hiding this comment.
I don't think falling back to the random Signer will work as expected (will not work across restarts, see
). While the fallback scenario is probably unlikely, I thinkbuild here should enforce passing in a signer by returning a Result.
| builder = builder.with_persistence_directory(dir); | ||
| } | ||
|
|
||
| let store = builder.build().unwrap(); |
There was a problem hiding this comment.
Similarly, this should return a Result instead of unwrapping.
There was a problem hiding this comment.
Hm, just noticed that Map::new also unwraps, not sure if that should also be fixed or if there was some rationale.
| match super::persistence::PersistenceStore::new(dir) { | ||
| Ok(store) => Some(store), | ||
| Err(e) => { | ||
| tracing::warn!("persistence: failed to initialize store: {:?}", e); |
There was a problem hiding this comment.
Will we be adding events here for observability into failures? Not sure if that is planned as a follow up.
| } | ||
|
|
||
| *last = current.keys().copied().collect(); | ||
| } |
There was a problem hiding this comment.
Should we be fsync-ing the journal too?
| @@ -0,0 +1,341 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
nit: Can we follow the convention in the repo of having a named module file (persistence.rs, here) and only the test file in the module folder? See map/rehandshake for example.
serdarhas
force-pushed
the
serdarsh/dc-ups-emission
branch
from
eb72730 to
9bc18fb
Compare
Capture the live (credential_id, peer) set during the cleaner's existing eviction walk and write it to a per-Map snapshot file. At startup, a free-function replay() walks the persistence directory, dedupes by credential_id, and emits a rate-limited UnknownPathSecret to each peer so clients re-handshake before the server admits application traffic. - persistence.rs: Writer (tmp + fsync + atomic rename, orphan-tmp sweep) and replay() primitive; per-Map peers-<pid>-<n>.txt plaintext format. - cleaner.rs: capture retained entries inline in queue.retain every PERSISTENCE_CYCLES-th cycle; no extra map walk or lock. - state.rs/store.rs/map.rs: thread peer_persistence_dir through the builder; test_run_cleaner hook for deterministic snapshot tests. Replaces the earlier snapshot/journal spike with a single fsynced snapshot file (no journal), a named module file (persistence.rs) per repo convention, and per-Map file naming to avoid directory collisions.
serdarhas
force-pushed
the
serdarsh/dc-ups-emission
branch
from
9bc18fb to
7f76965
Compare
| ) | ||
| } | ||
|
|
||
| pub fn new_with_persistence<C, S>( |
There was a problem hiding this comment.
rather than creating a bunch of new methods for this, we should change it to just take a State::builder. Having a proliferation of one-off methods with extra arguments isn't going to scale.
3 participants
Release Summary:
This feature is to add UnknownPathSecret (UPS) emission during startup of s2n-quic server to reset client-side path secret Entries
Description of changes:
Adds a new
replay_unknown_path_secretsmethod to s2n-quic map, and adds a new builder option to inject an observer to move client persistence outside of s2n-quic.Persistence is linked to the map cleaner loop where we can save snapshots and diffs to file through the observer.
replay_unknown_path_secretsloops through a series of entries, constructs a UPS packet, and sends it to the client.This feature relies on clients having
should_evict_on_unknown_path_secretenabled.Call-outs:
The implementation here pulls out the persistance logic for external libraries to deal with. This was chosen instead of the subscriber to de-couple persistence from the handshake hot-path, and to avoid having s2n-quic take on additional functionality to write to disk as a networking library.
Testing:
Unit tests + external library tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.