From 36ac13ac2743847e8d6000899ff9360a871ad7b7 Mon Sep 17 00:00:00 2001 From: Mathias Myrland Date: Sat, 5 Sep 2026 08:57:51 +0200 Subject: [PATCH 1/8] security: remediate September 2026 audit findings (24 issues, W1-F2) WebSocket (ras-jsonrpc-bidirectional-server/-macro/-client): - W1 re-authorize held subscriptions on credential re-validation; add PermissionChangePolicy (DropSubscriptions | Close); mirror context subscriptions into the manager's topic index - W2 enforce max_message_size/max_frame_size at the transport - W3 SubscriptionLimits (per message, per connection, topic length) - W4 KeepaliveConfig (server ping + idle timeout) - W5 route WITH_PERMISSIONS checks through AuthProvider::check_permissions - C1 remove AuthConfig::JwtParams (token in URL); C2 percent-encode CustomParams; browser auth via ras-jsonrpc + token. subprotocols, server selects ras-jsonrpc so the token is never echoed Sessions (ras-identity-session): - S1 drop inline write-locked cleanup from verify/begin_session - S2 iss/aud required by default (allow_unscoped_tokens opt-out) - S3 secret entropy + placeholder substring checks - S4 reject future iat/nbf; S5 max_sessions_per_user Identity (ras-identity-local, ras-identity-oauth2): - I1 skip_serializing password_hash / client_secret - I2 redact LocalAuthPayload Debug; I4 Argon2 on spawn_blocking with password length cap - I3 evict oldest pending OAuth2 flow instead of refusing (login lockout) - I5 optional callback code, fixed ProviderDenied error; I6 fixed message for upstream HTTP errors; I7 constant-time binding compare - I8 metadata_claims allow-list; I9 https-only endpoints by default Auth core / files / REST (ras-auth-core, ras-file-core/-macro, ras-rest-macro): - A1 rename weak CSRF modes to dangerous_*, deprecate old names, warn - A2 drop Serialize from AuthError; Display no longer lists held grants - F1 sanitize_filename + RFC 5987 filename* in attachment() - F2 generic bodies for axum path/query/multipart rejections Version bumps: ras-auth-core 0.3.0, ras-identity-session 0.4.0, ras-identity-local 0.3.0, ras-identity-oauth2 0.3.0, ras-jsonrpc-bidirectional-server 0.3.0, -client 0.3.0, -macro 0.2.1, ras-file-core 0.2.1, ras-file-macro 0.2.1, ras-rest-macro 0.3.1. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01NZEonxsxS45oAEC7bUTfnL --- CHANGELOG.md | 79 +++ Cargo.lock | 23 +- crates/core/ras-auth-core/Cargo.toml | 3 +- crates/core/ras-auth-core/README.md | 11 +- crates/core/ras-auth-core/src/lib.rs | 68 +- crates/core/ras-auth-core/src/transport.rs | 241 ++++++- crates/core/ras-observability-core/Cargo.toml | 2 +- crates/identity/ras-identity-local/Cargo.toml | 2 +- crates/identity/ras-identity-local/README.md | 10 +- crates/identity/ras-identity-local/src/lib.rs | 160 ++++- .../identity/ras-identity-oauth2/Cargo.toml | 5 +- crates/identity/ras-identity-oauth2/README.md | 12 +- .../examples/google_oauth2.rs | 12 +- .../ras-identity-oauth2/src/client.rs | 216 ++++-- .../ras-identity-oauth2/src/config.rs | 121 +++- .../identity/ras-identity-oauth2/src/error.rs | 58 +- .../ras-identity-oauth2/src/provider.rs | 132 +++- .../identity/ras-identity-oauth2/src/state.rs | 215 +++--- .../identity/ras-identity-oauth2/src/tests.rs | 11 +- .../identity/ras-identity-oauth2/src/types.rs | 5 +- .../identity/ras-identity-session/Cargo.toml | 6 +- .../identity/ras-identity-session/README.md | 22 +- .../identity/ras-identity-session/src/lib.rs | 613 ++++++++++++++++-- .../ras-observability-otel/Cargo.toml | 2 +- crates/rest/ras-file-core/Cargo.toml | 5 +- crates/rest/ras-file-core/src/lib.rs | 229 ++++++- crates/rest/ras-file-macro/Cargo.toml | 6 +- crates/rest/ras-file-macro/src/server.rs | 45 +- crates/rest/ras-file-macro/tests/e2e.rs | 2 +- ...ilename_and_rejection_sanitization_test.rs | 216 ++++++ crates/rest/ras-rest-core/Cargo.toml | 2 +- crates/rest/ras-rest-macro/Cargo.toml | 8 +- crates/rest/ras-rest-macro/src/lib.rs | 93 ++- .../ras-rest-macro/tests/http_integration.rs | 63 ++ .../Cargo.toml | 4 +- .../README.md | 19 +- .../examples/bidirectional_client_usage.rs | 1 - .../src/client.rs | 26 +- .../src/config.rs | 86 ++- .../src/wasm.rs | 13 +- .../Cargo.toml | 8 +- .../src/server.rs | 39 +- .../tests/e2e.rs | 94 ++- .../Cargo.toml | 4 +- .../src/handler.rs | 520 ++++++++++++++- .../src/lib.rs | 7 +- .../src/service.rs | 71 +- .../src/upgrade.rs | 36 +- .../Cargo.toml | 2 +- crates/rpc/ras-jsonrpc-core/Cargo.toml | 2 +- crates/rpc/ras-jsonrpc-core/src/lib.rs | 17 +- crates/rpc/ras-jsonrpc-macro/Cargo.toml | 4 +- documentation/src/identity-and-sessions.md | 5 + .../macros/bidirectional-jsonrpc-service.md | 30 + examples/bidirectional-chat/README.md | 7 +- examples/bidirectional-chat/api/Cargo.toml | 10 +- examples/bidirectional-chat/server/Cargo.toml | 10 +- .../server/config.example.toml | 2 +- .../bidirectional-chat/server/src/config.rs | 4 +- .../bidirectional-chat/server/src/main.rs | 17 +- .../server/tests/auth_lifecycle_tests.rs | 19 +- .../server/tests/server_tests.rs | 8 +- examples/file-service-example/Cargo.toml | 6 +- examples/file-service-example/src/main.rs | 4 +- .../file-service-api/Cargo.toml | 6 +- .../file-service-backend/Cargo.toml | 4 +- .../file-service-backend/src/file_service.rs | 2 +- examples/oauth2-demo/server/.env.example | 2 +- examples/oauth2-demo/server/Cargo.toml | 4 +- examples/oauth2-demo/server/README.md | 2 +- examples/oauth2-demo/server/src/main.rs | 19 +- .../rest-wasm-example/rest-api/Cargo.toml | 4 +- .../rest-wasm-example/rest-backend/Cargo.toml | 2 +- .../fixtures/jsonrpc-fixture/Cargo.toml | 2 +- .../fixtures/rest-fixture/Cargo.toml | 4 +- 75 files changed, 3332 insertions(+), 492 deletions(-) create mode 100644 crates/rest/ras-file-macro/tests/filename_and_rejection_sanitization_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b0e4c7..25ec8f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,85 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Security - 2026-09-05 (audit remediation, one PR) +Remediates every finding from the September 2026 security review plus the follow-up gap sweep: 24 issues across the WebSocket, session, identity and auth-core crates, each with a regression test named by its ID (W1–W5, C1–C2, S1–S5, I1–I9, A1–A2, F1–F2). Version bumps in this set: + +| Crate | From | To | Why | +|---|---|---|---| +| `ras-auth-core` | 0.2.0 | 0.3.0 | `AuthError` no longer `Serialize`; CSRF constructors renamed (A1, A2) | +| `ras-identity-session` | 0.3.0 | 0.4.0 | `iss`/`aud` required by default; `SessionConfig::new` signature (S2) | +| `ras-identity-local` | 0.2.1 | 0.3.0 | `password_hash` not serialized; `LocalAuthPayload` changes (I1, I2) | +| `ras-identity-oauth2` | 0.2.0 | 0.3.0 | secret not serialized; `code` optional; https enforced; `add_provider` fallible (I1, I5, I9) | +| `ras-jsonrpc-bidirectional-server` | 0.2.0 | 0.3.0 | `AuthRevalidation` gained a field; new service options (W1–W4) | +| `ras-jsonrpc-bidirectional-client` | 0.2.0 | 0.3.0 | `AuthConfig::JwtParams` removed (C1) | +| `ras-jsonrpc-bidirectional-macro` | 0.2.0 | 0.2.1 | generated handler routes permissions through the provider (W5) | +| `ras-file-core` | 0.2.0 | 0.2.1 | `sanitize_filename`, `attachment()` encoding (F1) | +| `ras-file-macro` | 0.2.0 | 0.2.1 | filename sanitization, generic rejection bodies (F1, F2) | +| `ras-rest-macro` | 0.3.0 | 0.3.1 | generic path/query rejection bodies (F2) | + +Dependent crates and examples had their path-dependency version specs updated to match. + + +### Fixed - 2026-09-05 (WebSocket hardening — `ras-jsonrpc-bidirectional-server`, `-macro`, `-client`) +- **W1: subscriptions no longer survive permission revocation** (`ras-jsonrpc-bidirectional-server`). On every successful credential re-validation the handler re-runs `authorize_subscribe` for each held topic against the refreshed user and drops the ones no longer authorized, in both the connection context and the manager's topic index. Previously only the cached user was refreshed, so a downgraded connection kept receiving topic broadcasts until it disconnected. The new `PermissionChangePolicy` (`WebSocketService::on_permission_change`, builder field `on_permission_change`) selects `DropSubscriptions` (default) or `Close`, which closes the socket whenever the permission set changes so the client must re-authenticate. +- **Subscriptions made through the default `handle_subscribe` now reach `broadcast_to_topic`.** The handler loop mirrors subscribe/unsubscribe changes from the connection context into the connection manager's topic index; previously the two stores were never reconciled, so topics accepted by `authorize_subscribe` were invisible to manager-driven broadcasts. +- **W2: the inbound message limit is enforced at the transport** (`ras-jsonrpc-bidirectional-server`). `handle_upgrade` now sets `max_message_size` and `max_frame_size` on the Axum upgrade from `WebSocketService::max_message_size()`. Previously the 1 MiB check ran only after tungstenite had buffered the whole frame under its 64 MiB default, so any client could force 64 MiB allocations per message. +- **W5: WebSocket permission checks route through `AuthProvider::check_permissions`** (`ras-jsonrpc-bidirectional-macro`). The generated handler now carries an optional `Arc` (`with_auth_provider`, set automatically by the generated builder) and uses `ras_auth_core::check_permission_groups`, so providers with wildcard, hierarchical or dynamic permission semantics behave identically over WebSocket, REST and JSON-RPC. Handlers built by hand without a provider fall back to plain set membership as before. The insufficient-permissions error now uses `JsonRpcError::insufficient_permissions` (code from `error_codes`, `required` only in `data`). +- **Browser clients can now authenticate** (`ras-jsonrpc-bidirectional-client`, `-server`). The WASM transport never sent a token at all, and the server never selected a subprotocol, so a browser offering `token.` had its upgrade rejected. The client now offers `ras-jsonrpc` plus `token.` (`ClientConfig::get_subprotocols`), the server parses comma-separated `Sec-WebSocket-Protocol` lists and selects `ras-jsonrpc` (`WS_SUBPROTOCOL`), so the token is read but never echoed in the response. + +### Added - 2026-09-05 (WebSocket hardening) +- **W3: `SubscriptionLimits`** (`ras-jsonrpc-bidirectional-server`) — `WebSocketService::subscription_limits()` / builder field `subscription_limits`. Defaults: 64 topics per message, 256 per connection, 256-byte topic names. An over-limit `Subscribe` is answered with an invalid-params error and leaves the connection open; the service's `handle_subscribe` never sees it. +- **W4: `KeepaliveConfig`** (`ras-jsonrpc-bidirectional-server`) — `WebSocketService::keepalive()` / builder field `keepalive`. The server pings every 30 s and closes a connection that produces no inbound frame for 90 s (browsers and tungstenite answer pings automatically). Either half can be disabled with `None`. `max_connections` stays unbounded by default; production deployments should set it. +- `WS_SUBPROTOCOL` / `WS_TOKEN_SUBPROTOCOL_PREFIX` constants exported from `ras-jsonrpc-bidirectional-server` and `-client`. + +### Changed - 2026-09-05 (WebSocket hardening — breaking, `ras-jsonrpc-bidirectional-client` 0.3.0) +- **C1: `AuthConfig::JwtParams` removed.** It placed the token in the URL query string, where it enters proxy logs, browser history and tracing spans, and the bundled server never read it from there. Use `AuthConfig::JwtHeader` (header on native, subprotocol in browsers). `ClientBuilder::with_jwt_in_header` is now a deprecated no-op. +- **C2: `AuthConfig::CustomParams` are percent-encoded** and emitted in sorted key order. Previously keys and values were concatenated raw. +- `AuthRevalidation` gained the `on_permission_change` field (`ras-jsonrpc-bidirectional-server` 0.3.0); `WebSocketHandler` gained `with_connection_manager`, `with_subscription_limits`, `with_keepalive`. + +### Changed - 2026-09-05 (`ras-identity-session` hardening, S1–S5) +- **`iss`/`aud` are now required by default (S2). Breaking.** `SessionConfig` gained `require_iss_aud: bool` (default `true`); `SessionConfig::validate` (and therefore `SessionService::new`) fails when either `iss` or `aud` is `None`. `SessionConfig::new` now takes the issuer and audience: `SessionConfig::new(secret, iss, aud)`. Single-service deployments that never share a secret can opt out with `SessionConfig::new_unscoped(secret)` or `.allow_unscoped_tokens()`. Struct-literal callers must add the new `require_iss_aud` and `max_sessions_per_user` fields. Examples (`bidirectional-chat`, `oauth2-demo`, `google_oauth2`) and the identity READMEs now set a real issuer/audience. +- **Stricter `jwt_secret` validation (S3).** In addition to the 32-byte minimum, a secret is rejected when it contains fewer than 10 distinct byte values, a run of 8 or more identical bytes, or (case-insensitive substring) any of `change-me`, `changeme`, `secret`, `password`, `example`, `placeholder`, `test-secret`, `dev-secret`, `insecure`, `12345678`, `abcdefgh`, `your-secret`. Placeholder secrets in the example configs, `.env.example`, READMEs and test fixtures were replaced with random hex values.. +- **`begin_session`/`verify_session` no longer sweep the session store inline (S1).** The previous implementation took the `active_sessions` write lock and walked the whole map on every call, before the token was even decoded. Expired entries are now pruned lazily at most once per 60 s (a cheap atomic check on the hot path) and by `start_cleanup_task`, which should be started whenever `enforce_active_sessions` is on. + +### Added - 2026-09-05 (`ras-identity-session` hardening) +- **`nbf` claim (S4).** `JwtClaims` gained an optional `nbf: Option` (serde default, omitted when `None`). `verify_session` rejects a token whose `iat` or `nbf` is more than `CLOCK_SKEW_LEEWAY_SECS` (60 s) in the future with `SessionError::InvalidSession`. +- **Per-user session cap (S5).** `SessionConfig::max_sessions_per_user` (default `DEFAULT_MAX_SESSIONS_PER_USER` = 32, builder `with_max_sessions_per_user`, must be ≥ 1). When `enforce_active_sessions` is on and a user already holds that many sessions, `begin_session` evicts their oldest sessions (by `iat`) before inserting the new one, so a credential-stuffing loop cannot grow the in-memory store without bound. +- `SessionConfig::new_unscoped`, `SessionConfig::allow_unscoped_tokens`, `SessionConfig::with_max_sessions_per_user`, and the `CLOCK_SKEW_LEEWAY_SECS` / `DEFAULT_MAX_SESSIONS_PER_USER` constants. `Debug` for `SessionConfig` shows the two new fields (secret still redacted). + +### Added - 2026-09-05 (identity provider hardening I1–I9) +- **`OAuth2ProviderConfig::metadata_claims: Vec` (I8).** Allow-list of additional userinfo claims copied into `VerifiedIdentity.metadata` (and therefore the session JWT). Defaults to empty via `#[serde(default)]`; previously *every* extra claim the IdP returned was merged into metadata. +- **`OAuth2ProviderConfig::allow_insecure_endpoints: bool` and `OAuth2ProviderConfig::validate()` (I9).** Authorization/token/userinfo endpoints must be `https://`; `validate()` rejects anything else with `OAuth2Error::ConfigError` unless the flag (serde default `false`) is set. Only enable it for a local mock IdP. +- **`OAuth2Error::ProviderDenied { error }` and `OAuth2Error::InvalidCallback` (I5).** A callback carrying `error=…` (e.g. `access_denied`) now maps to `ProviderDenied` with only the standardized error code; `error_description` is logged at `warn` server-side and never echoed. A callback with neither `code` nor `error` returns `InvalidCallback`. +- **`ras_identity_local::MAX_PASSWORD_BYTES` (1024) and `LocalUserError::{PasswordTooLong, HashTaskFailed}` (I4).** `add_user` rejects longer passwords with `PasswordTooLong`; `verify` rejects them with the usual `InvalidCredentials` so nothing about the account is revealed. +- `InMemoryStateStore::len()` / `is_empty()` accessors. +- `ras-identity-oauth2` now depends on `subtle` (workspace dep). + +### Changed - 2026-09-05 (identity provider hardening I1–I9) +- **`LocalUser.password_hash` is no longer serialized (I1a, breaking).** The field carries `#[serde(skip_serializing)]`; `Serialize` output omits it entirely. `Deserialize` still requires it. Anything that persisted `LocalUser` via serde must now store the hash separately. +- **`OAuth2ProviderConfig.client_secret` is no longer serialized (I1b, breaking).** Same treatment: dumped configs never contain the secret; deserialization still requires it. +- **`LocalAuthPayload` no longer derives `Serialize` and has a redacting `Debug` (I2, breaking).** `{:?}` prints `password: "[REDACTED]"`. Nothing in the workspace serialized the payload; build the login JSON with `serde_json::json!` instead. +- **Argon2 runs on the blocking pool and outside the users lock (I4).** `add_user` and `verify` clone the stored hash out of the `RwLock` and run `hash_password` / `verify_password` in `tokio::task::spawn_blocking`; the read lock is no longer held for the duration of a hash and the async executor is no longer stalled. The concurrency semaphore and the sentinel-hash timing behaviour for unknown users are unchanged. +- **`InMemoryStateStore` evicts instead of refusing at capacity, and sweeps at most every 10 s (I3).** `store` no longer returns `TooManyPendingFlows` when `max_states` is reached: it force-sweeps expired flows and, if still full, evicts the pending flow closest to expiry. The opportunistic expired-state sweep is rate-limited to once per 10 seconds instead of an O(n) `retain` on every call (`cleanup_expired` still sweeps unconditionally). Production deployments should rate-limit flow starts at the edge; see the type docs. `OAuth2Error::TooManyPendingFlows` is now `#[deprecated]` (kept for custom `OAuth2StateStore` implementations). +- **`AuthorizationResponse.code` and `OAuth2AuthPayload::Callback { code }` are `Option` (I5, breaking).** A legitimate `error=access_denied` redirect carries no code and is no longer an `InvalidPayload`. +- **`OAuth2Error::HttpError` displays a fixed `"upstream request failed"` (I6).** The underlying `reqwest::Error` (which embeds the request URL) is logged at `warn` at the transport and remains reachable via `source()`, but no longer reaches `IdentityError::ProviderError` strings. Undecodable token/userinfo responses likewise log the reqwest error and surface a fixed message. +- **OAuth2 session-binding comparison is constant-time (I7).** `handle_callback` compares the stored binding against the callback value with `subtle::ConstantTimeEq`; semantics (missing or mismatched value → `InvalidState`, unbound flow ignores the callback value) are unchanged. +- **Provider construction validates every provider config (I9, breaking).** `OAuth2Provider::try_new` returns `ConfigError` for a non-`https://` endpoint (unless `allow_insecure_endpoints`), `OAuth2Provider::new` panics with `invalid OAuth2 configuration` (consistent with the existing `OAuth2Client::new` panic), and **`OAuth2Provider::add_provider` now returns `OAuth2Result<()>`**. The in-crate mock-IdP tests set `allow_insecure_endpoints: true`. +- `OAuth2ProviderConfig` gained two fields, so struct literals must add `metadata_claims: Vec::new()` and `allow_insecure_endpoints: false` (updated: `examples/oauth2-demo`, the crate's `google_oauth2` example and README). + +### Changed - 2026-09-05 (security hardening — `ras-auth-core`, `ras-file-core`, `ras-file-macro`, `ras-rest-macro`) +- **Weak CSRF modes are renamed `dangerous_*` and warn when paired with cookie auth (A1).** `ras-auth-core`: `CsrfConfig::header_presence_only` → `CsrfConfig::dangerous_header_presence_only`, `CsrfConfig::with_expected_value` → `CsrfConfig::dangerous_static_value`. Neither mode binds the token to the session (presence-only relies entirely on restrictive credentialed CORS; a static value is a shared process-wide secret). The old names remain as `#[deprecated]` thin wrappers for one release. `AuthTransportConfig::with_cookie` / `with_csrf` now emit a `tracing::warn!` when cookie auth is combined with either mode, and `AuthTransportConfig::validate` warns as a fallback for struct-literal configs (rate-limited to once per distinct weak config per process, since `validate` runs on every request). New `CsrfConfig::dangerous_mode()` reports which weak mode, if any, is active. `ras-auth-core` gains a direct `tracing` dependency. The `pub` fields on `CsrfConfig` (`header_name`, `expected_value`, `cookie_name`) are left public so struct-literal construction keeps compiling; a literal that clears `cookie_name` still goes through the `validate` warning path. README and the `identity-and-sessions` book chapter document the new names. +- **`AuthError` is no longer `Serialize`/`Deserialize`, and its `Display` no longer lists the caller's permissions (A2).** `ras-auth-core`: the derives are removed (nothing in the workspace serialized `AuthError`; generated servers already map it to a generic per-class message). `AuthError::InsufficientPermissions`'s `Display` now reads `Insufficient permissions: required [...], caller holds N permission(s)` — the `has` field is retained for server-side logging via `Debug`. **Breaking** for any downstream that serialized `AuthError` directly; map to a wire type of your own instead. +- **`DownloadResponse::attachment` escapes properly and emits an RFC 5987 `filename*` (F1).** `ras-file-core`: `"` and `\` are backslash-escaped in the quoted `filename="..."` form (previously `"` was stripped and `\` passed through), control characters are stripped, non-ASCII is replaced by `_` in the legacy form, and a `filename*=UTF-8''` parameter carries the original Unicode name. Tests that assert the exact `Content-Disposition` string need updating (in-repo: `ras-file-macro` e2e, `file-service-example`, `file-service-backend`). + +### Added - 2026-09-05 (security hardening) +- **`ras_file_core::sanitize_filename(&str) -> String`** and **`ras_file_core::MAX_FILENAME_BYTES`** (F1). Reduces an untrusted filename to a single safe path component: keeps only the final component (split on both `/` and `\`), strips NUL and other control characters, maps dots-only names (`.`, `..`) and empty results to `"upload"`, and truncates to 255 bytes on a UTF-8 char boundary. Unicode is preserved. +- `ras-file-core` now depends on and re-exports `tracing` (`ras_file_core::tracing`) so generated `file_service!` code can log without consumers declaring a direct `tracing` dependency. + +### Fixed - 2026-09-05 (security hardening) +- **Upload filenames are sanitized before they reach the handler (F1).** `file_service!`: the multipart `filename=` parameter is passed through `ras_file_core::sanitize_filename` before `IncomingFile::file_name()` sees it, so a handler that joins the name onto a directory cannot be steered by `../` or `..\` segments. `filename: required` / `forbidden` policies are still evaluated on the raw presence of the parameter. +- **axum rejection bodies are no longer echoed to the client (F2).** `file_service!`: `Multipart` extractor rejections, multipart parse errors, and `Path` extraction failures previously returned axum's own text (e.g. `Invalid boundary ...`, or the offending path value). They now return fixed messages — `invalid multipart request`, `invalid multipart body`, `invalid path parameters` — and log the axum detail at `warn`. `rest_service!`: `Path` and `axum_extra::Query` extractors previously used axum's default plain-text rejection, which echoes the offending value and target type (``Cannot parse `abc` to a `i32` ``). Generated handlers now take those extractors as `Result<_, Rejection>` and return `400` with the JSON body `{"error": "Invalid path parameters"}` / `{"error": "Invalid query parameters"}`, logging the detail at `warn` in line with the existing rejection-logging convention. Note: the `VersionMigration` error `Display` is still echoed on a `400` — that message is application-authored, like `RestError::message`, and unchanged. + ### Changed - 2026-08-18 (`rest_service!` hardening — device-integration feedback) - **`rest_service!` now requires `application/json` on bodied endpoints by default.** A request whose `Content-Type` is not `application/json` (parameters like `; charset=utf-8` are allowed) is rejected with `415 Unsupported Media Type` before the body is read. This forces a CORS preflight for cross-origin requests, closing the simple-request CSRF shape (a cross-origin `text/plain` POST), and matches `file_service!`, which already validated. **Breaking:** clients that POST/PUT/PATCH a body without an `application/json` content type now get `415`; opt out per-service with `require_json_content_type: false`. Rides in the already-unreleased `ras-rest-macro` `0.3.0`. diff --git a/Cargo.lock b/Cargo.lock index 820a7b2..92775e7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2687,7 +2687,7 @@ dependencies = [ [[package]] name = "ras-auth-core" -version = "0.2.0" +version = "0.3.0" dependencies = [ "cookie", "http", @@ -2696,11 +2696,12 @@ dependencies = [ "subtle", "thiserror 2.0.18", "tokio", + "tracing", ] [[package]] name = "ras-file-core" -version = "0.2.0" +version = "0.2.1" dependencies = [ "bytes", "futures-core", @@ -2708,11 +2709,12 @@ dependencies = [ "http", "ras-auth-core", "thiserror 2.0.18", + "tracing", ] [[package]] name = "ras-file-macro" -version = "0.2.0" +version = "0.2.1" dependencies = [ "async-trait", "axum", @@ -2748,7 +2750,7 @@ dependencies = [ [[package]] name = "ras-identity-local" -version = "0.2.1" +version = "0.3.0" dependencies = [ "argon2", "async-trait", @@ -2761,7 +2763,7 @@ dependencies = [ [[package]] name = "ras-identity-oauth2" -version = "0.2.0" +version = "0.3.0" dependencies = [ "async-trait", "axum", @@ -2775,6 +2777,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "subtle", "thiserror 2.0.18", "tokio", "tracing", @@ -2785,7 +2788,7 @@ dependencies = [ [[package]] name = "ras-identity-session" -version = "0.3.0" +version = "0.4.0" dependencies = [ "async-trait", "base64 0.22.1", @@ -2804,7 +2807,7 @@ dependencies = [ [[package]] name = "ras-jsonrpc-bidirectional-client" -version = "0.2.0" +version = "0.3.0" dependencies = [ "anyhow", "async-trait", @@ -2833,7 +2836,7 @@ dependencies = [ [[package]] name = "ras-jsonrpc-bidirectional-macro" -version = "0.2.0" +version = "0.2.1" dependencies = [ "anyhow", "async-trait", @@ -2863,7 +2866,7 @@ dependencies = [ [[package]] name = "ras-jsonrpc-bidirectional-server" -version = "0.2.0" +version = "0.3.0" dependencies = [ "async-trait", "axum", @@ -3010,7 +3013,7 @@ dependencies = [ [[package]] name = "ras-rest-macro" -version = "0.3.0" +version = "0.3.1" dependencies = [ "async-trait", "axum", diff --git a/crates/core/ras-auth-core/Cargo.toml b/crates/core/ras-auth-core/Cargo.toml index 3d485bd..f27402b 100644 --- a/crates/core/ras-auth-core/Cargo.toml +++ b/crates/core/ras-auth-core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ras-auth-core" -version = "0.2.0" +version = "0.3.0" edition = "2024" rust-version = "1.88" description = "Core authentication and authorization traits for Rust Agent Stack services" @@ -15,6 +15,7 @@ http = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } subtle = { workspace = true } +tracing = { workspace = true } thiserror = { workspace = true } [dev-dependencies] diff --git a/crates/core/ras-auth-core/README.md b/crates/core/ras-auth-core/README.md index 713c7df..225a9b3 100644 --- a/crates/core/ras-auth-core/README.md +++ b/crates/core/ras-auth-core/README.md @@ -141,8 +141,15 @@ The default cookie is `HttpOnly`, `Secure`, `SameSite=Lax`, `Path=/`, and uses a `CsrfConfig::default()` uses a double-submit token: issue a CSRF cookie with `csrf_cookie_header_value(...)`, then have browser clients echo the same token in the `x-ras-csrf` header on cookie-authenticated `POST`, `PUT`, `PATCH`, and -`DELETE` requests. Use `header_presence_only(...)` only behind restrictive -credentialed CORS where a presence-only custom header is an intentional tradeoff. +`DELETE` requests. + +Two weaker modes exist and are named to discourage casual use: +`CsrfConfig::dangerous_header_presence_only(...)` (only requires the custom +header to be present; sound only behind restrictive credentialed CORS) and +`CsrfConfig::dangerous_static_value(...)` (a single process-wide value that is +not bound to a session). Configuring cookie auth with either logs a +`tracing::warn!`. The former names `header_presence_only` and +`with_expected_value` are deprecated aliases and will be removed. ## Usage diff --git a/crates/core/ras-auth-core/src/lib.rs b/crates/core/ras-auth-core/src/lib.rs index 18f24bb..a4bd561 100644 --- a/crates/core/ras-auth-core/src/lib.rs +++ b/crates/core/ras-auth-core/src/lib.rs @@ -14,7 +14,12 @@ pub use authorize::*; pub use transport::*; /// Errors that can occur during authentication or authorization. -#[derive(Debug, Error, Clone, Serialize, Deserialize)] +/// +/// Deliberately **not** `Serialize`/`Deserialize`: this is a server-side +/// diagnostic type. [`AuthError::InsufficientPermissions`] carries the caller's +/// full permission set in `has`, which must never be sent over the wire. Map +/// to a generic per-class client message instead (as the generated servers do). +#[derive(Debug, Error, Clone)] pub enum AuthError { /// The provided token is invalid or malformed. #[error("Invalid token")] @@ -25,7 +30,11 @@ pub enum AuthError { TokenExpired, /// The token does not have the required permissions. - #[error("Insufficient permissions: required {required:?}, has {has:?}")] + /// + /// The `Display` output names the required set and only the *count* of + /// permissions the caller holds; `has` is retained for server-side logging + /// via `Debug`. + #[error("Insufficient permissions: required {required:?}, caller holds {} permission(s)", has.len())] InsufficientPermissions { required: Vec, has: Vec, @@ -303,28 +312,47 @@ mod tests { } #[test] - fn auth_error_serializes_structured_permission_details() { + fn a2_insufficient_permissions_display_does_not_leak_held_permissions() { let error = AuthError::InsufficientPermissions { required: vec!["admin".to_string()], - has: vec!["user".to_string()], + has: vec!["user".to_string(), "billing:read".to_string()], }; - let value = serde_json::to_value(&error).expect("serialize auth error"); - assert_eq!( - value, - json!({ - "InsufficientPermissions": { - "required": ["admin"], - "has": ["user"] - } - }) - ); + let display = error.to_string(); + assert!(display.contains("required [\"admin\"]"), "{display}"); + assert!(display.contains("holds 2 permission(s)"), "{display}"); + assert!(!display.contains("user"), "{display}"); + assert!(!display.contains("billing:read"), "{display}"); - let decoded: AuthError = serde_json::from_value(value).expect("deserialize auth error"); - let AuthError::InsufficientPermissions { required, has } = decoded else { - panic!("expected insufficient permissions"); - }; - assert_eq!(required, vec!["admin"]); - assert_eq!(has, vec!["user"]); + // `has` is kept for server-side logging through `Debug`. + let debug = format!("{error:?}"); + assert!(debug.contains("billing:read"), "{debug}"); + } + + /// `AuthError` must not implement `Serialize` so it can never be emitted + /// on the wire by accident. Compile-time check via autoref specialization: + /// the `IsSerialize` impl on `Probe` wins when `T: Serialize`; otherwise + /// method lookup falls back to the `NotSerialize` impl on `&Probe`. + #[test] + fn a2_auth_error_is_not_serializable() { + struct Probe(std::marker::PhantomData); + trait NotSerialize { + fn is_serialize(&self) -> bool { + false + } + } + impl NotSerialize for &Probe {} + trait IsSerialize { + fn is_serialize(&self) -> bool { + true + } + } + impl IsSerialize for Probe {} + + let auth_error = &Probe::(std::marker::PhantomData); + assert!(!auth_error.is_serialize()); + // Sanity check that the probe detects a serializable type. + let user = &Probe::(std::marker::PhantomData); + assert!(user.is_serialize()); } } diff --git a/crates/core/ras-auth-core/src/transport.rs b/crates/core/ras-auth-core/src/transport.rs index 79dca80..92bb7bb 100644 --- a/crates/core/ras-auth-core/src/transport.rs +++ b/crates/core/ras-auth-core/src/transport.rs @@ -377,16 +377,29 @@ impl CsrfConfig { } } - /// Require the custom header to carry an exact value. + /// Require the custom header to carry a single, static, process-wide value. /// - /// This is intended for callers that validate a session-specific CSRF token - /// outside of the default double-submit cookie flow. - pub fn with_expected_value(mut self, expected_value: impl Into) -> Self { + /// **Dangerous.** A static value is not bound to a session: any attacker + /// who learns it once (from a leaked bundle, a shared client, or a single + /// captured request) can forge unsafe cookie-authenticated requests for + /// every user until the value is rotated. This disables the double-submit + /// cookie check. Prefer [`Self::default`] for browser sessions. + pub fn dangerous_static_value(mut self, expected_value: impl Into) -> Self { self.expected_value = Some(expected_value.into()); self.cookie_name = None; self } + /// Deprecated alias for [`Self::dangerous_static_value`]. + #[deprecated( + since = "0.3.0", + note = "renamed to `dangerous_static_value`; a static CSRF value is not \ + bound to a session and is a weak CSRF defense" + )] + pub fn with_expected_value(self, expected_value: impl Into) -> Self { + self.dangerous_static_value(expected_value) + } + /// Require the custom header to match this CSRF cookie. pub fn with_cookie_name(mut self, cookie_name: impl Into) -> Self { self.cookie_name = Some(cookie_name.into()); @@ -396,9 +409,12 @@ impl CsrfConfig { /// Require only a non-empty custom header. /// - /// This mode depends on restrictive credentialed CORS and is not a complete - /// CSRF defense by itself. Prefer [`Self::default`] for browser sessions. - pub fn header_presence_only(header_name: HeaderName) -> Self { + /// **Dangerous.** This mode relies entirely on the browser refusing to send + /// a custom header cross-origin without a successful CORS preflight. It is + /// only sound behind a restrictive credentialed CORS policy and is not a + /// complete CSRF defense by itself. Prefer [`Self::default`] for browser + /// sessions. + pub fn dangerous_header_presence_only(header_name: HeaderName) -> Self { Self { header_name, expected_value: None, @@ -406,6 +422,46 @@ impl CsrfConfig { } } + /// Deprecated alias for [`Self::dangerous_header_presence_only`]. + #[deprecated( + since = "0.3.0", + note = "renamed to `dangerous_header_presence_only`; presence-only CSRF \ + depends on restrictive CORS and is a weak CSRF defense" + )] + pub fn header_presence_only(header_name: HeaderName) -> Self { + Self::dangerous_header_presence_only(header_name) + } + + /// Whether this configuration uses one of the weak, opt-in modes + /// ([`Self::dangerous_static_value`] or + /// [`Self::dangerous_header_presence_only`]) rather than the default + /// double-submit cookie check. + /// + /// Returns the mode name for logging, or `None` for the double-submit mode. + pub fn dangerous_mode(&self) -> Option<&'static str> { + match (&self.expected_value, &self.cookie_name) { + (Some(_), _) => Some("static_value"), + (None, None) => Some("header_presence_only"), + (None, Some(_)) => None, + } + } + + /// Emit a `warn!` if this CSRF config is in a weak mode. Called from the + /// [`AuthTransportConfig`] builders (once per construction) and, as a + /// fallback for struct-literal construction, once per process from + /// [`AuthTransportConfig::validate`]. + fn warn_if_dangerous(&self) { + if let Some(mode) = self.dangerous_mode() { + tracing::warn!( + csrf_mode = mode, + csrf_header = %self.header_name, + "cookie auth is configured with a weak CSRF mode \ + (`CsrfConfig::dangerous_*`); this is not a complete CSRF defense. \ + Prefer the default double-submit cookie mode for browser sessions" + ); + } + } + /// Build a `Set-Cookie` header value for the double-submit CSRF token. /// /// The CSRF cookie is intentionally not `HttpOnly` so browser clients can @@ -424,7 +480,7 @@ impl CsrfConfig { pub fn validate(&self) -> Result<(), AuthTransportError> { // A CORS-safelisted or browser-controlled header name provides zero CSRF // protection (it is sent automatically cross-origin), so reject it — - // otherwise `header_presence_only(HeaderName::from_static("accept"))` + // otherwise `dangerous_header_presence_only(HeaderName::from_static("accept"))` // would produce a config that passes validation but never blocks a // forged request. let header = self.header_name.as_str(); @@ -546,15 +602,29 @@ impl AuthTransportConfig { if self.csrf.is_none() { self.csrf = Some(CsrfConfig::default()); } + self.warn_if_weak_csrf(); self } /// Enable CSRF protection for cookie-authenticated unsafe requests. + /// + /// Passing a `CsrfConfig::dangerous_*` mode together with cookie auth logs + /// a `warn!` at construction time. pub fn with_csrf(mut self, csrf: CsrfConfig) -> Self { self.csrf = Some(csrf); + self.warn_if_weak_csrf(); self } + /// Log a warning when cookie auth is paired with a weak CSRF mode. + fn warn_if_weak_csrf(&self) { + if self.cookie.is_some() + && let Some(csrf) = &self.csrf + { + csrf.warn_if_dangerous(); + } + } + /// Disable bearer-token extraction. pub fn without_bearer(mut self) -> Self { self.bearer = false; @@ -589,6 +659,26 @@ impl AuthTransportConfig { csrf.validate()?; } + // `validate` runs on every request, so the weak-mode warning is + // rate-limited here to once per distinct weak config per process. The + // builders (`with_cookie`, `with_csrf`) warn unconditionally at + // construction time; this is the fallback for struct-literal configs. + if self.cookie.is_some() + && let Some(csrf) = &self.csrf + && let Some(mode) = csrf.dangerous_mode() + { + static WEAK_CSRF_WARNED: std::sync::Mutex> = + std::sync::Mutex::new(Vec::new()); + let key = (csrf.header_name.as_str().to_string(), mode); + let mut warned = WEAK_CSRF_WARNED + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if !warned.contains(&key) { + warned.push(key); + csrf.warn_if_dangerous(); + } + } + Ok(()) } } @@ -950,7 +1040,7 @@ mod tests { fn csrf_expected_value_mode_does_not_require_csrf_cookie() { let config = AuthTransportConfig::default() .with_cookie(AuthCookieConfig::default()) - .with_csrf(CsrfConfig::default().with_expected_value("csrf-token")); + .with_csrf(CsrfConfig::default().dangerous_static_value("csrf-token")); let cookie = AuthCredential::new("cookie-token", AuthTokenSource::Cookie); let headers = headers(&[(DEFAULT_CSRF_HEADER, "csrf-token")]); @@ -1016,8 +1106,9 @@ mod tests { "cookie", "origin", ] { - let csrf = - CsrfConfig::header_presence_only(HeaderName::from_bytes(name.as_bytes()).unwrap()); + let csrf = CsrfConfig::dangerous_header_presence_only( + HeaderName::from_bytes(name.as_bytes()).unwrap(), + ); let error = csrf.validate().expect_err(name); assert!( matches!(error, AuthTransportError::InvalidCsrfConfig(_)), @@ -1026,7 +1117,8 @@ mod tests { } // A genuinely custom header (forces a CORS preflight) is accepted. - let ok = CsrfConfig::header_presence_only(HeaderName::from_static("x-csrf-token")); + let ok = + CsrfConfig::dangerous_header_presence_only(HeaderName::from_static("x-csrf-token")); assert!(ok.validate().is_ok()); } @@ -1085,4 +1177,129 @@ mod tests { HeaderValue::from_static("[REDACTED]") ); } + + /// Minimal `tracing` subscriber that records the messages of `WARN` events. + /// Kept dependency-free (no `tracing-subscriber`) since it only needs to + /// capture a handful of events for the A1 regression tests. + struct WarnCapture(std::sync::Mutex>); + + impl tracing::Subscriber for WarnCapture { + fn enabled(&self, metadata: &tracing::Metadata<'_>) -> bool { + *metadata.level() <= tracing::Level::WARN + } + fn new_span(&self, _: &tracing::span::Attributes<'_>) -> tracing::span::Id { + tracing::span::Id::from_u64(1) + } + fn record(&self, _: &tracing::span::Id, _: &tracing::span::Record<'_>) {} + fn record_follows_from(&self, _: &tracing::span::Id, _: &tracing::span::Id) {} + fn event(&self, event: &tracing::Event<'_>) { + struct Msg(String); + impl tracing::field::Visit for Msg { + fn record_debug( + &mut self, + field: &tracing::field::Field, + value: &dyn std::fmt::Debug, + ) { + self.0.push_str(&format!("{}={:?} ", field.name(), value)); + } + } + let mut msg = Msg(String::new()); + event.record(&mut msg); + self.0.lock().unwrap().push(msg.0); + } + fn enter(&self, _: &tracing::span::Id) {} + fn exit(&self, _: &tracing::span::Id) {} + } + + fn capture_warnings(f: impl FnOnce()) -> Vec { + let capture = std::sync::Arc::new(WarnCapture(std::sync::Mutex::new(Vec::new()))); + tracing::subscriber::with_default(capture.clone(), f); + capture.0.lock().unwrap().clone() + } + + #[test] + fn a1_dangerous_modes_are_reported_and_deprecated_aliases_still_work() { + let presence = + CsrfConfig::dangerous_header_presence_only(HeaderName::from_static("x-csrf-token")); + assert_eq!(presence.dangerous_mode(), Some("header_presence_only")); + + let static_value = CsrfConfig::default().dangerous_static_value("shared-secret"); + assert_eq!(static_value.dangerous_mode(), Some("static_value")); + + assert_eq!(CsrfConfig::default().dangerous_mode(), None); + assert_eq!( + CsrfConfig::default() + .with_cookie_name("__Host-other") + .dangerous_mode(), + None + ); + + // The deprecated names remain as thin wrappers for one release. + #[allow(deprecated)] + let legacy_presence = + CsrfConfig::header_presence_only(HeaderName::from_static("x-csrf-token")); + assert_eq!(legacy_presence, presence); + #[allow(deprecated)] + let legacy_static = CsrfConfig::default().with_expected_value("shared-secret"); + assert_eq!(legacy_static, static_value); + } + + #[test] + fn a1_cookie_auth_with_weak_csrf_mode_warns_at_construction() { + let warnings = capture_warnings(|| { + let _ = AuthTransportConfig::default() + .with_cookie(AuthCookieConfig::default()) + .with_csrf(CsrfConfig::dangerous_header_presence_only( + HeaderName::from_static("x-csrf-token"), + )); + }); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("csrf_mode=\"header_presence_only\"")); + assert!(warnings[0].contains("weak CSRF mode")); + + // Ordering does not matter: csrf first, then cookie. + let warnings = capture_warnings(|| { + let _ = AuthTransportConfig::default() + .with_csrf(CsrfConfig::default().dangerous_static_value("shared-secret")) + .with_cookie(AuthCookieConfig::default()); + }); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("csrf_mode=\"static_value\"")); + + // Weak CSRF without cookie auth is irrelevant (bearer-only) — no warning. + let warnings = capture_warnings(|| { + let _ = AuthTransportConfig::default().with_csrf( + CsrfConfig::dangerous_header_presence_only(HeaderName::from_static("x-csrf-token")), + ); + }); + assert!(warnings.is_empty(), "{warnings:?}"); + + // The default double-submit mode never warns. + let warnings = capture_warnings(|| { + let _ = AuthTransportConfig::default().with_cookie(AuthCookieConfig::default()); + }); + assert!(warnings.is_empty(), "{warnings:?}"); + } + + #[test] + fn a1_struct_literal_weak_csrf_warns_from_validate_once() { + // Struct-literal construction bypasses the builders, so `validate` + // warns as a fallback — but only once per distinct weak config per + // process, since it runs on every request. A header name unique to + // this test keeps it independent of test ordering. + let config = AuthTransportConfig { + bearer: true, + cookie: Some(AuthCookieConfig::default()), + csrf: Some(CsrfConfig::dangerous_header_presence_only( + HeaderName::from_static("x-a1-struct-literal-csrf"), + )), + }; + let warnings = capture_warnings(|| { + config.validate().unwrap(); + config.validate().unwrap(); + config.validate().unwrap(); + }); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("csrf_mode=\"header_presence_only\"")); + } } diff --git a/crates/core/ras-observability-core/Cargo.toml b/crates/core/ras-observability-core/Cargo.toml index f9fc64e..e4d241b 100644 --- a/crates/core/ras-observability-core/Cargo.toml +++ b/crates/core/ras-observability-core/Cargo.toml @@ -10,7 +10,7 @@ homepage = "https://github.com/JedimEmO/rust-api-stack" readme = "README.md" [dependencies] -ras-auth-core = { path = "../ras-auth-core", version = "0.2.0" } +ras-auth-core = { path = "../ras-auth-core", version = "0.3.0" } async-trait = { workspace = true } serde = { workspace = true } axum = { workspace = true } diff --git a/crates/identity/ras-identity-local/Cargo.toml b/crates/identity/ras-identity-local/Cargo.toml index 6f0e980..aa09c26 100644 --- a/crates/identity/ras-identity-local/Cargo.toml +++ b/crates/identity/ras-identity-local/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ras-identity-local" -version = "0.2.1" +version = "0.3.0" edition = "2024" rust-version = "1.88" description = "Local username/password authentication provider with Argon2 hashing" diff --git a/crates/identity/ras-identity-local/README.md b/crates/identity/ras-identity-local/README.md index c10982e..d27da3f 100644 --- a/crates/identity/ras-identity-local/README.md +++ b/crates/identity/ras-identity-local/README.md @@ -65,13 +65,15 @@ provider ) .await?; +let session_config = SessionConfig::new( + "fd2f56e597efef86b80c5484eb5247f4139b33bcdcb60dab", // openssl rand -hex 32 + "my-service", // iss + "my-service", // aud +)?; let session_service = Arc::new(SessionService::new(SessionConfig { - jwt_secret: "use-at-least-32-bytes-of-random-secret".to_string(), jwt_ttl: Duration::hours(1), - enforce_active_sessions: true, algorithm: JwtAlgorithm::HS256, - iss: None, - aud: None, + ..session_config })?); session_service.register_provider(Box::new(provider)).await; diff --git a/crates/identity/ras-identity-local/src/lib.rs b/crates/identity/ras-identity-local/src/lib.rs index 2af4f3b..fbaeb93 100644 --- a/crates/identity/ras-identity-local/src/lib.rs +++ b/crates/identity/ras-identity-local/src/lib.rs @@ -14,9 +14,15 @@ use std::fmt; use std::sync::Arc; use tokio::sync::RwLock; +/// Maximum accepted password length in bytes (I4). Longer inputs are rejected before +/// hashing so a client cannot make the server burn Argon2 time on multi-megabyte inputs. +pub const MAX_PASSWORD_BYTES: usize = 1024; + #[derive(Clone, Serialize, Deserialize)] pub struct LocalUser { pub username: String, + /// Argon2 PHC string. Never serialized (I1a); still required on deserialize. + #[serde(skip_serializing)] pub password_hash: String, pub email: Option, pub display_name: Option, @@ -36,12 +42,22 @@ impl fmt::Debug for LocalUser { } } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Deserialize)] pub struct LocalAuthPayload { pub username: String, pub password: String, } +/// Redacting `Debug` so the plaintext password never lands in logs (I2). +impl fmt::Debug for LocalAuthPayload { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("LocalAuthPayload") + .field("username", &self.username) + .field("password", &"[REDACTED]") + .finish() + } +} + /// Errors returned when managing local users. #[derive(Debug)] pub enum LocalUserError { @@ -49,6 +65,10 @@ pub enum LocalUserError { UserAlreadyExists { username: String }, /// Password hashing failed while creating the user. PasswordHash(argon2::password_hash::Error), + /// The password exceeds [`MAX_PASSWORD_BYTES`]. + PasswordTooLong { max_bytes: usize }, + /// The blocking hashing task was cancelled or panicked. + HashTaskFailed, } impl fmt::Display for LocalUserError { @@ -58,6 +78,10 @@ impl fmt::Display for LocalUserError { write!(f, "user '{username}' already exists") } Self::PasswordHash(error) => write!(f, "failed to hash password: {error}"), + Self::PasswordTooLong { max_bytes } => { + write!(f, "password exceeds maximum length of {max_bytes} bytes") + } + Self::HashTaskFailed => write!(f, "password hashing task failed"), } } } @@ -66,7 +90,9 @@ impl Error for LocalUserError { fn source(&self) -> Option<&(dyn Error + 'static)> { match self { Self::PasswordHash(error) => Some(error), - Self::UserAlreadyExists { .. } => None, + Self::UserAlreadyExists { .. } + | Self::PasswordTooLong { .. } + | Self::HashTaskFailed => None, } } } @@ -98,6 +124,12 @@ impl LocalUserProvider { email: Option, display_name: Option, ) -> Result<(), LocalUserError> { + if password.len() > MAX_PASSWORD_BYTES { + return Err(LocalUserError::PasswordTooLong { + max_bytes: MAX_PASSWORD_BYTES, + }); + } + { let users = self.users.read().await; if users.contains_key(&username) { @@ -105,11 +137,15 @@ impl LocalUserProvider { } } - let argon2 = Argon2::default(); - let salt = SaltString::generate(&mut OsRng); - let password_hash = argon2 - .hash_password(password.as_bytes(), &salt)? - .to_string(); + // Argon2 is CPU-bound; keep it off the async executor (I4). + let password_hash = tokio::task::spawn_blocking(move || { + let salt = SaltString::generate(&mut OsRng); + Argon2::default() + .hash_password(password.as_bytes(), &salt) + .map(|hash| hash.to_string()) + }) + .await + .map_err(|_| LocalUserError::HashTaskFailed)??; let user = LocalUser { username: username.clone(), @@ -139,23 +175,40 @@ impl LocalUserProvider { self.semaphore.clone().acquire_owned().await.map_err(|_| { IdentityError::ProviderError("local auth limiter closed".to_string()) })?; - let users = self.users.read().await; + + // Reject oversized passwords before spending Argon2 time on them (I4). Same error + // as a wrong password so nothing is leaked about the account. + if password.len() > MAX_PASSWORD_BYTES { + return Err(IdentityError::InvalidCredentials); + } // Verify missing users against a fixed sentinel hash to keep timing consistent. const SENTINEL_HASH: &str = "$argon2id$v=19$m=19456,t=2,p=1$9QsJRKgzJkKaOUvlp7gl2Q$qmE3qIFBNJ6nZYbLYXEI2uo0zZc7T0Q8LU1ZsqsZ3QE"; - let (user, password_hash) = if let Some(user) = users.get(username) { - (Some(user.clone()), user.password_hash.as_str()) - } else { - (None, SENTINEL_HASH) + // Clone the stored hash out of the lock so verification never holds it (I4). + let (user, password_hash) = { + let users = self.users.read().await; + match users.get(username) { + Some(user) => (Some(user.clone()), user.password_hash.clone()), + None => (None, SENTINEL_HASH.to_string()), + } }; - let parsed_hash = PasswordHash::new(password_hash) - .map_err(|e| IdentityError::ProviderError(e.to_string()))?; - - let password_valid = Argon2::default() - .verify_password(password.as_bytes(), &parsed_hash) - .is_ok(); + // Argon2 is CPU-bound; run it on the blocking pool (I4). + let password = password.to_string(); + let password_valid = tokio::task::spawn_blocking(move || { + let parsed_hash = PasswordHash::new(&password_hash) + .map_err(|e| IdentityError::ProviderError(e.to_string()))?; + Ok::( + Argon2::default() + .verify_password(password.as_bytes(), &parsed_hash) + .is_ok(), + ) + }) + .await + .map_err(|_| { + IdentityError::ProviderError("password verification task failed".to_string()) + })??; // Only succeed if both user exists AND password is valid. if password_valid { @@ -217,6 +270,77 @@ mod tests { assert!(debug.contains("alice")); } + #[test] + fn i1a_local_user_serialize_omits_password_hash() { + let user = LocalUser { + username: "alice".to_string(), + password_hash: "$argon2id$v=19$m=19456,t=2,p=1$secretsecret$hashhashhash".to_string(), + email: Some("alice@example.com".to_string()), + display_name: None, + metadata: None, + }; + let json = serde_json::to_value(&user).unwrap(); + assert!(json.get("password_hash").is_none()); + assert!(!json.to_string().contains("hashhashhash")); + assert_eq!(json["username"], "alice"); + + // Deserialize still requires the hash. + let full = serde_json::json!({ + "username": "alice", + "password_hash": "$argon2id$x", + "email": null, + "display_name": null, + "metadata": null + }); + let parsed: LocalUser = serde_json::from_value(full).unwrap(); + assert_eq!(parsed.password_hash, "$argon2id$x"); + assert!(serde_json::from_value::(json).is_err()); + } + + #[test] + fn i2_login_payload_debug_redacts_password() { + let payload = LocalAuthPayload { + username: "alice".to_string(), + password: "hunter2-super-secret".to_string(), + }; + let debug = format!("{payload:?}"); + assert!(!debug.contains("hunter2")); + assert!(debug.contains("[REDACTED]")); + assert!(debug.contains("alice")); + } + + #[tokio::test] + async fn i4_oversized_password_rejected_before_hashing() { + let provider = setup_test_provider().await; + + let too_long = "x".repeat(MAX_PASSWORD_BYTES + 1); + let result = provider + .add_user("bob".to_string(), too_long.clone(), None, None) + .await; + assert!(matches!( + result, + Err(LocalUserError::PasswordTooLong { max_bytes }) if max_bytes == MAX_PASSWORD_BYTES + )); + + let result = provider + .verify(serde_json::json!({ "username": "testuser", "password": too_long })) + .await; + assert!(matches!(result, Err(IdentityError::InvalidCredentials))); + + // Exactly at the limit is still accepted. + let at_limit = "y".repeat(MAX_PASSWORD_BYTES); + provider + .add_user("carol".to_string(), at_limit.clone(), None, None) + .await + .unwrap(); + assert!( + provider + .verify(serde_json::json!({ "username": "carol", "password": at_limit })) + .await + .is_ok() + ); + } + async fn setup_test_provider() -> LocalUserProvider { let provider = LocalUserProvider::new(); diff --git a/crates/identity/ras-identity-oauth2/Cargo.toml b/crates/identity/ras-identity-oauth2/Cargo.toml index 1bf4249..4726095 100644 --- a/crates/identity/ras-identity-oauth2/Cargo.toml +++ b/crates/identity/ras-identity-oauth2/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ras-identity-oauth2" -version = "0.2.0" +version = "0.3.0" edition = "2024" rust-version = "1.88" description = "OAuth2 authentication provider with Google support, PKCE, and state management" @@ -28,10 +28,11 @@ base64 = { workspace = true } sha2 = { workspace = true } rand = { workspace = true } url = { workspace = true } +subtle = { workspace = true } [dev-dependencies] axum-test = { workspace = true } tracing-subscriber = { workspace = true } # For the example -ras-identity-session = { path = "../ras-identity-session", version = "0.3.0" } +ras-identity-session = { path = "../ras-identity-session", version = "0.4.0" } diff --git a/crates/identity/ras-identity-oauth2/README.md b/crates/identity/ras-identity-oauth2/README.md index b9d8f78..e308035 100644 --- a/crates/identity/ras-identity-oauth2/README.md +++ b/crates/identity/ras-identity-oauth2/README.md @@ -45,6 +45,10 @@ let google_config = OAuth2ProviderConfig { auth_params: HashMap::new(), use_pkce: true, user_info_mapping: None, + // Extra userinfo claims to copy into session metadata (and the JWT). Empty = none. + metadata_claims: Vec::new(), + // Endpoints must be https:// unless this is set (local mock IdP only). + allow_insecure_endpoints: false, }; // Create OAuth2 configuration @@ -67,7 +71,11 @@ use ras_identity_session::{SessionConfig, SessionService}; // Register with session service. The provider is cheap to clone; keep one // handle for flow initiation and register the other for verification. -let session_config = SessionConfig::new("use-at-least-32-bytes-of-random-secret")?; +let session_config = SessionConfig::new( + "50f60a216877ea8c01d90deebfaf37ee95297d744bca0931", // openssl rand -hex 32 + "my-service", // iss + "my-service", // aud +)?; let session_service = SessionService::new(session_config)?; session_service.register_provider(Box::new(oauth2_provider.clone())).await; @@ -128,6 +136,8 @@ For a non-browser flow where login CSRF does not apply, use - `auth_params`: Additional authorization parameters - `use_pkce`: Enable PKCE for enhanced security - `user_info_mapping`: Custom field mapping for user info +- `metadata_claims`: Allow-list of additional userinfo claims copied into the identity metadata (and therefore the session JWT). Defaults to empty; only `picture` and `email_verified` are ever propagated without it +- `allow_insecure_endpoints`: Permit non-`https://` endpoint URLs. Defaults to `false`; `OAuth2Provider::new`/`try_new`/`add_provider` reject insecure endpoints unless set. Only enable for a local mock IdP ### OAuth2Config diff --git a/crates/identity/ras-identity-oauth2/examples/google_oauth2.rs b/crates/identity/ras-identity-oauth2/examples/google_oauth2.rs index 114a340..fc00319 100644 --- a/crates/identity/ras-identity-oauth2/examples/google_oauth2.rs +++ b/crates/identity/ras-identity-oauth2/examples/google_oauth2.rs @@ -38,6 +38,8 @@ async fn main() -> Result<(), Box> { auth_params: HashMap::new(), use_pkce: true, // Enable PKCE for security user_info_mapping: None, // Use default mapping + metadata_claims: Vec::new(), + allow_insecure_endpoints: false, }; // Create OAuth2 configuration @@ -53,8 +55,12 @@ async fn main() -> Result<(), Box> { let oauth2_provider = OAuth2Provider::new(oauth2_config, state_store); // Create session service - let session_config = - SessionConfig::new("oauth2-example-secret-that-is-long-enough-for-tests").unwrap(); + let session_config = SessionConfig::new( + "50f60a216877ea8c01d90deebfaf37ee95297d744bca0931", // openssl rand -hex 32 + "google-oauth2-example", + "google-oauth2-example", + ) + .unwrap(); let session_service = SessionService::new(session_config).unwrap(); // Register OAuth2 provider with session service @@ -168,6 +174,8 @@ mod tests { auth_params: HashMap::new(), use_pkce: true, user_info_mapping: None, + metadata_claims: Vec::new(), + allow_insecure_endpoints: false, }; let config = OAuth2Config::new().add_provider(google_config); diff --git a/crates/identity/ras-identity-oauth2/src/client.rs b/crates/identity/ras-identity-oauth2/src/client.rs index 824b725..e1be1f2 100644 --- a/crates/identity/ras-identity-oauth2/src/client.rs +++ b/crates/identity/ras-identity-oauth2/src/client.rs @@ -11,7 +11,8 @@ use sha2::{Digest, Sha256}; use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; -use tracing::{debug, error, info}; +use subtle::ConstantTimeEq; +use tracing::{debug, error, info, warn}; use url::Url; #[async_trait::async_trait] @@ -41,7 +42,13 @@ impl OAuth2HttpTransport for ReqwestOAuth2HttpTransport { token_endpoint: &str, params: &HashMap, ) -> OAuth2Result { - let response = self.client.post(token_endpoint).form(params).send().await?; + let response = self + .client + .post(token_endpoint) + .form(params) + .send() + .await + .map_err(log_upstream_error)?; if !response.status().is_success() { // Never log or propagate the raw provider response body — it can @@ -53,10 +60,11 @@ impl OAuth2HttpTransport for ReqwestOAuth2HttpTransport { ))); } - let token_response: TokenResponse = response - .json() - .await - .map_err(|e| OAuth2Error::InvalidTokenResponse(e.to_string()))?; + let token_response: TokenResponse = response.json().await.map_err(|e| { + // reqwest decode errors embed the request URL; keep that in the log only (I6). + warn!(error = %e, "token endpoint returned an undecodable response"); + OAuth2Error::InvalidTokenResponse("undecodable token response".to_string()) + })?; info!("Successfully exchanged code for tokens"); Ok(token_response) @@ -72,7 +80,8 @@ impl OAuth2HttpTransport for ReqwestOAuth2HttpTransport { .get(userinfo_endpoint) .bearer_auth(access_token) .send() - .await?; + .await + .map_err(log_upstream_error)?; if !response.status().is_success() { // Status only; the raw body may echo the bearer token (L1). @@ -83,10 +92,10 @@ impl OAuth2HttpTransport for ReqwestOAuth2HttpTransport { ))); } - let user_info: UserInfoResponse = response - .json() - .await - .map_err(|e| OAuth2Error::InvalidUserInfoResponse(e.to_string()))?; + let user_info: UserInfoResponse = response.json().await.map_err(|e| { + warn!(error = %e, "userinfo endpoint returned an undecodable response"); + OAuth2Error::InvalidUserInfoResponse("undecodable userinfo response".to_string()) + })?; debug!( "Successfully retrieved user info for subject: {}", @@ -352,30 +361,35 @@ impl OAuth2Client { } // When the flow was bound to a browser session, the callback must - // present the identical binding value (login-CSRF guard). - if state.binding.is_some() && state.binding != callback_response.binding { + // present the identical binding value (login-CSRF guard). Compared in + // constant time so the binding cannot be recovered byte-by-byte (I7). + if let Some(expected) = &state.binding + && !binding_matches(expected, callback_response.binding.as_deref()) + { return Err(OAuth2Error::InvalidState); } - // Check for errors in callback + // Check for errors in callback. Only the standardized error code is + // surfaced; the free-text description stays in the server log (I5). if let Some(error) = &callback_response.error { - let error_desc = callback_response - .error_description - .as_deref() - .unwrap_or("No description"); - return Err(OAuth2Error::CallbackError(format!( - "{}: {}", - error, error_desc - ))); + warn!( + provider = %provider_config.provider_id, + error = %error, + error_description = callback_response.error_description.as_deref().unwrap_or(""), + "OAuth2 provider returned an error on callback" + ); + return Err(OAuth2Error::ProviderDenied { + error: error.clone(), + }); } + let Some(code) = callback_response.code.as_deref() else { + return Err(OAuth2Error::InvalidCallback); + }; + // Exchange authorization code for tokens let token_response = self - .exchange_code( - provider_config, - &callback_response.code, - state.code_verifier.as_deref(), - ) + .exchange_code(provider_config, code, state.code_verifier.as_deref()) .await?; // Validate id_token claims when the provider returned one. The token @@ -472,6 +486,26 @@ fn decode_id_token_claims(id_token: &str) -> OAuth2Result { /// The signature is not verified: the token was received directly from the /// token endpoint over TLS, which OIDC Core §3.1.3.7 permits as a substitute /// for signature validation in the authorization-code flow. +/// Log a transport-level failure at `warn` (the `reqwest::Error` carries the +/// request URL) and hand back the fixed-message error variant (I6). +fn log_upstream_error(error: reqwest::Error) -> OAuth2Error { + warn!(error = %error, "upstream OAuth2 request failed"); + OAuth2Error::HttpError(error) +} + +/// Constant-time comparison of the stored session binding against the value +/// presented on callback (I7). A missing callback value never matches. +fn binding_matches(expected: &str, presented: Option<&str>) -> bool { + match presented { + Some(presented) => { + // `ct_eq` on slices short-circuits on length, but the length of the + // binding is not secret (it is a UUID or caller-chosen value). + expected.as_bytes().ct_eq(presented.as_bytes()).into() + } + None => false, + } +} + pub(crate) fn validate_id_token_claims( provider_config: &OAuth2ProviderConfig, id_token: &str, @@ -638,6 +672,8 @@ mod tests { auth_params: HashMap::new(), use_pkce: true, user_info_mapping: None, + metadata_claims: Vec::new(), + allow_insecure_endpoints: false, } } @@ -747,7 +783,7 @@ mod tests { .handle_callback( &wrong_provider, AuthorizationResponse { - code: "auth-code".to_string(), + code: Some("auth-code".to_string()), state, error: None, error_description: None, @@ -762,7 +798,7 @@ mod tests { } #[tokio::test] - async fn handle_callback_returns_provider_callback_error_without_transport_call() { + async fn i5_handle_callback_maps_provider_error_to_fixed_variant_without_description() { let state_store = Arc::new(InMemoryStateStore::new()); let transport = Arc::new(RecordingTransport::new()); let client = client_with_transport(state_store, transport.clone()); @@ -773,29 +809,80 @@ mod tests { .await .unwrap(); + // A legitimate denial carries no code at all. let error = client .handle_callback( &provider_config, AuthorizationResponse { - code: "ignored-code".to_string(), + code: None, state, error: Some("access_denied".to_string()), - error_description: Some("user denied consent".to_string()), + error_description: Some("user denied consent