diff --git a/crates/didbot-agentd/src/sessions.rs b/crates/didbot-agentd/src/sessions.rs index 7c053a01..35a12776 100644 --- a/crates/didbot-agentd/src/sessions.rs +++ b/crates/didbot-agentd/src/sessions.rs @@ -49,6 +49,14 @@ //! session until after it has written what the refresh returned, and a //! lock file held for the store's life keeps a second process out entirely. //! A refresh the server refuses for good takes the session out of the store. +//! +//! # Ending one +//! +//! [`SessionStore::end`] revokes the session where it was issued before it +//! deletes the file, because the tokens in that file go on working at the +//! authorization server after the file is gone. A refresh the server has +//! already refused for good is the one case with nothing to revoke: the grant +//! is over, and `update` deletes it. use std::collections::{BTreeMap, HashMap}; use std::fs::File; @@ -138,6 +146,17 @@ impl Session { pub fn into_data(self) -> ClientSessionData { self.data } + + /// Where the authorization server takes a revocation, if it advertised + /// one when this session was made (RFC 7009). + /// + /// `None` is not "this session cannot be revoked", it is "there is + /// nowhere to send one": the server said nothing about a revocation + /// endpoint in its metadata. This deployment's own authorization server + /// is one of those, by the decision in `plan/oauth.md`. + pub fn revocation_endpoint(&self) -> Option<&str> { + self.data.authserver_revocation_endpoint.as_deref() + } } impl std::fmt::Debug for Session { @@ -382,13 +401,48 @@ impl SessionStore { self.current_ref().values().cloned().collect() } - /// Stops holding the session under `key`, on disk and in memory. + /// Ends the session under `key`: revokes it at the server that issued it, + /// then stops holding it, on disk and in memory. + /// + /// Deleting the file ends this daemon's copy and nothing else. The refresh + /// token in it keeps working at the authorization server until it expires + /// or until a reuse ends its family, so a sign-out that only deleted would + /// leave a live credential behind wherever a copy of the file had got to. + /// + /// `revoke` is what makes the request. The store holds no OAuth client and + /// building one here would be a second implementation of a call + /// `jacquard_oauth::request::revoke` already makes, so the round trip + /// belongs to the caller the same way a refresh does in + /// [`SessionStore::update`]. It is called only for a session whose issuer + /// advertised somewhere to send one — see [`Session::revocation_endpoint`]. /// - /// `false` when there was none. - pub async fn remove(&self, key: &Key) -> Result { + /// The session goes whatever `revoke` answered. A server that refused or + /// could not be reached is logged and does not keep a session the caller + /// asked to end. + /// + /// `false` when there was no session under `key`. + pub async fn end(&self, key: &Key, revoke: F) -> Result + where + F: FnOnce(Session) -> Fut, + Fut: Future>, + E: std::error::Error + 'static, + { let lock = self.lock_for(key); let gone = { let _held = lock.lock().await; + match self.get(key) { + Some(session) if session.revocation_endpoint().is_some() => { + match revoke(session).await { + Ok(()) => { + debug!(account = %key.account, client_id = %key.client_id, "revoked a session where it was issued") + } + Err(err) => { + warn!(account = %key.account, client_id = %key.client_id, error = %err, "could not revoke a session where it was issued; ending it here anyway") + } + } + } + _ => {} + } self.forget(key) }; self.release(key, &lock); @@ -738,6 +792,19 @@ mod tests { #[error("{0}")] struct Said(&'static str); + /// The revoker for a session nobody advertised an endpoint for, which is + /// every session these tests build unless they say otherwise. + async fn never(session: Session) -> Result<(), Said> { + panic!("nothing to revoke: {session:?} names no revocation endpoint"); + } + + /// The same session, with an issuer that does advertise one. + fn revocable(session: &Session) -> Session { + let mut data = session.reveal().clone(); + data.authserver_revocation_endpoint = Some("https://pds.example/oauth/revoke".into()); + Session::new(session.client_id.clone(), data) + } + #[tokio::test] async fn a_session_comes_back_after_a_restart_with_its_dpop_key() { let scratch = Scratch::new("sessions-restart"); @@ -773,8 +840,11 @@ mod tests { assert_eq!(store.get(&one.key()).as_ref(), Some(one)); } - assert!(store.remove(&all[1].key()).await.unwrap()); - assert!(!store.remove(&all[1].key()).await.unwrap(), "already gone"); + assert!(store.end(&all[1].key(), never).await.unwrap()); + assert!( + !store.end(&all[1].key(), never).await.unwrap(), + "already gone" + ); drop(store); let store = SessionStore::open(&scratch.0).unwrap(); @@ -964,7 +1034,7 @@ mod tests { let key = held.key(); store.put(held).await.unwrap(); - assert!(store.remove(&key).await.unwrap()); + assert!(store.end(&key, never).await.unwrap()); assert!(store.locks.lock().unwrap().is_empty(), "removed"); let missing = store @@ -980,6 +1050,67 @@ mod tests { ); } + /// Deleting the file ends this daemon's copy of a session. The refresh + /// token in it goes on working at the authorization server until it + /// expires or a reuse ends its family, so a sign-out that only deleted + /// leaves a live credential behind and nothing holding it. + #[tokio::test] + async fn ending_a_session_revokes_it_where_it_was_issued() { + let scratch = Scratch::new("sessions-end"); + let store = SessionStore::open(&scratch.0).unwrap(); + let held = revocable(&session(APP, AGENT, "access-1", "refresh-1")); + let key = held.key(); + store.put(held.clone()).await.unwrap(); + + let presented = Arc::new(Mutex::new(None)); + let seen = Arc::clone(&presented); + assert!(store + .end(&key, |session| async move { + *seen.lock().unwrap() = Some(( + refresh_token(&session), + session.revocation_endpoint().map(str::to_owned), + )); + Ok::<(), Said>(()) + }) + .await + .unwrap()); + + let (token, endpoint) = presented.lock().unwrap().clone().expect("revoked"); + assert_eq!( + token, "refresh-1", + "the token the server has to be told about" + ); + assert_eq!( + endpoint.as_deref(), + Some("https://pds.example/oauth/revoke") + ); + assert_eq!(store.get(&key), None); + assert!(!store.path_of(&key).exists()); + } + + /// A revocation that did not land must not keep a session the caller has + /// asked to end: the local copy is the part this daemon controls, and + /// leaving it would go on presenting a credential the caller disowned. + #[tokio::test] + async fn a_session_ends_here_even_when_the_server_will_not_revoke_it() { + let scratch = Scratch::new("sessions-end-refused"); + let store = SessionStore::open(&scratch.0).unwrap(); + let held = revocable(&session(APP, AGENT, "access-1", "refresh-1")); + let key = held.key(); + store.put(held).await.unwrap(); + + assert!(store + .end(&key, |_| async { Err(Said("unreachable")) }) + .await + .unwrap()); + assert_eq!(store.get(&key), None); + assert!(!store.path_of(&key).exists()); + drop(store); + + let store = SessionStore::open(&scratch.0).unwrap(); + assert!(store.list().is_empty(), "not back after a restart"); + } + /// A format bump and then a rollback leaves every session file naming a /// format the older binary refuses, and one damaged file does the same to /// every intact session beside it. Either way a host that came back diff --git a/plan/credentials.md b/plan/credentials.md index e9494acb..dcf0a6c1 100644 --- a/plan/credentials.md +++ b/plan/credentials.md @@ -103,6 +103,8 @@ section decides what replaces it before any of it is built. context store and the poller: `SessionStore::update` holds a lock per session across a refresh, so a rotated refresh token has one place to land, and a refresh refused for good removes the session. + `SessionStore::end` revokes a session where it was issued before the + file goes, because the tokens in it outlive the file. - [x] **At rest, file permissions alone.** The node key sits in the same directory under the same permissions, so a key derived from it protects nothing from whoever can read either file. The OS keyring