From 1a766f622ee7c418fd1e8bec1d7ade746ac83a95 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Sun, 30 Aug 2026 00:47:47 -0400 Subject: [PATCH 01/11] spike(connector): butterstack-connector daemon + mock broker (#1575 group 1) The standalone proof for issue #1575 checkbox groups 0 and 1: a Go daemon a studio runs inside its own network, and the day-1 protocol schema that had to land before any verb did. New top-level `connector/`. Nothing else in the repo is touched: no /connect endpoint on the Rails app, no ActionCable change, no migration, no terraform, nothing deployed. Those are later PRs. What is here: - A daemon that dials out over WSS to one hostname on 443 and never listens on anything. The connector token travels in the Authorization header and nowhere else; an endpoint carrying a query string is refused at config load, so a copy-pasted ?token= URL cannot start the daemon (Shuri F1/F6, issue #935's lesson applied to a new surface before it exists). - A typed allowlist with a per-verb argument-constraint layer, in one readable file, because an IT director is asked to read the source. Two layers before any tool call: vocabulary membership, then a fixed per-verb argument schema where an unnamed key stops the command rather than being ignored into the call. Compiled: teamcity.server.info, teamcity.build.get, p4.describe, p4.changes, and the sys verbs. No mutating and no content-class verb is compiled in. - No caller-supplied trigger parameters, enforced structurally rather than by convention (Shuri F4). `bannedArgNames` lists the argument names no verb may declare, compiled or reserved, each with the reason; `Selfcheck()` runs in the tests and again at process start, so a build whose vocabulary grew one refuses to run. - Scoped arguments (depot_scope, allowed_build_types, repo_allowlist) that live only in connector.yml. A depot path's literal prefix must already sit inside a scoped prefix, so a wildcard cannot climb above the scope and `//...` is denied even to a P4 user who could read it (Shuri F5). - Credentials read only from connector.yml or a *_file path it names. No environment fallback, no flag that takes a secret, no remote configuration. 0600 or stricter, or it refuses to start. - p4 invoked as an argv array with no shell; the ticket passed via P4PASSWD in a minimal environment so it never appears in the host's process list. - A minimal RFC 6455 client written instead of a dependency, for the CVE surface and because it has to be readable. Only third-party dependency: yaml.v3. - A Ruby mock broker and the seven drills from design note 4.3, plus a round-trip phase and the broker-side half of drill (f). All nine pass. The Ruby WebSocket server half was written independently from the RFC, so a framing mistake fails a drill instead of agreeing with itself. PROTOCOL.md is checkbox group 0 written down, including the two rules this PR cannot enforce because they are Rails-side: the dedicated Rack endpoint (never ActionCable, never /cable, no change to allowed_request_origins) and the tenant scoping for Connector.call. README.md carries the "what this does not prove" list, which is the honest half: nothing ran against staging, demo, or prod; the argument-constraint layer is proved at the frame boundary against a mock, not end to end; conditions 1, 4 and 5 (signing, SBOM, version skew, the enforced egress spec) are untouched. Refs https://github.com/ButterStack/butter_stack/issues/1575 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SHDQkRkNToRZ7BSM3YBQpN --- .gitignore | 1 + Makefile | 45 +++ PROTOCOL.md | 356 +++++++++++++++++ README.md | 151 +++++++ cmd/butterstack-connector/main.go | 133 +++++++ connector.example.yml | 76 ++++ go.mod | 5 + go.sum | 4 + internal/audit/audit.go | 127 ++++++ internal/config/config.go | 314 +++++++++++++++ internal/config/config_test.go | 190 +++++++++ internal/protocol/protocol.go | 146 +++++++ internal/session/session.go | 431 ++++++++++++++++++++ internal/tools/perforce.go | 211 ++++++++++ internal/tools/teamcity.go | 191 +++++++++ internal/tools/tools.go | 42 ++ internal/vocab/vocab.go | 608 +++++++++++++++++++++++++++++ internal/vocab/vocab_test.go | 244 ++++++++++++ internal/wsclient/wsclient.go | 398 +++++++++++++++++++ internal/wsclient/wsclient_test.go | 36 ++ test/README.md | 45 +++ test/drills.rb | 545 ++++++++++++++++++++++++++ test/mock_broker.rb | 402 +++++++++++++++++++ test/support/fake_p4 | 60 +++ test/support/teamcity_stub.rb | 118 ++++++ test/support/tls.rb | 59 +++ test/support/ws.rb | 229 +++++++++++ 27 files changed, 5167 insertions(+) create mode 100644 .gitignore create mode 100644 Makefile create mode 100644 PROTOCOL.md create mode 100644 README.md create mode 100644 cmd/butterstack-connector/main.go create mode 100644 connector.example.yml create mode 100644 go.mod create mode 100644 go.sum create mode 100644 internal/audit/audit.go create mode 100644 internal/config/config.go create mode 100644 internal/config/config_test.go create mode 100644 internal/protocol/protocol.go create mode 100644 internal/session/session.go create mode 100644 internal/tools/perforce.go create mode 100644 internal/tools/teamcity.go create mode 100644 internal/tools/tools.go create mode 100644 internal/vocab/vocab.go create mode 100644 internal/vocab/vocab_test.go create mode 100644 internal/wsclient/wsclient.go create mode 100644 internal/wsclient/wsclient_test.go create mode 100644 test/README.md create mode 100644 test/drills.rb create mode 100644 test/mock_broker.rb create mode 100755 test/support/fake_p4 create mode 100644 test/support/teamcity_stub.rb create mode 100644 test/support/tls.rb create mode 100644 test/support/ws.rb diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..84c048a --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +/build/ diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..556df8a --- /dev/null +++ b/Makefile @@ -0,0 +1,45 @@ +# butterstack-connector (issue #1575 spike) +# +# Go is run in a container by default so no host toolchain is required. +# Set GO=go to use a host Go instead. + +GO ?= docker +GO_IMAGE ?= golang:1.23-alpine +RUBY ?= ruby +BIN := build/butterstack-connector +VERSION ?= 0.1.0-spike + +ifeq ($(GO),docker) +GORUN = docker run --rm -v "$(CURDIR)":/w -w /w \ + -v /tmp/butterstack-connector-gocache:/root/.cache/go-build \ + -v /tmp/butterstack-connector-gomod:/go/pkg/mod $(GO_IMAGE) sh -c +else +GORUN = sh -c +endif + +.PHONY: all build test drills check vocabulary fmt clean + +all: build + +build: + @mkdir -p build + $(GORUN) 'go build -ldflags "-X main.Version=$(VERSION)" -o $(BIN) ./cmd/butterstack-connector' + @echo "built $(BIN)" + +fmt: + $(GORUN) 'gofmt -l -w .' + +test: + $(GORUN) 'gofmt -l . && go vet ./... && go test ./...' + +# The seven drills from the design note section 4.3, against the mock broker. +drills: build + CONNECTOR_BIN=$(CURDIR)/$(BIN) $(RUBY) test/drills.rb + +check: test drills + +vocabulary: build + @$(BIN) -print-vocabulary + +clean: + rm -rf build diff --git a/PROTOCOL.md b/PROTOCOL.md new file mode 100644 index 0000000..3317411 --- /dev/null +++ b/PROTOCOL.md @@ -0,0 +1,356 @@ +# ButterStack Connector protocol, v0 + +Status: **day-1 spike schema** for issue #1575 group 1. This document is the +written-down form of checkbox group 0 (the day-1 protocol schema), which had to +land before any verb did, because argument constraints *are* schema. + +Sources: Devin's design note §2.2 and §2.4 +(`ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md`, +branch `plan/teamcity-private-reach`) and Shuri's must-fix list §6 +(`ai/team/agents/shuri/reports/2026-08-29-connector-design-security-review.md`). + +Two things this document does **not** do. It does not describe an endpoint that +exists: the broker side is a later PR, and the only implementation of this +protocol's server half today is the drill harness in `test/mock_broker.rb`. And +it does not restate the six survival conditions; it records the parts of them +that are schema. + +--- + +## 1. Transport + +One outbound TLS connection, from the studio's network to one ButterStack +hostname, on 443. The connector never listens on anything. + +``` +wss:///connect +``` + +| Rule | Why | +|---|---| +| `wss://` only. There is no plaintext option and no "skip verification" option for the broker connection. | `allow_insecure_tls` exists for a studio's self-signed *LAN* TeamCity, and is scoped to that host. The connection that carries commands is always verified. | +| The endpoint must carry **no query string**, no userinfo, and no fragment. | See §2. Enforced at config load, before a socket is opened. | +| TLS 1.2 minimum. | Matches the production ALB policy (`ELBSecurityPolicy-TLS13-1-2-2021-06`) and the instance nginx (`TLSv1.2 TLSv1.3`). | +| Heartbeat every 25 s by default; the broker may negotiate 5–120 s in `welcome`. | Must stay under the shortest idle timeout on the path: ALB `idle_timeout = 300`, instance nginx `proxy_read_timeout 300`, local docker nginx 60 s. | +| Reconnect: exponential backoff with full jitter, floor 500 ms, cap 60 s, forever. The backoff resets after a session that actually came up. | A connector that cannot connect is a log line on the studio side, never an alert. | + +### The broker endpoint (stated here so the later PR inherits it) + +Shuri §6 item 4, verbatim in effect: the broker is a **dedicated Rack endpoint +at `/connect`. Not ActionCable. Not `/cable`. Not `ApplicationCable::Connection`. +No change to `allowed_request_origins`. No `disable_request_forgery_protection`.** + +The reasons are code-verified in her review and are worth keeping next to the +schema, because the tempting shortcut is real: + +- `app/channels/application_cable/connection.rb:19-25` authenticates from the + warden session cookie and rejects everything else, so a bearer-token machine + client has no path through it. +- `config/environments/production.rb:56` restricts `allowed_request_origins` to + the two `RAILS_HOST` origins. A non-browser client sends no `Origin`, so + admitting one through ActionCable would mean widening that list or disabling + forgery protection — either of which weakens a live cross-site + WebSocket-hijacking control that protects real users' browser sockets. +- That connection carries `identified_by :current_user, :current_account`, + `impersonates :user`, and sets `ActsAsTenant.current_tenant`. A machine client + must inherit none of it. + +`/connect` therefore has its own auth (§2), its own connection object, and no +session, cookie, or CSRF machinery in the path. + +--- + +## 2. Authentication + +### Token format + +``` +bsc__<32 random bytes, base32> +``` + +- The `bsc_` prefix makes the credential greppable by secret scanners, ours and + the studio's. +- The id segment makes the broker's hashed lookup a primary-key read rather than + a table scan. +- 32 random bytes is 256 bits, which is why plain SHA-256 is the correct + storage: a slow KDF buys nothing against an input with that much entropy. + +### Storage on our side + +Store **only** `SHA-256(secret segment)`. Compare with +`ActiveSupport::SecurityUtils.secure_compare`. Display the token exactly once at +issue time; never make it retrievable afterwards, which is what makes the revoke +story in §6 honest. + +This is a **new pattern for this codebase and the nearest precedent is the wrong +one**: `Integration#webhook_token` is generated with good entropy +(`app/models/integration.rb:575-577`) but is stored reversibly and looked up by +equality (`jenkins_controller.rb:46`). A connector token stored that way means +one database read yields every studio's live connector credential. Do not copy +it. + +### The token travels in a header. Only in a header. + +``` +Authorization: Bearer bsc_... +``` + +**A token supplied as a query parameter on the `/connect` upgrade is rejected**, +and the rejection happens before any per-connection state is allocated. A +connector is not a browser: the only reason WebSocket clients put credentials in +query strings is a browser limitation that does not apply here, and a URL is +logged by every proxy on the path. This is the F1 lesson (issue #935, a live +webhook token in an nginx access log) applied to a new surface *before* it +exists rather than after. + +Both halves are enforced and drilled: + +- **Client half:** an endpoint carrying a query string is refused at config load + (`internal/config`), so a copy-pasted `?token=` URL cannot start the daemon. +- **Broker half:** the upgrade is refused with an HTTP status, never a 101, and + there is no code path that reads a token from a query string + (`test/mock_broker.rb`). + +### Refusal order + +The broker refuses, in this order, **before allocating any per-connection +state**: wrong path, query string present, not an upgrade, missing +`Sec-WebSocket-Key`, missing bearer token, malformed token, unknown token, +revoked token. An unknown verb must likewise never reveal what is configured +(§4). + +### Rate limiting + +`/connect` upgrades need a **dedicated, stricter throttle** in addition to the +generic `req/ip` 300-per-5-minutes at +`config/initializers/rack_attack.rb:112-115`. That generic budget is shared with +a studio's webhook traffic arriving from the same NAT address, and rack-attack +sees only the upgrade, never the frames — so per-session command budgets and the +per-integration connection cap (v0: 2) belong in the broker, not in rack-attack. + +--- + +## 3. Frames + +All frames are single JSON objects with a `type` discriminator. There is no +binary frame type and no streaming. + +### Connector → broker + +```jsonc +// hello -- first frame after the upgrade. Every field here is egress. +{ "type": "hello", "connector_id": "...", "integration_id": "...", + "version": "0.1.0", "protocol": "0", + "capabilities": ["p4.changes", "..."], "tool_versions": {"teamcity": "2025.03"} } + +// heartbeat -- egress too; queue_depth is a signal about the studio's load. +{ "type": "heartbeat", "ts": "2026-08-30T12:00:00Z", "queue_depth": 0 } + +// result -- answers exactly one command, keyed by its id. +{ "type": "result", "id": "", "status": "ok|error|denied|timeout", + "reason": "", "body": {}, "truncated": false, "bytes": 1234 } +``` + +### Broker → connector + +```jsonc +{ "type": "welcome", "session_id": "...", "server_time": "...", + "min_supported_version": "0", "heartbeat_interval": 25 } + +{ "type": "reject", "reason": "" } + +// command -- the only frame that can cause the connector to touch a tool. +{ "type": "command", "id": "", "verb": "teamcity.build.get", + "args": {"build_id": 9001}, "deadline_ms": 8000, "max_bytes": 32768 } +``` + +Note what a `command` does **not** contain: no host, no port, no URL, no shell +string, no credential. Those live in `connector.yml` on the studio's disk and +never appear on the wire. + +### Ordering, replay, and size + +- `id` is a UUID minted by us. The connector rejects a duplicate `id` inside a + 10-minute sliding window with `denied / duplicate_command_id`, rather than + re-executing it. +- A command past `deadline_ms` answers `timeout / deadline_exceeded`. The + connector caps the deadline at 60 s regardless of what the broker asks for. +- Every verb has a `max_bytes` default; the connector truncates and sets + `truncated: true` rather than streaming unbounded. (Same instinct as the + 256,000-character pushed-log cap at `jenkins_controller.rb:293`.) +- A `result` is discarded unless the responding session's authenticated + `integration_id` is the one that issued the command. Reply routing is derived + **server-side** from the authenticated session, never from a field in a frame. + +### Tenant scoping for `Connector.call` (Shuri §6 item 5) + +`config/initializers/acts_as_tenant.rb:8` sets `require_tenant = false`, so a +missed scope returns cross-tenant rows *silently* instead of raising. Therefore: + +- every command execution wraps in an explicit + `ActsAsTenant.with_tenant(integration.account)`; +- nothing relies on ambient `Current.account`, which is set by a controller + `before_action` (`app/controllers/concerns/set_current_request_details.rb:6-9`) + that does not run on a socket; +- the Redis reply channel is derived server-side from the socket's authenticated + `integration_id`; +- a `result` frame is dropped unless it matches the issuing session. + +The drill: assert tenant context is nil at the start of a request that follows a +connector frame on the same Puma thread. **That drill is Rails-side and is not +covered by this PR** (see `README.md`, "what this does not prove"). + +--- + +## 4. Vocabulary and the argument-constraint layer + +The authoritative, executable form of this section is +[`internal/vocab/vocab.go`](internal/vocab/vocab.go), which is one file on +purpose: an IT director is asked to read the source, so the source has to be +readable. `butterstack-connector -print-vocabulary` prints it. + +### Two layers, both before any tool call + +1. **The verb must be in the compiled vocabulary.** A name that is not in the + vocabulary at all → `denied / unknown_verb`. A name the schema *reserves* but + this build does not compile in → `denied / verb_not_compiled`. There is no + dynamic registration, no plugin path, no verb name read from config, and + there is no `sys.exec`. + +2. **Every argument is validated against a fixed per-verb schema.** + + | Rule | Denial reason | + |---|---| + | An argument the schema does not name stops the command. It is never ignored into the tool call. | `unknown_argument` | + | Every argument is a scalar. The schema has no map, object, or free-form kind, so it *cannot express* a parameter bag. | (structural) | + | Integers are type- and range-checked at the frame boundary. A JSON string `"42"` is not an integer. | `argument_type`, `argument_range` | + | Depot paths must begin `//`, carry no revision specifier (`@ # %`) and no traversal segment. | `argument_pattern` | + | Scoped arguments must fall inside a list that lives only in `connector.yml`: `depot_scope`, `allowed_build_types`, `repo_allowlist`. | `out_of_scope_path`, `out_of_scope_build_type`, `out_of_scope_repo` | + | A depot path's **literal prefix** (everything before the first wildcard) must already sit inside a scoped prefix, so a wildcard can never climb above the scope. `//...` is denied even to a P4 user who could read it. | `out_of_scope_path` | + | Content-class output is off in v0 at the schema level, not merely in config. | `content_verb_disabled` | + +A well-formed verb with an out-of-scope argument is denied **exactly like an +unknown verb**, and both write a local audit line. Only the second kind of +denial actually tests survival condition 2; the first only tests the dispatcher. +Both are drilled separately for that reason (Shuri §6 item 7b). + +### No caller-supplied trigger parameters. Ever, in v0. + +This is the finding that made this layer day-1 work rather than v1 polish +(Shuri F4). `allowed_jobs` constrains *which* job runs; a `params` map is +unconstrained, and Jenkins build parameters and TeamCity properties are +interpolated into shell build steps by design — including in our own +`Jenkinsfile.unreal` and `Jenkinsfile.minimobile` templates. A caller-supplied +parameter bag would therefore turn the typed allowlist into a code-execution +primitive on the studio's build agents, and falsify the single sentence the +Tier 2 sale rests on (README Appendix B answer 2: "a compromise of our cloud +yields read-access to changelist metadata via a P4 user **you** scoped, a +bounded blast radius"). + +So: + +- `jenkins.build.trigger` takes `{job}` and nothing else. When it ships it + triggers with the job's own default parameters. +- `teamcity.build.queue` takes `{build_type_id}` and nothing else; the connector + composes a **fixed** request body, `{"buildType":{"id":""}}`. No + `properties`, no `branchName`, no `comment`. +- Neither verb is compiled into v0 at all. +- A v1 may add per-job `allowed_params` in `connector.yml` — an allowlist of + parameter *names*, each with a value pattern or enum, enforced connector-side + before the call. + +This is enforced structurally, not by convention: `bannedArgNames` in +`vocab.go` lists the argument names no verb may declare — compiled or reserved — +each with the reason it is banned, and `Selfcheck()` runs both in the test suite +and at process start. A build whose vocabulary grew one of them refuses to run. + +### Perforce invocation + +If the connector shells out to the `p4` CLI rather than using P4Ruby, **every +invocation is an argv array with no shell interpretation** (Shuri F5's smaller +sibling). The ticket is passed through `P4PASSWD` in a minimal environment +rather than on the command line, so it never appears in the studio host's +process list. `p4 describe` is always invoked with `-s`, so diffs are excluded +at the tool boundary as well as in the schema. + +### v0 vocabulary + +| Verb | Class | v0 | +|---|---|---| +| `sys.ping`, `sys.version`, `sys.capabilities` | M | compiled | +| `teamcity.server.info` | M | compiled | +| `teamcity.build.get {build_id}` | M | compiled | +| `p4.describe {change, max_files, include_diff:false}` | P | compiled | +| `p4.changes {path, max}` | P | compiled | +| `teamcity.build.queue {build_type_id}` | X | reserved, denied | +| `jenkins.build.trigger {job}` | X | reserved, denied | +| `p4.file_contents {depot_path, rev, max_bytes}` | C | reserved, denied | +| `ghes.commit.get {repo, sha}` | P | reserved, denied | +| `horde.server.info` | M | reserved, denied | + +Classes are the egress classes from the design note: M = metadata, P = paths, +C = content, X = mutation. **No X-class and no C-class verb is compiled into +v0**, and `Selfcheck()` fails the build if one ever is. + +`p4.changes` is the one verb here beyond the three the spike was scoped to +(`teamcity.server.info`, `teamcity.build.get`, `p4.describe`). It is in the +design note's own §2.4 list, and it is compiled in because it is the natural +carrier for `depot_scope`: without a path-bearing argument, the out-of-scope +drill has nothing to deny before a tool call, and that drill is the one Shuri +singled out as the only real test of condition 2. + +--- + +## 5. Credential custody + +Every credential the connector uses comes from `connector.yml`, or from a +`*_file` path that `connector.yml` names. There is deliberately: + +- no environment-variable fallback for any credential; +- no command-line flag that takes a secret; +- no remote configuration — the broker cannot tell the connector where to find a + credential. + +If the broker could, "your credentials never leave your network" would depend on +our good behaviour rather than on the studio's file permissions. + +Enforced: `connector.yml` and every `*_file` must be mode 0600 or stricter, or +the daemon refuses to start. An unrecognised key in `connector.yml` is an error, +not a silent ignore. The redacted config rendering used by the startup banner +and the audit log never prints a secret; no code path prints the config +directly. + +Our side stores only the SHA-256 digest of the connector token, and +`credentials_ciphertext` holds nothing for a Connector-transport integration. + +--- + +## 6. Revocation and degradation + +- Revoking the connector token on our side closes the socket within one + heartbeat. The studio's own credentials are untouched, and reconnects with the + revoked token are refused at the upgrade. +- For every tool with a Tier 1 push path, no event path depends on the + connector. Stopping it removes pull verbs only. +- A verb-dependent feature checks connector presence and renders the offline + state; it records "needs connector" and moves on rather than raising. Nothing + retries a verb against an offline connector more than once. +- Horde is the stated exception: it has no push path, so it is connector-only + with no degraded mode, and its own setup docs have to say so. + +--- + +## 7. Audit + +One local audit line per command, whatever the outcome, including every rejected +and denied one: timestamp, session id, command id, verb, `SHA-256` of the +canonical arguments, status, denial reason, bytes out, duration, truncation +flag. JSON lines, mode 0600, dated files. + +Arguments are hashed rather than recorded verbatim, so the log correlates with +our side ("we sent command X, they ran command X") without becoming a second +copy of whatever the arguments contained. Denial detail strings stay local and +never travel in a `result` frame: a denial message that echoed the rejected +value back would be a small egress channel of its own. + +The log is the studio's evidence, not ours. No verb can read it. diff --git a/README.md b/README.md new file mode 100644 index 0000000..21594de --- /dev/null +++ b/README.md @@ -0,0 +1,151 @@ +# butterstack-connector (spike) + +An outbound-only daemon a studio runs inside its own network so ButterStack can +reach a private, on-premises Perforce or TeamCity **without the studio opening a +single inbound port**. + +It opens exactly one outbound TLS connection to one hostname on 443, announces +what it can do, and then executes only commands from a typed, versioned +allowlist with constrained arguments — each one logged locally. It holds the +studio's tool credentials in its own config file and never sends them. + +This is the **spike** from issue #1575, checkbox groups 0 and 1: a standalone +proof of the daemon and the protocol schema. Nothing here is deployed, and +nothing here touches the Rails app. + +- [`PROTOCOL.md`](PROTOCOL.md) — the day-1 protocol schema (issue #1575 group 0) +- [`internal/vocab/vocab.go`](internal/vocab/vocab.go) — the whole allowlist, in + one readable file, on purpose +- [`test/`](test/) — the mock broker and the seven drills + +Design sources, on branch `plan/teamcity-private-reach`: +`ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md` +§2.2–2.6, §4.3, §5, and +`ai/team/agents/shuri/reports/2026-08-29-connector-design-security-review.md` §6. + +--- + +## Build and run + +Go 1.23. No host Go install is needed; the Makefile builds in a container. + +```bash +make build # go build -> build/butterstack-connector +make test # go vet + go test ./... +make drills # the seven drills against the mock broker (needs Ruby 3.2+) +make check # test + drills +make vocabulary # print the compiled allowlist +``` + +`make build` uses `docker run golang:1.23-alpine`. If you have Go on the host, +`GO=go make build` uses it instead. + +```bash +./build/butterstack-connector -config /etc/butterstack/connector.yml +./build/butterstack-connector -print-vocabulary +``` + +## Configuration + +See [`connector.example.yml`](connector.example.yml). Two rules the daemon +enforces rather than documents: + +- **`connector.yml` and every `*_file` must be mode 0600 or stricter**, or it + refuses to start. +- **The endpoint must be `wss://` with no query string.** A copy-pasted + `?token=...` URL cannot start the daemon at all. + +Every credential comes from that file, or from a `*_file` path it names. There +is no environment-variable fallback, no flag that takes a secret, and no remote +configuration — the broker cannot tell the connector where to find a credential. + +## What is in the vocabulary + +| Verb | v0 | +|---|---| +| `sys.ping`, `sys.version`, `sys.capabilities` | compiled | +| `teamcity.server.info` | compiled | +| `teamcity.build.get {build_id}` | compiled | +| `p4.describe {change, max_files, include_diff:false}` | compiled | +| `p4.changes {path, max}` | compiled | +| `teamcity.build.queue`, `jenkins.build.trigger` | reserved, denied | +| `p4.file_contents`, `ghes.commit.get`, `horde.server.info` | reserved, denied | + +No verb accepts a host, port, URL, or shell string. No verb accepts +caller-supplied build parameters or properties — that is enforced structurally +(`bannedArgNames` plus `Selfcheck()`, which runs at process start as well as in +the tests), because a parameter map on a build-triggering verb interpolates into +shell build steps and would make the allowlist a code-execution primitive inside +the studio's LAN. No mutating verb and no content-class verb is compiled in. + +--- + +## The drills + +`make drills` runs the seven drills from design note §4.3 against +`test/mock_broker.rb`, plus a round-trip phase and the broker-side half of +drill (f). Every drill passes today: + +| | Drill | What it asserts | +|---|---|---| +| P0 | verbs round-trip | all five compiled verbs answer `ok`; results carry only the declared fields; the TeamCity token used on the LAN is the one from `connector.yml` | +| D1 | out-of-vocabulary verb | `sys.exec`, `p4.print`, … → `denied / unknown_verb`; reserved names → `denied / verb_not_compiled`; each with a local audit line | +| D2 | out-of-scope argument | `//...`, `//depot/...`, a smuggled `params`/`properties` map, a quoted integer, `include_diff:true` → all `denied`, none reaching the tool; an in-scope path carrying shell metacharacters reaches `p4` as one literal argv element and no shell runs | +| D3 | query-string token | the daemon refuses such an endpoint; the broker answers HTTP 400 and never a 101; no session state is allocated; missing and wrong bearer tokens are 401 | +| F* | cross-session result | a `result` whose command id the session never issued is discarded, not dispatched by id alone | +| R1 | network drop | the session dies and the connector reconnects with no operator action | +| R2 | connector stopped | we flip to offline; a verb-dependent feature renders "needs connector" instead of raising; the daemon logs a clean shutdown | +| R3 | our side stopped | the connector backs off, logs it, and reconnects when the broker returns | +| R4 | token revoked | the socket closes within one heartbeat; reconnects are refused; `connector.yml` is byte-identical and still 0600; and **no tool credential, LAN host, port, or URL ever appeared in a frame** | + +The mock broker is not the broker. The real one is a dedicated Rack endpoint at +`/connect` on the Rails app with hashed-token auth, Redis-routed command and +result, and explicit tenant scoping — a later PR. What `test/mock_broker.rb` +models is the surface these drills need, and it does implement faithfully the +four rules they exist to prove: refusal before session-state allocation, +header-only tokens, SHA-256 digest storage with constant-time compare, and +per-session result matching. + +--- + +## What this spike does **not** prove + +Carried forward from design note §5 and Shuri §6 item 7, plus what this +standalone shape adds: + +- **The argument-constraint layer end to end.** The drills prove denial at the + frame boundary against a mock broker. They do not prove it against a real + broker, a real TeamCity, or a real p4d. +- **Anything on the Rails side.** There is no `/connect` endpoint in this PR, no + ActionCable change, no migration, no UI. The tenant-context drill — "assert + tenant context is nil at the start of a request that follows a connector frame + on the same Puma thread" — is Rails-side and is **not** covered here. Only the + broker-side half of drill (f) is. +- **Anything on real infrastructure.** Nothing ran against staging, demo, or + production. No terraform, no security group, no hostname, no certificate. + Stage A (the Tier 1 TeamCity webhook run) has not been run, and it is gated on + the #1574 Phase -1 app fixes landing first; the "no token in + `webhook_events.payload` or the app log" drill therefore has no result yet. +- **The frame codec against an independent production stack.** Both ends here + were written from RFC 6455 — the Go client and the Ruby server independently, + which is why a masking or handshake mistake shows up as a failed drill. But + neither has met a real ALB, a real nginx `Upgrade` hop, or a real proxy. +- **Latency over a home connection.** The drills run on loopback. The design's + under-2-second target is untested against a NATed home network, and the + `ss`/`netstat` capture showing exactly one outbound established connection and + zero listeners has not been taken. +- **Survival conditions 1, 4, and 5.** No Sigstore keyless signing, no SBOM, no + build-from-source instructions, no digest-pinned base image, no version-skew + handling, and no `egress.md` with a per-verb output schema enforced as a field + allowlist with a conformance test. The fixed `fields=` projections in the + TeamCity executor are the beginning of that, not the whole of it. +- **Scale and multi-node routing.** Puma behaviour at tens of connectors, socket + routing under a real ASG scale-out, and the per-integration connection cap and + per-session command budget in the broker. +- **Everything beyond the five compiled verbs.** No Jenkins, GHES, or Horde + verb; no Perforce verb beyond `describe` and `changes`; no mutating verb; no + content verb; no poll-loop mode; no Windows service. +- **An actual IT-director review.** Appendix B is a script, not a test. + +This is the go/no-go input for the build, and it is deliberately smaller than +the product. diff --git a/cmd/butterstack-connector/main.go b/cmd/butterstack-connector/main.go new file mode 100644 index 0000000..4759612 --- /dev/null +++ b/cmd/butterstack-connector/main.go @@ -0,0 +1,133 @@ +// Command butterstack-connector is the outbound-only daemon a studio runs +// inside its own network so ButterStack can reach a private Perforce or +// TeamCity without the studio opening a single inbound port. +// +// It opens exactly one outbound TLS connection to one hostname on 443 and never +// listens on anything. Everything it will do is in internal/vocab/vocab.go. +// +// Spike scope (issue #1575, group 1): teamcity.server.info, teamcity.build.get, +// p4.describe, p4.changes and the sys verbs. No mutating verb and no +// content-class verb is compiled into this build. +package main + +import ( + "context" + "flag" + "fmt" + "os" + "os/signal" + "syscall" + + "github.com/ButterStack/butterstack-connector/internal/audit" + "github.com/ButterStack/butterstack-connector/internal/config" + "github.com/ButterStack/butterstack-connector/internal/protocol" + "github.com/ButterStack/butterstack-connector/internal/session" + "github.com/ButterStack/butterstack-connector/internal/vocab" +) + +// Version is the connector build version, reported in hello and in sys.version. +// It is overridden at build time with -ldflags "-X main.Version=...". +var Version = "0.0.0-spike" + +func main() { + var ( + cfgPath = flag.String("config", "/etc/butterstack/connector.yml", "path to connector.yml") + showVocab = flag.Bool("print-vocabulary", false, "print the compiled command allowlist and exit") + showVer = flag.Bool("version", false, "print the version and exit") + ) + flag.Parse() + + if *showVer { + fmt.Printf("butterstack-connector %s (protocol v%s)\n", Version, protocol.Version) + return + } + if *showVocab { + printVocabulary() + return + } + + if err := run(*cfgPath); err != nil { + fmt.Fprintf(os.Stderr, "butterstack-connector: %v\n", err) + os.Exit(1) + } +} + +func run(cfgPath string) error { + // The vocabulary's structural invariants are checked before anything else, + // so a build whose allowlist grew a banned argument refuses to start rather + // than running with a quietly weaker guarantee. + if err := vocab.Selfcheck(); err != nil { + return err + } + + cfg, err := config.Load(cfgPath) + if err != nil { + return err + } + + log, err := audit.New(cfg.LogDir, os.Stderr) + if err != nil { + return err + } + defer log.Close() + + log.Event("startup", fmt.Sprintf("butterstack-connector %s protocol v%s", Version, protocol.Version)) + for _, line := range splitLines(cfg.Redacted()) { + log.Event("config", line) + } + if os.Geteuid() == 0 { + log.Event("warning", "running as root; the install doc asks for a dedicated non-root user") + } + + runner, err := session.New(cfg, log, Version) + if err != nil { + return err + } + + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + err = runner.Run(ctx) + log.Event("shutdown", "signal received; socket closed and audit log flushed") + return err +} + +func printVocabulary() { + fmt.Printf("butterstack-connector %s, protocol v%s\n\n", Version, protocol.Version) + for i := range vocab.Vocabulary { + v := &vocab.Vocabulary[i] + state := "RESERVED (denied)" + if v.Compiled { + state = "compiled" + } + fmt.Printf("%-26s class=%s %s\n", v.Name, v.Class, state) + for j := range v.Args { + a := &v.Args[j] + req := "" + if a.Required { + req = " required" + } + scope := "" + if a.Scope != "" { + scope = fmt.Sprintf(" scope=%s", a.Scope) + } + fmt.Printf(" %-16s %s%s%s\n", a.Name, a.Kind, req, scope) + } + } + fmt.Printf("\nNo verb accepts a host, port, URL, or shell string. There is no sys.exec.\n") +} + +func splitLines(s string) []string { + var out []string + start := 0 + for i := 0; i < len(s); i++ { + if s[i] == '\n' { + out = append(out, s[start:i]) + start = i + 1 + } + } + if start < len(s) { + out = append(out, s[start:]) + } + return out +} diff --git a/connector.example.yml b/connector.example.yml new file mode 100644 index 0000000..d6b0128 --- /dev/null +++ b/connector.example.yml @@ -0,0 +1,76 @@ +# butterstack-connector configuration. +# +# This file holds live credentials. It must be mode 0600 or stricter, owned by +# the user the connector runs as; the daemon refuses to start otherwise. Prefer +# the *_file forms if you inject secrets from a vault. +# +# install -m 0600 connector.example.yml /etc/butterstack/connector.yml +# +# Every credential the connector uses comes from this file, or from a *_file +# path this file names. There is no environment-variable fallback and no flag +# that takes a secret, so what leaves your network is bounded by what is here. + +# The one hostname your egress rule needs. Must be wss:// and must carry no +# query string: the connector token is sent in the Authorization header only. +endpoint: wss://connect.butterstack.com/connect + +# Optional. Pins the trust anchor for the endpoint above, for a private CA or a +# TLS-inspecting proxy. There is no option to skip verification. +# endpoint_ca_file: /etc/butterstack/corporate-ca.pem + +# Issued in the ButterStack UI, shown exactly once. We store only its SHA-256 +# digest and cannot recover it; revoking it closes the socket and leaves every +# credential below untouched. +token: bsc_REPLACE_ME_REPLACE_ME_REPLACE_ME_REPLACE_ME +# token_file: /etc/butterstack/connector.token + +# A name for this host, shown in the Connection Status panel. +connector_id: studio-build-01 + +# Local audit log: one JSON line per command, including every denial. +log_dir: /var/log/butterstack-connector + +# Commands executed in parallel. 1..32. +max_concurrent: 4 + +# The argument-constraint lists. These live here and only here: no scope value +# is ever accepted over the socket. An in-vocabulary command whose argument +# falls outside these is denied exactly like an unknown verb, and logged. +scopes: + # Literal depot prefixes, no wildcards. A path whose literal prefix is not + # inside one of these is denied, so `//...` is refused even if your P4 user + # could read it. + depot_scope: + - //depot/game/ + # For the reserved teamcity.build.queue verb. Not compiled in v0. + allowed_build_types: [] + # For the reserved ghes.* verbs. Not compiled in v0. + repo_allowlist: [] + +toggles: + # Content-class verbs (file contents, diffs, log tails) are off in v0 at the + # schema level as well; this switch cannot turn one on yet. + content_verbs: false + +perforce: + enabled: false + binary: p4 + port: ssl:perforce.studio.lan:1666 + # A read-only user you scope in your own protections table. That remains the + # primary bound; depot_scope above is the second one. + user: butterstack-ro + # ticket: ... # prefer ticket_file + ticket_file: /etc/butterstack/p4.ticket + timeout: 20s + +teamcity: + enabled: false + url: https://teamcity.studio.lan + # A project-limited access token with a read-only role. + # token: ... # prefer token_file + token_file: /etc/butterstack/teamcity.token + # For a self-signed certificate on your LAN TeamCity. This applies to the LAN + # server only; the connection to ButterStack is always verified. + # ca_file: /etc/butterstack/teamcity-ca.pem + # allow_insecure_tls: false + timeout: 10s diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..ec90cd6 --- /dev/null +++ b/go.mod @@ -0,0 +1,5 @@ +module github.com/ButterStack/butterstack-connector + +go 1.23 + +require gopkg.in/yaml.v3 v3.0.1 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..a62c313 --- /dev/null +++ b/go.sum @@ -0,0 +1,4 @@ +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/audit/audit.go b/internal/audit/audit.go new file mode 100644 index 0000000..f6d29e2 --- /dev/null +++ b/internal/audit/audit.go @@ -0,0 +1,127 @@ +// Package audit writes the connector's local audit log: one line per command, +// including every rejected and denied one. +// +// The log is the studio's evidence, not ours. It stays on the studio's disk and +// no verb can read it. Arguments are recorded as a SHA-256 of their canonical +// JSON rather than verbatim, so the log is useful for correlating with our side +// ("we sent command X, they ran command X") without becoming a second copy of +// whatever the arguments contained. +package audit + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "io" + "os" + "path/filepath" + "sync" + "time" +) + +// Entry is one audit line. +type Entry struct { + TS string `json:"ts"` + Event string `json:"event"` + SessionID string `json:"session_id,omitempty"` + CommandID string `json:"command_id,omitempty"` + Verb string `json:"verb,omitempty"` + ArgsSHA256 string `json:"args_sha256,omitempty"` + Status string `json:"status,omitempty"` + Reason string `json:"reason,omitempty"` + Detail string `json:"detail,omitempty"` + BytesOut int `json:"bytes_out,omitempty"` + DurationMS int64 `json:"duration_ms,omitempty"` + Truncated bool `json:"truncated,omitempty"` + Message string `json:"message,omitempty"` +} + +// Logger appends JSON lines to a dated file and mirrors them to stderr. +type Logger struct { + mu sync.Mutex + dir string + day string + file *os.File + mirror io.Writer +} + +// New opens (or creates) the audit directory. +func New(dir string, mirror io.Writer) (*Logger, error) { + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("audit: %w", err) + } + return &Logger{dir: dir, mirror: mirror}, nil +} + +func (l *Logger) rotate(now time.Time) error { + day := now.UTC().Format("2006-01-02") + if l.file != nil && l.day == day { + return nil + } + if l.file != nil { + _ = l.file.Close() + } + path := filepath.Join(l.dir, "audit-"+day+".log") + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + if err != nil { + return fmt.Errorf("audit: %w", err) + } + l.file, l.day = f, day + return nil +} + +// Write emits one entry. +func (l *Logger) Write(e Entry) { + now := time.Now().UTC() + if e.TS == "" { + e.TS = now.Format(time.RFC3339Nano) + } + b, err := json.Marshal(e) + if err != nil { + return + } + b = append(b, '\n') + + l.mu.Lock() + defer l.mu.Unlock() + if err := l.rotate(now); err == nil && l.file != nil { + _, _ = l.file.Write(b) + } + if l.mirror != nil { + _, _ = l.mirror.Write(b) + } +} + +// Event records a non-command line (connect, reconnect, revoke, shutdown). +func (l *Logger) Event(event, message string) { + l.Write(Entry{Event: event, Message: message}) +} + +// Close flushes and closes the current file. +func (l *Logger) Close() error { + l.mu.Lock() + defer l.mu.Unlock() + if l.file == nil { + return nil + } + err := l.file.Close() + l.file = nil + return err +} + +// ArgsDigest is the canonical hash recorded for a command's arguments. +func ArgsDigest(raw []byte) string { + if len(raw) == 0 { + raw = []byte("{}") + } + // Canonicalise through a map so key order does not change the digest. + var m map[string]any + if err := json.Unmarshal(raw, &m); err == nil { + if c, err := json.Marshal(m); err == nil { + raw = c + } + } + sum := sha256.Sum256(raw) + return hex.EncodeToString(sum[:]) +} diff --git a/internal/config/config.go b/internal/config/config.go new file mode 100644 index 0000000..9328148 --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,314 @@ +// Package config loads connector.yml. +// +// Survival condition 3 is local credential custody, and the way this package +// keeps that promise is narrow and testable: every credential the connector +// uses is read from the YAML file the studio wrote, or from a *_file path that +// YAML names. There is deliberately no environment-variable fallback, no +// command-line flag that takes a secret, and no remote configuration: the +// broker cannot tell the connector where to find a credential, because if it +// could, "your credentials never leave your network" would depend on our good +// behaviour rather than on the studio's file permissions. +package config + +import ( + "errors" + "fmt" + "net/url" + "os" + "path/filepath" + "regexp" + "strings" + "time" + + "gopkg.in/yaml.v3" +) + +// tokenPattern is the connector token format from the design note: +// bsc__<32 random bytes, base32>. The prefix makes the +// credential greppable by secret scanners, ours and the studio's; the id +// segment makes the broker's hashed lookup a primary-key read. +var tokenPattern = regexp.MustCompile(`\Absc_[a-z0-9][a-z0-9\-]{0,62}_[A-Za-z2-7]{32,128}\z`) + +// Duration is a YAML-friendly time.Duration ("25s", "10s"). +type Duration time.Duration + +func (d *Duration) UnmarshalYAML(n *yaml.Node) error { + var s string + if err := n.Decode(&s); err != nil { + return err + } + v, err := time.ParseDuration(s) + if err != nil { + return fmt.Errorf("not a duration: %q", s) + } + *d = Duration(v) + return nil +} + +func (d Duration) D() time.Duration { return time.Duration(d) } + +// Config is the whole of connector.yml. +type Config struct { + Endpoint string `yaml:"endpoint"` + EndpointCAFile string `yaml:"endpoint_ca_file"` + Token string `yaml:"token"` + TokenFile string `yaml:"token_file"` + ConnectorID string `yaml:"connector_id"` + LogDir string `yaml:"log_dir"` + MaxConcurrent int `yaml:"max_concurrent"` + Scopes Scopes `yaml:"scopes"` + Toggles Toggles `yaml:"toggles"` + Perforce Perforce `yaml:"perforce"` + TeamCity TeamCity `yaml:"teamcity"` + + // path is where this config was loaded from; not a YAML field. + path string +} + +// Scopes are the argument-constraint lists. They live here and only here: no +// scope value is ever accepted from the wire. +type Scopes struct { + DepotScope []string `yaml:"depot_scope"` + AllowedBuildTypes []string `yaml:"allowed_build_types"` + RepoAllowlist []string `yaml:"repo_allowlist"` +} + +// Toggles are the studio's local switches. Content verbs are off in v0 at the +// schema level as well, so this switch cannot turn one on; it is here so the +// file shape does not change when they land. +type Toggles struct { + ContentVerbs bool `yaml:"content_verbs"` +} + +// Perforce is the local Helix Core connection. Note that port, user, and +// ticket are all local: no verb carries them. +type Perforce struct { + Enabled bool `yaml:"enabled"` + Binary string `yaml:"binary"` + Port string `yaml:"port"` + User string `yaml:"user"` + Ticket string `yaml:"ticket"` + TicketFile string `yaml:"ticket_file"` + Timeout Duration `yaml:"timeout"` +} + +// TeamCity is the local TeamCity server. allow_insecure_tls is deliberately +// scoped to this LAN server only; there is no equivalent switch for the broker +// connection, which always verifies. +type TeamCity struct { + Enabled bool `yaml:"enabled"` + URL string `yaml:"url"` + Token string `yaml:"token"` + TokenFile string `yaml:"token_file"` + CAFile string `yaml:"ca_file"` + AllowInsecureTLS bool `yaml:"allow_insecure_tls"` + Timeout Duration `yaml:"timeout"` +} + +// Load reads, permission-checks, and validates connector.yml. +func Load(path string) (*Config, error) { + if err := checkSecretFileMode(path); err != nil { + return nil, err + } + raw, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("config: %w", err) + } + var c Config + dec := yaml.NewDecoder(strings.NewReader(string(raw))) + dec.KnownFields(true) // an unrecognised key is an error, not a silent ignore + if err := dec.Decode(&c); err != nil { + return nil, fmt.Errorf("config: %s: %w", path, err) + } + c.path = path + c.applyDefaults() + if err := c.resolveSecretFiles(); err != nil { + return nil, err + } + if err := c.Validate(); err != nil { + return nil, err + } + return &c, nil +} + +func (c *Config) applyDefaults() { + if c.MaxConcurrent == 0 { + c.MaxConcurrent = 4 + } + if c.Perforce.Binary == "" { + c.Perforce.Binary = "p4" + } + if c.Perforce.Timeout == 0 { + c.Perforce.Timeout = Duration(20 * time.Second) + } + if c.TeamCity.Timeout == 0 { + c.TeamCity.Timeout = Duration(10 * time.Second) + } + if c.LogDir == "" { + c.LogDir = filepath.Join(filepath.Dir(c.path), "logs") + } +} + +// resolveSecretFiles implements the *_file indirection. A studio with a vault +// injects a file; a studio without one writes the value inline. Either way the +// value is on the studio's disk under the studio's permissions and never +// arrives over the socket. +func (c *Config) resolveSecretFiles() error { + pairs := []struct { + name string + value *string + file string + }{ + {"token", &c.Token, c.TokenFile}, + {"perforce.ticket", &c.Perforce.Ticket, c.Perforce.TicketFile}, + {"teamcity.token", &c.TeamCity.Token, c.TeamCity.TokenFile}, + } + for _, p := range pairs { + if p.file == "" { + continue + } + if *p.value != "" { + return fmt.Errorf("config: %s and %s_file are both set; pick one", p.name, p.name) + } + if err := checkSecretFileMode(p.file); err != nil { + return err + } + b, err := os.ReadFile(p.file) + if err != nil { + return fmt.Errorf("config: %s_file: %w", p.name, err) + } + *p.value = strings.TrimSpace(string(b)) + } + return nil +} + +// checkSecretFileMode fails closed on a credential file any other local user +// can read. The install doc asks for 0600; this enforces it rather than hoping. +func checkSecretFileMode(path string) error { + st, err := os.Stat(path) + if err != nil { + return fmt.Errorf("config: %w", err) + } + if st.IsDir() { + return fmt.Errorf("config: %s is a directory", path) + } + if perm := st.Mode().Perm(); perm&0o077 != 0 { + return fmt.Errorf("config: %s is mode %04o; credential files must be 0600 "+ + "(no group or other access)", path, perm) + } + return nil +} + +// ErrInsecureEndpoint is returned for anything but a verified wss:// endpoint. +var ErrInsecureEndpoint = errors.New("config: endpoint must be a wss:// URL") + +// Validate is where the transport rules that matter to security live. +func (c *Config) Validate() error { + if c.Endpoint == "" { + return errors.New("config: endpoint is required") + } + u, err := url.Parse(c.Endpoint) + if err != nil { + return fmt.Errorf("config: endpoint: %w", err) + } + if u.Scheme != "wss" { + return fmt.Errorf("%w (got %q)", ErrInsecureEndpoint, u.Scheme) + } + if u.Host == "" { + return errors.New("config: endpoint has no host") + } + // The token travels in a header and nowhere else. A query string on the + // endpoint is refused at load time so that a copy-pasted ?token=... URL + // cannot start the daemon at all -- the client half of the rule the broker + // enforces on its side. + if u.RawQuery != "" || strings.Contains(c.Endpoint, "?") { + return errors.New("config: endpoint must not carry a query string; the " + + "connector token is sent in the Authorization header only") + } + if u.User != nil { + return errors.New("config: endpoint must not carry userinfo credentials") + } + if u.Fragment != "" { + return errors.New("config: endpoint must not carry a fragment") + } + + if c.Token == "" { + return errors.New("config: token or token_file is required") + } + if !tokenPattern.MatchString(c.Token) { + return errors.New("config: token is not a connector token " + + "(expected bsc__)") + } + + if c.MaxConcurrent < 1 || c.MaxConcurrent > 32 { + return fmt.Errorf("config: max_concurrent must be 1..32, got %d", c.MaxConcurrent) + } + + if c.Perforce.Enabled { + if c.Perforce.Port == "" || c.Perforce.User == "" { + return errors.New("config: perforce.enabled needs port and user") + } + if len(c.Scopes.DepotScope) == 0 { + return errors.New("config: perforce.enabled needs at least one " + + "scopes.depot_scope entry; an unscoped depot verb is denied anyway") + } + for _, p := range c.Scopes.DepotScope { + if !strings.HasPrefix(p, "//") { + return fmt.Errorf("config: depot_scope %q must begin //", p) + } + if strings.ContainsAny(p, "*") || strings.Contains(p, "...") { + return fmt.Errorf("config: depot_scope %q must be a literal prefix, not a wildcard", p) + } + } + } + if c.TeamCity.Enabled { + if c.TeamCity.URL == "" || c.TeamCity.Token == "" { + return errors.New("config: teamcity.enabled needs url and token (or token_file)") + } + tu, err := url.Parse(c.TeamCity.URL) + if err != nil || tu.Host == "" || (tu.Scheme != "http" && tu.Scheme != "https") { + return fmt.Errorf("config: teamcity.url must be an http(s) URL, got %q", c.TeamCity.URL) + } + } + return nil +} + +// Tools reports which connector.yml sections are enabled, for the vocabulary's +// tool-configuration check. +func (c *Config) Tools() map[string]bool { + return map[string]bool{ + "perforce": c.Perforce.Enabled, + "teamcity": c.TeamCity.Enabled, + } +} + +// IntegrationID is the middle segment of the connector token. It identifies the +// integration to the broker; the broker re-derives it from the authenticated +// session rather than trusting this, but the connector reports it in hello. +func (c *Config) IntegrationID() string { + parts := strings.Split(c.Token, "_") + if len(parts) < 3 { + return "" + } + return parts[1] +} + +// Redacted renders the config for the log with every credential removed. The +// audit log and the startup banner both use this; no code path prints Config +// directly. +func (c *Config) Redacted() string { + var b strings.Builder + fmt.Fprintf(&b, "endpoint=%s connector_id=%s integration_id=%s max_concurrent=%d\n", + c.Endpoint, c.ConnectorID, c.IntegrationID(), c.MaxConcurrent) + fmt.Fprintf(&b, "token=bsc_%s_[redacted] log_dir=%s\n", c.IntegrationID(), c.LogDir) + fmt.Fprintf(&b, "scopes.depot_scope=%v allowed_build_types=%v repo_allowlist=%v\n", + c.Scopes.DepotScope, c.Scopes.AllowedBuildTypes, c.Scopes.RepoAllowlist) + fmt.Fprintf(&b, "perforce.enabled=%t port=%s user=%s ticket=[redacted]\n", + c.Perforce.Enabled, c.Perforce.Port, c.Perforce.User) + fmt.Fprintf(&b, "teamcity.enabled=%t url=%s token=[redacted]", + c.TeamCity.Enabled, c.TeamCity.URL) + return b.String() +} + +// Path returns where this config was loaded from. +func (c *Config) Path() string { return c.path } diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..69cee69 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,190 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +const goodToken = "bsc_intg7f3a_MFRGGZDFMZTWQ2LKNNWG23TPOBYXE43U" + +func writeCfg(t *testing.T, body string, mode os.FileMode) string { + t.Helper() + dir := t.TempDir() + p := filepath.Join(dir, "connector.yml") + if err := os.WriteFile(p, []byte(body), mode); err != nil { + t.Fatal(err) + } + return p +} + +func base(extra string) string { + return `endpoint: wss://staging.butterstack.com/connect +connector_id: home-lab-1 +token: ` + goodToken + ` +` + extra +} + +func TestLoadsAValidConfig(t *testing.T) { + p := writeCfg(t, base(`scopes: + depot_scope: + - //depot/game/ +teamcity: + enabled: true + url: http://localhost:8111 + token: tc-token +`), 0o600) + c, err := Load(p) + if err != nil { + t.Fatalf("load: %v", err) + } + if c.IntegrationID() != "intg7f3a" { + t.Fatalf("integration id = %q", c.IntegrationID()) + } + if !c.Tools()["teamcity"] || c.Tools()["perforce"] { + t.Fatalf("tools = %v", c.Tools()) + } +} + +// TestWorldReadableConfigIsRefused: condition 3 says the studio's credentials +// live in a 0600 file on the studio's host. This enforces it instead of +// documenting it. +func TestWorldReadableConfigIsRefused(t *testing.T) { + p := writeCfg(t, base(""), 0o644) + _, err := Load(p) + if err == nil || !strings.Contains(err.Error(), "0600") { + t.Fatalf("want a permission refusal, got %v", err) + } +} + +// TestQueryStringEndpointIsRefused is drill (g)'s client half: a copy-pasted +// ?token= URL cannot start the daemon at all. +func TestQueryStringEndpointIsRefused(t *testing.T) { + p := writeCfg(t, `endpoint: wss://staging.butterstack.com/connect?token=`+goodToken+` +token: `+goodToken+` +`, 0o600) + _, err := Load(p) + if err == nil || !strings.Contains(err.Error(), "query string") { + t.Fatalf("want a query-string refusal, got %v", err) + } +} + +func TestPlaintextAndUserinfoEndpointsAreRefused(t *testing.T) { + for _, ep := range []string{ + "ws://staging.butterstack.com/connect", + "https://staging.butterstack.com/connect", + "wss://user:pass@staging.butterstack.com/connect", + } { + p := writeCfg(t, "endpoint: "+ep+"\ntoken: "+goodToken+"\n", 0o600) + if _, err := Load(p); err == nil { + t.Errorf("%s was accepted", ep) + } + } +} + +func TestMalformedTokenIsRefused(t *testing.T) { + for _, tok := range []string{"hunter2", "bsc_short", "bsc_intg_abc", "Bearer bsc_a_MFRGGZDFMZTWQ2LKNNWG23TPOBYXE43U"} { + p := writeCfg(t, "endpoint: wss://x.example/connect\ntoken: "+tok+"\n", 0o600) + if _, err := Load(p); err == nil { + t.Errorf("token %q was accepted", tok) + } + } +} + +// TestNoEnvironmentCredentialFallback pins the custody rule: the only source of +// a credential is the file the studio wrote. +func TestNoEnvironmentCredentialFallback(t *testing.T) { + t.Setenv("BUTTERSTACK_CONNECTOR_TOKEN", goodToken) + t.Setenv("CONNECTOR_TOKEN", goodToken) + p := writeCfg(t, "endpoint: wss://x.example/connect\n", 0o600) + if _, err := Load(p); err == nil { + t.Fatal("a config with no token loaded; an environment variable must not fill it in") + } +} + +func TestSecretFileIndirection(t *testing.T) { + dir := t.TempDir() + tf := filepath.Join(dir, "token") + if err := os.WriteFile(tf, []byte(goodToken+"\n"), 0o600); err != nil { + t.Fatal(err) + } + p := filepath.Join(dir, "connector.yml") + if err := os.WriteFile(p, []byte("endpoint: wss://x.example/connect\ntoken_file: "+tf+"\n"), 0o600); err != nil { + t.Fatal(err) + } + c, err := Load(p) + if err != nil { + t.Fatalf("load: %v", err) + } + if c.Token != goodToken { + t.Fatalf("token not read from token_file") + } + + // A world-readable secret file is refused for the same reason as the config. + if err := os.Chmod(tf, 0o644); err != nil { + t.Fatal(err) + } + if _, err := Load(p); err == nil { + t.Fatal("a 0644 token_file was accepted") + } +} + +func TestUnknownKeyIsAnError(t *testing.T) { + p := writeCfg(t, base("tunnel: true\n"), 0o600) + if _, err := Load(p); err == nil { + t.Fatal("an unrecognised connector.yml key was ignored instead of refused") + } +} + +func TestPerforceNeedsADepotScope(t *testing.T) { + p := writeCfg(t, base(`perforce: + enabled: true + port: ssl:p4.lan:1666 + user: butterstack-ro +`), 0o600) + if _, err := Load(p); err == nil || !strings.Contains(err.Error(), "depot_scope") { + t.Fatalf("want a depot_scope requirement, got %v", err) + } +} + +func TestWildcardDepotScopeIsRefused(t *testing.T) { + p := writeCfg(t, base(`scopes: + depot_scope: + - //... +perforce: + enabled: true + port: ssl:p4.lan:1666 + user: butterstack-ro +`), 0o600) + if _, err := Load(p); err == nil { + t.Fatal("a wildcard depot_scope was accepted") + } +} + +// TestRedactedNeverPrintsASecret guards the audit log and the startup banner. +func TestRedactedNeverPrintsASecret(t *testing.T) { + p := writeCfg(t, base(`teamcity: + enabled: true + url: http://localhost:8111 + token: TEAMCITY-SUPER-SECRET +perforce: + enabled: true + port: ssl:p4.lan:1666 + user: butterstack-ro + ticket: P4-TICKET-SECRET +scopes: + depot_scope: + - //depot/game/ +`), 0o600) + c, err := Load(p) + if err != nil { + t.Fatal(err) + } + out := c.Redacted() + for _, secret := range []string{"TEAMCITY-SUPER-SECRET", "P4-TICKET-SECRET", "MFRGGZDFMZTWQ2LKNNWG23TPOBYXE43U"} { + if strings.Contains(out, secret) { + t.Errorf("Redacted() leaked %q:\n%s", secret, out) + } + } +} diff --git a/internal/protocol/protocol.go b/internal/protocol/protocol.go new file mode 100644 index 0000000..6af3b27 --- /dev/null +++ b/internal/protocol/protocol.go @@ -0,0 +1,146 @@ +// Package protocol defines the wire frames of the ButterStack Connector +// protocol v0, exactly as specified in connector/PROTOCOL.md. +// +// Every frame is a single JSON object carrying a "type" discriminator. There +// is no binary frame type and no streaming: a verb that would return more than +// its max_bytes budget truncates and says so. +package protocol + +import ( + "encoding/json" + "errors" + "fmt" +) + +// Version is the protocol version this build speaks. The broker compares it +// against min_supported_version and may reject. +const Version = "0" + +// Frame type discriminators. +const ( + TypeHello = "hello" + TypeWelcome = "welcome" + TypeReject = "reject" + TypeCommand = "command" + TypeResult = "result" + TypeHeartbeat = "heartbeat" +) + +// Result statuses. "denied" is reserved for the allowlist and the argument +// constraint layer; a tool that answers with an error is "error". Keeping them +// distinct is what makes the denial drills legible in the audit log. +const ( + StatusOK = "ok" + StatusError = "error" + StatusDenied = "denied" + StatusTimeout = "timeout" +) + +// Envelope is enough to route an inbound frame to its concrete type. +type Envelope struct { + Type string `json:"type"` +} + +// Hello is the connector's first frame. It is egress: every field here leaves +// the studio network on every connect and belongs in egress.md. +type Hello struct { + Type string `json:"type"` + ConnectorID string `json:"connector_id"` + IntegrationID string `json:"integration_id"` + Version string `json:"version"` + Protocol string `json:"protocol"` + Capabilities []string `json:"capabilities"` + ToolVersions map[string]string `json:"tool_versions,omitempty"` +} + +// Welcome is the broker's acceptance frame. +type Welcome struct { + Type string `json:"type"` + SessionID string `json:"session_id"` + ServerTime string `json:"server_time"` + MinSupportedVersion string `json:"min_supported_version"` + HeartbeatInterval int `json:"heartbeat_interval"` +} + +// Reject is the broker's refusal frame. A reject is a log line on the studio +// side, never an alert: the connector backs off and retries. +type Reject struct { + Type string `json:"type"` + Reason string `json:"reason"` +} + +// Command is the only frame that can cause the connector to touch a studio +// tool. Note what is absent: no host, no port, no URL, no shell string. Those +// live in connector.yml and never on the wire (survival condition 2). +type Command struct { + Type string `json:"type"` + ID string `json:"id"` + Verb string `json:"verb"` + Args json.RawMessage `json:"args"` + DeadlineMs int `json:"deadline_ms"` + MaxBytes int `json:"max_bytes"` +} + +// Result answers exactly one Command, keyed by its id. +type Result struct { + Type string `json:"type"` + ID string `json:"id"` + Status string `json:"status"` + Reason string `json:"reason,omitempty"` + Body any `json:"body,omitempty"` + Truncated bool `json:"truncated"` + Bytes int `json:"bytes"` +} + +// Heartbeat is egress too (queue depth is a signal about the studio's load). +type Heartbeat struct { + Type string `json:"type"` + TS string `json:"ts"` + QueueDepth int `json:"queue_depth"` +} + +// ErrUnknownFrame is returned for a frame type this protocol version does not +// define. The connector treats it as a no-op and logs it rather than closing: +// an older connector talking to a newer broker must degrade, not break. +var ErrUnknownFrame = errors.New("protocol: unknown frame type") + +// DecodeEnvelope reads just the discriminator. +func DecodeEnvelope(b []byte) (string, error) { + var e Envelope + if err := json.Unmarshal(b, &e); err != nil { + return "", fmt.Errorf("protocol: malformed frame: %w", err) + } + if e.Type == "" { + return "", errors.New("protocol: frame has no type") + } + return e.Type, nil +} + +// NewHello builds the connector's opening frame. +func NewHello(connectorID, integrationID, version string, capabilities []string, toolVersions map[string]string) Hello { + return Hello{ + Type: TypeHello, + ConnectorID: connectorID, + IntegrationID: integrationID, + Version: version, + Protocol: Version, + Capabilities: capabilities, + ToolVersions: toolVersions, + } +} + +// Deny builds a denied result. Reason is a stable machine token, not prose: +// the drills assert on it and the audit log records it. +func Deny(id, reason string) Result { + return Result{Type: TypeResult, ID: id, Status: StatusDenied, Reason: reason} +} + +// Errorf builds an error result. +func Errorf(id, format string, a ...any) Result { + return Result{Type: TypeResult, ID: id, Status: StatusError, Reason: fmt.Sprintf(format, a...)} +} + +// OK builds a successful result. +func OK(id string, body any, bytes int, truncated bool) Result { + return Result{Type: TypeResult, ID: id, Status: StatusOK, Body: body, Bytes: bytes, Truncated: truncated} +} diff --git a/internal/session/session.go b/internal/session/session.go new file mode 100644 index 0000000..7e2c445 --- /dev/null +++ b/internal/session/session.go @@ -0,0 +1,431 @@ +// Package session is the connector's run loop: dial, hello, heartbeat, +// dispatch, reconnect. +// +// The loop never gives up and never escalates. A broker that is down, a token +// that was revoked, and a network that dropped all produce the same studio-side +// behaviour: a log line and a backoff. That is survival condition 6 seen from +// the inside -- stopping the connector, or losing it, is a state change, not an +// outage. +package session + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "math/rand" + "sync" + "time" + + "github.com/ButterStack/butterstack-connector/internal/audit" + "github.com/ButterStack/butterstack-connector/internal/config" + "github.com/ButterStack/butterstack-connector/internal/protocol" + "github.com/ButterStack/butterstack-connector/internal/tools" + "github.com/ButterStack/butterstack-connector/internal/vocab" + "github.com/ButterStack/butterstack-connector/internal/wsclient" +) + +// Backoff bounds. Capped and jittered so a fleet of connectors reconnecting +// after a deploy does not arrive in lockstep. +const ( + backoffMin = 500 * time.Millisecond + backoffMax = 60 * time.Second + + defaultHeartbeat = 25 * time.Second + minHeartbeat = 5 * time.Second + maxHeartbeat = 120 * time.Second + + // readSlack is how long past the expected heartbeat interval the connector + // waits before concluding the socket is dead. + readSlack = 2 + + // dedupeWindow is the sliding window for duplicate command ids. + dedupeWindow = 10 * time.Minute + dedupeMaxKeys = 4096 + + maxDeadline = 60 * time.Second + defaultDeadline = 15 * time.Second +) + +// Runner owns one connector process's connection lifecycle. +type Runner struct { + cfg *config.Config + log *audit.Logger + version string + + execs map[string]tools.Executor + + mu sync.Mutex + seen map[string]time.Time + sessionID string +} + +// New builds a Runner. Executors are constructed once, from config, so no +// per-command code path can choose a different host or credential. +func New(cfg *config.Config, log *audit.Logger, version string) (*Runner, error) { + if err := vocab.Selfcheck(); err != nil { + return nil, err + } + r := &Runner{cfg: cfg, log: log, version: version, execs: map[string]tools.Executor{}, seen: map[string]time.Time{}} + + if cfg.TeamCity.Enabled { + tc, err := tools.NewTeamCity(cfg.TeamCity) + if err != nil { + return nil, err + } + r.execs["teamcity"] = tc + } + if cfg.Perforce.Enabled { + p4, err := tools.NewPerforce(cfg.Perforce) + if err != nil { + return nil, err + } + r.execs["perforce"] = p4 + } + return r, nil +} + +// Run blocks until ctx is cancelled, reconnecting forever. +func (r *Runner) Run(ctx context.Context) error { + attempt := 0 + for { + if ctx.Err() != nil { + return nil + } + connected, err := r.runOnce(ctx) + if ctx.Err() != nil { + return nil + } + if connected { + // A session that actually came up resets the backoff, so a long + // uptime is not punished by an old failure streak. + attempt = 0 + } + attempt++ + d := backoff(attempt) + reason := "connection closed" + if err != nil { + reason = err.Error() + } + r.log.Write(audit.Entry{ + Event: "reconnect_scheduled", + Reason: classify(err), + Message: fmt.Sprintf("%s; retrying in %s (attempt %d)", reason, d.Round(time.Millisecond), attempt), + }) + select { + case <-ctx.Done(): + return nil + case <-time.After(d): + } + } +} + +// classify turns an error into a stable token the drills can assert on. +func classify(err error) string { + switch { + case err == nil: + return "closed" + case errors.Is(err, wsclient.ErrUnauthorized): + return "unauthorized" + case errors.Is(err, wsclient.ErrClosedByServer): + return "closed_by_broker" + case errors.Is(err, wsclient.ErrQueryString): + return "endpoint_query_string" + default: + return "transport_error" + } +} + +func backoff(attempt int) time.Duration { + d := backoffMin << min(attempt-1, 12) + if d > backoffMax { + d = backoffMax + } + // Full jitter, so a fleet does not stampede. + return time.Duration(rand.Int63n(int64(d)) + int64(backoffMin)) +} + +func (r *Runner) runOnce(ctx context.Context) (connected bool, err error) { + conn, err := wsclient.Dial(wsclient.Options{ + Endpoint: r.cfg.Endpoint, + Token: r.cfg.Token, + CAFile: r.cfg.EndpointCAFile, + UserAgent: "butterstack-connector/" + r.version, + }) + if err != nil { + return false, err + } + defer conn.Close() + + hello := protocol.NewHello( + r.cfg.ConnectorID, + r.cfg.IntegrationID(), + r.version, + vocab.CompiledVerbs(), + r.toolVersions(ctx), + ) + if err := writeJSON(conn, hello); err != nil { + return false, err + } + + raw, err := conn.ReadMessage(time.Now().Add(30 * time.Second)) + if err != nil { + return false, err + } + kind, err := protocol.DecodeEnvelope(raw) + if err != nil { + return false, err + } + heartbeat := defaultHeartbeat + switch kind { + case protocol.TypeWelcome: + var w protocol.Welcome + if err := json.Unmarshal(raw, &w); err != nil { + return false, err + } + if w.HeartbeatInterval > 0 { + hb := time.Duration(w.HeartbeatInterval) * time.Second + if hb >= minHeartbeat && hb <= maxHeartbeat { + heartbeat = hb + } + } + r.mu.Lock() + r.sessionID = w.SessionID + r.mu.Unlock() + r.log.Write(audit.Entry{Event: "connected", SessionID: w.SessionID, + Message: fmt.Sprintf("broker accepted; heartbeat %s", heartbeat)}) + case protocol.TypeReject: + var rj protocol.Reject + _ = json.Unmarshal(raw, &rj) + r.log.Write(audit.Entry{Event: "rejected", Reason: rj.Reason}) + return false, fmt.Errorf("broker rejected the session: %s", rj.Reason) + default: + return false, fmt.Errorf("expected welcome or reject, got %q", kind) + } + + return true, r.pump(ctx, conn, heartbeat) +} + +// pump runs the heartbeat ticker and the read loop until either fails. +func (r *Runner) pump(ctx context.Context, conn *wsclient.Conn, heartbeat time.Duration) error { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + var wg sync.WaitGroup + errCh := make(chan error, 1) + sem := make(chan struct{}, r.cfg.MaxConcurrent) + + // fail reports the first error without ever blocking: a goroutine that + // cannot report must still be able to return, or the shutdown path hangs. + fail := func(err error) { + select { + case errCh <- err: + default: + } + } + + wg.Add(1) + go func() { + defer wg.Done() + t := time.NewTicker(heartbeat) + defer t.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.C: + hb := protocol.Heartbeat{Type: protocol.TypeHeartbeat, + TS: time.Now().UTC().Format(time.RFC3339), QueueDepth: len(sem)} + if err := writeJSON(conn, hb); err != nil { + fail(err) + return + } + } + } + }() + + wg.Add(1) + go func() { + defer wg.Done() + for { + // If the broker goes quiet for longer than a couple of heartbeat + // intervals the socket is dead even if TCP has not noticed. + raw, err := conn.ReadMessage(time.Now().Add(heartbeat * readSlack)) + if err != nil { + fail(err) + return + } + kind, err := protocol.DecodeEnvelope(raw) + if err != nil { + r.log.Write(audit.Entry{Event: "frame_dropped", Reason: "malformed"}) + continue + } + if kind != protocol.TypeCommand { + // An older connector must tolerate a newer broker's frames. + r.log.Write(audit.Entry{Event: "frame_ignored", Message: kind}) + continue + } + var cmd protocol.Command + if err := json.Unmarshal(raw, &cmd); err != nil { + r.log.Write(audit.Entry{Event: "frame_dropped", Reason: "malformed_command"}) + continue + } + select { + case sem <- struct{}{}: + case <-ctx.Done(): + return + } + wg.Add(1) + go func() { + defer wg.Done() + defer func() { <-sem }() + res := r.handle(ctx, cmd) + if err := writeJSON(conn, res); err != nil { + fail(err) + } + }() + } + }() + + select { + case err := <-errCh: + cancel() + conn.Close() + wg.Wait() + return err + case <-ctx.Done(): + conn.Close() + wg.Wait() + return nil + } +} + +// handle is the command path: dedupe, resolve against the vocabulary, execute, +// and write exactly one audit line whatever the outcome. +func (r *Runner) handle(ctx context.Context, cmd protocol.Command) protocol.Result { + start := time.Now() + digest := audit.ArgsDigest(cmd.Args) + + r.mu.Lock() + sessionID := r.sessionID + r.mu.Unlock() + + finish := func(res protocol.Result, detail string) protocol.Result { + r.log.Write(audit.Entry{ + Event: "command", + SessionID: sessionID, + CommandID: cmd.ID, + Verb: cmd.Verb, + ArgsSHA256: digest, + Status: res.Status, + Reason: res.Reason, + Detail: detail, + BytesOut: res.Bytes, + Truncated: res.Truncated, + DurationMS: time.Since(start).Milliseconds(), + }) + return res + } + + if cmd.ID == "" { + return finish(protocol.Deny("", vocab.ReasonMalformedArgs), "command has no id") + } + if !r.claim(cmd.ID) { + return finish(protocol.Deny(cmd.ID, vocab.ReasonDuplicateCommandID), "") + } + + verb, args, derr := vocab.Resolve(cmd.Verb, cmd.Args, vocab.Scopes{ + DepotScope: r.cfg.Scopes.DepotScope, + AllowedBuildTypes: r.cfg.Scopes.AllowedBuildTypes, + RepoAllowlist: r.cfg.Scopes.RepoAllowlist, + }, r.cfg.Tools()) + if derr != nil { + return finish(protocol.Deny(cmd.ID, derr.Reason), derr.Detail) + } + + deadline := time.Duration(cmd.DeadlineMs) * time.Millisecond + if deadline <= 0 { + deadline = defaultDeadline + } + if deadline > maxDeadline { + deadline = maxDeadline + } + cctx, cancel := context.WithTimeout(ctx, deadline) + defer cancel() + + maxBytes := cmd.MaxBytes + if maxBytes <= 0 || maxBytes > verb.DefaultMaxBytes { + maxBytes = verb.DefaultMaxBytes + } + + body, n, truncated, err := r.execute(cctx, verb, args, maxBytes) + switch { + case errors.Is(cctx.Err(), context.DeadlineExceeded): + res := protocol.Result{Type: protocol.TypeResult, ID: cmd.ID, + Status: protocol.StatusTimeout, Reason: vocab.ReasonDeadlineExceeded} + return finish(res, "") + case err != nil: + return finish(protocol.Errorf(cmd.ID, "%s", err.Error()), "") + } + return finish(protocol.OK(cmd.ID, body, n, truncated), "") +} + +func (r *Runner) execute(ctx context.Context, verb *vocab.Verb, args map[string]any, maxBytes int) (any, int, bool, error) { + switch verb.Name { + case "sys.ping": + return map[string]any{"pong": true, "ts": time.Now().UTC().Format(time.RFC3339Nano)}, 0, false, nil + case "sys.version": + return map[string]any{"version": r.version, "protocol": protocol.Version}, 0, false, nil + case "sys.capabilities": + return map[string]any{ + "verbs": vocab.CompiledVerbs(), + "tools": r.cfg.Tools(), + }, 0, false, nil + } + ex, ok := r.execs[verb.Tool] + if !ok { + return nil, 0, false, tools.ErrNotConfigured + } + return ex.Execute(ctx, verb.Name, args, maxBytes) +} + +// claim implements the sliding-window duplicate check. A replayed command id is +// denied rather than re-executed, which matters most for the mutating verbs +// this version does not yet compile in. +func (r *Runner) claim(id string) bool { + now := time.Now() + r.mu.Lock() + defer r.mu.Unlock() + if len(r.seen) > dedupeMaxKeys { + for k, t := range r.seen { + if now.Sub(t) > dedupeWindow { + delete(r.seen, k) + } + } + } + if t, ok := r.seen[id]; ok && now.Sub(t) <= dedupeWindow { + return false + } + r.seen[id] = now + return true +} + +func (r *Runner) toolVersions(ctx context.Context) map[string]string { + out := map[string]string{} + if tc, ok := r.execs["teamcity"].(interface{ Version(context.Context) string }); ok { + cctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if v := tc.Version(cctx); v != "" { + out["teamcity"] = v + } + } + return out +} + +func writeJSON(conn *wsclient.Conn, v any) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + return conn.WriteText(b) +} diff --git a/internal/tools/perforce.go b/internal/tools/perforce.go new file mode 100644 index 0000000..b458834 --- /dev/null +++ b/internal/tools/perforce.go @@ -0,0 +1,211 @@ +package tools + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "os" + "os/exec" + "strconv" + "strings" + + "github.com/ButterStack/butterstack-connector/internal/config" +) + +// Perforce shells out to the p4 CLI. +// +// Every invocation is an argv array passed straight to execve: there is no +// shell in the path, so a depot path is data to p4 and can never be a command. +// (Shuri F5's smaller sibling: "if the connector shells out to the p4 CLI +// rather than using P4Ruby, every invocation must be an argv array with no +// shell interpretation.") The port, user, and ticket come from connector.yml +// and are passed as flags; the ticket is handed over through the environment's +// P4PASSWD rather than argv so it does not appear in the host's process list. +type Perforce struct { + cfg config.Perforce +} + +// NewPerforce builds an executor from the local config section. +func NewPerforce(c config.Perforce) (*Perforce, error) { + if !c.Enabled { + return nil, ErrNotConfigured + } + return &Perforce{cfg: c}, nil +} + +// DescribedFile is one file in a changelist. +type DescribedFile struct { + DepotFile string `json:"depot_file"` + Action string `json:"action"` + Type string `json:"type"` + Rev string `json:"rev"` +} + +// Describe is the projected result of p4.describe. No diff, ever, in v0: the +// verb's include_diff argument is schema-pinned to false. +type Describe struct { + Change int64 `json:"change"` + User string `json:"user"` + Client string `json:"client"` + Time string `json:"time"` + Description string `json:"description"` + Status string `json:"status"` + Files []DescribedFile `json:"files"` + FileCount int `json:"file_count"` +} + +// ChangeSummary is one entry of p4.changes. +type ChangeSummary struct { + Change int64 `json:"change"` + User string `json:"user"` + Time string `json:"time"` + Description string `json:"description"` +} + +// Execute dispatches the Perforce verbs. +func (p *Perforce) Execute(ctx context.Context, verb string, args map[string]any, maxBytes int) (any, int, bool, error) { + switch verb { + case "p4.describe": + change := argInt(args, "change", 0) + maxFiles := int(argInt(args, "max_files", 200)) + return p.describe(ctx, change, maxFiles, maxBytes) + + case "p4.changes": + path := argString(args, "path") + max := int(argInt(args, "max", 25)) + return p.changes(ctx, path, max, maxBytes) + } + return nil, 0, false, fmt.Errorf("perforce: no executor for %s", verb) +} + +func (p *Perforce) describe(ctx context.Context, change int64, maxFiles, maxBytes int) (any, int, bool, error) { + // -s omits the diffs entirely; this is the content boundary enforced at the + // tool invocation, not only in the schema. + recs, n, err := p.run(ctx, maxBytes, "describe", "-s", strconv.FormatInt(change, 10)) + if err != nil { + return nil, n, false, err + } + if len(recs) == 0 { + return nil, n, false, fmt.Errorf("perforce: changelist %d not found", change) + } + r := recs[0] + out := Describe{ + Change: change, + User: str(r, "user"), + Client: str(r, "client"), + Time: str(r, "time"), + Description: str(r, "desc"), + Status: str(r, "status"), + } + // p4 -Mj returns indexed keys: depotFile0, action0, type0, rev0, ... + truncated := false + for i := 0; ; i++ { + df := str(r, "depotFile"+strconv.Itoa(i)) + if df == "" { + break + } + if len(out.Files) >= maxFiles { + truncated = true + break + } + out.Files = append(out.Files, DescribedFile{ + DepotFile: df, + Action: str(r, "action"+strconv.Itoa(i)), + Type: str(r, "type"+strconv.Itoa(i)), + Rev: str(r, "rev"+strconv.Itoa(i)), + }) + } + out.FileCount = len(out.Files) + return out, n, truncated, nil +} + +func (p *Perforce) changes(ctx context.Context, path string, max, maxBytes int) (any, int, bool, error) { + recs, n, err := p.run(ctx, maxBytes, "changes", "-m", strconv.Itoa(max), path) + if err != nil { + return nil, n, false, err + } + out := make([]ChangeSummary, 0, len(recs)) + for _, r := range recs { + c, _ := strconv.ParseInt(str(r, "change"), 10, 64) + out = append(out, ChangeSummary{ + Change: c, + User: str(r, "user"), + Time: str(r, "time"), + Description: str(r, "desc"), + }) + } + return out, n, false, nil +} + +// run invokes p4 with -Mj (one JSON object per record) and returns the parsed +// records. args is appended to a fixed prefix; nothing in args is interpreted. +func (p *Perforce) run(ctx context.Context, maxBytes int, args ...string) ([]map[string]any, int, error) { + argv := append([]string{ + "-p", p.cfg.Port, + "-u", p.cfg.User, + "-Mj", "-ztag", + }, args...) + + ctx, cancel := context.WithTimeout(ctx, p.cfg.Timeout.D()) + defer cancel() + + cmd := exec.CommandContext(ctx, p.cfg.Binary, argv...) // argv array; no shell + cmd.Env = p.env() + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + + if err := cmd.Run(); err != nil { + msg := strings.TrimSpace(stderr.String()) + if msg == "" { + msg = err.Error() + } + return nil, stdout.Len(), fmt.Errorf("perforce: %s", firstLine(msg)) + } + if maxBytes > 0 && stdout.Len() > maxBytes { + return nil, stdout.Len(), fmt.Errorf("perforce: response exceeded max_bytes") + } + + var recs []map[string]any + dec := json.NewDecoder(&stdout) + for { + var m map[string]any + if err := dec.Decode(&m); err != nil { + break + } + recs = append(recs, m) + } + return recs, stdout.Len(), nil +} + +// env builds a minimal environment. The ticket goes in P4PASSWD rather than on +// the command line so it never shows up in `ps` on the studio's host. +func (p *Perforce) env() []string { + env := []string{ + "PATH=" + os.Getenv("PATH"), + "HOME=" + os.Getenv("HOME"), + "P4PORT=" + p.cfg.Port, + "P4USER=" + p.cfg.User, + } + if p.cfg.Ticket != "" { + env = append(env, "P4PASSWD="+p.cfg.Ticket) + } + return env +} + +func str(m map[string]any, k string) string { + if v, ok := m[k]; ok { + if s, ok := v.(string); ok { + return s + } + return fmt.Sprint(v) + } + return "" +} + +func firstLine(s string) string { + if i := strings.IndexByte(s, '\n'); i >= 0 { + return s[:i] + } + return s +} diff --git a/internal/tools/teamcity.go b/internal/tools/teamcity.go new file mode 100644 index 0000000..10c0fef --- /dev/null +++ b/internal/tools/teamcity.go @@ -0,0 +1,191 @@ +package tools + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "os" + "strings" + + "github.com/ButterStack/butterstack-connector/internal/config" +) + +// TeamCity talks REST to the studio's on-prem TeamCity server. The base URL and +// the Bearer token come from connector.yml and are never on the wire, which is +// the whole of survival condition 3 for this tool: revoking our connector token +// closes the socket and leaves the studio's TeamCity token untouched. +type TeamCity struct { + base *url.URL + token string + client *http.Client +} + +// serverInfoFields and buildFields are fixed projections. They are the client +// half of the egress spec: the connector asks TeamCity only for the fields the +// verb is documented to return, so a future TeamCity that adds fields does not +// silently widen what leaves the network. +const ( + serverInfoFields = "version,versionMajor,versionMinor,buildNumber,webUrl" + buildFields = "id,buildTypeId,number,status,state,statusText,branchName,webUrl," + + "queuedDate,startDate,finishDate," + + "revisions(revision(version,vcsBranchName,vcs-root-instance(id,vcs-root-id,vcsName)))" +) + +// NewTeamCity builds an executor from the local config section. +func NewTeamCity(c config.TeamCity) (*TeamCity, error) { + if !c.Enabled { + return nil, ErrNotConfigured + } + u, err := url.Parse(strings.TrimRight(c.URL, "/")) + if err != nil { + return nil, fmt.Errorf("teamcity: url: %w", err) + } + tlsCfg := &tls.Config{MinVersion: tls.VersionTLS12} + if c.CAFile != "" { + pem, err := os.ReadFile(c.CAFile) + if err != nil { + return nil, fmt.Errorf("teamcity: ca_file: %w", err) + } + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(pem) { + return nil, fmt.Errorf("teamcity: ca_file contains no certificates") + } + tlsCfg.RootCAs = pool + } + // allow_insecure_tls applies to this LAN server only. There is deliberately + // no equivalent for the broker connection. + tlsCfg.InsecureSkipVerify = c.AllowInsecureTLS //nolint:gosec // documented, LAN-only, opt-in + return &TeamCity{ + base: u, + token: c.Token, + client: &http.Client{ + Timeout: c.Timeout.D(), + Transport: &http.Transport{TLSClientConfig: tlsCfg}, + CheckRedirect: func(*http.Request, []*http.Request) error { + // A redirect could send the Bearer token to another host. + return http.ErrUseLastResponse + }, + }, + }, nil +} + +// ServerInfo is the projected result of teamcity.server.info. +type ServerInfo struct { + Version string `json:"version"` + VersionMajor int `json:"versionMajor"` + VersionMinor int `json:"versionMinor"` + BuildNumber string `json:"buildNumber"` + WebURL string `json:"webUrl"` +} + +// Build is the projected result of teamcity.build.get. +type Build struct { + ID int64 `json:"id"` + BuildTypeID string `json:"buildTypeId"` + Number string `json:"number"` + Status string `json:"status"` + State string `json:"state"` + StatusText string `json:"statusText"` + BranchName string `json:"branchName"` + WebURL string `json:"webUrl"` + QueuedDate string `json:"queuedDate"` + StartDate string `json:"startDate"` + FinishDate string `json:"finishDate"` + Revisions struct { + Revision []struct { + Version string `json:"version"` + VCSBranchName string `json:"vcsBranchName"` + } `json:"revision"` + } `json:"revisions"` +} + +// Execute dispatches the TeamCity verbs. +func (t *TeamCity) Execute(ctx context.Context, verb string, args map[string]any, maxBytes int) (any, int, bool, error) { + switch verb { + case "teamcity.server.info": + var out ServerInfo + n, trunc, err := t.get(ctx, "/app/rest/server", serverInfoFields, maxBytes, &out) + return out, n, trunc, err + + case "teamcity.build.get": + id := argInt(args, "build_id", 0) + var out Build + path := fmt.Sprintf("/app/rest/builds/id:%d", id) + n, trunc, err := t.get(ctx, path, buildFields, maxBytes, &out) + return out, n, trunc, err + } + return nil, 0, false, fmt.Errorf("teamcity: no executor for %s", verb) +} + +func (t *TeamCity) get(ctx context.Context, path, fields string, maxBytes int, into any) (int, bool, error) { + u := *t.base + u.Path = t.base.Path + path + q := url.Values{} + if fields != "" { + q.Set("fields", fields) + } + u.RawQuery = q.Encode() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil) + if err != nil { + return 0, false, err + } + // The studio's TeamCity token rides this header to a host on the studio's + // own LAN. It is never echoed into a result frame. + req.Header.Set("Authorization", "Bearer "+t.token) + req.Header.Set("Accept", "application/json") + + resp, err := t.client.Do(req) + if err != nil { + return 0, false, fmt.Errorf("teamcity: %w", redactURL(err, t.token)) + } + defer resp.Body.Close() + + if maxBytes <= 0 { + maxBytes = 64 << 10 + } + body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxBytes)+1)) + if err != nil { + return 0, false, fmt.Errorf("teamcity: read: %w", err) + } + truncated := len(body) > maxBytes + if truncated { + body = body[:maxBytes] + } + if resp.StatusCode != http.StatusOK { + return len(body), truncated, fmt.Errorf("teamcity: HTTP %d", resp.StatusCode) + } + if err := json.Unmarshal(body, into); err != nil { + if truncated { + return len(body), true, fmt.Errorf("teamcity: response exceeded max_bytes") + } + return len(body), truncated, fmt.Errorf("teamcity: malformed response") + } + return len(body), truncated, nil +} + +// Version reports the tool version string announced in hello, or empty when the +// server cannot be reached at startup. Failing to reach TeamCity is not a +// startup error: the connector still connects and answers sys verbs. +func (t *TeamCity) Version(ctx context.Context) string { + var info ServerInfo + if _, _, err := t.get(ctx, "/app/rest/server", serverInfoFields, 8192, &info); err != nil { + return "" + } + return info.Version +} + +// redactURL keeps a token out of a wrapped transport error, which would +// otherwise reach the audit log through the error string. +func redactURL(err error, secret string) error { + if secret == "" { + return err + } + s := strings.ReplaceAll(err.Error(), secret, "[redacted]") + return fmt.Errorf("%s", s) +} diff --git a/internal/tools/tools.go b/internal/tools/tools.go new file mode 100644 index 0000000..84f62a5 --- /dev/null +++ b/internal/tools/tools.go @@ -0,0 +1,42 @@ +// Package tools holds the per-tool executors. +// +// Every executor takes an already-validated argument map: by the time control +// reaches this package, the verb was in the compiled vocabulary, every argument +// was type-checked, and every scoped argument was inside its scope. Executors +// must not re-derive anything from the wire, and must not accept a host, port, +// URL, or credential from their caller -- all of those come from the config +// value the executor was constructed with. +package tools + +import ( + "context" + "errors" +) + +// ErrNotConfigured is returned when a verb's tool section is disabled. +var ErrNotConfigured = errors.New("tools: tool is not configured") + +// Executor runs one verb. +type Executor interface { + Execute(ctx context.Context, verb string, args map[string]any, maxBytes int) (body any, bytes int, truncated bool, err error) +} + +// argInt reads a validated integer argument, with a default. +func argInt(args map[string]any, name string, def int64) int64 { + if v, ok := args[name]; ok { + if i, ok := v.(int64); ok { + return i + } + } + return def +} + +// argString reads a validated string argument. +func argString(args map[string]any, name string) string { + if v, ok := args[name]; ok { + if s, ok := v.(string); ok { + return s + } + } + return "" +} diff --git a/internal/vocab/vocab.go b/internal/vocab/vocab.go new file mode 100644 index 0000000..07d408c --- /dev/null +++ b/internal/vocab/vocab.go @@ -0,0 +1,608 @@ +// Package vocab is the typed command allowlist and the argument-constraint +// layer. It is the whole of survival condition 2 ("typed allowlist with +// constrained arguments, never a tunnel or shell") in one readable file, which +// is the point: an IT director is asked to read the source, so the source has +// to be readable. +// +// Two layers, both of which must pass before any tool is touched: +// +// 1. The verb must be in the compiled vocabulary. A verb that is not compiled +// in -- including a name this schema reserves for a later version -- is +// denied. There is no dynamic registration and no sys.exec. +// +// 2. Every argument is validated against a fixed per-verb schema before the +// tool call: unknown keys are refused outright (this is what stops a +// smuggled params map, Shuri F4), scalars are type- and range-checked, and +// path/id arguments are matched against scopes that live only in the +// studio's connector.yml (Shuri F5). +// +// A well-formed verb with an out-of-scope argument is denied exactly like an +// unknown verb, and both write a local audit line. Only the second kind of +// denial actually tests condition 2, which is why the drills assert both. +package vocab + +import ( + "encoding/json" + "errors" + "fmt" + "regexp" + "sort" + "strings" +) + +// Class is the egress class of a verb's output, per Devin design note 2.4. +// M = metadata, P = paths, C = content, X = mutation. +type Class string + +const ( + ClassMetadata Class = "M" + ClassPaths Class = "P" + ClassContent Class = "C" + ClassMutation Class = "X" +) + +// Kind is the type of a single argument. Every kind is a scalar. There is +// deliberately no map, object, or free-form kind: a verb that could carry an +// arbitrary key/value bag would be a code-execution primitive on the studio's +// build agents the moment those keys reach a build step, which is the finding +// that made this layer day-1 work rather than v1 polish. +type Kind string + +const ( + KindInt Kind = "int" + KindBool Kind = "bool" + KindString Kind = "string" + KindDepotPath Kind = "depot_path" +) + +// Scope names a constraint list that lives in connector.yml, never on the wire. +type Scope string + +const ( + ScopeNone Scope = "" + ScopeDepot Scope = "depot_scope" + ScopeAllowedBuildTys Scope = "allowed_build_types" + ScopeRepoAllowlist Scope = "repo_allowlist" +) + +// Arg is one argument's schema. +type Arg struct { + Name string + Kind Kind + Required bool + + // Int constraints. + Min int64 + Max int64 + + // String constraints. + Pattern *regexp.Regexp + MaxLen int + + // Bool constraints. MustBeFalse encodes a v0 toggle that is off at the + // schema level, not just in config: include_diff cannot be turned on by a + // caller no matter what the broker sends. + MustBeFalse bool + + // Scope, if any, that the value must fall inside. + Scope Scope + + Doc string +} + +// Verb is one entry in the vocabulary. +type Verb struct { + Name string + Class Class + + // Compiled reports whether this build can actually execute the verb. + // Reserved names are listed with Compiled=false so that the vocabulary is + // self-documenting and so that a reserved name's (empty) argument schema is + // covered by the same tests as a live one -- in particular the test that no + // verb anywhere accepts caller-supplied build parameters. + Compiled bool + + // Mutating marks X-class verbs. No X-class verb is compiled in v0. + Mutating bool + + Args []Arg + + // DefaultMaxBytes caps the result body when the broker does not say. + DefaultMaxBytes int + + // Tool names the connector.yml section this verb needs configured. + Tool string + + Doc string +} + +// Scopes are the studio-supplied constraint lists. Empty means "nothing is in +// scope" for the depot list -- fail closed -- while an empty build-type or repo +// list means the same. A studio that configures no depot_scope cannot run a +// path verb, which is the correct default for a daemon inside someone's LAN. +type Scopes struct { + DepotScope []string + AllowedBuildTypes []string + RepoAllowlist []string +} + +// DenyReason values are stable machine tokens. The drills assert on them and +// the audit log stores them, so they are part of the protocol surface. +const ( + ReasonUnknownVerb = "unknown_verb" + ReasonVerbNotCompiled = "verb_not_compiled" + ReasonToolNotConfigured = "tool_not_configured" + ReasonMalformedArgs = "malformed_args" + ReasonUnknownArgument = "unknown_argument" + ReasonMissingArgument = "missing_argument" + ReasonArgumentType = "argument_type" + ReasonArgumentRange = "argument_range" + ReasonArgumentPattern = "argument_pattern" + ReasonContentVerbOff = "content_verb_disabled" + ReasonOutOfScopePath = "out_of_scope_path" + ReasonOutOfScopeBuildTy = "out_of_scope_build_type" + ReasonOutOfScopeRepo = "out_of_scope_repo" + ReasonDuplicateCommandID = "duplicate_command_id" + ReasonDeadlineExceeded = "deadline_exceeded" +) + +// DenyError carries a stable reason plus a human detail. The detail is written +// to the local audit log only; it never leaves the studio network, because a +// denial message that echoed the rejected value back would be a small egress +// channel of its own. +type DenyError struct { + Reason string + Detail string +} + +func (e *DenyError) Error() string { + if e.Detail == "" { + return e.Reason + } + return e.Reason + ": " + e.Detail +} + +func deny(reason, format string, a ...any) *DenyError { + return &DenyError{Reason: reason, Detail: fmt.Sprintf(format, a...)} +} + +// bannedArgNames are argument names that must never appear in any verb's +// schema, compiled or reserved. Each one is a documented path from "the broker +// asked for a build" to "the broker ran a command on a studio build agent": +// Jenkins parameters and TeamCity properties are interpolated into build steps +// by design. The vocabulary test enforces this list, so re-adding one of these +// names fails the build rather than shipping. +var bannedArgNames = map[string]string{ + "params": "Jenkins build parameters interpolate into shell build steps", + "parameters": "same as params", + "properties": "TeamCity properties are consumed by build steps", + "env": "environment injection reaches build steps", + "branchname": "branch names reach VCS checkout logic and build steps", + "branch_name": "branch names reach VCS checkout logic and build steps", + "comment": "free text on a queue request, no v0 need", + "url": "a verb that takes a URL is a tunnel", + "host": "a verb that takes a host is a tunnel", + "port": "a verb that takes a port is a tunnel", + "command": "a verb that takes a command is a shell", + "script": "a verb that takes a script is a shell", + "args": "a verb that takes an argv is a shell", +} + +var ( + reBuildTypeID = regexp.MustCompile(`\A[A-Za-z0-9_]{1,190}\z`) + reHexSHA = regexp.MustCompile(`\A[0-9a-f]{7,64}\z`) +) + +// Vocabulary is the whole allowlist for protocol v0. Compiled verbs first, +// then reserved names. Adding a verb here is the only way to add one: there is +// no plugin path and nothing reads a verb name from config. +var Vocabulary = []Verb{ + // ---- connector-internal ------------------------------------------------- + { + Name: "sys.ping", Class: ClassMetadata, Compiled: true, DefaultMaxBytes: 1024, + Doc: "liveness round trip; touches no studio tool", + }, + { + Name: "sys.version", Class: ClassMetadata, Compiled: true, DefaultMaxBytes: 1024, + Doc: "connector build version and protocol version", + }, + { + Name: "sys.capabilities", Class: ClassMetadata, Compiled: true, DefaultMaxBytes: 8192, + Doc: "the compiled verb list and which tools are configured", + }, + + // ---- TeamCity ----------------------------------------------------------- + { + Name: "teamcity.server.info", Class: ClassMetadata, Compiled: true, Tool: "teamcity", + DefaultMaxBytes: 8192, + Doc: "GET /app/rest/server; the test_connection analog", + }, + { + Name: "teamcity.build.get", Class: ClassMetadata, Compiled: true, Tool: "teamcity", + DefaultMaxBytes: 32768, + Args: []Arg{ + {Name: "build_id", Kind: KindInt, Required: true, Min: 1, Max: 1 << 40, + Doc: "TeamCity build id, integer-validated at the frame boundary"}, + }, + Doc: "GET /app/rest/builds/id: with a fixed fields= projection", + }, + { + Name: "teamcity.build.queue", Class: ClassMutation, Mutating: true, Compiled: false, + Tool: "teamcity", + Args: []Arg{ + {Name: "build_type_id", Kind: KindString, Required: true, Pattern: reBuildTypeID, + MaxLen: 190, Scope: ScopeAllowedBuildTys, + Doc: "must appear in allowed_build_types in connector.yml"}, + }, + Doc: "RESERVED, not compiled in v0. When it ships, the connector composes a " + + "fixed request body {\"buildType\":{\"id\":...}}; there is no argument here " + + "for properties, branchName, or comment, and there never will be at v0.", + }, + + // ---- Perforce ----------------------------------------------------------- + { + Name: "p4.describe", Class: ClassPaths, Compiled: true, Tool: "perforce", + DefaultMaxBytes: 65536, + Args: []Arg{ + {Name: "change", Kind: KindInt, Required: true, Min: 1, Max: 1 << 40, + Doc: "changelist number, integer-validated before the p4 call"}, + {Name: "max_files", Kind: KindInt, Min: 1, Max: 1000, + Doc: "cap on the returned file list"}, + {Name: "include_diff", Kind: KindBool, MustBeFalse: true, + Doc: "content class; not available in v0 at any config setting"}, + }, + Doc: "p4 describe -s , invoked as an argv array with no shell", + }, + { + Name: "p4.changes", Class: ClassPaths, Compiled: true, Tool: "perforce", + DefaultMaxBytes: 65536, + Args: []Arg{ + {Name: "path", Kind: KindDepotPath, Required: true, Scope: ScopeDepot, MaxLen: 1024, + Doc: "depot path; prefix-matched against depot_scope, no wildcard above the prefix"}, + {Name: "max", Kind: KindInt, Min: 1, Max: 200, + Doc: "cap on the number of changelists returned"}, + }, + Doc: "p4 changes -m ; the path-scoped verb the out-of-scope drill exercises", + }, + { + Name: "p4.file_contents", Class: ClassContent, Compiled: false, Tool: "perforce", + Args: []Arg{ + {Name: "depot_path", Kind: KindDepotPath, Required: true, Scope: ScopeDepot, MaxLen: 1024}, + {Name: "rev", Kind: KindInt, Min: 1, Max: 1 << 32}, + {Name: "max_bytes", Kind: KindInt, Min: 1, Max: 1 << 20}, + }, + Doc: "RESERVED, not compiled in v0. Content class; the spike runs with every " + + "content verb off, so this name exists to be denied.", + }, + + // ---- Jenkins ------------------------------------------------------------ + { + Name: "jenkins.build.trigger", Class: ClassMutation, Mutating: true, Compiled: false, + Tool: "jenkins", + Args: []Arg{ + {Name: "job", Kind: KindString, Required: true, MaxLen: 190, + Pattern: regexp.MustCompile(`\A[A-Za-z0-9._\-/]{1,190}\z`), + Doc: "must appear in allowed_jobs in connector.yml"}, + }, + Doc: "RESERVED, not compiled in v0. When it ships it triggers with the job's own " + + "default parameters. There is no params argument: Jenkins parameters " + + "interpolate into shell build steps, which would make the allowlist a " + + "code-execution primitive and falsify the bounded-blast-radius claim.", + }, + + // ---- GitHub Enterprise Server ------------------------------------------- + { + Name: "ghes.commit.get", Class: ClassPaths, Compiled: false, Tool: "ghes", + Args: []Arg{ + {Name: "repo", Kind: KindString, Required: true, Scope: ScopeRepoAllowlist, MaxLen: 190, + Pattern: regexp.MustCompile(`\A[A-Za-z0-9._\-]{1,100}/[A-Za-z0-9._\-]{1,100}\z`)}, + {Name: "sha", Kind: KindString, Required: true, Pattern: reHexSHA, MaxLen: 64}, + }, + Doc: "RESERVED, not compiled in v0. Listed so repo_allowlist has a schema to bind to.", + }, + + // ---- Horde -------------------------------------------------------------- + { + Name: "horde.server.info", Class: ClassMetadata, Compiled: false, Tool: "horde", + Doc: "RESERVED, not compiled in v0. Horde is connector-only and has no push path.", + }, +} + +// index is the compiled lookup table, built once. +var index = func() map[string]*Verb { + m := make(map[string]*Verb, len(Vocabulary)) + for i := range Vocabulary { + if _, dup := m[Vocabulary[i].Name]; dup { + panic("vocab: duplicate verb " + Vocabulary[i].Name) + } + m[Vocabulary[i].Name] = &Vocabulary[i] + } + return m +}() + +// Lookup returns the verb schema, or nil when the name is not in the +// vocabulary at all. +func Lookup(name string) *Verb { return index[name] } + +// CompiledVerbs is the capability list announced in hello, sorted for stable +// comparison in tests and audit output. +func CompiledVerbs() []string { + out := make([]string, 0, len(Vocabulary)) + for i := range Vocabulary { + if Vocabulary[i].Compiled { + out = append(out, Vocabulary[i].Name) + } + } + sort.Strings(out) + return out +} + +// ConfiguredTools is the set of connector.yml sections that are enabled. +type ConfiguredTools map[string]bool + +// Resolve applies both layers and returns the validated argument map. +// +// The order matters and is asserted by the tests: vocabulary membership, then +// compiled-in, then tool configuration, then argument validation. A name that +// is not in the vocabulary must never reveal whether a tool is configured. +func Resolve(name string, rawArgs json.RawMessage, scopes Scopes, tools ConfiguredTools) (*Verb, map[string]any, *DenyError) { + v := Lookup(name) + if v == nil { + return nil, nil, deny(ReasonUnknownVerb, "%q is not in the vocabulary", name) + } + if !v.Compiled { + return v, nil, deny(ReasonVerbNotCompiled, "%q is reserved but not compiled into this build", name) + } + if v.Class == ClassContent { + // Belt and braces: no content-class verb is compiled in v0, and if one + // ever is, it still cannot run without an explicit local toggle that + // this build does not read. Fail closed. + return v, nil, deny(ReasonContentVerbOff, "%q is a content-class verb", name) + } + if v.Tool != "" && !tools[v.Tool] { + return v, nil, deny(ReasonToolNotConfigured, "no %s section in connector.yml", v.Tool) + } + args, derr := v.validateArgs(rawArgs, scopes) + if derr != nil { + return v, nil, derr + } + return v, args, nil +} + +// validateArgs enforces the per-verb argument schema. +func (v *Verb) validateArgs(raw json.RawMessage, scopes Scopes) (map[string]any, *DenyError) { + supplied := map[string]json.RawMessage{} + if len(raw) > 0 && string(raw) != "null" { + dec := json.NewDecoder(strings.NewReader(string(raw))) + dec.UseNumber() + if err := dec.Decode(&supplied); err != nil { + return nil, deny(ReasonMalformedArgs, "args is not a JSON object: %v", err) + } + } + + // Unknown keys are refused before anything else is looked at. This single + // rule is what stops a smuggled parameter bag: an argument the schema does + // not name cannot be ignored into the tool call, it stops the command. + known := make(map[string]*Arg, len(v.Args)) + for i := range v.Args { + known[v.Args[i].Name] = &v.Args[i] + } + for k := range supplied { + if _, ok := known[k]; !ok { + return nil, deny(ReasonUnknownArgument, "%s does not accept %q", v.Name, k) + } + } + + out := make(map[string]any, len(v.Args)) + for i := range v.Args { + a := &v.Args[i] + rawVal, present := supplied[a.Name] + if !present { + if a.Required { + return nil, deny(ReasonMissingArgument, "%s requires %q", v.Name, a.Name) + } + continue + } + val, derr := a.validate(rawVal, scopes) + if derr != nil { + return nil, derr + } + out[a.Name] = val + } + return out, nil +} + +func (a *Arg) validate(raw json.RawMessage, scopes Scopes) (any, *DenyError) { + switch a.Kind { + case KindInt: + // encoding/json will happily decode the JSON *string* "42" into a + // json.Number, because json.Number is a string type and "42" is a valid + // number literal. That would let a caller supply a quoted value where an + // integer is required, so the JSON type is checked first and only then + // the range: an argument's declared type is part of the allowlist. + var any0 any + dec := json.NewDecoder(strings.NewReader(string(raw))) + dec.UseNumber() + if err := dec.Decode(&any0); err != nil { + return nil, deny(ReasonArgumentType, "%s must be an integer", a.Name) + } + n, ok := any0.(json.Number) + if !ok { + return nil, deny(ReasonArgumentType, "%s must be an integer", a.Name) + } + i, err := n.Int64() + if err != nil { + return nil, deny(ReasonArgumentType, "%s must be an integer, not %s", a.Name, n.String()) + } + if i < a.Min || i > a.Max { + return nil, deny(ReasonArgumentRange, "%s must be between %d and %d", a.Name, a.Min, a.Max) + } + return i, nil + + case KindBool: + var b bool + if err := json.Unmarshal(raw, &b); err != nil { + return nil, deny(ReasonArgumentType, "%s must be a boolean", a.Name) + } + if a.MustBeFalse && b { + return nil, deny(ReasonContentVerbOff, "%s cannot be enabled in protocol v0", a.Name) + } + return b, nil + + case KindString, KindDepotPath: + var s string + if err := json.Unmarshal(raw, &s); err != nil { + return nil, deny(ReasonArgumentType, "%s must be a string", a.Name) + } + if a.MaxLen > 0 && len(s) > a.MaxLen { + return nil, deny(ReasonArgumentRange, "%s exceeds %d bytes", a.Name, a.MaxLen) + } + if strings.ContainsAny(s, "\x00\n\r") { + return nil, deny(ReasonArgumentPattern, "%s contains a control character", a.Name) + } + if a.Kind == KindDepotPath { + if derr := validateDepotPath(a.Name, s); derr != nil { + return nil, derr + } + } + if a.Pattern != nil && !a.Pattern.MatchString(s) { + return nil, deny(ReasonArgumentPattern, "%s does not match the required form", a.Name) + } + if derr := a.checkScope(s, scopes); derr != nil { + return nil, derr + } + return s, nil + } + return nil, deny(ReasonArgumentType, "%s has no validator", a.Name) +} + +// validateDepotPath applies Perforce-specific syntax rules before scope is even +// considered. Revision specifiers are rejected because they change what the +// path means, and traversal segments because a scope prefix that can be escaped +// is not a scope. +func validateDepotPath(name, s string) *DenyError { + if !strings.HasPrefix(s, "//") { + return deny(ReasonArgumentPattern, "%s must be a depot path beginning //", name) + } + if strings.ContainsAny(s, "@#%") { + return deny(ReasonArgumentPattern, "%s may not carry a revision specifier", name) + } + for _, seg := range strings.Split(strings.TrimPrefix(s, "//"), "/") { + if seg == ".." || seg == "." { + return deny(ReasonArgumentPattern, "%s may not contain a traversal segment", name) + } + } + return nil +} + +// checkScope is where an in-vocabulary, well-typed argument still gets denied. +// This is the layer that makes "the vocabulary is visible in the source you +// just read" an honest thing to tell an IT director. +func (a *Arg) checkScope(s string, scopes Scopes) *DenyError { + switch a.Scope { + case ScopeNone: + return nil + + case ScopeDepot: + // The literal prefix is everything before the first wildcard. A path + // whose literal prefix does not already sit inside a scoped prefix is + // denied, so //... is denied even for a P4 user who could read it: the + // wildcard cannot climb above the scope. + if !withinPrefixList(literalPrefix(s), scopes.DepotScope) { + return deny(ReasonOutOfScopePath, "%s is outside depot_scope", a.Name) + } + return nil + + case ScopeAllowedBuildTys: + if !containsExact(s, scopes.AllowedBuildTypes) { + return deny(ReasonOutOfScopeBuildTy, "%s is not in allowed_build_types", a.Name) + } + return nil + + case ScopeRepoAllowlist: + if !containsExact(s, scopes.RepoAllowlist) { + return deny(ReasonOutOfScopeRepo, "%s is not in repo_allowlist", a.Name) + } + return nil + } + return deny(ReasonOutOfScopePath, "%s has an unknown scope", a.Name) +} + +// literalPrefix returns the part of a depot path before the first Perforce +// wildcard. "//..." yields "//"; "//depot/game/..." yields "//depot/game/". +func literalPrefix(s string) string { + if i := strings.IndexAny(s, "*."); i >= 0 { + // "." alone is legal inside a filename; only "..." is the wildcard. + if s[i] == '.' { + if j := strings.Index(s, "..."); j >= 0 { + return s[:j] + } + if k := strings.IndexByte(s, '*'); k >= 0 { + return s[:k] + } + return s + } + return s[:i] + } + return s +} + +func withinPrefixList(s string, prefixes []string) bool { + for _, p := range prefixes { + if p == "" { + continue + } + if strings.HasPrefix(s, p) { + return true + } + } + return false +} + +func containsExact(s string, list []string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} + +// BannedArgNames exposes the banned list for the vocabulary test. +func BannedArgNames() map[string]string { return bannedArgNames } + +// ErrBannedArg is returned by Selfcheck. +var ErrBannedArg = errors.New("vocab: verb declares a banned argument name") + +// Selfcheck asserts the structural invariants of the vocabulary. It runs in the +// test suite and again at process start, so a build that violates one of these +// refuses to run rather than shipping a quietly weaker allowlist. +func Selfcheck() error { + for i := range Vocabulary { + v := &Vocabulary[i] + if v.Compiled && v.Mutating { + return fmt.Errorf("vocab: %s is mutating and compiled; no X-class verb ships in v0", v.Name) + } + if v.Compiled && v.Class == ClassContent { + return fmt.Errorf("vocab: %s is content class and compiled; content verbs are off in v0", v.Name) + } + for j := range v.Args { + a := &v.Args[j] + if why, banned := bannedArgNames[strings.ToLower(a.Name)]; banned { + return fmt.Errorf("%w: %s.%s (%s)", ErrBannedArg, v.Name, a.Name, why) + } + switch a.Kind { + case KindInt, KindBool, KindString, KindDepotPath: + default: + return fmt.Errorf("vocab: %s.%s has non-scalar kind %q", v.Name, a.Name, a.Kind) + } + if a.Kind == KindInt && a.Max <= 0 { + return fmt.Errorf("vocab: %s.%s is an unbounded integer", v.Name, a.Name) + } + if a.Kind == KindString && a.MaxLen <= 0 { + return fmt.Errorf("vocab: %s.%s is an unbounded string", v.Name, a.Name) + } + } + } + return nil +} diff --git a/internal/vocab/vocab_test.go b/internal/vocab/vocab_test.go new file mode 100644 index 0000000..b6919c4 --- /dev/null +++ b/internal/vocab/vocab_test.go @@ -0,0 +1,244 @@ +package vocab + +import ( + "encoding/json" + "strings" + "testing" +) + +var testScopes = Scopes{ + DepotScope: []string{"//butterstack-uat/", "//depot/game/"}, + AllowedBuildTypes: []string{"Uat_Build"}, + RepoAllowlist: []string{"studio/game"}, +} + +var allTools = ConfiguredTools{"teamcity": true, "perforce": true, "jenkins": true, "ghes": true, "horde": true} + +func TestSelfcheckPasses(t *testing.T) { + if err := Selfcheck(); err != nil { + t.Fatalf("vocabulary selfcheck failed: %v", err) + } +} + +// TestNoVerbAcceptsABannedArgument is the regression guard for Shuri F4. A +// caller-supplied parameter bag on a build-triggering verb is the finding that +// falsified the bounded-blast-radius claim; re-adding one must fail the build, +// not ship. +func TestNoVerbAcceptsABannedArgument(t *testing.T) { + banned := BannedArgNames() + for i := range Vocabulary { + v := &Vocabulary[i] + for j := range v.Args { + name := strings.ToLower(v.Args[j].Name) + if why, bad := banned[name]; bad { + t.Errorf("%s declares banned argument %q (%s)", v.Name, name, why) + } + } + } +} + +// TestEveryArgumentIsScalar keeps the schema incapable of expressing a map. +func TestEveryArgumentIsScalar(t *testing.T) { + for i := range Vocabulary { + v := &Vocabulary[i] + for j := range v.Args { + switch v.Args[j].Kind { + case KindInt, KindBool, KindString, KindDepotPath: + default: + t.Errorf("%s.%s has non-scalar kind %q", v.Name, v.Args[j].Name, v.Args[j].Kind) + } + } + } +} + +// TestNoMutatingOrContentVerbIsCompiled is the v0 posture: the spike runs with +// every X-class and C-class verb off. +func TestNoMutatingOrContentVerbIsCompiled(t *testing.T) { + for i := range Vocabulary { + v := &Vocabulary[i] + if v.Compiled && v.Mutating { + t.Errorf("%s is mutating and compiled", v.Name) + } + if v.Compiled && v.Class == ClassContent { + t.Errorf("%s is content class and compiled", v.Name) + } + } +} + +func TestNoSysExec(t *testing.T) { + for _, name := range []string{"sys.exec", "sys.shell", "sys.run", "exec"} { + if Lookup(name) != nil { + t.Fatalf("%s exists in the vocabulary", name) + } + } +} + +func resolve(t *testing.T, verb, args string) (*Verb, map[string]any, *DenyError) { + t.Helper() + return Resolve(verb, json.RawMessage(args), testScopes, allTools) +} + +func TestOutOfVocabularyVerbIsDenied(t *testing.T) { + for _, verb := range []string{"sys.exec", "p4.print", "teamcity.build.delete", ""} { + _, _, derr := resolve(t, verb, `{}`) + if derr == nil || derr.Reason != ReasonUnknownVerb { + t.Errorf("%q: want %s, got %v", verb, ReasonUnknownVerb, derr) + } + } +} + +func TestReservedVerbIsDenied(t *testing.T) { + for _, verb := range []string{"jenkins.build.trigger", "teamcity.build.queue", "horde.server.info"} { + _, _, derr := resolve(t, verb, `{}`) + if derr == nil || derr.Reason != ReasonVerbNotCompiled { + t.Errorf("%q: want %s, got %v", verb, ReasonVerbNotCompiled, derr) + } + } +} + +func TestContentVerbIsDeniedEvenWhenNamed(t *testing.T) { + _, _, derr := resolve(t, "p4.file_contents", `{"depot_path":"//depot/game/x.uasset"}`) + if derr == nil || derr.Reason != ReasonVerbNotCompiled { + t.Fatalf("want %s, got %v", ReasonVerbNotCompiled, derr) + } +} + +// TestSmuggledParamsAreRefused: the drill that actually tests condition 2 for +// the trigger verbs. Even if a trigger verb were compiled in, an unknown key +// stops the command instead of being ignored into the tool call. +func TestSmuggledParamsAreRefused(t *testing.T) { + cases := []struct{ verb, args string }{ + {"teamcity.build.get", `{"build_id":42,"properties":{"X":"$(id)"}}`}, + {"teamcity.build.get", `{"build_id":42,"params":{"X":"1"}}`}, + {"p4.describe", `{"change":7,"command":"rm -rf /"}`}, + {"p4.changes", `{"path":"//depot/game/...","url":"http://evil"}`}, + {"sys.ping", `{"host":"10.0.0.1"}`}, + } + for _, c := range cases { + _, _, derr := resolve(t, c.verb, c.args) + if derr == nil || derr.Reason != ReasonUnknownArgument { + t.Errorf("%s %s: want %s, got %v", c.verb, c.args, ReasonUnknownArgument, derr) + } + } +} + +func TestArgumentTypeValidationAtTheFrameBoundary(t *testing.T) { + cases := []struct{ verb, args, reason string }{ + {"teamcity.build.get", `{"build_id":"42"}`, ReasonArgumentType}, + {"teamcity.build.get", `{"build_id":1.5}`, ReasonArgumentType}, + {"teamcity.build.get", `{"build_id":0}`, ReasonArgumentRange}, + {"teamcity.build.get", `{}`, ReasonMissingArgument}, + {"teamcity.build.get", `[1,2]`, ReasonMalformedArgs}, + {"p4.describe", `{"change":7,"max_files":100000}`, ReasonArgumentRange}, + {"p4.changes", `{"path":"//depot/game/...","max":9999}`, ReasonArgumentRange}, + {"p4.changes", `{"path":42}`, ReasonArgumentType}, + } + for _, c := range cases { + _, _, derr := resolve(t, c.verb, c.args) + if derr == nil || derr.Reason != c.reason { + t.Errorf("%s %s: want %s, got %v", c.verb, c.args, c.reason, derr) + } + } +} + +// TestOutOfScopeArgumentIsDenied is the drill Shuri singled out: an +// in-vocabulary, well-typed verb whose argument falls outside the studio's own +// scope list is refused exactly like an unknown verb. +func TestOutOfScopeArgumentIsDenied(t *testing.T) { + cases := []struct{ args, reason string }{ + {`{"path":"//..."}`, ReasonOutOfScopePath}, + {`{"path":"//*"}`, ReasonOutOfScopePath}, + {`{"path":"//other/secret/..."}`, ReasonOutOfScopePath}, + {`{"path":"//depot/..."}`, ReasonOutOfScopePath}, + {`{"path":"//depot/game/../../other/..."}`, ReasonArgumentPattern}, + {`{"path":"//depot/game/x@=123"}`, ReasonArgumentPattern}, + {`{"path":"depot/game/..."}`, ReasonArgumentPattern}, + } + for _, c := range cases { + _, _, derr := resolve(t, "p4.changes", c.args) + if derr == nil || derr.Reason != c.reason { + t.Errorf("p4.changes %s: want %s, got %v", c.args, c.reason, derr) + } + } +} + +func TestInScopePathIsAllowed(t *testing.T) { + for _, p := range []string{ + `//depot/game/...`, + `//depot/game/Content/*`, + `//butterstack-uat/main/...`, + `//depot/game/Content/Maps/Main.umap`, + } { + _, args, derr := resolve(t, "p4.changes", `{"path":"`+p+`"}`) + if derr != nil { + t.Fatalf("%s: unexpected deny %v", p, derr) + } + if args["path"] != p { + t.Fatalf("%s: argument not passed through", p) + } + } +} + +// TestContentToggleCannotBeTurnedOnByACaller pins include_diff to false at the +// schema level, so no broker frame and no config value can flip it in v0. +func TestContentToggleCannotBeTurnedOnByACaller(t *testing.T) { + _, _, derr := resolve(t, "p4.describe", `{"change":7,"include_diff":true}`) + if derr == nil || derr.Reason != ReasonContentVerbOff { + t.Fatalf("want %s, got %v", ReasonContentVerbOff, derr) + } + if _, _, derr := resolve(t, "p4.describe", `{"change":7,"include_diff":false}`); derr != nil { + t.Fatalf("include_diff:false should be accepted, got %v", derr) + } +} + +func TestUnconfiguredToolIsDeniedButVocabularyIsCheckedFirst(t *testing.T) { + none := ConfiguredTools{} + // A configured-tool denial must never be reachable for a name that is not + // in the vocabulary: an unknown verb must not reveal what is configured. + if _, _, derr := Resolve("p4.nope", json.RawMessage(`{}`), testScopes, none); derr.Reason != ReasonUnknownVerb { + t.Fatalf("want %s, got %s", ReasonUnknownVerb, derr.Reason) + } + if _, _, derr := Resolve("p4.changes", json.RawMessage(`{"path":"//depot/game/..."}`), testScopes, none); derr.Reason != ReasonToolNotConfigured { + t.Fatalf("want %s, got %s", ReasonToolNotConfigured, derr.Reason) + } +} + +func TestEmptyDepotScopeDeniesEveryPath(t *testing.T) { + _, _, derr := Resolve("p4.changes", json.RawMessage(`{"path":"//depot/game/..."}`), Scopes{}, allTools) + if derr == nil || derr.Reason != ReasonOutOfScopePath { + t.Fatalf("an unconfigured depot_scope must fail closed, got %v", derr) + } +} + +func TestLiteralPrefix(t *testing.T) { + cases := map[string]string{ + "//...": "//", + "//depot/game/...": "//depot/game/", + "//depot/game/*": "//depot/game/", + "//depot/a.b/...": "//depot/a.b/", + "//depot/game/Main.umap": "//depot/game/Main.umap", + "//depot/game/*.uasset": "//depot/game/", + } + for in, want := range cases { + if got := literalPrefix(in); got != want { + t.Errorf("literalPrefix(%q) = %q, want %q", in, got, want) + } + } +} + +func TestCompiledVerbsAreTheAnnouncedCapabilities(t *testing.T) { + got := CompiledVerbs() + want := map[string]bool{ + "sys.ping": true, "sys.version": true, "sys.capabilities": true, + "teamcity.server.info": true, "teamcity.build.get": true, + "p4.describe": true, "p4.changes": true, + } + if len(got) != len(want) { + t.Fatalf("compiled verbs = %v", got) + } + for _, v := range got { + if !want[v] { + t.Errorf("unexpected compiled verb %q", v) + } + } +} diff --git a/internal/wsclient/wsclient.go b/internal/wsclient/wsclient.go new file mode 100644 index 0000000..5b2110e --- /dev/null +++ b/internal/wsclient/wsclient.go @@ -0,0 +1,398 @@ +// Package wsclient is a minimal RFC 6455 WebSocket client, client side only. +// +// It exists instead of a third-party dependency for two reasons. First, the +// connector's whole security story is "read the source": a studio's IT director +// can read this file in a sitting, which is not true of a general-purpose +// WebSocket library. Second, survival condition 1 asks for a small, auditable +// dependency surface, and a hand-written client that speaks the subset we +// actually use (client-initiated, masked text frames, ping/pong, close) is a +// materially smaller surface than one that also implements permessage-deflate, +// extensions, and a server half we never run. +// +// What this does NOT implement, on purpose: extensions, compression, subprotocol +// negotiation, and any server role. What it does implement strictly: the +// Sec-WebSocket-Accept check, client-side masking (required by RFC 6455 6.1), +// continuation frames up to a hard message cap, and control-frame size limits. +package wsclient + +import ( + "bufio" + "crypto/rand" + "crypto/sha1" + "crypto/tls" + "crypto/x509" + "encoding/base64" + "encoding/binary" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/url" + "os" + "strings" + "sync" + "time" +) + +// wsGUID is the RFC 6455 handshake constant. +const wsGUID = "258EAFA5-E914-47DA-95CA-5AB0DC85B11C" + +// Opcodes. +const ( + opContinuation = 0x0 + opText = 0x1 + opBinary = 0x2 + opClose = 0x8 + opPing = 0x9 + opPong = 0xA +) + +// Limits. A frame larger than these is a protocol error and closes the socket; +// the connector never needs a large inbound frame because commands are small +// and results flow the other way. +const ( + MaxMessageBytes = 1 << 20 // 1 MiB inbound message cap + maxControlBytes = 125 +) + +// Errors callers distinguish. +var ( + ErrQueryString = errors.New("wsclient: endpoint must not carry a query string") + ErrHandshake = errors.New("wsclient: handshake failed") + ErrUnauthorized = errors.New("wsclient: broker rejected the connector token") + ErrMessageTooBig = errors.New("wsclient: inbound message exceeds cap") + ErrBinaryFrame = errors.New("wsclient: binary frames are not part of this protocol") + ErrClosedByServer = errors.New("wsclient: server closed the connection") +) + +// Options configures a dial. +type Options struct { + // Endpoint must be a wss:// URL with no query string. The connector token + // is sent in the Authorization header and nowhere else. + Endpoint string + + // Token is the bsc_ connector token. + Token string + + // CAFile, when set, is the only root the TLS handshake will trust. It + // exists for a private CA and for the drill harness; there is no option to + // skip verification. + CAFile string + + UserAgent string + DialTimeout time.Duration + HandshakeLimit time.Duration +} + +// Conn is a live WebSocket connection. +type Conn struct { + raw net.Conn + br *bufio.Reader + + writeMu sync.Mutex + closed bool + closeMu sync.Mutex +} + +// Dial performs the TLS and WebSocket handshakes. +// +// The Authorization header is the only place the token appears. There is no +// code path in this package that writes the token into the request line, and +// a URL carrying a query string is refused before a socket is opened, so a +// misconfigured endpoint cannot leak a credential into a proxy access log. +func Dial(opts Options) (*Conn, error) { + u, err := url.Parse(opts.Endpoint) + if err != nil { + return nil, fmt.Errorf("wsclient: %w", err) + } + if u.Scheme != "wss" { + return nil, fmt.Errorf("wsclient: endpoint scheme must be wss, got %q", u.Scheme) + } + if u.RawQuery != "" { + return nil, ErrQueryString + } + if opts.DialTimeout == 0 { + opts.DialTimeout = 10 * time.Second + } + if opts.HandshakeLimit == 0 { + opts.HandshakeLimit = 15 * time.Second + } + + host := u.Host + if !strings.Contains(host, ":") { + host += ":443" + } + + tlsCfg := &tls.Config{ + ServerName: u.Hostname(), + MinVersion: tls.VersionTLS12, + } + if opts.CAFile != "" { + pem, err := os.ReadFile(opts.CAFile) + if err != nil { + return nil, fmt.Errorf("wsclient: endpoint_ca_file: %w", err) + } + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(pem) { + return nil, errors.New("wsclient: endpoint_ca_file contains no certificates") + } + tlsCfg.RootCAs = pool + } + + dialer := &net.Dialer{Timeout: opts.DialTimeout} + raw, err := tls.DialWithDialer(dialer, "tcp", host, tlsCfg) + if err != nil { + return nil, fmt.Errorf("wsclient: dial: %w", err) + } + + _ = raw.SetDeadline(time.Now().Add(opts.HandshakeLimit)) + + keyRaw := make([]byte, 16) + if _, err := rand.Read(keyRaw); err != nil { + raw.Close() + return nil, err + } + key := base64.StdEncoding.EncodeToString(keyRaw) + + path := u.EscapedPath() + if path == "" { + path = "/" + } + var req strings.Builder + fmt.Fprintf(&req, "GET %s HTTP/1.1\r\n", path) + fmt.Fprintf(&req, "Host: %s\r\n", u.Host) + req.WriteString("Upgrade: websocket\r\n") + req.WriteString("Connection: Upgrade\r\n") + fmt.Fprintf(&req, "Sec-WebSocket-Key: %s\r\n", key) + req.WriteString("Sec-WebSocket-Version: 13\r\n") + fmt.Fprintf(&req, "Authorization: Bearer %s\r\n", opts.Token) + if opts.UserAgent != "" { + fmt.Fprintf(&req, "User-Agent: %s\r\n", opts.UserAgent) + } + req.WriteString("\r\n") + + if _, err := io.WriteString(raw, req.String()); err != nil { + raw.Close() + return nil, fmt.Errorf("wsclient: write handshake: %w", err) + } + + br := bufio.NewReaderSize(raw, 8192) + resp, err := http.ReadResponse(br, nil) + if err != nil { + raw.Close() + return nil, fmt.Errorf("%w: %v", ErrHandshake, err) + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + raw.Close() + return nil, fmt.Errorf("%w (HTTP %d)", ErrUnauthorized, resp.StatusCode) + } + if resp.StatusCode != http.StatusSwitchingProtocols { + raw.Close() + return nil, fmt.Errorf("%w: HTTP %d", ErrHandshake, resp.StatusCode) + } + if !strings.EqualFold(resp.Header.Get("Upgrade"), "websocket") { + raw.Close() + return nil, fmt.Errorf("%w: missing Upgrade: websocket", ErrHandshake) + } + if got, want := resp.Header.Get("Sec-WebSocket-Accept"), AcceptKey(key); got != want { + raw.Close() + return nil, fmt.Errorf("%w: Sec-WebSocket-Accept mismatch", ErrHandshake) + } + + _ = raw.SetDeadline(time.Time{}) + return &Conn{raw: raw, br: br}, nil +} + +// AcceptKey computes the RFC 6455 Sec-WebSocket-Accept value. Exported so the +// drill harness can assert both halves agree. +func AcceptKey(clientKey string) string { + h := sha1.New() + io.WriteString(h, clientKey+wsGUID) + return base64.StdEncoding.EncodeToString(h.Sum(nil)) +} + +// WriteText sends one unfragmented masked text frame. +func (c *Conn) WriteText(p []byte) error { return c.writeFrame(opText, p) } + +// WritePing sends a ping. +func (c *Conn) WritePing() error { return c.writeFrame(opPing, nil) } + +// WriteClose sends a close frame with the given status code. +func (c *Conn) WriteClose(code uint16, reason string) error { + buf := make([]byte, 2+len(reason)) + binary.BigEndian.PutUint16(buf, code) + copy(buf[2:], reason) + if len(buf) > maxControlBytes { + buf = buf[:maxControlBytes] + } + return c.writeFrame(opClose, buf) +} + +func (c *Conn) writeFrame(opcode byte, payload []byte) error { + c.writeMu.Lock() + defer c.writeMu.Unlock() + + var hdr []byte + b0 := byte(0x80) | opcode // FIN set; this client never fragments + n := len(payload) + switch { + case n <= 125: + hdr = []byte{b0, byte(0x80) | byte(n)} + case n <= 0xFFFF: + hdr = make([]byte, 4) + hdr[0], hdr[1] = b0, 0x80|126 + binary.BigEndian.PutUint16(hdr[2:], uint16(n)) + default: + hdr = make([]byte, 10) + hdr[0], hdr[1] = b0, 0x80|127 + binary.BigEndian.PutUint64(hdr[2:], uint64(n)) + } + + // RFC 6455 6.1: a client MUST mask every frame it sends, with a fresh key. + var mask [4]byte + if _, err := rand.Read(mask[:]); err != nil { + return err + } + masked := make([]byte, n) + for i := 0; i < n; i++ { + masked[i] = payload[i] ^ mask[i%4] + } + + if err := c.raw.SetWriteDeadline(time.Now().Add(30 * time.Second)); err != nil { + return err + } + if _, err := c.raw.Write(hdr); err != nil { + return err + } + if _, err := c.raw.Write(mask[:]); err != nil { + return err + } + if n > 0 { + if _, err := c.raw.Write(masked); err != nil { + return err + } + } + return nil +} + +// ReadMessage returns the next complete text message, transparently answering +// pings and reassembling continuation frames. A read deadline is the connector's +// liveness check: if the broker stops heartbeating, the read fails and the +// session reconnects. +func (c *Conn) ReadMessage(deadline time.Time) ([]byte, error) { + var msg []byte + for { + if err := c.raw.SetReadDeadline(deadline); err != nil { + return nil, err + } + fin, opcode, payload, err := c.readFrame() + if err != nil { + return nil, err + } + switch opcode { + case opPing: + if err := c.writeFrame(opPong, payload); err != nil { + return nil, err + } + case opPong: + // liveness only + case opClose: + _ = c.writeFrame(opClose, payload) + return nil, ErrClosedByServer + case opBinary: + return nil, ErrBinaryFrame + case opText, opContinuation: + if len(msg)+len(payload) > MaxMessageBytes { + return nil, ErrMessageTooBig + } + msg = append(msg, payload...) + if fin { + return msg, nil + } + default: + return nil, fmt.Errorf("wsclient: unknown opcode 0x%x", opcode) + } + } +} + +func (c *Conn) readFrame() (fin bool, opcode byte, payload []byte, err error) { + var h [2]byte + if _, err = io.ReadFull(c.br, h[:]); err != nil { + return + } + fin = h[0]&0x80 != 0 + if h[0]&0x70 != 0 { + err = errors.New("wsclient: reserved bits set (no extensions are negotiated)") + return + } + opcode = h[0] & 0x0F + masked := h[1]&0x80 != 0 + length := uint64(h[1] & 0x7F) + + switch length { + case 126: + var e [2]byte + if _, err = io.ReadFull(c.br, e[:]); err != nil { + return + } + length = uint64(binary.BigEndian.Uint16(e[:])) + case 127: + var e [8]byte + if _, err = io.ReadFull(c.br, e[:]); err != nil { + return + } + length = binary.BigEndian.Uint64(e[:]) + } + + isControl := opcode&0x8 != 0 + if isControl { + if !fin { + err = errors.New("wsclient: fragmented control frame") + return + } + if length > maxControlBytes { + err = errors.New("wsclient: oversized control frame") + return + } + } + if length > MaxMessageBytes { + err = ErrMessageTooBig + return + } + + var mask [4]byte + if masked { + // A server must not mask, but tolerate and unmask rather than desync. + if _, err = io.ReadFull(c.br, mask[:]); err != nil { + return + } + } + payload = make([]byte, length) + if _, err = io.ReadFull(c.br, payload); err != nil { + return + } + if masked { + for i := range payload { + payload[i] ^= mask[i%4] + } + } + return +} + +// Close shuts the socket down. Safe to call more than once. +func (c *Conn) Close() error { + c.closeMu.Lock() + defer c.closeMu.Unlock() + if c.closed { + return nil + } + c.closed = true + _ = c.WriteClose(1000, "going away") + return c.raw.Close() +} + +// LocalAddr reports the local side, used by the drills to correlate with ss/netstat. +func (c *Conn) LocalAddr() net.Addr { return c.raw.LocalAddr() } diff --git a/internal/wsclient/wsclient_test.go b/internal/wsclient/wsclient_test.go new file mode 100644 index 0000000..6c82654 --- /dev/null +++ b/internal/wsclient/wsclient_test.go @@ -0,0 +1,36 @@ +package wsclient + +import ( + "errors" + "testing" +) + +// TestAcceptKey pins base64(sha1(key + RFC 6455 GUID)) for the RFC's sample +// key. The value is cross-checked against two independent implementations: a +// one-line Python hashlib computation, and the Ruby handshake in +// connector/test/support/ws.rb, which the drill harness runs against this +// client for real. A regression here would show up as a failed handshake in +// every drill, not just as a failed unit test. +func TestAcceptKey(t *testing.T) { + if got, want := AcceptKey("dGhlIHNhbXBsZSBub25jZQ=="), "84qioe71YN9dzYnTCQMk2L+0/kA="; got != want { + t.Fatalf("AcceptKey = %q, want %q", got, want) + } +} + +// TestDialRefusesAQueryString is the client half of drill (g). The token is a +// header value and nothing else, so an endpoint that carries a query string is +// refused before a socket is opened. +func TestDialRefusesAQueryString(t *testing.T) { + _, err := Dial(Options{Endpoint: "wss://example.invalid/connect?token=bsc_a_b", Token: "bsc_a_b"}) + if !errors.Is(err, ErrQueryString) { + t.Fatalf("want ErrQueryString, got %v", err) + } +} + +func TestDialRefusesPlaintextSchemes(t *testing.T) { + for _, ep := range []string{"ws://example.invalid/connect", "http://example.invalid/connect"} { + if _, err := Dial(Options{Endpoint: ep}); err == nil { + t.Errorf("%s was accepted", ep) + } + } +} diff --git a/test/README.md b/test/README.md new file mode 100644 index 0000000..4f490e1 --- /dev/null +++ b/test/README.md @@ -0,0 +1,45 @@ +# Drills + +`ruby drills.rb` (or `make drills` from the parent directory) runs the seven +drills from design note §4.3 against `mock_broker.rb`, plus a round-trip phase +and the broker-side half of drill (f). + +``` +CONNECTOR_BIN=../build/butterstack-connector ruby drills.rb +VERBOSE=1 CONNECTOR_BIN=../build/butterstack-connector ruby drills.rb # per-assertion output +``` + +Everything runs on loopback in a temporary directory: a throwaway CA and server +certificate so the connector dials a real `wss://` endpoint with real +certificate verification, a stub TeamCity that refuses any request not bearing +the token from `connector.yml`, and a fake `p4` that records its own argv. + +Requires Ruby 3.2+ (stdlib only) and a built connector binary. Takes about +15 seconds; the recovery drills contain real waits. + +## Files + +| | | +|---|---| +| `drills.rb` | the harness and the assertions | +| `mock_broker.rb` | the ButterStack side: `/connect` upgrade, header-only token auth, frame exchange, revoke, partition | +| `support/ws.rb` | server-side RFC 6455, written independently of the Go client so a framing mistake fails a drill rather than agreeing with itself | +| `support/tls.rb` | throwaway CA and server certificate | +| `support/teamcity_stub.rb` | a minimal on-prem TeamCity | +| `support/fake_p4` | a `p4` stand-in emitting `-Mj -ztag` records and logging its argv | + +## What the mock broker is not + +It is not the broker. The real one is a dedicated Rack endpoint at `/connect` on +the Rails app with hashed-token auth, a Redis-routed command and result path, +and explicit tenant scoping — none of which exists yet and none of which this +models. It does implement faithfully the four rules the drills exist to prove: + +1. the upgrade is refused **before any per-connection state is allocated** when + the token is absent, malformed, revoked, or wrong; +2. a query-string token is refused, and no code path here reads a token from a + query string; +3. tokens are stored as the SHA-256 digest of the secret segment and compared in + constant time — the plaintext is never held; +4. a `result` frame is matched against the issuing session's own outstanding + commands; one carrying another session's command id is discarded. diff --git a/test/drills.rb b/test/drills.rb new file mode 100644 index 0000000..4d90249 --- /dev/null +++ b/test/drills.rb @@ -0,0 +1,545 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# The seven drills from the connector design note section 4.3, run entirely +# locally against the mock broker. +# +# Four recovery drills: +# R1 the studio's network drops -> the connector reconnects +# R2 the connector is stopped -> we go offline and degrade +# R3 our side is stopped and restarted -> backoff, then reconnect +# R4 the connector token is revoked -> the socket closes within one +# heartbeat and the studio's own +# credentials are untouched +# Three denial drills: +# D1 an out-of-vocabulary verb -> denied +# D2 an in-vocabulary verb with an +# out-of-scope argument -> denied +# D3 a query-string token on /connect -> refused before session state +# +# Plus the broker-side half of drill (f): a result frame carrying another +# session's command id is discarded. The other half of (f) -- asserting that +# tenant context is nil at the start of the next request on the same Puma +# thread -- is Rails-side and is NOT covered here; see README "what this does +# not prove". +# +# Usage: CONNECTOR_BIN=./build/butterstack-connector ruby test/drills.rb +require 'fileutils' +require 'json' +require 'open3' +require 'tmpdir' +require_relative 'mock_broker' +require_relative 'support/teamcity_stub' + +VERBOSE = ENV['VERBOSE'] == '1' + +def log(msg) + warn(" #{msg}") if VERBOSE +end + +CONNECTOR_BIN = ENV.fetch('CONNECTOR_BIN', File.expand_path('../build/butterstack-connector', __dir__)) +unless File.executable?(CONNECTOR_BIN) + abort "drills: #{CONNECTOR_BIN} is missing or not executable. Run `make build` first." +end + +# A connector token in the specified format: bsc__. +# 52 base32 characters is 32 random bytes, which is why the design says a plain +# SHA-256 digest is the right storage and a slow KDF would buy nothing. +BASE32 = (('A'..'Z').to_a + ('2'..'7').to_a).freeze + +def new_token(integration_id = 'intg7f3a') + "bsc_#{integration_id}_#{Array.new(52) { BASE32.sample }.join}" +end + +TOKEN = new_token +TEAMCITY_TOKEN = 'tc-local-token-do-not-egress' +P4_TICKET = 'p4-local-ticket-do-not-egress' + +# --------------------------------------------------------------------------- +# Result plumbing +# --------------------------------------------------------------------------- +Result = Struct.new(:name, :title, :ok, :detail, :evidence, keyword_init: true) +RESULTS = [] + +def drill(name, title) + started = Time.now + detail = [] + ok = true + begin + yield ->(claim, cond, note = nil) { + pass = !!cond + ok &&= pass + detail << "#{pass ? 'PASS' : 'FAIL'} #{claim}#{note ? " (#{note})" : ''}" + } + rescue StandardError => e + ok = false + detail << "FAIL raised #{e.class}: #{e.message}" + detail.concat(e.backtrace.first(3).map { |l| " #{l}" }) if VERBOSE + end + RESULTS << Result.new(name: name, title: title, ok: ok, detail: detail, + evidence: format('%.2fs', Time.now - started)) + warn("#{ok ? 'ok ' : 'FAIL'} #{name} #{title}") + detail.each { |d| warn(" #{d}") } if VERBOSE || !ok +end + +# --------------------------------------------------------------------------- +# Harness +# --------------------------------------------------------------------------- +WORK = Dir.mktmpdir('connector-drills-') +at_exit { FileUtils.remove_entry(WORK) if File.directory?(WORK) } + +LOGGER = ->(m) { log(m) } + +teamcity = TeamCityStub.new(token: TEAMCITY_TOKEN, logger: LOGGER).start +teamcity.add_build(9001, build_type_id: 'Uat_Build', number: '512', revision: '41337') + +broker = MockBroker.new(tls_dir: File.join(WORK, 'tls'), logger: LOGGER) +broker.register_token(TOKEN, integration_id: 'intg7f3a') +broker.start + +ARGV_LOG = File.join(WORK, 'p4-argv.log') + +# The connector hands its p4 subprocess a deliberately minimal environment +# (PATH, HOME, P4PORT, P4USER, P4PASSWD and nothing else), which is the right +# hygiene and also means the harness cannot pass the fake p4 its log path +# through the environment. So connector.yml points at a wrapper with the path +# baked in, and the daemon stays unweakened for the sake of the test. +P4_WRAPPER = File.join(WORK, 'p4-wrapper') +File.write(P4_WRAPPER, <<~SH) + #!/bin/sh + FAKE_P4_ARGV_LOG='#{ARGV_LOG}' exec '#{File.expand_path('support/fake_p4', __dir__)}' "$@" +SH +File.chmod(0o755, P4_WRAPPER) +CONFIG_PATH = File.join(WORK, 'connector.yml') +LOG_DIR = File.join(WORK, 'logs') + +def write_config(path, broker:, teamcity:, log_dir:) + File.write(path, <<~YAML) + endpoint: #{broker.endpoint} + endpoint_ca_file: #{broker.ca_pem_path} + token: #{TOKEN} + connector_id: drill-harness + log_dir: #{log_dir} + max_concurrent: 4 + scopes: + depot_scope: + - //depot/game/ + allowed_build_types: + - Uat_Build + perforce: + enabled: true + binary: #{P4_WRAPPER} + port: ssl:p4.lan:1666 + user: butterstack-ro + ticket: #{P4_TICKET} + teamcity: + enabled: true + url: #{teamcity.base_url} + token: #{TEAMCITY_TOKEN} + YAML + File.chmod(0o600, path) +end + +write_config(CONFIG_PATH, broker: broker, teamcity: teamcity, log_dir: LOG_DIR) +CONFIG_FINGERPRINT = [File.read(CONFIG_PATH), File.stat(CONFIG_PATH).mode] + +CONNECTOR_STDERR = File.join(WORK, 'connector.stderr') + +def start_connector + env = { + 'FAKE_P4_ARGV_LOG' => ARGV_LOG, + 'PATH' => ENV.fetch('PATH', '/usr/bin:/bin'), + 'HOME' => ENV.fetch('HOME', '/tmp'), + # Deliberately present and deliberately ignored: the config loader has no + # environment fallback for any credential. + 'BUTTERSTACK_CONNECTOR_TOKEN' => 'bsc_evil_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', + 'P4PASSWD' => 'env-ticket-should-not-be-used' + } + pid = Process.spawn(env, CONNECTOR_BIN, '-config', CONFIG_PATH, unsetenv_others: true, + out: [CONNECTOR_STDERR, 'a'], err: [CONNECTOR_STDERR, 'a']) + at_exit { begin; Process.kill('KILL', pid); rescue StandardError; nil; end } + pid +end + +def stop_connector(pid, signal: 'TERM') + Process.kill(signal, pid) + Process.waitpid(pid) +rescue Errno::ESRCH, Errno::ECHILD + nil +end + +def audit_lines + Dir[File.join(LOG_DIR, 'audit-*.log')].flat_map { |f| File.readlines(f) } + .filter_map { |l| JSON.parse(l) rescue nil } +end + +def audit_for(command_id) + audit_lines.find { |e| e['command_id'] == command_id } +end + +# =========================================================================== +# Phase 0: the connector connects and the compiled verbs round-trip. +# =========================================================================== +PID = start_connector +session = broker.wait_for_session(20) +abort "drills: the connector never connected. stderr:\n#{File.read(CONNECTOR_STDERR)}" unless session + +LATENCIES = {} + +drill('P0', 'compiled verbs round-trip end to end') do |a| + hello = session.hello + a.call('hello announces the compiled vocabulary', + hello['capabilities'].sort == %w[p4.changes p4.describe sys.capabilities sys.ping sys.version + teamcity.build.get teamcity.server.info], + hello['capabilities'].sort.join(',')) + a.call('hello announces no mutating or content verb', + hello['capabilities'].none? { |v| v =~ /trigger|queue|contents|log\.tail/ }) + + %w[sys.ping sys.version sys.capabilities].each do |verb| + t0 = Time.now + res = broker.call(verb) + LATENCIES[verb] = Time.now - t0 + a.call("#{verb} returns ok", res['status'] == 'ok', res['reason']) + end + + t0 = Time.now + info = broker.call('teamcity.server.info') + LATENCIES['teamcity.server.info'] = Time.now - t0 + a.call('teamcity.server.info returns ok', info['status'] == 'ok', info['reason']) + a.call('teamcity.server.info projects only the declared fields', + info.dig('body', 'version').to_s.start_with?('2025.03') && + info['body'].keys.sort == %w[buildNumber version versionMajor versionMinor webUrl], + info['body']&.keys&.join(',')) + + t0 = Time.now + build = broker.call('teamcity.build.get', { 'build_id' => 9001 }) + LATENCIES['teamcity.build.get'] = Time.now - t0 + a.call('teamcity.build.get returns ok', build['status'] == 'ok', build['reason']) + a.call('teamcity.build.get returns the build we seeded', + build.dig('body', 'number') == '512' && build.dig('body', 'buildTypeId') == 'Uat_Build') + + t0 = Time.now + desc = broker.call('p4.describe', { 'change' => 41_337 }) + LATENCIES['p4.describe'] = Time.now - t0 + a.call('p4.describe returns ok', desc['status'] == 'ok', desc['reason']) + a.call('p4.describe returns the file list and no diff', + desc.dig('body', 'file_count') == 2 && !desc['body'].key?('diff')) + + t0 = Time.now + changes = broker.call('p4.changes', { 'path' => '//depot/game/...' }) + LATENCIES['p4.changes'] = Time.now - t0 + a.call('p4.changes returns ok', changes['status'] == 'ok', changes['reason']) + + a.call('every verb round-trips in under 2s', + LATENCIES.values.all? { |v| v < 2.0 }, + LATENCIES.map { |k, v| format('%s=%.0fms', k, v * 1000) }.join(' ')) + + # Local credential custody, positively: the TeamCity stub only answers a + # request bearing the token that lives in connector.yml. + a.call('the TeamCity token used on the LAN came from connector.yml', + teamcity.requests.any? && teamcity.requests.all? { |r| r[:authorization] == "Bearer #{TEAMCITY_TOKEN}" }) +end + +# =========================================================================== +# D1: an out-of-vocabulary verb is denied. +# =========================================================================== +drill('D1', 'out-of-vocabulary verb is denied') do |a| + %w[sys.exec p4.print teamcity.build.delete jenkins.node.exec].each do |verb| + id = session.issue(verb, {}, deadline_ms: 5000, max_bytes: 0) + res = session.await(id, 10) + a.call("#{verb} is denied", res && res['status'] == 'denied', res && res['reason']) + a.call("#{verb} is denied as unknown_verb", res && res['reason'] == 'unknown_verb') + entry = audit_for(id) + a.call("#{verb} wrote a local audit line", entry && entry['status'] == 'denied') + end + + # A verb name the schema reserves but this build does not compile in is + # refused the same way. This is what makes "a new verb requires a connector + # version with that verb compiled in" true rather than aspirational. + %w[jenkins.build.trigger teamcity.build.queue p4.file_contents horde.server.info].each do |verb| + id = session.issue(verb, {}, deadline_ms: 5000, max_bytes: 0) + res = session.await(id, 10) + a.call("reserved #{verb} is denied as verb_not_compiled", + res && res['status'] == 'denied' && res['reason'] == 'verb_not_compiled', + res && res['reason']) + end +end + +# =========================================================================== +# D2: an in-vocabulary verb with an out-of-scope argument is denied. +# This is the drill that actually tests survival condition 2; D1 alone only +# tests the dispatcher. +# =========================================================================== +drill('D2', 'in-vocabulary verb with an out-of-scope argument is denied') do |a| + cases = [ + ['p4.changes', { 'path' => '//...' }, 'out_of_scope_path', + 'the whole depot, which the P4 user may well be able to read'], + ['p4.changes', { 'path' => '//depot/...' }, 'out_of_scope_path', + 'a wildcard one level above the scoped prefix'], + ['p4.changes', { 'path' => '//other/secrets/...' }, 'out_of_scope_path', + 'a different depot entirely'], + ['p4.changes', { 'path' => '//depot/game/../../other/...' }, 'argument_pattern', + 'traversal out of the scoped prefix'], + ['p4.describe', { 'change' => '41337' }, 'argument_type', + 'a quoted integer'], + ['p4.describe', { 'change' => 41_337, 'include_diff' => true }, 'content_verb_disabled', + 'a content toggle the caller does not get to set'], + ['p4.describe', { 'change' => 41_337, 'params' => { 'X' => '$(id)' } }, 'unknown_argument', + 'a smuggled parameter bag'], + ['teamcity.build.get', { 'build_id' => 9001, 'properties' => { 'env.X' => '1' } }, 'unknown_argument', + 'TeamCity properties, which build steps consume'], + ['teamcity.build.get', { 'build_id' => 0 }, 'argument_range', nil], + ['teamcity.build.get', { 'build_id' => '9001 OR 1=1' }, 'argument_type', nil] + ] + + cases.each do |verb, args, reason, note| + id = session.issue(verb, args, deadline_ms: 5000, max_bytes: 0) + res = session.await(id, 10) + a.call("#{verb} #{JSON.generate(args)} is denied", res && res['status'] == 'denied', res && res['reason']) + a.call(" ... as #{reason}", res && res['reason'] == reason, note) + entry = audit_for(id) + a.call(' ... with a local audit line', entry && entry['status'] == 'denied' && entry['reason'] == reason) + end + + # The denial happened before any tool call: the fake p4 never saw these. + argv = File.exist?(ARGV_LOG) ? File.readlines(ARGV_LOG).map { |l| JSON.parse(l) } : [] + a.call('no denied p4 verb reached the p4 binary', + argv.none? { |v| v.join(' ').include?('//other') || v.join(' ') =~ %r{//\.\.\.} }, + "#{argv.size} p4 invocations recorded") + + # And the no-shell property: a path carrying shell metacharacters, inside the + # scope, arrives at p4 as exactly one argv element with its bytes unchanged. + metachar_path = '//depot/game/$(touch ' + File.join(WORK, 'pwned') + ')/...' + id = session.issue('p4.changes', { 'path' => metachar_path }, deadline_ms: 5000, max_bytes: 0) + res = session.await(id, 10) + a.call('an in-scope path with shell metacharacters is executed, not interpreted', + res && res['status'] == 'ok', res && res['reason']) + a.call('no shell ran: the side-effect file does not exist', !File.exist?(File.join(WORK, 'pwned'))) + argv = File.readlines(ARGV_LOG).map { |l| JSON.parse(l) } + a.call('the path arrived as one literal argv element', + argv.any? { |v| v.include?(metachar_path) }, + 'argv array invocation, no shell interpretation') +end + +# =========================================================================== +# D3: a query-string token on the /connect upgrade is refused before any +# session state is allocated. (Devin section 4.3 drill (g).) +# =========================================================================== +drill('D3', 'query-string token on /connect is rejected before session state') do |a| + before = broker.sessions.size + + # The connector itself refuses to start with such an endpoint: the client half + # of the rule, so a copy-pasted URL cannot leak a token into a proxy log. + bad_cfg = File.join(WORK, 'connector-querystring.yml') + File.write(bad_cfg, File.read(CONFIG_PATH).sub(broker.endpoint, "#{broker.endpoint}?token=#{TOKEN}")) + File.chmod(0o600, bad_cfg) + _out, err, status = Open3.capture3(CONNECTOR_BIN, '-config', bad_cfg) + a.call('the connector refuses an endpoint carrying a query string', + !status.success? && err.include?('query string'), err.strip.lines.first&.strip) + + # And the broker half: a raw client that puts the token in the URL and sends + # no Authorization header is refused with an HTTP status, before the upgrade. + require 'socket' + raw = TCPSocket.new('127.0.0.1', broker.port) + ctx = OpenSSL::SSL::SSLContext.new + ctx.cert_store = OpenSSL::X509::Store.new.tap { |s| s.add_file(broker.ca_pem_path) } + ctx.verify_mode = OpenSSL::SSL::VERIFY_PEER + ssl = OpenSSL::SSL::SSLSocket.new(raw, ctx) + ssl.hostname = 'localhost' + ssl.connect + ssl.write("GET /connect?token=#{TOKEN} HTTP/1.1\r\n" \ + "Host: localhost\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n" \ + "Sec-WebSocket-Key: AAAAAAAAAAAAAAAAAAAAAA==\r\nSec-WebSocket-Version: 13\r\n\r\n") + response = begin + ssl.readpartial(1024) + rescue StandardError + '' + end + ssl.close rescue nil + + a.call('the broker answers an HTTP refusal, never a 101', + response.start_with?('HTTP/1.1 400'), response.lines.first&.strip) + a.call('the refusal was recorded as a query-string token', + broker.refusals.any? { |r| r[:reason] == 'query_string_token' }) + a.call('no session state was allocated', broker.sessions.size == before) + + # A missing and a wrong bearer token are refused the same way. + %w[missing wrong].each do |kind| + raw2 = TCPSocket.new('127.0.0.1', broker.port) + ssl2 = OpenSSL::SSL::SSLSocket.new(raw2, ctx) + ssl2.hostname = 'localhost' + ssl2.connect + auth = kind == 'wrong' ? "Authorization: Bearer bsc_intg7f3a_ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ\r\n" : '' + ssl2.write("GET /connect HTTP/1.1\r\nHost: localhost\r\nUpgrade: websocket\r\n" \ + "Connection: Upgrade\r\nSec-WebSocket-Key: AAAAAAAAAAAAAAAAAAAAAA==\r\n" \ + "Sec-WebSocket-Version: 13\r\n#{auth}\r\n") + resp2 = begin + ssl2.readpartial(1024) + rescue StandardError + '' + end + ssl2.close rescue nil + a.call("a #{kind} bearer token is refused with 401", resp2.start_with?('HTTP/1.1 401'), resp2.lines.first&.strip) + end + a.call('no session state was allocated for any refusal', broker.sessions.size == before) +end + +# =========================================================================== +# (f) broker-side half: a result frame carrying another session's command id is +# discarded rather than dispatched by id alone. +# =========================================================================== +drill('F-partial', 'a result frame for another session\'s command id is discarded') do |a| + before = broker.discarded_results.size + + # Send a command whose id the broker never records as outstanding. The + # connector answers it, and the answer arrives as a result this session never + # asked for -- the same shape as a result frame carrying another session's + # command id. + ghost = session.issue_unregistered('sys.ping', {}, deadline_ms: 5000, max_bytes: 0) + a.call('the session does not know the ghost command id', !session.known_command?(ghost)) + + deadline = Time.now + 10 + sleep 0.1 while broker.discarded_results.size == before && Time.now < deadline + discarded = broker.discarded_results.last + a.call('the unmatched result was discarded, not dispatched by id alone', + broker.discarded_results.size > before && discarded && discarded[:command_id] == ghost, + discarded.inspect) + a.call('matching is per session, not global', + session.deliver({ 'type' => 'result', 'id' => SecureRandom.uuid, 'status' => 'ok' }) == false) + + # And the session still works: a discarded frame is not a fatal condition. + a.call('the session survives an unmatched result', broker.call('sys.ping')['status'] == 'ok') +end + +# =========================================================================== +# R1: the studio's network drops. The connector reconnects with no operator +# action on the studio side. +# =========================================================================== +drill('R1', 'network drop: the connector reconnects unaided') do |a| + a.call('a session is up before the drop', broker.online?) + t0 = Time.now + broker.partition(3) + a.call('the session went down with the network', broker.wait_for_offline(10)) + sleep 3.2 + session = broker.wait_for_session(45) + a.call('the connector reconnected without operator action', !session.nil?, + format('%.1fs to recover', Time.now - t0)) + a.call('the reconnected session works', session && broker.call('sys.ping', session: session)['status'] == 'ok') + a.call('the audit log recorded the reconnect schedule', + audit_lines.any? { |e| e['event'] == 'reconnect_scheduled' }) +end + +# =========================================================================== +# R3: our side is stopped and restarted. (Run before R2 so the connector is +# still alive.) The connector backs off, then reconnects. +# =========================================================================== +drill('R3', 'broker stopped and restarted: backoff then reconnect') do |a| + broker.stop + a.call('the session went down when the broker stopped', broker.wait_for_offline(10)) + sleep 2 + broker.start + s = broker.wait_for_session(60) + a.call('the connector reconnected to the restarted broker', !s.nil?) + a.call('the reconnected session serves verbs', s && broker.call('sys.ping', session: s)['status'] == 'ok') + + backoffs = audit_lines.select { |e| e['event'] == 'reconnect_scheduled' } + a.call('backoff was scheduled and logged, never an escalation', + backoffs.size >= 2 && backoffs.none? { |e| e['status'] == 'error' }, + "#{backoffs.size} reconnect_scheduled lines") +end + +# =========================================================================== +# R4: the token is revoked on our side. The socket closes within one heartbeat, +# and the studio's own credentials are untouched. +# =========================================================================== +drill('R4', 'token revoked: socket closes within one heartbeat, studio credentials untouched') do |a| + a.call('a session is up before the revoke', broker.wait_for_session(20)) + t0 = Time.now + closed = broker.revoke(TOKEN) + a.call('the broker closed the live session', closed.positive?) + a.call('the socket was gone within one heartbeat', + broker.wait_for_offline(MockBroker::HEARTBEAT_INTERVAL + 2), + format('%.2fs', Time.now - t0)) + + # Reconnect attempts are now refused, and refused the same way every time. + sleep 4 + a.call('reconnects with the revoked token are refused', + broker.refusals.any? { |r| r[:reason] == 'revoked_token' }, + "#{broker.refusals.count { |r| r[:reason] == 'revoked_token' }} refusals") + a.call('no session came back up', !broker.online?) + + # The studio side is untouched: connector.yml is byte-identical, still 0600, + # and no tool credential ever crossed the socket. + a.call('connector.yml is unchanged and still 0600', + [File.read(CONFIG_PATH), File.stat(CONFIG_PATH).mode] == CONFIG_FINGERPRINT) + wire = broker.received_frames.join("\n") + a.call('the TeamCity token never crossed the socket', !wire.include?(TEAMCITY_TOKEN)) + a.call('the P4 ticket never crossed the socket', !wire.include?(P4_TICKET)) + a.call('the connector token never appeared in a frame body', !wire.include?(TOKEN)) + a.call('no LAN host, port, or URL appeared in an inbound frame', + !wire.include?(teamcity.base_url) && !wire.include?('ssl:p4.lan:1666')) +end + +# =========================================================================== +# R2: the connector is stopped. We go offline and every verb-dependent feature +# renders the degraded state instead of raising. +# =========================================================================== +drill('R2', 'connector stopped: we flip to offline and degrade, not error') do |a| + # Re-issue a token so there is a live session to stop. + token2 = new_token + broker.register_token(token2, integration_id: 'intg7f3a') + cfg2 = File.join(WORK, 'connector-2.yml') + File.write(cfg2, File.read(CONFIG_PATH).sub(TOKEN, token2)) + File.chmod(0o600, cfg2) + env2 = { 'FAKE_P4_ARGV_LOG' => ARGV_LOG, 'PATH' => ENV.fetch('PATH', '/usr/bin:/bin'), + 'HOME' => ENV.fetch('HOME', '/tmp') } + pid2 = Process.spawn(env2, CONNECTOR_BIN, '-config', cfg2, unsetenv_others: true, + out: [CONNECTOR_STDERR, 'a'], err: [CONNECTOR_STDERR, 'a']) + s = broker.wait_for_session(30) + a.call('the second connector connected', !s.nil?) + a.call('it serves verbs while up', s && broker.call('sys.ping', session: s)['status'] == 'ok') + + t0 = Time.now + stop_connector(pid2, signal: 'TERM') + a.call('stopping the connector takes us offline', + broker.wait_for_offline(10), format('%.2fs', Time.now - t0)) + + # This is the kill-switch UX: a verb-dependent feature records "needs + # connector" and moves on rather than raising. + degraded = broker.degraded_call('teamcity.build.get') + a.call('a verb-dependent feature renders the degraded state, not an error', + degraded['status'] == 'needs_connector', degraded.inspect) + a.call('the connector shut down cleanly and logged it', + audit_lines.any? { |e| e['event'] == 'shutdown' }) +end + +# --------------------------------------------------------------------------- +# Report +# --------------------------------------------------------------------------- +stop_connector(PID) +broker.stop +teamcity.stop + +puts +puts '=' * 78 +puts 'butterstack-connector drills (issue #1575 group 1) -- design note section 4.3' +puts '=' * 78 +RESULTS.each do |r| + puts format('%-10s %-4s %-62s %s', r.name, r.ok ? 'ok' : 'FAIL', r.title, r.evidence) +end +puts '-' * 78 +puts 'Round-trip latency (local loopback; the design targets under 2s over a home connection):' +LATENCIES.sort.each { |verb, secs| puts format(' %-24s %6.1f ms', verb, secs * 1000) } +puts '-' * 78 +failed = RESULTS.reject(&:ok) +if failed.empty? + puts "All #{RESULTS.size} drills passed." +else + puts "#{failed.size} of #{RESULTS.size} drills FAILED:" + failed.each do |r| + puts " #{r.name} #{r.title}" + r.detail.select { |d| d.start_with?('FAIL') }.each { |d| puts " #{d}" } + end +end +puts '=' * 78 +exit(failed.empty? ? 0 : 1) diff --git a/test/mock_broker.rb b/test/mock_broker.rb new file mode 100644 index 0000000..d92c631 --- /dev/null +++ b/test/mock_broker.rb @@ -0,0 +1,402 @@ +# frozen_string_literal: true + +# A minimal stand-in for the ButterStack side of the Connector protocol. +# +# This is NOT the broker. The real broker is a dedicated Rack endpoint at +# /connect on the Rails app, with hashed-token auth, a Redis-routed command and +# result path, and explicit tenant scoping -- none of which exists yet and none +# of which this file models. What this models is exactly the surface the seven +# drills need: the /connect upgrade, the header-only token check, the frame +# exchange, and the ability to revoke a token or drop a connection on demand. +# +# It does implement, faithfully, the four rules the drills exist to prove: +# +# 1. The upgrade is refused before any per-connection state is allocated when +# the token is absent, malformed, revoked, or wrong. +# 2. A token supplied as a query parameter is refused, and there is no code +# path anywhere in this file that reads a token from a query string. +# 3. Tokens are stored as the SHA-256 digest of the secret segment and are +# compared in constant time. The plaintext token is never held. +# 4. A result frame is matched against the issuing session's own outstanding +# commands; a result carrying another session's command id is discarded. +require 'json' +require 'openssl' +require 'time' +require 'securerandom' +require 'socket' +require_relative 'support/ws' +require_relative 'support/tls' + +class MockBroker + HEARTBEAT_INTERVAL = 5 # seconds; the connector accepts 5..120, so this is negotiated for real + + # Session is one live connector connection. + class Session + attr_reader :id, :integration_id, :peer, :hello + attr_accessor :last_seen + + def initialize(id:, integration_id:, peer:, hello:) + @id = id + @integration_id = integration_id + @peer = peer + @hello = hello + @last_seen = Time.now + @outstanding = {} + @mutex = Mutex.new + @cond = ConditionVariable.new + end + + def issue(verb, args, deadline_ms:, max_bytes:, command_id: nil) + id = command_id || SecureRandom.uuid + @mutex.synchronize { @outstanding[id] = nil } + frame = { + type: 'command', id: id, verb: verb, args: args, + deadline_ms: deadline_ms, max_bytes: max_bytes + } + @peer.send_text(JSON.generate(frame)) + id + end + + def await(id, timeout) + deadline = Time.now + timeout + @mutex.synchronize do + loop do + v = @outstanding[id] + return v if v + + remaining = deadline - Time.now + return nil if remaining <= 0 + + @cond.wait(@mutex, remaining) + end + end + end + + # issue_unregistered sends a command frame WITHOUT recording its id, so the + # connector's answer arrives as a result this session never asked for. That + # is exactly the shape of a result frame belonging to another session, and + # it drives the broker's real discard path rather than a simulated one. + def issue_unregistered(verb, args, deadline_ms:, max_bytes:) + id = SecureRandom.uuid + @peer.send_text(JSON.generate({ + type: 'command', id: id, verb: verb, args: args, + deadline_ms: deadline_ms, max_bytes: max_bytes + })) + id + end + + # deliver returns true when the result belonged to this session. + def deliver(result) + @mutex.synchronize do + return false unless @outstanding.key?(result['id']) + + @outstanding[result['id']] = result + @cond.broadcast + true + end + end + + def known_command?(id) + @mutex.synchronize { @outstanding.key?(id) } + end + end + + attr_reader :port, :refusals, :received_frames, :discarded_results + + def initialize(host: 'localhost', tls_dir:, logger: nil) + @host = host + @tls = MockTLS.generate(tls_dir, host) + @logger = logger + @tokens = {} # digest => { integration_id:, revoked: } + @sessions = {} # session id => Session + @mutex = Mutex.new + @refusals = [] # every refused upgrade, with a stable reason + @received_frames = [] # raw inbound frame text, for the egress assertions + @discarded_results = [] # cross-session result frames + @partition_until = nil + @port = nil + @running = false + end + + # register_token stores only the SHA-256 digest of the secret segment, which + # is the pattern the design specifies and the opposite of the nearest + # precedent in the app (Integration#webhook_token is stored reversibly). + def register_token(token, integration_id:) + @mutex.synchronize do + @tokens[digest_of(token)] = { integration_id: integration_id, revoked: false } + end + token + end + + # revoke marks the token dead and closes its live sessions, which is the + # server half of "revoking on our side closes the socket within one heartbeat; + # the studio's credentials are untouched". + def revoke(token) + d = digest_of(token) + victims = [] + @mutex.synchronize do + @tokens[d][:revoked] = true if @tokens.key?(d) + victims = @sessions.values.select { |s| s.integration_id == integration_for(d) } + victims.each { |s| @sessions.delete(s.id) } + end + victims.each do |s| + s.peer.send_close(4401, 'token revoked') + s.peer.close + end + victims.size + end + + def start + @server = TCPServer.new('127.0.0.1', @port || 0) + @port = @server.addr[1] + ctx = OpenSSL::SSL::SSLContext.new + ctx.cert = @tls.server_cert + ctx.key = @tls.server_key + ctx.min_version = OpenSSL::SSL::TLS1_2_VERSION + @ssl = OpenSSL::SSL::SSLServer.new(@server, ctx) + @ssl.start_immediately = false + @running = true + @accept_thread = Thread.new { accept_loop } + self + end + + # stop closes the listener so new connections are refused at TCP level. This + # is the "staging stopped" drill; the connector must back off, not die. + def stop + @running = false + begin + @ssl&.close + rescue StandardError + nil + end + @accept_thread&.kill + @accept_thread = nil + close_all_sessions + self + end + + # partition simulates the home network dropping: live sockets die and new + # connections are dropped mid-handshake for the duration. + def partition(seconds) + @mutex.synchronize { @partition_until = Time.now + seconds } + close_all_sessions + end + + def ca_pem_path + @tls.ca_pem_path + end + + def endpoint(path = '/connect') + "wss://#{@host}:#{@port}#{path}" + end + + def sessions + @mutex.synchronize { @sessions.values.dup } + end + + def online? + !sessions.empty? + end + + # wait_for_session blocks until a connector is connected, or times out. + def wait_for_session(timeout) + deadline = Time.now + timeout + loop do + s = sessions.first + return s if s + return nil if Time.now > deadline + + sleep 0.05 + end + end + + def wait_for_offline(timeout) + deadline = Time.now + timeout + loop do + return true if sessions.empty? + return false if Time.now > deadline + + sleep 0.05 + end + end + + # call issues one command to the connected session and waits for the result. + # This is the shape Connector.call(integration, verb, args) will have; the + # difference on the real side is that it wraps in an explicit tenant scope and + # routes through Redis, neither of which is modelled here. + def call(verb, args = {}, timeout: 10, deadline_ms: 8000, max_bytes: 0, session: nil) + s = session || sessions.first + raise 'no connector session; the integration is offline' unless s + + id = s.issue(verb, args, deadline_ms: deadline_ms, max_bytes: max_bytes) + res = s.await(id, timeout) + raise "timed out waiting for result of #{verb}" unless res + + res + end + + # degraded_call is what a verb-dependent feature does when the connector is + # offline: it records "needs connector" and moves on, rather than raising. + def degraded_call(verb) + return { 'status' => 'needs_connector', 'verb' => verb } if sessions.empty? + + call(verb) + end + + private + + def digest_of(token) + secret = token.to_s.split('_', 3)[2].to_s + OpenSSL::Digest::SHA256.hexdigest(secret) + end + + def integration_for(dig) + @tokens.dig(dig, :integration_id) + end + + def secure_equal?(a, b) + return false unless a.bytesize == b.bytesize + + OpenSSL.fixed_length_secure_compare(a, b) + end + + def close_all_sessions + victims = @mutex.synchronize do + v = @sessions.values + @sessions = {} + v + end + victims.each { |s| s.peer.close } + end + + def log(msg) + @logger&.call("[broker] #{msg}") + end + + def accept_loop + while @running + begin + raw = @ssl.accept + rescue StandardError + break unless @running + + next + end + Thread.new(raw) { |sock| handle(sock) } + end + end + + def handle(raw_sock) + partitioned = @mutex.synchronize { @partition_until && Time.now < @partition_until } + if partitioned + begin + raw_sock.close + rescue StandardError + nil + end + return + end + + begin + raw_sock.accept # TLS handshake + rescue StandardError + return + end + + peer = MockWS::Peer.new(raw_sock) + begin + req = peer.read_request(Time.now + 10) + rescue StandardError + peer.close + return + end + + # ---- Refusals, all of which happen before any session state exists ------- + return refuse(peer, req, 404, 'not_found') unless req.path == '/connect' + + # A query string on the upgrade is refused outright. A connector is not a + # browser and can set headers, so the only reason to put a credential in a + # URL is a limitation that does not apply here -- and a URL is logged by + # every proxy on the path. + if req.query && !req.query.empty? + reason = req.query.include?('token') ? 'query_string_token' : 'query_string' + return refuse(peer, req, 400, reason) + end + return refuse(peer, req, 426, 'not_an_upgrade') unless req.header('upgrade').to_s.downcase == 'websocket' + + key = req.header('sec-websocket-key') + return refuse(peer, req, 400, 'missing_key') if key.nil? || key.empty? + + token = req.bearer_token + return refuse(peer, req, 401, 'missing_bearer_token') if token.nil? || token.empty? + return refuse(peer, req, 401, 'malformed_token') unless token.match?(/\Absc_[a-z0-9-]+_[A-Za-z2-7]+\z/) + + dig = digest_of(token) + record = @mutex.synchronize { @tokens.find { |k, _| secure_equal?(k, dig) }&.last } + return refuse(peer, req, 401, 'unknown_token') if record.nil? + return refuse(peer, req, 401, 'revoked_token') if record[:revoked] + + peer.accept_upgrade(key) + serve(peer, record[:integration_id]) + rescue StandardError => e + log("handler error: #{e.class}: #{e.message}") + peer&.close + end + + def refuse(peer, req, status, reason) + @mutex.synchronize { @refusals << { reason: reason, status: status, target: req&.target, at: Time.now } } + log("refused #{reason} (#{status}) target=#{req&.target}") + peer.respond_and_close(status, reason) + nil + end + + def serve(peer, integration_id) + hello_raw = peer.read_message(15) + record_frame(hello_raw) + hello = JSON.parse(hello_raw) + raise 'expected hello' unless hello['type'] == 'hello' + + session = Session.new(id: SecureRandom.uuid, integration_id: integration_id, peer: peer, hello: hello) + @mutex.synchronize { @sessions[session.id] = session } + log("session #{session.id} up: #{hello['connector_id']} v#{hello['version']} verbs=#{hello['capabilities']&.size}") + + peer.send_text(JSON.generate({ + type: 'welcome', session_id: session.id, + server_time: Time.now.utc.iso8601, + min_supported_version: '0', + heartbeat_interval: HEARTBEAT_INTERVAL + })) + + loop do + raw = peer.read_message(HEARTBEAT_INTERVAL * 6) + record_frame(raw) + frame = begin + JSON.parse(raw) + rescue JSON::ParserError + next + end + case frame['type'] + when 'heartbeat' + session.last_seen = Time.now + when 'result' + session.last_seen = Time.now + # A result is matched against the issuing session's own outstanding + # commands. A frame carrying a command id this session never received + # is discarded, never dispatched by id alone. + next if session.deliver(frame) + + @mutex.synchronize { @discarded_results << { session_id: session.id, command_id: frame['id'] } } + log("discarded result for unknown command id #{frame['id']} on session #{session.id}") + end + end + rescue MockWS::Closed, MockWS::Timeout, JSON::ParserError, StandardError => e + log("session down: #{e.class}: #{e.message}") + ensure + @mutex.synchronize { @sessions.delete(session.id) } if session + peer.close + end + + def record_frame(raw) + @mutex.synchronize { @received_frames << raw.to_s } + end +end diff --git a/test/support/fake_p4 b/test/support/fake_p4 new file mode 100755 index 0000000..ba72106 --- /dev/null +++ b/test/support/fake_p4 @@ -0,0 +1,60 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# A stand-in for the p4 CLI, emitting the -Mj -ztag JSON-per-record output the +# connector's Perforce executor parses. +# +# It also records its own argv, verbatim, to $FAKE_P4_ARGV_LOG. That log is what +# the no-shell drill asserts on: a depot path containing shell metacharacters +# must arrive as exactly one argv element with its bytes unchanged, which is +# only true if the connector execve's an argv array instead of building a +# command line. +require 'json' + +if (log = ENV['FAKE_P4_ARGV_LOG']) + File.open(log, 'a') { |f| f.puts(JSON.generate(ARGV)) } +end + +# Strip the fixed connection flags the connector always passes. +args = ARGV.dup +until args.empty? + case args.first + when '-p', '-u' then args.shift(2) + when '-Mj', '-ztag' then args.shift + else break + end +end + +command = args.shift + +case command +when 'describe' + args.shift if args.first == '-s' + change = args.shift.to_i + if change == 41_337 + rec = { + 'change' => change.to_s, 'user' => 'buildbot', 'client' => 'buildbot-ws', + 'time' => '1756512000', 'status' => 'submitted', + 'desc' => "Bump the UAT map\n", + 'depotFile0' => '//depot/game/Content/Maps/Main.umap', 'action0' => 'edit', + 'type0' => 'binary+l', 'rev0' => '7', + 'depotFile1' => '//depot/game/Config/DefaultEngine.ini', 'action1' => 'edit', + 'type1' => 'text', 'rev1' => '3' + } + puts JSON.generate(rec) + else + warn "#{change} - no such changelist." + exit 1 + end +when 'changes' + args.shift(2) if args.first == '-m' + # args.first is now the depot path; it is echoed back so the drill can prove + # the exact bytes survived the argv array. + puts JSON.generate({ 'change' => '41337', 'user' => 'buildbot', 'time' => '1756512000', + 'desc' => 'Bump the UAT map' }) + puts JSON.generate({ 'change' => '41336', 'user' => 'artist', 'time' => '1756425600', + 'desc' => 'Rebake lighting' }) +else + warn "unsupported p4 command: #{command}" + exit 1 +end diff --git a/test/support/teamcity_stub.rb b/test/support/teamcity_stub.rb new file mode 100644 index 0000000..091da08 --- /dev/null +++ b/test/support/teamcity_stub.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +# A tiny stand-in for an on-prem TeamCity server, on the studio's LAN side of +# the connector. +# +# It exists to prove one thing the drills care about: the Bearer token the +# connector presents to TeamCity comes from connector.yml and never crosses the +# broker socket. The stub refuses any request without the exact token the config +# file holds, so a connector that had somehow lost local custody would fail the +# round-trip drills rather than quietly passing them. +require 'json' +require 'socket' + +class TeamCityStub + attr_reader :port, :requests + + def initialize(token:, logger: nil) + @token = token + @logger = logger + @requests = [] + @mutex = Mutex.new + @builds = {} + end + + def add_build(id, build_type_id:, number:, status: 'SUCCESS', state: 'finished', revision: nil) + @mutex.synchronize do + @builds[id.to_i] = { + 'id' => id.to_i, 'buildTypeId' => build_type_id, 'number' => number, + 'status' => status, 'state' => state, 'statusText' => 'Tests passed', + 'branchName' => 'refs/heads/main', + 'webUrl' => "http://teamcity.invalid/viewLog.html?buildId=#{id}", + 'queuedDate' => '20260830T100000+0000', + 'startDate' => '20260830T100005+0000', + 'finishDate' => '20260830T100205+0000', + 'revisions' => { 'revision' => [{ 'version' => revision || '41337', 'vcsBranchName' => '//depot/game/main' }] } + } + end + end + + def start + @server = TCPServer.new('127.0.0.1', 0) + @port = @server.addr[1] + @running = true + @thread = Thread.new { accept_loop } + self + end + + def stop + @running = false + begin + @server&.close + rescue StandardError + nil + end + @thread&.kill + end + + def base_url + "http://127.0.0.1:#{@port}" + end + + private + + def accept_loop + while @running + begin + sock = @server.accept + rescue StandardError + break + end + Thread.new(sock) { |s| serve(s) } + end + end + + def serve(sock) + head = +'' + head << sock.readpartial(1) until head.end_with?("\r\n\r\n") + lines = head.split("\r\n") + _method, target, = lines.shift.split(' ') + headers = lines.each_with_object({}) do |l, h| + k, v = l.split(':', 2) + h[k.to_s.strip.downcase] = v.to_s.strip if k && v + end + path, query = target.split('?', 2) + @mutex.synchronize { @requests << { path: path, query: query, authorization: headers['authorization'] } } + + return respond(sock, 401, { 'error' => 'unauthorized' }) unless headers['authorization'] == "Bearer #{@token}" + + case path + when '/app/rest/server' + respond(sock, 200, { + 'version' => '2025.03 (build 189123)', 'versionMajor' => 2025, 'versionMinor' => 3, + 'buildNumber' => '189123', 'webUrl' => 'http://teamcity.invalid' + }) + when %r{\A/app/rest/builds/id:(\d+)\z} + build = @mutex.synchronize { @builds[Regexp.last_match(1).to_i] } + build ? respond(sock, 200, build) : respond(sock, 404, { 'error' => 'not found' }) + else + respond(sock, 404, { 'error' => 'not found' }) + end + rescue StandardError => e + @logger&.call("[teamcity-stub] #{e.class}: #{e.message}") + ensure + begin + sock.close + rescue StandardError + nil + end + end + + def respond(sock, status, body) + json = JSON.generate(body) + sock.write("HTTP/1.1 #{status} #{status == 200 ? 'OK' : 'Error'}\r\n" \ + "Content-Type: application/json\r\n" \ + "Content-Length: #{json.bytesize}\r\n" \ + "Connection: close\r\n\r\n#{json}") + end +end diff --git a/test/support/tls.rb b/test/support/tls.rb new file mode 100644 index 0000000..cad156c --- /dev/null +++ b/test/support/tls.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +# Generates a throwaway CA and server certificate for the drill harness, so the +# connector dials a real wss:// endpoint with real certificate verification. +# +# The connector has no option to skip verification for the broker connection. +# That is the point of doing this rather than testing over plaintext: if the +# daemon could be talked into an unverified broker connection, the drills would +# not be exercising the transport the design promises. +require 'openssl' +require 'fileutils' + +module MockTLS + Bundle = Struct.new(:ca_pem_path, :server_cert, :server_key, keyword_init: true) + + def self.generate(dir, hostname = 'localhost') + FileUtils.mkdir_p(dir) + + ca_key = OpenSSL::PKey::RSA.new(2048) + ca_cert = OpenSSL::X509::Certificate.new + ca_cert.version = 2 + ca_cert.serial = 1 + ca_cert.subject = OpenSSL::X509::Name.parse('/CN=butterstack-connector-drill-ca') + ca_cert.issuer = ca_cert.subject + ca_cert.public_key = ca_key.public_key + ca_cert.not_before = Time.now - 3600 + ca_cert.not_after = Time.now + 86_400 + ef = OpenSSL::X509::ExtensionFactory.new + ef.subject_certificate = ca_cert + ef.issuer_certificate = ca_cert + ca_cert.add_extension(ef.create_extension('basicConstraints', 'CA:TRUE', true)) + ca_cert.add_extension(ef.create_extension('keyUsage', 'keyCertSign,cRLSign', true)) + ca_cert.sign(ca_key, OpenSSL::Digest::SHA256.new) + + key = OpenSSL::PKey::RSA.new(2048) + cert = OpenSSL::X509::Certificate.new + cert.version = 2 + cert.serial = 2 + cert.subject = OpenSSL::X509::Name.parse("/CN=#{hostname}") + cert.issuer = ca_cert.subject + cert.public_key = key.public_key + cert.not_before = Time.now - 3600 + cert.not_after = Time.now + 86_400 + ef2 = OpenSSL::X509::ExtensionFactory.new + ef2.subject_certificate = cert + ef2.issuer_certificate = ca_cert + cert.add_extension(ef2.create_extension('basicConstraints', 'CA:FALSE', true)) + cert.add_extension(ef2.create_extension('keyUsage', 'digitalSignature,keyEncipherment', true)) + cert.add_extension(ef2.create_extension('extendedKeyUsage', 'serverAuth')) + cert.add_extension(ef2.create_extension('subjectAltName', "DNS:#{hostname},IP:127.0.0.1")) + cert.sign(ca_key, OpenSSL::Digest::SHA256.new) + + ca_path = File.join(dir, 'drill-ca.pem') + File.write(ca_path, ca_cert.to_pem) + File.chmod(0o600, ca_path) + + Bundle.new(ca_pem_path: ca_path, server_cert: cert, server_key: key) + end +end diff --git a/test/support/ws.rb b/test/support/ws.rb new file mode 100644 index 0000000..ee96d8e --- /dev/null +++ b/test/support/ws.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +# Minimal server-side RFC 6455 implementation for the drill harness. +# +# This is deliberately an independent implementation from the Go client in +# internal/wsclient: the two were written from the RFC rather than from each +# other, so a handshake or masking mistake on either side shows up as a failed +# drill instead of two halves of the same bug agreeing with one another. +# +# Scope: text frames, ping/pong, close, client-side masking. No extensions, no +# compression, no fragmentation on the server's send path. +require 'openssl' +require 'base64' +require 'socket' + +module MockWS + GUID = '258EAFA5-E914-47DA-95CA-5AB0DC85B11C' + + MAX_MESSAGE_BYTES = 1 << 20 + MAX_CONTROL_BYTES = 125 + + OP_CONTINUATION = 0x0 + OP_TEXT = 0x1 + OP_BINARY = 0x2 + OP_CLOSE = 0x8 + OP_PING = 0x9 + OP_PONG = 0xA + + Closed = Class.new(StandardError) + Timeout = Class.new(StandardError) + + # accept_key computes base64(sha1(key + GUID)). + def self.accept_key(client_key) + Base64.strict_encode64(OpenSSL::Digest::SHA1.digest(client_key.to_s + GUID)) + end + + # Request is a parsed HTTP upgrade request. + Request = Struct.new(:method, :target, :path, :query, :headers, keyword_init: true) do + def header(name) + headers[name.downcase] + end + + # bearer_token returns the token from the Authorization header, or nil. + # There is no fallback to a query parameter anywhere in this file; the + # broker's rejection of a query-string token depends on there being no + # second place to look. + def bearer_token + v = header('authorization').to_s + return nil unless v.start_with?('Bearer ') + + v.delete_prefix('Bearer ').strip + end + end + + # Peer wraps one accepted socket. + class Peer + attr_reader :sock + + def initialize(sock) + @sock = sock + @buf = +'' + @write_mutex = Mutex.new + end + + # read_request parses the HTTP request line and headers. + def read_request(deadline) + raw = +'' + raw << read_exact(1, deadline) until raw.end_with?("\r\n\r\n") + head, = raw.split("\r\n\r\n", 2) + lines = head.split("\r\n") + method, target, = lines.shift.to_s.split(' ') + path, query = target.to_s.split('?', 2) + headers = {} + lines.each do |l| + k, v = l.split(':', 2) + next if k.nil? || v.nil? + + headers[k.strip.downcase] = v.strip + end + Request.new(method: method, target: target, path: path, query: query, headers: headers) + end + + def write_raw(str) + @write_mutex.synchronize { @sock.write(str) } + end + + # respond_and_close writes a plain HTTP response. Used for every refusal, so + # a refused upgrade never reaches the frame layer. + def respond_and_close(status, body = '') + text = { + 400 => 'Bad Request', 401 => 'Unauthorized', 403 => 'Forbidden', + 426 => 'Upgrade Required', 429 => 'Too Many Requests' + }.fetch(status, 'Error') + write_raw("HTTP/1.1 #{status} #{text}\r\n" \ + "Content-Type: text/plain\r\n" \ + "Content-Length: #{body.bytesize}\r\n" \ + "Connection: close\r\n\r\n#{body}") + close + rescue StandardError + nil + end + + def accept_upgrade(key) + write_raw("HTTP/1.1 101 Switching Protocols\r\n" \ + "Upgrade: websocket\r\n" \ + "Connection: Upgrade\r\n" \ + "Sec-WebSocket-Accept: #{MockWS.accept_key(key)}\r\n\r\n") + end + + def send_text(str) + write_frame(OP_TEXT, str.to_s.dup.force_encoding(Encoding::BINARY)) + end + + def send_ping(payload = '') + write_frame(OP_PING, payload) + end + + def send_close(code = 1000, reason = '') + payload = [code].pack('n') + reason.to_s + payload = payload.byteslice(0, MAX_CONTROL_BYTES) + write_frame(OP_CLOSE, payload) + rescue StandardError + nil + end + + # read_message reassembles one text message, answering pings inline. + # Returns the payload string, or raises Closed / Timeout. + def read_message(timeout) + deadline = Time.now + timeout + msg = +'' + loop do + fin, opcode, payload = read_frame(deadline) + case opcode + when OP_PING then write_frame(OP_PONG, payload) + when OP_PONG then nil + when OP_CLOSE then raise Closed, 'client sent close' + when OP_BINARY then raise Closed, 'binary frame' + when OP_TEXT, OP_CONTINUATION + raise Closed, 'message too large' if msg.bytesize + payload.bytesize > MAX_MESSAGE_BYTES + + msg << payload + return msg.force_encoding(Encoding::UTF_8) if fin + else + raise Closed, "unknown opcode #{opcode}" + end + end + end + + def close + @sock.close + rescue StandardError + nil + end + + def closed? + @sock.closed? + end + + private + + def write_frame(opcode, payload) + payload = payload.to_s.dup.force_encoding(Encoding::BINARY) + n = payload.bytesize + header = +'' + header << (0x80 | opcode).chr + # A server never masks (RFC 6455 5.1). + if n <= 125 + header << n.chr + elsif n <= 0xFFFF + header << 126.chr << [n].pack('n') + else + header << 127.chr << [n].pack('Q>') + end + write_raw(header + payload) + end + + def read_frame(deadline) + h = read_exact(2, deadline).bytes + fin = (h[0] & 0x80) != 0 + raise Closed, 'reserved bits set' if (h[0] & 0x70) != 0 + + opcode = h[0] & 0x0F + masked = (h[1] & 0x80) != 0 + len = h[1] & 0x7F + len = read_exact(2, deadline).unpack1('n') if len == 126 + len = read_exact(8, deadline).unpack1('Q>') if len == 127 + + control = (opcode & 0x8) != 0 + raise Closed, 'fragmented control frame' if control && !fin + raise Closed, 'oversized control frame' if control && len > MAX_CONTROL_BYTES + raise Closed, 'frame too large' if len > MAX_MESSAGE_BYTES + # RFC 6455 6.1: a client MUST mask. An unmasked client frame is a + # protocol error, and asserting it here is a real check on the Go side. + raise Closed, 'client frame was not masked' unless masked + + mask = read_exact(4, deadline).bytes + payload = read_exact(len, deadline) + unmasked = +'' + payload.bytes.each_with_index { |b, i| unmasked << (b ^ mask[i % 4]).chr } + [fin, opcode, unmasked.force_encoding(Encoding::BINARY)] + end + + def read_exact(n, deadline) + return +'' if n.zero? + + while @buf.bytesize < n + remaining = deadline - Time.now + raise Timeout, 'read timed out' if remaining <= 0 + + begin + @buf << @sock.read_nonblock([n - @buf.bytesize, 16_384].max) + rescue IO::WaitReadable, OpenSSL::SSL::SSLErrorWaitReadable + raise Timeout, 'read timed out' unless IO.select([@sock.to_io], nil, nil, remaining) + + retry + rescue IO::WaitWritable, OpenSSL::SSL::SSLErrorWaitWritable + raise Timeout, 'write-wait timed out' unless IO.select(nil, [@sock.to_io], nil, remaining) + + retry + rescue EOFError, Errno::ECONNRESET, IOError, OpenSSL::SSL::SSLError + raise Closed, 'socket closed' + end + end + out = @buf.byteslice(0, n) + @buf = @buf.byteslice(n, @buf.bytesize - n) || +'' + out + end + end +end From 3ab9c62d054986cd9115281614b75ff9475e29cf Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Sun, 30 Aug 2026 14:39:08 -0400 Subject: [PATCH 02/11] fix(connector): depot_scope boundary, error-frame egress, ping/pong liveness Review follow-ups to the PR #1578 spike (issue #1575). - depot_scope was a bare prefix match, so a scope of //depot/game admitted //depot/gamesecret/... . config.Validate now normalizes every entry to a trailing slash and vocab.withinPrefixList matches only on a path-segment boundary (the scope root itself, or "prefix/"). Tests for both. - A failed tool call sent err.Error() to the broker inside the result frame; p4 stderr and Go url.Error carry the studio's LAN host, port and URL. The frame now carries the stable reason tool_error and the detail goes only to the local audit log. PROTOCOL.md sections 3 and 7 updated. - A healthy idle connector reconnected every ~10s: the read loop treated two silent heartbeat intervals as a dead socket, but the protocol has no broker-to-connector traffic while idle. The heartbeat tick now also sends a WebSocket ping, Conn tracks the last inbound frame of any kind (pong included), and a read deadline only fails the session when nothing at all arrived in the window. Surfaced by the UAT suite; drills never ran long enough to see it. - Em dashes replaced in lines this PR added. Upstream: PR #1578. Refs https://github.com/ButterStack/butter_stack/issues/1575 Co-Authored-By: Claude Fable 5 (cherry picked from commit 3728e673cf88d18d1337148cda4a4cda3bf84ca8) --- PROTOCOL.md | 28 ++++++++++++----- internal/config/config.go | 9 +++++- internal/config/config_test.go | 30 ++++++++++++++++++ internal/session/session.go | 37 ++++++++++++++++++++-- internal/vocab/vocab.go | 13 ++++++-- internal/vocab/vocab_test.go | 50 ++++++++++++++++++++++++++++++ internal/wsclient/wsclient.go | 37 ++++++++++++++++++++-- internal/wsclient/wsclient_test.go | 46 +++++++++++++++++++++++++++ test/README.md | 4 +-- 9 files changed, 236 insertions(+), 18 deletions(-) diff --git a/PROTOCOL.md b/PROTOCOL.md index 3317411..b9b4693 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -49,7 +49,7 @@ schema, because the tempting shortcut is real: - `config/environments/production.rb:56` restricts `allowed_request_origins` to the two `RAILS_HOST` origins. A non-browser client sends no `Origin`, so admitting one through ActionCable would mean widening that list or disabling - forgery protection — either of which weakens a live cross-site + forgery protection, either of which weakens a live cross-site WebSocket-hijacking control that protects real users' browser sockets. - That connection carries `identified_by :current_user, :current_account`, `impersonates :user`, and sets `ActsAsTenant.current_tenant`. A machine client @@ -125,7 +125,7 @@ revoked token. An unknown verb must likewise never reveal what is configured generic `req/ip` 300-per-5-minutes at `config/initializers/rack_attack.rb:112-115`. That generic budget is shared with a studio's webhook traffic arriving from the same NAT address, and rack-attack -sees only the upgrade, never the frames — so per-session command budgets and the +sees only the upgrade, never the frames, so per-session command budgets and the per-integration connection cap (v0: 2) belong in the broker, not in rack-attack. --- @@ -148,9 +148,17 @@ binary frame type and no streaming. // result -- answers exactly one command, keyed by its id. { "type": "result", "id": "", "status": "ok|error|denied|timeout", - "reason": "", "body": {}, "truncated": false, "bytes": 1234 } + "reason": "", "body": {}, "truncated": false, "bytes": 1234 } ``` +`reason` is always a stable machine token, on every status that carries one, +never free text. For `status: "error"` (a tool call that reached the tool and +failed), `reason` is the fixed token `tool_error`: the real error text -- p4 +stderr with a server host:port, a Go `*url.Error` with the TeamCity URL, any +of it -- carries detail about the studio's LAN and never leaves it. That text +is written only to the connector's local audit log (see §7); the broker never +receives it. + ### Broker → connector ```jsonc @@ -239,7 +247,7 @@ Both are drilled separately for that reason (Shuri §6 item 7b). This is the finding that made this layer day-1 work rather than v1 polish (Shuri F4). `allowed_jobs` constrains *which* job runs; a `params` map is unconstrained, and Jenkins build parameters and TeamCity properties are -interpolated into shell build steps by design — including in our own +interpolated into shell build steps by design, including in our own `Jenkinsfile.unreal` and `Jenkinsfile.minimobile` templates. A caller-supplied parameter bag would therefore turn the typed allowlist into a code-execution primitive on the studio's build agents, and falsify the single sentence the @@ -255,12 +263,12 @@ So: composes a **fixed** request body, `{"buildType":{"id":""}}`. No `properties`, no `branchName`, no `comment`. - Neither verb is compiled into v0 at all. -- A v1 may add per-job `allowed_params` in `connector.yml` — an allowlist of +- A v1 may add per-job `allowed_params` in `connector.yml` - an allowlist of parameter *names*, each with a value pattern or enum, enforced connector-side before the call. This is enforced structurally, not by convention: `bannedArgNames` in -`vocab.go` lists the argument names no verb may declare — compiled or reserved — +`vocab.go` lists the argument names no verb may declare - compiled or reserved - each with the reason it is banned, and `Selfcheck()` runs both in the test suite and at process start. A build whose vocabulary grew one of them refuses to run. @@ -308,7 +316,7 @@ Every credential the connector uses comes from `connector.yml`, or from a - no environment-variable fallback for any credential; - no command-line flag that takes a secret; -- no remote configuration — the broker cannot tell the connector where to find a +- no remote configuration - the broker cannot tell the connector where to find a credential. If the broker could, "your credentials never leave your network" would depend on @@ -351,6 +359,10 @@ Arguments are hashed rather than recorded verbatim, so the log correlates with our side ("we sent command X, they ran command X") without becoming a second copy of whatever the arguments contained. Denial detail strings stay local and never travel in a `result` frame: a denial message that echoed the rejected -value back would be a small egress channel of its own. +value back would be a small egress channel of its own. The same rule applies +to a failed tool call: the `result` frame's `reason` is the stable token +`tool_error` only, and the real error text -- which can carry LAN detail like +a p4 server host:port or a TeamCity URL -- is written to the local audit +line's `detail` field and never leaves the studio. The log is the studio's evidence, not ours. No verb can read it. diff --git a/internal/config/config.go b/internal/config/config.go index 9328148..21b09f8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -252,13 +252,20 @@ func (c *Config) Validate() error { return errors.New("config: perforce.enabled needs at least one " + "scopes.depot_scope entry; an unscoped depot verb is denied anyway") } - for _, p := range c.Scopes.DepotScope { + for i, p := range c.Scopes.DepotScope { if !strings.HasPrefix(p, "//") { return fmt.Errorf("config: depot_scope %q must begin //", p) } if strings.ContainsAny(p, "*") || strings.Contains(p, "...") { return fmt.Errorf("config: depot_scope %q must be a literal prefix, not a wildcard", p) } + // Normalize to a trailing slash so vocab.withinPrefixList can match on + // a real path-segment boundary: without this, a scope of "//depot/game" + // (no trailing slash) would admit "//depot/gamesecret/..." via a bare + // strings.HasPrefix, because "gamesecret" also starts with "game". + if !strings.HasSuffix(p, "/") { + c.Scopes.DepotScope[i] = p + "/" + } } } if c.TeamCity.Enabled { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 69cee69..1af4f2b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -148,6 +148,36 @@ func TestPerforceNeedsADepotScope(t *testing.T) { } } +// TestDepotScopeIsNormalizedWithTrailingSlash guards the segment-boundary +// fix: vocab.withinPrefixList only matches a scope entry that ends with "/", +// so an entry configured without one (e.g. "//depot/game") must be +// normalized here at load time, or every depot verb against that scope +// would silently deny everything. +func TestDepotScopeIsNormalizedWithTrailingSlash(t *testing.T) { + p := writeCfg(t, base(`scopes: + depot_scope: + - //depot/game + - //depot/other/ +perforce: + enabled: true + port: ssl:p4.lan:1666 + user: butterstack-ro +`), 0o600) + c, err := Load(p) + if err != nil { + t.Fatalf("load: %v", err) + } + want := []string{"//depot/game/", "//depot/other/"} + if len(c.Scopes.DepotScope) != len(want) { + t.Fatalf("depot_scope = %v, want %v", c.Scopes.DepotScope, want) + } + for i, w := range want { + if c.Scopes.DepotScope[i] != w { + t.Errorf("depot_scope[%d] = %q, want %q", i, c.Scopes.DepotScope[i], w) + } + } +} + func TestWildcardDepotScopeIsRefused(t *testing.T) { p := writeCfg(t, base(`scopes: depot_scope: diff --git a/internal/session/session.go b/internal/session/session.go index 7e2c445..3115c1b 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -45,6 +45,13 @@ const ( maxDeadline = 60 * time.Second defaultDeadline = 15 * time.Second + + // reasonToolError is the stable machine token sent to the broker when a + // tool call fails. The real error text (p4 stderr with a server + // host:port, a Go *url.Error with the TeamCity URL, etc.) carries LAN + // detail and never leaves the studio: it is written to the local audit + // log's Detail field only, never into the result frame's `reason`. + reasonToolError = "tool_error" ) // Runner owns one connector process's connection lifecycle. @@ -240,6 +247,15 @@ func (r *Runner) pump(ctx context.Context, conn *wsclient.Conn, heartbeat time.D fail(err) return } + // A WS-layer ping too, independent of the application-level + // heartbeat frame above: the broker answers a ping with a pong + // automatically at the transport layer even if it never sends + // its own traffic, which is what gives the read loop something + // to see besides "nothing" during a quiet, healthy session. + if err := conn.WritePing(); err != nil { + fail(err) + return + } } } }() @@ -248,10 +264,21 @@ func (r *Runner) pump(ctx context.Context, conn *wsclient.Conn, heartbeat time.D go func() { defer wg.Done() for { - // If the broker goes quiet for longer than a couple of heartbeat - // intervals the socket is dead even if TCP has not noticed. + // The protocol has no broker-to-connector traffic while idle: the + // broker only ever answers a command. Without a liveness signal, + // this deadline alone would fire against a perfectly healthy, + // quiet broker every ~heartbeat*readSlack, which is why the + // heartbeat goroutine also sends a WS-layer ping: the broker's + // automatic pong is the "something happened" this check needs. A + // deadline-exceeded error is therefore reconnect-worthy only when + // NO inbound frame of any kind (pong included, via + // conn.LastActivity) arrived within that same window; otherwise + // it is just an idle, alive socket and the loop keeps waiting. raw, err := conn.ReadMessage(time.Now().Add(heartbeat * readSlack)) if err != nil { + if wsclient.IsTimeout(err) && time.Since(conn.LastActivity()) < heartbeat*readSlack { + continue + } fail(err) return } @@ -365,7 +392,11 @@ func (r *Runner) handle(ctx context.Context, cmd protocol.Command) protocol.Resu Status: protocol.StatusTimeout, Reason: vocab.ReasonDeadlineExceeded} return finish(res, "") case err != nil: - return finish(protocol.Errorf(cmd.ID, "%s", err.Error()), "") + // The broker gets only the stable reason token; the actual error text + // (which can carry LAN detail: p4 stderr with a server host:port, a Go + // *url.Error with the TeamCity URL) is passed to finish's `detail` + // argument, which lands in the local audit log only. + return finish(protocol.Errorf(cmd.ID, "%s", reasonToolError), err.Error()) } return finish(protocol.OK(cmd.ID, body, n, truncated), "") } diff --git a/internal/vocab/vocab.go b/internal/vocab/vocab.go index 07d408c..1ff5178 100644 --- a/internal/vocab/vocab.go +++ b/internal/vocab/vocab.go @@ -547,12 +547,21 @@ func literalPrefix(s string) string { return s } +// withinPrefixList reports whether s falls inside one of the configured depot +// scopes, matching only on a real path-segment boundary. A scope entry is +// expected to end with "/" (config.Validate normalizes every configured +// entry this way); an entry without a trailing slash is skipped rather than +// matched with a bare strings.HasPrefix, because that would let a scope of +// "//depot/game" wrongly admit "//depot/gamesecret/...": "gamesecret" also +// starts with the literal string "game". With the trailing slash required, s +// matches only when it is exactly the scope without its trailing slash (the +// scope root itself) or has the full "prefix/" as a genuine path prefix. func withinPrefixList(s string, prefixes []string) bool { for _, p := range prefixes { - if p == "" { + if !strings.HasSuffix(p, "/") { continue } - if strings.HasPrefix(s, p) { + if s == strings.TrimSuffix(p, "/") || strings.HasPrefix(s, p) { return true } } diff --git a/internal/vocab/vocab_test.go b/internal/vocab/vocab_test.go index b6919c4..1a2755d 100644 --- a/internal/vocab/vocab_test.go +++ b/internal/vocab/vocab_test.go @@ -179,6 +179,56 @@ func TestInScopePathIsAllowed(t *testing.T) { } } +// TestDepotScopeRespectsSegmentBoundary is the regression guard for the +// bare-strings.HasPrefix bug: a scope of "//depot/game/" must not admit +// "//depot/gamesecret/...", because "gamesecret" also starts with the +// literal string "game". Only a real path-segment boundary (the scope root +// itself, or the scope as a genuine "prefix/" match) counts as in-scope. +func TestDepotScopeRespectsSegmentBoundary(t *testing.T) { + cases := []struct { + path string + allowed bool + }{ + {`//depot/gamesecret/...`, false}, // segment-boundary bypass: denied + {`//depot/game/...`, true}, // genuine child of the scope: allowed + {`//depot/game`, true}, // the scope root itself, exact: allowed + } + for _, c := range cases { + _, _, derr := resolve(t, "p4.changes", `{"path":"`+c.path+`"}`) + if c.allowed && derr != nil { + t.Errorf("%s: want allowed, got deny %v", c.path, derr) + } + if !c.allowed && (derr == nil || derr.Reason != ReasonOutOfScopePath) { + t.Errorf("%s: want %s, got %v", c.path, ReasonOutOfScopePath, derr) + } + } +} + +// TestWithinPrefixList unit-tests the matcher directly: a scope entry without +// a trailing slash is skipped rather than matched with a bare prefix check. +func TestWithinPrefixList(t *testing.T) { + scopes := []string{"//depot/game/"} + cases := map[string]bool{ + "//depot/gamesecret/...": false, + "//depot/game/...": true, + "//depot/game": true, + "//depot/gam": false, + } + for path, want := range cases { + if got := withinPrefixList(literalPrefix(path), scopes); got != want { + t.Errorf("withinPrefixList(%q) = %v, want %v", path, got, want) + } + } + + // An un-normalized scope entry (no trailing slash) is skipped entirely: + // config.Validate is what normalizes entries, and this function must not + // silently fall back to the unsafe bare-prefix match if it somehow sees + // one that wasn't. + if withinPrefixList("//depot/game", []string{"//depot/game"}) { + t.Fatalf("an un-normalized scope entry (no trailing slash) must not match") + } +} + // TestContentToggleCannotBeTurnedOnByACaller pins include_diff to false at the // schema level, so no broker frame and no config value can flip it in v0. func TestContentToggleCannotBeTurnedOnByACaller(t *testing.T) { diff --git a/internal/wsclient/wsclient.go b/internal/wsclient/wsclient.go index 5b2110e..1daad3e 100644 --- a/internal/wsclient/wsclient.go +++ b/internal/wsclient/wsclient.go @@ -32,6 +32,7 @@ import ( "os" "strings" "sync" + "sync/atomic" "time" ) @@ -93,6 +94,26 @@ type Conn struct { writeMu sync.Mutex closed bool closeMu sync.Mutex + + // lastActivity is the unix-nanos timestamp of the most recently received + // frame of any kind, pong included. It is written from the single + // goroutine that calls ReadMessage and read from any goroutine (the + // session's idle-reconnect check), hence atomic rather than a plain field. + lastActivity atomic.Int64 +} + +// LastActivity returns the time of the most recently received frame of any +// kind, including a pong answering our own ping. The protocol has no +// broker-to-connector traffic while idle apart from that pong, so this is +// what lets a read-deadline timeout distinguish "quiet but alive" from +// "actually dead" instead of treating every idle period as a dead socket. +// Zero until the first frame arrives. +func (c *Conn) LastActivity() time.Time { + ns := c.lastActivity.Load() + if ns == 0 { + return time.Time{} + } + return time.Unix(0, ns) } // Dial performs the TLS and WebSocket handshakes. @@ -203,7 +224,18 @@ func Dial(opts Options) (*Conn, error) { } _ = raw.SetDeadline(time.Time{}) - return &Conn{raw: raw, br: br}, nil + c := &Conn{raw: raw, br: br} + c.lastActivity.Store(time.Now().UnixNano()) + return c, nil +} + +// IsTimeout reports whether err is a read/write deadline expiring, as opposed +// to a genuine transport failure (connection reset, EOF, protocol error). +// The session's idle-reconnect check uses this to decide whether a +// ReadMessage deadline is cause to reconnect or just an idle broker. +func IsTimeout(err error) bool { + var ne net.Error + return errors.As(err, &ne) && ne.Timeout() } // AcceptKey computes the RFC 6455 Sec-WebSocket-Accept value. Exported so the @@ -292,13 +324,14 @@ func (c *Conn) ReadMessage(deadline time.Time) ([]byte, error) { if err != nil { return nil, err } + c.lastActivity.Store(time.Now().UnixNano()) switch opcode { case opPing: if err := c.writeFrame(opPong, payload); err != nil { return nil, err } case opPong: - // liveness only + // liveness only; never surfaced as a message to the caller. case opClose: _ = c.writeFrame(opClose, payload) return nil, ErrClosedByServer diff --git a/internal/wsclient/wsclient_test.go b/internal/wsclient/wsclient_test.go index 6c82654..272bfbf 100644 --- a/internal/wsclient/wsclient_test.go +++ b/internal/wsclient/wsclient_test.go @@ -1,8 +1,11 @@ package wsclient import ( + "bufio" "errors" + "net" "testing" + "time" ) // TestAcceptKey pins base64(sha1(key + RFC 6455 GUID)) for the RFC's sample @@ -34,3 +37,46 @@ func TestDialRefusesPlaintextSchemes(t *testing.T) { } } } + +// TestPongUpdatesLastActivity is the regression guard for the idle-reconnect +// fix: a pong is "silent" (ReadMessage never returns it as a message, per the +// opPong case in the read loop), but it must still count as activity, or the +// session's idle check would treat every quiet-but-alive period as dead. Built +// directly against a Conn over a net.Pipe rather than through Dial, since Dial +// requires a full HTTP upgrade handshake this test has no need for. +func TestPongUpdatesLastActivity(t *testing.T) { + serverSide, clientSide := net.Pipe() + defer serverSide.Close() + defer clientSide.Close() + + c := &Conn{raw: clientSide, br: bufio.NewReader(clientSide)} + if !c.LastActivity().IsZero() { + t.Fatalf("LastActivity should be zero before any frame arrives") + } + + // A minimal, unmasked pong frame (servers must not mask, per RFC 6455): + // FIN+opcode byte 0x8A, then a zero-length payload byte 0x00. + go func() { + _, _ = serverSide.Write([]byte{0x8A, 0x00}) + }() + + before := time.Now() + // The pong never completes a message, so ReadMessage blocks until its + // deadline; that timeout is expected here; what matters is LastActivity + // having moved in the meantime. + _, err := c.ReadMessage(time.Now().Add(200 * time.Millisecond)) + if err == nil { + t.Fatalf("expected a deadline timeout (only a pong was sent, no message)") + } + if !IsTimeout(err) { + t.Fatalf("expected a timeout error, got %v", err) + } + + got := c.LastActivity() + if got.IsZero() { + t.Fatalf("LastActivity was not updated by the pong") + } + if got.Before(before) { + t.Fatalf("LastActivity = %v, should be at or after %v", got, before) + } +} diff --git a/test/README.md b/test/README.md index 4f490e1..cab3a5d 100644 --- a/test/README.md +++ b/test/README.md @@ -32,7 +32,7 @@ Requires Ruby 3.2+ (stdlib only) and a built connector binary. Takes about It is not the broker. The real one is a dedicated Rack endpoint at `/connect` on the Rails app with hashed-token auth, a Redis-routed command and result path, -and explicit tenant scoping — none of which exists yet and none of which this +and explicit tenant scoping - none of which exists yet and none of which this models. It does implement faithfully the four rules the drills exist to prove: 1. the upgrade is refused **before any per-connection state is allocated** when @@ -40,6 +40,6 @@ models. It does implement faithfully the four rules the drills exist to prove: 2. a query-string token is refused, and no code path here reads a token from a query string; 3. tokens are stored as the SHA-256 digest of the secret segment and compared in - constant time — the plaintext is never held; + constant time - the plaintext is never held; 4. a `result` frame is matched against the issuing session's own outstanding commands; one carrying another session's command id is discarded. From 15d61901566358815aee2e90f1a5b1fc397ab4e0 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Sun, 30 Aug 2026 14:39:08 -0400 Subject: [PATCH 03/11] test(uat): uat:connector suite against an isolated studio LAN New `npm run test:uat:connector` (tests/uat/connector.spec.js) covering TeamCity Tier 1 webhook intake and the ButterStack Connector spike, the automated form of Devin's home-lab plan (design note section 4). Topology (docker-compose.uat-connector.yml, profile `connector`): a `studio_lan` network with internal: true holds a TeamCity stub and the connector; the only ways out are the connector's own outbound wss:// leg to a mock broker and the stub's webhook POSTs through a socat egress forwarder, the model of a studio firewall that allows outbound only. Phase 100 proves the shape: Rails cannot resolve or reach the stub, the connector publishes no port and holds no listening socket, and the connector can reach the stub. Phases: TeamCity jenkins-type integration with ci_provider teamcity; curl-step webhook (X-Webhook-Token) lands a BuildRun with the p4- commit hash, the teamcity label and the stamped integration; the native {eventType, payload} envelope with php-auth-pw authenticates and is recorded but creates no BuildRun (dropped until #1574 Phase 1; the assertion flips then); wrong or missing credential 401s; the connector session comes up with the compiled verb list; sys/teamcity/p4 verbs round trip (fake_p4 argv proves no shell); every denial reason (unknown verb, out-of-scope path, reserved verb, string-for-int, content toggle, unknown argument); query-string and missing-token upgrades are refused; a cross-session result is discarded; stop/start, revoke and re-register degrade and recover. Also: connector/Dockerfile (multi-stage, non-root, no EXPOSE) with a UAT-only entrypoint that renders connector.yml from env under umask 077 (the daemon itself still reads only the file); mock_broker_server.rb with a small admin API; teamcity_stub.rb runnable standalone with an outbound fire-webhook endpoint; a connector-go CI job (vet, test, build); README UAT section listing what the suite does not prove (no Rails /connect broker yet, no real TeamCity or p4d). Upstream: PR #1578 (or its own test PR). Refs https://github.com/ButterStack/butter_stack/issues/1575 Refs https://github.com/ButterStack/butter_stack/issues/1574 Co-Authored-By: Claude Fable 5 (cherry picked from commit 1d970c9e2c5846a56e6d43f4f0080f99e445da06) --- Dockerfile | 47 ++++++++ README.md | 123 ++++++++++++++++++-- test/mock_broker.rb | 11 +- test/mock_broker_server.rb | 205 ++++++++++++++++++++++++++++++++++ test/support/fake_p4 | 16 ++- test/support/teamcity_stub.rb | 141 ++++++++++++++++++++++- test/uat/entrypoint.sh | 73 ++++++++++++ 7 files changed, 597 insertions(+), 19 deletions(-) create mode 100644 Dockerfile create mode 100644 test/mock_broker_server.rb create mode 100755 test/uat/entrypoint.sh diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 0000000..b007250 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,47 @@ +# Multi-stage build for the butterstack-connector UAT/drill image +# (connector.spec.js, issue #1574/#1575). +# +# The daemon itself is a static Go binary with no runtime dependencies. The +# final stage is Ruby-based anyway because the UAT "studio LAN" stands the +# real `p4` CLI up with test/support/fake_p4 -- a Ruby script bind-mounted at +# /usr/local/bin/p4 by docker-compose.uat-connector.yml -- and the connector +# execs whatever binary connector.yml names, argv-only, no shell. +# +# NOT hardened for production (issue #1575 survival conditions 1/4/5: no +# Sigstore keyless signing, no SBOM, no digest-pinned base image, no +# reproducible-build docs -- see connector/README.md "what this does not +# prove"). This image exists for the UAT drill environment only. + +FROM golang:1.25-alpine AS build +WORKDIR /src +COPY go.mod go.sum ./ +RUN go mod download +COPY . . +# -trimpath drops build-machine file paths from the binary; -s -w strips the +# symbol table and DWARF debug info. CGO_ENABLED=0 keeps the binary static, so +# the runtime stage needs no libc compatibility shim. +RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o /out/butterstack-connector ./cmd/butterstack-connector + +FROM ruby:3.3-alpine + +RUN apk add --no-cache ca-certificates \ + && addgroup -g 10001 connector \ + && adduser -D -u 10001 -G connector -h /home/connector -s /sbin/nologin connector \ + && mkdir -p /etc/butterstack /var/log/connector /tls \ + && chown -R connector:connector /etc/butterstack /var/log/connector /tls /home/connector + +COPY --from=build /out/butterstack-connector /usr/local/bin/butterstack-connector +COPY test/uat/entrypoint.sh /usr/local/bin/uat-entrypoint.sh +RUN chmod 0755 /usr/local/bin/butterstack-connector /usr/local/bin/uat-entrypoint.sh + +# No EXPOSE: the connector never listens on anything. It opens exactly one +# outbound TLS connection and never accepts an inbound one. + +USER connector:connector +WORKDIR /home/connector + +# Production entrypoint: reads connector.yml from the path a studio wrote. +# The UAT compose service overrides this with uat-entrypoint.sh, which +# renders that file from UAT_CONNECTOR_* env vars first (see that script's +# header for why that is a UAT-only pattern, not a protocol exception). +ENTRYPOINT ["/usr/local/bin/butterstack-connector", "-config", "/etc/butterstack/connector.yml"] diff --git a/README.md b/README.md index 21594de..28e3f5a 100644 --- a/README.md +++ b/README.md @@ -6,17 +6,17 @@ single inbound port**. It opens exactly one outbound TLS connection to one hostname on 443, announces what it can do, and then executes only commands from a typed, versioned -allowlist with constrained arguments — each one logged locally. It holds the +allowlist with constrained arguments, each one logged locally. It holds the studio's tool credentials in its own config file and never sends them. This is the **spike** from issue #1575, checkbox groups 0 and 1: a standalone proof of the daemon and the protocol schema. Nothing here is deployed, and nothing here touches the Rails app. -- [`PROTOCOL.md`](PROTOCOL.md) — the day-1 protocol schema (issue #1575 group 0) -- [`internal/vocab/vocab.go`](internal/vocab/vocab.go) — the whole allowlist, in +- [`PROTOCOL.md`](PROTOCOL.md) - the day-1 protocol schema (issue #1575 group 0) +- [`internal/vocab/vocab.go`](internal/vocab/vocab.go) - the whole allowlist, in one readable file, on purpose -- [`test/`](test/) — the mock broker and the seven drills +- [`test/`](test/) - the mock broker and the seven drills Design sources, on branch `plan/teamcity-private-reach`: `ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md` @@ -57,7 +57,7 @@ enforces rather than documents: Every credential comes from that file, or from a `*_file` path it names. There is no environment-variable fallback, no flag that takes a secret, and no remote -configuration — the broker cannot tell the connector where to find a credential. +configuration: the broker cannot tell the connector where to find a credential. ## What is in the vocabulary @@ -72,7 +72,7 @@ configuration — the broker cannot tell the connector where to find a credentia | `p4.file_contents`, `ghes.commit.get`, `horde.server.info` | reserved, denied | No verb accepts a host, port, URL, or shell string. No verb accepts -caller-supplied build parameters or properties — that is enforced structurally +caller-supplied build parameters or properties, that is enforced structurally (`bannedArgNames` plus `Selfcheck()`, which runs at process start as well as in the tests), because a parameter map on a build-triggering verb interpolates into shell build steps and would make the allowlist a code-execution primitive inside @@ -100,7 +100,7 @@ drill (f). Every drill passes today: The mock broker is not the broker. The real one is a dedicated Rack endpoint at `/connect` on the Rails app with hashed-token auth, Redis-routed command and -result, and explicit tenant scoping — a later PR. What `test/mock_broker.rb` +result, and explicit tenant scoping - a later PR. What `test/mock_broker.rb` models is the surface these drills need, and it does implement faithfully the four rules they exist to prove: refusal before session-state allocation, header-only tokens, SHA-256 digest storage with constant-time compare, and @@ -117,9 +117,9 @@ standalone shape adds: frame boundary against a mock broker. They do not prove it against a real broker, a real TeamCity, or a real p4d. - **Anything on the Rails side.** There is no `/connect` endpoint in this PR, no - ActionCable change, no migration, no UI. The tenant-context drill — "assert + ActionCable change, no migration, no UI. The tenant-context drill - "assert tenant context is nil at the start of a request that follows a connector frame - on the same Puma thread" — is Rails-side and is **not** covered here. Only the + on the same Puma thread" - is Rails-side and is **not** covered here. Only the broker-side half of drill (f) is. - **Anything on real infrastructure.** Nothing ran against staging, demo, or production. No terraform, no security group, no hostname, no certificate. @@ -127,7 +127,7 @@ standalone shape adds: the #1574 Phase -1 app fixes landing first; the "no token in `webhook_events.payload` or the app log" drill therefore has no result yet. - **The frame codec against an independent production stack.** Both ends here - were written from RFC 6455 — the Go client and the Ruby server independently, + were written from RFC 6455 - the Go client and the Ruby server independently, which is why a masking or handshake mistake shows up as a failed drill. But neither has met a real ALB, a real nginx `Upgrade` hop, or a real proxy. - **Latency over a home connection.** The drills run on loopback. The design's @@ -149,3 +149,106 @@ standalone shape adds: This is the go/no-go input for the build, and it is deliberately smaller than the product. + +--- + +## UAT + +`tests/uat/connector.spec.js` (repo root) is a second, Docker-based test of +this same daemon: TeamCity Tier 1 webhook intake into the real Rails app, and +the compiled verbs, denials, and degradation drills against a containerized +version of this connector, run inside a Docker-modelled "studio LAN" that is +NOT reachable from the cloud side (outbound allowed, inbound blocked), which +is the shape issue #1574/#1575 actually sell. + +```bash +docker-compose --profile core --profile connector up -d --build connector +npm run test:uat:connector +npm run test:uat:connector:keep-data # leaves the signup/project for inspection +``` + +**Always scope `--build` to `connector`.** `web`/`sidekiq` still carry a +`build:` key in the base `docker-compose.yml` even though +`docker-compose.uat-connector.yml` pins them to the prebuilt +`image: butter_stack-web:latest`; an unscoped `--build` rebuilds *and retags* +that shared image, which every worktree's dev stack uses. + +### Topology + +``` + "cloud" side (network: default) "studio LAN" (network: studio_lan, + internal: true -- no route out) + +-----------+ +--------------+ +----------------+ + | web | | mock-broker |<===wss:9443====>| connector | + | (Rails) | | (drill stand-| | (the daemon | + +-----+-----+ | in for the | | under test) | + ^ | real /connect| +--------+-------+ + | | endpoint) | | + | http :3000| | | (LAN-local) + +-----+---------+ +--------------+ | + | studio-egress |<===================studio_lan==============+ + | (socat, models| ^ + | outbound-only| +--------+---------+ + | firewall) | | teamcity-stub | + +---------------+ | (fake on-prem CI, | + | no published port)| + +--------------------+ +``` + +`internal: true` on `studio_lan` is the whole isolation guarantee. `web` +cannot resolve or reach `teamcity-stub`; `connector` publishes no port at all +(`docker inspect` shows an empty port map, and `netstat -tln` inside it lists +nothing but Docker's own embedded DNS resolver); `teamcity-stub`'s only way to +reach Rails is the one TCP port `studio-egress` forwards, which is the studio +firewall's "outbound allowed" in miniature. `connector.spec.js` phase 100 +asserts this shape directly before doing anything else. + +### What this proves + +- The TeamCity Tier 1 webhook path both ways Teddy's design note describes: + the curl-step flat payload (`X-Webhook-Token`) creates a `BuildRun` with + `ci_provider == 'teamcity'`; TeamCity's own built-in webhook envelope + (`php-auth-user`/`php-auth-pw`) authenticates and records a `WebhookEvent` + but does **not** yet create a `BuildRun` - there is no adapter for the + `{eventType, payload}` shape in `jenkins_controller.rb` (issue #1574 Phase + 1, `normalize_ci_payload`). The spec documents this as the real current + behavior; the assertion is written to flip the day that adapter lands. +- Neither webhook token nor its HTTP headers ever land in a persisted + `WebhookEvent`. +- The connector's compiled vocabulary, denials (unknown verb, out-of-scope + path, reserved-but-not-compiled verb, wrong argument type, a disabled + content toggle, an unknown argument), and the no-shell proof on the p4 argv + log, all against a real containerized daemon rather than the in-process + drill harness. +- Broker-side refusals (query-string token, missing bearer token) before any + session state exists, cross-session result discard, and the degradation + drills (connector stop/start, token revoke/re-register) - the same four + rules `test/drills.rb` proves, exercised over the network instead of a + Ruby method call. + +### What this UAT suite does NOT prove + +Everything `test/README.md`'s own "what this spike does not prove" section +already says, plus: + +- **There is still no real `/connect` Rack endpoint.** `mock-broker` here is + the same drill stand-in as `test/mock_broker.rb`, containerized + (`test/mock_broker_server.rb`) with a small HTTP admin API bolted on so the + Playwright test can drive sessions/refusals/revoke/partition without + speaking the wire protocol itself. It is not, and does not claim to be, the + real broker. +- **No real TeamCity.** `teamcity-stub` (`test/support/teamcity_stub.rb`) is a + hand-rolled fake that answers exactly the REST calls the connector makes and + can fire the two outbound webhook shapes a real TeamCity delivers. It has + never seen a real TeamCity server's actual quirks. +- **No real p4d.** `test/support/fake_p4` stands in for the `p4` CLI, same as + in the drills. +- **Idle-connector liveness** is a WS-layer ping/pong rule: the heartbeat + goroutine sends a WebSocket ping alongside the application-level heartbeat + frame, and the read loop's deadline only means "reconnect" when no inbound + frame of any kind (a pong included) arrived within heartbeat x readSlack, + so a quiet-but-healthy broker no longer cycles the session offline at rest. +- Everything else `test/README.md` already lists: real infrastructure, a + production-hardened image (no Sigstore signing, no SBOM, no digest-pinned + base - see `connector/Dockerfile`'s header comment), scale, and any verb + beyond the five compiled ones. diff --git a/test/mock_broker.rb b/test/mock_broker.rb index d92c631..0b49173 100644 --- a/test/mock_broker.rb +++ b/test/mock_broker.rb @@ -103,7 +103,11 @@ def known_command?(id) attr_reader :port, :refusals, :received_frames, :discarded_results - def initialize(host: 'localhost', tls_dir:, logger: nil) + # bind/port let a container-hosted broker (mock_broker_server.rb) listen on + # a fixed, published address instead of loopback + an ephemeral port. The + # defaults are unchanged so drills.rb, which relies on '127.0.0.1'/`nil` + # port picking a free one, needs no changes at all. + def initialize(host: 'localhost', tls_dir:, logger: nil, bind: '127.0.0.1', port: nil) @host = host @tls = MockTLS.generate(tls_dir, host) @logger = logger @@ -114,7 +118,8 @@ def initialize(host: 'localhost', tls_dir:, logger: nil) @received_frames = [] # raw inbound frame text, for the egress assertions @discarded_results = [] # cross-session result frames @partition_until = nil - @port = nil + @bind = bind + @port = port @running = false end @@ -147,7 +152,7 @@ def revoke(token) end def start - @server = TCPServer.new('127.0.0.1', @port || 0) + @server = TCPServer.new(@bind, @port || 0) @port = @server.addr[1] ctx = OpenSSL::SSL::SSLContext.new ctx.cert = @tls.server_cert diff --git a/test/mock_broker_server.rb b/test/mock_broker_server.rb new file mode 100644 index 0000000..c958d79 --- /dev/null +++ b/test/mock_broker_server.rb @@ -0,0 +1,205 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# Standalone process wrapping MockBroker for container use in +# docker-compose.uat-connector.yml (connector.spec.js, issue #1574/#1575 UAT). +# +# `mock_broker.rb` is NOT the broker -- see its header comment and +# `test/README.md` "What the mock broker is not". The real broker is a +# dedicated Rack endpoint at /connect on the Rails app, with hashed-token +# auth, a Redis-routed command/result path, and explicit tenant scoping. What +# runs here is the same drill stand-in `drills.rb` uses, given two things a +# container needs that a same-process test harness does not: +# +# 1. a fixed, published wss:// listener (rather than loopback + an ephemeral +# port), with a server certificate whose SAN matches the compose hostname +# the connector container dials; and +# 2. a small HTTP admin API, PLAIN HTTP (not TLS, not the /connect protocol +# itself), so the Playwright test on the host can drive sessions/refusals/ +# revoke/partition without speaking the wire protocol. This is exactly +# the same "test harness talks to the broker object directly" shape +# drills.rb uses in-process -- just reachable over the network instead of +# via a Ruby method call. It is reachable only on the compose default +# network and the host-published admin port; it is not part of the +# Connector protocol and a real broker exposes nothing like it. +# +# Env: +# BROKER_TOKEN required: the connector token to register +# BROKER_INTEGRATION_ID required: the integration id segment of that token +# BROKER_HOST hostname baked into the server cert's SAN +# (default 'mock-broker' -- the compose service name +# the connector dials) +# BROKER_BIND wss:// listen address (default '0.0.0.0') +# BROKER_PORT wss:// listen port (default 9443) +# BROKER_ADMIN_BIND admin HTTP listen address (default '0.0.0.0') +# BROKER_ADMIN_PORT admin HTTP listen port (default 9400) +# BROKER_TLS_DIR where the throwaway CA/cert are generated and +# where ca.pem is published for other containers to +# trust (default '/tls', a shared volume) +require 'json' +require 'socket' +require 'securerandom' +require 'fileutils' +require_relative 'mock_broker' + +TOKEN = ENV.fetch('BROKER_TOKEN') +INTEGRATION_ID = ENV.fetch('BROKER_INTEGRATION_ID') +HOST = ENV['BROKER_HOST'] || 'mock-broker' +BIND = ENV['BROKER_BIND'] || '0.0.0.0' +PORT = (ENV['BROKER_PORT'] || '9443').to_i +ADMIN_BIND = ENV['BROKER_ADMIN_BIND'] || '0.0.0.0' +ADMIN_PORT = (ENV['BROKER_ADMIN_PORT'] || '9400').to_i +TLS_DIR = ENV['BROKER_TLS_DIR'] || '/tls' + +LOGGER = ->(m) { warn(m) } + +FileUtils.mkdir_p(TLS_DIR) +BROKER = MockBroker.new(host: HOST, tls_dir: TLS_DIR, logger: LOGGER, bind: BIND, port: PORT) +BROKER.register_token(TOKEN, integration_id: INTEGRATION_ID) +BROKER.start +warn("[mock-broker] wss listening on #{BIND}:#{BROKER.port}, cert CN/SAN=#{HOST}, integration=#{INTEGRATION_ID}") + +# Publish the CA cert on the shared /tls volume so the connector container (and +# anything else that wants to verify this broker) can trust it. Not 0600: this +# is a public certificate, not a secret, and other containers need to read it. +CA_PEM_PATH = File.join(TLS_DIR, 'ca.pem') +FileUtils.cp(BROKER.ca_pem_path, CA_PEM_PATH) +File.chmod(0o644, CA_PEM_PATH) +warn("[mock-broker] CA published at #{CA_PEM_PATH}") + +# --------------------------------------------------------------------------- +# Admin HTTP API -- plain HTTP, hand-rolled in the style of teamcity_stub.rb. +# No gems: this is a drill/UAT fixture, not a shipped service. +# --------------------------------------------------------------------------- +def respond_json(sock, status, body) + json = JSON.generate(body) + text = { 200 => 'OK', 400 => 'Bad Request', 404 => 'Not Found', 502 => 'Bad Gateway' }.fetch(status, 'Error') + sock.write("HTTP/1.1 #{status} #{text}\r\n" \ + "Content-Type: application/json\r\n" \ + "Content-Length: #{json.bytesize}\r\n" \ + "Connection: close\r\n\r\n#{json}") +end + +def respond_text(sock, status, body, content_type: 'text/plain') + text = { 200 => 'OK', 404 => 'Not Found' }.fetch(status, 'Error') + sock.write("HTTP/1.1 #{status} #{text}\r\n" \ + "Content-Type: #{content_type}\r\n" \ + "Content-Length: #{body.bytesize}\r\n" \ + "Connection: close\r\n\r\n#{body}") +end + +def session_json(s) + { + 'id' => s.id, + 'integration_id' => s.integration_id, + 'connector_id' => s.hello['connector_id'], + 'version' => s.hello['version'], + 'capabilities' => s.hello['capabilities'], + 'tool_versions' => s.hello['tool_versions'], + 'last_seen' => s.last_seen.utc.iso8601 + } +end + +def handle_admin(sock) + head = +'' + head << sock.readpartial(1) until head.end_with?("\r\n\r\n") + lines = head.split("\r\n") + method, target, = lines.shift.split(' ') + headers = lines.each_with_object({}) do |l, h| + k, v = l.split(':', 2) + h[k.to_s.strip.downcase] = v.to_s.strip if k && v + end + body = +'' + if (len = headers['content-length'].to_i) > 0 + body << sock.read(len).to_s + end + path = target.split('?', 2).first + params = body.empty? ? {} : JSON.parse(body) + + case [method, path] + when ['GET', '/health'] + respond_json(sock, 200, { 'ok' => true }) + + when ['GET', '/ca.pem'] + respond_text(sock, 200, File.read(CA_PEM_PATH), content_type: 'application/x-pem-file') + + when ['GET', '/status'] + sessions = BROKER.sessions + respond_json(sock, 200, { + 'online' => BROKER.online?, + 'sessions' => sessions.map { |s| session_json(s) }, + 'refusals' => BROKER.refusals.map { |r| r.merge(at: r[:at].utc.iso8601) }, + 'discarded_results_count' => BROKER.discarded_results.size, + 'discarded_results' => BROKER.discarded_results + }) + + when ['POST', '/call'] + verb = params.fetch('verb') + args = params['args'] || {} + deadline_ms = params['deadline_ms'] || 8000 + max_bytes = params['max_bytes'] || 0 + begin + result = BROKER.call(verb, args, deadline_ms: deadline_ms, max_bytes: max_bytes) + respond_json(sock, 200, result) + rescue StandardError => e + respond_json(sock, 200, { 'error' => e.message }) + end + + when ['POST', '/degraded_call'] + verb = params.fetch('verb') + respond_json(sock, 200, BROKER.degraded_call(verb)) + + when ['POST', '/issue_unregistered'] + session = BROKER.sessions.first + if session + id = session.issue_unregistered(params.fetch('verb'), params['args'] || {}, deadline_ms: 5000, max_bytes: 0) + respond_json(sock, 200, { 'ok' => true, 'id' => id }) + else + respond_json(sock, 200, { 'error' => 'no connector session' }) + end + + when ['POST', '/revoke'] + closed = BROKER.revoke(TOKEN) + respond_json(sock, 200, { 'ok' => true, 'closed' => closed }) + + when ['POST', '/register'] + BROKER.register_token(TOKEN, integration_id: INTEGRATION_ID) + respond_json(sock, 200, { 'ok' => true }) + + when ['POST', '/partition'] + seconds = (params['seconds'] || 5).to_i + BROKER.partition(seconds) + respond_json(sock, 200, { 'ok' => true, 'seconds' => seconds }) + + else + respond_json(sock, 404, { 'error' => "no such admin route #{method} #{path}" }) + end +rescue KeyError, JSON::ParserError => e + respond_json(sock, 400, { 'error' => e.message }) +rescue StandardError => e + respond_json(sock, 502, { 'error' => "#{e.class}: #{e.message}" }) +ensure + begin + sock.close + rescue StandardError + nil + end +end + +admin_server = TCPServer.new(ADMIN_BIND, ADMIN_PORT) +warn("[mock-broker] admin HTTP listening on #{ADMIN_BIND}:#{ADMIN_PORT}") +admin_running = true +admin_thread = Thread.new do + while admin_running + begin + sock = admin_server.accept + rescue StandardError + break + end + Thread.new(sock) { |s| handle_admin(s) } + end +end + +Signal.trap('TERM') { exit(0) } +Signal.trap('INT') { exit(0) } +admin_thread.join diff --git a/test/support/fake_p4 b/test/support/fake_p4 index ba72106..a737cb6 100755 --- a/test/support/fake_p4 +++ b/test/support/fake_p4 @@ -9,11 +9,21 @@ # must arrive as exactly one argv element with its bytes unchanged, which is # only true if the connector execve's an argv array instead of building a # command line. +# +# The connector's Perforce executor (internal/tools/perforce.go #env) builds a +# deliberately minimal child environment -- PATH, HOME, P4PORT, P4USER, +# P4PASSWD only -- and does not forward arbitrary variables, so a caller can't +# rely on FAKE_P4_ARGV_LOG surviving into the child process unless something +# else arranges it. drills.rb arranges it itself, with a tiny wrapper script +# that bakes the path in before exec'ing this file (see drills.rb's +# P4_WRAPPER). The UAT container (docker-compose.uat-connector.yml) has no +# such wrapper -- connector.yml points straight at this file -- so the +# fallback below is what makes the UAT no-shell drill (connector.spec.js +# phase 510) work: same default path as the connector's own log_dir. require 'json' -if (log = ENV['FAKE_P4_ARGV_LOG']) - File.open(log, 'a') { |f| f.puts(JSON.generate(ARGV)) } -end +log = ENV['FAKE_P4_ARGV_LOG'] || '/var/log/connector/p4-argv.jsonl' +File.open(log, 'a') { |f| f.puts(JSON.generate(ARGV)) } # Strip the fixed connection flags the connector always passes. args = ARGV.dup diff --git a/test/support/teamcity_stub.rb b/test/support/teamcity_stub.rb index 091da08..4b3ac80 100644 --- a/test/support/teamcity_stub.rb +++ b/test/support/teamcity_stub.rb @@ -8,18 +8,41 @@ # broker socket. The stub refuses any request without the exact token the config # file holds, so a connector that had somehow lost local custody would fail the # round-trip drills rather than quietly passing them. +# +# UAT addition (connector.spec.js, issue #1574/#1575): this file is also run +# standalone as a container in docker-compose.uat-connector.yml, modelling the +# studio's real on-prem TeamCity. Two extra jobs on top of the original +# in-process API, both admin-only and unauthenticated (LAN-internal, drill/UAT +# use only -- never anything a real TeamCity exposes): +# +# POST /uat/builds -- seed a build from JSON, so the standalone +# process can be seeded over HTTP instead of a +# constructor call. +# POST /uat/webhooks/fire -- fire an OUTBOUND webhook at a ButterStack +# endpoint, in either of the two shapes a real +# TeamCity delivery can take: its own built-in +# webhook envelope ("native"), or the flat +# payload a curl build step produces +# ("curl_step"). This proves the same intake +# endpoint that accepts a Jenkins/generic-CI +# payload also accepts what TeamCity actually +# sends, for both of the Tier 1 wiring options. require 'json' require 'socket' +require 'net/http' +require 'uri' class TeamCityStub attr_reader :port, :requests - def initialize(token:, logger: nil) + def initialize(token:, logger: nil, bind: '127.0.0.1', port: 0) @token = token @logger = logger @requests = [] @mutex = Mutex.new @builds = {} + @bind = bind + @port = port end def add_build(id, build_type_id:, number:, status: 'SUCCESS', state: 'finished', revision: nil) @@ -38,7 +61,7 @@ def add_build(id, build_type_id:, number:, status: 'SUCCESS', state: 'finished', end def start - @server = TCPServer.new('127.0.0.1', 0) + @server = TCPServer.new(@bind, @port || 0) @port = @server.addr[1] @running = true @thread = Thread.new { accept_loop } @@ -76,14 +99,28 @@ def serve(sock) head = +'' head << sock.readpartial(1) until head.end_with?("\r\n\r\n") lines = head.split("\r\n") - _method, target, = lines.shift.split(' ') + method, target, = lines.shift.split(' ') headers = lines.each_with_object({}) do |l, h| k, v = l.split(':', 2) h[k.to_s.strip.downcase] = v.to_s.strip if k && v end + body = +'' + if (len = headers['content-length'].to_i) > 0 + body << sock.read(len).to_s + end path, query = target.split('?', 2) @mutex.synchronize { @requests << { path: path, query: query, authorization: headers['authorization'] } } + # UAT/drill admin endpoints. Unauthenticated on purpose: this stub only + # ever runs on the studio_lan network (no host port published), which is + # exactly the boundary the connector.spec.js isolation phase (100) proves. + case [method, path] + when ['POST', '/uat/builds'] + return handle_uat_add_build(sock, body) + when ['POST', '/uat/webhooks/fire'] + return handle_uat_fire_webhook(sock, body) + end + return respond(sock, 401, { 'error' => 'unauthorized' }) unless headers['authorization'] == "Bearer #{@token}" case path @@ -108,6 +145,79 @@ def serve(sock) end end + def handle_uat_add_build(sock, body) + params = JSON.parse(body) + id = params.fetch('id') + add_build(id, + build_type_id: params.fetch('build_type_id'), + number: params.fetch('number'), + status: params['status'] || 'SUCCESS', + state: params['state'] || 'finished', + revision: params['revision']) + respond(sock, 200, { 'ok' => true, 'id' => id.to_i }) + rescue KeyError, JSON::ParserError => e + respond(sock, 400, { 'error' => e.message }) + end + + # handle_uat_fire_webhook is the UAT stand-in for what a real TeamCity + # delivers on "Build finished": either its own built-in webhook envelope + # (teamcity.internal.webhooks.*, mode "native") with the credential in the + # non-standard php-auth-user/php-auth-pw headers (TeamCity's built-in webhook + # feature cannot set a custom header and cannot sign), or the flat payload a + # curl build step composes (mode "curl_step") with the credential in + # X-Webhook-Token, matching the recipe in the #1574 Tier 1 wiring doc. + # + # This request is OUTBOUND from the studio's TeamCity to ButterStack's + # webhook intake -- exactly the direction studio_lan permits. + def handle_uat_fire_webhook(sock, body) + params = JSON.parse(body) + mode = params.fetch('mode') + build_id = params.fetch('build_id') + url = URI.parse(params.fetch('url')) + build = @mutex.synchronize { @builds[build_id.to_i] } + raise "no such seeded build #{build_id}" unless build + + req = Net::HTTP::Post.new(url) + req['Content-Type'] = 'application/json' + # If the receiving app enforces Host-header allowlisting (production/ + # staging config.hosts), the outbound hop here goes through studio-egress, + # whose compose hostname is not on that list. Pin the Host header to one + # that is, so the drill exercises intake auth rather than host rejection. + req['Host'] = params['host_header'] || 'localhost' + + case mode + when 'native' + # Credentials are optional on purpose: omitting username/password lets + # the connector.spec.js "missing credential" drill (phase 420) fire a + # request with no Authorization channel at all, distinct from a + # present-but-wrong one. + req['php-auth-user'] = params['username'] if params['username'] + req['php-auth-pw'] = params['password'] if params['password'] + req.body = JSON.generate({ 'eventType' => 'BUILD_FINISHED', 'payload' => build }) + when 'curl_step' + req['X-Webhook-Token'] = params['token'] if params['token'] + flat = { + 'job_name' => build['buildTypeId'], + 'build_number' => build['number'], + 'status' => (build['status'] == 'SUCCESS') ? 'success' : 'failure', + 'build_url' => build['webUrl'], + 'changelist' => build.dig('revisions', 'revision', 0, 'version') + } + flat['logs_tail'] = params['logs_tail'] if params['logs_tail'] + req.body = JSON.generate(flat) + else + raise "unknown mode #{mode.inspect}" + end + + http = Net::HTTP.new(url.host, url.port) + http.open_timeout = 10 + http.read_timeout = 15 + response = http.request(req) + respond(sock, 200, { 'upstream_status' => response.code.to_i, 'upstream_body' => response.body.to_s }) + rescue StandardError => e + respond(sock, 502, { 'error' => "#{e.class}: #{e.message}" }) + end + def respond(sock, status, body) json = JSON.generate(body) sock.write("HTTP/1.1 #{status} #{status == 200 ? 'OK' : 'Error'}\r\n" \ @@ -116,3 +226,28 @@ def respond(sock, status, body) "Connection: close\r\n\r\n#{json}") end end + +# Standalone entry point: `ruby teamcity_stub.rb`, for the UAT container. +# Reads TC_TOKEN (required), TC_BIND / TC_PORT (default 0.0.0.0:8111), and +# optionally seeds one build from TC_SEED_* env vars so the container is +# useful even before the test calls POST /uat/builds. +if $PROGRAM_NAME == __FILE__ + token = ENV.fetch('TC_TOKEN') + bind = ENV['TC_BIND'] || '0.0.0.0' + port = (ENV['TC_PORT'] || '8111').to_i + + stub = TeamCityStub.new(token: token, logger: ->(m) { warn(m) }, bind: bind, port: port) + + if ENV['TC_SEED_BUILD_ID'] + stub.add_build(ENV.fetch('TC_SEED_BUILD_ID').to_i, + build_type_id: ENV.fetch('TC_SEED_BUILD_TYPE_ID', 'Uat_Build'), + number: ENV.fetch('TC_SEED_BUILD_NUMBER', '512'), + revision: ENV['TC_SEED_REVISION']) + end + + stub.start + warn("[teamcity-stub] listening on #{bind}:#{stub.port}, token=#{token[0, 4]}...") + Signal.trap('TERM') { exit(0) } + Signal.trap('INT') { exit(0) } + sleep +end diff --git a/test/uat/entrypoint.sh b/test/uat/entrypoint.sh new file mode 100755 index 0000000..05acbfc --- /dev/null +++ b/test/uat/entrypoint.sh @@ -0,0 +1,73 @@ +#!/bin/sh +# UAT-ONLY entrypoint for docker-compose.uat-connector.yml's `connector` +# service (connector.spec.js, issue #1574/#1575). +# +# The daemon itself NEVER reads a credential from an environment variable -- +# see connector/PROTOCOL.md section 5 ("Credential custody") and +# connector/internal/config/config.go's doc comment: every secret comes from +# connector.yml or a *_file path it names, full stop, no env fallback, no +# flag, no remote configuration. That rule is what makes "your credentials +# never leave your network" depend on the studio's file permissions rather +# than on our good behaviour, and it is enforced in code, not by convention. +# +# So this script does NOT hand the daemon a credential via the environment. +# It renders /etc/butterstack/connector.yml FROM the environment and then +# execs the real, unmodified daemon, which reads only that file -- this is +# the container-native form of "the studio's vault (or config-management +# push, or hand-authored file) injects connector.yml"; the studio's actual +# mechanism for getting bytes onto disk is out of scope for the protocol, and +# so is this script's mechanism. Both produce the same artifact: a 0600 file +# on local disk. +set -eu + +umask 077 + +CONFIG_PATH="${UAT_CONNECTOR_CONFIG_PATH:-/etc/butterstack/connector.yml}" +mkdir -p "$(dirname "$CONFIG_PATH")" + +: "${UAT_CONNECTOR_ENDPOINT:?UAT_CONNECTOR_ENDPOINT is required (wss://mock-broker:9443/connect)}" +: "${UAT_CONNECTOR_TOKEN:?UAT_CONNECTOR_TOKEN is required (bsc__)}" + +# depot_scope is a comma-separated list in the env, rendered as a YAML +# sequence. Default matches the UAT fixtures' seeded depot. +depot_scope_yaml="" +old_ifs="$IFS" +IFS=',' +for p in ${UAT_CONNECTOR_DEPOT_SCOPE:-//depot/game/}; do + depot_scope_yaml="${depot_scope_yaml} - ${p} +" +done +IFS="$old_ifs" + +teamcity_enabled="${UAT_CONNECTOR_TEAMCITY_ENABLED:-true}" +if [ "$teamcity_enabled" = "true" ]; then + : "${UAT_CONNECTOR_TEAMCITY_TOKEN:?UAT_CONNECTOR_TEAMCITY_TOKEN is required when teamcity is enabled}" +fi + +cat > "$CONFIG_PATH" < Date: Sun, 30 Aug 2026 15:11:29 -0400 Subject: [PATCH 04/11] test(uat): TeamCity stub authenticates like the real 2026.1.3 server Stage A against a real TeamCity 2026.1.3 (fixture report 2026-08-30-teamcity-fixture-stage-a.md) showed the built-in webhooks send a standard Authorization: Basic header built from teamcity.internal.webhooks.username/.password (plain parameter only; Password-typed sends no auth header), not the php-auth-* pair the plans assumed. The uat:connector stub's native mode now sends Basic with the real server's User-Agent by default and builds the envelope in the full REST Build shape from the captured payloads; auth: "php-auth" keeps the secondary channel covered. Spec phases: 410 native+Basic, new 415 native+php-auth, 420 covers wrong Basic, wrong php-auth and missing credential. Full uat:connector run and all nine drills green. Upstream: PR #1578 (rides the uat:connector commit). Refs https://github.com/ButterStack/butter_stack/issues/1574 Refs https://github.com/ButterStack/butter_stack/issues/1575 Co-Authored-By: Claude Fable 5 (cherry picked from commit 397db4b5f19e59121f26bea459c48bab1b18ba60) --- README.md | 22 ++++++-- test/support/teamcity_stub.rb | 96 ++++++++++++++++++++++++++++++++--- 2 files changed, 106 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 28e3f5a..6c50cd1 100644 --- a/README.md +++ b/README.md @@ -208,11 +208,23 @@ asserts this shape directly before doing anything else. - The TeamCity Tier 1 webhook path both ways Teddy's design note describes: the curl-step flat payload (`X-Webhook-Token`) creates a `BuildRun` with `ci_provider == 'teamcity'`; TeamCity's own built-in webhook envelope - (`php-auth-user`/`php-auth-pw`) authenticates and records a `WebhookEvent` - but does **not** yet create a `BuildRun` - there is no adapter for the - `{eventType, payload}` shape in `jenkins_controller.rb` (issue #1574 Phase - 1, `normalize_ci_payload`). The spec documents this as the real current - behavior; the assertion is written to flip the day that adapter lands. + authenticates and records a `WebhookEvent` but does **not** yet create a + `BuildRun` - there is no adapter for the `{eventType, payload}` shape in + `jenkins_controller.rb` (issue #1574 Phase 1, `normalize_ci_payload`). The + spec documents this as the real current behavior; the assertion is written + to flip the day that adapter lands. +- A real TeamCity 2026.1.3 server (captured on Ryan's LAN with a request + logger) authenticates its built-in webhook with a standard `Authorization: + Basic base64(username:password)` header built from + `teamcity.internal.webhooks.username`/`.password`, and only when + `.password` is declared as a plain-typed parameter - a Password-typed + `.password` sends no `Authorization` header at all, not + `php-auth-user`/`php-auth-pw`. `teamcity-stub`'s `mode: 'native'` mirrors + this: it sends Basic auth by default (phase 410) and the real server's + `User-Agent` string, and falls back to the non-standard + `php-auth-user`/`php-auth-pw` pair only when the caller passes `auth: + 'php-auth'` (phase 415) - kept as the secondary credential channel + `WebhookTokenSources` still accepts (#935). - Neither webhook token nor its HTTP headers ever land in a persisted `WebhookEvent`. - The connector's compiled vocabulary, denials (unknown verb, out-of-scope diff --git a/test/support/teamcity_stub.rb b/test/support/teamcity_stub.rb index 4b3ac80..d9ee5ee 100644 --- a/test/support/teamcity_stub.rb +++ b/test/support/teamcity_stub.rb @@ -161,12 +161,24 @@ def handle_uat_add_build(sock, body) # handle_uat_fire_webhook is the UAT stand-in for what a real TeamCity # delivers on "Build finished": either its own built-in webhook envelope - # (teamcity.internal.webhooks.*, mode "native") with the credential in the - # non-standard php-auth-user/php-auth-pw headers (TeamCity's built-in webhook - # feature cannot set a custom header and cannot sign), or the flat payload a - # curl build step composes (mode "curl_step") with the credential in + # (teamcity.internal.webhooks.*, mode "native") or the flat payload a curl + # build step composes (mode "curl_step") with the credential in # X-Webhook-Token, matching the recipe in the #1574 Tier 1 wiring doc. # + # A real TeamCity 2026.1.3 server (captured on Ryan's LAN with a request + # logger; see test/fixtures/files/teamcity/delivery_headers.json) sends its + # built-in webhook with a standard `Authorization: Basic + # base64(username:password)` header built from + # teamcity.internal.webhooks.username/.password when `.password` is a + # plain-typed parameter -- never php-auth-user/php-auth-pw. With a + # Password-typed `.password`, no Authorization header is sent at all. Mode + # "native" therefore defaults to Basic auth (the real channel) and only + # falls back to the non-standard php-auth-user/php-auth-pw pair when + # `auth: "php-auth"` is passed explicitly -- kept because + # WebhookTokenSources still accepts it as a secondary credential channel + # (#935). Passing neither username nor password models the Password-typed + # case: no Authorization header at all. + # # This request is OUTBOUND from the studio's TeamCity to ButterStack's # webhook intake -- exactly the direction studio_lan permits. def handle_uat_fire_webhook(sock, body) @@ -187,13 +199,23 @@ def handle_uat_fire_webhook(sock, body) case mode when 'native' + # Real TeamCity identifies itself with this exact User-Agent on every + # request, regardless of auth channel. + req['User-Agent'] = 'TeamCity Server 2026.1.3 (build 222742)' + # Credentials are optional on purpose: omitting username/password lets # the connector.spec.js "missing credential" drill (phase 420) fire a # request with no Authorization channel at all, distinct from a # present-but-wrong one. - req['php-auth-user'] = params['username'] if params['username'] - req['php-auth-pw'] = params['password'] if params['password'] - req.body = JSON.generate({ 'eventType' => 'BUILD_FINISHED', 'payload' => build }) + if params['auth'] == 'php-auth' + req['php-auth-user'] = params['username'] if params['username'] + req['php-auth-pw'] = params['password'] if params['password'] + elsif params['username'] || params['password'] + creds = "#{params['username']}:#{params['password']}" + req['Authorization'] = "Basic #{[creds].pack('m0')}" + end + + req.body = JSON.generate({ 'eventType' => 'BUILD_FINISHED', 'payload' => native_payload_for(build) }) when 'curl_step' req['X-Webhook-Token'] = params['token'] if params['token'] flat = { @@ -218,6 +240,66 @@ def handle_uat_fire_webhook(sock, body) respond(sock, 502, { 'error' => "#{e.class}: #{e.message}" }) end + # native_payload_for builds the "native" envelope's payload in the shape of + # a real TeamCity REST Build resource -- the same keys captured in + # test/fixtures/files/teamcity/build_finished_success.json (id, + # buildTypeId, number, status, state, href, webUrl, statusText, + # buildType{id,name,projectName,projectId,href,webUrl}, queuedDate/ + # startDate/finishDate, triggered, changes, revisions, agent, artifacts, + # properties) -- rather than the reconstructed minimal object this stub + # used to hand back. It's inlined here (not read from that fixture at + # runtime) because the teamcity-stub container only mounts connector/test, + # not the repo-root test/fixtures tree. + # + # The seeded build's own id/buildTypeId/number/status/state/revision are + # threaded through verbatim, including `revisions`, so the curl_step and + # native paths carry the same version and the Perforce path still has one + # to chase. + def native_payload_for(build) + build_type_id = build['buildTypeId'] + { + 'id' => build['id'], + 'buildTypeId' => build_type_id, + 'number' => build['number'], + 'status' => build['status'], + 'state' => build['state'], + 'href' => "/app/rest/builds/id:#{build['id']}", + 'webUrl' => build['webUrl'], + 'statusText' => build['statusText'], + 'buildType' => { + 'id' => build_type_id, + 'name' => build_type_id.to_s.tr('_', ' '), + 'projectName' => 'ButterStack UAT Fixture', + 'projectId' => 'ButterStackUatFixture', + 'href' => "/app/rest/buildTypes/id:#{build_type_id}", + 'webUrl' => "http://teamcity.invalid/buildConfiguration/#{build_type_id}?mode=builds" + }, + 'queuedDate' => build['queuedDate'], + 'startDate' => build['startDate'], + 'finishDate' => build['finishDate'], + 'triggered' => { + 'type' => 'user', + 'date' => build['queuedDate'], + 'user' => { 'username' => 'uat', 'id' => 1, 'href' => '/app/rest/users/id:1' } + }, + 'changes' => { 'href' => "/app/rest/changes?locator=build:(id:#{build['id']})" }, + 'revisions' => build['revisions'], + 'agent' => { + 'id' => 1, 'name' => 'teamcity-agent-1', 'typeId' => 1, + 'href' => '/app/rest/agents/id:1', + 'webUrl' => 'http://teamcity.invalid/agentDetails.html?id=1&agentTypeId=1&realAgentName=teamcity-agent-1' + }, + 'artifacts' => { 'count' => 0, 'href' => "/app/rest/builds/id:#{build['id']}/artifacts/children/" }, + 'properties' => { + 'count' => 2, + 'property' => [ + { 'name' => 'teamcity.internal.webhooks.username', 'value' => 'teamcity', 'inherited' => true }, + { 'name' => 'teamcity.internal.webhooks.password', 'value' => '******', 'inherited' => true } + ] + } + } + end + def respond(sock, status, body) json = JSON.generate(body) sock.write("HTTP/1.1 #{status} #{status == 200 ? 'OK' : 'Error'}\r\n" \ From 2f6c25cc30b9eb0f5cb4dd9296ec90b5374f6e96 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Sun, 30 Aug 2026 16:04:42 -0400 Subject: [PATCH 05/11] test(uat): mock broker admin root returns a route index Round-1 review, Ryan: GET / on the mock broker's admin port answered with a bare "no such admin route" error. It now returns a JSON index naming what the process is (the drill stand-in for the future Rails /connect broker, not the real broker), the wss:// endpoint, and each admin route with a one-line description. No route behavior changed. Upstream: PR #1578. Refs https://github.com/ButterStack/butter_stack/issues/1575 Co-Authored-By: Claude Fable 5 (cherry picked from commit 312dac0a2843e331015da8858ee869caacbfc7b7) --- test/mock_broker_server.rb | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/mock_broker_server.rb b/test/mock_broker_server.rb index c958d79..d9c4d47 100644 --- a/test/mock_broker_server.rb +++ b/test/mock_broker_server.rb @@ -71,6 +71,24 @@ # Admin HTTP API -- plain HTTP, hand-rolled in the style of teamcity_stub.rb. # No gems: this is a drill/UAT fixture, not a shipped service. # --------------------------------------------------------------------------- + +# One-line description per admin route, served back from GET / and GET +# /routes so a human hitting the admin port in a browser gets an index +# instead of a bare 404 "no such admin route" error. +ADMIN_ROUTES = { + 'GET /' => 'This index', + 'GET /routes' => 'This index (alias)', + 'GET /health' => 'Liveness check', + 'GET /ca.pem' => 'Fetch the throwaway TLS CA cert connector containers trust', + 'GET /status' => 'Current connector sessions, refusals, and discarded results', + 'POST /call' => 'Issue a synchronous call to the connected connector session', + 'POST /degraded_call' => 'Issue a call via the degraded/slow-path code path', + 'POST /issue_unregistered' => 'Issue a call outside the normal tracked-call table', + 'POST /revoke' => 'Revoke the registered connector token, closing its session', + 'POST /register' => 'Re-register the connector token after a revoke', + 'POST /partition' => 'Simulate a network partition for N seconds' +}.freeze + def respond_json(sock, status, body) json = JSON.generate(body) text = { 200 => 'OK', 400 => 'Bad Request', 404 => 'Not Found', 502 => 'Bad Gateway' }.fetch(status, 'Error') @@ -117,6 +135,15 @@ def handle_admin(sock) params = body.empty? ? {} : JSON.parse(body) case [method, path] + when ['GET', '/'], ['GET', '/routes'] + respond_json(sock, 200, { + 'service' => 'mock-broker', + 'description' => 'Drill stand-in for the future Rails /connect broker -- not the real broker. ' \ + 'See connector/test/README.md "What the mock broker is not".', + 'wss_url' => "wss://#{HOST}:#{PORT}", + 'admin_routes' => ADMIN_ROUTES + }) + when ['GET', '/health'] respond_json(sock, 200, { 'ok' => true }) From 5385512f9a325e4350abc490afa068001a77668b Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Sun, 30 Aug 2026 21:46:27 -0400 Subject: [PATCH 06/11] feat(connectors): per-project Connector model + Connectors UI (#1575 group 2b) Adds the Connector record (one active per project, v0), the token format bsc__, and a Connectors section on the project Integrations page (create/reveal-once/revoke). Scaffolding only: the /connect broker, verb execution, and tenant-scoped frame routing are a separate security-gated follow-up. Refs #1575 --- PROTOCOL.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/PROTOCOL.md b/PROTOCOL.md index b9b4693..ba6921a 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -65,7 +65,7 @@ session, cookie, or CSRF machinery in the path. ### Token format ``` -bsc__<32 random bytes, base32> +bsc__<32 random bytes, base32> ``` - The `bsc_` prefix makes the credential greppable by secret scanners, ours and @@ -75,6 +75,17 @@ bsc__<32 random bytes, base32> - 32 random bytes is 256 bits, which is why plain SHA-256 is the correct storage: a slow KDF buys nothing against an input with that much entropy. +**Superseded (Ryan, 2026-08-30, #1575 group 2b):** the id segment was +originally ``. A Connector is now a first-class +per-project record rather than a per-integration one, so the id segment is +`` instead - a compromise or misconfiguration of one +connector can only ever be scoped to its own project's account, never leak +across projects. This is a cross-project-isolation change to the id segment +only, not a wire-format redesign: the frame shapes in §3 are unaffected, and +`connector/internal/config`'s `tokenPattern` is id-segment-agnostic +(`\Absc_[a-z0-9][a-z0-9\-]{0,62}_[A-Za-z2-7]{32,128}\z`), so the daemon needs +no code change to accept a project-scoped token. + ### Storage on our side Store **only** `SHA-256(secret segment)`. Compare with From dbeae00e4af7cdd4f45c6ea82e1c71a75492a001 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Mon, 31 Aug 2026 09:12:46 -0400 Subject: [PATCH 07/11] fix(connectors): RecordNotUnique race, YAML-unsafe name, contrast, entropy wording Review + security fast-follow for PR #1589 (#1575 group 2b): - Rescue ActiveRecord::RecordNotUnique in Projects::ConnectorsController#create so a concurrent double-create against idx_one_active_connector_per_project shows the same refusal alert as the app-level validation instead of a 500. Extracted the shared message to Connector::ACTIVE_CONNECTOR_LIMIT_MESSAGE so both refusal paths can never drift apart. - Escape the connector name in the generated connector.yml snippet (connector_id: #{name.to_json}) so names containing YAML-significant characters (":", "#", quotes) can't produce a broken/reinterpreted file. - Fix contrast on the one-connector note: --bs-fg-faint -> --bs-fg-mute. - Correct connector/PROTOCOL.md's token entropy claim: the secret segment is 32 characters from a 58-symbol alphabet ([A-Za-z2-7]), not standard base32, which is ~187 bits of entropy, not 256. Refs #1575 --- PROTOCOL.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/PROTOCOL.md b/PROTOCOL.md index ba6921a..042d309 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -65,15 +65,18 @@ session, cookie, or CSRF machinery in the path. ### Token format ``` -bsc__<32 random bytes, base32> +bsc__<32 characters, 58-symbol alphabet> ``` - The `bsc_` prefix makes the credential greppable by secret scanners, ours and the studio's. - The id segment makes the broker's hashed lookup a primary-key read rather than a table scan. -- 32 random bytes is 256 bits, which is why plain SHA-256 is the correct - storage: a slow KDF buys nothing against an input with that much entropy. +- The secret segment is 32 characters drawn from a 58-symbol alphabet + (`[A-Za-z2-7]`, i.e. A-Z + a-z + 2-7), not standard base32 - that's + `32 * log2(58) ~ 187` bits of entropy, which is why plain SHA-256 is the + correct storage: a slow KDF buys nothing against an input with that much + entropy. **Superseded (Ryan, 2026-08-30, #1575 group 2b):** the id segment was originally ``. A Connector is now a first-class From fabc06cd6ea622f6fe2c682088d0d4f9eb95a5ef Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Thu, 3 Sep 2026 17:58:58 -0400 Subject: [PATCH 08/11] Extract connector from butter_stack with full history Bring the connector/ subtree from ButterStack/butter_stack into this repo as a standalone Go module, preserving all commit history via git subtree split. Add: - LICENSE (MIT, matching butterstack-cli) - .github/workflows/ci.yml (go vet, go test, go build) - docs/design-notes.md (spike scope and vocabulary table from README) - docs/uat.md (UAT topology and assertions from README) Rewrite README for a standalone repo reader: security model, install, configure (field-by-field from config.go), run (foreground, systemd, docker compose), verify (drills and make check), supported backends (compiled vs planned from vocab.go). Append mock-broker protocol rules to PROTOCOL.md. Co-Authored-By: Claude Opus 4.6 --- .github/workflows/ci.yml | 26 +++ .gitignore | 1 + LICENSE | 21 +++ PROTOCOL.md | 11 ++ README.md | 361 ++++++++++++++------------------------- docs/design-notes.md | 39 +++++ docs/uat.md | 53 ++++++ 7 files changed, 279 insertions(+), 233 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 LICENSE create mode 100644 docs/design-notes.md create mode 100644 docs/uat.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..6a8a7be --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,26 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + branches: [main] + +jobs: + connector-go: + timeout-minutes: 10 + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v7 + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version: '1.25' + cache-dependency-path: go.sum + - name: go vet + run: go vet ./... + - name: go test + run: go test ./... + - name: go build + run: go build ./cmd/butterstack-connector diff --git a/.gitignore b/.gitignore index 84c048a..90cbdfd 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ /build/ +/butterstack-connector diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..e6146ae --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 ButterStack + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/PROTOCOL.md b/PROTOCOL.md index 042d309..d31c5e0 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -380,3 +380,14 @@ a p4 server host:port or a TeamCity URL -- is written to the local audit line's `detail` field and never leaves the studio. The log is the studio's evidence, not ours. No verb can read it. + +--- + +## Appendix: mock broker and the four drill rules + +The mock broker (`test/mock_broker.rb`) is not the broker. The real one is a dedicated Rack endpoint at `/connect` on the Rails app with hashed-token auth, Redis-routed command and result, and explicit tenant scoping, a later PR. What the mock broker models is the surface the drills need, and it does implement faithfully the four rules the drills exist to prove: + +1. The upgrade is refused **before any per-connection state is allocated** when the token is absent, malformed, revoked, or wrong. +2. A query-string token is refused, and no code path reads a token from a query string. +3. Tokens are stored as the SHA-256 digest of the secret segment and compared in constant time; the plaintext is never held. +4. A `result` frame is matched against the issuing session's own outstanding commands; one carrying another session's command id is discarded. diff --git a/README.md b/README.md index 6c50cd1..c988b8c 100644 --- a/README.md +++ b/README.md @@ -1,266 +1,161 @@ -# butterstack-connector (spike) +# ButterStack Connector -An outbound-only daemon a studio runs inside its own network so ButterStack can -reach a private, on-premises Perforce or TeamCity **without the studio opening a -single inbound port**. +An outbound-only daemon a game studio runs inside its own network so ButterStack can reach a private, on-premises Perforce, TeamCity, Jenkins, GitHub Enterprise Server, or Horde without the studio opening a single inbound port. One outbound TLS connection to one hostname on 443. A typed command allowlist, never a tunnel and never a shell. Credentials stay on the studio's disk and never cross the wire. -It opens exactly one outbound TLS connection to one hostname on 443, announces -what it can do, and then executes only commands from a typed, versioned -allowlist with constrained arguments, each one logged locally. It holds the -studio's tool credentials in its own config file and never sends them. +Status: pre-release. Tracking: [ButterStack/butter_stack#1575](https://github.com/ButterStack/butter_stack/issues/1575). -This is the **spike** from issue #1575, checkbox groups 0 and 1: a standalone -proof of the daemon and the protocol schema. Nothing here is deployed, and -nothing here touches the Rails app. +## What it is -- [`PROTOCOL.md`](PROTOCOL.md) - the day-1 protocol schema (issue #1575 group 0) -- [`internal/vocab/vocab.go`](internal/vocab/vocab.go) - the whole allowlist, in - one readable file, on purpose -- [`test/`](test/) - the mock broker and the seven drills +The connector is a daemon the studio runs on its own hardware (or in a container on its own network). It opens exactly one outbound TLS connection to `wss://connect.butterstack.com/connect` on port 443, announces what it can do, and then executes only commands from a typed, versioned allowlist with constrained arguments, each one logged locally. Every credential the studio configures (Perforce tickets, TeamCity tokens) stays in the studio's config file and never crosses the wire. There is no inbound port, no tunnel, no shell, and no remote configuration: the broker cannot tell the connector where to find a credential. -Design sources, on branch `plan/teamcity-private-reach`: -`ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md` -§2.2–2.6, §4.3, §5, and -`ai/team/agents/shuri/reports/2026-08-29-connector-design-security-review.md` §6. +## Requirements ---- +- Go 1.25 or later (for building from source) +- Ruby 3.2+ (only for running the drill harness in `test/`) +- A ButterStack account with a connector token issued from the project's Connectors UI -## Build and run +## Install -Go 1.23. No host Go install is needed; the Makefile builds in a container. +**From source:** ```bash -make build # go build -> build/butterstack-connector -make test # go vet + go test ./... -make drills # the seven drills against the mock broker (needs Ruby 3.2+) -make check # test + drills -make vocabulary # print the compiled allowlist +go build -o butterstack-connector ./cmd/butterstack-connector ``` -`make build` uses `docker run golang:1.23-alpine`. If you have Go on the host, -`GO=go make build` uses it instead. +**Docker image (from the in-tree Dockerfile):** ```bash -./build/butterstack-connector -config /etc/butterstack/connector.yml -./build/butterstack-connector -print-vocabulary +docker build -t butterstack-connector . ``` -## Configuration +The Dockerfile produces a minimal image with a static Go binary and a Ruby runtime for the UAT entrypoint. In production, the entrypoint runs the connector directly from `/usr/local/bin/butterstack-connector`. + +## Configure -See [`connector.example.yml`](connector.example.yml). Two rules the daemon -enforces rather than documents: +Copy `connector.example.yml` to your install location (e.g. `/etc/butterstack/connector.yml`) and set its permissions to 0600. The daemon refuses to start if the config file or any `*_file` path is readable by group or other users. -- **`connector.yml` and every `*_file` must be mode 0600 or stricter**, or it - refuses to start. -- **The endpoint must be `wss://` with no query string.** A copy-pasted - `?token=...` URL cannot start the daemon at all. +```bash +install -m 0600 connector.example.yml /etc/butterstack/connector.yml +``` -Every credential comes from that file, or from a `*_file` path it names. There -is no environment-variable fallback, no flag that takes a secret, and no remote -configuration: the broker cannot tell the connector where to find a credential. +Every credential comes from this file, or from a `*_file` path it names. There is no environment-variable fallback, no flag that takes a secret, and no remote configuration. + +### Fields + +| Field | Type | Secret | Source | Description | +|---|---|---|---|---| +| `endpoint` | string | no | file | The broker URL. Must be `wss://` with no query string, no userinfo, no fragment. This is the one hostname your egress rule needs. | +| `endpoint_ca_file` | string | no | file | Optional. Pins the trust anchor for the endpoint, for a private CA or a TLS-inspecting proxy. There is no option to skip verification for the broker connection. | +| `token` | string | **yes** | file | The connector token issued in the ButterStack UI. Shown exactly once at issue time. Format: `bsc__`. | +| `token_file` | string | no | file | Alternative to `token`: path to a file containing the token (for vault-injected secrets). Mutually exclusive with `token`. The file must be mode 0600. | +| `connector_id` | string | no | file | A name for this host, shown in the Connection Status panel. | +| `log_dir` | string | no | file | Local audit log directory. One JSON line per command, including every denial. Defaults to a `logs/` directory next to the config file. | +| `max_concurrent` | int | no | file | Commands executed in parallel. Range: 1-32. Default: 4. | +| `scopes.depot_scope` | list | no | file | Literal Perforce depot prefixes (no wildcards). A path whose literal prefix is not inside one of these is denied. | +| `scopes.allowed_build_types` | list | no | file | For the reserved `teamcity.build.queue` verb. Not compiled in v0. | +| `scopes.repo_allowlist` | list | no | file | For the reserved `ghes.*` verbs. Not compiled in v0. | +| `toggles.content_verbs` | bool | no | file | Content-class verbs are off in v0 at the schema level. This switch cannot turn one on yet. | +| `perforce.enabled` | bool | no | file | Enable the Perforce tool. | +| `perforce.binary` | string | no | file | Path to the `p4` CLI. Default: `p4`. | +| `perforce.port` | string | no | file | Helix Core server address (e.g. `ssl:perforce.studio.lan:1666`). | +| `perforce.user` | string | no | file | A read-only Perforce user. | +| `perforce.ticket` | string | **yes** | file | The Perforce ticket. Passed to `p4` via the `P4PASSWD` environment variable so it does not appear in the process list. | +| `perforce.ticket_file` | string | no | file | Alternative to `perforce.ticket`. Must be mode 0600. | +| `perforce.timeout` | duration | no | file | Timeout for `p4` commands. Default: `20s`. | +| `teamcity.enabled` | bool | no | file | Enable the TeamCity tool. | +| `teamcity.url` | string | no | file | The TeamCity server URL on your LAN (e.g. `https://teamcity.studio.lan`). | +| `teamcity.token` | string | **yes** | file | A project-limited TeamCity access token with a read-only role. Never crosses the wire. | +| `teamcity.token_file` | string | no | file | Alternative to `teamcity.token`. Must be mode 0600. | +| `teamcity.ca_file` | string | no | file | Optional. Trust anchor for a self-signed certificate on your LAN TeamCity. Scoped to this server only, not the broker connection. | +| `teamcity.allow_insecure_tls` | bool | no | file | Skip TLS verification for the LAN TeamCity. There is no equivalent for the broker connection, which always verifies. | +| `teamcity.timeout` | duration | no | file | Timeout for TeamCity REST calls. Default: `10s`. | + +Two rules the daemon enforces at startup rather than documents: + +- `connector.yml` and every `*_file` must be mode 0600 or stricter, or it refuses to start. +- The endpoint must be `wss://` with no query string. A copy-pasted `?token=...` URL cannot start the daemon at all. + +## Run + +**Foreground:** -## What is in the vocabulary +```bash +./butterstack-connector -config /etc/butterstack/connector.yml +``` -| Verb | v0 | -|---|---| -| `sys.ping`, `sys.version`, `sys.capabilities` | compiled | -| `teamcity.server.info` | compiled | -| `teamcity.build.get {build_id}` | compiled | -| `p4.describe {change, max_files, include_diff:false}` | compiled | -| `p4.changes {path, max}` | compiled | -| `teamcity.build.queue`, `jenkins.build.trigger` | reserved, denied | -| `p4.file_contents`, `ghes.commit.get`, `horde.server.info` | reserved, denied | +**Print the compiled vocabulary:** -No verb accepts a host, port, URL, or shell string. No verb accepts -caller-supplied build parameters or properties, that is enforced structurally -(`bannedArgNames` plus `Selfcheck()`, which runs at process start as well as in -the tests), because a parameter map on a build-triggering verb interpolates into -shell build steps and would make the allowlist a code-execution primitive inside -the studio's LAN. No mutating verb and no content-class verb is compiled in. +```bash +./butterstack-connector -print-vocabulary +``` ---- +**systemd unit (example):** -## The drills +```ini +[Unit] +Description=ButterStack Connector +After=network-online.target +Wants=network-online.target -`make drills` runs the seven drills from design note §4.3 against -`test/mock_broker.rb`, plus a round-trip phase and the broker-side half of -drill (f). Every drill passes today: +[Service] +Type=simple +User=butterstack-connector +ExecStart=/usr/local/bin/butterstack-connector -config /etc/butterstack/connector.yml +Restart=on-failure +RestartSec=5s -| | Drill | What it asserts | -|---|---|---| -| P0 | verbs round-trip | all five compiled verbs answer `ok`; results carry only the declared fields; the TeamCity token used on the LAN is the one from `connector.yml` | -| D1 | out-of-vocabulary verb | `sys.exec`, `p4.print`, … → `denied / unknown_verb`; reserved names → `denied / verb_not_compiled`; each with a local audit line | -| D2 | out-of-scope argument | `//...`, `//depot/...`, a smuggled `params`/`properties` map, a quoted integer, `include_diff:true` → all `denied`, none reaching the tool; an in-scope path carrying shell metacharacters reaches `p4` as one literal argv element and no shell runs | -| D3 | query-string token | the daemon refuses such an endpoint; the broker answers HTTP 400 and never a 101; no session state is allocated; missing and wrong bearer tokens are 401 | -| F* | cross-session result | a `result` whose command id the session never issued is discarded, not dispatched by id alone | -| R1 | network drop | the session dies and the connector reconnects with no operator action | -| R2 | connector stopped | we flip to offline; a verb-dependent feature renders "needs connector" instead of raising; the daemon logs a clean shutdown | -| R3 | our side stopped | the connector backs off, logs it, and reconnects when the broker returns | -| R4 | token revoked | the socket closes within one heartbeat; reconnects are refused; `connector.yml` is byte-identical and still 0600; and **no tool credential, LAN host, port, or URL ever appeared in a frame** | - -The mock broker is not the broker. The real one is a dedicated Rack endpoint at -`/connect` on the Rails app with hashed-token auth, Redis-routed command and -result, and explicit tenant scoping - a later PR. What `test/mock_broker.rb` -models is the surface these drills need, and it does implement faithfully the -four rules they exist to prove: refusal before session-state allocation, -header-only tokens, SHA-256 digest storage with constant-time compare, and -per-session result matching. - ---- - -## What this spike does **not** prove - -Carried forward from design note §5 and Shuri §6 item 7, plus what this -standalone shape adds: - -- **The argument-constraint layer end to end.** The drills prove denial at the - frame boundary against a mock broker. They do not prove it against a real - broker, a real TeamCity, or a real p4d. -- **Anything on the Rails side.** There is no `/connect` endpoint in this PR, no - ActionCable change, no migration, no UI. The tenant-context drill - "assert - tenant context is nil at the start of a request that follows a connector frame - on the same Puma thread" - is Rails-side and is **not** covered here. Only the - broker-side half of drill (f) is. -- **Anything on real infrastructure.** Nothing ran against staging, demo, or - production. No terraform, no security group, no hostname, no certificate. - Stage A (the Tier 1 TeamCity webhook run) has not been run, and it is gated on - the #1574 Phase -1 app fixes landing first; the "no token in - `webhook_events.payload` or the app log" drill therefore has no result yet. -- **The frame codec against an independent production stack.** Both ends here - were written from RFC 6455 - the Go client and the Ruby server independently, - which is why a masking or handshake mistake shows up as a failed drill. But - neither has met a real ALB, a real nginx `Upgrade` hop, or a real proxy. -- **Latency over a home connection.** The drills run on loopback. The design's - under-2-second target is untested against a NATed home network, and the - `ss`/`netstat` capture showing exactly one outbound established connection and - zero listeners has not been taken. -- **Survival conditions 1, 4, and 5.** No Sigstore keyless signing, no SBOM, no - build-from-source instructions, no digest-pinned base image, no version-skew - handling, and no `egress.md` with a per-verb output schema enforced as a field - allowlist with a conformance test. The fixed `fields=` projections in the - TeamCity executor are the beginning of that, not the whole of it. -- **Scale and multi-node routing.** Puma behaviour at tens of connectors, socket - routing under a real ASG scale-out, and the per-integration connection cap and - per-session command budget in the broker. -- **Everything beyond the five compiled verbs.** No Jenkins, GHES, or Horde - verb; no Perforce verb beyond `describe` and `changes`; no mutating verb; no - content verb; no poll-loop mode; no Windows service. -- **An actual IT-director review.** Appendix B is a script, not a test. - -This is the go/no-go input for the build, and it is deliberately smaller than -the product. - ---- - -## UAT - -`tests/uat/connector.spec.js` (repo root) is a second, Docker-based test of -this same daemon: TeamCity Tier 1 webhook intake into the real Rails app, and -the compiled verbs, denials, and degradation drills against a containerized -version of this connector, run inside a Docker-modelled "studio LAN" that is -NOT reachable from the cloud side (outbound allowed, inbound blocked), which -is the shape issue #1574/#1575 actually sell. +[Install] +WantedBy=multi-user.target +``` -```bash -docker-compose --profile core --profile connector up -d --build connector -npm run test:uat:connector -npm run test:uat:connector:keep-data # leaves the signup/project for inspection +**Docker Compose (example):** + +```yaml +services: + connector: + image: butterstack-connector + build: . + entrypoint: + - /usr/local/bin/butterstack-connector + - -config + - /etc/butterstack/connector.yml + volumes: + - ./connector.yml:/etc/butterstack/connector.yml:ro + restart: unless-stopped ``` -**Always scope `--build` to `connector`.** `web`/`sidekiq` still carry a -`build:` key in the base `docker-compose.yml` even though -`docker-compose.uat-connector.yml` pins them to the prebuilt -`image: butter_stack-web:latest`; an unscoped `--build` rebuilds *and retags* -that shared image, which every worktree's dev stack uses. +## Verify -### Topology +The drill harness (`test/drills.rb`) runs seven drills from design note section 4.3 against the mock broker (`test/mock_broker.rb`), plus a round-trip phase and the broker-side half of drill (f). Requires Ruby 3.2+ and a built binary. +```bash +make check # go vet + go test + drills +make test # go vet + go test only +make drills # drills only (needs a built binary) +make build # build the binary +make vocabulary # print the compiled allowlist ``` - "cloud" side (network: default) "studio LAN" (network: studio_lan, - internal: true -- no route out) - +-----------+ +--------------+ +----------------+ - | web | | mock-broker |<===wss:9443====>| connector | - | (Rails) | | (drill stand-| | (the daemon | - +-----+-----+ | in for the | | under test) | - ^ | real /connect| +--------+-------+ - | | endpoint) | | - | http :3000| | | (LAN-local) - +-----+---------+ +--------------+ | - | studio-egress |<===================studio_lan==============+ - | (socat, models| ^ - | outbound-only| +--------+---------+ - | firewall) | | teamcity-stub | - +---------------+ | (fake on-prem CI, | - | no published port)| - +--------------------+ -``` -`internal: true` on `studio_lan` is the whole isolation guarantee. `web` -cannot resolve or reach `teamcity-stub`; `connector` publishes no port at all -(`docker inspect` shows an empty port map, and `netstat -tln` inside it lists -nothing but Docker's own embedded DNS resolver); `teamcity-stub`'s only way to -reach Rails is the one TCP port `studio-egress` forwards, which is the studio -firewall's "outbound allowed" in miniature. `connector.spec.js` phase 100 -asserts this shape directly before doing anything else. - -### What this proves - -- The TeamCity Tier 1 webhook path both ways Teddy's design note describes: - the curl-step flat payload (`X-Webhook-Token`) creates a `BuildRun` with - `ci_provider == 'teamcity'`; TeamCity's own built-in webhook envelope - authenticates and records a `WebhookEvent` but does **not** yet create a - `BuildRun` - there is no adapter for the `{eventType, payload}` shape in - `jenkins_controller.rb` (issue #1574 Phase 1, `normalize_ci_payload`). The - spec documents this as the real current behavior; the assertion is written - to flip the day that adapter lands. -- A real TeamCity 2026.1.3 server (captured on Ryan's LAN with a request - logger) authenticates its built-in webhook with a standard `Authorization: - Basic base64(username:password)` header built from - `teamcity.internal.webhooks.username`/`.password`, and only when - `.password` is declared as a plain-typed parameter - a Password-typed - `.password` sends no `Authorization` header at all, not - `php-auth-user`/`php-auth-pw`. `teamcity-stub`'s `mode: 'native'` mirrors - this: it sends Basic auth by default (phase 410) and the real server's - `User-Agent` string, and falls back to the non-standard - `php-auth-user`/`php-auth-pw` pair only when the caller passes `auth: - 'php-auth'` (phase 415) - kept as the secondary credential channel - `WebhookTokenSources` still accepts (#935). -- Neither webhook token nor its HTTP headers ever land in a persisted - `WebhookEvent`. -- The connector's compiled vocabulary, denials (unknown verb, out-of-scope - path, reserved-but-not-compiled verb, wrong argument type, a disabled - content toggle, an unknown argument), and the no-shell proof on the p4 argv - log, all against a real containerized daemon rather than the in-process - drill harness. -- Broker-side refusals (query-string token, missing bearer token) before any - session state exists, cross-session result discard, and the degradation - drills (connector stop/start, token revoke/re-register) - the same four - rules `test/drills.rb` proves, exercised over the network instead of a - Ruby method call. - -### What this UAT suite does NOT prove - -Everything `test/README.md`'s own "what this spike does not prove" section -already says, plus: - -- **There is still no real `/connect` Rack endpoint.** `mock-broker` here is - the same drill stand-in as `test/mock_broker.rb`, containerized - (`test/mock_broker_server.rb`) with a small HTTP admin API bolted on so the - Playwright test can drive sessions/refusals/revoke/partition without - speaking the wire protocol itself. It is not, and does not claim to be, the - real broker. -- **No real TeamCity.** `teamcity-stub` (`test/support/teamcity_stub.rb`) is a - hand-rolled fake that answers exactly the REST calls the connector makes and - can fire the two outbound webhook shapes a real TeamCity delivers. It has - never seen a real TeamCity server's actual quirks. -- **No real p4d.** `test/support/fake_p4` stands in for the `p4` CLI, same as - in the drills. -- **Idle-connector liveness** is a WS-layer ping/pong rule: the heartbeat - goroutine sends a WebSocket ping alongside the application-level heartbeat - frame, and the read loop's deadline only means "reconnect" when no inbound - frame of any kind (a pong included) arrived within heartbeat x readSlack, - so a quiet-but-healthy broker no longer cycles the session offline at rest. -- Everything else `test/README.md` already lists: real infrastructure, a - production-hardened image (no Sigstore signing, no SBOM, no digest-pinned - base - see `connector/Dockerfile`'s header comment), scale, and any verb - beyond the five compiled ones. +`make build` uses `docker run golang:1.23-alpine` by default. If you have Go on the host, `GO=go make build` uses it directly. + +See `test/README.md` for details on the individual drills and the mock broker. + +## Supported backends + +| Backend | Status | Compiled verbs | +|---|---|---| +| **Perforce** (Helix Core) | Implemented | `p4.describe`, `p4.changes` | +| **TeamCity** | Implemented | `teamcity.server.info`, `teamcity.build.get` | +| **Jenkins** | Planned (reserved) | `jenkins.build.trigger` (denied in v0) | +| **GitHub Enterprise Server** | Planned (reserved) | `ghes.commit.get` (denied in v0) | +| **Horde** | Planned (reserved) | `horde.server.info` (denied in v0) | + +System verbs (`sys.ping`, `sys.version`, `sys.capabilities`) are always compiled and touch no studio tool. + +Reserved verbs are listed in the vocabulary so that the schema is self-documenting and the drills exercise their denial path, but they cannot be executed in this build. `p4.file_contents` is reserved as a content-class verb and is off at both the schema level and the config level. `teamcity.build.queue` is reserved as a mutation-class verb. No mutating verb and no content-class verb is compiled in v0. + +See `internal/vocab/vocab.go` for the full allowlist. + +## Protocol + +See [PROTOCOL.md](PROTOCOL.md) for the wire-level protocol details: transport, authentication, frame format, vocabulary resolution, and the audit log. diff --git a/docs/design-notes.md b/docs/design-notes.md new file mode 100644 index 0000000..5cab459 --- /dev/null +++ b/docs/design-notes.md @@ -0,0 +1,39 @@ +# Design notes + +This file preserves the design rationale and review history that was originally in the README when the connector lived inside the butter_stack monorepo as the issue #1575 spike. + +## Design sources + +Branch `plan/teamcity-private-reach` in ButterStack/butter_stack: +`ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md` sections 2.2-2.6, 4.3, 5, and +`ai/team/agents/shuri/reports/2026-08-29-connector-design-security-review.md` section 6. + +## What is in the vocabulary + +| Verb | v0 | +|---|---| +| `sys.ping`, `sys.version`, `sys.capabilities` | compiled | +| `teamcity.server.info` | compiled | +| `teamcity.build.get {build_id}` | compiled | +| `p4.describe {change, max_files, include_diff:false}` | compiled | +| `p4.changes {path, max}` | compiled | +| `teamcity.build.queue`, `jenkins.build.trigger` | reserved, denied | +| `p4.file_contents`, `ghes.commit.get`, `horde.server.info` | reserved, denied | + +No verb accepts a host, port, URL, or shell string. No verb accepts caller-supplied build parameters or properties, that is enforced structurally (`bannedArgNames` plus `Selfcheck()`, which runs at process start as well as in the tests), because a parameter map on a build-triggering verb interpolates into shell build steps and would make the allowlist a code-execution primitive inside the studio's LAN. No mutating verb and no content-class verb is compiled in. + +## What the spike does not prove + +Carried forward from design note section 5 and Shuri section 6 item 7, plus what the standalone shape adds: + +- **The argument-constraint layer end to end.** The drills prove denial at the frame boundary against a mock broker. They do not prove it against a real broker, a real TeamCity, or a real p4d. +- **Anything on the Rails side.** There is no `/connect` endpoint, no ActionCable change, no migration, no UI. The tenant-context drill ("assert tenant context is nil at the start of a request that follows a connector frame on the same Puma thread") is Rails-side and is not covered here. Only the broker-side half of drill (f) is. +- **Anything on real infrastructure.** Nothing ran against staging, demo, or production. No terraform, no security group, no hostname, no certificate. Stage A (the Tier 1 TeamCity webhook run) has not been run, and it is gated on the #1574 Phase -1 app fixes landing first; the "no token in `webhook_events.payload` or the app log" drill therefore has no result yet. +- **The frame codec against an independent production stack.** Both ends here were written from RFC 6455, the Go client and the Ruby server independently, which is why a masking or handshake mistake shows up as a failed drill. But neither has met a real ALB, a real nginx `Upgrade` hop, or a real proxy. +- **Latency over a home connection.** The drills run on loopback. The design's under-2-second target is untested against a NATed home network, and the `ss`/`netstat` capture showing exactly one outbound established connection and zero listeners has not been taken. +- **Survival conditions 1, 4, and 5.** No Sigstore keyless signing, no SBOM, no build-from-source instructions, no digest-pinned base image, no version-skew handling, and no `egress.md` with a per-verb output schema enforced as a field allowlist with a conformance test. The fixed `fields=` projections in the TeamCity executor are the beginning of that, not the whole of it. +- **Scale and multi-node routing.** Puma behaviour at tens of connectors, socket routing under a real ASG scale-out, and the per-integration connection cap and per-session command budget in the broker. +- **Everything beyond the five compiled verbs.** No Jenkins, GHES, or Horde verb; no Perforce verb beyond `describe` and `changes`; no mutating verb; no content verb; no poll-loop mode; no Windows service. +- **An actual IT-director review.** Appendix B is a script, not a test. + +This is the go/no-go input for the build, and it is deliberately smaller than the product. diff --git a/docs/uat.md b/docs/uat.md new file mode 100644 index 0000000..776060e --- /dev/null +++ b/docs/uat.md @@ -0,0 +1,53 @@ +# UAT + +`tests/uat/connector.spec.js` (in the ButterStack/butter_stack repo) is a second, Docker-based test of this daemon: TeamCity Tier 1 webhook intake into the real Rails app, and the compiled verbs, denials, and degradation drills against a containerized version of this connector, run inside a Docker-modelled "studio LAN" that is NOT reachable from the cloud side (outbound allowed, inbound blocked), which is the shape issue #1574/#1575 actually sell. + +```bash +docker-compose --profile core --profile connector up -d --build connector +npm run test:uat:connector +npm run test:uat:connector:keep-data # leaves the signup/project for inspection +``` + +**Always scope `--build` to `connector`.** `web`/`sidekiq` still carry a `build:` key in the base `docker-compose.yml` even though `docker-compose.uat-connector.yml` pins them to the prebuilt `image: butter_stack-web:latest`; an unscoped `--build` rebuilds and retags that shared image, which every worktree's dev stack uses. + +## Topology + +``` + "cloud" side (network: default) "studio LAN" (network: studio_lan, + internal: true -- no route out) + +-----------+ +--------------+ +----------------+ + | web | | mock-broker |<===wss:9443====>| connector | + | (Rails) | | (drill stand-| | (the daemon | + +-----+-----+ | in for the | | under test) | + ^ | real /connect| +--------+-------+ + | | endpoint) | | + | http :3000| | | (LAN-local) + +-----+---------+ +--------------+ | + | studio-egress |<===================studio_lan==============+ + | (socat, models| ^ + | outbound-only| +--------+---------+ + | firewall) | | teamcity-stub | + +---------------+ | (fake on-prem CI, | + | no published port)| + +--------------------+ +``` + +`internal: true` on `studio_lan` is the whole isolation guarantee. `web` cannot resolve or reach `teamcity-stub`; `connector` publishes no port at all (`docker inspect` shows an empty port map, and `netstat -tln` inside it lists nothing but Docker's own embedded DNS resolver); `teamcity-stub`'s only way to reach Rails is the one TCP port `studio-egress` forwards, which is the studio firewall's "outbound allowed" in miniature. `connector.spec.js` phase 100 asserts this shape directly before doing anything else. + +## What the UAT proves + +- The TeamCity Tier 1 webhook path both ways Teddy's design note describes: the curl-step flat payload (`X-Webhook-Token`) creates a `BuildRun` with `ci_provider == 'teamcity'`; TeamCity's own built-in webhook envelope authenticates and records a `WebhookEvent` but does not yet create a `BuildRun` (there is no adapter for the `{eventType, payload}` shape in `jenkins_controller.rb`, issue #1574 Phase 1, `normalize_ci_payload`). The spec documents this as the real current behavior; the assertion is written to flip the day that adapter lands. +- A real TeamCity 2026.1.3 server (captured on Ryan's LAN with a request logger) authenticates its built-in webhook with a standard `Authorization: Basic base64(username:password)` header built from `teamcity.internal.webhooks.username`/`.password`, and only when `.password` is declared as a plain-typed parameter. `teamcity-stub`'s `mode: 'native'` mirrors this. +- Neither webhook token nor its HTTP headers ever land in a persisted `WebhookEvent`. +- The connector's compiled vocabulary, denials (unknown verb, out-of-scope path, reserved-but-not-compiled verb, wrong argument type, a disabled content toggle, an unknown argument), and the no-shell proof on the p4 argv log, all against a real containerized daemon rather than the in-process drill harness. +- Broker-side refusals (query-string token, missing bearer token) before any session state exists, cross-session result discard, and the degradation drills (connector stop/start, token revoke/re-register), the same four rules `test/drills.rb` proves, exercised over the network instead of a Ruby method call. + +## What the UAT does not prove + +Everything `test/README.md`'s own "what this spike does not prove" section already says, plus: + +- **There is still no real `/connect` Rack endpoint.** `mock-broker` here is the same drill stand-in as `test/mock_broker.rb`, containerized (`test/mock_broker_server.rb`) with a small HTTP admin API bolted on so the Playwright test can drive sessions/refusals/revoke/partition without speaking the wire protocol itself. +- **No real TeamCity.** `teamcity-stub` (`test/support/teamcity_stub.rb`) is a hand-rolled fake that answers exactly the REST calls the connector makes and can fire the two outbound webhook shapes a real TeamCity delivers. +- **No real p4d.** `test/support/fake_p4` stands in for the `p4` CLI, same as in the drills. +- **Idle-connector liveness** is a WS-layer ping/pong rule: the heartbeat goroutine sends a WebSocket ping alongside the application-level heartbeat frame, and the read loop's deadline only means "reconnect" when no inbound frame of any kind (a pong included) arrived within heartbeat x readSlack, so a quiet-but-healthy broker no longer cycles the session offline at rest. +- Everything else `test/README.md` already lists: real infrastructure, a production-hardened image (no Sigstore signing, no SBOM, no digest-pinned base), scale, and any verb beyond the five compiled ones. From 3fd3d1882e401733b6763efba426ee3f1cf8067d Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Thu, 3 Sep 2026 21:13:03 -0400 Subject: [PATCH 09/11] docs(readme): link ButterStack and the sign-up page with UTM tags Co-Authored-By: Claude Fable 5.1 --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c988b8c..646f2ac 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # ButterStack Connector -An outbound-only daemon a game studio runs inside its own network so ButterStack can reach a private, on-premises Perforce, TeamCity, Jenkins, GitHub Enterprise Server, or Horde without the studio opening a single inbound port. One outbound TLS connection to one hostname on 443. A typed command allowlist, never a tunnel and never a shell. Credentials stay on the studio's disk and never cross the wire. +An outbound-only daemon a game studio runs inside its own network so [ButterStack](https://butterstack.com/?utm_source=github&utm_medium=readme&utm_campaign=butterstack-connector) can reach a private, on-premises Perforce, TeamCity, Jenkins, GitHub Enterprise Server, or Horde without the studio opening a single inbound port. One outbound TLS connection to one hostname on 443. A typed command allowlist, never a tunnel and never a shell. Credentials stay on the studio's disk and never cross the wire. Status: pre-release. Tracking: [ButterStack/butter_stack#1575](https://github.com/ButterStack/butter_stack/issues/1575). @@ -12,7 +12,7 @@ The connector is a daemon the studio runs on its own hardware (or in a container - Go 1.25 or later (for building from source) - Ruby 3.2+ (only for running the drill harness in `test/`) -- A ButterStack account with a connector token issued from the project's Connectors UI +- A [ButterStack account](https://butterstack.com/users/sign_up?utm_source=github&utm_medium=readme&utm_campaign=butterstack-connector) with a connector token issued from the project's Connectors UI ## Install From f32bbd9774ca8b453181411b10eaf00e86705859 Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Thu, 3 Sep 2026 21:15:55 -0400 Subject: [PATCH 10/11] docs: drop internal reviewer names and private repo paths before the repo goes public PROTOCOL.md and docs/ referred to internal review notes by the reviewer's agent name and by paths inside the private ButterStack repository. Those now read as "the design note" / "the security review" with the date, so the public text stands on its own. No technical content changed. Co-Authored-By: Claude Fable 5.1 --- PROTOCOL.md | 20 ++++++++++---------- docs/design-notes.md | 6 +++--- docs/uat.md | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/PROTOCOL.md b/PROTOCOL.md index d31c5e0..625691a 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -4,10 +4,10 @@ Status: **day-1 spike schema** for issue #1575 group 1. This document is the written-down form of checkbox group 0 (the day-1 protocol schema), which had to land before any verb did, because argument constraints *are* schema. -Sources: Devin's design note §2.2 and §2.4 -(`ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md`, -branch `plan/teamcity-private-reach`) and Shuri's must-fix list §6 -(`ai/team/agents/shuri/reports/2026-08-29-connector-design-security-review.md`). +Sources: the ButterStack team's design note §2.2 and §2.4 +(the ButterStack connector design note (2026-08-29, internal), +an internal planning branch) and the security review's must-fix list §6 +(the connector security review (2026-08-29, internal)). Two things this document does **not** do. It does not describe an endpoint that exists: the broker side is a later PR, and the only implementation of this @@ -36,7 +36,7 @@ wss:///connect ### The broker endpoint (stated here so the later PR inherits it) -Shuri §6 item 4, verbatim in effect: the broker is a **dedicated Rack endpoint +security review §6 item 4, verbatim in effect: the broker is a **dedicated Rack endpoint at `/connect`. Not ActionCable. Not `/cable`. Not `ApplicationCable::Connection`. No change to `allowed_request_origins`. No `disable_request_forgery_protection`.** @@ -204,7 +204,7 @@ never appear on the wire. `integration_id` is the one that issued the command. Reply routing is derived **server-side** from the authenticated session, never from a field in a frame. -### Tenant scoping for `Connector.call` (Shuri §6 item 5) +### Tenant scoping for `Connector.call` (security review §6 item 5) `config/initializers/acts_as_tenant.rb:8` sets `require_tenant = false`, so a missed scope returns cross-tenant rows *silently* instead of raising. Therefore: @@ -254,12 +254,12 @@ readable. `butterstack-connector -print-vocabulary` prints it. A well-formed verb with an out-of-scope argument is denied **exactly like an unknown verb**, and both write a local audit line. Only the second kind of denial actually tests survival condition 2; the first only tests the dispatcher. -Both are drilled separately for that reason (Shuri §6 item 7b). +Both are drilled separately for that reason (security review §6 item 7b). ### No caller-supplied trigger parameters. Ever, in v0. This is the finding that made this layer day-1 work rather than v1 polish -(Shuri F4). `allowed_jobs` constrains *which* job runs; a `params` map is +(security review F4). `allowed_jobs` constrains *which* job runs; a `params` map is unconstrained, and Jenkins build parameters and TeamCity properties are interpolated into shell build steps by design, including in our own `Jenkinsfile.unreal` and `Jenkinsfile.minimobile` templates. A caller-supplied @@ -289,7 +289,7 @@ and at process start. A build whose vocabulary grew one of them refuses to run. ### Perforce invocation If the connector shells out to the `p4` CLI rather than using P4Ruby, **every -invocation is an argv array with no shell interpretation** (Shuri F5's smaller +invocation is an argv array with no shell interpretation** (security review F5's smaller sibling). The ticket is passed through `P4PASSWD` in a minimal environment rather than on the command line, so it never appears in the studio host's process list. `p4 describe` is always invoked with `-s`, so diffs are excluded @@ -318,7 +318,7 @@ v0**, and `Selfcheck()` fails the build if one ever is. (`teamcity.server.info`, `teamcity.build.get`, `p4.describe`). It is in the design note's own §2.4 list, and it is compiled in because it is the natural carrier for `depot_scope`: without a path-bearing argument, the out-of-scope -drill has nothing to deny before a tool call, and that drill is the one Shuri +drill has nothing to deny before a tool call, and that drill is the one the security review singled out as the only real test of condition 2. --- diff --git a/docs/design-notes.md b/docs/design-notes.md index 5cab459..2b21670 100644 --- a/docs/design-notes.md +++ b/docs/design-notes.md @@ -5,8 +5,8 @@ This file preserves the design rationale and review history that was originally ## Design sources Branch `plan/teamcity-private-reach` in ButterStack/butter_stack: -`ai/team/agents/devin/runbooks/2026-08-29-private-instance-reach-connector-design.md` sections 2.2-2.6, 4.3, 5, and -`ai/team/agents/shuri/reports/2026-08-29-connector-design-security-review.md` section 6. +the ButterStack connector design note (2026-08-29, internal) sections 2.2-2.6, 4.3, 5, and +the connector security review (2026-08-29, internal) section 6. ## What is in the vocabulary @@ -24,7 +24,7 @@ No verb accepts a host, port, URL, or shell string. No verb accepts caller-suppl ## What the spike does not prove -Carried forward from design note section 5 and Shuri section 6 item 7, plus what the standalone shape adds: +Carried forward from design note section 5 and security review section 6 item 7, plus what the standalone shape adds: - **The argument-constraint layer end to end.** The drills prove denial at the frame boundary against a mock broker. They do not prove it against a real broker, a real TeamCity, or a real p4d. - **Anything on the Rails side.** There is no `/connect` endpoint, no ActionCable change, no migration, no UI. The tenant-context drill ("assert tenant context is nil at the start of a request that follows a connector frame on the same Puma thread") is Rails-side and is not covered here. Only the broker-side half of drill (f) is. diff --git a/docs/uat.md b/docs/uat.md index 776060e..9bcb595 100644 --- a/docs/uat.md +++ b/docs/uat.md @@ -36,7 +36,7 @@ npm run test:uat:connector:keep-data # leaves the signup/project for inspectio ## What the UAT proves -- The TeamCity Tier 1 webhook path both ways Teddy's design note describes: the curl-step flat payload (`X-Webhook-Token`) creates a `BuildRun` with `ci_provider == 'teamcity'`; TeamCity's own built-in webhook envelope authenticates and records a `WebhookEvent` but does not yet create a `BuildRun` (there is no adapter for the `{eventType, payload}` shape in `jenkins_controller.rb`, issue #1574 Phase 1, `normalize_ci_payload`). The spec documents this as the real current behavior; the assertion is written to flip the day that adapter lands. +- The TeamCity Tier 1 webhook path both ways the design note describes: the curl-step flat payload (`X-Webhook-Token`) creates a `BuildRun` with `ci_provider == 'teamcity'`; TeamCity's own built-in webhook envelope authenticates and records a `WebhookEvent` but does not yet create a `BuildRun` (there is no adapter for the `{eventType, payload}` shape in `jenkins_controller.rb`, issue #1574 Phase 1, `normalize_ci_payload`). The spec documents this as the real current behavior; the assertion is written to flip the day that adapter lands. - A real TeamCity 2026.1.3 server (captured on Ryan's LAN with a request logger) authenticates its built-in webhook with a standard `Authorization: Basic base64(username:password)` header built from `teamcity.internal.webhooks.username`/`.password`, and only when `.password` is declared as a plain-typed parameter. `teamcity-stub`'s `mode: 'native'` mirrors this. - Neither webhook token nor its HTTP headers ever land in a persisted `WebhookEvent`. - The connector's compiled vocabulary, denials (unknown verb, out-of-scope path, reserved-but-not-compiled verb, wrong argument type, a disabled content toggle, an unknown argument), and the no-shell proof on the p4 argv log, all against a real containerized daemon rather than the in-process drill harness. From edf1533958aa3157a3258c87484d9cf78a336cbd Mon Sep 17 00:00:00 2001 From: Ryan L'Italien Date: Thu, 3 Sep 2026 21:16:21 -0400 Subject: [PATCH 11/11] docs: last private-branch reference in design-notes Co-Authored-By: Claude Fable 5.1 --- docs/design-notes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design-notes.md b/docs/design-notes.md index 2b21670..f1cc024 100644 --- a/docs/design-notes.md +++ b/docs/design-notes.md @@ -4,7 +4,7 @@ This file preserves the design rationale and review history that was originally ## Design sources -Branch `plan/teamcity-private-reach` in ButterStack/butter_stack: +Origin: an internal ButterStack planning branch (2026-08-29): the ButterStack connector design note (2026-08-29, internal) sections 2.2-2.6, 4.3, 5, and the connector security review (2026-08-29, internal) section 6.