From 510ee692ba015ebddd0fa586872ec0eb65c32e03 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Thu, 10 Sep 2026 14:19:53 +0200 Subject: [PATCH] feat: address the review on the split-deployment branch by moving the two broker routes onto their own NODE_BROKER_TOKEN so the internet-facing tracking and forms services no longer hold a credential that can open any organization's data key, refusing to presign any key outside the prefixes a node reaches, fixing IAM policies that named an alias ARN KMS never resolves in a Resource element, bounding both brokered HTTP clients because the sync loop's context never expires, no longer reporting a 403 from the object store as a missing body, and redacting the DSN and URL credentials the dry-run listing printed in clear --- AGENTS.md | 1 + deploy/split-cloud/bus/certbot-deploy-hook.sh | 22 ++++++- deploy/split-cloud/bus/docker-compose.yml | 11 +++- deploy/split-cloud/control-plane.env.example | 6 ++ deploy/split-cloud/node/worker.env.example | 2 + .../docs/development/configuration.mdx | 1 + .../docs/development/split-deployment.mdx | 4 ++ internal/api/handler/fleet_nodes.go | 7 +++ internal/api/handler/internal_blobs.go | 39 ++++++++++++ internal/api/handler/nodescript/join.sh | 29 ++++++++- internal/api/middleware/internal_auth.go | 50 +++++++++++++++ internal/api/routes.go | 28 +++++---- internal/app/instancecheck/checks_fleet.go | 36 ++++++++++- .../app/instancecheck/checks_fleet_test.go | 38 ++++++++++++ internal/infrastructure/kms/brokered.go | 4 +- internal/infrastructure/kms/factory.go | 8 ++- internal/infrastructure/storage/brokered.go | 58 ++++++++++++------ .../infrastructure/storage/brokered_test.go | 61 ++++++++++++++++--- internal/infrastructure/storage/factory.go | 8 ++- scripts/aws-bootstrap.sh | 14 ++++- scripts/check-join-script.sh | 35 +++++++++++ 21 files changed, 410 insertions(+), 52 deletions(-) create mode 100644 internal/app/instancecheck/checks_fleet_test.go diff --git a/AGENTS.md b/AGENTS.md index ef404fad..b3510102 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -406,6 +406,7 @@ Design intent: - relational data the worker needs (encrypted DEKs, the messageId→internal-email map) is reached over the backend's internal HTTP API (`/api/v1/internal/...`), never via direct SQL - worker-local state should be minimal and disposable - **a node holds no cloud credential.** The two privileged operations it needs are brokered through the internal API: `KMS_PROVIDER=brokered` posts sealed keys to `/api/v1/internal/dek/decrypt` and `BLOB_PROVIDER=brokered` asks `/api/v1/internal/blobs/presign` to sign one operation on one key. `renderNodeEnv` translates `aws`/`s3` into these automatically when rendering a node's env, so an IAM key never reaches a machine in the fleet. Blob bytes still travel node↔store directly; only the signature comes from the control plane +- those two routes are the one place the internal API hands out something that is worth more than a record, so they take `NODE_BROKER_TOKEN` (falling back to `INTERNAL_API_TOKEN`) rather than the token the internet-facing tracking and forms services also carry, and presign refuses any key outside `nodeKeyPrefixes`. Extend that list when a node starts touching a new prefix; a signed URL is the whole authorisation Current code matches that intent in `cmd/worker/main.go`: the worker boots Kafka, Redis cache, KMS, and S3 clients, and reaches DEKs + the email message map through the backend's internal API, but does not open a PostgreSQL connection. diff --git a/deploy/split-cloud/bus/certbot-deploy-hook.sh b/deploy/split-cloud/bus/certbot-deploy-hook.sh index ace1a4cd..d8eceda5 100644 --- a/deploy/split-cloud/bus/certbot-deploy-hook.sh +++ b/deploy/split-cloud/bus/certbot-deploy-hook.sh @@ -8,8 +8,20 @@ # neither container can read: NATS runs as uid 1000 and Redis as uid 999. The # copy exists to widen that deliberately and in one place, rather than by # loosening /etc/letsencrypt. +# +# It is widened to a group, not to the world. The compose file puts both +# containers in gid 2000 with group_add, so each can read the key and nothing +# else on the machine can. Create it once: +# +# groupadd -g 2000 warmbly-certs +# +# This matters more than it looks: the same guide suggests running the first +# worker on this box, and a world-readable private key is readable by that +# container too. set -eu +CERT_GID="${WARMBLY_CERT_GID:-2000}" + DOMAIN="${WARMBLY_BUS_DOMAIN:-bus.example.com}" SRC="/etc/letsencrypt/live/$DOMAIN" DEST="/opt/warmbly/certs" @@ -21,10 +33,14 @@ cp "$SRC/fullchain.pem" "$DEST/fullchain.pem" cp "$SRC/chain.pem" "$DEST/chain.pem" cp "$SRC/privkey.pem" "$DEST/privkey.pem" -# World-readable on a box whose only job is this. Narrow it to a shared group -# if anything else ever runs here. +# The chain is public by definition. The key is not: group-readable only, and +# only for the group the two containers are added to. chmod 0644 "$DEST/fullchain.pem" "$DEST/chain.pem" -chmod 0644 "$DEST/privkey.pem" +chgrp "$CERT_GID" "$DEST/privkey.pem" || { + echo "no group $CERT_GID; create it with: groupadd -g $CERT_GID warmbly-certs" >&2 + exit 1 +} +chmod 0640 "$DEST/privkey.pem" # Both hold the certificate open and neither re-reads it on its own. cd /opt/warmbly/bus && docker compose restart nats redis diff --git a/deploy/split-cloud/bus/docker-compose.yml b/deploy/split-cloud/bus/docker-compose.yml index 9de01984..e4162c7a 100644 --- a/deploy/split-cloud/bus/docker-compose.yml +++ b/deploy/split-cloud/bus/docker-compose.yml @@ -8,8 +8,9 @@ # Before the first start: # 1. point bus.example.com at this machine # 2. get a certificate for it (certbot certonly --standalone -d bus.example.com) -# 3. install certbot-deploy-hook.sh so renewals land in /opt/warmbly/certs -# 4. write .env next to this file with NATS_TOKEN and REDIS_PASSWORD +# 3. groupadd -g 2000 warmbly-certs, the group that may read the private key +# 4. install certbot-deploy-hook.sh so renewals land in /opt/warmbly/certs +# 5. write .env next to this file with NATS_TOKEN and REDIS_PASSWORD # # NATS_TOKEN=$(openssl rand -hex 32) # REDIS_PASSWORD=$(openssl rand -hex 32) @@ -21,6 +22,10 @@ services: command: ["-c", "/etc/nats/nats.conf", "-js", "-sd", "/data", "-m", "8222"] environment: NATS_TOKEN: ${NATS_TOKEN:?set NATS_TOKEN in .env} + # The private key is 0640 and owned by this group, so neither container + # needs it world-readable and nothing else on the box can read it. + group_add: + - "2000" volumes: - ./nats.conf:/etc/nats/nats.conf:ro - /opt/warmbly/certs:/certs:ro @@ -54,6 +59,8 @@ services: --requirepass ${REDIS_PASSWORD:?set REDIS_PASSWORD in .env} --appendonly yes --maxmemory-policy noeviction + group_add: + - "2000" volumes: - /opt/warmbly/certs:/certs:ro - redis_data:/data diff --git a/deploy/split-cloud/control-plane.env.example b/deploy/split-cloud/control-plane.env.example index 8384d65e..bfa0e30a 100644 --- a/deploy/split-cloud/control-plane.env.example +++ b/deploy/split-cloud/control-plane.env.example @@ -37,6 +37,12 @@ CREDENTIALS_ENCRYPTION_KEY= INTERNAL_API_TOKEN= SECRET_KEY_BASE= +# The credential for the two routes that open a sealed key and sign a blob +# operation. Falls back to INTERNAL_API_TOKEN when unset; set it to its own +# value here, because the tracking and forms services are internet-facing and +# carry the shared one. Nodes receive this, the edge services do not. +NODE_BROKER_TOKEN= + # --- storage ---------------------------------------------------------------- # s3, not filesystem: a worker on another machine reads the message body the # backend wrote, and it does not have your disk. diff --git a/deploy/split-cloud/node/worker.env.example b/deploy/split-cloud/node/worker.env.example index 004f5e16..f4b58b38 100644 --- a/deploy/split-cloud/node/worker.env.example +++ b/deploy/split-cloud/node/worker.env.example @@ -24,6 +24,8 @@ ENCRYPTED_KEYS_PROVIDER=http ENCRYPTED_KEYS_BACKEND_URL=https://api.example.com ENCRYPTED_KEYS_WORKER_TOKEN= INTERNAL_API_TOKEN= +# Only when the control plane sets one. Used for the brokered providers below. +NODE_BROKER_TOKEN= # --- the bus and cache ------------------------------------------------------ # TLS on both: the bus carries recipient addresses and the cache carries each diff --git a/docs/content/docs/development/configuration.mdx b/docs/content/docs/development/configuration.mdx index 99032f60..8db5b204 100644 --- a/docs/content/docs/development/configuration.mdx +++ b/docs/content/docs/development/configuration.mdx @@ -57,6 +57,7 @@ Five values protect the whole instance. Compose ships a working default for each |---|---|---|---| | `AUTH_SECRET` | 32 characters or more | JWT and session signing. The realtime service reads the same value as `JWT_SECRET` | yes | | `INTERNAL_API_TOKEN` | any random string | The backend's `/api/v1/internal/` routes, which workers and the tracking service authenticate against | yes | +| `NODE_BROKER_TOKEN` | any random string | The two routes that perform a privileged operation for the caller: opening a sealed data key and signing a blob operation. Optional, and falls back to `INTERNAL_API_TOKEN`. Worth setting in a split deployment, where the tracking and forms services are internet-facing and hold the shared token | yes | | `SECRET_KEY_BASE` | 64 characters or more | Phoenix session signing in the realtime service | yes | | `KMS_LOCAL_MASTER_KEY` | base64, exactly 32 bytes | The root key that seals every per-organization data key | yes | | `CREDENTIALS_ENCRYPTION_KEY` | exactly 64 hex characters | Mailbox credentials at rest: SMTP and IMAP passwords, and Gmail and Outlook OAuth access and refresh tokens | yes | diff --git a/docs/content/docs/development/split-deployment.mdx b/docs/content/docs/development/split-deployment.mdx index ece297cd..fe411f60 100644 --- a/docs/content/docs/development/split-deployment.mdx +++ b/docs/content/docs/development/split-deployment.mdx @@ -67,6 +67,10 @@ Blob bytes are not proxied. The control plane signs a URL and the node transfers The cost is one HTTPS call per DEK open, which Redis caches, and one per blob operation. If you would rather a node talk to AWS directly, put `KMS_PROVIDER`, `BLOB_PROVIDER` and the credentials in `/etc/warmbly/node.local.env`, which a re-join does not overwrite. +Two limits are worth knowing. A signed URL covers one verb on one key, and only for the prefixes a node reaches: transport bodies, campaign attachments, and the mailbox bodies a sync stores. Avatars, form assets and workspace export archives cannot be signed for at all, so the credential a node holds is not a credential for the bucket. + +That credential is `NODE_BROKER_TOKEN`. It falls back to `INTERNAL_API_TOKEN`, which is what a single-machine install uses, but set it to its own value here: the tracking and forms services are internet-facing and carry the shared token, and there is no reason for what they hold to be enough to open a data key. + ## Building it diff --git a/internal/api/handler/fleet_nodes.go b/internal/api/handler/fleet_nodes.go index f9136033..2acaa9db 100644 --- a/internal/api/handler/fleet_nodes.go +++ b/internal/api/handler/fleet_nodes.go @@ -308,6 +308,13 @@ func renderNodeEnv(nodeID uuid.UUID, role models.NodeRole, region string) string fmt.Fprintf(&b, "KMS_PROVIDER=%s\n", kmsProvider) fmt.Fprintf(&b, "BLOB_PROVIDER=%s\n", blobProvider) + // The credential for the two endpoints that open a key and sign a blob + // operation. Sent only when the instance issues a separate one; otherwise + // the node falls back to the internal token it already has. + if v := os.Getenv("NODE_BROKER_TOKEN"); v != "" { + fmt.Fprintf(&b, "NODE_BROKER_TOKEN=%s\n", v) + } + for _, k := range nodeEnvKeys { if v := os.Getenv(k); v != "" { fmt.Fprintf(&b, "%s=%s\n", k, v) diff --git a/internal/api/handler/internal_blobs.go b/internal/api/handler/internal_blobs.go index 5eaf53e8..f0d54806 100644 --- a/internal/api/handler/internal_blobs.go +++ b/internal/api/handler/internal_blobs.go @@ -3,6 +3,7 @@ package handler import ( "errors" "net/http" + "strings" "time" "github.com/gin-gonic/gin" @@ -22,6 +23,38 @@ import ( // 200 {"url":"...","method":"GET","expires_in":300} // 501 the instance's blob backend cannot sign (filesystem) +// nodeKeyPrefixes are the object prefixes a node may have signed. A signed URL +// is the whole authorisation, so without this the broker turns one token into +// read, write and delete over every object in the bucket, including avatars, +// form assets and workspace export archives that no node has any business +// touching. +// +// The list is what a node actually reaches: +// +// emails/ transport bodies the backend wrote for a send +// attachments/ campaign attachments the send pipeline references +// users/ mailbox bodies a sync stores +// +// It has to grow when a node starts touching a new prefix. A miss is a refused +// operation that names the key, not a mystery, which is the tradeoff being +// bought here. +var nodeKeyPrefixes = []string{"emails/", "attachments/", "users/"} + +// keyAllowedForNode reports whether key sits under a prefix a node may reach. +// Traversal is rejected outright rather than cleaned: no legitimate key +// contains "..", so the only caller producing one is probing. +func keyAllowedForNode(key string) bool { + if strings.Contains(key, "..") { + return false + } + for _, p := range nodeKeyPrefixes { + if strings.HasPrefix(key, p) { + return true + } + } + return false +} + // blobPresignTTL is how long a signed URL lives. Long enough for a large // attachment over a slow link, short enough that one captured in a log is // already dead. @@ -59,6 +92,12 @@ func (h *Handler) InternalPresignBlob(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "key required"}) return } + if !keyAllowedForNode(req.Key) { + c.JSON(http.StatusForbidden, gin.H{ + "error": "key " + req.Key + " is outside the prefixes a node may reach", + }) + return + } url, err := h.Storage.PresignedURL(c.Request.Context(), op, req.Key, req.ContentType, blobPresignTTL) if err != nil { diff --git a/internal/api/handler/nodescript/join.sh b/internal/api/handler/nodescript/join.sh index 91e8d03b..37512e1f 100755 --- a/internal/api/handler/nodescript/join.sh +++ b/internal/api/handler/nodescript/join.sh @@ -178,7 +178,7 @@ write_config() { if [ "$DRY_RUN" = "true" ]; then log "" log "--dry-run: would write $CONFIG_DIR/node.env with:" - printf '%s\n' "$NODE_ENV" | sed 's/\(TOKEN=\|KEY=\|SECRET=\|PASSWORD=\).*/\1***/' | sed 's/^/ /' + printf '%s\n' "$NODE_ENV" | redact | sed 's/^/ /' log "" log "--dry-run: would create $CONFIG_DIR/node.local.env if absent, and leave it" log " alone if present. Both files are passed to the container." @@ -233,6 +233,33 @@ write_config() { log "Wrote $CONFIG_DIR/node.env" } +# redact masks every value in the env that carries a credential, for the +# --dry-run listing. Two shapes, because they leak differently: +# +# NAME_TOKEN=secret the whole value goes +# NAME=scheme://user:pass@host only the userinfo goes, so the address the +# node will actually use stays readable, which +# is the thing --dry-run exists to show +# +# The second shape is why a name list is not enough on its own: PRIMARY_DB, +# NATS_URL, REDIS and SENTRY_DSN all carry their credential inside a URL and +# none of them is called TOKEN, KEY, SECRET or PASSWORD. +# +# One -e per word rather than a `\|` alternation: alternation in a BRE is a GNU +# extension, and on a sed without it the expression matches nothing and every +# secret prints in clear, which is the failure mode this function exists to +# prevent. Matched as a SUFFIX so ENCRYPTED_KEYS_BACKEND_URL, an address worth +# reading, is not masked for containing "KEY". +redact() { + sed -e 's/^\([A-Z0-9_]*TOKEN\)=.*/\1=***/' \ + -e 's/^\([A-Z0-9_]*KEY\)=.*/\1=***/' \ + -e 's/^\([A-Z0-9_]*SECRET\)=.*/\1=***/' \ + -e 's/^\([A-Z0-9_]*PASSWORD\)=.*/\1=***/' \ + -e 's/^\([A-Z0-9_]*DSN\)=.*/\1=***/' \ + -e 's|^\([A-Z0-9_]*\)=\([a-z][a-z0-9+.-]*://\)[^:/@]*:[^@]*@|\1=\2***:***@|' \ + -e 's|^\([A-Z0-9_]*\)=\([a-z][a-z0-9+.-]*://\)[^:/@]*@|\1=\2***@|' +} + # ensure_local_env creates the operator's own env file, once. node.env is # rewritten wholesale on every join, so anything added there is lost the next # time this runs; this file is the place that survives. The container reads diff --git a/internal/api/middleware/internal_auth.go b/internal/api/middleware/internal_auth.go index 82e26998..146daab9 100644 --- a/internal/api/middleware/internal_auth.go +++ b/internal/api/middleware/internal_auth.go @@ -52,3 +52,53 @@ func internalAuth(c *gin.Context) { } c.Next() } + +// NodeBrokerAuthMiddleware protects the two endpoints that perform a +// privileged operation on a caller's behalf: opening a sealed data key, and +// signing a blob operation. +// +// They are a step up from the rest of the internal API, which only moves +// records around. A caller here gets plaintext key material and a URL into the +// object store, so the token that opens them should not have to be the same +// one the tracking and forms services carry: those are internet-facing, and +// widening what their credential is worth is the whole risk. +// +// NODE_BROKER_TOKEN is that separate credential. It falls back to +// INTERNAL_API_TOKEN when unset, so an existing single-token deployment keeps +// working, and a split deployment can hand nodes something the edge services +// never see. +func (h *Handler) NodeBrokerAuthMiddleware() gin.HandlerFunc { + return nodeBrokerAuth +} + +var ( + brokerTokenOnce sync.Once + brokerToken []byte +) + +func loadBrokerToken() { + if v := os.Getenv("NODE_BROKER_TOKEN"); v != "" { + brokerToken = []byte(v) + return + } + brokerToken = []byte(os.Getenv("INTERNAL_API_TOKEN")) +} + +func nodeBrokerAuth(c *gin.Context) { + brokerTokenOnce.Do(loadBrokerToken) + if len(brokerToken) == 0 { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "internal auth not configured"}) + return + } + header := c.GetHeader("Authorization") + if !strings.HasPrefix(header, "Bearer ") { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "missing bearer token"}) + return + } + provided := []byte(strings.TrimPrefix(header, "Bearer ")) + if subtle.ConstantTimeCompare(provided, brokerToken) != 1 { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid bearer token"}) + return + } + c.Next() +} diff --git a/internal/api/routes.go b/internal/api/routes.go index 510a6569..4b50cd2c 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -121,6 +121,23 @@ func Run( // Internal backend-to-backend endpoints. Workers call these instead of // touching Postgres directly, per the no-direct-data-services rule in // CLAUDE.md. Auth: shared bearer token (INTERNAL_API_TOKEN). + // The two broker endpoints sit in their own group. They perform a + // privileged operation for the caller rather than moving a record, so they + // take NODE_BROKER_TOKEN, which falls back to INTERNAL_API_TOKEN but lets a + // split deployment keep the edge services off this credential. + broker := r.Group("/api/v1/internal") + broker.Use(m.NodeBrokerAuthMiddleware()) + { + // Opens a sealed data key for a node running KMS_PROVIDER=brokered, so + // a machine you own needs no cloud credential of its own. + broker.POST("/dek/decrypt", h.InternalDecryptDEK) + + // Signs one blob operation for a node running BLOB_PROVIDER=brokered. + // The node then transfers directly against the object store, so bodies + // and attachments never pass through here. + broker.POST("/blobs/presign", h.InternalPresignBlob) + } + internal := r.Group("/api/v1/internal") internal.Use(m.InternalAuthMiddleware()) { @@ -128,17 +145,6 @@ func Run( internal.PUT("/dek/:orgID", h.InternalPutDEK) internal.DELETE("/dek/:orgID", h.InternalDeleteDEK) - // Opens a sealed data key for a node running KMS_PROVIDER=brokered, so - // a machine you own needs no cloud credential of its own. Registered - // before the :orgID routes would ever match it: gin routes the static - // segment first, but keeping them adjacent makes the pair obvious. - internal.POST("/dek/decrypt", h.InternalDecryptDEK) - - // Signs one blob operation for a node running BLOB_PROVIDER=brokered. - // The node then transfers directly against the object store, so bodies - // and attachments never pass through here. - internal.POST("/blobs/presign", h.InternalPresignBlob) - // Click-link tickets: the tracking service resolves /c/ redirects // here instead of touching Postgres (read-only, heavily cached there). internal.GET("/tracked-links/:id", h.InternalGetTrackedLink) diff --git a/internal/app/instancecheck/checks_fleet.go b/internal/app/instancecheck/checks_fleet.go index beb85866..47e75541 100644 --- a/internal/app/instancecheck/checks_fleet.go +++ b/internal/app/instancecheck/checks_fleet.go @@ -68,15 +68,30 @@ func checkFleetBlobsNotShared(ctx context.Context, d Deps, _ Input) *Finding { // It keeps heartbeating over HTTP the whole time, so the fleet looks healthy. func checkFleetInfraUnreachable(ctx context.Context, d Deps, _ Input) *Finding { var bad []string - for _, key := range []string{"NATS_URL", "REDIS", "ENCRYPTED_KEYS_BACKEND_URL"} { + for _, key := range []string{"NATS_URL", "REDIS"} { v := env(key) if v == "" { continue } - if isLoopbackURL(v) || isContainerInternalURL(v) { + if unreachableOffHost(v) { bad = append(bad, fmt.Sprintf("%s=%s", key, v)) } } + // renderNodeEnv falls back to APP_INTERNAL_URL, so checking only the first + // name would pass every instance that uses the second one. + backendKey, backendURL := "ENCRYPTED_KEYS_BACKEND_URL", env("ENCRYPTED_KEYS_BACKEND_URL") + if backendURL == "" { + backendKey, backendURL = "APP_INTERNAL_URL", env("APP_INTERNAL_URL") + } + switch { + case backendURL == "": + // Neither is set, so a node is told to call back on nothing. With the + // brokered providers that is not a degraded node but one that cannot + // open a single key. + bad = append(bad, "ENCRYPTED_KEYS_BACKEND_URL is unset") + case unreachableOffHost(backendURL): + bad = append(bad, fmt.Sprintf("%s=%s", backendKey, backendURL)) + } if len(bad) == 0 { return nil } @@ -91,6 +106,23 @@ func checkFleetInfraUnreachable(ctx context.Context, d Deps, _ Input) *Finding { docsSplitDeployment) } +// unreachableOffHost reports whether a value names something only this machine +// can resolve. +// +// isLoopbackURL needs a scheme, and REDIS is sometimes written bare, so the +// host is recovered either way before it is judged. Missing that meant +// REDIS=localhost:6379 read as reachable, which is precisely the value most +// likely to be sitting there. +func unreachableOffHost(raw string) bool { + if isLoopbackURL(raw) || isContainerInternalURL(raw) { + return true + } + if strings.Contains(raw, "://") { + return false + } + return isLoopbackHost(hostOnly(raw)) +} + // isContainerInternalURL matches the service names the shipped compose file // uses. They resolve inside that network and nowhere else, and they are what // an instance that grew out of `make up` is still carrying. diff --git a/internal/app/instancecheck/checks_fleet_test.go b/internal/app/instancecheck/checks_fleet_test.go new file mode 100644 index 00000000..0a0296ef --- /dev/null +++ b/internal/app/instancecheck/checks_fleet_test.go @@ -0,0 +1,38 @@ +package instancecheck + +import "testing" + +// The values most likely to be sitting in a config that grew out of `make up`, +// plus the bare host:port form REDIS is sometimes written in. Missing that form +// meant the check passed the one value it most needed to catch. +func TestUnreachableOffHost(t *testing.T) { + unreachable := []string{ + "nats://nats:4222", + "tls://token@nats:4222", + "redis://redis:6379", + "rediss://:pass@redis:6379", + "http://localhost:8080", + "http://127.0.0.1:8080", + "localhost:6379", + "127.0.0.1:6379", + "redis:6379", + } + for _, v := range unreachable { + if !unreachableOffHost(v) { + t.Errorf("%q reported reachable from another machine", v) + } + } + + reachable := []string{ + "tls://token@bus.example.com:4222", + "rediss://:pass@bus.example.com:6380", + "https://api.example.com", + "bus.example.com:4222", + "nats://10.0.1.5:4222", + } + for _, v := range reachable { + if unreachableOffHost(v) { + t.Errorf("%q reported unreachable", v) + } + } +} diff --git a/internal/infrastructure/kms/brokered.go b/internal/infrastructure/kms/brokered.go index 014fd911..8db1ce7a 100644 --- a/internal/infrastructure/kms/brokered.go +++ b/internal/infrastructure/kms/brokered.go @@ -45,10 +45,10 @@ type BrokeredProvider struct { // internal API token both sides share. func NewBrokered(baseURL, token string) (*BrokeredProvider, error) { if baseURL == "" { - return nil, errors.New("kms.brokered: baseURL is required") + return nil, errors.New("kms.brokered: no control plane address; set ENCRYPTED_KEYS_BACKEND_URL on the backend to a URL this machine can reach, then re-join this node") } if token == "" { - return nil, errors.New("kms.brokered: token is required") + return nil, errors.New("kms.brokered: no credential; set INTERNAL_API_TOKEN (or NODE_BROKER_TOKEN) to the same value the backend uses") } if _, err := url.Parse(baseURL); err != nil { return nil, fmt.Errorf("kms.brokered: invalid baseURL: %w", err) diff --git a/internal/infrastructure/kms/factory.go b/internal/infrastructure/kms/factory.go index 1ea92a79..402ee6ca 100644 --- a/internal/infrastructure/kms/factory.go +++ b/internal/infrastructure/kms/factory.go @@ -42,7 +42,13 @@ func FromEnv(ctx context.Context, awscfg aws.Config, fallbackAWSKeyID string) (P if baseURL == "" { baseURL = os.Getenv("WARMBLY_BACKEND_URL") } - token := os.Getenv("INTERNAL_API_TOKEN") + // The broker endpoints take their own credential when the instance + // issues one, so a split deployment can keep this off the edge + // services. It falls back to the shared internal token. + token := os.Getenv("NODE_BROKER_TOKEN") + if token == "" { + token = os.Getenv("INTERNAL_API_TOKEN") + } if token == "" { token = os.Getenv("ENCRYPTED_KEYS_WORKER_TOKEN") } diff --git a/internal/infrastructure/storage/brokered.go b/internal/infrastructure/storage/brokered.go index 5511801d..ad067924 100644 --- a/internal/infrastructure/storage/brokered.go +++ b/internal/infrastructure/storage/brokered.go @@ -36,7 +36,12 @@ import ( type BrokeredStore struct { baseURL string token string - client *http.Client + // broker talks to the control plane, transfer talks to the object store. + // Two clients because the deadlines are not the same question: a presign + // is a small local call, a transfer can legitimately be a large object on + // a bad link. + broker *http.Client + transfer *http.Client } // maxBrokeredBody caps what Put will buffer when the caller's reader cannot @@ -50,31 +55,38 @@ const maxBrokeredBody = 64 << 20 // time anyone reads it. const brokerTTL = 5 * time.Minute +// Timeouts, because the callers do not supply one. The mailbox sync loop runs +// on a context derived from context.Background(), so a request that never +// answers wedges that mailbox forever rather than failing and being retried. +const ( + brokerCallTimeout = 15 * time.Second + transferTimeout = 5 * time.Minute +) + // BrokeredOption configures a BrokeredStore. type BrokeredOption func(*BrokeredStore) -// WithBrokeredHTTPClient overrides the client used for both the broker call -// and the transfer itself. +// WithBrokeredHTTPClient overrides both clients, for tests and for callers +// that need a custom transport. func WithBrokeredHTTPClient(c *http.Client) BrokeredOption { - return func(s *BrokeredStore) { s.client = c } + return func(s *BrokeredStore) { s.broker, s.transfer = c, c } } func NewBrokered(baseURL, token string, opts ...BrokeredOption) (*BrokeredStore, error) { if baseURL == "" { - return nil, errors.New("storage.brokered: baseURL is required") + return nil, errors.New("storage.brokered: no control plane address; set ENCRYPTED_KEYS_BACKEND_URL on the backend to a URL this machine can reach, then re-join this node") } if token == "" { - return nil, errors.New("storage.brokered: token is required") + return nil, errors.New("storage.brokered: no credential; set INTERNAL_API_TOKEN (or NODE_BROKER_TOKEN) to the same value the backend uses") } if _, err := url.Parse(baseURL); err != nil { return nil, fmt.Errorf("storage.brokered: invalid baseURL: %w", err) } s := &BrokeredStore{ - baseURL: strings.TrimRight(baseURL, "/"), - token: token, - // No global timeout: a large attachment on a slow link is a legitimate - // long request, and the per-call context already bounds it. - client: &http.Client{}, + baseURL: strings.TrimRight(baseURL, "/"), + token: token, + broker: &http.Client{Timeout: brokerCallTimeout}, + transfer: &http.Client{Timeout: transferTimeout}, } for _, o := range opts { o(s) @@ -115,7 +127,7 @@ func (s *BrokeredStore) presign(ctx context.Context, op PresignOp, key, contentT req.Header.Set("Content-Type", "application/json") req.Header.Set("User-Agent", "warmbly-node/storage-brokered") - resp, err := s.client.Do(req) + resp, err := s.broker.Do(req) if err != nil { return out, fmt.Errorf("storage.brokered: presign %s: %w", op, err) } @@ -152,7 +164,7 @@ func (s *BrokeredStore) do(ctx context.Context, signed presignResponse, body io. if contentType != "" { req.Header.Set("Content-Type", contentType) } - return s.client.Do(req) + return s.transfer.Do(req) } func (s *BrokeredStore) Get(ctx context.Context, key string) (io.ReadCloser, error) { @@ -167,12 +179,17 @@ func (s *BrokeredStore) Get(ctx context.Context, key string) (io.ReadCloser, err switch resp.StatusCode { case http.StatusOK, http.StatusPartialContent: return resp.Body, nil - case http.StatusNotFound, http.StatusForbidden: - // A bucket with ListBucket withheld answers a missing key with 403 - // rather than 404, and a caller checking for ErrNotFound must not have - // to know which of the two it is talking to. + case http.StatusNotFound: resp.Body.Close() return nil, ErrNotFound + case http.StatusForbidden: + // Not "missing". The URL is signed for this exact key, so an absent + // object answers 404; a 403 means the signature expired in flight or + // the signing principal has lost its permission. Reporting that as + // ErrNotFound would turn a config error into "the body is gone" on + // every send, which is the wrong thing to go looking for. + resp.Body.Close() + return nil, fmt.Errorf("storage.brokered: get: refused by the object store (403); the signature expired in flight or the control plane's credential no longer grants this bucket") default: resp.Body.Close() return nil, fmt.Errorf("storage.brokered: get: unexpected status %d", resp.StatusCode) @@ -237,8 +254,13 @@ func (s *BrokeredStore) Has(ctx context.Context, key string) (bool, error) { switch resp.StatusCode { case http.StatusOK: return true, nil - case http.StatusNotFound, http.StatusForbidden: + case http.StatusNotFound: return false, nil + case http.StatusForbidden: + // Same reasoning as Get: reporting this as "no such object" would make + // a broken credential look like an empty bucket, and the caller would + // happily re-store everything it already had. + return false, fmt.Errorf("storage.brokered: has: refused by the object store (403); the signature expired in flight or the control plane's credential no longer grants this bucket") default: return false, fmt.Errorf("storage.brokered: has: unexpected status %d", resp.StatusCode) } diff --git a/internal/infrastructure/storage/brokered_test.go b/internal/infrastructure/storage/brokered_test.go index 224b8b52..f22d9a54 100644 --- a/internal/infrastructure/storage/brokered_test.go +++ b/internal/infrastructure/storage/brokered_test.go @@ -73,16 +73,59 @@ func TestBrokeredGet(t *testing.T) { } } -// A bucket that withholds ListBucket answers a missing key with 403 rather -// than 404, and callers check for ErrNotFound without knowing which. +// A URL signed for one key answers a missing object with 404. func TestBrokeredGetMissingIsNotFound(t *testing.T) { - for _, status := range []int{http.StatusNotFound, http.StatusForbidden} { - s, _ := brokerFor(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(status) - })) - if _, err := s.Get(context.Background(), "gone"); !errors.Is(err, ErrNotFound) { - t.Errorf("status %d: got %v, want ErrNotFound", status, err) - } + s, _ := brokerFor(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + if _, err := s.Get(context.Background(), "gone"); !errors.Is(err, ErrNotFound) { + t.Errorf("got %v, want ErrNotFound", err) + } +} + +// A 403 is an expired signature or a credential that lost the bucket, not a +// missing object. Reporting it as ErrNotFound turns a config error into "the +// body is gone" on every send, and sends the operator looking for the wrong +// thing entirely. +func TestBrokeredRefusedIsNotMissing(t *testing.T) { + s, _ := brokerFor(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + _, err := s.Get(context.Background(), "k") + if err == nil { + t.Fatal("a 403 was accepted") + } + if errors.Is(err, ErrNotFound) { + t.Error("a 403 was reported as a missing object") + } + if !strings.Contains(err.Error(), "403") { + t.Errorf("error does not name the status: %v", err) + } + + // Has must not answer "no such object" either, or a caller re-stores + // everything it already had. + ok, err := s.Has(context.Background(), "k") + if err == nil { + t.Fatal("Has accepted a 403") + } + if ok { + t.Error("Has reported true on a refusal") + } +} + +// The callers do not supply a deadline: the mailbox sync loop runs on a +// context derived from Background, so a request that never answers has to be +// bounded here or it wedges that mailbox forever. +func TestBrokeredClientsHaveTimeouts(t *testing.T) { + s, err := NewBrokered("https://x", "tok") + if err != nil { + t.Fatalf("NewBrokered: %v", err) + } + if s.broker.Timeout == 0 { + t.Error("the broker client has no timeout") + } + if s.transfer.Timeout == 0 { + t.Error("the transfer client has no timeout") } } diff --git a/internal/infrastructure/storage/factory.go b/internal/infrastructure/storage/factory.go index a94bd20b..91942c88 100644 --- a/internal/infrastructure/storage/factory.go +++ b/internal/infrastructure/storage/factory.go @@ -61,7 +61,13 @@ func NewFromEnv(ctx context.Context, awscfg aws.Config, defaultBucket string) (S if baseURL == "" { baseURL = os.Getenv("WARMBLY_BACKEND_URL") } - token := os.Getenv("INTERNAL_API_TOKEN") + // The broker endpoints take their own credential when the instance + // issues one, so a split deployment can keep this off the edge + // services. It falls back to the shared internal token. + token := os.Getenv("NODE_BROKER_TOKEN") + if token == "" { + token = os.Getenv("INTERNAL_API_TOKEN") + } if token == "" { token = os.Getenv("ENCRYPTED_KEYS_WORKER_TOKEN") } diff --git a/scripts/aws-bootstrap.sh b/scripts/aws-bootstrap.sh index 0ee6a55c..f1836480 100644 --- a/scripts/aws-bootstrap.sh +++ b/scripts/aws-bootstrap.sh @@ -88,11 +88,14 @@ create_kms() { KEY_ALIAS="alias/$PREFIX" if aws kms describe-key --key-id "$KEY_ALIAS" --region "$REGION" >/dev/null 2>&1; then log "KMS: $KEY_ALIAS already exists" + KEY_ARN=$(aws kms describe-key --key-id "$KEY_ALIAS" --region "$REGION" \ + --query 'KeyMetadata.Arn' --output text) return 0 fi log "KMS: creating $KEY_ALIAS" if [ "$DRY_RUN" = "true" ]; then log " would create a symmetric key and alias it $KEY_ALIAS" + KEY_ARN="arn:aws:kms:$REGION:$ACCOUNT_ID:key/" return 0 fi key_id=$(aws kms create-key \ @@ -100,6 +103,8 @@ create_kms() { --region "$REGION" \ --query 'KeyMetadata.KeyId' --output text) aws kms create-alias --alias-name "$KEY_ALIAS" --target-key-id "$key_id" --region "$REGION" + KEY_ARN=$(aws kms describe-key --key-id "$key_id" --region "$REGION" \ + --query 'KeyMetadata.Arn' --output text) # Losing this key makes every stored mailbox credential unreadable, so it # gets the longest window AWS offers against an accidental delete. aws kms enable-key-rotation --key-id "$key_id" --region "$REGION" || true @@ -138,6 +143,10 @@ create_bucket() { # control_policy is what the backend and consumer need: mint and open data # keys, read and write the bucket, send platform mail. +# +# Resource is the key ARN, never the alias ARN. IAM does not resolve an alias +# in a Resource element, so a policy naming one grants nothing and the first +# GenerateDataKey fails with AccessDenied against a policy that reads correctly. control_policy() { cat < /tmp/warmbly-redact.$$ && \ + printf '%s\n' \ + 'PRIMARY_DB=postgres://warmbly:dbsecret@db.example.com:5432/warmbly' \ + 'NATS_URL=tls://bussecret@bus.example.com:4222' \ + 'REDIS=rediss://:cachesecret@bus.example.com:6380' \ + 'INTERNAL_API_TOKEN=tokensecret' \ + 'NODE_BROKER_TOKEN=brokersecret' \ + 'CREDENTIALS_ENCRYPTION_KEY=keysecret' \ + 'BOX_GOOGLE_CLIENT_SECRET=oauthsecret' \ + 'ENCRYPTED_KEYS_BACKEND_URL=https://api.example.com' \ + | sh -c ". /tmp/warmbly-redact.$$; redact") +rm -f "/tmp/warmbly-redact.$$" + +for leaked in dbsecret bussecret cachesecret tokensecret brokersecret keysecret oauthsecret; do + if printf '%s\n' "$redacted" | grep -q "$leaked"; then + fail "--dry-run prints $leaked in clear; the redaction does not cover it on this sed" + fi +done +# The addresses are the reason --dry-run exists, so they have to survive. +printf '%s\n' "$redacted" | grep -q 'db.example.com:5432' \ + || fail "redaction ate the database host; only the credential should go" +printf '%s\n' "$redacted" | grep -q '^ENCRYPTED_KEYS_BACKEND_URL=https://api.example.com$' \ + || fail "redaction masked an address that carries no credential" +ok "--dry-run masks every credential and keeps the addresses" + # Two invariants that leave no trace in the rendered unit and so cannot be # caught above: both were real defects, so they are asserted at their call # sites. Comments are stripped and the call is matched in command position, so