Efren Yevale Varela 1 месяц назад
Родитель
Сommit
84ddbe2695
8 измененных файлов с 169 добавлено и 31 удалено
  1. 10 0
      docs/CHANGELOG.md
  2. 9 2
      docs/SECURITY.md
  3. 1 24
      docs/TODO.md
  4. 21 3
      src/middleware/security_headers.rs
  5. 25 0
      src/routes.rs
  6. 62 0
      src/state/health.rs
  7. 18 0
      src/state/test_support.rs
  8. 23 2
      src/state/tests_topology.rs

+ 10 - 0
docs/CHANGELOG.md

@@ -33,6 +33,16 @@ While the version stays below `1.0.0`, a minor bump may carry a breaking change.
   hand-typing. The new `qrcode` dependency is pinned SVG-only (no `image`
   hand-typing. The new `qrcode` dependency is pinned SVG-only (no `image`
   codecs); the text secret, URI, and one-time recovery codes are unchanged.
   codecs); the text secret, URI, and one-time recovery codes are unchanged.
 
 
+### Changed
+
+- `GET /.well-known/oauth-authorization-server` now carries an explicit
+  `Cache-Control: public, max-age=3600` instead of relying on browser
+  heuristic freshness, so metadata caching is intentional and tunable. The
+  plaintext health listener (which does not mount the public router's
+  security-header layer) now sets `Cache-Control: no-store` and
+  `X-Content-Type-Options: nosniff` on its own probe responses, with a
+  regression test pinning both.
+
 ### Fixed
 ### Fixed
 
 
 - `GET /.well-known/oauth-authorization-server` reads the enabled scope catalog
 - `GET /.well-known/oauth-authorization-server` reads the enabled scope catalog

+ 9 - 2
docs/SECURITY.md

@@ -83,8 +83,15 @@ tracked in `docs/TODO.md`.
   session-bound CSRF token and the bound `request_id`), the `/authorize`
   session-bound CSRF token and the bound `request_id`), the `/authorize`
   redirects and error responses, the token endpoint (success and error), PAR,
   redirects and error responses, the token endpoint (success and error), PAR,
   device authorization, DCR, introspection, and revocation.
   device authorization, DCR, introspection, and revocation.
-- OAuth metadata discovery is intentionally left cacheable; a metadata handler
-  that fails closed returns a `503` error, which is itself `no-store`.
+- The plaintext health listener does not mount the security-header layer, so
+  the health router inserts `Cache-Control: no-store` and
+  `X-Content-Type-Options: nosniff` on each probe response itself. A regression
+  test pins both headers so a route added to that listener cannot silently drop
+  the policy.
+- OAuth metadata discovery is cacheable by design and carries an explicit
+  positive `Cache-Control: public, max-age=3600` (not browser heuristic
+  freshness). A metadata handler that fails closed returns a `503` error, which
+  is itself `no-store`.
 
 
 ## Opaque tokens, hashed at rest
 ## Opaque tokens, hashed at rest
 
 

+ 1 - 24
docs/TODO.md

@@ -175,34 +175,13 @@ the mitigations to implement.
 - **Proposed change.** Restrict who may exec and audit it.
 - **Proposed change.** Restrict who may exec and audit it.
 - **Files.** `docs/OPERATIONS.md`, `docs/DEPLOYMENT.md`.
 - **Files.** `docs/OPERATIONS.md`, `docs/DEPLOYMENT.md`.
 
 
-## HTTP response hardening
-
-### 16. Make the HTTP cache policy explicit on every endpoint
-
-- **Risk.** `Cache-Control: no-store` is applied at a single choke point on the
-  public router (`src/middleware/security_headers.rs`), with `/.well-known/…`
-  metadata exempt, so it covers every OAuth endpoint today. The policy is
-  nevertheless implicit: metadata discovery is merely left unmarked and its
-  caching relies on browser heuristics, the plaintext health listener never
-  passes through the layer, and a handler exercised off-router would carry no
-  header. A new route mounted outside `build_router_with_options` would silently
-  miss the policy.
-- **Proposed change.** Make the policy explicit and complete: give
-  `/.well-known/oauth-authorization-server` a positive `Cache-Control` (for
-  example `public, max-age=…`) so metadata caching is intentional and tunable;
-  mark the health probes `no-store` (or mount the security-header layer on the
-  health listener); and consider `Vary: Cookie` on the authenticated pages as
-  defense in depth. Keep metadata cacheable; do not extend `no-store` to it.
-- **Files.** `src/middleware/security_headers.rs`, `src/routes.rs`,
-  `src/serve.rs`, `src/state/health.rs`, `docs/SECURITY.md`.
-
 ## Priority
 ## Priority
 
 
 Ranked by value against effort:
 Ranked by value against effort:
 
 
 - **High value, low effort:** items 1, 2, 5.
 - **High value, low effort:** items 1, 2, 5.
 - **Medium:** items 3, 4.
 - **Medium:** items 3, 4.
-- **Lower, mostly operational:** items 6 and 16.
+- **Lower, mostly operational:** item 6.
 - **Container/host hardening:** items 7, 8, 9, 10 are high value and low effort;
 - **Container/host hardening:** items 7, 8, 9, 10 are high value and low effort;
   items 11, 12, 13 are medium/operational.
   items 11, 12, 13 are medium/operational.
 
 
@@ -220,5 +199,3 @@ items 7 through 10 are high-value, low-effort container and host controls.
 - Should the pepper and TOTP key move to file-based secrets now, or wait for the
 - Should the pepper and TOTP key move to file-based secrets now, or wait for the
   envelope-encryption work (item 3)?
   envelope-encryption work (item 3)?
 - Is a sandboxed runtime class (gVisor/Kata) available in the target cluster?
 - Is a sandboxed runtime class (gVisor/Kata) available in the target cluster?
-- Should metadata discovery carry an explicit positive `Cache-Control`, or is
-  leaving it to browser heuristics acceptable (item 16)?

+ 21 - 3
src/middleware/security_headers.rs

@@ -49,6 +49,12 @@ pub const CORS_MAX_AGE_SECS: u64 = 600;
 pub const NO_STORE_VALUE: &str = "no-store";
 pub const NO_STORE_VALUE: &str = "no-store";
 /// Legacy `Pragma` companion to `no-store` (RFC 6749 §5.1).
 /// Legacy `Pragma` companion to `no-store` (RFC 6749 §5.1).
 pub const NO_CACHE_VALUE: &str = "no-cache";
 pub const NO_CACHE_VALUE: &str = "no-cache";
+/// Positive `Cache-Control` for OAuth metadata discovery.
+///
+/// Metadata is public and changes only when the scope catalog or endpoints do,
+/// so caching it is intentional and safe. The explicit directive replaces
+/// browser heuristic freshness with a tunable, reviewable policy.
+pub const METADATA_CACHE_VALUE: &str = "public, max-age=3600";
 
 
 /// Whether `path` serves a server-rendered page (strict script policy).
 /// Whether `path` serves a server-rendered page (strict script policy).
 ///
 ///
@@ -165,6 +171,12 @@ fn insert_headers(headers: &mut axum::http::HeaderMap, page: bool, hsts: bool, c
     );
     );
     headers.insert(header::PRAGMA, HeaderValue::from_static(NO_CACHE_VALUE));
     headers.insert(header::PRAGMA, HeaderValue::from_static(NO_CACHE_VALUE));
   }
   }
+  if cacheable {
+    headers.insert(
+      header::CACHE_CONTROL,
+      HeaderValue::from_static(METADATA_CACHE_VALUE),
+    );
+  }
   if hsts {
   if hsts {
     headers.insert(
     headers.insert(
       header::STRICT_TRANSPORT_SECURITY,
       header::STRICT_TRANSPORT_SECURITY,
@@ -267,7 +279,8 @@ mod tests {
   use axum::http::header;
   use axum::http::header;
 
 
   use super::{
   use super::{
-    HSTS_VALUE, NO_CACHE_VALUE, NO_STORE_VALUE, insert_headers, is_cacheable_path, is_page_path,
+    HSTS_VALUE, METADATA_CACHE_VALUE, NO_CACHE_VALUE, NO_STORE_VALUE, insert_headers,
+    is_cacheable_path, is_page_path,
   };
   };
 
 
   #[test]
   #[test]
@@ -305,10 +318,15 @@ mod tests {
   }
   }
 
 
   #[test]
   #[test]
-  fn cacheable_metadata_omits_storage_directives() {
+  fn cacheable_metadata_sets_positive_cache_control() {
     let mut headers = axum::http::HeaderMap::new();
     let mut headers = axum::http::HeaderMap::new();
     insert_headers(&mut headers, false, false, true);
     insert_headers(&mut headers, false, false, true);
-    assert!(!headers.contains_key(header::CACHE_CONTROL));
+    let cache_control = headers
+      .get(header::CACHE_CONTROL)
+      .and_then(|value| value.to_str().ok())
+      .unwrap_or_default();
+    assert_eq!(cache_control, METADATA_CACHE_VALUE);
+    assert!(!cache_control.contains("no-store"));
     assert!(!headers.contains_key(header::PRAGMA));
     assert!(!headers.contains_key(header::PRAGMA));
     assert!(!headers.contains_key(header::STRICT_TRANSPORT_SECURITY));
     assert!(!headers.contains_key(header::STRICT_TRANSPORT_SECURITY));
     for name in [
     for name in [

+ 25 - 0
src/routes.rs

@@ -623,6 +623,31 @@ mod router_tests {
     Ok(())
     Ok(())
   }
   }
 
 
+  /// Metadata discovery is cacheable by design: an explicit positive
+  /// `Cache-Control`, not browser heuristic freshness.
+  #[tokio::test]
+  async fn router_metadata_carries_explicit_positive_cache_control() -> TestResult {
+    let (_, headers, _) = drive(
+      build_router(passthrough_state()?),
+      "GET",
+      "/.well-known/oauth-authorization-server",
+      &[],
+      "",
+    )
+    .await?;
+    let value = headers
+      .get(header::CACHE_CONTROL)
+      .and_then(|value| value.to_str().ok())
+      .unwrap_or_default();
+    if !value.starts_with("public") || !value.contains("max-age=") {
+      return Err(format!("metadata must carry a positive public max-age, got {value:?}").into());
+    }
+    if value.contains("no-store") {
+      return Err(format!("metadata must stay cacheable, got {value:?}").into());
+    }
+    Ok(())
+  }
+
   /// HSTS is emitted only when the TLS option is on (never plaintext).
   /// HSTS is emitted only when the TLS option is on (never plaintext).
   #[tokio::test]
   #[tokio::test]
   async fn router_hsts_disabled_when_plaintext() -> TestResult {
   async fn router_hsts_disabled_when_plaintext() -> TestResult {

+ 62 - 0
src/state/health.rs

@@ -10,9 +10,16 @@
 
 
 use std::collections::HashSet;
 use std::collections::HashSet;
 
 
+use std::future::Future;
+use std::pin::Pin;
+use std::task::{Context, Poll};
+
+use axum::body::Body;
+use axum::http::{HeaderValue, Request, Response, header};
 use axum::{Json, extract::State, http::StatusCode, response::IntoResponse, routing::get};
 use axum::{Json, extract::State, http::StatusCode, response::IntoResponse, routing::get};
 use serde::Serialize;
 use serde::Serialize;
 use tokio::time::timeout;
 use tokio::time::timeout;
+use tower::{Layer, Service};
 
 
 use super::{AppState, MIGRATOR, READINESS_TIMEOUT};
 use super::{AppState, MIGRATOR, READINESS_TIMEOUT};
 
 
@@ -85,9 +92,64 @@ async fn ready(State(state): State<AppState>) -> impl IntoResponse {
 }
 }
 
 
 /// The dedicated health-only router (`/health/live` + `/health/ready` ONLY).
 /// The dedicated health-only router (`/health/live` + `/health/ready` ONLY).
+///
+/// The plaintext health listener is built without the public router's
+/// [`crate::middleware::security_headers::SecurityHeadersLayer`], so each probe
+/// response owns its hardening here: `Cache-Control: no-store` and
+/// `X-Content-Type-Options: nosniff` are inserted directly rather than relying
+/// on an outer layer that this listener never mounts.
 pub fn health_router(state: AppState) -> axum::Router {
 pub fn health_router(state: AppState) -> axum::Router {
   axum::Router::new()
   axum::Router::new()
     .route("/health/live", get(live))
     .route("/health/live", get(live))
     .route("/health/ready", get(ready))
     .route("/health/ready", get(ready))
+    .layer(HealthHeadersLayer)
     .with_state(state)
     .with_state(state)
 }
 }
+
+/// Tower layer inserting the probe header set on every health response.
+#[derive(Debug, Clone, Copy)]
+struct HealthHeadersLayer;
+
+impl<S> Layer<S> for HealthHeadersLayer {
+  type Service = HealthHeadersService<S>;
+
+  fn layer(&self, inner: S) -> Self::Service {
+    HealthHeadersService { inner }
+  }
+}
+
+/// Service produced by [`HealthHeadersLayer`].
+#[derive(Debug, Clone, Copy)]
+struct HealthHeadersService<S> {
+  inner: S,
+}
+
+impl<S, B> Service<Request<B>> for HealthHeadersService<S>
+where
+  S: Service<Request<B>, Response = Response<Body>> + Clone + Send + 'static,
+  S::Future: Send + 'static,
+  S::Error: Send + 'static,
+  B: Send + 'static,
+{
+  type Response = Response<Body>;
+  type Error = S::Error;
+  type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;
+
+  fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
+    self.inner.poll_ready(cx)
+  }
+
+  fn call(&mut self, request: Request<B>) -> Self::Future {
+    let mut inner = self.inner.clone();
+    Box::pin(async move {
+      let mut response = inner.call(request).await?;
+      let headers = response.headers_mut();
+      headers.insert(header::CACHE_CONTROL, HeaderValue::from_static("no-store"));
+      headers.insert(
+        header::X_CONTENT_TYPE_OPTIONS,
+        HeaderValue::from_static("nosniff"),
+      );
+      Ok(response)
+    })
+  }
+}

+ 18 - 0
src/state/test_support.rs

@@ -126,6 +126,24 @@ pub async fn get_health(state: &AppState, path: &str) -> Result<(u16, Value), Te
   Ok((status, body))
   Ok((status, body))
 }
 }
 
 
+/// Drive `GET <path>` at the health-only router and return its headers.
+///
+/// The health listener is built without the public router's security-header
+/// layer, so this probe verifies the header set the health router inserts on
+/// its own responses (`Cache-Control`, `X-Content-Type-Options`).
+pub async fn get_health_headers(
+  state: &AppState,
+  path: &str,
+) -> Result<axum::http::HeaderMap, TestError> {
+  use tower::ServiceExt as _;
+  let router = super::health_router(state.clone());
+  let request = axum::http::Request::builder()
+    .uri(path)
+    .body(axum::body::Body::empty())?;
+  let response = router.oneshot(request).await?;
+  Ok(response.headers().clone())
+}
+
 /// Freeze (`pause`) or resume (`unpause`) a container via the Docker CLI.
 /// Freeze (`pause`) or resume (`unpause`) a container via the Docker CLI.
 pub async fn docker_pause(id: &str, pause: bool) -> Result<(), TestError> {
 pub async fn docker_pause(id: &str, pause: bool) -> Result<(), TestError> {
   let action = if pause { "pause" } else { "unpause" };
   let action = if pause { "pause" } else { "unpause" };

+ 23 - 2
src/state/tests_topology.rs

@@ -12,8 +12,8 @@ use testcontainers::{GenericImage, ImageExt, core::WaitFor, runners::AsyncRunner
 use super::{
 use super::{
   AppState,
   AppState,
   test_support::{
   test_support::{
-    TestError, TestResult, get_health, mark_migrated, postgres_container, redis_single_container,
-    single_redis_block, test_config,
+    TestError, TestResult, get_health, get_health_headers, mark_migrated, postgres_container,
+    redis_single_container, single_redis_block, test_config,
   },
   },
 };
 };
 use crate::test_guard::ContainerGuard;
 use crate::test_guard::ContainerGuard;
@@ -55,6 +55,27 @@ async fn health_ready_single_reports_ready() -> TestResult {
     ready_body.get("status").and_then(|v| v.as_str()),
     ready_body.get("status").and_then(|v| v.as_str()),
     Some("ready")
     Some("ready")
   );
   );
+
+  // The health listener does not mount the public router's security-header
+  // layer, so the health router owns its own `no-store` hardening. Pin it here
+  // so a future route added to this listener cannot silently drop the policy.
+  for path in ["/health/live", "/health/ready"] {
+    let headers = get_health_headers(&state, path).await?;
+    assert_eq!(
+      headers
+        .get(axum::http::header::CACHE_CONTROL)
+        .and_then(|value| value.to_str().ok()),
+      Some("no-store"),
+      "{path} must forbid caching"
+    );
+    assert_eq!(
+      headers
+        .get(axum::http::header::X_CONTENT_TYPE_OPTIONS)
+        .and_then(|value| value.to_str().ok()),
+      Some("nosniff"),
+      "{path} must forbid MIME sniffing"
+    );
+  }
   Ok(())
   Ok(())
 }
 }