From 846aa95c3278c09c2c39987cd166f78f98e87298 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sat, 11 Jul 2026 13:18:27 +0800 Subject: [PATCH] test(security): GHSA-named regression tests for 3p3x and r5qv (backlog#1151 sec-6) (#4707) test(security): add GHSA-named regression tests for 3p3x and r5qv (backlog#1151 sec-6) Anchor the two fixed advisories to discoverable, named regression tests so `rg -i "ghsa|3p3x|r5qv"` finds a guard for each, and future fixes are forced to update pinned behavior (red -> green). GHSA-3p3x-734c-h5vx (constant-time WebDAV/FTPS secret comparison, rustfs#4403): - ftps_core.rs: new `assert_ftps_ghsa_3p3x_wrong_credentials_rejected` drives the `ct_eq` reject branch in FtpsAuthenticator::authenticate; asserts wrong password and unknown user are both rejected (530) and indistinguishable. - webdav_core.rs: the auth-failure block now sends a valid access key with a wrong secret (exercising the `ct_eq` branch, not just the unknown-access-key path) plus an unknown user, asserting both 401 and indistinguishable. - Module doc comments map advisory -> tests -> fix PR on both files. GHSA-r5qv-rc46-hv8q (internode RPC fail-closed, rustfs#4402): - http_auth.rs: renamed the default-fallback rejection test to `ghsa_r5qv_resolve_shared_secret_rejects_default_fallback` (and broadened it to cover default env secret + blank secrets), and added `ghsa_r5qv_verify_rpc_signature_fails_closed_on_missing_or_invalid_auth` pinning the exact advisory scenario (missing/forged/cross-URL signature is rejected; a correctly signed request still passes). File-level doc maps the advisory. Docs: new docs/testing/security-regressions.md with the advisory -> test mapping table and where each layer runs; linked from docs/testing/README.md. sec-14 will formalize the written policy in AGENTS.md. The unit-level ghsa_r5qv_* tests run in the default CI pass. The WebDAV/FTPS e2e live in the protocols suite (fixed ports, --test-threads=1); they cannot join the e2e-smoke profile and are wired into CI by sec-5. Refs: rustfs/backlog#1151 (sec-6), rustfs/backlog#1155 --- crates/e2e_test/src/protocols/ftps_core.rs | 115 ++++++++++++++++--- crates/e2e_test/src/protocols/webdav_core.rs | 52 ++++++++- crates/ecstore/src/cluster/rpc/http_auth.rs | 72 +++++++++++- docs/testing/README.md | 7 ++ docs/testing/security-regressions.md | 48 ++++++++ 5 files changed, 270 insertions(+), 24 deletions(-) create mode 100644 docs/testing/security-regressions.md diff --git a/crates/e2e_test/src/protocols/ftps_core.rs b/crates/e2e_test/src/protocols/ftps_core.rs index 1a03c714a..aedb1fc12 100644 --- a/crates/e2e_test/src/protocols/ftps_core.rs +++ b/crates/e2e_test/src/protocols/ftps_core.rs @@ -13,6 +13,23 @@ // limitations under the License. //! Core FTPS tests +//! +//! # Security regression coverage +//! +//! GHSA-3p3x-734c-h5vx (constant-time secret comparison on the WebDAV/FTPS +//! password login path, fixed in rustfs/rustfs#4403) is anchored end-to-end +//! by [`assert_ftps_ghsa_3p3x_wrong_credentials_rejected`], invoked from +//! [`test_ftps_core_operations`]. It exercises the `ct_eq` rejection branch in +//! `FtpsAuthenticator::authenticate` (`crates/protocols/src/ftps/server.rs`), +//! proving that a wrong password and an unknown user are both rejected and are +//! indistinguishable at the protocol layer (both return FTP `530 Not logged +//! in`). The sibling WebDAV assertion lives in `webdav_core.rs`; the internode +//! RPC fail-closed advisory (GHSA-r5qv-rc46-hv8q, rustfs/rustfs#4402) is +//! anchored by the `ghsa_r5qv_*` unit tests in +//! `crates/ecstore/src/cluster/rpc/http_auth.rs`. See +//! `docs/testing/security-regressions.md` for the full advisory -> test map. +//! +//! Advisory: use crate::common::rustfs_binary_path_with_features; use crate::protocols::test_env::{DEFAULT_ACCESS_KEY, DEFAULT_SECRET_KEY, ProtocolTestEnvironment}; @@ -24,8 +41,8 @@ use rustls::{ClientConfig, DigitallySignedStruct, Error as RustlsError, Signatur use std::io::Cursor; use std::path::PathBuf; use std::sync::Arc; -use suppaftp::RustlsConnector; -use suppaftp::RustlsFtpStream; +use suppaftp::types::Response; +use suppaftp::{FtpError, RustlsConnector, RustlsFtpStream, Status}; use tokio::process::Command; use tracing::info; @@ -73,6 +90,78 @@ impl ServerCertVerifier for AcceptAnyServerCertVerifier { } } +/// Open a fresh, unauthenticated FTPS control connection to the test server +/// and upgrade it to TLS. The caller is responsible for logging in. +/// +/// Assumes the process-wide rustls crypto provider has already been installed +/// (done once at the top of [`test_ftps_core_operations`]). +fn ftps_connect_secure() -> Result { + let config = ClientConfig::builder() + .dangerous() + .with_custom_certificate_verifier(Arc::new(AcceptAnyServerCertVerifier)) + .with_no_client_auth(); + + let tls_connector = RustlsConnector::from(Arc::new(config)); + + let ftp_stream = RustlsFtpStream::connect(FTPS_ADDRESS).map_err(|e| anyhow::anyhow!("Failed to connect: {}", e))?; + + ftp_stream + .into_secure(tls_connector, "127.0.0.1") + .map_err(|e| anyhow::anyhow!("Failed to upgrade to TLS: {}", e)) +} + +/// Extract the FTP reply status from a failed login, asserting the failure is a +/// protocol-level rejection (`FtpError::UnexpectedResponse`) rather than a +/// transport error. +fn login_rejection_status(user: &str, password: &str) -> Result { + let mut ftp_stream = ftps_connect_secure()?; + match ftp_stream.login(user, password) { + Ok(()) => anyhow::bail!("login unexpectedly succeeded for user '{user}'"), + Err(FtpError::UnexpectedResponse(Response { status, .. })) => Ok(status), + Err(other) => anyhow::bail!("expected a protocol rejection, got transport error: {other}"), + } +} + +/// Security regression for GHSA-3p3x-734c-h5vx. +/// +/// FTPS password verification must reject invalid credentials via the +/// constant-time `ct_eq` branch in `FtpsAuthenticator::authenticate` +/// (`crates/protocols/src/ftps/server.rs`, fixed in rustfs/rustfs#4403). This +/// asserts, purely on behavior (no timing assertions): +/// +/// 1. A valid user with a wrong password is rejected (`530`), exercising the +/// secret-mismatch branch that the advisory hardened. +/// 2. An unknown user is rejected (`530`). +/// 3. The two failures are indistinguishable at the protocol layer — both +/// return the same FTP status, so an attacker cannot use the reply code to +/// tell "user exists, wrong password" from "no such user". +async fn assert_ftps_ghsa_3p3x_wrong_credentials_rejected() -> Result<()> { + info!("Testing FTPS (GHSA-3p3x): wrong password is rejected"); + let wrong_password_status = login_rejection_status(DEFAULT_ACCESS_KEY, "definitely-not-the-secret")?; + assert_eq!( + wrong_password_status, + Status::NotLoggedIn, + "valid user + wrong password must be rejected with 530, got {wrong_password_status:?}" + ); + info!("PASS: FTPS wrong password rejected with 530"); + + info!("Testing FTPS (GHSA-3p3x): unknown user is rejected"); + let unknown_user_status = login_rejection_status("no-such-access-key", DEFAULT_SECRET_KEY)?; + assert_eq!( + unknown_user_status, + Status::NotLoggedIn, + "unknown user must be rejected with 530, got {unknown_user_status:?}" + ); + info!("PASS: FTPS unknown user rejected with 530"); + + assert_eq!( + wrong_password_status, unknown_user_status, + "invalid-user and invalid-password failures must be indistinguishable at the protocol layer" + ); + info!("PASS: FTPS invalid-user and invalid-password failures are indistinguishable"); + Ok(()) +} + /// Test FTPS: put, ls, mkdir, rmdir, delete operations pub async fn test_ftps_core_operations() -> Result<()> { let env = ProtocolTestEnvironment::new().map_err(|e| anyhow::anyhow!("{}", e))?; @@ -116,26 +205,18 @@ pub async fn test_ftps_core_operations() -> Result<()> { .await .map_err(|e| anyhow::anyhow!("{}", e))?; - // Build ServerConfig with SNI support + // Install the default crypto provider once for this process before any + // TLS handshake. Subsequent connections reuse it via `ftps_connect_secure`. rustls::crypto::aws_lc_rs::default_provider() .install_default() .map_err(|e| anyhow::anyhow!("Failed to install crypto provider: {:?}", e))?; - let config = ClientConfig::builder() - .dangerous() - .with_custom_certificate_verifier(Arc::new(AcceptAnyServerCertVerifier)) - .with_no_client_auth(); + // Security regression (GHSA-3p3x-734c-h5vx): wrong credentials must be + // rejected before any successful login is attempted below. + assert_ftps_ghsa_3p3x_wrong_credentials_rejected().await?; - // Wrap in suppaftp's RustlsConnector - let tls_connector = RustlsConnector::from(Arc::new(config)); - - // Connect to FTPS server - let ftp_stream = RustlsFtpStream::connect(FTPS_ADDRESS).map_err(|e| anyhow::anyhow!("Failed to connect: {}", e))?; - - // Upgrade to secure connection - let mut ftp_stream = ftp_stream - .into_secure(tls_connector, "127.0.0.1") - .map_err(|e| anyhow::anyhow!("Failed to upgrade to TLS: {}", e))?; + // Connect and log in with valid credentials for the functional flow. + let mut ftp_stream = ftps_connect_secure()?; ftp_stream.login(DEFAULT_ACCESS_KEY, DEFAULT_SECRET_KEY)?; info!("Testing FTPS: mkdir bucket"); diff --git a/crates/e2e_test/src/protocols/webdav_core.rs b/crates/e2e_test/src/protocols/webdav_core.rs index 99bc9c187..761f9ffe4 100644 --- a/crates/e2e_test/src/protocols/webdav_core.rs +++ b/crates/e2e_test/src/protocols/webdav_core.rs @@ -13,6 +13,23 @@ // limitations under the License. //! Core WebDAV tests +//! +//! # Security regression coverage +//! +//! GHSA-3p3x-734c-h5vx (constant-time secret comparison on the WebDAV/FTPS +//! password login path, fixed in rustfs/rustfs#4403) is anchored end-to-end by +//! the authentication-failure block in [`test_webdav_core_operations`], marked +//! with a `GHSA-3p3x` comment. It drives the `ct_eq` rejection branch in +//! `WebDavAuth::authenticate` (`crates/protocols/src/webdav/server.rs`): a +//! valid access key with a wrong secret is rejected (the exact branch the +//! advisory hardened), and an unknown user is rejected with the same status so +//! the two failures are indistinguishable. The sibling FTPS assertion lives in +//! `ftps_core.rs`; the internode RPC fail-closed advisory +//! (GHSA-r5qv-rc46-hv8q, rustfs/rustfs#4402) is anchored by the `ghsa_r5qv_*` +//! unit tests in `crates/ecstore/src/cluster/rpc/http_auth.rs`. See +//! `docs/testing/security-regressions.md` for the full advisory -> test map. +//! +//! Advisory: use crate::common::local_http_client; use crate::common::rustfs_binary_path_with_features; @@ -653,15 +670,38 @@ pub async fn test_webdav_core_operations() -> Result<()> { ); info!("PASS: DELETE bucket '{}' successful", bucket_name); - // Test authentication failure - info!("Testing WebDAV: Authentication failure"); - let resp = client + // Security regression (GHSA-3p3x-734c-h5vx): constant-time secret + // comparison on the WebDAV password login path (fixed in + // rustfs/rustfs#4403). A valid access key with a wrong secret must be + // rejected via the `ct_eq` branch in `WebDavAuth::authenticate`, and an + // unknown user must be rejected with the same status so the two are + // indistinguishable. Behavior-only assertion (no timing checks). + info!("Testing WebDAV (GHSA-3p3x): valid access key + wrong secret is rejected"); + let wrong_secret_resp = client .request(reqwest::Method::from_bytes(b"PROPFIND").unwrap(), &base_url) - .header("Authorization", "Basic aW52YWxpZDppbnZhbGlk") // invalid:invalid + .header("Authorization", basic_auth_header_for(DEFAULT_ACCESS_KEY, "definitely-not-the-secret")) .send() .await?; - assert_eq!(resp.status().as_u16(), 401, "Invalid auth should return 401, got: {}", resp.status()); - info!("PASS: Authentication failure test successful"); + let wrong_secret_status = wrong_secret_resp.status().as_u16(); + assert_eq!( + wrong_secret_status, 401, + "valid access key + wrong secret should return 401, got: {wrong_secret_status}" + ); + info!("PASS: WebDAV wrong secret rejected with 401"); + + info!("Testing WebDAV (GHSA-3p3x): unknown user is rejected"); + let unknown_user_resp = client + .request(reqwest::Method::from_bytes(b"PROPFIND").unwrap(), &base_url) + .header("Authorization", basic_auth_header_for("no-such-access-key", "definitely-not-the-secret")) + .send() + .await?; + let unknown_user_status = unknown_user_resp.status().as_u16(); + assert_eq!(unknown_user_status, 401, "unknown user should return 401, got: {unknown_user_status}"); + assert_eq!( + wrong_secret_status, unknown_user_status, + "invalid-user and invalid-secret failures must be indistinguishable at the protocol layer" + ); + info!("PASS: WebDAV authentication failure (GHSA-3p3x) test successful"); info!("WebDAV core tests passed"); Ok(()) diff --git a/crates/ecstore/src/cluster/rpc/http_auth.rs b/crates/ecstore/src/cluster/rpc/http_auth.rs index 28885e9c5..fe2207fd6 100644 --- a/crates/ecstore/src/cluster/rpc/http_auth.rs +++ b/crates/ecstore/src/cluster/rpc/http_auth.rs @@ -12,6 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. +//! Internode RPC HMAC authentication. +//! +//! # Security regression coverage +//! +//! GHSA-r5qv-rc46-hv8q (internode RPC authentication must fail closed, fixed in +//! rustfs/rustfs#4402) is anchored by the `ghsa_r5qv_*` tests in the module +//! below, plus the broader negative-signature suite. The advisory class is: a +//! node must never accept an RPC whose auth is missing, malformed, replayed, or +//! signed with the default/empty shared secret. See +//! `docs/testing/security-regressions.md` for the full advisory -> test map. +//! +//! Advisory: + use crate::cluster::rpc::context_propagation::{inject_request_id_into_http_headers, inject_trace_context_into_http_headers}; use base64::Engine as _; use base64::engine::general_purpose; @@ -227,13 +240,70 @@ mod tests { runtime_sources::ensure_test_rpc_secret(); } + /// Security regression for GHSA-r5qv-rc46-hv8q (internode RPC fail-closed, + /// fixed in rustfs/rustfs#4402): secret resolution must never silently fall + /// back to a default/empty shared secret. Missing and default secrets both + /// resolve to an error, so a misconfigured node cannot come up with a + /// predictable, attacker-known RPC key. #[test] - fn test_resolve_shared_secret_rejects_default_fallback() { + fn ghsa_r5qv_resolve_shared_secret_rejects_default_fallback() { let err = resolve_shared_secret(None, None).expect_err("default fallback must be rejected"); assert_eq!(err.to_string(), RPC_SECRET_REQUIRED_MESSAGE); let err = resolve_shared_secret(None, Some(DEFAULT_SECRET_KEY)).expect_err("default global secret must be rejected"); assert_eq!(err.to_string(), RPC_SECRET_REQUIRED_MESSAGE); + + let err = resolve_shared_secret(Some(DEFAULT_SECRET_KEY), None).expect_err("default env secret must be rejected"); + assert_eq!(err.to_string(), RPC_SECRET_REQUIRED_MESSAGE); + + let err = resolve_shared_secret(Some(" "), Some(" ")).expect_err("blank secrets must be rejected"); + assert_eq!(err.to_string(), RPC_SECRET_REQUIRED_MESSAGE); + } + + /// Security regression for GHSA-r5qv-rc46-hv8q: `verify_rpc_signature` must + /// fail closed for every shape of missing or malformed authentication. + /// Consolidates the advisory's exact scenario (no valid signature/timestamp + /// pair => rejected, never silently allowed) into one named test so the + /// advisory maps to a discoverable regression. + #[test] + fn ghsa_r5qv_verify_rpc_signature_fails_closed_on_missing_or_invalid_auth() { + ensure_test_rpc_secret(); + let url = "http://example.com/api/test"; + let method = Method::GET; + + // No auth headers at all. + let empty = HeaderMap::new(); + assert!( + verify_rpc_signature(url, &method, &empty).is_err(), + "request with no auth headers must be rejected" + ); + + // Signature header present but garbage; timestamp is current. + let mut forged = HeaderMap::new(); + let now = OffsetDateTime::now_utc().unix_timestamp(); + forged.insert(SIGNATURE_HEADER, HeaderValue::from_static("not-a-real-signature")); + forged.insert(TIMESTAMP_HEADER, HeaderValue::from_str(&now.to_string()).unwrap()); + assert!( + verify_rpc_signature(url, &method, &forged).is_err(), + "request with a forged signature must be rejected" + ); + + // A validly signed request for a *different* URL must not authorize this one. + let mut cross = HeaderMap::new(); + build_auth_headers("http://example.com/api/other", &method, &mut cross).expect("auth headers should build"); + assert!( + verify_rpc_signature(url, &method, &cross).is_err(), + "a signature bound to a different URL must not authorize this request" + ); + + // Control: a correctly signed request for this URL still succeeds, so the + // gate is fail-closed rather than fail-everything. + let mut valid = HeaderMap::new(); + build_auth_headers(url, &method, &mut valid).expect("auth headers should build"); + assert!( + verify_rpc_signature(url, &method, &valid).is_ok(), + "a correctly signed request must be accepted" + ); } #[test] diff --git a/docs/testing/README.md b/docs/testing/README.md index e4ad04829..05a335477 100644 --- a/docs/testing/README.md +++ b/docs/testing/README.md @@ -11,6 +11,13 @@ _TODO (infra-11): unit / e2e-smoke / e2e-full / e2e-nightly / protocols / s3-tests / fuzz / perf, with naming conventions._ +### Security advisory regression tests + +Fixed GHSA advisories map to named, discoverable regression tests. The +advisory -> test map lives in +[`docs/testing/security-regressions.md`](security-regressions.md). sec-14 +(backlog#1151) formalizes the written admission policy in `AGENTS.md`. + ## Serial groups & CI profiles _TODO (infra-11): document the `[test-groups]` mechanism and the `default` vs diff --git a/docs/testing/security-regressions.md b/docs/testing/security-regressions.md new file mode 100644 index 000000000..49dd1efc8 --- /dev/null +++ b/docs/testing/security-regressions.md @@ -0,0 +1,48 @@ +# Security Advisory Regression Tests + +Every fixed RustFS GitHub Security Advisory (GHSA) should map to at least one +named, discoverable regression test. The convention is: name the test (or a +helper / doc comment on the exact assertion) after the advisory so that + +```bash +rg -i "ghsa|3p3x|r5qv" +``` + +finds the guard for any advisory, and a future fix of a still-open advisory is +forced to update its pinned test (red -> green). + +> This is the lightweight inventory. sec-14 (backlog#1151) formalizes the +> written admission policy in `AGENTS.md`; keep this file as the map. + +## Advisory -> test mapping + +| Advisory | Class | Fix PR | Named regression tests | Layer | +| --- | --- | --- | --- | --- | +| [GHSA-3p3x-734c-h5vx](https://github.com/rustfs/rustfs/security/advisories/GHSA-3p3x-734c-h5vx) | Constant-time secret comparison on WebDAV/FTPS password login | rustfs/rustfs#4403 | `assert_ftps_ghsa_3p3x_wrong_credentials_rejected` (`crates/e2e_test/src/protocols/ftps_core.rs`); `GHSA-3p3x` auth-failure block in `test_webdav_core_operations` (`crates/e2e_test/src/protocols/webdav_core.rs`) | e2e (protocols suite) | +| [GHSA-r5qv-rc46-hv8q](https://github.com/rustfs/rustfs/security/advisories/GHSA-r5qv-rc46-hv8q) | Internode RPC authentication must fail closed | rustfs/rustfs#4402 | `ghsa_r5qv_resolve_shared_secret_rejects_default_fallback`, `ghsa_r5qv_verify_rpc_signature_fails_closed_on_missing_or_invalid_auth` (`crates/ecstore/src/cluster/rpc/http_auth.rs`) | unit | +| [GHSA-m77q-r63m-pj89](https://github.com/rustfs/rustfs/security/advisories/GHSA-m77q-r63m-pj89) | STS JWTs signed with shared root secret (intentionally unfixed) | n/a | `test_created_sts_credentials_authorize_with_session_token_claims` (`crates/iam/src/sys.rs`) — pins current behavior; sec-7 adds GHSA naming | unit | + +## Where these run + +- **Unit tests** (`ghsa_r5qv_*` in `crates/ecstore`) run automatically in the + default CI pass via `cargo nextest run` / `cargo test` — no special wiring. +- **Protocol e2e tests** (WebDAV/FTPS constant-time login rejection) live in the + `e2e_test` protocols suite, which binds **fixed ports** and requires + `--test-threads=1`. Because of the fixed ports they **cannot** join the + `e2e-smoke` nextest profile (parallel, dynamic ports). They run manually today: + + ```bash + RUSTFS_BUILD_FEATURES=ftps,webdav cargo test --package e2e_test \ + test_protocol_core_suite -- --test-threads=1 --nocapture + ``` + + Wiring the security e2e lane into CI is tracked separately (sec-5, backlog#1151). + +## Adding a new advisory guard + +1. Reproduce the advisory's bypass form as a focused negative test. +2. Name the test (or the helper/assertion) `ghsa__*`, or attach a + `GHSA-` doc comment with the advisory URL and fix PR. +3. Add a row to the table above. +4. Land the unit-level guard where the default CI pass runs it; land any e2e + guard in the protocols suite (and register it with sec-5's filterset seam).