mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(core): keep refused token rotations in the kernel keyring
A rotation the Secret Service refused lived only in the daemon's memory, so a crash or SIGKILL before the retry landed lost the only live refresh token. Keep it in the kernel keyring as well until it is written. The next resume, by the daemon or `pdfs unlock`, puts it back in when the stored blob still holds the refresh token it spent, and writes it; one left over from an older login is dropped. `kernelkey` now holds named keys, the passphrases and the unsaved tokens (B61).
This commit is contained in:
1 parent
0ac5928138
commit
3458e2e72d
3 files changed
+210
-44
No files matched your search
+151
-20
@@ -364,11 +364,11 @@ fn keep_passphrases(
|
||||
let outcome = if remember {
|
||||
stored.key_passphrases = Some(encode(passphrases));
|
||||
// Best-effort: a system without a kernel keyring has nothing to clear.
|
||||
let _ = kernelkey::clear();
|
||||
let _ = kernelkey::PASSPHRASES.clear();
|
||||
Ok(())
|
||||
} else {
|
||||
stored.key_passphrases = None;
|
||||
kernelkey::store(&encode_secret(passphrases)).map_err(|e| {
|
||||
kernelkey::PASSPHRASES.store(&encode_secret(passphrases)).map_err(|e| {
|
||||
Error::Other(format!(
|
||||
"the kernel keyring is unavailable, so the unlock cannot be kept for this session: {e}"
|
||||
))
|
||||
@@ -469,7 +469,7 @@ fn find_passphrases(stored: &StoredSession) -> Result<Option<(KeyPassphrases, Ke
|
||||
if let Some(found) = stored.key_passphrases.as_ref().and_then(decode) {
|
||||
return Ok(Some((found, KeyState::Stored)));
|
||||
}
|
||||
match kernelkey::load() {
|
||||
match kernelkey::PASSPHRASES.load() {
|
||||
Ok(Some(secret)) => Ok(decode_secret(&secret).map(|p| (p, KeyState::Session))),
|
||||
Ok(None) => Ok(None),
|
||||
// No keyring on this system: nothing was ever kept there.
|
||||
@@ -495,8 +495,7 @@ pub async fn unlock(mailbox: &str, remember: bool) -> Result<()> {
|
||||
return Err(Error::ReloginRequired);
|
||||
}
|
||||
let mailbox = Zeroizing::new(mailbox.to_owned());
|
||||
let session = ProtonApiSession::resume(client_config(), stored.to_params())?;
|
||||
register_refresh_handler(&session, stored.refresh_token.clone());
|
||||
let session = resume_stored(&stored)?;
|
||||
let passphrases =
|
||||
derive_verified(&session, mailbox.as_bytes(), stored.key_salts.clone()).await?;
|
||||
// Read again: verifying may have rotated the tokens, and the copy read
|
||||
@@ -507,7 +506,7 @@ pub async fn unlock(mailbox: &str, remember: bool) -> Result<()> {
|
||||
/// Drop the passphrases held in kernel memory, so the account is locked again
|
||||
/// until [`unlock`]. With `forget`, also those in the keyring blob.
|
||||
pub fn lock(forget: bool) -> Result<()> {
|
||||
kernelkey::clear()?;
|
||||
kernelkey::PASSPHRASES.clear()?;
|
||||
if forget {
|
||||
let mut stored = load()?;
|
||||
if stored.key_passphrases.take().is_some() {
|
||||
@@ -535,9 +534,10 @@ pub fn logout() -> Result<()> {
|
||||
let _ = std::fs::remove_file(format!("{}{suffix}", path.display()));
|
||||
}
|
||||
}
|
||||
if let Err(e) = kernelkey::clear() {
|
||||
if let Err(e) = kernelkey::PASSPHRASES.clear() {
|
||||
tracing::warn!(error = %e, "clearing the kernel-keyring unlock on logout failed");
|
||||
}
|
||||
let _ = kernelkey::UNSAVED_TOKENS.clear();
|
||||
match keyring_entry()?.delete_credential() {
|
||||
Ok(()) | Err(keyring_core::Error::NoEntry) => Ok(()),
|
||||
Err(e) => Err(e.into()),
|
||||
@@ -557,8 +557,7 @@ pub fn logout() -> Result<()> {
|
||||
/// a daemon starting offline can still mount from its cache.
|
||||
pub async fn resume_client() -> Result<(ProtonDriveClient, ProtonApiSession)> {
|
||||
let stored = load()?;
|
||||
let session = ProtonApiSession::resume(client_config(), stored.to_params())?;
|
||||
register_refresh_handler(&session, stored.refresh_token.clone());
|
||||
let session = resume_stored(&stored)?;
|
||||
|
||||
let passphrases = match find_passphrases(&stored)? {
|
||||
Some((passphrases, _)) => passphrases,
|
||||
@@ -573,6 +572,48 @@ pub async fn resume_client() -> Result<(ProtonDriveClient, ProtonApiSession)> {
|
||||
Ok((client, session))
|
||||
}
|
||||
|
||||
/// Resume `stored`, with the rotation the keyring refused last time put in
|
||||
/// first, if there is one: Proton has spent the stored refresh token then,
|
||||
/// and the kernel keyring holds the one that replaced it (B61).
|
||||
fn resume_stored(stored: &StoredSession) -> Result<ProtonApiSession> {
|
||||
let unsaved = recover_unsaved(stored);
|
||||
let mut resumed = stored.clone();
|
||||
if let Some(tokens) = &unsaved {
|
||||
tracing::info!("resuming with session tokens the keyring refused last time");
|
||||
resumed.access_token = tokens.access_token.clone();
|
||||
resumed.refresh_token = tokens.refresh_token.clone();
|
||||
}
|
||||
let session = ProtonApiSession::resume(client_config(), resumed.to_params())?;
|
||||
let sink = register_refresh_handler(&session, stored.refresh_token.clone());
|
||||
if let Some(tokens) = unsaved {
|
||||
let _ = sink.rotated(tokens);
|
||||
}
|
||||
Ok(session)
|
||||
}
|
||||
|
||||
/// The rotation the kernel keyring holds for `stored`, if it belongs there.
|
||||
/// One that does not is from an older login, or was written since, and is
|
||||
/// dropped.
|
||||
fn recover_unsaved(stored: &StoredSession) -> Option<Tokens> {
|
||||
let kept = match kernelkey::UNSAVED_TOKENS.load() {
|
||||
Ok(kept) => kept?,
|
||||
Err(e) => {
|
||||
tracing::debug!(error = %e, "kernel keyring unavailable; no unsaved tokens to recover");
|
||||
return None;
|
||||
}
|
||||
};
|
||||
match serde_json::from_slice::<UnsavedRotation>(&kept) {
|
||||
Ok(unsaved) if unsaved.belongs_in(stored) => Some(Tokens {
|
||||
access_token: unsaved.access_token,
|
||||
refresh_token: unsaved.refresh_token,
|
||||
}),
|
||||
_ => {
|
||||
let _ = kernelkey::UNSAVED_TOKENS.clear();
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Turn a blob written by 3.0.x — which holds the mailbox password — into one
|
||||
/// that holds passphrases, so the password stops being stored.
|
||||
///
|
||||
@@ -747,6 +788,27 @@ struct SinkState {
|
||||
stored_refresh: String,
|
||||
/// The latest rotation, until the keyring has taken it.
|
||||
unsaved: Option<Tokens>,
|
||||
/// Whether `unsaved` is also in [`kernelkey::UNSAVED_TOKENS`].
|
||||
kept: bool,
|
||||
}
|
||||
|
||||
/// A rotation the keyring refused, as kept in the kernel keyring until it is
|
||||
/// written: the tokens, and the blob they go into.
|
||||
#[derive(Serialize, Deserialize)]
|
||||
struct UnsavedRotation {
|
||||
session_id: String,
|
||||
/// The refresh token this rotation spent, which the blob still holds.
|
||||
spent: String,
|
||||
access_token: String,
|
||||
refresh_token: String,
|
||||
}
|
||||
|
||||
impl UnsavedRotation {
|
||||
/// Whether `stored` is the blob this rotation was refused for, still
|
||||
/// holding the refresh token it spent.
|
||||
fn belongs_in(&self, stored: &StoredSession) -> bool {
|
||||
self.session_id == stored.session_id && self.spent == stored.refresh_token
|
||||
}
|
||||
}
|
||||
|
||||
/// What became of a rotation offered to a stored blob.
|
||||
@@ -784,6 +846,7 @@ impl TokenSink {
|
||||
state: parking_lot::Mutex::new(SinkState {
|
||||
stored_refresh,
|
||||
unsaved: None,
|
||||
kept: false,
|
||||
}),
|
||||
retrying: AtomicBool::new(false),
|
||||
}
|
||||
@@ -796,12 +859,44 @@ impl TokenSink {
|
||||
fn rotated(self: &Arc<Self>, tokens: Tokens) -> Result<()> {
|
||||
self.state.lock().unsaved = Some(tokens);
|
||||
let written = self.flush();
|
||||
if written.is_err() {
|
||||
if let Err(e) = &written {
|
||||
tracing::error!(
|
||||
error = %e,
|
||||
"the keyring refused the refreshed session tokens; retrying"
|
||||
);
|
||||
self.keep_in_kernel();
|
||||
self.retry_in_background();
|
||||
}
|
||||
written
|
||||
}
|
||||
|
||||
/// Keep the unsaved rotation in the kernel keyring too, so a process that
|
||||
/// dies before the retry lands does not take the only live refresh token
|
||||
/// with it. The next resume puts it back (see [`resume_stored`]).
|
||||
fn keep_in_kernel(&self) {
|
||||
let mut state = self.state.lock();
|
||||
let Some(tokens) = &state.unsaved else {
|
||||
return;
|
||||
};
|
||||
let unsaved = UnsavedRotation {
|
||||
session_id: self.session_id.clone(),
|
||||
spent: state.stored_refresh.clone(),
|
||||
access_token: tokens.access_token.clone(),
|
||||
refresh_token: tokens.refresh_token.clone(),
|
||||
};
|
||||
let Ok(json) = serde_json::to_vec(&unsaved).map(Zeroizing::new) else {
|
||||
return;
|
||||
};
|
||||
match kernelkey::UNSAVED_TOKENS.store(&json) {
|
||||
Ok(()) => state.kept = true,
|
||||
Err(e) => tracing::error!(
|
||||
error = %e,
|
||||
"the kernel keyring refused them too; if this process stops first, \
|
||||
the next start needs `pdfs login`"
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
/// Retry the unsaved rotation on [`KEYRING_RETRY`] until it is written or
|
||||
/// has nowhere left to go. One thread per sink; a newer rotation simply
|
||||
/// replaces what it is retrying.
|
||||
@@ -850,7 +945,7 @@ impl TokenSink {
|
||||
let mut stored = match load() {
|
||||
Ok(stored) => stored,
|
||||
Err(Error::NotLoggedIn) => {
|
||||
state.unsaved = None;
|
||||
state.settle();
|
||||
return Ok(());
|
||||
}
|
||||
Err(e) => return Err(e),
|
||||
@@ -872,11 +967,22 @@ impl TokenSink {
|
||||
);
|
||||
}
|
||||
}
|
||||
state.unsaved = None;
|
||||
state.settle();
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl SinkState {
|
||||
/// The unsaved rotation has gone where it can: drop it, and its copy in
|
||||
/// the kernel keyring.
|
||||
fn settle(&mut self) {
|
||||
self.unsaved = None;
|
||||
if std::mem::take(&mut self.kept) {
|
||||
let _ = kernelkey::UNSAVED_TOKENS.clear();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Persist rotated tokens the moment the session obtains them.
|
||||
///
|
||||
/// `stored_refresh` is the refresh token the keyring holds for this session
|
||||
@@ -884,21 +990,18 @@ impl TokenSink {
|
||||
/// cannot write back a stale copy over a later unlock or lock, and it writes
|
||||
/// only into a blob that is still this session's (see [`TokenSink`]), so it
|
||||
/// cannot resurrect a logged-out session or overwrite a newer login.
|
||||
fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String) {
|
||||
fn register_refresh_handler(session: &ProtonApiSession, stored_refresh: String) -> Arc<TokenSink> {
|
||||
let session_id = session.session_id().as_str();
|
||||
let sink = Arc::new(TokenSink::new(session_id, stored_refresh));
|
||||
TOKEN_SINKS
|
||||
.lock()
|
||||
.insert(session_id.to_owned(), Arc::clone(&sink));
|
||||
let handler = Arc::clone(&sink);
|
||||
session.http().set_on_tokens_refreshed(move |tokens| {
|
||||
if let Err(e) = sink.rotated(tokens) {
|
||||
tracing::error!(
|
||||
error = %e,
|
||||
"the keyring refused the refreshed session tokens; retrying. \
|
||||
If the daemon stops first, the next start needs `pdfs login`"
|
||||
)
|
||||
}
|
||||
// A refusal is logged and retried by the sink itself.
|
||||
let _ = handler.rotated(tokens);
|
||||
});
|
||||
sink
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -1118,6 +1221,15 @@ mod tests {
|
||||
std::io::Error::other("locked").into(),
|
||||
));
|
||||
assert!(sink.rotated(tokens("a2", "r2")).is_err());
|
||||
// Until it is written, a stop does not lose it: the next resume finds
|
||||
// it in the kernel keyring, where one is offered.
|
||||
let kept = sink.state.lock().kept;
|
||||
if kept {
|
||||
assert_eq!(
|
||||
recover_unsaved(&stored_with("r")).unwrap().refresh_token,
|
||||
"r2"
|
||||
);
|
||||
}
|
||||
|
||||
let deadline = std::time::Instant::now() + Duration::from_secs(5);
|
||||
while load().unwrap().refresh_token != "r2" {
|
||||
@@ -1128,5 +1240,24 @@ mod tests {
|
||||
std::thread::sleep(Duration::from_millis(50));
|
||||
}
|
||||
assert!(sink.state.lock().unsaved.is_none());
|
||||
if kept {
|
||||
assert!(kernelkey::UNSAVED_TOKENS.load().unwrap().is_none());
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unsaved_rotation_goes_only_into_the_blob_it_was_refused_for() {
|
||||
let unsaved = UnsavedRotation {
|
||||
session_id: "s".into(),
|
||||
spent: "r".into(),
|
||||
access_token: "a2".into(),
|
||||
refresh_token: "r2".into(),
|
||||
};
|
||||
assert!(unsaved.belongs_in(&stored_with("r")));
|
||||
// Written since, by a retry or another process.
|
||||
assert!(!unsaved.belongs_in(&stored_with("r2")));
|
||||
let mut login = stored_with("r");
|
||||
login.session_id = "new".into();
|
||||
assert!(!unsaved.belongs_in(&login));
|
||||
}
|
||||
}
|
||||
@@ -5,6 +5,8 @@
|
||||
//! kept in kernel memory — not on disk, not in the Secret Service — and vanish
|
||||
//! when the user's last session ends or the machine reboots. Both the daemon and
|
||||
//! `pdfs unlock` reach the same entry because it hangs off the *user* keyring.
|
||||
//! It is also where session tokens wait while the Secret Service refuses them
|
||||
//! ([`UNSAVED_TOKENS`]), so they outlive the process that holds them.
|
||||
//!
|
||||
//! Kernel keys are not paged to swap, are invisible to other users, and are
|
||||
//! wiped by [`clear`] (`KEYCTL_REVOKE`), which makes them a better home than a
|
||||
@@ -20,12 +22,34 @@ use std::io;
|
||||
|
||||
use zeroize::Zeroizing;
|
||||
|
||||
/// Key description. Namespaced so it cannot collide with another program's key.
|
||||
/// One secret in the user keyring, by its description. Descriptions are
|
||||
/// namespaced so they cannot collide with another program's keys.
|
||||
pub struct Key {
|
||||
description: &'static [u8],
|
||||
}
|
||||
|
||||
/// The account's key passphrases (see the module docs).
|
||||
#[cfg(not(test))]
|
||||
const DESCRIPTION: &[u8] = b"pdfs:key-passphrases\0";
|
||||
pub const PASSPHRASES: Key = Key {
|
||||
description: b"pdfs:key-passphrases\0",
|
||||
};
|
||||
/// Tests must never touch a real unlock.
|
||||
#[cfg(test)]
|
||||
const DESCRIPTION: &[u8] = b"pdfs:key-passphrases:test\0";
|
||||
pub const PASSPHRASES: Key = Key {
|
||||
description: b"pdfs:key-passphrases:test\0",
|
||||
};
|
||||
|
||||
/// Session tokens the Secret Service refused to store, until it takes them.
|
||||
/// A refresh token held only in a process's memory dies with the process.
|
||||
#[cfg(not(test))]
|
||||
pub const UNSAVED_TOKENS: Key = Key {
|
||||
description: b"pdfs:unsaved-tokens\0",
|
||||
};
|
||||
#[cfg(test)]
|
||||
pub const UNSAVED_TOKENS: Key = Key {
|
||||
description: b"pdfs:unsaved-tokens:test\0",
|
||||
};
|
||||
|
||||
const KEY_TYPE: &[u8] = b"user\0";
|
||||
|
||||
const KEY_SPEC_USER_KEYRING: i32 = -4;
|
||||
@@ -49,7 +73,7 @@ fn last_error<T>() -> io::Result<T> {
|
||||
Err(io::Error::last_os_error())
|
||||
}
|
||||
|
||||
fn find() -> io::Result<Option<libc::c_long>> {
|
||||
fn find(description: &[u8]) -> io::Result<Option<libc::c_long>> {
|
||||
// SAFETY: both strings are NUL-terminated and outlive the call.
|
||||
let id = unsafe {
|
||||
libc::syscall(
|
||||
@@ -57,7 +81,7 @@ fn find() -> io::Result<Option<libc::c_long>> {
|
||||
KEYCTL_SEARCH,
|
||||
KEY_SPEC_USER_KEYRING,
|
||||
KEY_TYPE.as_ptr(),
|
||||
DESCRIPTION.as_ptr(),
|
||||
description.as_ptr(),
|
||||
0,
|
||||
)
|
||||
};
|
||||
@@ -83,15 +107,16 @@ pub fn forbid_dumps() {
|
||||
}
|
||||
}
|
||||
|
||||
impl Key {
|
||||
/// Store `secret`, replacing any earlier one.
|
||||
pub fn store(secret: &[u8]) -> io::Result<()> {
|
||||
pub fn store(&self, secret: &[u8]) -> io::Result<()> {
|
||||
// SAFETY: strings are NUL-terminated; the payload pointer/length pair
|
||||
// describes `secret`, which outlives the call.
|
||||
let id = unsafe {
|
||||
libc::syscall(
|
||||
libc::SYS_add_key,
|
||||
KEY_TYPE.as_ptr(),
|
||||
DESCRIPTION.as_ptr(),
|
||||
self.description.as_ptr(),
|
||||
secret.as_ptr(),
|
||||
secret.len(),
|
||||
KEY_SPEC_USER_KEYRING,
|
||||
@@ -112,15 +137,15 @@ pub fn store(secret: &[u8]) -> io::Result<()> {
|
||||
if rc < 0 {
|
||||
// A key whose permissions are unknown is not one to leave behind.
|
||||
let err = io::Error::last_os_error();
|
||||
let _ = clear();
|
||||
let _ = self.clear();
|
||||
return Err(err);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Read the stored secret, or `None` when there is none.
|
||||
pub fn load() -> io::Result<Option<Zeroizing<Vec<u8>>>> {
|
||||
let Some(id) = find()? else {
|
||||
pub fn load(&self) -> io::Result<Option<Zeroizing<Vec<u8>>>> {
|
||||
let Some(id) = find(self.description)? else {
|
||||
return Ok(None);
|
||||
};
|
||||
// Size first, then read; retry if the key was replaced in between.
|
||||
@@ -164,8 +189,8 @@ pub fn load() -> io::Result<Option<Zeroizing<Vec<u8>>>> {
|
||||
}
|
||||
|
||||
/// Remove the stored secret. Succeeds when there is none.
|
||||
pub fn clear() -> io::Result<()> {
|
||||
let Some(id) = find()? else {
|
||||
pub fn clear(&self) -> io::Result<()> {
|
||||
let Some(id) = find(self.description)? else {
|
||||
return Ok(());
|
||||
};
|
||||
// SAFETY: plain integer arguments.
|
||||
@@ -178,6 +203,7 @@ pub fn clear() -> io::Result<()> {
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
@@ -187,18 +213,21 @@ mod tests {
|
||||
// so one test covers the whole round trip and skips when it is unavailable.
|
||||
#[test]
|
||||
fn stored_secret_round_trips_and_clears() {
|
||||
if store(b"probe").is_err() {
|
||||
if PASSPHRASES.store(b"probe").is_err() {
|
||||
eprintln!("kernel keyring unavailable; skipping");
|
||||
return;
|
||||
}
|
||||
let saved = load().unwrap().expect("stored secret is readable");
|
||||
let saved = PASSPHRASES
|
||||
.load()
|
||||
.unwrap()
|
||||
.expect("stored secret is readable");
|
||||
assert_eq!(saved.as_slice(), b"probe");
|
||||
|
||||
store(b"replaced").unwrap();
|
||||
assert_eq!(load().unwrap().unwrap().as_slice(), b"replaced");
|
||||
PASSPHRASES.store(b"replaced").unwrap();
|
||||
assert_eq!(PASSPHRASES.load().unwrap().unwrap().as_slice(), b"replaced");
|
||||
|
||||
clear().unwrap();
|
||||
assert!(load().unwrap().is_none());
|
||||
clear().unwrap();
|
||||
PASSPHRASES.clear().unwrap();
|
||||
assert!(PASSPHRASES.load().unwrap().is_none());
|
||||
PASSPHRASES.clear().unwrap();
|
||||
}
|
||||
}
|
||||
+11
-5
@@ -4951,8 +4951,7 @@ daemon/tray/FUSE, upgrade, rollback, and uninstall without deleting user state.
|
||||
|
||||
### B61 — Rotated single-use credentials can be lost (HIGH-18)
|
||||
|
||||
**Status:** Partly fixed. A process that dies before a retried write lands still loses the
|
||||
rotation, and only the log says so.
|
||||
**Status:** Partly fixed. Only the log says that the keyring refused a rotation.
|
||||
**Found:** 2026-07-22, 1.0 authentication audit
|
||||
**Where:** `crates/pdfs-core/src/auth.rs`, refresh callback
|
||||
|
||||
@@ -4968,10 +4967,17 @@ unavailable keyrings, callback races, repeated rotations, death, and restart.
|
||||
in the background after 1 s, 5 s, 30 s and then every 5 minutes, until it is written or the blob
|
||||
has moved on (B201). The refusal is an error in the log naming `pdfs login`, and the daemon's stop
|
||||
writes a rotation still unsaved, logging an error if that fails too. The re-login path is B201's:
|
||||
an old session never writes into a newer login. Left: a crash or SIGKILL before a retry lands,
|
||||
and telling the GUI.
|
||||
an old session never writes into a newer login.
|
||||
|
||||
**Test:** `auth::tests::a_rotation_the_keyring_refused_is_written_once_it_takes_it`.
|
||||
Until it is written, the rotation also waits in the kernel keyring (`kernelkey::UNSAVED_TOKENS`),
|
||||
so a crash or SIGKILL before a retry lands no longer loses it. The next resume, by the daemon or
|
||||
`pdfs unlock`, puts it back in when the blob still holds the refresh token it spent, and writes
|
||||
it. One that belongs to an older login is dropped. The kernel keyring lasts until the user's last
|
||||
session ends, so a refused write followed by a crash and a logout still loses the rotation. Left:
|
||||
telling the GUI.
|
||||
|
||||
**Test:** `auth::tests::a_rotation_the_keyring_refused_is_written_once_it_takes_it` and
|
||||
`auth::tests::an_unsaved_rotation_goes_only_into_the_blob_it_was_refused_for`.
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in new issue
Block a user