refactor!: secure JWT authentication with RS256 and token revocation (ADR-T-007)#853
Conversation
d11aad4 to
e96e720
Compare
…hase 1) Introduce `src/jwt.rs` — a single module that owns all `jsonwebtoken` usage (key loading, signing, verification, algorithm selection). Key changes: - Move `JsonWebToken` service from `services::authentication` to `jwt` module, covering both session and email-verification tokens. - Move `UserClaims` from `models::user` and `VerifyClaims` from `mailer` into `jwt`, co-located with the code that produces and consumes them. - Rename config field `user_claim_token_pepper` → `jwt_signing_secret` (with legacy alias accepted during config loading). - Add `session_token_lifetime_secs` and `email_verification_token_lifetime_secs` config options, replacing the hard-coded magic numbers. - Make `sign()` return `Result` instead of panicking on encoding failure. - Make `parse_token()` return `Result<String, AuthError>` instead of panicking on malformed `Authorization` headers. - Propagate the `JsonWebToken` service via `Arc` into `mailer::Service` and `user::RegistrationService`, removing their direct `jsonwebtoken` crate dependencies. BREAKING CHANGE: The config key `auth.user_claim_token_pepper` is renamed to `auth.jwt_signing_secret`. The old key is still accepted at startup but the canonical name has changed. The settings JSON response now includes `jwt_signing_secret`, `session_token_lifetime_secs`, and `email_verification_token_lifetime_secs` fields.
… and RFC 7519 claims
Split the single shared JWT signing secret into two separate keys:
`session_signing_key` and `email_verification_signing_key`, so a
compromise of one key does not affect the other token type.
Session tokens (`SessionClaims`, née `UserClaims`) now carry proper
RFC 7519 registered claims: `sub` (user ID), `iss` ("torrust-index"),
`aud` ("session"), `iat`, and `exp`. The `role` and `username` fields
are advisory only — the authoritative role is always re-checked from
the database on each authenticated request.
Email-verification tokens (`VerifyClaims`) gain an `aud`
("email-verification") claim and `iat`, and are now validated with
audience + issuer checks by the `jsonwebtoken` library instead of a
manual `iss` string comparison.
Other changes:
- Enforce a 32-byte minimum length on `JwtSigningSecret`.
- Accept `jwt_signing_secret` and `user_claim_token_pepper` as legacy
serde aliases for `session_signing_key` for backward compatibility.
- Provide `UserClaims` as a type alias so existing imports keep
compiling.
- Update all config files, doc examples, and tests.
Ref: ADR-T-007
…DR-T-007 Phase 3) Replace the shared HMAC secret (`session_signing_key` / `email_verification_signing_key`) with an RSA-2048 key pair for all JWT operations. Key changes: - Sign tokens with `EncodingKey` (private) and verify with `DecodingKey` (public), both resolved once at startup from PEM files or inline PEM configuration. - Include a `kid` (Key ID) header derived from the SHA-256 hash of the public key, preparing for future key rotation. - Ship a development key pair at `share/default/jwt/` with a loud startup warning when it is used. - Remove `JwtSigningSecret`, the mandatory-option check for `auth.session_signing_key`, and all three serde aliases. - Verification (`verify`, `verify_email_token`) is now synchronous since keys are pre-loaded — no config lock needed per request. - Update all config files, container scripts, tests, and docs to use `private_key_path` / `public_key_path`. BREAKING CHANGE: The `auth.session_signing_key` and `auth.email_verification_signing_key` config fields are removed. Deployers must provide an RSA key pair via `auth.private_key_path` / `auth.public_key_path` (or the inline `_pem` variants). Existing HS256 tokens will be rejected after upgrade.
e96e720 to
b7ec439
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #853 +/- ##
===========================================
- Coverage 53.76% 53.54% -0.23%
===========================================
Files 116 118 +2
Lines 6182 6441 +259
Branches 6182 6441 +259
===========================================
+ Hits 3324 3449 +125
- Misses 2776 2893 +117
- Partials 82 99 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…T-007 Phase 4) Add a `token_generation` column to `torrust_users` and embed a `gen` claim in every session JWT. Password changes, admin-role grants, and bans increment the counter, instantly invalidating all outstanding tokens for that user. Revocation is checked at three entry points (defence in depth): - `Authentication::get_user_id_from_bearer_token` - `verify_token_handler` - `authentication::Service::renew_token` Rework the `BearerToken` extractor to reject missing or malformed `Authorization` headers at the extraction boundary instead of deferring the check downstream. Remove the `bearer_token::Extract` wrapper and `get_optional_logged_in_user` free function; the logic now lives directly in the extractors. Add `AuthError::TokenRevoked` for revoked-token responses. Add crate tests for the JWT module (sign/verify round-trips, audience cross-contamination, tampered tokens) and for `parse_token` (whitespace trimming, empty bearer, non-ASCII rejection).
b7ec439 to
4adbe3c
Compare
Copy the RS256 key pair into the volume-mounted storage path during E2E install for both SQLite and MySQL setups, aligning the container environment with the HMAC→RS256 migration (ADR-T-007 Phase 3). Also make `wait_for_container_to_be_healthy.sh` directly invocable by forwarding arguments to the function it defines.
…-T-007 Phase 5)
Remove the development RSA key pair shipped at `share/default/jwt/`
and replace it with on-demand ephemeral key generation. When no
`private_key_path`/`public_key_path` or inline PEM values are
configured, the server auto-generates an RSA-2048 key pair in memory
at startup via the `rsa` crate. Sessions work immediately but do not
survive restarts — deployers who want persistent sessions supply
their own key pair.
Key changes:
- `Auth::resolve_{private,public}_key_pem()` now return `Option`
instead of panicking, letting `JsonWebToken::new()` decide whether
to use host-supplied or ephemeral keys.
- `JsonWebToken::new()` matches on (Some, Some) / (None, None) and
panics on mismatched key configuration.
- `generate_ephemeral_key_pair()` offloads CPU-heavy RSA generation
to a blocking Tokio task.
- Default config no longer includes key paths; all container configs,
e2e scripts, compose.yaml, and docs updated accordingly.
- Tests now exercise the ephemeral code path by default.
- ADR-T-007 updated with Phase 5/6 design, migration notes, and
testing strategy.�
…R-T-007 Phase 6) Add the `torrust-generate-auth-keypair` binary that generates an RSA-2048 key pair and writes both PEM blocks to stdout. The tool refuses to run when stdout is a terminal, uses structured JSON tracing on stderr, and supports a `--debug` flag for verbose output. The container entry script now auto-generates persistent auth keys on first boot into `/etc/torrust/index/auth/` with restrictive permissions (0400 private, 0440 public). A `mktemp` + `trap` pattern ensures key material is never world-readable and the temp file is cleaned up on interruption. Changes: - New binary: src/bin/generate_auth_keypair.rs - Containerfile: copy new binary in both debug and release stages - entry_script_sh: key-generation block before `exec su-exec` - All container configs: set auth key paths to /etc/torrust/index/auth/ - ADR-007: update Phase 6 to reflect implementation - docs/containers.md: document auth/ directory and volume layout - README: note container auto-generation for new users
fb98c4a to
49504f6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 59 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 59 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ff6966e to
9a25d3c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 59 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…DR-T-007)
Introduce atomic database methods that combine state changes with
`token_generation` bumps in a single transaction, closing the race
window where a password change or ban could commit but the generation
increment could fail — leaving revoked credentials with valid tokens:
- `change_user_password_and_revoke_tokens` (transaction)
- `ban_user_and_revoke_tokens` (transaction)
- `grant_admin_role_and_revoke_tokens` (single UPDATE)
Harden token-generation checking from `<` to `!=` (exact match) at
all three validation sites so tokens are also rejected when the
database generation *decreases* (e.g. restore from backup).
Add defence-in-depth `is_user_banned` checks after generation
validation — if `token_generation` somehow matches despite an active
ban, the ban table catches it.
Other fixes included in this changeset:
- `BearerToken::value()` → `as_str()`: return `&str` instead of
cloning.
- `parse_token`: require the space in `"Bearer "` prefix
(`strip_prefix("Bearer")` → `strip_prefix("Bearer ")`).
- `get_token_generation`: distinguish `RowNotFound` from other
errors; replace `unwrap_or(0)` with proper error propagation.
- `Settings::remove_secrets_from_settings`: conditionally redact
`Option` key fields instead of unconditionally overwriting `None`
with `Some("***")`. Also redact public key pem/path.
- Warn when configured key paths don't exist (falling back to
ephemeral keys).
- Replace hardcoded JWT tokens in doc examples with `<JWT_TOKEN>`.
- Fix doc typo: "JWT is not invalid" → "JWT is invalid".
Condense ADR-T-007 and add Phase 7 (consolidate session validation)
design.
…ADR-T-007 Phase 7) Introduce `JsonWebToken::validate_session` as the sole entry point for session-token validation — JWT signature/expiry verification, token- generation counter check, and banned-user rejection all happen in one place. Three call sites that previously re-implemented this sequence now delegate to `validate_session`: - `Authentication::get_user_id_from_bearer_token` - `verify_token_handler` - `authentication::Service::renew_token` This removes ~45 lines of duplicated validation logic and eliminates the private `Authentication::validate_token_generation` helper. Mark ADR-T-007 Phase 7 as complete.
9a25d3c to
593276a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 59 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Parse Bearer scheme case-insensitively per RFC 7235 §2.1 - Replace `parse_from_str` + `expect` with a `LazyLock` constant for the permanent-ban expiry date, removing two panic sites - Propagate `is_user_banned` errors with `?` instead of swallowing them via `unwrap_or(false)` - Use `saturating_sub` for token-expiry arithmetic to avoid underflow
|
ACK 6126eec |
1 similar comment
|
ACK 6126eec |
Summary
Replace the JWT authentication system end-to-end: HMAC-HS256 shared secret → RS256 asymmetric signing, RFC 7519 claims, per-user token revocation with atomic database operations, ephemeral key auto-generation, a CLI for persistent key provisioning, and consolidated session validation via a single code path. Implemented across 7 phases in 9 incremental commits (ADR-T-007).
Motivation
The previous setup used a single low-entropy HMAC-HS256 secret for all token types, had no standard registered claims, baked stale role data into payloads with no way to invalidate it, hard-coded expiration durations, panicked on encode/decode failures, and offered no mechanism to revoke tokens on password change or ban.
Changes by phase
b6f51c3jwt.rsmodule centralises alljsonwebtokenusage;sign()/parse_token()returnResult; configurable token lifetimes replace hard-coded durations; config key renamed tojwt_signing_secret8411e7dsession/email_verification); RFC 7519 registered claims (sub,iss,aud,iat,exp); 32-byte minimum key length enforced; role/username fields marked advisory-only40fe5e5kidheader (SHA-256 of public key) for future key rotation; verification is synchronous since keys are pre-loaded at startup4adbe3ctoken_generationcolumn added totorrust_users(MySQL + SQLite migrations);genclaim in session JWTs; password changes, role grants, and bans increment the counter, invalidating all outstanding tokens7c8ca724183c1249504f6torrust-generate-auth-keypairbinary writes PEM key pair to stdout; container entry script auto-generates persistent keys on first boot into/etc/torrust/index/auth/with restrictive permissionse23cd3ftransaction/ singleUPDATE); generation check tightened from<to!=; defence-in-depthis_user_bannedfallback;BearerToken::value()→as_str();parse_tokenrequires space after"Bearer"prefix593276aJsonWebToken::validate_sessionbecomes the sole entry point for session-token validation — JWT signature/expiry, generation counter, and ban check all happen in one place; removes ~45 lines of duplicated logic across three call sitesBreaking changes
auth.user_claim_token_pepper→auth.jwt_signing_secret(legacy alias accepted).auth.session_signing_keyandauth.email_verification_signing_keyremoved. Replaced byauth.private_key_path/auth.public_key_path(or inline_pemvariants). Existing HS256 tokens are rejected after upgrade.token_generation BIGINT/INTEGER NOT NULL DEFAULT 0column ontorrust_users(migration included for both MySQL and SQLite).BearerTokenextractor rejects missing or malformedAuthorizationheaders at the extraction boundary.get_optional_logged_in_userremoved; logic moved into extractors.New files
validate_sessionparse_tokenwhitespace trimming, empty bearer, non-ASCII rejectionmigrations/*/20260414000000_torrust_user_token_generation.sqlTesting
References