security: close the holes a session or a stranger could walk through

DELETE /api/media/{id} joined the raw, percent-decoded id onto uploads/,
so ..%2Fdiary.db deleted the database. Ids are now parsed as uuids before
they reach the filesystem, and a file is only removed when its row was.

Import panicked on a link like %aé: percent_decode sliced the string
between the bytes of a character. Hex digits are now read as bytes. The
SFTP write_at offset is checked before it is cast, so an offset near
u64::MAX no longer wraps past the size limit and panics the session.

The global body limit was the upload ceiling, about a gigabyte, and
applied to the login too. Only the upload routes take that much now;
other bodies stop at 16 MiB and the login at 16 KiB.

Writes a browser says came from another origin are refused, which
SameSite=Lax alone does not cover for sibling subdomains or for
multipart forms. Logout only locks the vault for a caller who is
logged in, so a cross-site form can no longer shut it.

Login and vault attempts go through one at a time, so a burst of
parallel guesses cannot all pass the throttle before the first failure
is recorded, and parallel Argon2 runs cannot pile up.

Sealed files are served with no-store instead of a year of cache, so
their plaintext does not outlive :lock in the browser. The SFTP host key
is created 0600 rather than narrowed after being written.

SIGTERM is handled: as PID 1 in the container it was ignored, and every
docker stop ended in SIGKILL. Requests get eight seconds to finish, the
pool is closed so the WAL is checkpointed, and compose runs tini.

README.md

@@ -438,6 +438,14 @@ re-run `:share` on those entries to publish them again.
 Uploaded files are only ever served as types that cannot execute — anything else
 is handed back as an opaque download.
 
+A write that a browser says came from another origin — `Sec-Fetch-Site`, or
+`Origin` against `Host` where a browser is too old to send that — is refused,
+so a proxy must pass the `Host` header through unchanged (traefik and nginx's
+`proxy_set_header Host $host` both do). Only the upload routes accept a body as
+large as `WORKSPACE_MAX_UPLOAD_MB`; everything else stops at 16 MiB, and the
+login at 16 KiB, before anyone has proved who they are. `SIGTERM` gets in-flight
+requests eight seconds to finish, inside the ten `docker stop` waits.
+
 ### Docker
 
 ```sh

compose.yml

@@ -3,6 +3,8 @@ services:
     build: .
     image: narl-workspace:latest
     restart: unless-stopped
+    # tini as PID 1: forwards SIGTERM and reaps anything the binary leaves behind.
+    init: true
     env_file:
       - .env
     environment:

src/auth.rs

@@ -1,6 +1,11 @@
+use std::convert::Infallible;
+
 use axum::{
-    extract::FromRequestParts,
-    http::{header, request::Parts},
+    extract::{FromRequestParts, OptionalFromRequestParts, Request},
+    http::{header, request::Parts, HeaderMap, StatusCode},
+    middleware::Next,
+    response::{IntoResponse, Response},
+    Json,
 };
 use base64::{engine::general_purpose::URL_SAFE_NO_PAD as B64, Engine};
 use hmac::{Hmac, Mac};
@@ -97,9 +102,66 @@ impl FromRequestParts<AppState> for Session {
     }
 }
 
+/// For a handler that behaves differently for a caller who is logged in, but
+/// must also answer one who is not.
+impl OptionalFromRequestParts<AppState> for Session {
+    type Rejection = Infallible;
+
+    async fn from_request_parts(
+        parts: &mut Parts,
+        state: &AppState,
+    ) -> Result<Option<Self>, Self::Rejection> {
+        Ok(<Session as FromRequestParts<AppState>>::from_request_parts(parts, state)
+            .await
+            .ok())
+    }
+}
+
+/// Refuse a write that another site's page asked the browser to make.
+/// `SameSite=Lax` already keeps the cookie off a cross-site POST, but not off
+/// one from a sibling subdomain, which counts as the same site — and a
+/// multipart form needs no preflight to get sent. A browser says where a
+/// request came from; a request that says nothing is not from a browser page
+/// and carries no ambient cookie worth stealing.
+pub async fn same_origin(request: Request, next: Next) -> Response {
+    if !request.method().is_safe() && !is_same_origin(request.headers()) {
+        tracing::warn!(
+            "refused a cross-origin {} {}",
+            request.method(),
+            request.uri().path()
+        );
+        return (
+            StatusCode::FORBIDDEN,
+            Json(serde_json::json!({ "error": "cross-origin request refused" })),
+        )
+            .into_response();
+    }
+    next.run(request).await
+}
+
+fn is_same_origin(headers: &HeaderMap) -> bool {
+    // Every current browser sends this, and it cannot be set from a page.
+    if let Some(site) = headers.get("sec-fetch-site") {
+        return matches!(site.as_bytes(), b"same-origin" | b"none");
+    }
+    // Older browsers: the Origin they send against the host they asked.
+    let Some(origin) = headers.get(header::ORIGIN) else {
+        return true;
+    };
+    let origin_host = origin
+        .to_str()
+        .ok()
+        .and_then(|origin| origin.split_once("://"))
+        .map(|(_, host)| host);
+    let host = headers.get(header::HOST).and_then(|h| h.to_str().ok());
+    matches!((origin_host, host), (Some(a), Some(b)) if a.eq_ignore_ascii_case(b))
+}
+
 #[cfg(test)]
 mod tests {
-    use super::{credentials_match, issue_token, verify_token};
+    use axum::http::{HeaderMap, HeaderValue};
+
+    use super::{credentials_match, is_same_origin, issue_token, verify_token};
     use crate::config::{BackupConfig, Config, MailConfig, SftpConfig};
 
     fn config(session_days: i64) -> Config {
@@ -188,4 +250,27 @@ mod tests {
         assert!(!credentials_match(&config, "nar", "hunter2"));
         assert!(!credentials_match(&config, "", ""));
     }
+
+    fn headers(pairs: &[(&'static str, &'static str)]) -> HeaderMap {
+        let mut map = HeaderMap::new();
+        for (name, value) in pairs {
+            map.insert(*name, HeaderValue::from_static(value));
+        }
+        map
+    }
+
+    #[test]
+    fn only_same_origin_writes_get_through() {
+        assert!(is_same_origin(&headers(&[("sec-fetch-site", "same-origin")])));
+        assert!(!is_same_origin(&headers(&[("sec-fetch-site", "same-site")])));
+        assert!(!is_same_origin(&headers(&[("sec-fetch-site", "cross-site")])));
+
+        let host = ("host", "diary.example.org");
+        assert!(is_same_origin(&headers(&[("origin", "https://diary.example.org"), host])));
+        assert!(!is_same_origin(&headers(&[("origin", "https://evil.example.org"), host])));
+        assert!(!is_same_origin(&headers(&[("origin", "null"), host])));
+
+        // curl, the CLI, a script with its own credentials.
+        assert!(is_same_origin(&headers(&[host])));
+    }
 }

src/main.rs

@@ -129,7 +129,12 @@ async fn main() -> Result<()> {
         });
     }
 
-    serve(bind, max_upload, state).await
+    let db = state.db.clone();
+    serve(bind, max_upload, state).await?;
+    // Checkpoints the write-ahead log, so what was written is in diary.db
+    // itself and not only beside it.
+    db.close().await;
+    Ok(())
 }
 
 /// Open the workspace database and bring it up to date. Both the server and the
@@ -162,9 +167,9 @@ async fn open_db(config: &Config) -> Result<sqlx::SqlitePool> {
 
 async fn serve(bind: std::net::SocketAddr, max_upload: usize, state: AppState) -> Result<()> {
     let app = Router::new()
-        .nest("/api", routes::api_router())
+        .nest("/api", routes::api_router(max_upload))
         .fallback(static_files::handler)
-        .layer(axum::extract::DefaultBodyLimit::max(max_upload + 1024 * 1024))
+        .layer(axum::middleware::from_fn(auth::same_origin))
         .layer(CompressionLayer::new())
         .layer(security_headers())
         .layer(TraceLayer::new_for_http())
@@ -176,12 +181,26 @@ async fn serve(bind: std::net::SocketAddr, max_upload: usize, state: AppState) -
 
     tracing::info!("narl-workspace listening on http://{bind}");
 
-    axum::serve(listener, app)
-        .with_graceful_shutdown(shutdown_signal())
-        .await?;
+    let server = axum::serve(listener, app).with_graceful_shutdown(async {
+        shutdown_signal().await;
+        tracing::info!("shutting down");
+    });
+    // Requests in flight get a few seconds to finish; an upload that would
+    // take longer is cut off rather than holding the process past the point
+    // where a supervisor stops asking and kills it.
+    tokio::select! {
+        result = server => result?,
+        () = async {
+            shutdown_signal().await;
+            tokio::time::sleep(SHUTDOWN_GRACE).await;
+        } => tracing::warn!("requests still open after {SHUTDOWN_GRACE:?}, closing anyway"),
+    }
     Ok(())
 }
 
+/// Below the ten seconds `docker stop` waits before it sends SIGKILL.
+const SHUTDOWN_GRACE: std::time::Duration = std::time::Duration::from_secs(8);
+
 /// Content-Security-Policy for the app shell. `unsafe-inline` is granted to
 /// styles only, because CodeMirror injects its theme as a <style> element at
 /// runtime; scripts stay confined to the bundled files.
@@ -209,7 +228,30 @@ fn security_headers() -> (HeaderLayer, HeaderLayer, HeaderLayer, HeaderLayer) {
     )
 }
 
+/// Ctrl-C at a terminal, SIGTERM from anything that supervises the process —
+/// docker, systemd. In a container this is PID 1, and the kernel gives PID 1 no
+/// default action for SIGTERM: unless it is handled here, it is ignored.
 async fn shutdown_signal() {
-    let _ = tokio::signal::ctrl_c().await;
-    tracing::info!("shutting down");
+    let interrupt = async {
+        let _ = tokio::signal::ctrl_c().await;
+    };
+    #[cfg(unix)]
+    let terminate = async {
+        match tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) {
+            Ok(mut signal) => {
+                signal.recv().await;
+            }
+            Err(e) => {
+                tracing::warn!("cannot listen for SIGTERM: {e}");
+                std::future::pending::<()>().await;
+            }
+        }
+    };
+    #[cfg(not(unix))]
+    let terminate = std::future::pending::<()>();
+
+    tokio::select! {
+        () = interrupt => {}
+        () = terminate => {}
+    }
 }

src/routes/import.rs

@@ -136,9 +136,11 @@ fn percent_decode(value: &str) -> String {
     let mut out = Vec::with_capacity(bytes.len());
     let mut i = 0;
     while i < bytes.len() {
+        // Hex digits are read as bytes, never as a slice of the string: the
+        // two after a `%` may well be the first half of a multi-byte character.
         if bytes[i] == b'%' && i + 2 < bytes.len() {
-            if let Ok(byte) = u8::from_str_radix(&value[i + 1..i + 3], 16) {
-                out.push(byte);
+            if let (Some(hi), Some(lo)) = (hex(bytes[i + 1]), hex(bytes[i + 2])) {
+                out.push(hi << 4 | lo);
                 i += 3;
                 continue;
             }
@@ -149,6 +151,10 @@ fn percent_decode(value: &str) -> String {
     String::from_utf8(out).unwrap_or_else(|_| value.to_string())
 }
 
+fn hex(digit: u8) -> Option<u8> {
+    (digit as char).to_digit(16).map(|d| d as u8)
+}
+
 /// Split a link target into the part that names a file and the `#fragment` or
 /// `?query` that follows it. `None` for anything that does not point inside the
 /// import: a URL, an absolute path, a bare anchor, a mail address.
@@ -679,8 +685,8 @@ mod tests {
     use std::collections::HashMap;
 
     use super::{
-        expand_zip, is_markdown, join_relative, normalize_path, rewrite_links, split_target,
-        split_title, Content, Landed,
+        expand_zip, is_markdown, join_relative, normalize_path, percent_decode, rewrite_links,
+        split_target, split_title, Content, Landed,
     };
 
     /// A zip with a note and a 4 KiB blob in it, written to `dir/in.zip`.
@@ -779,6 +785,17 @@ mod tests {
         assert_eq!(split_target("a%20b.md#top"), Some(("a b.md".into(), "#top")));
     }
 
+    #[test]
+    fn escapes_next_to_multibyte_characters_do_not_panic() {
+        assert_eq!(percent_decode("%aé"), "%aé");
+        assert_eq!(percent_decode("é%"), "é%");
+        assert_eq!(percent_decode("%é"), "%é");
+        assert_eq!(percent_decode("a%20"), "a ");
+        assert_eq!(percent_decode("%C3%A9"), "é");
+        // Bytes that do not decode to UTF-8 leave the target as it was written.
+        assert_eq!(percent_decode("%FF"), "%FF");
+    }
+
     #[test]
     fn links_point_at_where_the_files_landed() {
         let landed = HashMap::from([

src/routes/media.rs

@@ -422,7 +422,13 @@ pub async fn stream_media(state: &AppState, id: &str, private: bool) -> AppResul
         [
             (header::CONTENT_TYPE, mime),
             (header::CONTENT_LENGTH, size.to_string()),
-            (header::CACHE_CONTROL, "private, max-age=31536000".to_string()),
+            // An upload never changes under its id, so the browser may keep
+            // it — unless it was sealed: the plaintext would then sit in the
+            // browser's cache long after the vault had been shut.
+            (
+                header::CACHE_CONTROL,
+                if encrypted { "no-store" } else { "private, max-age=31536000" }.to_string(),
+            ),
             (
                 header::CONTENT_DISPOSITION,
                 format!(
@@ -470,12 +476,27 @@ async fn open_stream(
     }))
 }
 
+/// The id as it is stored and as it names the file on disk, or `None` for
+/// anything that is not a uuid. Every id that reaches the filesystem from a
+/// request goes through here first: `Path` is percent-decoded, so an id is
+/// otherwise free to be `../diary.db`.
+pub(crate) fn canonical_id(id: &str) -> Option<String> {
+    Uuid::try_parse(id).ok().map(|uuid| uuid.to_string())
+}
+
 pub async fn delete_media(state: &AppState, id: &str) -> AppResult<()> {
-    sqlx::query("DELETE FROM media WHERE id = ?1")
-        .bind(id)
+    let id = canonical_id(id).ok_or(AppError::NotFound)?;
+    let deleted = sqlx::query("DELETE FROM media WHERE id = ?1")
+        .bind(&id)
         .execute(&state.db)
-        .await?;
-    let path = state.config.uploads_dir().join(id);
+        .await?
+        .rows_affected();
+    // No row, no file of ours: nothing on disk is removed on the word of a
+    // request alone.
+    if deleted == 0 {
+        return Err(AppError::NotFound);
+    }
+    let path = state.config.uploads_dir().join(&id);
     if let Err(e) = tokio::fs::remove_file(&path).await {
         if e.kind() != std::io::ErrorKind::NotFound {
             return Err(e.into());
@@ -565,7 +586,17 @@ fn urlencode(value: &str) -> String {
 
 #[cfg(test)]
 mod tests {
-    use super::sanitize_mime;
+    use super::{canonical_id, sanitize_mime};
+
+    #[test]
+    fn only_a_uuid_is_an_id() {
+        let id = "0b9e3c1a-5f2d-4e7a-9c3b-1d2e3f4a5b6c";
+        assert_eq!(canonical_id(id).as_deref(), Some(id));
+        assert_eq!(canonical_id(&id.to_uppercase()).as_deref(), Some(id));
+        for bad in ["../diary.db", "/etc/passwd", "", "..", "0b9e3c1a/../../x"] {
+            assert_eq!(canonical_id(bad), None, "{bad:?}");
+        }
+    }
 
     #[test]
     fn keeps_types_that_cannot_execute() {

src/routes/mod.rs

@@ -11,16 +11,29 @@ pub mod ssh_keys;
 pub mod vault;
 
 use axum::{
+    extract::DefaultBodyLimit,
     routing::{delete, get, post, put},
     Router,
 };
 
 use crate::state::AppState;
 
-pub fn api_router() -> Router<AppState> {
+/// Anything that is not an upload: a document body, a card, a password.
+/// The size of the largest document anyone would write by hand, many times
+/// over, and the same ceiling the SFTP mount puts on one.
+const JSON_LIMIT: usize = 16 * 1024 * 1024;
+
+/// The only body read before anyone has proved who they are.
+const LOGIN_LIMIT: usize = 16 * 1024;
+
+/// `max_upload` applies to the routes that take files and to nothing else, so
+/// the upload ceiling is not also how much a stranger may make the server read.
+pub fn api_router(max_upload: usize) -> Router<AppState> {
+    // Multipart framing on top of the files themselves.
+    let uploads = DefaultBodyLimit::max(max_upload.saturating_add(1024 * 1024));
     Router::new()
         .route("/me", get(session::me))
-        .route("/login", post(session::login))
+        .route("/login", post(session::login).layer(DefaultBodyLimit::max(LOGIN_LIMIT)))
         .route("/logout", post(session::logout))
         .route("/export", get(export::export))
         .route("/backup", get(backup::status).post(backup::run))
@@ -39,8 +52,8 @@ pub fn api_router() -> Router<AppState> {
         .route("/nodes/{id}", put(nodes::update))
         .route("/nodes/{id}", delete(nodes::remove))
         .route("/nodes/{id}/move", post(nodes::move_node))
-        .route("/nodes/{id}/import", post(import::import))
-        .route("/nodes/{id}/files", post(media::upload_into))
+        .route("/nodes/{id}/import", post(import::import).layer(uploads))
+        .route("/nodes/{id}/files", post(media::upload_into).layer(uploads))
         .route("/nodes/{id}/encrypt", post(seal::encrypt))
         .route("/nodes/{id}/decrypt", post(seal::decrypt))
         .route("/vault", get(vault::status).post(vault::setup).put(vault::change_password))
@@ -48,10 +61,11 @@ pub fn api_router() -> Router<AppState> {
         .route("/vault/lock", post(vault::lock))
         .route("/nodes/{id}/share", post(share::enable))
         .route("/nodes/{id}/share", delete(share::disable))
-        .route("/media", post(media::upload).get(media::list))
+        .route("/media", post(media::upload).layer(uploads).get(media::list))
         .route("/media/{id}", get(media::serve).delete(media::remove))
         .route("/share/{token}/{key}", get(share::read))
         .route("/share/{token}/{key}/media/{id}", get(share::serve_media))
         .route("/ssh-keys", get(ssh_keys::list).post(ssh_keys::add))
         .route("/ssh-keys/{id}", delete(ssh_keys::remove))
+        .layer(DefaultBodyLimit::max(JSON_LIMIT))
 }

src/routes/nodes.rs

@@ -654,7 +654,10 @@ pub async fn remove(
     }
 
     for media_id in orphans {
-        media::delete_media(&state, &media_id).await?;
+        match media::delete_media(&state, &media_id).await {
+            Ok(()) | Err(AppError::NotFound) => {}
+            Err(e) => return Err(e),
+        }
     }
 
     state.backup.signal();

src/routes/session.rs

@@ -25,13 +25,13 @@ pub async fn login(
     headers: HeaderMap,
     Json(body): Json<LoginRequest>,
 ) -> AppResult<impl IntoResponse> {
-    state.login_throttle.check().map_err(|wait| {
+    let attempt = state.login_throttle.begin().map_err(|wait| {
         tracing::warn!("login refused, {wait}s of backoff remaining");
         AppError::TooManyRequests(wait)
     })?;
 
     if !auth::credentials_match(&state.config, &body.username, &body.password) {
-        let failures = state.login_throttle.record_failure();
+        let failures = attempt.failed();
         // Worth a mail once someone is past guessing and into trying.
         if failures > FREE_ATTEMPTS {
             state.notify.note_failed_logins(failures).await;
@@ -39,7 +39,7 @@ pub async fn login(
         return Err(AppError::Unauthorized);
     }
 
-    state.login_throttle.record_success();
+    attempt.succeeded();
     let user_agent = headers
         .get(header::USER_AGENT)
         .and_then(|v| v.to_str().ok())
@@ -57,9 +57,13 @@ pub async fn login(
     ))
 }
 
-pub async fn logout(State(state): State<AppState>) -> impl IntoResponse {
-    // Leaving is the moment the vault should not outlive.
-    state.vault.lock();
+pub async fn logout(session: Option<Session>, State(state): State<AppState>) -> impl IntoResponse {
+    // Leaving is the moment the vault should not outlive — but only the owner
+    // leaving: anyone may clear a cookie they do not have, and that must not
+    // be a way to shut the vault on the one who does.
+    if session.is_some() {
+        state.vault.lock();
+    }
     (
         StatusCode::OK,
         [(header::SET_COOKIE, auth::clear_cookie(&state.config))],

src/routes/vault.rs

@@ -7,6 +7,7 @@ use crate::{
     auth::Session,
     error::{AppError, AppResult},
     state::AppState,
+    throttle::Attempt,
     vault::{Status, Unlock, MIN_PASSWORD},
 };
 
@@ -53,21 +54,21 @@ pub async fn setup(
 
 /// Wrong guesses are slowed down exactly like logins. Argon2 already makes
 /// each one cost a fraction of a second; this makes a run of them cost minutes.
-fn throttle(state: &AppState) -> AppResult<()> {
-    state.vault_throttle.check().map_err(|wait| {
+fn throttle(state: &AppState) -> AppResult<Attempt<'_>> {
+    state.vault_throttle.begin().map_err(|wait| {
         tracing::warn!("vault unlock refused, {wait}s of backoff remaining");
         AppError::TooManyRequests(wait)
     })
 }
 
-fn answer(state: &AppState, outcome: Unlock) -> AppResult<()> {
+fn answer(attempt: Attempt<'_>, outcome: Unlock) -> AppResult<()> {
     match outcome {
         Unlock::Opened => {
-            state.vault_throttle.record_success();
+            attempt.succeeded();
             Ok(())
         }
         Unlock::WrongPassword => {
-            state.vault_throttle.record_failure();
+            attempt.failed();
             Err(AppError::BadRequest("wrong vault password".into()))
         }
         Unlock::NotConfigured => Err(AppError::BadRequest(
@@ -81,8 +82,8 @@ pub async fn unlock(
     State(state): State<AppState>,
     Json(input): Json<PasswordInput>,
 ) -> AppResult<Json<Status>> {
-    throttle(&state)?;
-    answer(&state, state.vault.unlock(input.password).await?)?;
+    let attempt = throttle(&state)?;
+    answer(attempt, state.vault.unlock(input.password).await?)?;
     Ok(Json(state.vault.status().await?))
 }
 
@@ -96,9 +97,9 @@ pub async fn change_password(
     State(state): State<AppState>,
     Json(input): Json<ChangeInput>,
 ) -> AppResult<Json<Status>> {
-    throttle(&state)?;
     strong_enough(&input.password)?;
-    answer(&state, state.vault.change_password(input.current, input.password).await?)?;
+    let attempt = throttle(&state)?;
+    answer(attempt, state.vault.change_password(input.current, input.password).await?)?;
     tracing::info!("vault password changed");
     state.backup.signal();
     Ok(Json(state.vault.status().await?))

src/sftp/fs.rs

@@ -145,11 +145,13 @@ fn split_heading(text: &str) -> (Option<String>, String) {
 }
 
 fn write_at(buf: &mut Vec<u8>, offset: u64, data: &[u8]) -> SftpResult<()> {
+    // Checked before anything is cast: an offset is the client's to choose,
+    // and one near `u64::MAX` must not wrap past the size limit.
+    let end = offset
+        .checked_add(data.len() as u64)
+        .filter(|&end| end <= MAX_FILE as u64)
+        .ok_or(StatusCode::Failure)? as usize;
     let offset = offset as usize;
-    let end = offset + data.len();
-    if end > MAX_FILE {
-        return Err(StatusCode::Failure);
-    }
     if buf.len() < end {
         buf.resize(end, 0);
     }
@@ -891,7 +893,17 @@ mod tests {
     };
     use sqlx::sqlite::SqlitePoolOptions;
 
-    use super::{doc_slug, normalize, render_doc, segments, split_heading, SpaceFs};
+    use super::{doc_slug, normalize, render_doc, segments, split_heading, write_at, SpaceFs, MAX_FILE};
+
+    #[test]
+    fn writes_past_the_limit_fail_instead_of_wrapping() {
+        let mut buf = Vec::new();
+        assert!(write_at(&mut buf, u64::MAX - 1, b"abc").is_err());
+        assert!(write_at(&mut buf, MAX_FILE as u64, b"a").is_err());
+        assert!(buf.is_empty());
+        write_at(&mut buf, 2, b"ab").unwrap();
+        assert_eq!(buf, b"\0\0ab");
+    }
 
     #[test]
     fn dot_dot_climbs_and_stops_at_the_root() {

src/sftp/mod.rs

@@ -38,13 +38,18 @@ fn load_or_create_host_key(path: &std::path::Path) -> Result<PrivateKey> {
             .with_context(|| format!("could not read {}", path.display()));
     }
     let key = PrivateKey::random(&mut rand10::rng(), Algorithm::Ed25519)?;
-    key.write_openssh_file(path, russh::keys::ssh_key::LineEnding::LF)
-        .with_context(|| format!("could not write {}", path.display()))?;
+    let pem = key.to_openssh(russh::keys::ssh_key::LineEnding::LF)?;
+    // Created owner-only rather than narrowed afterwards, so the private key
+    // is never on disk under the default umask, not even for a moment.
+    let mut options = std::fs::OpenOptions::new();
+    options.write(true).create_new(true);
     #[cfg(unix)]
-    {
-        use std::os::unix::fs::PermissionsExt;
-        std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?;
-    }
+    std::os::unix::fs::OpenOptionsExt::mode(&mut options, 0o600);
+    let mut file = options
+        .open(path)
+        .with_context(|| format!("could not write {}", path.display()))?;
+    std::io::Write::write_all(&mut file, pem.as_bytes())?;
+    file.sync_all()?;
     Ok(key)
 }
 

src/throttle.rs

@@ -16,6 +16,10 @@ pub struct LoginThrottle {
 struct State {
     failures: u32,
     next_allowed: i64,
+    /// An attempt has been let through and not yet settled. Checking and
+    /// recording are otherwise two steps, and a burst of guesses sent at once
+    /// would all pass the check before the first failure was written down.
+    in_flight: bool,
 }
 
 /// Wrong passwords happen; the first few are answered at full speed.
@@ -31,32 +35,63 @@ fn delay_after(failures: u32) -> i64 {
 }
 
 impl LoginThrottle {
-    /// `Err(seconds)` when this attempt is too soon after the last failure.
-    pub fn check(&self) -> Result<(), i64> {
-        let state = self.state.lock().unwrap_or_else(|e| e.into_inner());
+    /// Let one attempt through, or `Err(seconds)` to wait first. Attempts go
+    /// one at a time: a second one while the first is still being decided is
+    /// told to come back in a second, so guesses cannot be made in parallel —
+    /// and neither can the Argon2 runs behind a vault password, each of which
+    /// takes 64 MiB.
+    pub fn begin(&self) -> Result<Attempt<'_>, i64> {
+        let mut state = self.lock();
+        if state.in_flight {
+            return Err(1);
+        }
         match state.next_allowed - crate::now() {
             wait if wait > 0 => Err(wait),
-            _ => Ok(()),
+            _ => {
+                state.in_flight = true;
+                Ok(Attempt { throttle: self })
+            }
         }
     }
 
+    fn lock(&self) -> std::sync::MutexGuard<'_, State> {
+        self.state.lock().unwrap_or_else(|e| e.into_inner())
+    }
+}
+
+/// One attempt that [`LoginThrottle::begin`] let through. Dropping it without
+/// a verdict — a request that failed for some other reason — counts for
+/// nothing either way.
+pub struct Attempt<'a> {
+    throttle: &'a LoginThrottle,
+}
+
+impl Attempt<'_> {
     /// Returns how many failures have piled up, so the caller can decide
     /// whether this one is worth telling anybody about.
-    pub fn record_failure(&self) -> u32 {
-        let mut state = self.state.lock().unwrap_or_else(|e| e.into_inner());
+    pub fn failed(self) -> u32 {
+        let mut state = self.throttle.lock();
         state.failures = state.failures.saturating_add(1);
         state.next_allowed = crate::now() + delay_after(state.failures);
         state.failures
     }
 
-    pub fn record_success(&self) {
-        *self.state.lock().unwrap_or_else(|e| e.into_inner()) = State::default();
+    pub fn succeeded(self) {
+        let mut state = self.throttle.lock();
+        state.failures = 0;
+        state.next_allowed = 0;
+    }
+}
+
+impl Drop for Attempt<'_> {
+    fn drop(&mut self) {
+        self.throttle.lock().in_flight = false;
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use super::{delay_after, LoginThrottle, MAX_DELAY};
+    use super::{delay_after, Attempt, LoginThrottle, MAX_DELAY};
 
     #[test]
     fn the_first_few_mistakes_are_free() {
@@ -77,11 +112,21 @@ mod tests {
     #[test]
     fn a_correct_password_clears_the_debt() {
         let throttle = LoginThrottle::default();
+        // Made directly: past the free attempts `begin` would make them wait.
         for _ in 0..10 {
-            throttle.record_failure();
+            Attempt { throttle: &throttle }.failed();
         }
-        assert!(throttle.check().is_err());
-        throttle.record_success();
-        assert!(throttle.check().is_ok());
+        assert!(throttle.begin().is_err());
+        Attempt { throttle: &throttle }.succeeded();
+        assert!(throttle.begin().is_ok());
+    }
+
+    #[test]
+    fn attempts_go_one_at_a_time() {
+        let throttle = LoginThrottle::default();
+        let first = throttle.begin().unwrap();
+        assert_eq!(throttle.begin().err(), Some(1));
+        drop(first);
+        assert!(throttle.begin().is_ok());
     }
 }