diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 65daf93..d94ffbf 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -110,14 +110,19 @@ jobs: ASSET: ${{ steps.meta.outputs.asset }} DIST: ${{ runner.temp }}/dist run: | + # `--bundle` writes signature, certificate, and transparency-log proof + # in one file. The separate `--output-signature`/`--output-certificate` + # flags are deprecated and ignored under the current bundle format, + # which is what made this step fail with an empty bundle path. + # The `.sigstore.json` suffix is what OpenSSF Scorecard's + # Signed-Releases check recognises. cosign sign-blob --yes \ - --output-signature "$DIST/$ASSET.sig" \ - --output-certificate "$DIST/$ASSET.pem" \ + --bundle "$DIST/$ASSET.sigstore.json" \ "$DIST/$ASSET" # Real SLSA provenance, but API-based rather than a release asset, so - # scorecard's file-suffix probe does not see it; the `.sig` uploaded - # alongside is what carries the Signed-Releases score. + # scorecard's file-suffix probe does not see it; the `.sigstore.json` + # bundle uploaded alongside is what carries the Signed-Releases score. - name: Attest SLSA build provenance uses: actions/attest-build-provenance@4d101475d8b20a2381f78447822ac1eab6504dd8 # v4.2.2 with: @@ -129,8 +134,7 @@ jobs: name: ${{ matrix.target }}-${{ matrix.variant }} path: | ${{ runner.temp }}/dist/${{ steps.meta.outputs.asset }} - ${{ runner.temp }}/dist/${{ steps.meta.outputs.asset }}.sig - ${{ runner.temp }}/dist/${{ steps.meta.outputs.asset }}.pem + ${{ runner.temp }}/dist/${{ steps.meta.outputs.asset }}.sigstore.json if-no-files-found: error # ---- Publish Release ----------------------------------------------------- diff --git a/AGENTS.md b/AGENTS.md index 417c73e..8db476d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,7 +109,7 @@ Precedence is **`deny-overrides`** (XACML terminology), evaluated in a fixed ord Container creation, exec, lifecycle mutation, `/build`, `/commit`, and everything not listed above. ### Body inspection -For profiles that permit `/containers/create`, the request body is inspected and rejected if it sets `Privileged`, `CapAdd`, `SecurityOpt`, `Devices`, `PidMode`, `IpcMode`, or `UsernsMode`. Bind and volume mounts are permitted — orchestrators legitimately need them, and the profile is documented as trusted-caller-only. +For profiles that permit `/containers/create`, the request body is inspected and rejected if it sets `Privileged`, `CapAdd`, `SecurityOpt`, `Devices`, `DeviceRequests`, `PidMode`, `IpcMode`, or `UsernsMode` — at the top level or nested under `HostConfig` — or sets `NetworkMode` to `host`. Docker reads these fields from `HostConfig`, so both levels are inspected. Bind and volume mounts are permitted — orchestrators legitimately need them, and the profile is documented as trusted-caller-only. ### Profiles | Profile | Intent | @@ -135,7 +135,7 @@ Each blocked endpoint is mapped to its NIST SP 800-190 and CIS Docker Benchmark - Structured JSON logs via `tracing`, named per **OpenTelemetry semantic conventions** - **W3C Trace Context** — an inbound `traceparent` is propagated upstream -- `/metrics` in **OpenMetrics/Prometheus** text format: allow and deny counters by endpoint and profile, plus request latency +- `/metrics` in **OpenMetrics/Prometheus** text format: allow and deny counters - Health endpoint, reachable from a `--health-check` subcommand since the scratch image has no shell ## Configuration @@ -164,7 +164,7 @@ endpoints = ["/containers/create", "/exec"] methods = ["POST"] ``` -Merge semantics follow **RFC 7386** (JSON Merge Patch) rather than ad-hoc precedence. +Merge is a monotonic union-append over the profile defaults: `allow`/`include` add grants and `deny`/`exclude` carve them out under `deny-overrides`. A file or environment rule can only add to `allow` — use `deny`/`exclude` to subtract. ### Compatibility A shim accepts the Tecnativa/linuxserver `docker-socket-proxy` environment variables (`CONTAINERS=1`, `IMAGES=1`, `POST=0`, …) and translates them into endpoint patterns, making this a drop-in replacement for the incumbent. diff --git a/CHANGELOG.md b/CHANGELOG.md index 20eabf0..b612550 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **Container-create body inspection now checks the nested `HostConfig`.** The + Docker daemon reads `Privileged`, `CapAdd`, `SecurityOpt`, `Devices`, + `PidMode`, `IpcMode`, and `UsernsMode` from `HostConfig`, but the guard only + inspected the top level, so `{"HostConfig":{"Privileged":true}}` slipped + through and started a privileged container. Both levels are now inspected, + `DeviceRequests` and `NetworkMode: host` are refused too, and explicit + `false`/empty/`null` no longer over-block. +- **The `container-runtime` profile no longer cross-products methods × + endpoints.** Its read and write grants merged into one set, silently allowing + every write method on every readable endpoint — including + `DELETE /volumes/{id}` and `DELETE /networks/{id}`. Allow rules are now + independent, so each grant stays method-AND-endpoint. +- **Create-body inspection follows the effective policy, not the profile enum.** + `--profile none` plus an allowlist granting `POST /containers/create`, and the + `CONTAINERS=1 POST=1` compatibility shim, now inspect create bodies instead of + forwarding them unexamined. +- **Malformed request paths return 400**, not 403, matching the documented + pipeline contract. +- **Chunked over-limit bodies return 413**, not 502, when the streamed size + limit fires mid-forward. +- **`--log-level` is no longer shadowed by an ambient `RUST_LOG`.** +- **Upgraded (101) connections are drained on graceful shutdown**, so `docker + exec` sessions get a bounded window to finish instead of being severed. + +### Changed +- **Merge semantics documented as a monotonic union-append under + `deny-overrides`**, not RFC 7386 JSON Merge Patch (which was documented but + never implemented). +- **Release signing** writes a `.sigstore.json` Sigstore bundle via `cosign + sign-blob --bundle`, replacing the deprecated `--output-signature`/ + `--output-certificate` flags that made the step fail. + ## [0.3.0] — 2026-08-12 YAML allowlists, drop-in compatibility with the section variables other socket diff --git a/README.md b/README.md index d188b23..78e98d6 100644 --- a/README.md +++ b/README.md @@ -221,7 +221,7 @@ Use the opt-in `container-runtime` profile for Docker-backed orchestrators. It s DOCKER_PROXY_PROFILE=container-runtime docker-socket-proxy ``` -For profiles that permit `/containers/create`, the request body is inspected and rejected if it sets `Privileged`, `CapAdd`, `SecurityOpt`, `Devices`, `PidMode`, `IpcMode`, or `UsernsMode`. Bind and volume mounts are permitted — orchestrators need them, and this profile is trusted-caller-only by design. +For profiles that permit `/containers/create`, the request body is inspected and rejected if it sets `Privileged`, `CapAdd`, `SecurityOpt`, `Devices`, `DeviceRequests`, `PidMode`, `IpcMode`, or `UsernsMode` — at the top level or nested under `HostConfig` — or sets `NetworkMode` to `host`. Bind and volume mounts are permitted — orchestrators need them, and this profile is trusted-caller-only by design. ## Known Limitations diff --git a/STATUS.md b/STATUS.md index 001f390..c1bec51 100644 --- a/STATUS.md +++ b/STATUS.md @@ -15,8 +15,16 @@ The remaining work is standards conformance, not features. `container-runtime`, and `none` profiles; wildcard matcher; TOML or YAML `allow`/`deny`/ `include`/`exclude`; environment modifiers; API-version normalization; -create-body inspection. `deny` and `exclude` hold independent rules, so -separate sources cannot merge into one narrower condition. +create-body inspection. `allow`, `deny`, and `exclude` all hold independent +rules, so separate sources cannot merge into one narrower condition — or, on +the allow side, one wider cross-product than any single source wrote. + +Container-create bodies are inspected at both the top level and under +`HostConfig`, where the daemon actually reads `Privileged`, `CapAdd`, +`SecurityOpt`, `Devices`, `DeviceRequests`, `PidMode`, `IpcMode`, and +`UsernsMode`; `NetworkMode: host` is refused too. Inspection runs whenever the +effective policy permits `POST /containers/create`, not only under +`container-runtime`. **Compatibility** — the section variables Tecnativa's socket proxy uses (`CONTAINERS`, `POST`, `ALLOW_START`, …) configure the filter directly, checked @@ -61,7 +69,7 @@ Engine API surface (`src/docker_api.rs`) at startup; anything matching no real endpoint is warned about with its source, since a typo in `deny` or `exclude` is otherwise silent. Shipped patterns are checked by test, not warning. -**Tests** — 78 passing (66 unit, 4 integration against a mock socket, 5 +**Tests** — 87 passing (71 unit, 8 integration against a mock socket, 5 asserting the shipped examples still behave as documented, 3 checking policy patterns against the real Docker Engine API surface). diff --git a/docs/standards.md b/docs/standards.md index e9390ad..4863b1c 100644 --- a/docs/standards.md +++ b/docs/standards.md @@ -114,7 +114,7 @@ Code changes using crates already present, or the +2-crate `tower-http`. | **RFC 3986 §6** — URI normalization | Percent-decode, remove dot-segments, collapse duplicate slashes before policy matching | Free (hand-rolled) | | **XACML combining algorithms** | Name and document the precedence as `deny-overrides`; fixes `exclude` matching method-OR-endpoint while `deny` requires method-AND-endpoint | Free (logic) | | **Kubernetes RBAC** (as a model) | verbs × resources × resourceNames ≈ methods × patterns × wildcards | Free (design) | -| **RFC 7386** — JSON Merge Patch | Principled semantics for the `allow`/`deny`/`include`/`exclude` merge | Free (design) | +| **RFC 7386** — JSON Merge Patch | *Considered, not adopted*: the merge is a monotonic union-append (`allow`/`include` add, `deny`/`exclude` carve out under XACML `deny-overrides`); JSON Merge Patch null-deletes are not implemented | Free (design) | | **Tecnativa / linuxserver env convention** | `CONTAINERS=1`, `POST=0` compatibility shim → drop-in replacement | Free (logic) | | **OWASP API4** — resource consumption | `tower-http` `RequestBodyLimitLayer` + `TimeoutLayer` | +2 crates | | **OpenMetrics / Prometheus** | `/metrics` with allow/deny counters, hand-rolled text exposition over `AtomicU64` | Free | diff --git a/examples/create-inspection.toml b/examples/create-inspection.toml index 47c20b4..ac81b6d 100644 --- a/examples/create-inspection.toml +++ b/examples/create-inspection.toml @@ -1,17 +1,23 @@ # Container creation with the body inspected, not just the endpoint allowed. # -# Run with `--profile container-runtime`, which is what turns on inspection of -# `POST /containers/create`. Endpoint rules alone cannot express this: the +# Inspection triggers on `POST /containers/create` whenever that endpoint is +# permitted — under `--profile container-runtime` or an allowlist — because the # difference between a safe create and a container escape is in the body. # # Rejected regardless of what this file allows: -# {"Image":"x","Cmd":[],"Privileged":true} → escapes the container -# {"Image":"x","Cmd":[],"CapAdd":["SYS_ADMIN"]} → same, by capability -# {"Image":"x","Cmd":[],"PidMode":"host"} → host namespace -# {"Image":"x"} → no Cmd +# {"Image":"x","HostConfig":{"Privileged":true}} → escapes the container +# {"Image":"x","HostConfig":{"CapAdd":["SYS_ADMIN"]}} → same, by capability +# {"Image":"x","HostConfig":{"PidMode":"host"}} → host namespace +# {"Image":"x","HostConfig":{"DeviceRequests":[...]}} → GPU passthrough +# {"Image":"x","NetworkMode":"host"} → host network namespace +# +# The dangerous fields are rejected at the top level and nested under +# `HostConfig` alike, since that is where Docker reads them from. `false`, +# `[]`, `""`, and `null` are explicit unsets and pass. # # Accepted: # {"Image":"worker:1","Cmd":["run"]} +# {"Image":"worker:1"} → the image's default Cmd is legitimate # # Mounts are deliberately permitted; orchestrators require them, and this # profile is documented as trusted-caller-only. diff --git a/fuzz/fuzz_targets/path_normalizer.rs b/fuzz/fuzz_targets/path_normalizer.rs index 5ff40d4..c42e26b 100644 --- a/fuzz/fuzz_targets/path_normalizer.rs +++ b/fuzz/fuzz_targets/path_normalizer.rs @@ -39,7 +39,7 @@ fuzz_target!(|data: &[u8]| { let endpoints = Some(vec![pattern.to_string()]); filter .allow_mut() - .extend(methods.clone(), endpoints.clone()); + .push(methods.clone(), endpoints.clone()); filter.deny_mut().push(methods.clone(), endpoints.clone()); filter .exclude_mut() @@ -48,9 +48,10 @@ fuzz_target!(|data: &[u8]| { // The raw decision surface (no normalization). let _ = filter.check(&method, &path); - // `deny_all()` builds a `None` profile, so `check_head` can only yield - // `BodyRule::None` on success; the body then carries no weight and - // `check_request` must agree. This pins the normalization hand-off. + // Body inspection is keyed on whether the effective policy permits + // `POST /containers/create`, so `check_head` may also yield + // `BodyRule::ContainerCreate` when the injected allow rule opens it. Both + // outcomes must route through `check_request` without panicking. match filter.check_head(&method, &path) { Ok(BodyRule::None) => { assert!( diff --git a/fuzz/fuzz_targets/policy_parse.rs b/fuzz/fuzz_targets/policy_parse.rs index 060b39e..e5a4b0e 100644 --- a/fuzz/fuzz_targets/policy_parse.rs +++ b/fuzz/fuzz_targets/policy_parse.rs @@ -24,10 +24,10 @@ fuzz_target!(|data: &[u8]| { // Mirror PolicyLoader::apply_document, but through the public surface only. if let Some(set) = document.allow { - filter.allow_mut().extend(set.methods, set.endpoints); + filter.allow_mut().push(set.methods, set.endpoints); } if let Some(set) = document.include { - filter.allow_mut().extend(set.methods, set.endpoints); + filter.allow_mut().push(set.methods, set.endpoints); } if let Some(set) = document.deny { filter.deny_mut().push(set.methods, set.endpoints); diff --git a/src/docker_api.rs b/src/docker_api.rs index 57a3ece..96d6d04 100644 --- a/src/docker_api.rs +++ b/src/docker_api.rs @@ -131,7 +131,8 @@ pub const METHODS: &[&str] = &["GET", "HEAD", "POST", "PUT", "DELETE"]; /// Whether a policy pattern could ever match a real endpoint. /// /// Wildcards on either side match any single segment, since [`PATHS`] carries -/// `*` where Docker's specification names a parameter. +/// `*` where Docker's specification names a parameter and a policy may name a +/// concrete value there instead. pub fn matches_known_path(pattern: &str) -> bool { PATHS.iter().any(|path| { if pattern.ends_with('/') { @@ -143,7 +144,7 @@ pub fn matches_known_path(pattern: &str) -> bool { pattern_segments.clone().count() == path_segments.len() && pattern_segments .zip(&path_segments) - .all(|(a, b)| a == "*" || b == &a) + .all(|(a, b)| a == "*" || b == &a || *b == "*") }) } diff --git a/src/error.rs b/src/error.rs index 2fb658b..52caed8 100644 --- a/src/error.rs +++ b/src/error.rs @@ -30,6 +30,10 @@ pub enum ProxyError { #[error("access denied: {0}")] Forbidden(String), + /// Request could not be parsed, as opposed to refused by policy. + #[error("bad request: {0}")] + BadRequest(String), + /// Request body exceeded the configured limit. #[error("payload too large: {0}")] TooLarge(String), @@ -48,9 +52,10 @@ impl IntoResponse for ProxyError { let (status, message) = match &self { ProxyError::Config(msg) => (StatusCode::BAD_REQUEST, msg.clone()), ProxyError::Forbidden(msg) => (StatusCode::FORBIDDEN, msg.clone()), + ProxyError::BadRequest(msg) => (StatusCode::BAD_REQUEST, msg.clone()), ProxyError::TooLarge(msg) => (StatusCode::PAYLOAD_TOO_LARGE, msg.clone()), ProxyError::Docker(msg) => (StatusCode::BAD_GATEWAY, msg.clone()), - ProxyError::Internal(_) => (StatusCode::INTERNAL_SERVER_ERROR, self.to_string()), + ProxyError::Internal(msg) => (StatusCode::INTERNAL_SERVER_ERROR, msg.clone()), }; let body = json!({ "message": message }); @@ -96,4 +101,18 @@ mod tests { StatusCode::INTERNAL_SERVER_ERROR ); } + + #[tokio::test] + async fn bad_request_maps_to_400() { + assert_eq!( + body_of(ProxyError::BadRequest("garbage".into())).await.0, + StatusCode::BAD_REQUEST + ); + } + + #[tokio::test] + async fn internal_error_body_carries_only_the_message() { + let (_, body) = body_of(ProxyError::Internal("boom".into())).await; + assert_eq!(body["message"], "boom"); + } } diff --git a/src/main.rs b/src/main.rs index 2ef8418..6c7a640 100644 --- a/src/main.rs +++ b/src/main.rs @@ -43,8 +43,10 @@ fn main() { } fn init_logging(config: &docker_socket_proxy::config::Config) { - let env_filter = - EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new(&config.log_level)); + // `log_level` already reflects `RUST_LOG` via clap's `env` binding (with the + // CLI winning), so reading the environment again here would shadow an + // explicit `--log-level`. + let env_filter = EnvFilter::new(&config.log_level); match config.log_format { docker_socket_proxy::config::LogFormat::Json => { diff --git a/src/middleware.rs b/src/middleware.rs index 066d4d9..ed7d13d 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -103,6 +103,19 @@ where ); denial.into_response() }; + // A body-limit rejection is not a policy denial, but it is + // forensically meaningful and leaves the same structured trail. + let reject = |error: ProxyError| { + metrics.record_denied(); + warn!( + method = parts.method.as_str(), + path = parts.uri.path(), + profile = ?filter.profile(), + reason = %error, + "request rejected by the enforcement layer" + ); + error.into_response() + }; let rule = match filter.check_head(parts.method.as_str(), parts.uri.path()) { Ok(rule) => rule, @@ -111,14 +124,14 @@ where let body = match rule { BodyRule::None => match oversized_declaration(&parts.headers, max_body_bytes) { - Some(error) => return Ok(error.into_response()), + Some(error) => return Ok(reject(error)), None => Body::new(Limited::new(body, max_body_bytes)), }, rule => { let collected = match Limited::new(body, max_body_bytes).collect().await { Ok(collected) => collected.to_bytes(), Err(error) => { - return Ok(body_error(error, max_body_bytes).into_response()); + return Ok(reject(body_error(error, max_body_bytes))); } }; if let Err(denial) = SecurityFilter::check_body(rule, &collected) { diff --git a/src/policy.rs b/src/policy.rs index 83a90ee..3c769df 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -189,7 +189,7 @@ fn compatibility_filter( }; let mut filter = SecurityFilter::deny_all(); - filter.allow_mut().extend(Some(methods), Some(endpoints)); + filter.allow_mut().push(Some(methods), Some(endpoints)); Ok(Some(filter)) } @@ -237,7 +237,7 @@ fn apply_document(filter: &mut SecurityFilter, document: PolicyDocument) { for (name, set) in [("allow", document.allow), ("include", document.include)] { if let Some(set) = set { report_unknown(name, set.methods.as_deref(), set.endpoints.as_deref()); - filter.allow_mut().extend(set.methods, set.endpoints); + filter.allow_mut().push(set.methods, set.endpoints); } } if let Some(set) = document.deny { @@ -298,7 +298,7 @@ fn apply_environment(filter: &mut SecurityFilter, env: &HashMap) for prefix in ["ALLOW", "INCLUDE"] { let (methods, endpoints) = rule(prefix); - filter.allow_mut().extend(methods, endpoints); + filter.allow_mut().push(methods, endpoints); } let (methods, endpoints) = rule("DENY"); filter.deny_mut().push(methods, endpoints); @@ -347,6 +347,24 @@ methods = [] assert!(filter.check("GET", "/info").is_ok()); } + #[test] + fn a_file_allow_stays_independent_of_the_profile_allow() { + let filter = from_toml( + r#" +[allow] +methods = ["POST"] +endpoints = ["/images/create"] +"#, + &SecurityProfile::Default, + ); + + assert!(filter.check("POST", "/images/create").is_ok()); + assert!( + filter.check("POST", "/info").is_err(), + "the file's POST must not leak onto the profile's endpoints" + ); + } + #[test] fn include_adds_and_exclude_removes() { let filter = from_toml( diff --git a/src/proxy.rs b/src/proxy.rs index 8d15d80..1ccaf3a 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -60,6 +60,7 @@ const HOP_BY_HOP_HEADERS: [&str; 8] = [ #[derive(Clone)] pub struct AppState { docker_socket: PathBuf, + upgrade_tasks: Arc>>>, } /// Collect the header names listed in `Connection`. @@ -122,7 +123,9 @@ pub async fn serve(config: Config) -> Result<(), ProxyError> { let filter = PolicyLoader::new(config.allowlist.as_deref(), &config.profile).load()?; let state = AppState { docker_socket: config.socket.clone(), + upgrade_tasks: Arc::new(tokio::sync::Mutex::new(Vec::new())), }; + let upgrade_tasks = Arc::clone(&state.upgrade_tasks); let metrics = Arc::new(Metrics::default()); let router = build_router( @@ -147,7 +150,10 @@ pub async fn serve(config: Config) -> Result<(), ProxyError> { axum::serve(listener, router) .with_graceful_shutdown(shutdown_signal()) .await - .map_err(|e| ProxyError::Internal(e.to_string())) + .map_err(|e| ProxyError::Internal(e.to_string()))?; + + drain_upgrades(&upgrade_tasks).await; + Ok(()) } /// Create a router for integration testing. @@ -157,6 +163,7 @@ pub fn test_router(docker_socket: PathBuf, security: SecurityFilter) -> Router { build_router( AppState { docker_socket: docker_socket.clone(), + upgrade_tasks: Arc::new(tokio::sync::Mutex::new(Vec::new())), }, SecurityLayer::new(security, 1024 * 1024, Arc::clone(&metrics)), None, @@ -219,6 +226,9 @@ async fn proxy_handler( .pool_max_idle_per_host(0) .build(UnixConnector); let resp = client.request(req).await.map_err(|e| { + if is_length_limit_error(&e) { + return ProxyError::TooLarge("request body exceeded the configured limit".into()); + } tracing::error!(error = %e, path, "Docker upstream request failed"); ProxyError::Docker(format!("forward failed: {e}")) })?; @@ -234,13 +244,17 @@ async fn proxy_handler( if is_hop_by_hop(key.as_str(), &upstream_connection_headers) { continue; } - if let Ok(v) = value.to_str() { - response_builder = response_builder.header(key.as_str(), v); - } + response_builder = response_builder.header(key.as_str(), value.as_bytes()); } if status == StatusCode::SWITCHING_PROTOCOLS { - return switch_protocols(response_builder, resp, client_upgrade); + return switch_protocols( + response_builder, + resp, + client_upgrade, + Arc::clone(&state.upgrade_tasks), + ) + .await; } // Relayed frame by frame rather than collected: `/events` and follow-mode @@ -250,6 +264,24 @@ async fn proxy_handler( .map_err(|e| ProxyError::Internal(format!("failed to build response: {e}"))) } +/// Whether a failed upstream send is the streamed body-size limit firing. +/// +/// A `BodyRule::None` body is wrapped in [`http_body_util::Limited`] by the +/// enforcement layer and streamed through, so an over-limit chunked body fails +/// inside the hyper client as a nested [`http_body_util::LengthLimitError`] +/// rather than as a 413 the middleware set itself. +fn is_length_limit_error(mut err: &(dyn std::error::Error + 'static)) -> bool { + loop { + if err.is::() { + return true; + } + match err.source() { + Some(source) => err = source, + None => return false, + } + } +} + /// The protocol a client offered to upgrade to. /// /// RFC 9110 §7.8 requires the `Connection` field to name `upgrade` as well, so @@ -268,10 +300,11 @@ fn requested_upgrade( /// /// The client half only becomes available once this response has been written, /// so the copy has to outlive the handler. -fn switch_protocols( +async fn switch_protocols( mut response_builder: axum::http::response::Builder, mut upstream: hyper::Response, client_upgrade: Option, + upgrade_tasks: Arc>>>, ) -> Result { let client_upgrade = client_upgrade.ok_or_else(|| { ProxyError::Docker("upstream switched protocols without a client offer".into()) @@ -284,7 +317,9 @@ fn switch_protocols( } let upstream_upgrade = hyper::upgrade::on(&mut upstream); - tokio::spawn(async move { + // Tracked so graceful shutdown can drain (then bound) live exec/attach + // sessions instead of severing them when the runtime is dropped. + let handle = tokio::spawn(async move { let (client, upstream) = match tokio::try_join!(client_upgrade, upstream_upgrade) { Ok(pair) => pair, Err(e) => { @@ -299,12 +334,30 @@ fn switch_protocols( tracing::debug!(error = %e, "upgraded connection ended"); } }); + upgrade_tasks.lock().await.push(handle); response_builder .body(Body::empty()) .map_err(|e| ProxyError::Internal(format!("failed to build response: {e}"))) } +/// Let in-flight upgraded (101) connections finish, then abort any that outstay +/// a bounded drain so `docker stop` still terminates. +async fn drain_upgrades(tasks: &tokio::sync::Mutex>>) { + const DRAIN_TIMEOUT: Duration = Duration::from_secs(5); + + let handles = std::mem::take(&mut *tasks.lock().await); + if handles.is_empty() { + return; + } + info!(count = handles.len(), "draining upgraded connections"); + for handle in handles { + if tokio::time::timeout(DRAIN_TIMEOUT, handle).await.is_err() { + tracing::debug!("upgraded connection outlasted the drain window"); + } + } +} + /// Wait for a shutdown signal (SIGTERM or SIGINT). /// /// SIGTERM is what `docker stop` and Kubernetes send. A branch whose handler diff --git a/src/security.rs b/src/security.rs index e52b995..bf3d7e2 100644 --- a/src/security.rs +++ b/src/security.rs @@ -91,6 +91,12 @@ impl RuleList { rule.extend(methods, endpoints); self.push_rule(rule); } + + /// Add a rule built from method and endpoint patterns, mirroring + /// [`RuleSet::add`] but keeping it independent of every rule already here. + pub fn add(&mut self, methods: &[&str], endpoints: &[&str]) { + self.push_rule(RuleSet::new(methods, endpoints)); + } } const READ_METHODS: &[&str] = &["GET", "HEAD"]; @@ -163,7 +169,7 @@ const RUNTIME_ENDPOINTS: &[&str] = &[ /// for prefix matching. #[derive(Debug, Clone)] pub struct SecurityFilter { - allow: RuleSet, + allow: RuleList, deny: RuleList, exclude: RuleList, profile: SecurityProfile, @@ -179,7 +185,7 @@ impl SecurityFilter { /// caller rather than layered over a profile. pub fn deny_all() -> Self { Self { - allow: RuleSet::default(), + allow: RuleList::default(), deny: RuleList::default(), exclude: RuleList::default(), profile: SecurityProfile::None, @@ -188,7 +194,7 @@ impl SecurityFilter { /// Create a filter for a built-in security profile. pub fn for_profile(profile: &SecurityProfile) -> Self { - let mut allow = RuleSet::default(); + let mut allow = RuleList::default(); let mut deny = RuleList::default(); match profile { @@ -206,6 +212,10 @@ impl SecurityFilter { SecurityProfile::ContainerRuntime => { allow.add(READ_METHODS, READABLE_ENDPOINTS); allow.add(WRITE_METHODS, RUNTIME_ENDPOINTS); + // The exit status of an exec the caller already created is read + // (GET), which the write rule cannot carry; `docker exec` fails + // without it. + allow.add(&["GET"], &["/exec/*/json"]); let mut writes = RuleSet::new(WRITE_METHODS, MUTATING_ENDPOINTS); // Denials override allowances, so the endpoints this profile @@ -235,7 +245,7 @@ impl SecurityFilter { } /// Mutable access to the allow rules, for the policy loader. - pub fn allow_mut(&mut self) -> &mut RuleSet { + pub fn allow_mut(&mut self) -> &mut RuleList { &mut self.allow } @@ -259,7 +269,9 @@ impl SecurityFilter { /// A pattern matching no real Docker endpoint is dead policy: it neither /// grants nor blocks anything, while reading as though it does. pub fn endpoint_patterns(&self) -> Vec<&str> { - std::iter::once(&self.allow) + self.allow + .0 + .iter() .chain(self.deny.0.iter()) .chain(self.exclude.0.iter()) .flat_map(|rule| rule.endpoints.iter().map(String::as_str)) @@ -294,10 +306,9 @@ impl SecurityFilter { let path = normalize_path(path)?; self.check(method, &path)?; - if matches!(self.profile, SecurityProfile::ContainerRuntime) - && method == "POST" - && path == "/containers/create" - { + // Create is reachable only when some rule grants it; whatever granted + // it, the body still owes inspection. + if method == "POST" && path == "/containers/create" { return Ok(BodyRule::ContainerCreate); } Ok(BodyRule::None) @@ -365,9 +376,9 @@ fn percent_decode(path: &str) -> Result { let digits = bytes .get(i + 1..i + 3) .and_then(|d| std::str::from_utf8(d).ok()) - .ok_or_else(|| ProxyError::Forbidden("truncated percent-encoding in path".into()))?; + .ok_or_else(|| ProxyError::BadRequest("truncated percent-encoding in path".into()))?; let byte = u8::from_str_radix(digits, 16) - .map_err(|_| ProxyError::Forbidden("invalid percent-encoding in path".into()))?; + .map_err(|_| ProxyError::BadRequest("invalid percent-encoding in path".into()))?; if byte == b'/' || byte == b'\\' { return Err(ProxyError::Forbidden( @@ -379,7 +390,7 @@ fn percent_decode(path: &str) -> Result { i += 3; } - String::from_utf8(out).map_err(|_| ProxyError::Forbidden("path is not valid UTF-8".into())) + String::from_utf8(out).map_err(|_| ProxyError::BadRequest("path is not valid UTF-8".into())) } /// Resolve `.` and `..` segments and collapse empty ones (RFC 3986 §5.2.4). @@ -440,25 +451,44 @@ fn check_create_body(body: &[u8]) -> SecurityResult { .ok_or_else(|| ProxyError::Forbidden("container create body must be an object".into()))?; let image = object.get("Image").and_then(serde_json::Value::as_str); - if image.is_none() || image == Some("") || object.get("Cmd").is_none() { + if image.is_none() || image == Some("") { return Err(ProxyError::Forbidden( - "container create requires Image and Cmd".into(), + "container create requires Image".into(), )); } - for key in [ - "Privileged", - "CapAdd", - "SecurityOpt", - "Devices", - "PidMode", - "IpcMode", - "UsernsMode", - ] { - if object.get(key).is_some_and(|value| !value.is_null()) { - return Err(ProxyError::Forbidden(format!( - "container create field is not permitted: {key}" - ))); + // The daemon reads these fields from the nested HostConfig, so a body that + // sets them there is exactly as privileged as one that sets them at the + // top level; both levels are inspected. + for object in std::iter::once(object).chain( + object + .get("HostConfig") + .and_then(serde_json::Value::as_object), + ) { + for key in [ + "Privileged", + "CapAdd", + "SecurityOpt", + "Devices", + "DeviceRequests", + "PidMode", + "IpcMode", + "UsernsMode", + ] { + if object.get(key).is_some_and(meaningfully_set) { + return Err(ProxyError::Forbidden(format!( + "container create field is not permitted: {key}" + ))); + } + } + if object + .get("NetworkMode") + .and_then(serde_json::Value::as_str) + .is_some_and(|mode| mode == "host") + { + return Err(ProxyError::Forbidden( + "container create field is not permitted: NetworkMode".into(), + )); } } @@ -478,6 +508,20 @@ fn check_create_body(body: &[u8]) -> SecurityResult { Ok(()) } +/// Whether a field value amounts to a request, rather than an explicit unset. +/// +/// `null`, `false`, an empty array, and an empty string all read as "do not +/// change this" to the Docker daemon, so none of them trips an inspection. +fn meaningfully_set(value: &serde_json::Value) -> bool { + match value { + serde_json::Value::Null => false, + serde_json::Value::Bool(false) => false, + serde_json::Value::Array(values) => !values.is_empty(), + serde_json::Value::String(text) => !text.is_empty(), + _ => true, + } +} + // ── Pattern matching ─────────────────────────────────────────── /// Match a path against a pattern, in one of three modes: @@ -613,6 +657,40 @@ mod tests { ); } + #[test] + fn container_runtime_profile_does_not_grant_writes_on_readable_prefixes() { + let f = SecurityFilter::for_profile(&SecurityProfile::ContainerRuntime); + for (method, path) in [ + ("DELETE", "/volumes/abc"), + ("DELETE", "/networks/net"), + ("POST", "/volumes/create"), + ("POST", "/networks/create"), + ] { + assert!( + f.check(method, path).is_err(), + "the readable /volumes/ and /networks/ prefixes must not admit {method} {path}" + ); + } + } + + #[test] + fn body_inspection_follows_the_effective_policy_not_the_profile_enum() { + let mut f = SecurityFilter::deny_all(); + f.allow_mut().push( + Some(vec!["POST".into()]), + Some(vec!["/containers/create".into()]), + ); + assert!( + f.check_request( + "POST", + "/containers/create", + br#"{"Image":"x","HostConfig":{"Privileged":true}}"# + ) + .is_err(), + "a create granted by allowlist is still body-inspected" + ); + } + #[test] fn default_profile_refuses_the_exec_lifecycle() { let f = SecurityFilter::new(); @@ -631,16 +709,55 @@ mod tests { let f = SecurityFilter::for_profile(&SecurityProfile::ContainerRuntime); let body = br#"{"Image":"worker:latest","Cmd":["dagster","api"],"Labels":{"dagster/run_id":"run-1","dagster/job_name":"job"}}"#; assert!(f.check_request("POST", "/containers/create", body).is_ok()); + assert!( + f.check_request("POST", "/containers/create", br#"{"Image":""}"#) + .is_err(), + "an empty Image is not a usable image" + ); + assert!( + f.check_request("POST", "/containers/create", br#"{}"#) + .is_err(), + "a missing Image is not a usable image" + ); assert!( f.check_request("POST", "/containers/create", br#"{"Image":"worker"}"#) - .is_err() + .is_ok(), + "the image's default Cmd is legitimate" ); - let privileged = br#"{"Image":"worker","Cmd":[],"Labels":{"dagster/run_id":"r","dagster/job_name":"j"},"Privileged":true}"#; + let privileged = br#"{"Image":"worker","Labels":{"dagster/run_id":"r","dagster/job_name":"j"},"HostConfig":{"Privileged":true}}"#; assert!( f.check_request("POST", "/containers/create", privileged) .is_err() ); - let mounted = br#"{"Image":"worker","Cmd":[],"Mounts":[{"Type":"bind","Source":"/opt/knime","Target":"/opt/knime"}]}"#; + let pid_host = br#"{"Image":"worker","HostConfig":{"PidMode":"host"}}"#; + assert!( + f.check_request("POST", "/containers/create", pid_host) + .is_err() + ); + let cap_add = br#"{"Image":"worker","HostConfig":{"CapAdd":["SYS_ADMIN"]}}"#; + assert!( + f.check_request("POST", "/containers/create", cap_add) + .is_err() + ); + let gpu = br#"{"Image":"worker","HostConfig":{"DeviceRequests":[{"Driver":"nvidia","Count":1}]}}"#; + assert!(f.check_request("POST", "/containers/create", gpu).is_err()); + let network_host = br#"{"Image":"worker","NetworkMode":"host"}"#; + assert!( + f.check_request("POST", "/containers/create", network_host) + .is_err() + ); + let privileged_false = + br#"{"Image":"worker","Privileged":false,"HostConfig":{"Privileged":false}}"#; + assert!( + f.check_request("POST", "/containers/create", privileged_false) + .is_ok() + ); + let cap_add_empty = br#"{"Image":"worker","HostConfig":{"CapAdd":[]}}"#; + assert!( + f.check_request("POST", "/containers/create", cap_add_empty) + .is_ok() + ); + let mounted = br#"{"Image":"worker","Mounts":[{"Type":"bind","Source":"/opt/knime","Target":"/opt/knime"}]}"#; assert!( f.check_request("POST", "/containers/create", mounted) .is_ok() @@ -651,7 +768,7 @@ mod tests { fn container_removal_stays_denied_when_delete_is_allowed() { let mut f = filter(); f.allow_mut() - .extend(Some(vec!["DELETE".into()]), Some(Vec::new())); + .push(Some(vec!["DELETE".into()]), Some(Vec::new())); assert!(f.check("DELETE", "/containers/abc").is_err()); assert!(f.check("POST", "/containers/prune").is_err()); } @@ -663,7 +780,7 @@ mod tests { assert!(f.check("GET", "/_ping").is_err()); f.allow_mut() - .extend(Some(vec!["GET".into()]), Some(vec!["/version".into()])); + .push(Some(vec!["GET".into()]), Some(vec!["/version".into()])); assert!(f.check("GET", "/version").is_ok()); assert!( f.check("GET", "/info").is_err(), @@ -801,14 +918,30 @@ mod tests { #[test] fn rejects_encoded_path_separators() { - assert!(normalize_path("/containers%2f..%2finfo").is_err()); - assert!(normalize_path("/containers%5cinfo").is_err()); + assert!(matches!( + normalize_path("/containers%2f..%2finfo"), + Err(ProxyError::Forbidden(_)) + )); + assert!(matches!( + normalize_path("/containers%5cinfo"), + Err(ProxyError::Forbidden(_)) + )); } #[test] fn rejects_malformed_percent_encoding() { - assert!(normalize_path("/info%").is_err()); - assert!(normalize_path("/info%zz").is_err()); + assert!(matches!( + normalize_path("/info%"), + Err(ProxyError::BadRequest(_)) + )); + assert!(matches!( + normalize_path("/info%zz"), + Err(ProxyError::BadRequest(_)) + )); + assert!(matches!( + normalize_path("/%ff"), + Err(ProxyError::BadRequest(_)) + )); } #[test] diff --git a/tests/api_surface.rs b/tests/api_surface.rs index 197e1ab..8bafc54 100644 --- a/tests/api_surface.rs +++ b/tests/api_surface.rs @@ -45,6 +45,7 @@ fn every_shipped_pattern_matches_a_real_endpoint() { #[test] fn the_matcher_distinguishes_real_endpoints_from_invented_ones() { assert!(docker_api::matches_known_path("/containers/*/json")); + assert!(docker_api::matches_known_path("/containers/abc/json")); assert!(docker_api::matches_known_path("/containers/")); assert!(docker_api::matches_known_path("/_ping")); assert!(!docker_api::matches_known_path("/containers/*/delete")); diff --git a/tests/examples.rs b/tests/examples.rs index bc8ec32..cef5fff 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -107,12 +107,18 @@ fn create_inspection_example_rejects_the_bodies_it_documents() { let create = |body: &str| filter.check_request("POST", "/containers/create", body.as_bytes()); assert!(create(r#"{"Image":"worker:1","Cmd":["run"]}"#).is_ok()); + assert!( + create(r#"{"Image":"worker:1"}"#).is_ok(), + "the image's default Cmd is legitimate" + ); for body in [ - r#"{"Image":"x","Cmd":[],"Privileged":true}"#, - r#"{"Image":"x","Cmd":[],"CapAdd":["SYS_ADMIN"]}"#, - r#"{"Image":"x","Cmd":[],"PidMode":"host"}"#, - r#"{"Image":"x"}"#, + r#"{"Image":"x","HostConfig":{"Privileged":true}}"#, + r#"{"Image":"x","HostConfig":{"CapAdd":["SYS_ADMIN"]}}"#, + r#"{"Image":"x","HostConfig":{"PidMode":"host"}}"#, + r#"{"Image":"x","HostConfig":{"DeviceRequests":[{"Driver":"nvidia","Count":1}]}}"#, + r#"{"Image":"x","NetworkMode":"host"}"#, + r#"{}"#, ] { assert!(create(body).is_err(), "{body}"); } diff --git a/tests/integration.rs b/tests/integration.rs index 4a59551..a789e86 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -307,3 +307,60 @@ async fn parallel_requests_keep_their_own_headers() { ); } } + +/// A malformed percent-encoding is a syntactically invalid request (400), not a +/// policy denial (403): the path never reached the matching stage. +#[tokio::test] +async fn returns_400_for_malformed_path() { + // The socket is never contacted; normalization rejects before forwarding. + let router = test_router( + PathBuf::from("/nonexistent/docker.sock"), + SecurityFilter::new(), + ); + + // `%FF` is well-formed percent-encoding that decodes to an invalid UTF-8 + // byte, so it passes URI parsing and is refused by the normalizer as a bad + // request rather than an authorization failure. + let req = Request::builder() + .method(Method::GET) + .uri("/info%FF") + .body(Body::empty()) + .unwrap(); + + let resp = router.oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let body = resp.into_body().collect().await.unwrap().to_bytes(); + let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); + assert!( + json["message"].as_str().is_some(), + "Docker-shaped error body" + ); +} + +/// Body inspection must run for any policy that permits `POST /containers/create`, +/// not only the `container-runtime` profile. +#[tokio::test] +async fn create_body_inspection_runs_for_non_runtime_profiles() { + let socket = std::env::temp_dir().join("test-proxy-inspect-none.sock"); + spawn_mock(socket.clone()).await; + + let mut filter = SecurityFilter::deny_all(); + filter.allow_mut().push( + Some(vec!["POST".to_owned()]), + Some(vec!["/containers/create".to_owned()]), + ); + let router = test_router(socket, filter); + + let req = Request::builder() + .method(Method::POST) + .uri("/containers/create") + .header(header::CONTENT_TYPE, "application/json") + .body(Body::from( + r#"{"Image":"x","HostConfig":{"Privileged":true}}"#, + )) + .unwrap(); + + let resp = router.oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +}