diff --git a/AGENTS.md b/AGENTS.md index ef6a44b..3f482e1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,17 +39,17 @@ deploy/ — Docker Compose + integration tests - Zero external deps where possible (Go: yaml.v3, Rust: tokio/hyper/serde/clap, TS: yaml) ## Test Coverage -- Go: 114 unit tests (main/listener: 24, policy: 10, middleware: 26, proxy: 50, audit: 4) -- Rust: 146 unit tests (main/listener: 23, policy: 15, middleware: 42, proxy: 49, handler: 11, audit: 4, transport: 2) -- TypeScript: 165 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 33, proxy: 36, policy: 10, handler: 15, shutdown: 5, transport: 5, audit: 4) -- Integration, per implementation: 72 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) -- Quint: `make test-spec` runs the `spec/listener.qnt` `run` tests (instances `listener_locked`, `listener_unlocked`) and the `spec/router.qnt` `run` tests (instances `router`, `router_pre48`, `router_pre53`, `router_pre55`, `router_readonly`, whose `readonly*` tests run with `--match`, `router_pre76`, `router_pre78` and `router_pre70`), and the `spec/docker_socket_policy.qnt` `run` tests (instances `network_endpoints`, `network_endpoints_pre87`) +- Go: 118 unit tests (main/listener: 24, policy: 11, middleware: 27, proxy: 52, audit: 4) +- Rust: 145 unit tests (main/listener: 23, policy: 13, middleware: 43, proxy: 47, handler: 13, audit: 4, transport: 2) +- TypeScript: 169 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 34, proxy: 35, policy: 12, handler: 17, shutdown: 5, transport: 5, audit: 4) +- Integration, per implementation: 77 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) +- Quint: `make test-spec` runs the `spec/listener.qnt` `run` tests (instances `listener_locked`, `listener_unlocked`) and the `spec/router.qnt` `run` tests (instances `router`, `router_pre48`, `router_pre53`, `router_pre55`, `router_readonly`, whose `readonly*` tests run with `--match`, `router_pre76`, `router_pre78` and `router_pre70`; and, for the image pull, `pull`, `pull_pre83`, `pull_body_ignored` and `pull_pre84`), and the `spec/docker_socket_policy.qnt` `run` tests (instances `network_endpoints`, `network_endpoints_pre87`) ## Test Conventions - Go: stdlib `testing` package, `go test ./...` - Rust: `#[cfg(test)]` inline modules, `cargo test` - TypeScript: `node:test` framework, `npm run build && node --test dist/*.test.js` -- Integration: `make test-integration` (72 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose +- Integration: `make test-integration` (77 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose ## Contribution Workflow diff --git a/Makefile b/Makefile index 6905cd9..6c28ed6 100644 --- a/Makefile +++ b/Makefile @@ -108,6 +108,10 @@ test-spec: $(QUINT) test $(ROUTER_SPEC) --main=router_pre70 $(QUINT) test $(SPEC) --main=network_endpoints $(QUINT) test $(SPEC) --main=network_endpoints_pre87 + $(QUINT) test $(ROUTER_SPEC) --main=pull + $(QUINT) test $(ROUTER_SPEC) --main=pull_pre83 + $(QUINT) test $(ROUTER_SPEC) --main=pull_body_ignored + $(QUINT) test $(ROUTER_SPEC) --main=pull_pre84 verify-ts: $(QUINT) run $(SPEC) --max-steps=50 --invariants allInvariants --backend typescript diff --git a/README.md b/README.md index 7a82e07..f392234 100644 --- a/README.md +++ b/README.md @@ -100,9 +100,9 @@ All three implementations expose the same API surface, share the same [Quint spe | Language | Directory | Tests | Stack | |----------|-----------|-------|-------| -| Go | [go/](go/) | 114 unit + 72 integration + 15 socket | stdlib net/http + yaml.v3 | -| Rust | [rs/](rs/) | 146 unit + 72 integration + 15 socket | tokio, hyper, serde, clap | -| TypeScript | [ts/](ts/) | 165 unit (1 skipped) + 72 integration + 15 socket | Node 22 ESM, built-in http | +| Go | [go/](go/) | 118 unit + 77 integration + 15 socket | stdlib net/http + yaml.v3 | +| Rust | [rs/](rs/) | 145 unit + 77 integration + 15 socket | tokio, hyper, serde, clap | +| TypeScript | [ts/](ts/) | 169 unit (1 skipped) + 77 integration + 15 socket | Node 22 ESM, built-in http | ### Build All @@ -193,7 +193,7 @@ docker pull attacker/malware:latest # denied: image not in allowlist | Middleware | Type | What it checks | |------------|------|----------------| | ContainerConfigMutator | Mutator | Forces `network_mode`, `user`, `security_options`, `restart_policy` from policy, and makes `NetworkingConfig.EndpointsConfig` match a forced `network_mode` | -| RegistryGate | Gate | Validates image ref against `allowed_image_prefixes` | +| RegistryGate | Gate | Validates the normalised image ref against `allowed_image_prefixes`, then `image_tag_pattern` and `image_digest_allowed` | | MountSourceGate | Gate | Validates volume binds against whitelist | | EnvFileGate | Gate | Strips `Env` field from create body; env must come from locked `env_file` | | CmdGate | Gate | Validates each CLI flag in `Cmd` array against allowlist + denylist | @@ -220,7 +220,7 @@ The chain runs only on container create. `--readonly` is not a gate: the router | Any | `/exec/*` | **DENIED** | | Any | `/containers/{name}/attach…` (an `attach` segment anywhere under `/containers`) | **DENIED** | | POST | `/containers/{name}/rename\|update` | **DENIED** | -| POST | `/images/create` | Validated by registry gate | +| POST | `/images/create` | Pull: `fromImage` and `tag` from the query string get the image checks (see below); a request body or `fromSrc` (import) **DENIED** | | POST | `/auth` | **DENIED** | | POST | `/build` | **DENIED** | | POST | `/commit` | **DENIED** | @@ -235,6 +235,10 @@ Attach is matched the same way: an `attach` segment under `/containers` is denie `/_ping`, `/version`, `/info` and `/events` are ordinary reads: `GET` and `HEAD` pass, and every other method is denied with `endpoint is not allowed`, `OPTIONS` (a CORS preflight) included ([#78](https://github.com/ChainSafe/docker-socket-policy/issues/78)). `CONNECT` is refused with 403 `CONNECT is not allowed` before anything else, whatever its target or headers; the TypeScript proxy answers it on the raw socket, since Node never hands `CONNECT` to the request handler. +An image pull is routed on its query string, as the Docker Engine API defines it and the Docker CLI sends it: `POST /images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19` and no body ([#83](https://github.com/ChainSafe/docker-socket-policy/issues/83)). A pull with any body is denied (`image pull must not have a request body`), whatever its `Content-Type` or framing: the daemon reads a form body's `fromImage` over the query's, so a body could name an image the router never checked. The image gets the same checks as `Image` on create: its normalised repository must match a policy's prefix, then the tag must match that policy's `image_tag_pattern` and a digest needs `image_digest_allowed` (the CLI sends a digest in `tag`). The proxy denies a pull with no `fromImage` (`fromImage query parameter is required for image pull`), an import (`image import (fromSrc) is not allowed`: it would store any tarball under an allowed name), a tag in both `fromImage` and `tag`, a repeated `fromImage`, `tag` or `fromSrc`, a query the daemon would reject, and a pull with neither tag nor digest, which makes the daemon pull every tag of the repository (`image pull without a tag or digest pulls every tag and is not allowed`; `docker pull alpine` sends `tag=latest`, `docker pull --all-tags` is denied). + +Image references are normalised before policy matching, in all three implementations and for both the request and the policy's `allowed_image_prefixes` (at load time) ([#84](https://github.com/ChainSafe/docker-socket-policy/issues/84)). The reference is split the way Docker does: the digest follows `@`, the tag follows the last `:` after the last `/` (so `localhost:5000/team/app:1.2` has tag `1.2`), and the first component is a registry host when it contains `.` or `:` or is `localhost`. Docker Hub's `docker.io/`, `index.docker.io/` and `registry-1.docker.io/` are dropped, and so is `library/` before a single-component name, so `alpine`, `library/alpine` and `docker.io/library/alpine` are the same image, as are `chainsafe/lodestar` and `docker.io/chainsafe/lodestar`. The registry host is compared lower-cased; upper case in the repository path is invalid, as it is for Docker (`invalid image reference "…"`). The prefix match stays on whole path components: `alpine` does not match `alpine-evil`, and `alpine` (Docker Hub) does not match `localhost:5000/alpine`. A prefix that is not a valid repository name, or that carries a tag or digest, fails the policy load. + ## Configuration ### CLI Flags diff --git a/deploy/config/pull-allowed.yaml b/deploy/config/pull-allowed.yaml new file mode 100644 index 0000000..850c384 --- /dev/null +++ b/deploy/config/pull-allowed.yaml @@ -0,0 +1,6 @@ +# Integration-only policy: lets deploy/test.sh pull a real image through the +# proxy (#83). busybox is a few MB, so the pull is quick. +service_name: pull-allowed +allowed_image_prefixes: + - busybox +image_tag_pattern: '^latest$' diff --git a/deploy/test.sh b/deploy/test.sh index a7dfd4a..c304df0 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -304,30 +304,61 @@ fi echo "" echo "--- Docker API passthrough ---" -# Image pull: allowed image passes proxy, daemon fails (image doesn't exist) -S=$(post_json '{"fromImage":"chainsafe/lodestar:beacon"}' "$PROXY/images/create") -if [ "$S" = "200" ]; then - echo " PASS: image pull allowed -> $S" - PASS=$((PASS+1)) -elif [ "$S" = "500" ]; then - echo " PASS: image pull allowed (500 = daemon can't fetch, not proxy) -> $S" - PASS=$((PASS+1)) -elif [ "$S" = "404" ]; then - echo " PASS: image pull allowed (404 = daemon can't fetch, not proxy) -> $S" +# #83: image pulls are routed on the query string, as the Docker CLI sends +# them: fromImage (fully qualified, URL-encoded) and tag, with an empty body. +# config/pull-allowed.yaml allows busybox:latest, a few MB, so the allowed pull +# runs to completion against the daemon: 200 and a stream with no error in it. +# The image is sent as docker.io/library/busybox, so the check also covers +# normalisation of the policy's bare busybox prefix (#84). Before +# #83 every such pull was denied with "empty request body". +# pull_query QUERY [BODY [CONTENT_TYPE [HEADER]]]: POST /images/create?QUERY, +# empty body unless given, JSON unless CONTENT_TYPE is given. +pull_query() { + : >"$BODY" + out=$(curl -s -o "$BODY" -w '%{http_code}' --max-time 120 --connect-timeout 2 --unix-socket "$PROXY_SOCK" \ + -X POST -H "Content-Type: ${3:-application/json}" ${4:+-H "$4"} -d "${2:-}" \ + "$PROXY/v1.45/images/create?$1" 2>/dev/null || true) + echo "${out:-000}" +} + +S=$(pull_query 'fromImage=docker.io%2Flibrary%2Fbusybox&tag=latest') +if [ "$S" = "200" ] && grep -q '"status"' "$BODY" && ! grep -q '"error' "$BODY"; then + echo " PASS: pull allowed image the CLI way -> 200 (pulled)" PASS=$((PASS+1)) else - echo " FAIL: image pull (expected 200|404|500, got $S)" + echo " FAIL: pull allowed image the CLI way (expected 200 and a clean pull, got $S: $(head -c 200 "$BODY"))" FAIL=$((FAIL+1)) fi -# Unlisted image pull -> proxy denies -S=$(post_json '{"fromImage":"alpine:latest"}' "$PROXY/images/create") -check_denied "pull unlisted image -> 403" "$S" "$BODY" 'image alpine:latest not allowed by any policy' +S=$(pull_query 'fromImage=docker.io%2Flibrary%2Falpine&tag=latest') +check_denied "pull unlisted image the CLI way -> 403" "$S" "$BODY" 'image docker.io/library/alpine not allowed by any policy' + +S=$(pull_query 'fromImage=alpine&tag=latest') +check_denied "pull alpine (unlisted, short name) -> 403" "$S" "$BODY" 'image alpine not allowed by any policy' -# Real container lifecycle with busybox (image that exists) -# Step 1: Pull busybox through proxy (should be denied — not in policy) -S=$(post_json '{"fromImage":"busybox:latest"}' "$PROXY/images/create") -check_denied "pull busybox (unlisted) -> 403" "$S" "$BODY" 'image busybox:latest not allowed by any policy' +# #83: pull-allowed.yaml's tag pattern applies to pulls as it does to creates. +S=$(pull_query 'fromImage=busybox&tag=musl') +check_denied "pull allowed image with a tag outside the pattern -> 403" "$S" "$BODY" 'image tag "musl" does not match pattern' + +# #83: a pull has no body. Before #83 an allowed image in a JSON body +# forwarded a pull of the query's image (alpine here, pulled for real). The +# daemon reads a form body over the query, so routing on the query and +# forwarding the body would let a form body name any image: with busybox +# (allowed) in the query and alpine in the form body, the daemon pulls +# alpine. Every body is denied, whatever its Content-Type or framing. +body_msg='image pull must not have a request body' +S=$(pull_query 'fromImage=alpine&tag=latest' '{"fromImage":"chainsafe/lodestar"}') +check_denied "pull with allowed image in JSON body, unlisted in query -> 403" "$S" "$BODY" "$body_msg" + +S=$(pull_query 'fromImage=busybox&tag=latest' 'fromImage=alpine&tag=3.19' 'application/x-www-form-urlencoded') +check_denied "pull with allowed image in query, unlisted in form body -> 403" "$S" "$BODY" "$body_msg" + +S=$(pull_query 'fromImage=busybox&tag=latest' 'fromImage=alpine&tag=3.19' 'application/x-www-form-urlencoded' 'Transfer-Encoding: chunked') +check_denied "pull with a chunked form body -> 403" "$S" "$BODY" "$body_msg" + +# #83: an import (fromSrc) would store any tarball under an allowed name. +S=$(pull_query 'fromSrc=http%3A%2F%2F127.0.0.1%3A9%2Fx.tar&repo=chainsafe%2Flodestar&tag=imported') +check_denied "image import (fromSrc) -> 403" "$S" "$BODY" 'image import (fromSrc) is not allowed' # ─── Endpoint completeness ───────────────────────────── @@ -542,15 +573,10 @@ check_denied "GET /containers/./json -> 403 (dot segment)" "$S" "$BODY" 'dot pat echo "" echo "--- Forced network_mode (#87) ---" -# pull IMAGE TAG: pull through the proxy. The router reads the body (and the -# query string, #83); the daemon reads the query string. A pull streams until -# it completes, so it gets a longer timeout. +# pull IMAGE TAG: pull through the proxy the way the Docker CLI does, +# fromImage and tag in the query string and no body (#83); see pull_query. pull() { - : >"$BODY" - out=$(curl -s -o "$BODY" -w '%{http_code}' --max-time 120 --connect-timeout 2 --unix-socket "$PROXY_SOCK" \ - -X POST -H "Content-Type: application/json" -d "{\"fromImage\":\"$1:$2\"}" \ - "$PROXY/images/create?fromImage=$1&tag=$2" 2>/dev/null || true) - echo "${out:-000}" + pull_query "fromImage=$1&tag=$2" } # start_forced NAME IMAGE MODE: create NAME as the CLI does, start it, and diff --git a/go/internal/middleware/chain_test.go b/go/internal/middleware/chain_test.go index 35afe77..319fc12 100644 --- a/go/internal/middleware/chain_test.go +++ b/go/internal/middleware/chain_test.go @@ -507,3 +507,42 @@ func TestChain_NetworkEndpoints(t *testing.T) { t.Fatalf("%d mismatches:\n%s", len(mismatches), strings.Join(mismatches, "\n")) } } + +// TestRegistryGate_ParsesReferences pins the registry gate's reference parser +// (#84): a registry port is not a tag, Docker Hub spellings match the +// normalised prefix, and the tag pattern sees only the tag. The policy's +// prefixes are normalised, as the manager stores them. Same rows in Rust and +// TypeScript. +func TestRegistryGate_ParsesReferences(t *testing.T) { + gate := &RegistryGate{} + r := testRequest("POST", "/containers/create") + p := testPolicy(map[string]interface{}{ + "allowed_image_prefixes": []string{"chainsafe/lodestar", "localhost:5000/team", "registry.example.com:443/a"}, + }) + p.ImageTagPattern = `^(v[0-9]+|[0-9.]+)$` + + tests := []struct { + image string + wantErr string + }{ + {"localhost:5000/team/app:1.2", ""}, + {"localhost:5000/team/app", ""}, + {"docker.io/chainsafe/lodestar:v1", ""}, + {"registry.example.com:443/a", ""}, + {"Registry.Example.com:443/a:2", ""}, + {"localhost:5000/team/app:latest", `image tag "latest" does not match pattern "^(v[0-9]+|[0-9.]+)$"`}, + {"localhost:5000/other:1", `image "localhost:5000/other" is not in allowed prefixes`}, + {"chainsafe/Lodestar:v1", `invalid image reference "chainsafe/Lodestar:v1"`}, + } + for _, tt := range tests { + t.Run(tt.image, func(t *testing.T) { + err := gate.Check(r, p, map[string]interface{}{"Image": tt.image}) + switch { + case tt.wantErr == "" && err != nil: + t.Errorf("Check(%s) = %v, want nil", tt.image, err) + case tt.wantErr != "" && (err == nil || !strings.Contains(err.Error(), tt.wantErr)): + t.Errorf("Check(%s) = %v, want error containing %q", tt.image, err, tt.wantErr) + } + }) + } +} diff --git a/go/internal/middleware/registry.go b/go/internal/middleware/registry.go index 63ca931..7708b67 100644 --- a/go/internal/middleware/registry.go +++ b/go/internal/middleware/registry.go @@ -3,8 +3,6 @@ package middleware import ( "fmt" "net/http" - "regexp" - "strings" "github.com/ChainSafe/docker-socket-policy/go/internal/policy" ) @@ -17,53 +15,15 @@ func (g *RegistryGate) Check(r *http.Request, p *policy.Policy, body map[string] return nil } - imageName, tagOrDigest := splitImageRef(image) - - if !isAllowedPrefix(imageName, p.AllowedImagePrefixes) { - return fmt.Errorf("image %q is not in allowed prefixes: %v", imageName, p.AllowedImagePrefixes) - } - - if tagOrDigest != "" { - if len(tagOrDigest) > 128 { - return fmt.Errorf("image tag/digest exceeds 128 characters") - } - - if strings.HasPrefix(tagOrDigest, "sha256:") { - if !p.ImageDigestAllowed { - return fmt.Errorf("image digests not allowed for service %s", p.ServiceName) - } - matched, _ := regexp.MatchString(`^sha256:[a-f0-9]{64}$`, tagOrDigest) - if !matched { - return fmt.Errorf("invalid digest format: %s", tagOrDigest) - } - } else if p.ImageTagPattern != "" { - matched, err := regexp.MatchString(p.ImageTagPattern, tagOrDigest) - if err != nil { - return fmt.Errorf("invalid tag pattern regex: %w", err) - } - if !matched { - return fmt.Errorf("image tag %q does not match pattern %q", tagOrDigest, p.ImageTagPattern) - } - } + // The same parser as the router, so a registry port is not a tag (#84). + ref, err := policy.ParseImageRef(image) + if err != nil { + return err } - return nil -} - -func splitImageRef(ref string) (name, tagOrDigest string) { - for i, ch := range ref { - if ch == ':' || ch == '@' { - return ref[:i], ref[i+1:] - } + if !policy.MatchesImagePrefix(ref.Name, p.AllowedImagePrefixes) { + return fmt.Errorf("image %q is not in allowed prefixes: %v", ref.Name, p.AllowedImagePrefixes) } - return ref, "" -} -func isAllowedPrefix(imageName string, prefixes []string) bool { - for _, prefix := range prefixes { - if imageName == prefix || strings.HasPrefix(imageName, prefix+"/") { - return true - } - } - return false + return policy.CheckImageTag(p, ref) } diff --git a/go/internal/policy/image.go b/go/internal/policy/image.go new file mode 100644 index 0000000..7f99651 --- /dev/null +++ b/go/internal/policy/image.go @@ -0,0 +1,140 @@ +package policy + +import ( + "fmt" + "regexp" + "strings" +) + +// ImageRef is a parsed image reference (#84). Name is the normalised +// repository: the registry host, lower-cased, and the path; Docker Hub's host +// is dropped, and so is library/ before a single-component name, so alpine, +// library/alpine and docker.io/library/alpine are all alpine. Tag and Digest +// are empty when absent. +type ImageRef struct { + Name string + Tag string + Digest string +} + +var ( + defaultRegistries = map[string]bool{"docker.io": true, "index.docker.io": true, "registry-1.docker.io": true} + domainRe = regexp.MustCompile(`^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?(?:\.[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)*(?::[0-9]+)?$`) + pathComponentRe = regexp.MustCompile(`^[a-z0-9]+(?:(?:[._]|__|-+)[a-z0-9]+)*$`) + tagRe = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_.-]*$`) + sha256DigestRe = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) +) + +// ParseImageRef parses [registry/]repository[:tag][@digest] the way Docker +// does (#84). The digest follows the first @; the tag follows the last : after +// the last /, so a registry port is not a tag. The first path component is a +// registry host when it contains . or : or is localhost. Only the host is +// lower-cased: upper case in the path is invalid. The digest is checked by +// CheckImageTag, not here. +func ParseImageRef(ref string) (ImageRef, error) { + invalid := fmt.Errorf(`invalid image reference "%s"`, ref) + rest, digest, hasDigest := strings.Cut(ref, "@") + if hasDigest && digest == "" { + return ImageRef{}, invalid + } + afterSlash := strings.LastIndexByte(rest, '/') + 1 + name, tag := rest, "" + if i := strings.LastIndexByte(rest[afterSlash:], ':'); i != -1 { + name, tag = rest[:afterSlash+i], rest[afterSlash+i+1:] + if !tagRe.MatchString(tag) { + return ImageRef{}, invalid + } + } + normalized, ok := normalizeName(name) + if !ok { + return ImageRef{}, invalid + } + return ImageRef{Name: normalized, Tag: tag, Digest: digest}, nil +} + +func normalizeName(name string) (string, bool) { + domain, path := "", name + if first, rest, ok := strings.Cut(name, "/"); ok && (strings.ContainsAny(first, ".:") || first == "localhost") { + domain, path = strings.ToLower(first), rest + if !domainRe.MatchString(domain) { + return "", false + } + } + for _, c := range strings.Split(path, "/") { + if !pathComponentRe.MatchString(c) { + return "", false + } + } + if defaultRegistries[domain] { + domain = "" + } + full := path + if domain != "" { + full = domain + "/" + path + } else if official, ok := strings.CutPrefix(path, "library/"); ok && !strings.Contains(official, "/") { + full = official + } + if len(full) > 255 { + return "", false + } + return full, true +} + +// NormalizeImagePrefix normalises a policy prefix at load time (#84). A +// prefix names a repository or a namespace, so a tag or digest makes it +// invalid. +func NormalizeImagePrefix(prefix string) (string, error) { + r, err := ParseImageRef(prefix) + if err != nil { + return "", err + } + if r.Tag != "" || r.Digest != "" { + return "", fmt.Errorf(`image prefix "%s" has a tag or digest`, prefix) + } + return r.Name, nil +} + +// MatchesImagePrefix matches a normalised repository against normalised +// prefixes on namespace boundaries ("prefix" or "prefix/..."), never a bare +// substring. +func MatchesImagePrefix(name string, prefixes []string) bool { + for _, prefix := range prefixes { + if name == prefix || strings.HasPrefix(name, prefix+"/") { + return true + } + } + return false +} + +// CheckImageTag applies a policy's digest and tag rules to a parsed +// reference: the digest must be allowed and well-formed, the tag must match +// image_tag_pattern. Shared by the registry gate (create) and the router +// (pull, #83). +func CheckImageTag(p *Policy, r ImageRef) error { + if r.Digest != "" { + if len(r.Digest) > 128 { + return fmt.Errorf("image tag/digest exceeds 128 characters") + } + if !p.ImageDigestAllowed { + return fmt.Errorf("image digests not allowed for service %s", p.ServiceName) + } + if !sha256DigestRe.MatchString(r.Digest) { + return fmt.Errorf("invalid digest format: %s", r.Digest) + } + } + if r.Tag != "" { + if len(r.Tag) > 128 { + return fmt.Errorf("image tag/digest exceeds 128 characters") + } + if p.ImageTagPattern != "" { + matched, err := regexp.MatchString(p.ImageTagPattern, r.Tag) + if err != nil { + return fmt.Errorf("invalid tag pattern regex: %w", err) + } + if !matched { + return fmt.Errorf("image tag %q does not match pattern %q", r.Tag, p.ImageTagPattern) + } + } + } + return nil +} diff --git a/go/internal/policy/manager.go b/go/internal/policy/manager.go index 72f8f71..bf157fb 100644 --- a/go/internal/policy/manager.go +++ b/go/internal/policy/manager.go @@ -5,7 +5,6 @@ import ( "log/slog" "os" "path/filepath" - "strings" "gopkg.in/yaml.v3" ) @@ -72,6 +71,15 @@ func loadPolicy(filePath string) (*Policy, error) { return nil, fmt.Errorf("policy file %s missing required field 'allowed_image_prefixes'", filePath) } + // Prefixes are compared in normalised form (#84). + for i, prefix := range p.AllowedImagePrefixes { + normalized, err := NormalizeImagePrefix(prefix) + if err != nil { + return nil, fmt.Errorf("policy file %s has an invalid allowed_image_prefixes entry %q", filePath, prefix) + } + p.AllowedImagePrefixes[i] = normalized + } + return &p, nil } @@ -83,11 +91,16 @@ func (m *Manager) Get(serviceName string) (*Policy, error) { return p, nil } +// GetByImage returns the policy whose prefixes match the reference's +// normalised repository (#84). An invalid reference matches nothing. func (m *Manager) GetByImage(imageRef string) (*Policy, error) { - imageName := extractImageName(imageRef) + ref, err := ParseImageRef(imageRef) + if err != nil { + return nil, err + } for _, p := range m.policiesByName { - if matchesImagePrefix(imageName, p.AllowedImagePrefixes) { + if MatchesImagePrefix(ref.Name, p.AllowedImagePrefixes) { return p, nil } } @@ -95,17 +108,6 @@ func (m *Manager) GetByImage(imageRef string) (*Policy, error) { return nil, fmt.Errorf("no policy found for image: %s", imageRef) } -// matchesImagePrefix matches a full image name against a prefix allowing only -// namespace boundaries ("prefix" or "prefix/..."), never a bare substring. -func matchesImagePrefix(imageName string, prefixes []string) bool { - for _, prefix := range prefixes { - if imageName == prefix || strings.HasPrefix(imageName, prefix+"/") { - return true - } - } - return false -} - func (m *Manager) List() []string { var names []string for name := range m.policiesByName { @@ -113,11 +115,3 @@ func (m *Manager) List() []string { } return names } - -func extractImageName(imageRef string) string { - idx := strings.IndexAny(imageRef, ":@") - if idx == -1 { - return imageRef - } - return imageRef[:idx] -} diff --git a/go/internal/policy/manager_test.go b/go/internal/policy/manager_test.go index a9b2bc8..46b08b4 100644 --- a/go/internal/policy/manager_test.go +++ b/go/internal/policy/manager_test.go @@ -3,6 +3,7 @@ package policy import ( "os" "path/filepath" + "strings" "testing" ) @@ -206,21 +207,82 @@ allowed_image_prefixes: } } -func TestExtractImageName(t *testing.T) { +// TestParseImageRef is the #84 normalisation table, on the parser itself. +// The same rows run in Rust and TypeScript. +func TestParseImageRef(t *testing.T) { + const digest = "sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789" tests := []struct { - ref string - expected string + in string + want ImageRef + invalid bool }{ - {"chainsafe/lodestar:next", "chainsafe/lodestar"}, - {"chainsafe/lodestar@sha256:abc123", "chainsafe/lodestar"}, - {"ubuntu", "ubuntu"}, - {"registry.example.com/image:v1.0", "registry.example.com/image"}, + {in: "alpine", want: ImageRef{Name: "alpine"}}, + {in: "library/alpine", want: ImageRef{Name: "alpine"}}, + {in: "docker.io/library/alpine", want: ImageRef{Name: "alpine"}}, + {in: "index.docker.io/library/alpine:3.19", want: ImageRef{Name: "alpine", Tag: "3.19"}}, + {in: "registry-1.docker.io/library/alpine", want: ImageRef{Name: "alpine"}}, + {in: "docker.io/chainsafe/lodestar:v1", want: ImageRef{Name: "chainsafe/lodestar", Tag: "v1"}}, + {in: "ghcr.io/chainsafe/lodestar@" + digest, want: ImageRef{Name: "ghcr.io/chainsafe/lodestar", Digest: digest}}, + {in: "localhost:5000/x/y:2", want: ImageRef{Name: "localhost:5000/x/y", Tag: "2"}}, + {in: "registry.example.com:443/a", want: ImageRef{Name: "registry.example.com:443/a"}}, + {in: "REGISTRY.Example.com:443/a:V1", want: ImageRef{Name: "registry.example.com:443/a", Tag: "V1"}}, + {in: "docker.io/library/a/b", want: ImageRef{Name: "library/a/b"}}, + {in: "ghcr.io/library/alpine", want: ImageRef{Name: "ghcr.io/library/alpine"}}, + {in: "localhost/x", want: ImageRef{Name: "localhost/x"}}, + {in: "localhost", want: ImageRef{Name: "localhost"}}, + {in: "alpine:3.19@" + digest, want: ImageRef{Name: "alpine", Tag: "3.19", Digest: digest}}, + {in: "Alpine", invalid: true}, + {in: "chainsafe/Lodestar", invalid: true}, + {in: "alpine:", invalid: true}, + {in: "alpine@", invalid: true}, + {in: "", invalid: true}, + {in: "a//b", invalid: true}, + {in: "/alpine", invalid: true}, + {in: "alpine/", invalid: true}, + {in: "-alpine", invalid: true}, + {in: "alpine:a/b", invalid: true}, + {in: "a:b/c", invalid: true}, + {in: "alpine:t$g", invalid: true}, } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + got, err := ParseImageRef(tt.in) + if tt.invalid { + want := `invalid image reference "` + tt.in + `"` + if err == nil || err.Error() != want { + t.Errorf("ParseImageRef(%q) = %+v, %v; want error %q", tt.in, got, err, want) + } + return + } + if err != nil || got != tt.want { + t.Errorf("ParseImageRef(%q) = %+v, %v; want %+v", tt.in, got, err, tt.want) + } + }) + } +} - for _, tc := range tests { - got := extractImageName(tc.ref) - if got != tc.expected { - t.Errorf("extractImageName(%q) = %q, want %q", tc.ref, got, tc.expected) +// TestManagerNormalizesPrefixes checks that prefixes are normalised at load +// time, and that a prefix with a tag or an invalid one fails the load (#84). +func TestManagerNormalizesPrefixes(t *testing.T) { + dir := t.TempDir() + write := func(content string) { + if err := os.WriteFile(filepath.Join(dir, "a.yaml"), []byte(content), 0644); err != nil { + t.Fatal(err) + } + } + write("service_name: a\nallowed_image_prefixes:\n - docker.io/library/alpine\n - GHCR.io/team\n") + m, err := NewManager(dir) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + p, _ := m.Get("a") + if got := strings.Join(p.AllowedImagePrefixes, ","); got != "alpine,ghcr.io/team" { + t.Errorf("prefixes = %s, want alpine,ghcr.io/team", got) + } + for _, bad := range []string{"alpine:3.19", "Alpine"} { + write("service_name: a\nallowed_image_prefixes:\n - \"" + bad + "\"\n") + if _, err := NewManager(dir); err == nil || !strings.Contains(err.Error(), "invalid allowed_image_prefixes entry") { + t.Errorf("%s: NewManager error = %v, want an invalid entry error", bad, err) } } } diff --git a/go/internal/proxy/handler.go b/go/internal/proxy/handler.go index 15d0ea2..8aa7c91 100644 --- a/go/internal/proxy/handler.go +++ b/go/internal/proxy/handler.go @@ -72,8 +72,13 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } // Route on the raw path: r.URL.Path is already decoded and would hide %-escapes (#53). + // The query goes along for the image pull, which reads it (#83). path := r.URL.EscapedPath() - route := h.router.Route(r.Method, path, bodyJSON) + target := path + if r.URL.RawQuery != "" { + target += "?" + r.URL.RawQuery + } + route := h.router.RouteRequest(r.Method, target, len(body) > 0, bodyJSON) extra := map[string]interface{}{ "method": r.Method, diff --git a/go/internal/proxy/handler_test.go b/go/internal/proxy/handler_test.go index db12cd9..42e413c 100644 --- a/go/internal/proxy/handler_test.go +++ b/go/internal/proxy/handler_test.go @@ -162,6 +162,46 @@ func TestHandler_ForwardsPercentEncodedQuery(t *testing.T) { // #55: the audit uri is the raw request target, query and escapes included, // for allowed and denied requests alike. +// TestHandler_PullDeniesBody checks that a pull with a body is never +// forwarded (#83). The daemon reads a form body's fromImage over the query's, +// so an allowed image in the query and another in the body would pull the +// body's. Any body is denied, whatever its Content-Type or framing. +func TestHandler_PullDeniesBody(t *testing.T) { + files := map[string]string{"probe.yaml": "service_name: probe\nallowed_image_prefixes:\n - alpine\n"} + const target = "/v1.56/images/create?fromImage=alpine&tag=3.19" + tests := []struct { + name string + contentType string + body string + chunked bool + }{ + {"form body", "application/x-www-form-urlencoded", "fromImage=busybox&tag=latest", false}, + {"chunked form body", "application/x-www-form-urlencoded", "fromImage=busybox&tag=latest", true}, + {"JSON body", "application/json", `{"fromImage":"alpine"}`, false}, + {"text body", "text/plain", "x", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := &recorderTransport{} + h := newTestHandler(t, files, rec) + req := httptest.NewRequest("POST", target, strings.NewReader(tt.body)) + req.Header.Set("Content-Type", tt.contentType) + if tt.chunked { + req.ContentLength = -1 + req.TransferEncoding = []string{"chunked"} + } + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusForbidden || rec.lastRequest != nil { + t.Fatalf("status %d, forwarded %v, want 403 and no forward", w.Code, rec.lastRequest != nil) + } + if got := strings.TrimSpace(w.Body.String()); got != "image pull must not have a request body" { + t.Errorf("deny body = %q", got) + } + }) + } +} + func TestHandlerAuditURIIsRequestTarget(t *testing.T) { dir := t.TempDir() os.WriteFile(filepath.Join(dir, "default.yaml"), []byte("service_name: default\nallowed_image_prefixes:\n - scratch\n"), 0644) @@ -528,3 +568,34 @@ func TestHandlerRefusesConnect(t *testing.T) { t.Fatalf("CONNECT mismatches:\n%s", strings.Join(mismatches, "\n")) } } + +// TestHandler_PullRoutesOnQuery checks that the handler hands the query to +// the router (#83): a pull sent the Docker CLI way, empty body and image in +// the query, is forwarded with its query unchanged, and a disallowed one is +// not forwarded. +func TestHandler_PullRoutesOnQuery(t *testing.T) { + files := map[string]string{"probe.yaml": "service_name: probe\nallowed_image_prefixes:\n - alpine\n"} + const query = "fromImage=docker.io%2Flibrary%2Falpine&tag=3.19" + + rec := &recorderTransport{} + h := newTestHandler(t, files, rec) + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("POST", "/v1.56/images/create?"+query, nil)) + if w.Code != http.StatusOK || rec.lastRequest == nil { + t.Fatalf("allowed pull: status %d, forwarded %v: %s", w.Code, rec.lastRequest != nil, w.Body.String()) + } + if rec.lastRequest.URL.RawQuery != query { + t.Errorf("forwarded query = %q, want %q", rec.lastRequest.URL.RawQuery, query) + } + + rec = &recorderTransport{} + h = newTestHandler(t, files, rec) + w = httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("POST", "/v1.56/images/create?fromImage=busybox&tag=latest", nil)) + if w.Code != http.StatusForbidden || rec.lastRequest != nil { + t.Fatalf("disallowed pull: status %d, forwarded %v", w.Code, rec.lastRequest != nil) + } + if !strings.Contains(w.Body.String(), "image busybox not allowed by any policy") { + t.Errorf("deny body = %q", w.Body.String()) + } +} diff --git a/go/internal/proxy/router.go b/go/internal/proxy/router.go index 506888e..fbeb3a9 100644 --- a/go/internal/proxy/router.go +++ b/go/internal/proxy/router.go @@ -2,6 +2,7 @@ package proxy import ( "fmt" + "net/url" "slices" "strings" @@ -41,12 +42,25 @@ func (r *Router) WithReadonly(readonly bool) *Router { return r } -func (r *Router) Route(method, path string, body map[string]interface{}) *RouteResult { +// Route routes a request whose body, if any, is the parsed JSON body. It is +// RouteRequest with hasBody set when body is non-nil. +func (r *Router) Route(method, target string, body map[string]interface{}) *RouteResult { + return r.RouteRequest(method, target, body != nil, body) +} + +// RouteRequest routes a request. target is the raw request target: the path, +// still percent-encoded, and the query string, if any. The path checks read +// the path only; the image pull reads the query (#83). hasBody reports a +// non-empty raw body of any Content-Type; body is that body parsed as JSON, +// or nil. +func (r *Router) RouteRequest(method, target string, hasBody bool, body map[string]interface{}) *RouteResult { // Read-only mode denies every write, for every path (#75). if r.readonly && method != "GET" && method != "HEAD" { return &RouteResult{Action: ActionDeny, DenyMsg: fmt.Sprintf("read-only mode: %s requests are not allowed", method)} } + path, rawQuery, _ := strings.Cut(target, "?") + // The daemon decodes the path before routing, so deny any escape (#53). if strings.Contains(path, "%") { return &RouteResult{Action: ActionDeny, DenyMsg: "percent-encoded path not allowed"} @@ -118,7 +132,7 @@ func (r *Router) Route(method, path string, body map[string]interface{}) *RouteR } if path == "/images/create" && method == "POST" { - return r.routeImagePull(body) + return r.routeImagePull(rawQuery, hasBody) } if method == "GET" || method == "HEAD" { @@ -140,6 +154,9 @@ func (r *Router) routeCreate(body map[string]interface{}) *RouteResult { if image == "" { return &RouteResult{Action: ActionDeny, DenyMsg: "image field is required"} } + if _, err := policy.ParseImageRef(image); err != nil { + return &RouteResult{Action: ActionDeny, DenyMsg: err.Error()} + } p, err := r.manager.GetByImage(image) if err != nil { @@ -169,21 +186,66 @@ func (r *Router) routeByContainerName(containerName, action string) *RouteResult } } -func (r *Router) routeImagePull(body map[string]interface{}) *RouteResult { - if len(body) == 0 { - return &RouteResult{Action: ActionDeny, DenyMsg: "empty request body"} +// routeImagePull routes an image pull (#83). The Docker CLI sends fromImage +// and tag in the query string and no body. The image then gets the create checks: a policy prefix on the +// normalised repository (#84), and that policy's tag and digest rules. +func (r *Router) routeImagePull(rawQuery string, hasBody bool) *RouteResult { + deny := func(msg string) *RouteResult { return &RouteResult{Action: ActionDeny, DenyMsg: msg} } + + // The daemon reads a form body's fromImage over the query's, so a body + // could name any image: a pull has no body, whatever its Content-Type. + if hasBody { + return deny("image pull must not have a request body") } - fromImage, _ := body["fromImage"].(string) + // The daemon parses the query with url.ParseQuery too, and rejects it on error. + q, err := url.ParseQuery(rawQuery) + if err != nil { + return deny("invalid query string") + } + for _, key := range []string{"fromImage", "tag", "fromSrc"} { + if len(q[key]) > 1 { + return deny(fmt.Sprintf("duplicate %s query parameter", key)) + } + } + if q.Has("fromSrc") { + return deny("image import (fromSrc) is not allowed") + } + fromImage := q.Get("fromImage") if fromImage == "" { - return &RouteResult{Action: ActionDeny, DenyMsg: "fromImage field is required for image pull"} + return deny("fromImage query parameter is required for image pull") + } + ref, err := policy.ParseImageRef(fromImage) + if err != nil { + return deny(err.Error()) + } + if tag := q.Get("tag"); tag != "" { + if ref.Tag != "" || ref.Digest != "" { + return deny(fmt.Sprintf("fromImage %s already has a tag or digest; the tag query parameter is ambiguous", fromImage)) + } + // The CLI sends a digest in tag; a tag never contains ':'. + sep := ":" + if strings.Contains(tag, ":") { + sep = "@" + } + if ref, err = policy.ParseImageRef(fromImage + sep + tag); err != nil { + return deny(err.Error()) + } + } + // With neither, the daemon pulls every tag of the repository. + if ref.Tag == "" && ref.Digest == "" { + return deny("image pull without a tag or digest pulls every tag and is not allowed") } - if _, err := r.manager.GetByImage(fromImage); err != nil { - return &RouteResult{Action: ActionDeny, DenyMsg: fmt.Sprintf("image %s not allowed by any policy", fromImage)} + p, err := r.manager.GetByImage(ref.Name) + if err != nil { + return deny(fmt.Sprintf("image %s not allowed by any policy", fromImage)) + } + if err := policy.CheckImageTag(p, ref); err != nil { + return deny(err.Error()) } - return &RouteResult{Action: ActionAllow, Image: fromImage} + return &RouteResult{Action: ActionAllow, Service: p.ServiceName, Image: fromImage} } // stripAPIVersion strips one leading /v/ or /v./ (ASCII digits only), else returns path unchanged (#57). diff --git a/go/internal/proxy/router_test.go b/go/internal/proxy/router_test.go index 46bac74..6aba9f2 100644 --- a/go/internal/proxy/router_test.go +++ b/go/internal/proxy/router_test.go @@ -1,6 +1,7 @@ package proxy import ( + "net/url" "os" "path/filepath" "strings" @@ -236,44 +237,157 @@ func TestRouterStripAPIVersion(t *testing.T) { } } -func TestRouterImagePull(t *testing.T) { - m := newTestManager(t, map[string]string{ - "beacon.yaml": ` +// pullTestPolicies is the policy set for the #83 and #84 tables. beacon's +// prefixes are written in spellings that normalisation must reduce at load +// time: docker.io/chainsafe/lodestar is chainsafe/lodestar, and the registry +// host Registry.Example.com:443 is lower-cased. +var pullTestPolicies = map[string]string{ + "probe.yaml": ` +service_name: probe +allowed_image_prefixes: + - alpine +image_digest_allowed: true +`, + "beacon.yaml": ` service_name: beacon allowed_image_prefixes: - - chainsafe/lodestar + - docker.io/chainsafe/lodestar + - ghcr.io/chainsafe/lodestar + - localhost:5000/x + - Registry.Example.com:443/a +image_tag_pattern: '^(v[0-9]+|[0-9]+)$' `, - }) - r := NewRouter(m) +} - body := map[string]interface{}{ - "fromImage": "chainsafe/lodestar:next", - } - result := r.Route("POST", "/images/create", body) - if result.Action != ActionAllow { - t.Fatalf("expected ActionAllow, got %v: %s", result.Action, result.DenyMsg) +const testDigest = "sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789" + +// TestRoutePullQuery is the cross-language parity guard for #83 and #84. +// +// POST /images/create is routed on its query string, the way the Docker CLI +// sends it: fromImage (fully qualified, URL-encoded) and tag, with an empty +// body. The body is ignored, fromSrc (import) is denied, and the image gets +// the create checks: a policy prefix on the normalised repository, then that +// policy's tag pattern and digest rule. Rows mirror the pull* runs in +// spec/router.qnt; the rest are language-only. +func TestRoutePullQuery(t *testing.T) { + r := NewRouter(newTestManager(t, pullTestPolicies)) + alpineBody := map[string]interface{}{"fromImage": "alpine"} + notAllowed := func(image string) string { return "image " + image + " not allowed by any policy" } + required := "fromImage query parameter is required for image pull" + bodyMsg := "image pull must not have a request body" + fromSrc := "image import (fromSrc) is not allowed" + + tests := []struct { + path string + body map[string]interface{} + want Action + service string + wantMsg string + }{ + // pullCliAllowedTest (#83): the issue's reproduction, as the CLI sends it. + {"/v1.56/images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19", nil, ActionAllow, "probe", ""}, + // pullShortNameAllowedTest (#84) + {"/images/create?fromImage=alpine&tag=3.19", nil, ActionAllow, "probe", ""}, + // pullLibraryNameAllowedTest (#84) + {"/images/create?fromImage=library%2Falpine&tag=latest", nil, ActionAllow, "probe", ""}, + // pullCliOrgAllowedTest (#84): the prefix is written docker.io/chainsafe/lodestar. + {"/images/create?fromImage=docker.io%2Fchainsafe%2Flodestar&tag=v1", nil, ActionAllow, "beacon", ""}, + // #84: the tag in fromImage, on another Docker Hub host name. + {"/images/create?fromImage=index.docker.io%2Flibrary%2Falpine%3A3.19", nil, ActionAllow, "probe", ""}, + // #83: the CLI sends a digest in tag. + {"/images/create?fromImage=alpine&tag=" + url.QueryEscape(testDigest), nil, ActionAllow, "probe", ""}, + // #83: a digest in fromImage. + {"/images/create?fromImage=" + url.QueryEscape("alpine@"+testDigest), nil, ActionAllow, "probe", ""}, + // #83: image_digest_allowed is false for beacon. + {"/images/create?fromImage=ghcr.io%2Fchainsafe%2Flodestar&tag=" + url.QueryEscape(testDigest), nil, ActionDeny, "", "image digests not allowed for service beacon"}, + // #84: a registry port is not a tag. + {"/images/create?fromImage=localhost%3A5000%2Fx%2Fy&tag=2", nil, ActionAllow, "beacon", ""}, + // #84: the registry host is compared lower-cased. + {"/images/create?fromImage=REGISTRY.example.com%3A443%2Fa&tag=1", nil, ActionAllow, "beacon", ""}, + // #83: the tag pattern applies to pulls. + {"/images/create?fromImage=chainsafe%2Flodestar&tag=latest", nil, ActionDeny, "", `image tag "latest" does not match pattern "^(v[0-9]+|[0-9]+)$"`}, + // pullDisallowedCliDeniedTest (#83) + {"/images/create?fromImage=docker.io%2Fattacker%2Fmalware&tag=latest", nil, ActionDeny, "", notAllowed("docker.io/attacker/malware")}, + // pullOtherRegistryDeniedTest (#84): another registry is not Docker Hub's alpine. + {"/images/create?fromImage=localhost%3A5000%2Falpine&tag=1", nil, ActionDeny, "", notAllowed("localhost:5000/alpine")}, + // #84: the prefix match stays on namespace boundaries. + {"/images/create?fromImage=alpine-evil&tag=1", nil, ActionDeny, "", notAllowed("alpine-evil")}, + // pullNoFromImageDeniedTest (#83) + {"/images/create", nil, ActionDeny, "", required}, + // #83: the old contract, fromImage in a JSON body: a pull has no body. + {"/images/create", map[string]interface{}{"fromImage": "alpine:3.19"}, ActionDeny, "", bodyMsg}, + // pullBodyIgnoredDeniedTest (#83): the daemon would pull the query's image. + {"/images/create?fromImage=attacker%2Fmalware&tag=latest", alpineBody, ActionDeny, "", bodyMsg}, + // pullJsonBodyDeniedTest (#83): any body is denied, even one that agrees. + {"/images/create?fromImage=alpine&tag=3.19", alpineBody, ActionDeny, "", bodyMsg}, + // pullImportDeniedTest (#83): the daemon would import the tarball as alpine:latest. + {"/images/create?fromSrc=http%3A%2F%2Fexample.com%2Fx.tar&repo=alpine&tag=latest", nil, ActionDeny, "", fromSrc}, + // pullImportWithImageDeniedTest (#83) + {"/images/create?fromImage=alpine&tag=latest&fromSrc=-", nil, ActionDeny, "", fromSrc}, + // #83: a tag in fromImage and in tag is ambiguous. + {"/images/create?fromImage=alpine%3A3.19&tag=3.20", nil, ActionDeny, "", "fromImage alpine:3.19 already has a tag or digest; the tag query parameter is ambiguous"}, + // #83: no tag and no digest pulls every tag. + {"/images/create?fromImage=alpine", nil, ActionDeny, "", "image pull without a tag or digest pulls every tag and is not allowed"}, + // #84: upper case in the repository path is invalid. + {"/images/create?fromImage=Alpine&tag=latest", nil, ActionDeny, "", `invalid image reference "Alpine"`}, + // #83: a repeated parameter could be read two ways. + {"/images/create?fromImage=alpine&fromImage=attacker%2Fmalware&tag=1", nil, ActionDeny, "", "duplicate fromImage query parameter"}, + // #83: the daemon rejects a malformed query; so does the proxy. + {"/images/create?fromImage=alpine%zz&tag=1", nil, ActionDeny, "", "invalid query string"}, + // #53 still holds: the percent check reads the path, not the query. + {"/images/%63reate?fromImage=alpine&tag=1", nil, ActionDeny, "", "percent-encoded path not allowed"}, } - if result.Image != "chainsafe/lodestar:next" { - t.Fatalf("expected image 'chainsafe/lodestar:next', got '%s'", result.Image) + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + got := r.Route("POST", tt.path, tt.body) + if got.Action != tt.want || got.DenyMsg != tt.wantMsg || got.Service != tt.service { + t.Errorf("Route(POST, %s) = %v/%q/%q, want %v/%q/%q", + tt.path, got.Action, got.Service, got.DenyMsg, tt.want, tt.service, tt.wantMsg) + } + }) } } -func TestRouterImagePullDenied(t *testing.T) { - m := newTestManager(t, map[string]string{ - "beacon.yaml": ` -service_name: beacon -allowed_image_prefixes: - - chainsafe/lodestar -`, - }) - r := NewRouter(m) +// TestRouteImageNormalization is the normalisation table for #84, on the +// create route: each spelling selects the policy its normalised repository +// matches, and a path with upper case is invalid. The same table runs in +// Rust and TypeScript. +func TestRouteImageNormalization(t *testing.T) { + r := NewRouter(newTestManager(t, pullTestPolicies)) + notAllowed := func(image string) string { return "image " + image + " not allowed by any policy" } - body := map[string]interface{}{ - "fromImage": "ubuntu:latest", + tests := []struct { + image string + want Action + service string + wantMsg string + }{ + {"alpine", ActionCreateContainer, "probe", ""}, + {"library/alpine", ActionCreateContainer, "probe", ""}, + {"docker.io/library/alpine", ActionCreateContainer, "probe", ""}, + {"docker.io/alpine", ActionCreateContainer, "probe", ""}, + {"index.docker.io/library/alpine:3.19", ActionCreateContainer, "probe", ""}, + {"docker.io/chainsafe/lodestar:v1", ActionCreateContainer, "beacon", ""}, + {"chainsafe/lodestar:v1", ActionCreateContainer, "beacon", ""}, + {"registry-1.docker.io/chainsafe/lodestar:v1", ActionCreateContainer, "beacon", ""}, + {"ghcr.io/chainsafe/lodestar@" + testDigest, ActionCreateContainer, "beacon", ""}, + {"localhost:5000/x/y:2", ActionCreateContainer, "beacon", ""}, + {"registry.example.com:443/a", ActionCreateContainer, "beacon", ""}, + {"Alpine:latest", ActionDeny, "", `invalid image reference "Alpine:latest"`}, + {"chainsafe/Lodestar:v1", ActionDeny, "", `invalid image reference "chainsafe/Lodestar:v1"`}, + {"alpine-evil", ActionDeny, "", notAllowed("alpine-evil")}, + {"ghcr.io/library/alpine", ActionDeny, "", notAllowed("ghcr.io/library/alpine")}, + {"ghcr.io/chainsafe/lodestar-evil", ActionDeny, "", notAllowed("ghcr.io/chainsafe/lodestar-evil")}, + {"localhost:5000/xy:1", ActionDeny, "", notAllowed("localhost:5000/xy:1")}, } - result := r.Route("POST", "/images/create", body) - if result.Action != ActionDeny { - t.Fatalf("expected ActionDeny, got %v", result.Action) + for _, tt := range tests { + t.Run(tt.image, func(t *testing.T) { + got := r.Route("POST", "/containers/create", map[string]interface{}{"Image": tt.image}) + if got.Action != tt.want || got.DenyMsg != tt.wantMsg || got.Service != tt.service { + t.Errorf("create %s = %v/%q/%q, want %v/%q/%q", + tt.image, got.Action, got.Service, got.DenyMsg, tt.want, tt.service, tt.wantMsg) + } + }) } } @@ -552,7 +666,6 @@ allowed_image_prefixes: }) r := NewRouter(m) createBody := map[string]interface{}{"Image": "chainsafe/lodestar:next"} - pullBody := map[string]interface{}{"fromImage": "chainsafe/lodestar:next"} tests := []struct { method string @@ -580,7 +693,7 @@ allowed_image_prefixes: // Exec is matched under containers or exec only (#49, language-only). {"GET", "/images/exec", nil, ActionAllow, ""}, // An allowed image, so only the path decides (#49, language-only). - {"POST", "/images/create/extra", pullBody, ActionDeny, ""}, + {"POST", "/images/create/extra?fromImage=chainsafe/lodestar&tag=next", nil, ActionDeny, ""}, // A name ending in exec is a plain name (#49, language-only). {"GET", "/containers/myexec/json", nil, ActionAllow, ""}, // A name starting with exec is a plain name (#49, language-only). @@ -697,7 +810,6 @@ allowed_image_prefixes: r := NewRouter(m).WithReadonly(true) post := "read-only mode: POST requests are not allowed" createBody := map[string]interface{}{"Image": "chainsafe/lodestar:latest"} - pullBody := map[string]interface{}{"fromImage": "chainsafe/lodestar", "tag": "latest"} tests := []struct { method string @@ -719,7 +831,7 @@ allowed_image_prefixes: // readonlyDeleteDeniedTest (#75) {"DELETE", "/containers/mycontainer", nil, ActionDeny, "read-only mode: DELETE requests are not allowed"}, // #75: an image pull of an allowed image. - {"POST", "/images/create", pullBody, ActionDeny, post}, + {"POST", "/images/create?fromImage=chainsafe%2Flodestar&tag=latest", nil, ActionDeny, post}, // readonlyCreateDeniedTest (#75): an allowed image, so only the flag decides. {"POST", "/containers/create", createBody, ActionDeny, post}, // #75: methods outside the spec's universe. diff --git a/rs/src/handler.rs b/rs/src/handler.rs index 7e9eba2..f6ce688 100644 --- a/rs/src/handler.rs +++ b/rs/src/handler.rs @@ -87,7 +87,11 @@ impl Handler { None }; - let route = self.router.route(&method, &path, body_json.as_ref()); + // Route on the raw target: the path still percent-encoded (#53), and the + // query, which the image pull reads (#83). + let route = self + .router + .route_request(&method, &target, !body_bytes.is_empty(), body_json.as_ref()); if route.action == Action::Deny { let msg = route.deny_msg.unwrap_or_default(); @@ -516,6 +520,50 @@ mod tests { assert_eq!(uri.query(), Some(query)); } + /// #83: a pull with a body is never forwarded. The daemon reads a form + /// body's fromImage over the query's, so an allowed image in the query + /// and another in the body would pull the body's. Any body is denied, + /// whatever its Content-Type. + #[tokio::test] + async fn test_handler_pull_denies_body() { + let cases = [ + ("application/x-www-form-urlencoded", "fromImage=busybox&tag=latest"), + ("application/json", r#"{"fromImage":"alpine"}"#), + ("text/plain", "x"), + ]; + for (content_type, body) in cases { + let policy: crate::policy::Policy = + serde_yaml::from_str("service_name: probe\nallowed_image_prefixes:\n - alpine\n").unwrap(); + let mut policies = std::collections::HashMap::new(); + policies.insert("probe".to_string(), policy); + let router = Arc::new(Router::new(Manager::from_map(policies))); + let captured_uri = Arc::new(std::sync::Mutex::new(None)); + let transport = UriRecordingTransport { + captured_uri: captured_uri.clone(), + }; + let audit = AuditLogger::new("/dev/null").unwrap(); + let handler = Handler::new(router, Chain::new(), audit, Box::new(transport)); + let req = Request::post("http://localhost/v1.56/images/create?fromImage=alpine&tag=3.19") + .header("content-type", content_type) + .body(Full::new(Bytes::from(body))) + .unwrap(); + let resp = handler.handle(req).await; + assert_eq!(resp.status(), StatusCode::FORBIDDEN, "{}", content_type); + let got = resp.into_body().collect().await.unwrap().to_bytes(); + assert_eq!( + String::from_utf8_lossy(&got), + "image pull must not have a request body", + "{}", + content_type + ); + assert!( + captured_uri.lock().unwrap().is_none(), + "{}: expected no forward", + content_type + ); + } + } + /// #55: the audit uri is the raw request target, query and escapes /// included, for allowed and denied requests alike. #[tokio::test] @@ -710,4 +758,51 @@ mod tests { } assert!(mismatches.is_empty(), "mismatches:\n{}", mismatches.join("\n")); } + + /// #83: the handler hands the query to the router. A pull sent the Docker + /// CLI way, empty body and image in the query, is forwarded with its + /// query unchanged, and a disallowed one is not forwarded. + #[tokio::test] + async fn test_handler_pull_routes_on_query() { + let make = || { + let policy: crate::policy::Policy = + serde_yaml::from_str("service_name: probe\nallowed_image_prefixes:\n - alpine\n").unwrap(); + let mut policies = std::collections::HashMap::new(); + policies.insert("probe".to_string(), policy); + let router = Arc::new(Router::new(Manager::from_map(policies))); + let captured_uri = Arc::new(std::sync::Mutex::new(None)); + let transport = UriRecordingTransport { + captured_uri: captured_uri.clone(), + }; + let audit = AuditLogger::new("/dev/null").unwrap(); + ( + Handler::new(router, Chain::new(), audit, Box::new(transport)), + captured_uri, + ) + }; + + let (handler, captured_uri) = make(); + let query = "fromImage=docker.io%2Flibrary%2Falpine&tag=3.19"; + let req = Request::post(format!("http://localhost/v1.56/images/create?{}", query)) + .body(Full::new(Bytes::new())) + .unwrap(); + let resp = handler.handle(req).await; + assert_eq!(resp.status(), StatusCode::OK); + let captured = captured_uri.lock().unwrap(); + let uri = captured.as_ref().expect("expected the allowed pull to be forwarded"); + assert_eq!(uri.query(), Some(query)); + + let (handler, captured_uri) = make(); + let req = Request::post("http://localhost/v1.56/images/create?fromImage=busybox&tag=latest") + .body(Full::new(Bytes::new())) + .unwrap(); + let resp = handler.handle(req).await; + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let body = resp.into_body().collect().await.unwrap().to_bytes(); + assert_eq!( + String::from_utf8_lossy(&body), + "image busybox not allowed by any policy" + ); + assert!(captured_uri.lock().unwrap().is_none(), "expected no forward"); + } } diff --git a/rs/src/middleware.rs b/rs/src/middleware.rs index 9bbefec..ffccf36 100644 --- a/rs/src/middleware.rs +++ b/rs/src/middleware.rs @@ -1,4 +1,4 @@ -use crate::policy::Policy; +use crate::policy::{check_image_tag, matches_image_prefix, parse_image_ref, Policy}; use std::collections::HashMap; #[cfg(test)] @@ -98,44 +98,12 @@ impl Gate for RegistryGate { _ => return Ok(()), }; - let (image_name, tag_or_digest) = if let Some(idx) = image.find(|c: char| c == ':' || c == '@') { - (&image[..idx], &image[idx + 1..]) - } else { - (image, "") - }; - - let allowed = policy - .allowed_image_prefixes - .iter() - .any(|prefix| image_name == prefix || image_name.starts_with(&format!("{}/", prefix))); - if !allowed { - return Err(format!("image {:?} is not in allowed prefixes", image_name)); - } - - if !tag_or_digest.is_empty() { - if tag_or_digest.len() > 128 { - return Err("image tag/digest exceeds 128 characters".into()); - } - - if tag_or_digest.starts_with("sha256:") { - if !policy.image_digest_allowed { - return Err(format!("image digests not allowed for service {}", policy.service_name)); - } - let re = - regex::Regex::new(r"^sha256:[a-f0-9]{64}$").map_err(|e| format!("invalid digest regex: {}", e))?; - if !re.is_match(tag_or_digest) { - return Err(format!("invalid digest format: {}", tag_or_digest)); - } - } else if let Some(ref pattern) = policy.image_tag_pattern { - let re = regex::Regex::new(pattern).map_err(|e| format!("invalid tag pattern regex: {}", e))?; - if !re.is_match(tag_or_digest) { - return Err(format!( - "image tag {:?} does not match pattern {:?}", - tag_or_digest, pattern - )); - } - } + // The same parser as the router, so a registry port is not a tag (#84). + let image_ref = parse_image_ref(image)?; + if !matches_image_prefix(&image_ref.name, &policy.allowed_image_prefixes) { + return Err(format!("image {:?} is not in allowed prefixes", image_ref.name)); } + check_image_tag(policy, &image_ref)?; Ok(()) } @@ -1167,4 +1135,59 @@ mod tests { mismatches.join("\n") ); } + + /// Pins the registry gate's reference parser (#84): a registry port is + /// not a tag, Docker Hub spellings match the normalised prefix, and the + /// tag pattern sees only the tag. The policy's prefixes are normalised, + /// as the manager stores them. Same rows in Go and TypeScript. + #[test] + fn test_registry_gate_parses_references() { + let gate = RegistryGate; + let mut policy = make_policy( + "test", + vec![ + "chainsafe/lodestar", + "localhost:5000/team", + "registry.example.com:443/a", + ], + ); + policy.image_tag_pattern = Some("^(v[0-9]+|[0-9.]+)$".into()); + let cases = [ + ("localhost:5000/team/app:1.2", None), + ("localhost:5000/team/app", None), + ("docker.io/chainsafe/lodestar:v1", None), + ("registry.example.com:443/a", None), + ("Registry.Example.com:443/a:2", None), + ( + "localhost:5000/team/app:latest", + Some(r#"image tag "latest" does not match pattern "^(v[0-9]+|[0-9.]+)$""#), + ), + ( + "localhost:5000/other:1", + Some(r#"image "localhost:5000/other" is not in allowed prefixes"#), + ), + ( + "chainsafe/Lodestar:v1", + Some(r#"invalid image reference "chainsafe/Lodestar:v1""#), + ), + ]; + let mut mismatches = Vec::new(); + for (image, want_err) in cases { + let body = body_from_json(serde_json::json!({ "Image": image })); + let got = gate.check("POST", "/containers/create", &policy, &body); + let ok = match (&got, want_err) { + (Ok(()), None) => true, + (Err(e), Some(want)) => e.contains(want), + _ => false, + }; + if !ok { + mismatches.push(format!("{}: got {:?}, want {:?}", image, got, want_err)); + } + } + assert!( + mismatches.is_empty(), + "registry gate mismatches:\n{}", + mismatches.join("\n") + ); + } } diff --git a/rs/src/policy.rs b/rs/src/policy.rs index 7217148..8e42e87 100644 --- a/rs/src/policy.rs +++ b/rs/src/policy.rs @@ -1,5 +1,6 @@ use serde::Deserialize; use std::collections::HashMap; +use std::sync::OnceLock; #[derive(Debug, Clone, Deserialize)] pub struct Policy { @@ -81,6 +82,20 @@ impl Manager { if policy.allowed_image_prefixes.is_empty() { return Err("missing allowed_image_prefixes".into()); } + // Prefixes are compared in normalised form (#84). + let mut policy = policy; + let mut prefixes = Vec::with_capacity(policy.allowed_image_prefixes.len()); + for prefix in &policy.allowed_image_prefixes { + let normalized = normalize_image_prefix(prefix).map_err(|_| { + format!( + "policy file {} has an invalid allowed_image_prefixes entry \"{}\"", + path.display(), + prefix + ) + })?; + prefixes.push(normalized); + } + policy.allowed_image_prefixes = prefixes; policies_by_name.insert(policy.service_name.clone(), policy); } @@ -91,13 +106,13 @@ impl Manager { self.policies_by_name.get(name) } + /// The policy whose prefixes match the reference's normalised repository + /// (#84). An invalid reference matches nothing. pub fn get_by_image(&self, image_ref: &str) -> Option<&Policy> { - let image_name = extract_image_name(image_ref); - self.policies_by_name.values().find(|p| { - p.allowed_image_prefixes - .iter() - .any(|prefix| image_name == *prefix || image_name.starts_with(&format!("{}/", prefix))) - }) + let name = parse_image_ref(image_ref).ok()?.name; + self.policies_by_name + .values() + .find(|p| matches_image_prefix(&name, &p.allowed_image_prefixes)) } pub fn list(&self) -> Vec<&str> { @@ -105,12 +120,151 @@ impl Manager { } } -pub(crate) fn extract_image_name(image_ref: &str) -> &str { - if let Some(idx) = image_ref.find(|c: char| c == ':' || c == '@') { - &image_ref[..idx] - } else { - image_ref +/// A parsed image reference (#84). `name` is the normalised repository: the +/// registry host, lower-cased, and the path; Docker Hub's host is dropped, +/// and so is `library/` before a single-component name, so `alpine`, +/// `library/alpine` and `docker.io/library/alpine` are all `alpine`. `tag` +/// and `digest` are empty when absent. +#[derive(Debug, Clone, PartialEq)] +pub struct ImageRef { + pub name: String, + pub tag: String, + pub digest: String, +} + +const DEFAULT_REGISTRIES: [&str; 3] = ["docker.io", "index.docker.io", "registry-1.docker.io"]; + +fn domain_re() -> &'static regex::Regex { + static RE: OnceLock = OnceLock::new(); + RE.get_or_init(|| { + regex::Regex::new( + r"^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?(?:\.[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)*(?::[0-9]+)?$", + ) + .unwrap() + }) +} + +fn path_component_re() -> &'static regex::Regex { + static RE: OnceLock = OnceLock::new(); + RE.get_or_init(|| regex::Regex::new(r"^[a-z0-9]+(?:(?:[._]|__|-+)[a-z0-9]+)*$").unwrap()) +} + +fn tag_re() -> &'static regex::Regex { + static RE: OnceLock = OnceLock::new(); + RE.get_or_init(|| regex::Regex::new(r"^[A-Za-z0-9_][A-Za-z0-9_.-]*$").unwrap()) +} + +fn sha256_digest_re() -> &'static regex::Regex { + static RE: OnceLock = OnceLock::new(); + RE.get_or_init(|| regex::Regex::new(r"^sha256:[a-f0-9]{64}$").unwrap()) +} + +/// Parses `[registry/]repository[:tag][@digest]` the way Docker does (#84). +/// The digest follows the first `@`; the tag follows the last `:` after the +/// last `/`, so a registry port is not a tag. The first path component is a +/// registry host when it contains `.` or `:` or is `localhost`. Only the +/// host is lower-cased: upper case in the path is invalid. The digest is +/// checked by `check_image_tag`, not here. +pub fn parse_image_ref(reference: &str) -> Result { + let invalid = || format!("invalid image reference \"{}\"", reference); + let (rest, digest) = match reference.split_once('@') { + Some((_, "")) => return Err(invalid()), + Some((r, d)) => (r, d), + None => (reference, ""), + }; + let after_slash = rest.rfind('/').map_or(0, |i| i + 1); + let (name, tag) = match rest[after_slash..].rfind(':') { + Some(i) => { + let tag = &rest[after_slash + i + 1..]; + if !tag_re().is_match(tag) { + return Err(invalid()); + } + (&rest[..after_slash + i], tag) + } + None => (rest, ""), + }; + let name = normalize_name(name).ok_or_else(invalid)?; + Ok(ImageRef { + name, + tag: tag.to_string(), + digest: digest.to_string(), + }) +} + +fn normalize_name(name: &str) -> Option { + let (domain, path) = match name.split_once('/') { + Some((first, path)) if first.contains('.') || first.contains(':') || first == "localhost" => { + (Some(first.to_ascii_lowercase()), path) + } + _ => (None, name), + }; + if let Some(d) = &domain { + if !domain_re().is_match(d) { + return None; + } + } + if !path.split('/').all(|c| path_component_re().is_match(c)) { + return None; + } + let domain = domain.filter(|d| !DEFAULT_REGISTRIES.contains(&d.as_str())); + let full = match domain { + Some(d) => format!("{}/{}", d, path), + None => match path.strip_prefix("library/") { + Some(official) if !official.contains('/') => official.to_string(), + _ => path.to_string(), + }, + }; + if full.len() > 255 { + return None; + } + Some(full) +} + +/// Normalises a policy prefix at load time (#84). A prefix names a +/// repository or a namespace, so a tag or digest makes it invalid. +pub fn normalize_image_prefix(prefix: &str) -> Result { + let r = parse_image_ref(prefix)?; + if !r.tag.is_empty() || !r.digest.is_empty() { + return Err(format!("image prefix \"{}\" has a tag or digest", prefix)); + } + Ok(r.name) +} + +/// Matches a normalised repository against normalised prefixes on namespace +/// boundaries ("prefix" or "prefix/..."), never a bare substring. +pub fn matches_image_prefix(name: &str, prefixes: &[String]) -> bool { + prefixes + .iter() + .any(|p| name == p || (name.starts_with(p.as_str()) && name.as_bytes().get(p.len()) == Some(&b'/'))) +} + +/// Applies a policy's digest and tag rules to a parsed reference: the digest +/// must be allowed and well-formed, the tag must match `image_tag_pattern`. +/// Shared by the registry gate (create) and the router (pull, #83). +pub fn check_image_tag(policy: &Policy, r: &ImageRef) -> Result<(), String> { + if !r.digest.is_empty() { + if r.digest.len() > 128 { + return Err("image tag/digest exceeds 128 characters".into()); + } + if !policy.image_digest_allowed { + return Err(format!("image digests not allowed for service {}", policy.service_name)); + } + if !sha256_digest_re().is_match(&r.digest) { + return Err(format!("invalid digest format: {}", r.digest)); + } + } + if !r.tag.is_empty() { + if r.tag.len() > 128 { + return Err("image tag/digest exceeds 128 characters".into()); + } + if let Some(ref pattern) = policy.image_tag_pattern { + let re = regex::Regex::new(pattern).map_err(|e| format!("invalid tag pattern regex: {}", e))?; + if !re.is_match(&r.tag) { + return Err(format!("image tag {:?} does not match pattern {:?}", r.tag, pattern)); + } + } } + Ok(()) } #[cfg(test)] @@ -183,27 +337,95 @@ denied_flags: assert_eq!(vols[0].host_path, "/data"); } + /// The #84 normalisation table, on the parser itself. The same rows run + /// in Go and TypeScript. #[test] - fn test_extract_image_name_no_tag() { - assert_eq!(extract_image_name("myimage"), "myimage"); - } - - #[test] - fn test_extract_image_name_with_tag() { - assert_eq!(extract_image_name("myimage:latest"), "myimage"); - } - - #[test] - fn test_extract_image_name_with_digest() { - assert_eq!(extract_image_name("myimage@sha256:abc123"), "myimage"); + fn test_parse_image_ref_table() { + let digest = "sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + let ghcr = format!("ghcr.io/chainsafe/lodestar@{}", digest); + let both = format!("alpine:3.19@{}", digest); + let cases: Vec<(&str, Option<(&str, &str, &str)>)> = vec![ + ("alpine", Some(("alpine", "", ""))), + ("library/alpine", Some(("alpine", "", ""))), + ("docker.io/library/alpine", Some(("alpine", "", ""))), + ("index.docker.io/library/alpine:3.19", Some(("alpine", "3.19", ""))), + ("registry-1.docker.io/library/alpine", Some(("alpine", "", ""))), + ( + "docker.io/chainsafe/lodestar:v1", + Some(("chainsafe/lodestar", "v1", "")), + ), + (&ghcr, Some(("ghcr.io/chainsafe/lodestar", "", digest))), + ("localhost:5000/x/y:2", Some(("localhost:5000/x/y", "2", ""))), + ( + "registry.example.com:443/a", + Some(("registry.example.com:443/a", "", "")), + ), + ( + "REGISTRY.Example.com:443/a:V1", + Some(("registry.example.com:443/a", "V1", "")), + ), + ("docker.io/library/a/b", Some(("library/a/b", "", ""))), + ("ghcr.io/library/alpine", Some(("ghcr.io/library/alpine", "", ""))), + ("localhost/x", Some(("localhost/x", "", ""))), + ("localhost", Some(("localhost", "", ""))), + (&both, Some(("alpine", "3.19", digest))), + ("Alpine", None), + ("chainsafe/Lodestar", None), + ("alpine:", None), + ("alpine@", None), + ("", None), + ("a//b", None), + ("/alpine", None), + ("alpine/", None), + ("-alpine", None), + ("alpine:a/b", None), + ("a:b/c", None), + ("alpine:t$g", None), + ]; + let mut mismatches = Vec::new(); + for (input, want) in &cases { + let got = parse_image_ref(input); + let ok = match (&got, want) { + (Ok(r), Some((name, tag, digest))) => r.name == *name && r.tag == *tag && r.digest == *digest, + (Err(e), None) => *e == format!("invalid image reference \"{}\"", input), + _ => false, + }; + if !ok { + mismatches.push(format!("{:?}: got {:?}, want {:?}", input, got, want)); + } + } + assert!(mismatches.is_empty(), "parse mismatches:\n{}", mismatches.join("\n")); } + /// #84: prefixes are normalised at load time, and a prefix with a tag or + /// an invalid one fails the load. #[test] - fn test_extract_image_name_registry_path() { + fn test_manager_normalizes_prefixes() { + let dir = std::env::temp_dir().join(format!("dsp-prefix-{}", rand::random::())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("a.yaml"), + "service_name: a\nallowed_image_prefixes:\n - docker.io/library/alpine\n - GHCR.io/team\n", + ) + .unwrap(); + let mgr = Manager::new(dir.to_str().unwrap()).unwrap(); assert_eq!( - extract_image_name("registry.example.com/myimage:tag"), - "registry.example.com/myimage" + mgr.get("a").unwrap().allowed_image_prefixes, + vec!["alpine", "ghcr.io/team"] ); + for bad in ["alpine:3.19", "Alpine"] { + std::fs::write( + dir.join("a.yaml"), + format!("service_name: a\nallowed_image_prefixes:\n - \"{}\"\n", bad), + ) + .unwrap(); + let err = Manager::new(dir.to_str().unwrap()) + .err() + .expect("load must fail") + .to_string(); + assert!(err.contains("invalid allowed_image_prefixes entry"), "{}: {}", bad, err); + } + std::fs::remove_dir_all(&dir).unwrap(); } #[test] diff --git a/rs/src/proxy.rs b/rs/src/proxy.rs index 5a42f19..5197406 100644 --- a/rs/src/proxy.rs +++ b/rs/src/proxy.rs @@ -1,4 +1,4 @@ -use crate::policy::{Manager, Policy}; +use crate::policy::{check_image_tag, parse_image_ref, Manager, Policy}; use std::collections::HashMap; #[derive(Debug, Clone, PartialEq)] @@ -38,12 +38,31 @@ impl Router { self } - pub fn route(&self, method: &str, path: &str, body: Option<&HashMap>) -> RouteResult { + /// Routes a request whose body, if any, is the parsed JSON body. It is + /// `route_request` with `has_body` set when `body` is `Some`. + pub fn route(&self, method: &str, target: &str, body: Option<&HashMap>) -> RouteResult { + self.route_request(method, target, body.is_some(), body) + } + + /// Routes a request. `target` is the raw request target: the path, still + /// percent-encoded, and the query string, if any. The path checks read + /// the path only; the image pull reads the query (#83). `has_body` + /// reports a non-empty raw body of any Content-Type; `body` is that body + /// parsed as JSON, or `None`. + pub fn route_request( + &self, + method: &str, + target: &str, + has_body: bool, + body: Option<&HashMap>, + ) -> RouteResult { // Read-only mode denies every write, for every path (#75). if self.readonly && method != "GET" && method != "HEAD" { return deny(&format!("read-only mode: {} requests are not allowed", method)); } + let (path, query) = target.split_once('?').unwrap_or((target, "")); + // The daemon decodes the path before routing, so deny any escape (#53). if path.contains('%') { return deny("percent-encoded path not allowed"); @@ -103,7 +122,7 @@ impl Router { // Image pull if method == "POST" && path == "/images/create" { - return self.route_image_pull(body); + return self.route_image_pull(query, has_body); } // Default GET/HEAD passthrough @@ -124,6 +143,9 @@ impl Router { if image.is_empty() { return deny("image field is required"); } + if let Err(msg) = parse_image_ref(image) { + return deny(&msg); + } match self.manager.get_by_image(image) { Some(policy) => RouteResult { @@ -162,30 +184,118 @@ impl Router { } } - fn route_image_pull(&self, body: Option<&HashMap>) -> RouteResult { - let body = match body { - Some(b) if !b.is_empty() => b, - _ => return deny("empty request body"), + /// Image pull (#83). The Docker CLI sends `fromImage` and `tag` in the + /// query string and no body. + /// The image then gets the create checks: a policy prefix on the + /// normalised repository (#84), and that policy's tag and digest rules. + fn route_image_pull(&self, raw_query: &str, has_body: bool) -> RouteResult { + // The daemon reads a form body's fromImage over the query's, so a + // body could name any image: a pull has no body, whatever its + // Content-Type. + if has_body { + return deny("image pull must not have a request body"); + } + let params = match parse_query(raw_query) { + Some(p) => p, + None => return deny("invalid query string"), }; - - let from_image = body.get("fromImage").and_then(|v| v.as_str()).unwrap_or(""); + for key in ["fromImage", "tag", "fromSrc"] { + if params.iter().filter(|(k, _)| k == key).count() > 1 { + return deny(&format!("duplicate {} query parameter", key)); + } + } + let get = |key: &str| params.iter().find(|(k, _)| k == key).map(|(_, v)| v.as_str()); + if get("fromSrc").is_some() { + return deny("image import (fromSrc) is not allowed"); + } + let from_image = get("fromImage").unwrap_or(""); if from_image.is_empty() { - return deny("fromImage field is required for image pull"); + return deny("fromImage query parameter is required for image pull"); + } + let mut image_ref = match parse_image_ref(from_image) { + Ok(r) => r, + Err(msg) => return deny(&msg), + }; + let tag = get("tag").unwrap_or(""); + if !tag.is_empty() { + if !image_ref.tag.is_empty() || !image_ref.digest.is_empty() { + return deny(&format!( + "fromImage {} already has a tag or digest; the tag query parameter is ambiguous", + from_image + )); + } + // The CLI sends a digest in tag; a tag never contains ':'. + let sep = if tag.contains(':') { '@' } else { ':' }; + image_ref = match parse_image_ref(&format!("{}{}{}", from_image, sep, tag)) { + Ok(r) => r, + Err(msg) => return deny(&msg), + }; + } + // With neither, the daemon pulls every tag of the repository. + if image_ref.tag.is_empty() && image_ref.digest.is_empty() { + return deny("image pull without a tag or digest pulls every tag and is not allowed"); + } + let policy = match self.manager.get_by_image(&image_ref.name) { + Some(p) => p, + None => return deny(&format!("image {} not allowed by any policy", from_image)), + }; + if let Err(msg) = check_image_tag(policy, &image_ref) { + return deny(&msg); } + RouteResult { + action: Action::Allow, + service: Some(policy.service_name.clone()), + policy: None, + body: None, + container: None, + image: Some(from_image.to_string()), + deny_msg: None, + } + } +} - match self.manager.get_by_image(from_image) { - Some(_) => RouteResult { - action: Action::Allow, - service: None, - policy: None, - body: None, - container: None, - image: Some(from_image.to_string()), - deny_msg: None, - }, - None => deny(&format!("image {} not allowed by any policy", from_image)), +/// Parses a query string as Go's `url.ParseQuery` does, which is what the +/// daemon uses: `&` separates pairs, `+` is a space, and a `;` or a bad +/// `%` escape fails the whole query. +fn parse_query(raw: &str) -> Option> { + let mut out = Vec::new(); + for part in raw.split('&') { + if part.contains(';') { + return None; } + if part.is_empty() { + continue; + } + let (k, v) = part.split_once('=').unwrap_or((part, "")); + out.push((query_unescape(k)?, query_unescape(v)?)); } + Some(out) +} + +fn query_unescape(s: &str) -> Option { + let hex = |b: u8| (b as char).to_digit(16).map(|d| d as u8); + let bytes = s.as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + match bytes[i] { + b'%' => { + let hi = hex(*bytes.get(i + 1)?)?; + let lo = hex(*bytes.get(i + 2)?)?; + out.push(hi << 4 | lo); + i += 3; + } + b'+' => { + out.push(b' '); + i += 1; + } + b => { + out.push(b); + i += 1; + } + } + } + Some(String::from_utf8_lossy(&out).into_owned()) } fn allow() -> RouteResult { @@ -715,8 +825,6 @@ mod tests { let router = Router::new(make_manager(vec!["alpine"])); let create_body: HashMap = serde_json::from_value(serde_json::json!({"Image": "alpine:latest"})).unwrap(); - let pull_body: HashMap = - serde_json::from_value(serde_json::json!({"fromImage": "alpine:latest"})).unwrap(); let exec = Some("exec is not allowed"); let cases = [ // execSubpathDeleteDeniedTest (#49) @@ -744,7 +852,13 @@ mod tests { // Exec is matched under containers or exec only (#49, language-only). ("GET", "/images/exec", None, Action::Allow, None), // An allowed image, so only the path decides (#49, language-only). - ("POST", "/images/create/extra", Some(&pull_body), Action::Deny, None), + ( + "POST", + "/images/create/extra?fromImage=alpine&tag=latest", + None, + Action::Deny, + None, + ), // A name ending in exec is a plain name (#49, language-only). ("GET", "/containers/myexec/json", None, Action::Allow, None), // A name starting with exec is a plain name (#49, language-only). @@ -844,8 +958,6 @@ mod tests { let post = Some("read-only mode: POST requests are not allowed"); let create_body: HashMap = serde_json::from_value(serde_json::json!({"Image": "chainsafe/lodestar:latest"})).unwrap(); - let pull_body: HashMap = - serde_json::from_value(serde_json::json!({"fromImage": "chainsafe/lodestar", "tag": "latest"})).unwrap(); let cases = [ // readonlyGetAllowedTest (#75) ("GET", "/containers/json", None, Action::Allow, None), @@ -866,7 +978,13 @@ mod tests { Some("read-only mode: DELETE requests are not allowed"), ), // #75: an image pull of an allowed image. - ("POST", "/images/create", Some(&pull_body), Action::Deny, post), + ( + "POST", + "/images/create?fromImage=chainsafe%2Flodestar&tag=latest", + None, + Action::Deny, + post, + ), // readonlyCreateDeniedTest (#75): an allowed image, so only the flag decides. ("POST", "/containers/create", Some(&create_body), Action::Deny, post), // #75: methods outside the spec's universe. @@ -1121,41 +1239,381 @@ mod tests { } } - #[test] - fn test_route_image_pull() { - let router = Router::new(make_manager(vec!["alpine"])); - let map: HashMap = - serde_json::from_value(serde_json::json!({"fromImage": "alpine"})).unwrap(); - let result = router.route("POST", "/images/create", Some(&map)); - assert_eq!(result.action, Action::Allow); - assert_eq!(result.image, Some("alpine".into())); + const TEST_DIGEST: &str = "sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + + /// The policy set for the #83 and #84 tables, loaded from YAML so that + /// the prefixes go through load-time normalisation: beacon's prefixes are + /// written docker.io/chainsafe/lodestar and Registry.Example.com:443/a. + fn pull_test_manager() -> Manager { + let dir = std::env::temp_dir().join(format!("dsp-pull-{}", rand::random::())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("probe.yaml"), + "service_name: probe\nallowed_image_prefixes:\n - alpine\nimage_digest_allowed: true\n", + ) + .unwrap(); + std::fs::write( + dir.join("beacon.yaml"), + "service_name: beacon\nallowed_image_prefixes:\n - docker.io/chainsafe/lodestar\n - ghcr.io/chainsafe/lodestar\n - localhost:5000/x\n - Registry.Example.com:443/a\nimage_tag_pattern: '^(v[0-9]+|[0-9]+)$'\n", + ) + .unwrap(); + let manager = Manager::new(dir.to_str().unwrap()).unwrap(); + std::fs::remove_dir_all(&dir).unwrap(); + manager } + /// Cross-language parity guard for #83 and #84. POST /images/create is + /// routed on its query string, the way the Docker CLI sends it: fromImage + /// (fully qualified, URL-encoded) and tag, with an empty body. The body is + /// ignored, fromSrc (import) is denied, and the image gets the create + /// checks: a policy prefix on the normalised repository, then that + /// policy's tag pattern and digest rule. Rows mirror the pull* runs in + /// spec/router.qnt; the rest are language-only. #[test] - fn test_route_image_pull_empty_body() { - let router = Router::new(make_manager(vec!["alpine"])); - let result = router.route("POST", "/images/create", None); - assert_eq!(result.action, Action::Deny); - } - - #[test] - fn test_route_image_pull_no_fromimage() { - let router = Router::new(make_manager(vec!["alpine"])); - let map: HashMap = - serde_json::from_value(serde_json::json!({"tag": "latest"})).unwrap(); - let result = router.route("POST", "/images/create", Some(&map)); - assert_eq!(result.action, Action::Deny); - assert!(result.deny_msg.unwrap().contains("fromImage")); + fn test_route_pull_query() { + let router = Router::new(pull_test_manager()); + let alpine_body: HashMap = + serde_json::from_value(serde_json::json!({"fromImage": "alpine"})).unwrap(); + let old_body: HashMap = + serde_json::from_value(serde_json::json!({"fromImage": "alpine:3.19"})).unwrap(); + let not_allowed = |image: &str| format!("image {} not allowed by any policy", image); + let required = "fromImage query parameter is required for image pull".to_string(); + let body_msg = "image pull must not have a request body".to_string(); + let from_src = "image import (fromSrc) is not allowed".to_string(); + let enc_digest = TEST_DIGEST.replace(':', "%3A"); + let cases: Vec<( + String, + Option<&HashMap>, + Action, + &str, + String, + )> = vec![ + // pullCliAllowedTest (#83): the issue's reproduction, as the CLI sends it. + ( + "/v1.56/images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19".into(), + None, + Action::Allow, + "probe", + String::new(), + ), + // pullShortNameAllowedTest (#84) + ( + "/images/create?fromImage=alpine&tag=3.19".into(), + None, + Action::Allow, + "probe", + String::new(), + ), + // pullLibraryNameAllowedTest (#84) + ( + "/images/create?fromImage=library%2Falpine&tag=latest".into(), + None, + Action::Allow, + "probe", + String::new(), + ), + // pullCliOrgAllowedTest (#84): the prefix is written docker.io/chainsafe/lodestar. + ( + "/images/create?fromImage=docker.io%2Fchainsafe%2Flodestar&tag=v1".into(), + None, + Action::Allow, + "beacon", + String::new(), + ), + // #84: the tag in fromImage, on another Docker Hub host name. + ( + "/images/create?fromImage=index.docker.io%2Flibrary%2Falpine%3A3.19".into(), + None, + Action::Allow, + "probe", + String::new(), + ), + // #83: the CLI sends a digest in tag. + ( + format!("/images/create?fromImage=alpine&tag={}", enc_digest), + None, + Action::Allow, + "probe", + String::new(), + ), + // #83: a digest in fromImage. + ( + format!("/images/create?fromImage=alpine%40{}", enc_digest), + None, + Action::Allow, + "probe", + String::new(), + ), + // #83: image_digest_allowed is false for beacon. + ( + format!( + "/images/create?fromImage=ghcr.io%2Fchainsafe%2Flodestar&tag={}", + enc_digest + ), + None, + Action::Deny, + "", + "image digests not allowed for service beacon".into(), + ), + // #84: a registry port is not a tag. + ( + "/images/create?fromImage=localhost%3A5000%2Fx%2Fy&tag=2".into(), + None, + Action::Allow, + "beacon", + String::new(), + ), + // #84: the registry host is compared lower-cased. + ( + "/images/create?fromImage=REGISTRY.example.com%3A443%2Fa&tag=1".into(), + None, + Action::Allow, + "beacon", + String::new(), + ), + // #83: the tag pattern applies to pulls. + ( + "/images/create?fromImage=chainsafe%2Flodestar&tag=latest".into(), + None, + Action::Deny, + "", + r#"image tag "latest" does not match pattern "^(v[0-9]+|[0-9]+)$""#.into(), + ), + // pullDisallowedCliDeniedTest (#83) + ( + "/images/create?fromImage=docker.io%2Fattacker%2Fmalware&tag=latest".into(), + None, + Action::Deny, + "", + not_allowed("docker.io/attacker/malware"), + ), + // pullOtherRegistryDeniedTest (#84): another registry is not Docker Hub's alpine. + ( + "/images/create?fromImage=localhost%3A5000%2Falpine&tag=1".into(), + None, + Action::Deny, + "", + not_allowed("localhost:5000/alpine"), + ), + // #84: the prefix match stays on namespace boundaries. + ( + "/images/create?fromImage=alpine-evil&tag=1".into(), + None, + Action::Deny, + "", + not_allowed("alpine-evil"), + ), + // pullNoFromImageDeniedTest (#83) + ("/images/create".into(), None, Action::Deny, "", required.clone()), + // #83: the old contract, fromImage in a JSON body: a pull has no body. + ( + "/images/create".into(), + Some(&old_body), + Action::Deny, + "", + body_msg.clone(), + ), + // pullBodyIgnoredDeniedTest (#83): the daemon would pull the query's image. + ( + "/images/create?fromImage=attacker%2Fmalware&tag=latest".into(), + Some(&alpine_body), + Action::Deny, + "", + body_msg.clone(), + ), + // pullJsonBodyDeniedTest (#83): any body is denied, even one that agrees. + ( + "/images/create?fromImage=alpine&tag=3.19".into(), + Some(&alpine_body), + Action::Deny, + "", + body_msg.clone(), + ), + // pullImportDeniedTest (#83): the daemon would import the tarball as alpine:latest. + ( + "/images/create?fromSrc=http%3A%2F%2Fexample.com%2Fx.tar&repo=alpine&tag=latest".into(), + None, + Action::Deny, + "", + from_src.clone(), + ), + // pullImportWithImageDeniedTest (#83) + ( + "/images/create?fromImage=alpine&tag=latest&fromSrc=-".into(), + None, + Action::Deny, + "", + from_src.clone(), + ), + // #83: a tag in fromImage and in tag is ambiguous. + ( + "/images/create?fromImage=alpine%3A3.19&tag=3.20".into(), + None, + Action::Deny, + "", + "fromImage alpine:3.19 already has a tag or digest; the tag query parameter is ambiguous".into(), + ), + // #83: no tag and no digest pulls every tag. + ( + "/images/create?fromImage=alpine".into(), + None, + Action::Deny, + "", + "image pull without a tag or digest pulls every tag and is not allowed".into(), + ), + // #84: upper case in the repository path is invalid. + ( + "/images/create?fromImage=Alpine&tag=latest".into(), + None, + Action::Deny, + "", + r#"invalid image reference "Alpine""#.into(), + ), + // #83: a repeated parameter could be read two ways. + ( + "/images/create?fromImage=alpine&fromImage=attacker%2Fmalware&tag=1".into(), + None, + Action::Deny, + "", + "duplicate fromImage query parameter".into(), + ), + // #83: the daemon rejects a malformed query; so does the proxy. + ( + "/images/create?fromImage=alpine%zz&tag=1".into(), + None, + Action::Deny, + "", + "invalid query string".into(), + ), + // #53 still holds: the percent check reads the path, not the query. + ( + "/images/%63reate?fromImage=alpine&tag=1".into(), + None, + Action::Deny, + "", + "percent-encoded path not allowed".into(), + ), + ]; + let mut mismatches = Vec::new(); + for (path, body, want, service, want_msg) in &cases { + let got = router.route("POST", path, *body); + let got_service = got.service.as_deref().unwrap_or(""); + let got_msg = got.deny_msg.as_deref().unwrap_or(""); + if got.action != *want || got_service != *service || got_msg != want_msg { + mismatches.push(format!( + "POST {}: got {:?}/{:?}/{:?}, want {:?}/{:?}/{:?}", + path, got.action, got_service, got_msg, want, service, want_msg + )); + } + } + assert!( + mismatches.is_empty(), + "pull routing mismatches:\n{}", + mismatches.join("\n") + ); } + /// The normalisation table for #84, on the create route: each spelling + /// selects the policy its normalised repository matches, and a path with + /// upper case is invalid. The same table runs in Go and TypeScript. #[test] - fn test_route_image_pull_not_found() { - let router = Router::new(make_manager(vec!["alpine"])); - let map: HashMap = - serde_json::from_value(serde_json::json!({"fromImage": "ubuntu"})).unwrap(); - let result = router.route("POST", "/images/create", Some(&map)); - assert_eq!(result.action, Action::Deny); - assert!(result.deny_msg.unwrap().contains("not allowed by any policy")); + fn test_route_image_normalization() { + let router = Router::new(pull_test_manager()); + let not_allowed = |image: &str| format!("image {} not allowed by any policy", image); + let ghcr_digest = format!("ghcr.io/chainsafe/lodestar@{}", TEST_DIGEST); + let cases: Vec<(&str, Action, &str, String)> = vec![ + ("alpine", Action::CreateContainer, "probe", String::new()), + ("library/alpine", Action::CreateContainer, "probe", String::new()), + ( + "docker.io/library/alpine", + Action::CreateContainer, + "probe", + String::new(), + ), + ("docker.io/alpine", Action::CreateContainer, "probe", String::new()), + ( + "index.docker.io/library/alpine:3.19", + Action::CreateContainer, + "probe", + String::new(), + ), + ( + "docker.io/chainsafe/lodestar:v1", + Action::CreateContainer, + "beacon", + String::new(), + ), + ( + "chainsafe/lodestar:v1", + Action::CreateContainer, + "beacon", + String::new(), + ), + ( + "registry-1.docker.io/chainsafe/lodestar:v1", + Action::CreateContainer, + "beacon", + String::new(), + ), + (&ghcr_digest, Action::CreateContainer, "beacon", String::new()), + ("localhost:5000/x/y:2", Action::CreateContainer, "beacon", String::new()), + ( + "registry.example.com:443/a", + Action::CreateContainer, + "beacon", + String::new(), + ), + ( + "Alpine:latest", + Action::Deny, + "", + r#"invalid image reference "Alpine:latest""#.into(), + ), + ( + "chainsafe/Lodestar:v1", + Action::Deny, + "", + r#"invalid image reference "chainsafe/Lodestar:v1""#.into(), + ), + ("alpine-evil", Action::Deny, "", not_allowed("alpine-evil")), + ( + "ghcr.io/library/alpine", + Action::Deny, + "", + not_allowed("ghcr.io/library/alpine"), + ), + ( + "ghcr.io/chainsafe/lodestar-evil", + Action::Deny, + "", + not_allowed("ghcr.io/chainsafe/lodestar-evil"), + ), + ( + "localhost:5000/xy:1", + Action::Deny, + "", + not_allowed("localhost:5000/xy:1"), + ), + ]; + let mut mismatches = Vec::new(); + for (image, want, service, want_msg) in &cases { + let body: HashMap = + serde_json::from_value(serde_json::json!({ "Image": image })).unwrap(); + let got = router.route("POST", "/containers/create", Some(&body)); + let got_service = got.service.as_deref().unwrap_or(""); + let got_msg = got.deny_msg.as_deref().unwrap_or(""); + if got.action != *want || got_service != *service || got_msg != want_msg { + mismatches.push(format!( + "create {}: got {:?}/{:?}/{:?}, want {:?}/{:?}/{:?}", + image, got.action, got_service, got_msg, want, service, want_msg + )); + } + } + assert!( + mismatches.is_empty(), + "normalisation mismatches:\n{}", + mismatches.join("\n") + ); } #[test] diff --git a/spec/README.md b/spec/README.md index 9a0ce50..7a07493 100644 --- a/spec/README.md +++ b/spec/README.md @@ -9,7 +9,7 @@ This directory contains a [Quint](https://quint-lang.org/) formal specification | `docker_socket_policy.qnt` | Request-handling spec: policy types, state machine, endpoint routing table, 9 invariants (6 P0 / 3 P1), 6 attack scenario simulations. Module `network_endpoints_model` models `NetworkingConfig.EndpointsConfig` under a forced `network_mode` (#87), one `run` test per table row, instances `network_endpoints` (all three after #87) and `network_endpoints_pre87` | | `listener.qnt` | Listening-socket startup: flag/group selection, existing-path checks, single-instance lock, 6 invariants, one `run` test per design-table row. Instances `listener_locked` (Go, Rust) and `listener_unlocked` (TypeScript) | | `listener-design.md` | Design of the listening socket (dockerd parity) that `listener.qnt` models | -| `router.qnt` | The percent-encoded path deny, the empty interior segment deny, the dot segment deny, the segment-exact exec deny and `POST /containers/create` route, the segment-exact attach deny, the GET/HEAD-only read passthrough with `/_ping` standing for the read endpoints, container-name extraction and the container-lifecycle branch of the router only, not the full router. The daemon's percent-decoding as a fixed table. One `run` test per table row. Instances `router` (Go, TypeScript, and Rust after #48, #53, #49, #55, #76, #78 and #70), `router_pre48` (Rust's extraction rule before #48), `router_pre53` (Rust and TypeScript before #53, routing on the raw path), `router_pre55` (all three before #55, letting empty interior segments through), `router_readonly` (as `router`, with `--readonly`, #75), `router_pre76` (all three before #76, letting attach through), `router_pre78` (all three before #78, allowing the read endpoints for every method) and `router_pre70` (all three before #70, letting `.` and `..` segments through). Every instance includes the #49 exec and create rule, so the pre instances isolate one earlier rule each rather than being full snapshots | +| `router.qnt` | The percent-encoded path deny, the empty interior segment deny, the dot segment deny, the segment-exact exec deny and `POST /containers/create` route, the segment-exact attach deny, the GET/HEAD-only read passthrough with `/_ping` standing for the read endpoints, container-name extraction and the container-lifecycle branch of the router only, not the full router. The daemon's percent-decoding as a fixed table. One `run` test per table row. Instances `router` (Go, TypeScript, and Rust after #48, #53, #49, #55, #76, #78 and #70), `router_pre48` (Rust's extraction rule before #48), `router_pre53` (Rust and TypeScript before #53, routing on the raw path), `router_pre55` (all three before #55, letting empty interior segments through), `router_readonly` (as `router`, with `--readonly`, #75), `router_pre76` (all three before #76, letting attach through), `router_pre78` (all three before #78, allowing the read endpoints for every method) and `router_pre70` (all three before #70, letting `.` and `..` segments through). Every instance includes the #49 exec and create rule, so the pre instances isolate one earlier rule each rather than being full snapshots. Module `pull_model` models the `POST /images/create` branch: which of the body and the query names the image, and image-name normalisation as a fixed table. Instances `pull` (all three after #83 and #84), `pull_pre83` (all three before #83, reading `fromImage` from the body), `pull_body_ignored` (the first #83 fix, which forwarded a pull's body) and `pull_pre84` (all three before #84, matching the spelling as sent) | ## How to Run @@ -34,7 +34,7 @@ quint run spec/listener.qnt --main=listener_locked --max-steps=30 --invariant al quint test spec/listener.qnt --main=listener_locked quint test spec/listener.qnt --main=listener_unlocked -# Router model: typecheck, run the table tests on all eight instances +# Router model: typecheck, run the table tests on all twelve instances quint typecheck spec/router.qnt quint test spec/router.qnt --main=router quint test spec/router.qnt --main=router_pre48 @@ -44,6 +44,10 @@ quint test spec/router.qnt --main=router_readonly --match='^readonly.*Test$' quint test spec/router.qnt --main=router_pre76 quint test spec/router.qnt --main=router_pre78 quint test spec/router.qnt --main=router_pre70 +quint test spec/router.qnt --main=pull +quint test spec/router.qnt --main=pull_pre83 +quint test spec/router.qnt --main=pull_body_ignored +quint test spec/router.qnt --main=pull_pre84 # Network endpoints under a forced network_mode (#87): the table tests on both instances quint test spec/docker_socket_policy.qnt --main=network_endpoints @@ -227,6 +231,38 @@ Each implementation's router tests use the same row names in comments (added wit | `namedExtraDenied` | `mynet` | `{"default": {}, "other": {}}` | deny | | `namedOwnOverDefault` | `mynet` | `{"default": {Aliases}, "mynet": {}}` | `{"mynet": {}}` | +### Image pull (`router.qnt`, `pull_model`) + +`POST /images/create` is modelled on its own, from the point where the router has taken that branch. A request is its body (`none`, `json` or `form`), the `fromImage` the body names, the `fromImage` query parameter, and whether the query carries `fromSrc`; `""` stands for an absent field. The daemon reads the pull parameters with Go's `r.Form`: a form body (`application/x-www-form-urlencoded`) wins over the query, and a JSON body is ignored. With an image it pulls it; with none and `fromSrc` set it imports a tarball under the `repo` and `tag` the caller names. The Docker CLI sends the image fully qualified in the query (`fromImage=docker.io/library/alpine&tag=3.19`) and no body. The table `CANONICAL` maps each spelling to the repository the daemon pulls, which is what the implementations' normalisation returns; the policies' prefixes (`ALLOWED_REPOS`: `alpine`, `chainsafe/lodestar`) are canonical, as the implementations normalise them at load time. + +`READ_PULL_FROM_BODY = true` models all three before #83, which read `fromImage` from the JSON body. `NORMALIZE_IMAGES = false` models all three before #84, which matched the spelling as sent against the prefix as written. `ALLOW_PULL_BODY = true` models the first #83 fix, which routed on the query but forwarded the body unchanged, so a form body naming another image overrode the query at the daemon; the final fix denies any pull with a body (`image pull must not have a request body`). + +- `pullForwardsOnlyAllowedImages` holds when every forwarded request makes the daemon pull (not import) an image whose canonical form a policy allows. It mentions no flag and reads what the daemon reads: the form body's `fromImage` over the query's. +- `pullWithBodyNeverForwarded` holds when a pull with a body of any kind is denied. +- `cliPullOfAllowedImageForwarded` holds when a pull of an allowed image, sent the Docker CLI way (empty body, image in the query), is forwarded, in every spelling of the image. +- `pullDecidedOnCanonicalImage` holds when two spellings of the same image in the query get the same outcome. + +On `pull`, `pullSoundTest` asserts all four. `pull_pre83` runs `pre83UnsoundTest`, which asserts that `pullForwardsOnlyAllowedImages` and `cliPullOfAllowedImageForwarded` fail, that the CLI's pull of `docker.io/library/alpine` is denied, and that a body naming `alpine` forwards a query pull of `attacker/malware` and a `fromSrc` import ([#83](https://github.com/ChainSafe/docker-socket-policy/issues/83)). `pull_body_ignored` runs `bodyIgnoredUnsoundTest`, which asserts that `pullForwardsOnlyAllowedImages` and `pullWithBodyNeverForwarded` fail, that the CLI pull still works, and that a form body naming `attacker/malware` with `fromImage=alpine` in the query is forwarded. `pull_pre84` runs `pre84UnsoundTest`, which asserts that `pullForwardsOnlyAllowedImages` still holds (the old match only added denials), that `cliPullOfAllowedImageForwarded` and `pullDecidedOnCanonicalImage` fail, and that `docker.io/library/alpine` is denied while `alpine` is allowed ([#84](https://github.com/ChainSafe/docker-socket-policy/issues/84)). Setting `pull`'s `READ_PULL_FROM_BODY` to `true` fails `pullSoundTest` and six rows; setting its `NORMALIZE_IMAGES` to `false` fails `pullSoundTest` and three rows; setting its `ALLOW_PULL_BODY` to `true` fails `pullSoundTest` and two rows (`pullFormBodyDenied`, `pullJsonBodyDenied`). + +Rows for the image pull ([#83](https://github.com/ChainSafe/docker-socket-policy/issues/83), [#84](https://github.com/ChainSafe/docker-socket-policy/issues/84)). The query is written as the CLI sends it; the implementations deny with the messages shown: + +| Row (`run Test`) | Request | Outcome | Instances | +|-----|---------|---------|-----------| +| `pullCliAllowed` | `?fromImage=docker.io/library/alpine`, no body | allow | `pull` | +| `pullCliOrgAllowed` | `?fromImage=docker.io/chainsafe/lodestar`, empty body | allow | `pull` | +| `pullShortNameAllowed` | `?fromImage=alpine`, empty body | allow | `pull` | +| `pullLibraryNameAllowed` | `?fromImage=library/alpine`, empty body | allow | `pull` | +| `pullNoFromImageDenied` | no `fromImage`, empty body | deny (`fromImage query parameter is required for image pull`) | `pull` | +| `pullBodyIgnoredDenied` | `?fromImage=attacker/malware`, JSON body `{"fromImage":"alpine"}` | deny (`image pull must not have a request body`) | `pull` | +| `pullFormBodyDenied` | `?fromImage=alpine`, form body `fromImage=attacker/malware` | deny (`image pull must not have a request body`) | `pull` | +| `pullJsonBodyDenied` | `?fromImage=alpine`, JSON body `{"fromImage":"alpine"}` | deny (`image pull must not have a request body`) | `pull` | +| `pullImportDenied` | `?fromSrc=…`, no body | deny (`image import (fromSrc) is not allowed`) | `pull` | +| `pullImportWithImageDenied` | `?fromImage=alpine&fromSrc=…` | deny (`image import (fromSrc) is not allowed`) | `pull` | +| `pullDisallowedCliDenied` | `?fromImage=docker.io/attacker/malware`, empty body | deny | all `pull*` | +| `pullOtherRegistryDenied` | `?fromImage=localhost:5000/alpine`, empty body | deny | all `pull*` | + +The language tables add rows the model cannot express, because its images are atoms: the tag and digest (`tag=` in the query, or in `fromImage`, but not both), the tag pattern, `image_digest_allowed`, the pull of every tag when neither is given, duplicate parameters, and the reference parser itself (registry ports, upper case, the namespace-boundary prefix match). + ### Modeling Notes Two invariants are structurally tautological within the Quint model — they can't be falsified by any action sequence the simulator generates, so they don't get real coverage from `quint run`/`quint verify`: @@ -239,10 +275,10 @@ Two invariants are structurally tautological within the Quint model — they can - **`lifecycleOnlyTargetsRealNames` is close to a tautology.** `route` only allows by name when `hasContainerName` holds, and `hasContainerName` is nearly the property itself. It is not vacuous: on `router_pre48`, where an empty segment counts as a name, the property fails, and `pre48UnsoundTest` asserts that. Most of the evidence comes from the table rows, whose names the language router tests reuse (added with the #48 fix). - **`routerSeesWhatDaemonSees` is also close to a tautology on `router`.** The percent check denies exactly the paths with a segment that is a key of `DAEMON_DECODING`, and those are exactly the paths that decoding changes. It is not vacuous: on `router_pre53` the property fails, and `pre53UnsoundTest` asserts that. Setting `router`'s `ALLOW_PERCENT` to `true` makes `percentSoundTest` and five of the six `percent*` rows fail. `percentSubpathStartDenied` still passes then, because the lifecycle branch denies `POST` when the last segment (`beacon%2Fstart`) is not an action. That row is a pin: routing on the raw path already denies it, and only Go's decode-first handler before #53 would have allowed it, since the daemon reads `POST /containers/beacon/start`. `percentNameStartDenied` (`POST /containers/%2F/start`) is the discriminating `POST` row. `lifecycleOnlyTargetsRealNames` holds on `router_pre53` too: `%2F` is a non-empty, non-reserved segment. That is why #53 needs a property that compares the router's reading with the daemon's. - **Decoding is a fixed table.** Quint cannot inspect the characters of a string, so the model cannot find a `%` or decode one. `DAEMON_DECODING` lists the encoded tokens in the path universe and the segments the daemon reads them as; a segment is encoded iff it is a key. The implementations check for any `%` in the path. The table covers the shapes that matter: an encoded `/` (upper and lower case) that splits a segment, a fully encoded reserved word, and an encoded `/` inside a name that adds a lifecycle action. -- **The query string is outside the model.** Paths are segment lists with the query string already removed, so the model says nothing about `%` in the query. The implementations never inspect the query string for this rule; encoded queries such as `?filters=%7B…%7D` are routine and must still be forwarded. That is pinned by the language handler tests and the integration tests, not by the model. +- **The query string is outside `router_model`.** Paths are segment lists with the query string already removed, so the model says nothing about `%` in the query. `pull_model` reads two query parameters (`fromImage`, `fromSrc`) and a body's `fromImage` as already decoded tokens. The implementations never inspect the query string for this rule; encoded queries such as `?filters=%7B…%7D` are routine and must still be forwarded. That is pinned by the language handler tests and the integration tests, not by the model. - **GET is covered.** The percent check runs before the GET passthrough, and `routerSeesWhatDaemonSees` ranges over `GET` as well as `POST` and `DELETE`. On `router_pre53` the passthrough lets `GET /containers/%2F` through, and the property fails for it too. - **No instance models Go before #53.** Go decoded before routing, which is `route(m, decodePath(segs))`. With this table that agrees with the daemon by construction, so `routerSeesWhatDaemonSees` cannot express Go's risk: decoding that differs from the daemon's, such as encoded version segments ([#57](https://github.com/ChainSafe/docker-socket-policy/issues/57)). Go's change is covered by the language handler tests. -- **`router.qnt` is not the full router.** `route` models the read-only check, which runs first in all three routers, the percent check, the empty interior segment check, the dot segment check, the exec check, the attach check, the exact `POST /containers/create` route and the container-lifecycle branch, in that order. It leaves out the API-version strip and the other checks that the routers run before the lifecycle branch, such as auth and the build and commit denials, as well as `POST /images/create` and the endpoints after the lifecycle branch. For paths that those checks catch, the model's outcome can differ from what an implementation does: `GET /build` is `allowRead` in the model, for example, while the routers deny it as `build is not allowed`. The `images` rows of #49 (`GET /images/exec` allowed, `POST /images/create/extra` denied) are therefore language-only rows, as are `GET /containers/myexec/json` and `DELETE /containers/executor`: the model's words are atoms, so it cannot express a name that merely contains `exec`. Only the properties and the table rows are claims about the code. +- **`router.qnt` is not the full router.** `route` models the read-only check, which runs first in all three routers, the percent check, the empty interior segment check, the dot segment check, the exec check, the attach check, the exact `POST /containers/create` route and the container-lifecycle branch, in that order. It leaves out the API-version strip and the other checks that the routers run before the lifecycle branch, such as auth and the build and commit denials, as well as `POST /images/create` (modelled separately in `pull_model`) and the endpoints after the lifecycle branch. For paths that those checks catch, the model's outcome can differ from what an implementation does: `GET /build` is `allowRead` in the model, for example, while the routers deny it as `build is not allowed`. The `images` rows of #49 (`GET /images/exec` allowed, `POST /images/create/extra` denied) are therefore language-only rows, as are `GET /containers/myexec/json` and `DELETE /containers/executor`: the model's words are atoms, so it cannot express a name that merely contains `exec`. Only the properties and the table rows are claims about the code. - **Exec and create match whole segments (#49).** Before #49 the routers disagreed: Go matched `exec` only in the name position (`/containers/exec`), so `DELETE /containers/mycontainer/exec` went down the lifecycle branch and was allowed. Rust matched the substring `/exec` anywhere after `/containers/`, and TypeScript anywhere in the path, so Rust and TypeScript denied containers named `exec-runner` or `executor`. Neither Go nor Rust checked the `/exec/…` namespace, so both let `GET /exec//json` through the GET passthrough. Go also accepted `POST /containers/create/extra` as a container create. In the model the exec check runs before the create route, the lifecycle branch and the GET passthrough, so it denies every method, GET included: exec inspect shows the full command line. `reservedExecDeleteDenied` (`DELETE /containers/exec`) is decided by the exec check, as in all three implementations, and now asserts `denyExec`. `exec` stays in the reserved set, but no row, in the model or in the language tests, would catch it being dropped from there. - **The #49 properties are not vacuous.** None of them is shown on a separate instance; replacing the rule in `route` makes them fail. With a substring-style rule (a segment that starts with `exec`, modeled as the set `exec`, `exec-runner`), `execPrefixSoundTest` and the three `execPrefixName*Allowed` rows fail. With Go's rule before #49 (`exec` only as the second segment under `containers`), `execSoundTest` and the four `exec*Denied` rows fail. `execSubpathPostDenied` and `execNamespacePostDenied` fail there only because the outcome is the default `deny`, not `denyExec`: the request is still refused, as before #49, but not by the exec check. With Go's prefix match for create, `createExactSoundTest` and `createSubpathDenied` fail. `execNeverAllowed` writes out the exec positions itself instead of calling `isExecPath`, so that a narrower `isExecPath` cannot narrow the property with it. - **HEAD is routed like GET (#55).** `METHODS` is `GET`, `HEAD`, `POST`, `DELETE`. Before #55 the lifecycle branches disagreed on `HEAD /containers/x`: Go and TypeScript fell through to the GET/HEAD passthrough and allowed it, while Rust denied it. All three now allow it like `GET`, and the model's lifecycle branch allows `GET` and `HEAD` alike. @@ -252,6 +288,8 @@ Two invariants are structurally tautological within the Quint model — they can - **Attach is a router rule; the upgrade refusal is not modelled (#76).** Before #76, `GET /containers/{name}/attach/ws` with `Upgrade: websocket` fell through to the GET passthrough, and Go relayed the daemon's `101`, giving the caller the container's stdin. The router now denies `attach` as a whole segment after `containers`, for every method, right after the exec check. As with `/containers/exec/…`, a container literally named `attach` is denied too (`attachNamedContainerDenied`). `attachNeverAllowed` fails on `router_pre76`, and setting `router`'s `ALLOW_ATTACH` to `true` makes `attachSoundTest` and three of the four `attach*` rows fail; `attachPostDenied` is a pin, since the lifecycle branch already denies `POST` when the last segment is not an action. The handler also refuses every request with an `Upgrade` header, or a `Connection` header listing `upgrade`, with `protocol upgrade not allowed`, before routing. That check is on headers, which the model does not have, so like the query string it is pinned by the language handler tests and the integration tests, not by the model. - **The read endpoints admit only `GET` and `HEAD`; the `CONNECT` refusal is not modelled (#78).** Before #78 an early clause in all three routers allowed `/_ping`, `/version`, `/info` and any path starting with `/events` for every method, so `POST /_ping` and `DELETE /version` reached the daemon, and `/eventsxyz` matched too. The clause is gone: `GET` and `HEAD` on those paths reach the read passthrough as before, and every other method falls through to the default deny. `readPassthroughOnlyForReads` fails on `router_pre78`, and setting `router`'s `ALLOW_ANY_METHOD_PING` to `true` makes `readPassthroughSoundTest`, `pingPostDenied` and `pingDeleteDenied` fail. The handlers also refuse every `CONNECT` request with `CONNECT is not allowed` before routing, ahead of the upgrade check; `METHODS` has no `CONNECT`, so that is pinned by the language handler tests and the integration tests, not by the model. - **Network endpoints are a pure function, not part of the state machine (#87).** `createContainer` records the forced `networkMode` but takes no `NetworkingConfig`, and nothing in the state machine models the daemon's start, so the endpoint rule sits in its own module beside it and is checked by `quint test`, not by `make verify`. When the policy does not force `network_mode`, `NetworkingConfig` is forwarded untouched; neither the model nor the implementations check it then. Whether the daemon accepts the settings kept on a re-keyed entry (aliases on the default `bridge` network, for example) is the daemon's decision; the model fixes only the keys. `extraNetworkDenied` and `onlyForcedNetwork` are close to tautologies on `network_endpoints`, since `handleEndpoints` tests the key set directly; `network_endpoints_pre87` shows they are not vacuous. Dropping the extra keys instead of denying would keep `onlyForcedNetwork` and fail `extraNetworkDenied`; that was rejected, because the caller asked for a network the policy does not allow and should be told. A malformed `NetworkingConfig` or `EndpointsConfig` (not a JSON object) is denied by the implementations under a forced mode; the model's maps cannot express it, so the language tests pin it. + +- **Image references are tokens, normalisation is a table (#84).** Quint cannot split a string, so `pull_model` cannot parse `[registry/]repository[:tag][@digest]`. `CANONICAL` lists the spellings in the universe and the repository each one names; the implementations compute the same thing with one parser per language, used for the request's image (create `Image` and pull `fromImage`), for the registry gate and for each policy prefix at load time. The parser's rules (the tag is after the last `:` that follows the last `/`; the first component is a registry when it contains `.` or `:` or is `localhost`; `docker.io`, `index.docker.io` and `registry-1.docker.io` are dropped, and so is `library/` before a single-component name; only the registry is lower-cased, and upper case in the path is invalid) are pinned by the same normalisation table in the Go, Rust and TypeScript tests, not by the model. So are the namespace-boundary prefix match (`prefix` or `prefix/…`), which the model reduces to equality, and the tag and digest rules. - **Listener fault bias.** `step` crashes an instance on 1 in 10 draws instead of half of all steps, so random runs actually interleave live instances. Every crash stays reachable from every phase, so the reachable state space is unchanged. ### Attack Scenarios Prevented by Invariants diff --git a/spec/router.qnt b/spec/router.qnt index d02874e..f0cd6b4 100644 --- a/spec/router.qnt +++ b/spec/router.qnt @@ -621,3 +621,254 @@ module router_pre70 { route("GET", [".", "containers", "attach"]) == "allowRead", }) } + +// ─── Module: pull_model ───────────────────────────────────────────────── +// +// The `POST /images/create` branch of the router (#83, #84), which +// `router_model` leaves out. The path checks above it are the same for every +// request here, so the model starts at the branch: a request is its body +// (none, JSON or a form), the image the body names, the `fromImage` query +// parameter, and whether the query carries `fromSrc`. +// +// The daemon reads the pull parameters with Go's `r.Form`: for a form body +// (`Content-Type: application/x-www-form-urlencoded`) the body's values come +// first and win over the query; a JSON body is ignored. With an image to +// pull it pulls it; with none and `fromSrc` set it imports a tarball (from a +// URL or the body) under the `repo` and `tag` the caller names. The Docker +// CLI sends `fromImage` fully qualified (`docker.io/library/alpine`) in the +// query, and no body. +// +// An image reference is a token. Quint cannot split strings, so parsing and +// normalisation are a fixed table (CANONICAL) from each spelling to the +// repository the daemon pulls: Docker Hub's `docker.io/` and `library/` are +// dropped, any other registry stays. Policy prefixes are canonical (the +// implementations normalise them at load time), and a match is equality: the +// namespace-boundary prefix rule, tags, digests and the tag pattern are left +// to the language tests. +// +// READ_PULL_FROM_BODY = true models all three before #83, which read +// `fromImage` from a JSON body and never looked at the query: the Docker +// CLI's empty body was denied, and a body naming an allowed image let any +// query through. false models all three after the fix: the query decides +// and `fromSrc` is denied. +// +// NORMALIZE_IMAGES = false models all three before #84, which compared the +// spelling as sent with the prefix as written, so `docker.io/library/alpine` +// did not match `alpine`. true models all three after the fix. +// +// ALLOW_PULL_BODY = true models the first #83 fix, which routed on the query +// and forwarded any body unchanged, so a form body naming another image +// overrode the query at the daemon. false models the final fix: a pull with +// a body is denied. It only matters when READ_PULL_FROM_BODY is false. + +module pull_model { + const READ_PULL_FROM_BODY: bool + const NORMALIZE_IMAGES: bool + const ALLOW_PULL_BODY: bool + + // bodyKind is "none", "json" or "form"; bodyImage is the `fromImage` the + // body names ("" for none). + type PullRequest = { bodyKind: str, bodyImage: str, queryImage: str, fromSrc: bool } + + // Every spelling in the universe, "" for an absent parameter or body field. + pure val IMAGES = Set( + "", "alpine", "library/alpine", "docker.io/library/alpine", + "chainsafe/lodestar", "docker.io/chainsafe/lodestar", + "attacker/malware", "docker.io/attacker/malware", "localhost:5000/alpine", + ) + + // The repository the daemon pulls for each spelling, which is also what the + // implementations' normalisation returns. `localhost:5000/alpine` is on + // another registry, so it is not `alpine`. + pure val CANONICAL: str -> str = Map( + "alpine" -> "alpine", + "library/alpine" -> "alpine", + "docker.io/library/alpine" -> "alpine", + "chainsafe/lodestar" -> "chainsafe/lodestar", + "docker.io/chainsafe/lodestar" -> "chainsafe/lodestar", + "attacker/malware" -> "attacker/malware", + "docker.io/attacker/malware" -> "attacker/malware", + "localhost:5000/alpine" -> "localhost:5000/alpine", + ) + + // The policies' prefixes, canonical. + pure val ALLOWED_REPOS = Set("alpine", "chainsafe/lodestar") + + pure def canonicalAllowed(s: str): bool = + s != "" and ALLOWED_REPOS.contains(CANONICAL.get(s)) + + // The router's image match: on the canonical form after #84, on the + // spelling as sent before it. + pure def imageAllowed(s: str): bool = + if (s == "") false + else if (NORMALIZE_IMAGES) canonicalAllowed(s) + else ALLOWED_REPOS.contains(s) + + pure def routePull(req: PullRequest): str = + if (READ_PULL_FROM_BODY) { + if (req.bodyKind != "json" or req.bodyImage == "") "deny" + else if (imageAllowed(req.bodyImage)) "allowPull" + else "deny" + } + else if (not(ALLOW_PULL_BODY) and req.bodyKind != "none") "deny" + else if (req.fromSrc) "deny" + else if (req.queryImage == "") "deny" + else if (imageAllowed(req.queryImage)) "allowPull" + else "deny" + + // The image the daemon reads: a form body's `fromImage` wins over the + // query's. + pure def daemonImage(req: PullRequest): str = + if (req.bodyKind == "form" and req.bodyImage != "") req.bodyImage else req.queryImage + + // What the daemon does with a forwarded request. + pure def daemonAction(req: PullRequest): str = + if (daemonImage(req) != "") "pull" + else if (req.fromSrc) "import" + else "none" + + pure def cliPull(s: str): PullRequest = { bodyKind: "none", bodyImage: "", queryImage: s, fromSrc: false } + + pure val REQUESTS: Set[PullRequest] = + tuples(Set("none", "json", "form"), IMAGES, IMAGES, Set(false, true)) + .filter(t => t._1 != "none" or t._2 == "") + .map(t => { bodyKind: t._1, bodyImage: t._2, queryImage: t._3, fromSrc: t._4 }) + + // ─── Properties ───────────────────────────────────────────────────── + + // The security claim: a forwarded request makes the daemon pull an image a + // policy allows, never import and never pull another image (#83). It + // mentions no flag and reads only what the daemon reads: the form body + // over the query. + pure val pullForwardsOnlyAllowedImages: bool = + REQUESTS.forall(req => + routePull(req) == "allowPull" implies and { + daemonAction(req) == "pull", + canonicalAllowed(daemonImage(req)), + } + ) + + // A pull with a body, of any kind, is never forwarded. + pure val pullWithBodyNeverForwarded: bool = + REQUESTS.forall(req => req.bodyKind != "none" implies routePull(req) == "deny") + + // A pull of an allowed image sent the Docker CLI way (no body, image in + // the query), in any spelling of it, is forwarded (#83, #84). + pure val cliPullOfAllowedImageForwarded: bool = + IMAGES.forall(s => canonicalAllowed(s) implies routePull(cliPull(s)) == "allowPull") + + // Two spellings of the same image in the query get the same outcome (#84). + pure val pullDecidedOnCanonicalImage: bool = + REQUESTS.forall(req => IMAGES.forall(s => + (req.queryImage != "" and s != "" and CANONICAL.get(req.queryImage) == CANONICAL.get(s)) + implies routePull(req) == routePull({ ...req, queryImage: s }) + )) + + // ─── Tests: one run per table row ─────────────────────────────────── + + // Rows whose outcome depends on no flag run on every pull instance. + + run pullDisallowedCliDeniedTest = + assert(routePull(cliPull("docker.io/attacker/malware")) == "deny") + + run pullOtherRegistryDeniedTest = + assert(routePull(cliPull("localhost:5000/alpine")) == "deny") +} + +// ─── Pull instances ───────────────────────────────────────────────────── + +// All three after #83 and #84: the query decides, a body is denied, and +// images are compared in canonical form. +module pull { + import pull_model(READ_PULL_FROM_BODY = false, NORMALIZE_IMAGES = true, ALLOW_PULL_BODY = false).* + + // `docker pull alpine:3.19`, as the CLI sends it. + run pullCliAllowedTest = + assert(routePull(cliPull("docker.io/library/alpine")) == "allowPull") + + run pullCliOrgAllowedTest = + assert(routePull(cliPull("docker.io/chainsafe/lodestar")) == "allowPull") + + run pullShortNameAllowedTest = + assert(routePull(cliPull("alpine")) == "allowPull") + + run pullLibraryNameAllowedTest = + assert(routePull(cliPull("library/alpine")) == "allowPull") + + run pullNoFromImageDeniedTest = + assert(routePull(cliPull("")) == "deny") + + // A JSON body naming an allowed image does not let a disallowed query through. + run pullBodyIgnoredDeniedTest = + assert(routePull({ bodyKind: "json", bodyImage: "alpine", queryImage: "attacker/malware", fromSrc: false }) == "deny") + + // A form body would override the query at the daemon. + run pullFormBodyDeniedTest = + assert(routePull({ bodyKind: "form", bodyImage: "attacker/malware", queryImage: "alpine", fromSrc: false }) == "deny") + + // Any body is denied, even one that agrees with an allowed query. + run pullJsonBodyDeniedTest = + assert(routePull({ bodyKind: "json", bodyImage: "alpine", queryImage: "alpine", fromSrc: false }) == "deny") + + run pullImportDeniedTest = + assert(routePull({ bodyKind: "none", bodyImage: "", queryImage: "", fromSrc: true }) == "deny") + + run pullImportWithImageDeniedTest = + assert(routePull({ bodyKind: "none", bodyImage: "", queryImage: "alpine", fromSrc: true }) == "deny") + + run pullSoundTest = + assert(and { + pullForwardsOnlyAllowedImages, + pullWithBodyNeverForwarded, + cliPullOfAllowedImageForwarded, + pullDecidedOnCanonicalImage, + }) +} + +// All three before #83: `fromImage` was read from a JSON body. The CLI's +// empty body was denied, and an allowed image in the body forwarded a pull +// of any image in the query, or an import. Normalisation is on, so that this +// instance isolates #83. +module pull_pre83 { + import pull_model(READ_PULL_FROM_BODY = true, NORMALIZE_IMAGES = true, ALLOW_PULL_BODY = false).* + + run pre83UnsoundTest = + assert(and { + not(pullForwardsOnlyAllowedImages), + not(cliPullOfAllowedImageForwarded), + routePull(cliPull("docker.io/library/alpine")) == "deny", + routePull({ bodyKind: "json", bodyImage: "alpine", queryImage: "attacker/malware", fromSrc: false }) == "allowPull", + routePull({ bodyKind: "json", bodyImage: "alpine", queryImage: "", fromSrc: true }) == "allowPull", + }) +} + +// The first #83 fix: the query decided, but the body was forwarded +// unchanged, so a form body naming any image overrode an allowed query at +// the daemon. Normalisation is on. +module pull_body_ignored { + import pull_model(READ_PULL_FROM_BODY = false, NORMALIZE_IMAGES = true, ALLOW_PULL_BODY = true).* + + run bodyIgnoredUnsoundTest = + assert(and { + not(pullForwardsOnlyAllowedImages), + not(pullWithBodyNeverForwarded), + cliPullOfAllowedImageForwarded, + routePull({ bodyKind: "form", bodyImage: "attacker/malware", queryImage: "alpine", fromSrc: false }) == "allowPull", + }) +} + +// All three before #84, with #83 fixed: the spelling as sent was compared +// with the prefix as written. That only adds denials; a disallowed image is +// still never forwarded. +module pull_pre84 { + import pull_model(READ_PULL_FROM_BODY = false, NORMALIZE_IMAGES = false, ALLOW_PULL_BODY = false).* + + run pre84UnsoundTest = + assert(and { + pullForwardsOnlyAllowedImages, + not(cliPullOfAllowedImageForwarded), + not(pullDecidedOnCanonicalImage), + routePull(cliPull("docker.io/library/alpine")) == "deny", + routePull(cliPull("alpine")) == "allowPull", + }) +} diff --git a/ts/src/handler.test.ts b/ts/src/handler.test.ts index 838353f..c5caca5 100644 --- a/ts/src/handler.test.ts +++ b/ts/src/handler.test.ts @@ -84,6 +84,38 @@ const beaconConfig = { "service_name: beacon\nallowed_image_prefixes:\n - chainsafe/lodestar\ncontainer_config:\n network_mode: host\n", }; +// #83: a pull with a body is never forwarded. The daemon reads a form body's +// fromImage over the query's, so an allowed image in the query and another in +// the body would pull the body's. Any body is denied, whatever its +// Content-Type. +describe("Handler image pull with a body (#83)", () => { + it("denies a pull that has a body", async () => { + const dir = makeEnv({ "probe.yaml": "service_name: probe\nallowed_image_prefixes:\n - alpine\n" }); + try { + const cases: [string, string][] = [ + ["application/x-www-form-urlencoded", "fromImage=busybox&tag=latest"], + ["application/json", '{"fromImage":"alpine"}'], + ["text/plain", "x"], + ]; + for (const [contentType, body] of cases) { + const { handler, transport } = newHandler(dir); + const { res, recorder } = makeResponse(); + await handler.handle( + makeRequest("POST", "/v1.56/images/create?fromImage=alpine&tag=3.19", body, { + "content-type": contentType, + }), + res, + ); + assert.equal(recorder.statusCode, 403, contentType); + assert.equal(recorder.body, "image pull must not have a request body", contentType); + assert.equal(transport.lastRequest, undefined, contentType); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe("Handler", () => { let tmpDir: string; @@ -387,3 +419,31 @@ describe("Handler", () => { } }); }); + +// #83: the handler hands the request target, query included, to the router. +// A pull sent the Docker CLI way, empty body and image in the query, is +// forwarded, and a disallowed one is not. +describe("Handler image pull (#83)", () => { + it("routes a pull on its query string", async () => { + const dir = makeEnv({ "probe.yaml": "service_name: probe\nallowed_image_prefixes:\n - alpine\n" }); + try { + let { handler, transport } = newHandler(dir); + let { res, recorder } = makeResponse(); + await handler.handle( + makeRequest("POST", "/v1.56/images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19"), + res, + ); + assert.equal(recorder.statusCode, 200); + assert.equal(transport.lastRequest?.url, "/v1.56/images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19"); + + ({ handler, transport } = newHandler(dir)); + ({ res, recorder } = makeResponse()); + await handler.handle(makeRequest("POST", "/v1.56/images/create?fromImage=busybox&tag=latest"), res); + assert.equal(recorder.statusCode, 403); + assert.equal(recorder.body, "image busybox not allowed by any policy"); + assert.equal(transport.lastRequest, undefined); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/ts/src/handler.ts b/ts/src/handler.ts index 314750e..07a4520 100644 --- a/ts/src/handler.ts +++ b/ts/src/handler.ts @@ -53,7 +53,7 @@ export class Handler { } } - const route = this.router.route(method, path, bodyJSON); + const route = this.router.route(method, path, bodyJSON, bodyBytes.length > 0); if (route.action === Action.Deny) { const msg = route.denyMsg ?? "denied"; diff --git a/ts/src/middleware.test.ts b/ts/src/middleware.test.ts index d2c691d..9d3cece 100644 --- a/ts/src/middleware.test.ts +++ b/ts/src/middleware.test.ts @@ -339,3 +339,34 @@ describe("Chain", () => { assert.deepEqual(mismatches, []); }); }); + +// Pins the registry gate's reference parser (#84): a registry port is not a +// tag, Docker Hub spellings match the normalised prefix, and the tag pattern +// sees only the tag. The policy's prefixes are normalised, as the manager +// stores them. Same rows in Go and Rust. +describe("RegistryGate reference parsing (#84)", () => { + it("parses registry ports, Docker Hub spellings and tags", () => { + const gate = new RegistryGate(); + const policy = makePolicy({ + allowed_image_prefixes: ["chainsafe/lodestar", "localhost:5000/team", "registry.example.com:443/a"], + image_tag_pattern: "^(v[0-9]+|[0-9.]+)$", + }); + const cases: [string, string | null][] = [ + ["localhost:5000/team/app:1.2", null], + ["localhost:5000/team/app", null], + ["docker.io/chainsafe/lodestar:v1", null], + ["registry.example.com:443/a", null], + ["Registry.Example.com:443/a:2", null], + ["localhost:5000/team/app:latest", 'image tag "latest" does not match pattern "^(v[0-9]+|[0-9.]+)$"'], + ["localhost:5000/other:1", 'image "localhost:5000/other" is not in allowed prefixes'], + ["chainsafe/Lodestar:v1", 'invalid image reference "chainsafe/Lodestar:v1"'], + ]; + const mismatches: string[] = []; + for (const [image, wantErr] of cases) { + const got = gate.check("POST", "/containers/create", policy, { Image: image }); + const ok = wantErr === null ? got === null : got !== null && got.includes(wantErr); + if (!ok) mismatches.push(`${image}: got ${got}, want ${wantErr}`); + } + assert.deepEqual(mismatches, []); + }); +}); diff --git a/ts/src/middleware.ts b/ts/src/middleware.ts index 3a7da45..1196386 100644 --- a/ts/src/middleware.ts +++ b/ts/src/middleware.ts @@ -1,4 +1,5 @@ -import type { Policy } from "./policy.js"; +import type { ImageRef, Policy } from "./policy.js"; +import { checkImageTag, matchesImagePrefix, parseImageRef } from "./policy.js"; export interface Gate { check(method: string, path: string, policy: Policy, body: Record): string | null; @@ -51,36 +52,17 @@ export class RegistryGate implements Gate { const image = body["Image"] as string | undefined; if (!image) return null; - const colonIdx = image.lastIndexOf(":"); - const atIdx = image.lastIndexOf("@"); - const sepIdx = atIdx !== -1 ? atIdx : colonIdx; - const tagOrDigest = sepIdx !== -1 && sepIdx > image.lastIndexOf("/") ? image.slice(sepIdx + 1) : ""; - const imageName = sepIdx !== -1 && sepIdx > image.lastIndexOf("/") ? image.slice(0, sepIdx) : image; - - const allowed = policy.allowed_image_prefixes.some( - (prefix) => imageName === prefix || imageName.startsWith(prefix + "/"), - ); - if (!allowed) return `image "${imageName}" is not in allowed prefixes`; - - if (tagOrDigest) { - if (tagOrDigest.length > 128) { - return `image tag/digest exceeds 128 characters`; - } - if (tagOrDigest.startsWith("sha256:")) { - if (!policy.image_digest_allowed) { - return `image digests not allowed for service ${policy.service_name}`; - } - if (!/^sha256:[a-f0-9]{64}$/.test(tagOrDigest)) { - return `invalid digest format: ${tagOrDigest}`; - } - } else if (policy.image_tag_pattern) { - if (!new RegExp(policy.image_tag_pattern).test(tagOrDigest)) { - return `image tag "${tagOrDigest}" does not match pattern "${policy.image_tag_pattern}"`; - } - } + // The same parser as the router, so a registry port is not a tag (#84). + let ref: ImageRef; + try { + ref = parseImageRef(image); + } catch (err) { + return (err as Error).message; } - - return null; + if (!matchesImagePrefix(ref.name, policy.allowed_image_prefixes)) { + return `image "${ref.name}" is not in allowed prefixes`; + } + return checkImageTag(policy, ref); } } diff --git a/ts/src/policy.test.ts b/ts/src/policy.test.ts index c000963..57b6fd7 100644 --- a/ts/src/policy.test.ts +++ b/ts/src/policy.test.ts @@ -3,7 +3,7 @@ import assert from "node:assert/strict"; import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { Manager } from "./policy.js"; +import { Manager, parseImageRef, type ImageRef } from "./policy.js"; describe("Manager", () => { let tmpDir: string; @@ -83,3 +83,72 @@ describe("Manager", () => { assert.equal(m.getByImage("redis:latest"), undefined); }); }); + +// The #84 normalisation table, on the parser itself. The same rows run in Go +// and Rust. +describe("parseImageRef (#84)", () => { + it("normalises references the way Docker does", () => { + const digest = "sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + const cases: [string, ImageRef | null][] = [ + ["alpine", { name: "alpine", tag: "", digest: "" }], + ["library/alpine", { name: "alpine", tag: "", digest: "" }], + ["docker.io/library/alpine", { name: "alpine", tag: "", digest: "" }], + ["index.docker.io/library/alpine:3.19", { name: "alpine", tag: "3.19", digest: "" }], + ["registry-1.docker.io/library/alpine", { name: "alpine", tag: "", digest: "" }], + ["docker.io/chainsafe/lodestar:v1", { name: "chainsafe/lodestar", tag: "v1", digest: "" }], + [`ghcr.io/chainsafe/lodestar@${digest}`, { name: "ghcr.io/chainsafe/lodestar", tag: "", digest }], + ["localhost:5000/x/y:2", { name: "localhost:5000/x/y", tag: "2", digest: "" }], + ["registry.example.com:443/a", { name: "registry.example.com:443/a", tag: "", digest: "" }], + ["REGISTRY.Example.com:443/a:V1", { name: "registry.example.com:443/a", tag: "V1", digest: "" }], + ["docker.io/library/a/b", { name: "library/a/b", tag: "", digest: "" }], + ["ghcr.io/library/alpine", { name: "ghcr.io/library/alpine", tag: "", digest: "" }], + ["localhost/x", { name: "localhost/x", tag: "", digest: "" }], + ["localhost", { name: "localhost", tag: "", digest: "" }], + [`alpine:3.19@${digest}`, { name: "alpine", tag: "3.19", digest }], + ["Alpine", null], + ["chainsafe/Lodestar", null], + ["alpine:", null], + ["alpine@", null], + ["", null], + ["a//b", null], + ["/alpine", null], + ["alpine/", null], + ["-alpine", null], + ["alpine:a/b", null], + ["a:b/c", null], + ["alpine:t$g", null], + ]; + const mismatches: string[] = []; + for (const [input, want] of cases) { + let got: ImageRef | string; + try { + got = parseImageRef(input); + } catch (err) { + got = (err as Error).message; + } + const ok = + want === null ? got === `invalid image reference "${input}"` : JSON.stringify(got) === JSON.stringify(want); + if (!ok) mismatches.push(`${JSON.stringify(input)}: got ${JSON.stringify(got)}, want ${JSON.stringify(want)}`); + } + assert.deepEqual(mismatches, []); + }); + + // Prefixes are normalised at load time, and a prefix with a tag or an + // invalid one fails the load (#84). + it("normalises policy prefixes at load time", () => { + const dir = mkdtempSync(join(tmpdir(), "policy-prefix-test-")); + try { + writeFileSync( + join(dir, "a.yaml"), + "service_name: a\nallowed_image_prefixes:\n - docker.io/library/alpine\n - GHCR.io/team\n", + ); + assert.deepEqual(new Manager(dir).get("a")?.allowed_image_prefixes, ["alpine", "ghcr.io/team"]); + for (const bad of ["alpine:3.19", "Alpine"]) { + writeFileSync(join(dir, "a.yaml"), `service_name: a\nallowed_image_prefixes:\n - "${bad}"\n`); + assert.throws(() => new Manager(dir), /invalid allowed_image_prefixes entry/, bad); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/ts/src/policy.ts b/ts/src/policy.ts index 4551dab..e458a7e 100644 --- a/ts/src/policy.ts +++ b/ts/src/policy.ts @@ -67,6 +67,14 @@ export class Manager { if (!policy.allowed_image_prefixes?.length) { throw new Error(`policy file ${entry} missing required field 'allowed_image_prefixes'`); } + // Prefixes are compared in normalised form (#84). + policy.allowed_image_prefixes = policy.allowed_image_prefixes.map((prefix) => { + try { + return normalizeImagePrefix(prefix); + } catch { + throw new Error(`policy file ${entry} has an invalid allowed_image_prefixes entry "${prefix}"`); + } + }); this.policiesByName.set(policy.service_name, policy); } @@ -76,15 +84,17 @@ export class Manager { return this.policiesByName.get(serviceName); } + // The policy whose prefixes match the reference's normalised repository + // (#84). An invalid reference matches nothing. getByImage(imageRef: string): Policy | undefined { - const imageName = extractImageName(imageRef); - + let name: string; + try { + name = parseImageRef(imageRef).name; + } catch { + return undefined; + } for (const policy of this.policiesByName.values()) { - for (const prefix of policy.allowed_image_prefixes) { - if (imageName === prefix || imageName.startsWith(prefix + "/")) { - return policy; - } - } + if (matchesImagePrefix(name, policy.allowed_image_prefixes)) return policy; } return undefined; } @@ -94,7 +104,108 @@ export class Manager { } } -function extractImageName(imageRef: string): string { - const idx = imageRef.search(/[:@]/); - return idx === -1 ? imageRef : imageRef.slice(0, idx); +// A parsed image reference (#84). `name` is the normalised repository: the +// registry host, lower-cased, and the path; Docker Hub's host is dropped, and +// so is `library/` before a single-component name, so `alpine`, +// `library/alpine` and `docker.io/library/alpine` are all `alpine`. `tag` and +// `digest` are empty when absent. +export interface ImageRef { + name: string; + tag: string; + digest: string; +} + +const DEFAULT_REGISTRIES = new Set(["docker.io", "index.docker.io", "registry-1.docker.io"]); +const DOMAIN_RE = + /^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?(?:\.[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)*(?::[0-9]+)?$/; +const PATH_COMPONENT_RE = /^[a-z0-9]+(?:(?:[._]|__|-+)[a-z0-9]+)*$/; +const TAG_RE = /^[A-Za-z0-9_][A-Za-z0-9_.-]*$/; +const SHA256_DIGEST_RE = /^sha256:[a-f0-9]{64}$/; + +// Parses `[registry/]repository[:tag][@digest]` the way Docker does (#84). +// The digest follows the first `@`; the tag follows the last `:` after the +// last `/`, so a registry port is not a tag. The first path component is a +// registry host when it contains `.` or `:` or is `localhost`. Only the host +// is lower-cased: upper case in the path is invalid. The digest is checked by +// checkImageTag, not here. Throws on an invalid reference. +export function parseImageRef(reference: string): ImageRef { + const invalid = () => new Error(`invalid image reference "${reference}"`); + let rest = reference; + let digest = ""; + const at = reference.indexOf("@"); + if (at !== -1) { + digest = reference.slice(at + 1); + rest = reference.slice(0, at); + if (!digest) throw invalid(); + } + const afterSlash = rest.lastIndexOf("/") + 1; + const colon = rest.slice(afterSlash).lastIndexOf(":"); + let name = rest; + let tag = ""; + if (colon !== -1) { + tag = rest.slice(afterSlash + colon + 1); + name = rest.slice(0, afterSlash + colon); + if (!TAG_RE.test(tag)) throw invalid(); + } + const normalized = normalizeName(name); + if (normalized === undefined) throw invalid(); + return { name: normalized, tag, digest }; +} + +function normalizeName(name: string): string | undefined { + let domain: string | undefined; + let path = name; + const slash = name.indexOf("/"); + if (slash !== -1) { + const first = name.slice(0, slash); + if (first.includes(".") || first.includes(":") || first === "localhost") { + domain = first.toLowerCase(); + path = name.slice(slash + 1); + } + } + if (domain !== undefined && !DOMAIN_RE.test(domain)) return undefined; + if (!path.split("/").every((c) => PATH_COMPONENT_RE.test(c))) return undefined; + if (domain !== undefined && DEFAULT_REGISTRIES.has(domain)) domain = undefined; + let full: string; + if (domain !== undefined) { + full = `${domain}/${path}`; + } else if (path.startsWith("library/") && !path.slice("library/".length).includes("/")) { + full = path.slice("library/".length); + } else { + full = path; + } + return full.length > 255 ? undefined : full; +} + +// Normalises a policy prefix at load time (#84). A prefix names a repository +// or a namespace, so a tag or digest makes it invalid. Throws when invalid. +export function normalizeImagePrefix(prefix: string): string { + const r = parseImageRef(prefix); + if (r.tag || r.digest) throw new Error(`image prefix "${prefix}" has a tag or digest`); + return r.name; +} + +// Matches a normalised repository against normalised prefixes on namespace +// boundaries ("prefix" or "prefix/..."), never a bare substring. +export function matchesImagePrefix(name: string, prefixes: string[]): boolean { + return prefixes.some((p) => name === p || name.startsWith(p + "/")); +} + +// Applies a policy's digest and tag rules to a parsed reference: the digest +// must be allowed and well-formed, the tag must match image_tag_pattern. +// Shared by the registry gate (create) and the router (pull, #83). Returns +// the deny reason, or null. +export function checkImageTag(policy: Policy, r: ImageRef): string | null { + if (r.digest) { + if (r.digest.length > 128) return "image tag/digest exceeds 128 characters"; + if (!policy.image_digest_allowed) return `image digests not allowed for service ${policy.service_name}`; + if (!SHA256_DIGEST_RE.test(r.digest)) return `invalid digest format: ${r.digest}`; + } + if (r.tag) { + if (r.tag.length > 128) return "image tag/digest exceeds 128 characters"; + if (policy.image_tag_pattern && !new RegExp(policy.image_tag_pattern).test(r.tag)) { + return `image tag "${r.tag}" does not match pattern "${policy.image_tag_pattern}"`; + } + } + return null; } diff --git a/ts/src/proxy.test.ts b/ts/src/proxy.test.ts index 860653a..89897c4 100644 --- a/ts/src/proxy.test.ts +++ b/ts/src/proxy.test.ts @@ -137,21 +137,6 @@ describe("Router", () => { assert.equal(r.action, Action.Allow); }); - it("allows POST /images/create for allowed image", () => { - const r = router.route("POST", "/images/create", { fromImage: "nginx:latest" }); - assert.equal(r.action, Action.Allow); - }); - - it("denies POST /images/create for unknown image", () => { - const r = router.route("POST", "/images/create", { fromImage: "unknown:latest" }); - assert.equal(r.action, Action.Deny); - }); - - it("denies POST /images/create with empty body", () => { - const r = router.route("POST", "/images/create", {}); - assert.equal(r.action, Action.Deny); - }); - it("allows GET passthrough for unknown paths", () => { const r = router.route("GET", "/containers/foo/logs"); assert.equal(r.action, Action.Allow); @@ -339,7 +324,6 @@ describe("Router", () => { // two segments. Rows mirror the exec* and create* runs in spec/router.qnt. it("matches exec and create endpoints on whole path segments", () => { const createBody = { Image: "nginx:latest" }; - const pullBody = { fromImage: "nginx:latest" }; const exec = "exec is not allowed"; const cases: [string, string, Record | undefined, Action, string | undefined][] = [ // execSubpathDeleteDeniedTest (#49) @@ -361,7 +345,7 @@ describe("Router", () => { // Exec is matched under containers or exec only (#49, language-only). ["GET", "/images/exec", undefined, Action.Allow, undefined], // An allowed image, so only the path decides (#49, language-only). - ["POST", "/images/create/extra", pullBody, Action.Deny, undefined], + ["POST", "/images/create/extra?fromImage=nginx&tag=latest", undefined, Action.Deny, undefined], // A name ending in exec is a plain name (#49, language-only). ["GET", "/containers/myexec/json", undefined, Action.Allow, undefined], // A name starting with exec is a plain name (#49, language-only). @@ -457,7 +441,6 @@ describe("Router", () => { const ro = new Router(m).withReadonly(true); const post = "read-only mode: POST requests are not allowed"; const createBody = { Image: "chainsafe/lodestar:latest" }; - const pullBody = { fromImage: "chainsafe/lodestar", tag: "latest" }; const cases: [string, string, Record | undefined, Action, string | undefined][] = [ // readonlyGetAllowedTest (#75) ["GET", "/containers/json", undefined, Action.Allow, undefined], @@ -472,7 +455,7 @@ describe("Router", () => { // readonlyDeleteDeniedTest (#75) ["DELETE", "/containers/mycontainer", undefined, Action.Deny, "read-only mode: DELETE requests are not allowed"], // #75: an image pull of an allowed image. - ["POST", "/images/create?fromImage=chainsafe/lodestar&tag=latest", pullBody, Action.Deny, post], + ["POST", "/images/create?fromImage=chainsafe%2Flodestar&tag=latest", undefined, Action.Deny, post], // readonlyCreateDeniedTest (#75): an allowed image, so only the flag decides. ["POST", "/containers/create", createBody, Action.Deny, post], // #75: methods outside the spec's universe. @@ -639,3 +622,199 @@ describe("Router", () => { assert.deepEqual(mismatches, []); }); }); + +const TEST_DIGEST = "sha256:abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + +// The policy set for the #83 and #84 tables. beacon's prefixes are written in +// spellings that normalisation must reduce at load time: +// docker.io/chainsafe/lodestar is chainsafe/lodestar, and the registry host +// Registry.Example.com:443 is lower-cased. +function createPullManager(dir: string): Manager { + writeFileSync( + join(dir, "probe.yaml"), + "service_name: probe\nallowed_image_prefixes:\n - alpine\nimage_digest_allowed: true\n", + ); + writeFileSync( + join(dir, "beacon.yaml"), + "service_name: beacon\nallowed_image_prefixes:\n - docker.io/chainsafe/lodestar\n - ghcr.io/chainsafe/lodestar\n - localhost:5000/x\n - Registry.Example.com:443/a\nimage_tag_pattern: '^(v[0-9]+|[0-9]+)$'\n", + ); + return new Manager(dir); +} + +describe("Router image pull and normalisation (#83, #84)", () => { + let tmpDir: string; + let router: Router; + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), "proxy-pull-test-")); + router = new Router(createPullManager(tmpDir)); + }); + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }); + }); + + // Cross-language parity guard for #83 and #84. POST /images/create is + // routed on its query string, the way the Docker CLI sends it: fromImage + // (fully qualified, URL-encoded) and tag, with an empty body. The body is + // ignored, fromSrc (import) is denied, and the image gets the create checks: + // a policy prefix on the normalised repository, then that policy's tag + // pattern and digest rule. Rows mirror the pull* runs in spec/router.qnt; + // the rest are language-only. + it("routes POST /images/create on the query string", () => { + const alpineBody = { fromImage: "alpine" }; + const notAllowed = (image: string) => `image ${image} not allowed by any policy`; + const required = "fromImage query parameter is required for image pull"; + const fromSrc = "image import (fromSrc) is not allowed"; + const bodyMsg = "image pull must not have a request body"; + const encDigest = encodeURIComponent(TEST_DIGEST); + const cases: [string, Record | undefined, Action, string, string][] = [ + // pullCliAllowedTest (#83): the issue's reproduction, as the CLI sends it. + ["/v1.56/images/create?fromImage=docker.io%2Flibrary%2Falpine&tag=3.19", undefined, Action.Allow, "probe", ""], + // pullShortNameAllowedTest (#84) + ["/images/create?fromImage=alpine&tag=3.19", undefined, Action.Allow, "probe", ""], + // pullLibraryNameAllowedTest (#84) + ["/images/create?fromImage=library%2Falpine&tag=latest", undefined, Action.Allow, "probe", ""], + // pullCliOrgAllowedTest (#84): the prefix is written docker.io/chainsafe/lodestar. + ["/images/create?fromImage=docker.io%2Fchainsafe%2Flodestar&tag=v1", undefined, Action.Allow, "beacon", ""], + // #84: the tag in fromImage, on another Docker Hub host name. + ["/images/create?fromImage=index.docker.io%2Flibrary%2Falpine%3A3.19", undefined, Action.Allow, "probe", ""], + // #83: the CLI sends a digest in tag. + [`/images/create?fromImage=alpine&tag=${encDigest}`, undefined, Action.Allow, "probe", ""], + // #83: a digest in fromImage. + [`/images/create?fromImage=alpine%40${encDigest}`, undefined, Action.Allow, "probe", ""], + // #83: image_digest_allowed is false for beacon. + [ + `/images/create?fromImage=ghcr.io%2Fchainsafe%2Flodestar&tag=${encDigest}`, + undefined, + Action.Deny, + "", + "image digests not allowed for service beacon", + ], + // #84: a registry port is not a tag. + ["/images/create?fromImage=localhost%3A5000%2Fx%2Fy&tag=2", undefined, Action.Allow, "beacon", ""], + // #84: the registry host is compared lower-cased. + ["/images/create?fromImage=REGISTRY.example.com%3A443%2Fa&tag=1", undefined, Action.Allow, "beacon", ""], + // #83: the tag pattern applies to pulls. + [ + "/images/create?fromImage=chainsafe%2Flodestar&tag=latest", + undefined, + Action.Deny, + "", + 'image tag "latest" does not match pattern "^(v[0-9]+|[0-9]+)$"', + ], + // pullDisallowedCliDeniedTest (#83) + [ + "/images/create?fromImage=docker.io%2Fattacker%2Fmalware&tag=latest", + undefined, + Action.Deny, + "", + notAllowed("docker.io/attacker/malware"), + ], + // pullOtherRegistryDeniedTest (#84): another registry is not Docker Hub's alpine. + [ + "/images/create?fromImage=localhost%3A5000%2Falpine&tag=1", + undefined, + Action.Deny, + "", + notAllowed("localhost:5000/alpine"), + ], + // #84: the prefix match stays on namespace boundaries. + ["/images/create?fromImage=alpine-evil&tag=1", undefined, Action.Deny, "", notAllowed("alpine-evil")], + // pullNoFromImageDeniedTest (#83) + ["/images/create", undefined, Action.Deny, "", required], + // #83: the old contract, fromImage in a JSON body: a pull has no body. + ["/images/create", { fromImage: "alpine:3.19" }, Action.Deny, "", bodyMsg], + // pullBodyIgnoredDeniedTest (#83): the daemon would pull the query's image. + ["/images/create?fromImage=attacker%2Fmalware&tag=latest", alpineBody, Action.Deny, "", bodyMsg], + // pullJsonBodyDeniedTest (#83): any body is denied, even one that agrees. + ["/images/create?fromImage=alpine&tag=3.19", alpineBody, Action.Deny, "", bodyMsg], + // pullImportDeniedTest (#83): the daemon would import the tarball as alpine:latest. + [ + "/images/create?fromSrc=http%3A%2F%2Fexample.com%2Fx.tar&repo=alpine&tag=latest", + undefined, + Action.Deny, + "", + fromSrc, + ], + // pullImportWithImageDeniedTest (#83) + ["/images/create?fromImage=alpine&tag=latest&fromSrc=-", undefined, Action.Deny, "", fromSrc], + // #83: a tag in fromImage and in tag is ambiguous. + [ + "/images/create?fromImage=alpine%3A3.19&tag=3.20", + undefined, + Action.Deny, + "", + "fromImage alpine:3.19 already has a tag or digest; the tag query parameter is ambiguous", + ], + // #83: no tag and no digest pulls every tag. + [ + "/images/create?fromImage=alpine", + undefined, + Action.Deny, + "", + "image pull without a tag or digest pulls every tag and is not allowed", + ], + // #84: upper case in the repository path is invalid. + ["/images/create?fromImage=Alpine&tag=latest", undefined, Action.Deny, "", 'invalid image reference "Alpine"'], + // #83: a repeated parameter could be read two ways. + [ + "/images/create?fromImage=alpine&fromImage=attacker%2Fmalware&tag=1", + undefined, + Action.Deny, + "", + "duplicate fromImage query parameter", + ], + // #83: the daemon rejects a malformed query; so does the proxy. + ["/images/create?fromImage=alpine%zz&tag=1", undefined, Action.Deny, "", "invalid query string"], + // #53 still holds: the percent check reads the path, not the query. + ["/images/%63reate?fromImage=alpine&tag=1", undefined, Action.Deny, "", "percent-encoded path not allowed"], + ]; + const mismatches: string[] = []; + for (const [path, body, want, service, wantMsg] of cases) { + const r = router.route("POST", path, body); + if (r.action !== want || (r.service ?? "") !== service || (r.denyMsg ?? "") !== wantMsg) { + mismatches.push( + `POST ${path}: got ${Action[r.action]}/${r.service ?? ""}/${r.denyMsg ?? ""}, want ${Action[want]}/${service}/${wantMsg}`, + ); + } + } + assert.deepEqual(mismatches, []); + }); + + // The normalisation table for #84, on the create route: each spelling + // selects the policy its normalised repository matches, and a path with + // upper case is invalid. The same table runs in Go and Rust. + it("normalises image references before policy matching", () => { + const notAllowed = (image: string) => `image ${image} not allowed by any policy`; + const cases: [string, Action, string, string][] = [ + ["alpine", Action.CreateContainer, "probe", ""], + ["library/alpine", Action.CreateContainer, "probe", ""], + ["docker.io/library/alpine", Action.CreateContainer, "probe", ""], + ["docker.io/alpine", Action.CreateContainer, "probe", ""], + ["index.docker.io/library/alpine:3.19", Action.CreateContainer, "probe", ""], + ["docker.io/chainsafe/lodestar:v1", Action.CreateContainer, "beacon", ""], + ["chainsafe/lodestar:v1", Action.CreateContainer, "beacon", ""], + ["registry-1.docker.io/chainsafe/lodestar:v1", Action.CreateContainer, "beacon", ""], + [`ghcr.io/chainsafe/lodestar@${TEST_DIGEST}`, Action.CreateContainer, "beacon", ""], + ["localhost:5000/x/y:2", Action.CreateContainer, "beacon", ""], + ["registry.example.com:443/a", Action.CreateContainer, "beacon", ""], + ["Alpine:latest", Action.Deny, "", 'invalid image reference "Alpine:latest"'], + ["chainsafe/Lodestar:v1", Action.Deny, "", 'invalid image reference "chainsafe/Lodestar:v1"'], + ["alpine-evil", Action.Deny, "", notAllowed("alpine-evil")], + ["ghcr.io/library/alpine", Action.Deny, "", notAllowed("ghcr.io/library/alpine")], + ["ghcr.io/chainsafe/lodestar-evil", Action.Deny, "", notAllowed("ghcr.io/chainsafe/lodestar-evil")], + ["localhost:5000/xy:1", Action.Deny, "", notAllowed("localhost:5000/xy:1")], + ]; + const mismatches: string[] = []; + for (const [image, want, service, wantMsg] of cases) { + const r = router.route("POST", "/containers/create", { Image: image }); + if (r.action !== want || (r.service ?? "") !== service || (r.denyMsg ?? "") !== wantMsg) { + mismatches.push( + `create ${image}: got ${Action[r.action]}/${r.service ?? ""}/${r.denyMsg ?? ""}, want ${Action[want]}/${service}/${wantMsg}`, + ); + } + } + assert.deepEqual(mismatches, []); + }); +}); diff --git a/ts/src/proxy.ts b/ts/src/proxy.ts index 92a2c0a..02a104b 100644 --- a/ts/src/proxy.ts +++ b/ts/src/proxy.ts @@ -1,5 +1,5 @@ -import type { Policy } from "./policy.js"; -import { Manager } from "./policy.js"; +import type { ImageRef, Policy } from "./policy.js"; +import { Manager, checkImageTag, parseImageRef } from "./policy.js"; export enum Action { Deny, @@ -28,13 +28,18 @@ export class Router { return this; } - route(method: string, path: string, body?: Record): RouteResult { + // Routes a request. path is the raw request target: the path, still + // percent-encoded, and the query string, if any. hasBody reports a + // non-empty raw body of any Content-Type (by default, a parsed JSON body); + // body is that body parsed as JSON, if it was. + route(method: string, path: string, body?: Record, hasBody = body !== undefined): RouteResult { // Read-only mode denies every write, for every path (#75). if (this.readonlyMode && method !== "GET" && method !== "HEAD") { return { action: Action.Deny, denyMsg: `read-only mode: ${method} requests are not allowed` }; } const qmIdx = path.indexOf("?"); const cleanPath = qmIdx !== -1 ? path.slice(0, qmIdx) : path; + const query = qmIdx !== -1 ? path.slice(qmIdx + 1) : ""; // The daemon decodes the path before routing, so deny any escape (#53). if (cleanPath.includes("%")) return { action: Action.Deny, denyMsg: "percent-encoded path not allowed" }; // An empty interior segment can hide a denied prefix such as /exec (#55). @@ -75,7 +80,7 @@ export class Router { // Image pull if (method === "POST" && path === "/images/create") { - return this.routeImagePull(body); + return this.routeImagePull(query, hasBody); } // Default GET/HEAD passthrough @@ -95,6 +100,11 @@ export class Router { if (!image) { return { action: Action.Deny, denyMsg: "image field is required" }; } + try { + parseImageRef(image); + } catch (err) { + return { action: Action.Deny, denyMsg: (err as Error).message }; + } const policy = this.manager.getByImage(image); if (!policy) { @@ -112,23 +122,85 @@ export class Router { return { action: Action.Allow, container: name }; } - private routeImagePull(body?: Record): RouteResult { - if (!body || !Object.keys(body).length) { - return { action: Action.Deny, denyMsg: "empty request body" }; + // Image pull (#83). The Docker CLI sends fromImage and tag in the query + // string and no body. The image then + // gets the create checks: a policy prefix on the normalised repository + // (#84), and that policy's tag and digest rules. + private routeImagePull(rawQuery: string, hasBody: boolean): RouteResult { + const deny = (denyMsg: string): RouteResult => ({ action: Action.Deny, denyMsg }); + // The daemon reads a form body's fromImage over the query's, so a body + // could name any image: a pull has no body, whatever its Content-Type. + if (hasBody) return deny("image pull must not have a request body"); + const params = parseQuery(rawQuery); + if (!params) return deny("invalid query string"); + for (const key of ["fromImage", "tag", "fromSrc"]) { + if (params.filter(([k]) => k === key).length > 1) return deny(`duplicate ${key} query parameter`); } - - const fromImage = body["fromImage"] as string | undefined; - if (!fromImage) { - return { action: Action.Deny, denyMsg: "fromImage field is required for image pull" }; + const get = (key: string) => params.find(([k]) => k === key)?.[1]; + if (get("fromSrc") !== undefined) return deny("image import (fromSrc) is not allowed"); + const fromImage = get("fromImage") ?? ""; + if (!fromImage) return deny("fromImage query parameter is required for image pull"); + let ref: ImageRef; + try { + ref = parseImageRef(fromImage); + const tag = get("tag") ?? ""; + if (tag) { + if (ref.tag || ref.digest) { + return deny(`fromImage ${fromImage} already has a tag or digest; the tag query parameter is ambiguous`); + } + // The CLI sends a digest in tag; a tag never contains ':'. + ref = parseImageRef(fromImage + (tag.includes(":") ? "@" : ":") + tag); + } + } catch (err) { + return deny((err as Error).message); } + // With neither, the daemon pulls every tag of the repository. + if (!ref.tag && !ref.digest) return deny("image pull without a tag or digest pulls every tag and is not allowed"); + const policy = this.manager.getByImage(ref.name); + if (!policy) return deny(`image ${fromImage} not allowed by any policy`); + const reason = checkImageTag(policy, ref); + if (reason !== null) return deny(reason); + return { action: Action.Allow, service: policy.service_name, image: fromImage }; + } +} - const policy = this.manager.getByImage(fromImage); - if (!policy) { - return { action: Action.Deny, denyMsg: `image ${fromImage} not allowed by any policy` }; - } +// Parses a query string as Go's url.ParseQuery does, which is what the daemon +// uses: & separates pairs, + is a space, and a ; or a bad % escape fails the +// whole query (undefined). +function parseQuery(raw: string): [string, string][] | undefined { + const out: [string, string][] = []; + for (const part of raw.split("&")) { + if (part.includes(";")) return undefined; + if (!part) continue; + const eq = part.indexOf("="); + const k = queryUnescape(eq === -1 ? part : part.slice(0, eq)); + const v = queryUnescape(eq === -1 ? "" : part.slice(eq + 1)); + if (k === undefined || v === undefined) return undefined; + out.push([k, v]); + } + return out; +} - return { action: Action.Allow, image: fromImage }; +function queryUnescape(s: string): string | undefined { + const bytes: number[] = []; + for (let i = 0; i < s.length;) { + const c = s[i]; + if (c === "%") { + const hex = s.slice(i + 1, i + 3); + if (!/^[0-9A-Fa-f]{2}$/.test(hex)) return undefined; + bytes.push(parseInt(hex, 16)); + i += 3; + } else if (c === "+") { + bytes.push(0x20); + i += 1; + } else { + const cp = s.codePointAt(i)!; + const ch = String.fromCodePoint(cp); + bytes.push(...Buffer.from(ch, "utf-8")); + i += ch.length; + } } + return Buffer.from(bytes).toString("utf-8"); } function stripAPIVersion(path: string): string {