ts_tunnel: cleanup expired sessions that no longer receive traffic#264
ts_tunnel: cleanup expired sessions that no longer receive traffic#264danderson wants to merge 1 commit into
Conversation
Updates #17 Signed-off-by: David Anderson <[email protected]> Change-Id: I972c8c4833e5b2930462dba2487e15cc6a6a6964
nrc
left a comment
nrc
left a comment
There was a problem hiding this comment.
LGTM, a bunch of tiny and optional suggestions inline, none of them are very important.
| self.session_cleanup = Some(endpoint.scheduler.add( | ||
| TimeRange::new(expiry, expiry + SESSION_CLEANUP_GRACE), | ||
| Event::ExpireSession(self.id), | ||
| )); |
There was a problem hiding this comment.
nit: this repeated code could be factored out since it is computing timeouts it seems like code which could get out of sync. Only two cases, so borderline though
|
|
||
| /// Grace time for cleaning up a session that has exceeded [`SESSION_LIFETIME`]. | ||
| /// | ||
| /// Once a session has expired, we need to delete it key material to ensure forward secrecy |
There was a problem hiding this comment.
| /// Once a session has expired, we need to delete it key material to ensure forward secrecy | |
| /// Once a session has expired, we need to delete its key material to ensure forward secrecy |
|
|
||
| pub fn expired(&self, now: Instant) -> bool { | ||
| now.duration_since(self.created) > Duration::from_secs(240) // TODO: constants | ||
| now.duration_since(self.created) > SESSION_LIFETIME |
There was a problem hiding this comment.
Nit: the tests could use the constants +/- a delta
| /// Endpoints start a new handshake to rotate onto fresh session keys once a session | ||
| /// has been alive for this long, if it's still exchanging traffic. The session can | ||
| /// continue to be used while the rotation handshake proceeds, up to [`SESSION_LIFETIME`]. | ||
| pub const SESSION_FRESH_LIFETIME: Duration = Duration::from_secs(120); |
There was a problem hiding this comment.
COOKIE_ROTATION_TIME (in ts_tunnel/src/macs.rs) is also 120 seconds, should these be a single const or is it just coincidence?
| } | ||
| } | ||
|
|
||
| fn expire_sessions(&mut self, endpoint: &mut EndpointState) { |
There was a problem hiding this comment.
I wonder if this function should be used in get_recv to avoid duplicating the 'is session expired' logic? Not sure exactly how that would work, maybe as simple as calling this function from there then assuming things are not expired?
| { | ||
| recv_prev | ||
| .take() | ||
| .inspect(|recv_prev| endpoint.ids.remove_session(recv_prev.id())); |
There was a problem hiding this comment.
This idiom is used a few times and might be worth factoring out, not sure if it can be done nicely or if it is worthwhile.
2 participants
Updates #17
Change-Id: I972c8c4833e5b2930462dba2487e15cc6a6a6964