From 8beafc5aa23b7e67d05e8fe43de33ed3434517b3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 31 Oct 2026 00:25:58 +0000 Subject: [PATCH 1/2] Add opt-in organization Copilot requests token broker with Key Vault credentials Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/cao.json | 5 + .github/workflows/shared/cao.schema.json | 38 +- .github/workflows/shared/policy.mjs | 36 +- ARCHITECTURE.md | 15 + CODEBASE.yml | 3 + server/SECURITY.md | 97 ++++ server/internal/admission/admission_test.go | 16 + server/internal/admission/identity.go | 4 + server/internal/server/azure.go | 6 + server/internal/server/hosted.go | 7 + server/internal/server/server.go | 31 +- server/internal/server/token_broker.go | 110 +++++ server/internal/server/token_broker_test.go | 189 +++++++ server/internal/tokenbroker/issuer.go | 142 ++++++ server/internal/tokenbroker/issuer_test.go | 239 +++++++++ server/internal/tokenbroker/policy.go | 102 ++++ server/internal/tokenbroker/policy_test.go | 197 ++++++++ server/internal/tokenbroker/protocol.go | 147 ++++++ server/internal/tokenbroker/protocol_test.go | 25 + server/internal/tokenbroker/service.go | 354 +++++++++++++ server/internal/tokenbroker/service_test.go | 492 +++++++++++++++++++ server/internal/tokenbroker/vault.go | 196 ++++++++ server/internal/tokenbroker/vault_test.go | 267 ++++++++++ server/spec/README.md | 6 +- server/spec/contract.test.mjs | 16 +- server/spec/generated/openapi.json | 296 ++++++++++- server/spec/main.tsp | 3 +- server/spec/token-broker.tsp | 55 +++ tests/unit/control-token-broker.test.mjs | 172 +++++++ 29 files changed, 3255 insertions(+), 11 deletions(-) create mode 100644 server/internal/server/token_broker.go create mode 100644 server/internal/server/token_broker_test.go create mode 100644 server/internal/tokenbroker/issuer.go create mode 100644 server/internal/tokenbroker/issuer_test.go create mode 100644 server/internal/tokenbroker/policy.go create mode 100644 server/internal/tokenbroker/policy_test.go create mode 100644 server/internal/tokenbroker/protocol.go create mode 100644 server/internal/tokenbroker/protocol_test.go create mode 100644 server/internal/tokenbroker/service.go create mode 100644 server/internal/tokenbroker/service_test.go create mode 100644 server/internal/tokenbroker/vault.go create mode 100644 server/internal/tokenbroker/vault_test.go create mode 100644 server/spec/token-broker.tsp create mode 100644 tests/unit/control-token-broker.test.mjs diff --git a/.github/workflows/cao.json b/.github/workflows/cao.json index dfd5c5ab4b..890fd7f650 100644 --- a/.github/workflows/cao.json +++ b/.github/workflows/cao.json @@ -3,6 +3,11 @@ "version": 1, "gh-aw-version": "v1.0.1", "control-plane": { + "token-broker": { + "copilot-requests": { + "workflows": [] + } + }, "scope": { "allowed-owners": [ "githubnext", diff --git a/.github/workflows/shared/cao.schema.json b/.github/workflows/shared/cao.schema.json index d900e39f86..240e00a685 100644 --- a/.github/workflows/shared/cao.schema.json +++ b/.github/workflows/shared/cao.schema.json @@ -664,6 +664,41 @@ "queue": { "type": "object", "description": "Strict bounded QueueOptions contract validated by the gateway at startup; see RFC-033." } } }, + "tokenBroker": { + "type": "object", + "description": "Credential authorization only, independent of global rollout scope; does not grant write or live capabilities. Omission disables the broker.", + "additionalProperties": false, + "required": ["copilot-requests"], + "properties": { + "copilot-requests": { + "type": "object", + "additionalProperties": false, + "required": ["workflows"], + "properties": { + "workflows": { + "type": "array", + "description": "Exact approved workflow identities. An empty list authorizes nothing.", + "maxItems": 128, + "uniqueItems": true, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["repository-id", "owner-id", "workflow-ref", "workflow-sha"], + "properties": { + "repository-id": { "type": "string", "pattern": "^[1-9][0-9]{0,19}(?![\\s\\S])" }, + "owner-id": { "type": "string", "pattern": "^[1-9][0-9]{0,19}(?![\\s\\S])" }, + "workflow-ref": { + "type": "string", + "pattern": "^(?!.*\\.\\.)[A-Za-z0-9][A-Za-z0-9-]*/[A-Za-z0-9][A-Za-z0-9._-]*/\\.github/workflows/[A-Za-z0-9_-][A-Za-z0-9._-]*\\.ya?ml@refs/(?:heads|tags)/(?!.*\\.lock(?:/|$))[A-Za-z0-9_-]+(?:[./][A-Za-z0-9_-]+)*(?![\\s\\S])" + }, + "workflow-sha": { "type": "string", "pattern": "^[a-f0-9]{40}(?![\\s\\S])" } + } + } + } + } + } + } + }, "controlPlane": { "type": "object", "additionalProperties": false, @@ -678,7 +713,8 @@ "operations": { "$ref": "#/$defs/controlOperations" }, "publishing": { "$ref": "#/$defs/publishing" }, "marketplace": { "$ref": "#/$defs/marketplace" }, - "webhook-gateway": { "$ref": "#/$defs/webhookGateway" } + "webhook-gateway": { "$ref": "#/$defs/webhookGateway" }, + "token-broker": { "$ref": "#/$defs/tokenBroker" } } }, "targetCampaign": { diff --git a/.github/workflows/shared/policy.mjs b/.github/workflows/shared/policy.mjs index 236a81ee9a..61af4d4971 100644 --- a/.github/workflows/shared/policy.mjs +++ b/.github/workflows/shared/policy.mjs @@ -5,7 +5,11 @@ export { AuthorityError as PolicyError } from "./authority.mjs"; const SCHEMA_URI = "https://raw.githubusercontent.com/githubnext/gh-cao/main/.github/workflows/shared/cao.schema.json"; const ROOT_KEYS = ["$schema", "version", "gh-aw-version", "control-plane", "target-authority"]; -const CONTROL_KEYS = ["scope", "activity", "inventory", "web", "defaults", "campaigns", "workflows", "operations", "publishing", "marketplace", "webhook-gateway"]; +const CONTROL_KEYS = ["scope", "activity", "inventory", "web", "defaults", "campaigns", "workflows", "operations", "publishing", "marketplace", "webhook-gateway", "token-broker"]; +const TOKEN_BROKER_WORKFLOW_KEYS = ["repository-id", "owner-id", "workflow-ref", "workflow-sha"]; +const TOKEN_BROKER_ID_PATTERN = /^[1-9][0-9]{0,19}(?![\s\S])/; +const TOKEN_BROKER_SHA_PATTERN = /^[a-f0-9]{40}(?![\s\S])/; +const TOKEN_BROKER_WORKFLOW_REF_PATTERN = /^(?!.*\.\.)[A-Za-z0-9][A-Za-z0-9-]*\/[A-Za-z0-9][A-Za-z0-9._-]*\/\.github\/workflows\/[A-Za-z0-9_-][A-Za-z0-9._-]*\.ya?ml@refs\/(?:heads|tags)\/(?!.*\.lock(?:\/|$))[A-Za-z0-9_-]+(?:[./][A-Za-z0-9_-]+)*(?![\s\S])/; const SCOPE_KEYS = ["allowed-owners", "allowed-repositories"]; const ACTIVITY_KEYS = ["repositories", "public-only"]; const INVENTORY_KEYS = ["max-scan-repositories", "cell-count", "cell-index", "batch-size", "batch-index"]; @@ -294,6 +298,36 @@ function validateControlPlane(control) { if ("publishing" in control) validatePublishing(control.publishing); if ("marketplace" in control) validateMarketplace(control.marketplace); if ("webhook-gateway" in control) validateWebhookGateway(control["webhook-gateway"], control.scope); + if ("token-broker" in control) validateTokenBroker(control["token-broker"]); +} + +function validateTokenBroker(broker) { + const path = "control-plane.token-broker"; + assertMapping(broker, path); + assertKeys(broker, ["copilot-requests"], path); + const requests = broker["copilot-requests"]; + assertMapping(requests, `${path}.copilot-requests`); + assertKeys(requests, ["workflows"], `${path}.copilot-requests`); + const workflows = requests.workflows; + if (!Array.isArray(workflows) || workflows.length > 128) { + throw new PolicyError(`${path}.copilot-requests.workflows must contain at most 128 entries`); + } + const identities = new Set(); + for (const [index, workflow] of workflows.entries()) { + const entryPath = `${path}.copilot-requests.workflows[${index}]`; + assertMapping(workflow, entryPath); + assertKeys(workflow, TOKEN_BROKER_WORKFLOW_KEYS, entryPath); + for (const key of ["repository-id", "owner-id"]) { + assertString(workflow[key], `${entryPath}.${key}`, TOKEN_BROKER_ID_PATTERN); + } + assertString(workflow["workflow-ref"], `${entryPath}.workflow-ref`, TOKEN_BROKER_WORKFLOW_REF_PATTERN); + assertString(workflow["workflow-sha"], `${entryPath}.workflow-sha`, TOKEN_BROKER_SHA_PATTERN); + const identity = JSON.stringify(TOKEN_BROKER_WORKFLOW_KEYS.map(key => workflow[key])); + if (identities.has(identity)) { + throw new PolicyError(`${path}.copilot-requests.workflows must contain unique entries`); + } + identities.add(identity); + } } function validateWebhookGateway(gateway, scope) { diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 9c388c263f..45e979e334 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -544,6 +544,21 @@ state and available history, not every missed event or expired artifact. There is no memory disk journal, automatic failover, or dual-write path. `specs/server-operational-storage.md` specifies these guarantees. +The separately opt-in organization Copilot requests broker +(`server/internal/tokenbroker/`, `POST /api/token-broker`) exchanges GitHub +Actions OIDC identities for dedicated organization App installation tokens with +only `copilot_requests: read`. Its protected +`control-plane.token-broker.copilot-requests.workflows` policy pins immutable +owner/repository IDs, workflow reference and revision; signed job context +isolates issuance and invalidation. It never grants rollout, admission, or +repository-write authority and does not implement Copilot tasks. Private keys +are read from Azure Key Vault using managed identity, never from policy or +caller inputs. This initial HTTPS, process-owned, explicitly single-replica +profile keeps a bounded process-local credential registry for revocation; +crashes can lose cleanup state until upstream expiry, bounded to one hour. +Platform/external listeners and overlapping replicas are unsupported. Live +Copilot inference, renewal and cleanup acceptance remains operator-owned. + The optional hosted activation-admission service (`server/internal/admission/`) implements gh-aw's separate OIDC-only `POST /api/admission` contract, never CAO rollout authority. Explicit server configuration selects a protected, atomically diff --git a/CODEBASE.yml b/CODEBASE.yml index bb1b9cd509..de6c8673cd 100644 --- a/CODEBASE.yml +++ b/CODEBASE.yml @@ -130,6 +130,9 @@ components: actions_credentials: path: activity/authentication.mjs responsibility: Actions-only credential profiles, owner-scoped naming and bounded Activity collection planning; native gh cao setup auth exclusively owns operator provisioning + copilot_requests_broker: + path: server/internal/tokenbroker/ + responsibility: opt-in HTTPS OIDC exchange for dedicated organization App copilot_requests read tokens; protected cao.json pins immutable owner/repository IDs and workflow reference/revision, signed job context isolates issuance/invalidation, Azure Key Vault managed identity supplies private keys; grants no rollout, admission, repository-write or Copilot task authority; bounded process-local revocation registry requires one explicitly acknowledged process replica, rejects platform/external listeners and overlapping replicas, and can lose cleanup after crashes until upstream expiry bounded to one hour; live inference/renewal/cleanup acceptance remains operator-owned dashboard: path: dashboard/ responsibility: publish evidence and build the shared operational dashboard UI for either Pages or the hosted Go server diff --git a/server/SECURITY.md b/server/SECURITY.md index ffb2bfbe9f..c8c029fb06 100644 --- a/server/SECURITY.md +++ b/server/SECURITY.md @@ -34,6 +34,103 @@ normative [hosted CLI actions specification](../specs/server-hosted-cli.md). ## Supported security boundary +### Optional Copilot requests broker + +The opt-in `/api/token-broker` service has a separate Actions OIDC boundary; +dashboard sessions, API bearers and admission decisions never authorize +issuance. Its protected `cao.json` allowlist pins immutable owner/repository +IDs and exact workflow references and revisions. It checks signed execution +and job evidence, denies fork/pull-request and reusable execution, and supports +only `copilot-request` with the trusted `github` provider. Unsupported tasks do +not trigger minting. + +A dedicated organization App's private key is read from Azure Key Vault using +managed identity, never from policy or caller inputs. GitHub installation +identity, organization ownership and least-privilege `copilot_requests: read` +are checked before minting. Issuance is billable organization access, not +read-only financial behavior. GitHub Copilot organization budgets must be +configured separately; CAO admission budgets are not a broker spending cap. + +Request sizes, upstream responses, timeouts, in-flight issuance, retained +credentials and rate subjects are bounded. Redirects and caller-selected +upstream destinations are forbidden. Never capture broker bodies, OIDC tokens, +App JWTs, Key Vault secrets or returned credentials in logs, traces or proxies. +Failures have empty no-store responses without upstream diagnostics. + +The initial broker requires one explicitly acknowledged process replica. +Job invalidation and graceful shutdown revoke tracked tokens, but a crash loses +the process-local revocation registry. Installation tokens can remain valid +until their actual expiry, bounded to one hour. No durable cleanup, +multi-replica operation, Azure Functions hosting, or integrated-deployment +certification is claimed. + +#### Broker enrollment and setup + +The organization permission is **Copilot Requests: Read-only** +(`copilot_requests: read`); despite its name it authorizes billable AI requests. +Use a dedicated organization-installed App, not a personal/enterprise +installation or the collector App. Review the +[GitHub organization App access documentation](https://docs.github.com/en/enterprise-cloud@latest/copilot/how-tos/administer-copilot/manage-for-organization/manage-app-copilot-access) +and enable organization spending controls before activation. + +The checked-in `control-plane.token-broker.copilot-requests.workflows` list in +`cao.json` is empty: no workflow is enrolled by this change. Each enrollment +requires exact `repository-id`, `owner-id`, `workflow-ref`, and `workflow-sha` +strings. The reference includes the repository, workflow filename and branch/tag, +for example `acme/project/.github/workflows/triage.lock.yml@refs/heads/main`; +the SHA is the 40-character lowercase commit SHA of that workflow revision. +Wildcards are not supported. Protect the mounted policy from untrusted writes +and serve the reviewed policy snapshot corresponding to enrolled workflow +revisions. Update the pin only after reviewing a new revision. Broker +authorization is independent of admission, rollout mode and repository writes. + +Configure these server environment settings; none contains a private key: + +| Setting | Required value | +| --- | --- | +| `CAO_TOKEN_BROKER_ENABLED` | `true` (otherwise disabled) | +| `CAO_TOKEN_BROKER_SINGLE_PROCESS` | `true`, acknowledging one process/replica and volatile cleanup state | +| `CAO_TOKEN_BROKER_URL` | Exact public `https://HOST/api/token-broker`, also the OIDC audience | +| `CAO_TOKEN_BROKER_APP_ID` | Dedicated GitHub App's numeric ID | +| `CAO_TOKEN_BROKER_INSTALLATION_ID` | Organization installation's numeric ID | +| `CAO_TOKEN_BROKER_OWNER_ID` | Organization's immutable numeric ID | +| `CAO_TOKEN_BROKER_KEY_VAULT_SECRET_URL` | `https://VAULT.vault.azure.net/secrets/SECRET`, optionally with a version | +| `CAO_TOKEN_BROKER_MANAGED_IDENTITY_CLIENT_ID` | Optional user-assigned managed identity client ID | +| `CAO_POLICY_PATH` | Protected control-repository `cao.json` path | + +Store the App's PEM private key as an Azure Key Vault **secret**, provisioned by +an operator outside the server. Grant the server's managed identity secret-read +access only to that secret (for example, Azure RBAC Key Vault Secrets User at +the narrowest supported scope). App Service managed identity and Azure IMDS +are supported; the runtime never provisions a vault, writes keys, or accepts a +PEM through environment variables, workflow inputs, or request bodies. +An unversioned secret URL reads the current version on each mint, supporting +rotation without persisting keys in CAO. Use a private endpoint/firewall where +appropriate and audit secret reads without capturing values. + +Run through hosted HTTPS with the reviewed allowed host and one process-owned +listener. Azure Functions, externally owned listeners, and multiple replicas +are rejected for this initial profile. Do not autoscale, overlap deployments, +or enable broker request/response-body capture in a reverse proxy or APM system. +Only explicitly trusted push, schedule and manual-dispatch execution is +supported; pull requests, reusable workflows, environment-based subjects and +identities lacking signed `check_run_id` job evidence fail closed. +Cleanup uses gh-aw's fresh-OIDC `invalidate` request to revoke only that job's +issued tokens; graceful shutdown also attempts revocation. A crash can lose +cleanup state while GitHub tokens remain usable until their actual expiry +(at most one hour). This is not a durable/multi-replica broker. + +For compatible gh-aw releases, opt in with `token-broker.copilot-request: true` +and set `GH_AW_TOKEN_BROKER_URL` to that exact endpoint. Do not also set +`permissions.copilot-requests: write`. As of the upstream experimental +[broker specification](https://github.com/github/gh-aw/blob/main/docs/src/content/docs/specs/token-broker-specification.md), +bounded threat-detection/evals consumers are wired; main-agent inference still +requires renewable AWF credential support. Existing compiled workflows are not +changed here. Before claiming integrated support, test real GitHub OIDC +exchange, authorized Copilot inference, renewal, key rotation and job-scoped +invalidation with your selected gh-aw version and organization entitlements. +Offline tests or a token-shaped response do not establish that acceptance. + The supported deployment is: - one trusted operator; diff --git a/server/internal/admission/admission_test.go b/server/internal/admission/admission_test.go index 9ada6c9fd8..58f49ddcaa 100644 --- a/server/internal/admission/admission_test.go +++ b/server/internal/admission/admission_test.go @@ -92,6 +92,22 @@ func (f *fixture) token(t *testing.T, claims jwt.MapClaims) string { type policyFunc func(context.Context, Identity) (Allowances, error) +func TestVerifierExposesSignedBrokerJobClaimsWithoutChangingAdmissionProfile(t *testing.T) { + f := newFixture(t) + f.claims["check_run_id"] = "987654321" + identity, err := f.verifier.Verify(context.Background(), f.token(t, f.claims)) + if err != nil || identity.CheckRunID != "987654321" || identity.EnvironmentPresent { + t.Fatal("signed job claim was not preserved") + } + key := identity.Key() + f.claims["check_run_id"] = "987654322" + f.claims["environment"] = "production" + identity, err = f.verifier.Verify(context.Background(), f.token(t, f.claims)) + if err != nil || identity.CheckRunID != "987654322" || !identity.EnvironmentPresent || identity.Key() != key { + t.Fatal("broker claim extraction changed admission identity semantics") + } +} + func (f policyFunc) Evaluate(ctx context.Context, i Identity) (Allowances, error) { return f(ctx, i) } type decisionsFunc func(context.Context, string, *Allowances) (Allowances, error) diff --git a/server/internal/admission/identity.go b/server/internal/admission/identity.go index 86a8d7ed63..dc3ee6f4e6 100644 --- a/server/internal/admission/identity.go +++ b/server/internal/admission/identity.go @@ -26,6 +26,8 @@ type Identity struct { Issuer, Subject, Repository, RepositoryID, OwnerID string RunID, RunAttempt, Ref, Event, Actor, ActorID string WorkflowRef, WorkflowSHA string + CheckRunID string + EnvironmentPresent bool } func (i Identity) Key() string { @@ -134,7 +136,9 @@ func (v *Verifier) Verify(ctx context.Context, raw string) (Identity, error) { RunID: get("run_id"), RunAttempt: get("run_attempt"), Ref: get("ref"), Event: get("event_name"), Actor: get("actor"), ActorID: get("actor_id"), WorkflowRef: get("workflow_ref"), WorkflowSHA: get("workflow_sha"), + CheckRunID: get("check_run_id"), } + _, i.EnvironmentPresent = claims["environment"] if !repository.MatchString(i.Repository) || i.Subject == "" || i.Ref == "" || i.Event == "" || !clean(i.Actor, 100) || !decimal.MatchString(i.RepositoryID) || !decimal.MatchString(i.OwnerID) || !decimal.MatchString(i.ActorID) || diff --git a/server/internal/server/azure.go b/server/internal/server/azure.go index aee3ffe093..56e53e3478 100644 --- a/server/internal/server/azure.go +++ b/server/internal/server/azure.go @@ -310,8 +310,14 @@ func NewAzureFunctionsHandlerFromEnv(ctx context.Context, siteDirectory, dashboa _ = database.Close() return nil, err } + brokerConfig, err := tokenBrokerConfigFromEnv() + if err != nil { + _ = database.Close() + return nil, err + } app, err := New(ctx, store, Config{ Admission: admissionConfig, + TokenBroker: brokerConfig, Database: database, Audit: auditLog, DatabaseQueriesPath: databaseQueries, diff --git a/server/internal/server/hosted.go b/server/internal/server/hosted.go index ab245e7aa8..979c549c14 100644 --- a/server/internal/server/hosted.go +++ b/server/internal/server/hosted.go @@ -142,8 +142,14 @@ func newHostedAppWithPolicy( _ = database.Close() return nil, err } + brokerConfig, err := tokenBrokerConfigFromEnv() + if err != nil { + _ = database.Close() + return nil, err + } config := Config{ Admission: admissionConfig, + TokenBroker: brokerConfig, Database: database, Audit: auditLog, HostProfile: host.Profile, @@ -186,6 +192,7 @@ func newHostedAppWithPolicy( }, Logger: logger, } + config.TokenBrokerSingleProcessConfirmed = os.Getenv("CAO_TOKEN_BROKER_SINGLE_PROCESS") == "true" app, err := New(ctx, store, config) if err != nil { _ = database.Close() diff --git a/server/internal/server/server.go b/server/internal/server/server.go index 8e3f91241d..617f6a4261 100644 --- a/server/internal/server/server.go +++ b/server/internal/server/server.go @@ -50,6 +50,7 @@ import ( "github.com/githubnext/gh-cao/server/internal/sessionarchiveblob" "github.com/githubnext/gh-cao/server/internal/sqlitex" "github.com/githubnext/gh-cao/server/internal/telemetry" + "github.com/githubnext/gh-cao/server/internal/tokenbroker" ) var serverLog = logger.New("cao:server") @@ -97,6 +98,7 @@ type Config struct { // operations; nil disables them (local mode and tests). Audit *audit.Logger Admission *AdmissionConfig + TokenBroker *tokenbroker.Config ADX *adxingest.Config ADXTraces *adxtrace.Config SessionArchive *sessionarchive.Config @@ -109,6 +111,10 @@ type Config struct { // RedisMaxBytes bounds cache pressure against whole-node memory usage. // Zero selects CAO_REDIS_MAX_BYTES or the 200 MB default. RedisMaxBytes int64 + + // TokenBrokerSingleProcessConfirmed acknowledges volatile credential cleanup + // and prohibits multiple or overlapping broker process replicas. + TokenBrokerSingleProcessConfirmed bool } type App struct { @@ -149,6 +155,7 @@ type App struct { hostedCLI *hostedCLI valuePublisher operationalValuePublisher admission *admission.Service + tokenBroker *tokenbroker.Service adx *adxingest.Worker adxTraces *adxtrace.Worker sessionArchive *sessionArchiveService @@ -255,6 +262,10 @@ func New(ctx context.Context, store operational.Store, config Config) (*App, err if err != nil { return nil, err } + broker, err := newTokenBroker(config) + if err != nil { + return nil, err + } info, err := os.Stat(config.SiteDirectory) if err != nil || !info.IsDir() { return nil, errors.New("site directory must exist and contain the built dashboard") @@ -347,9 +358,10 @@ func New(ctx context.Context, store operational.Store, config Config) (*App, err } serverLog.Printf("initialized host_profile=%s oauth=%t source_ingestion=%t", profile.Name, oauth != nil, config.SourceDirectory != "") app := &App{ - admission: activationAdmission, - services: services, - database: config.Database, databaseQueries: databaseQueries, config: config, accessToken: accessToken, oauth: oauth, hub: newEventHub(), + admission: activationAdmission, + tokenBroker: broker, + services: services, + database: config.Database, databaseQueries: databaseQueries, config: config, accessToken: accessToken, oauth: oauth, hub: newEventHub(), canonical: canonicalService{store: config.Database, definitions: databaseQueries}, reconciler: reconciler, memory: memoryResolver, webhookSecret: []byte(config.WebhookSecret), actionsToken: actionsToken, actionsActor: actionsActor, quota: quota, drain: make(chan struct{}), @@ -572,6 +584,10 @@ func (a *App) Stop(ctx context.Context) error { return err } } + var brokerErr error + if a.tokenBroker != nil { + brokerErr = a.tokenBroker.Close(ctx) + } if a.ownedDatabase != nil || a.ownedOperational != nil || a.sessionArchive != nil { a.closeDatabase.Do(func() { if a.sessionArchive != nil { @@ -584,9 +600,9 @@ func (a *App) Stop(ctx context.Context) error { a.closeError = errors.Join(a.closeError, a.ownedOperational.Close()) } }) - return a.closeError + return errors.Join(a.closeError, brokerErr) } - return nil + return brokerErr } func (a *App) waitTasks(ctx context.Context) error { @@ -753,6 +769,7 @@ func (a *App) Handler() http.Handler { register("POST /api/v1/value", a.publishValue) register("POST /api/admission", a.activationAdmission) register("POST /api/conclusion", a.conclusionAdmission) + register("POST /api/token-broker", a.exchangeBrokerToken) if a.mcp != nil { register("POST /mcp", a.mcp.ServeHTTP) } @@ -788,6 +805,10 @@ func (a *App) Handler() http.Handler { } dashboardHandler := handler handler = http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) { + if request.URL.Path == tokenBrokerPath { + a.exchangeBrokerToken(response, request) + return + } if admissionPath(request.URL.Path) { route := semconv.HTTPRoute(request.URL.Path) trace.SpanFromContext(request.Context()).SetAttributes(route) diff --git a/server/internal/server/token_broker.go b/server/internal/server/token_broker.go new file mode 100644 index 0000000000..224bcc1ccf --- /dev/null +++ b/server/internal/server/token_broker.go @@ -0,0 +1,110 @@ +package server + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "net/http" + "net/url" + "os" + "strconv" + "strings" + "time" + + "github.com/githubnext/gh-cao/server/internal/tokenbroker" +) + +const tokenBrokerPath = "/api/token-broker" + +func tokenBrokerConfigFromEnv() (*tokenbroker.Config, error) { + const prefix = "CAO_TOKEN_BROKER_" + enabled := os.Getenv(prefix + "ENABLED") + if enabled == "" || enabled == "false" { + return nil, nil //nolint:nilnil // This optional service is disabled unless explicitly selected. + } + if enabled != "true" { + return nil, errors.New("CAO_TOKEN_BROKER_ENABLED must be true or false") + } + appID, appErr := strconv.ParseInt(os.Getenv(prefix+"APP_ID"), 10, 64) + installationID, installationErr := strconv.ParseInt(os.Getenv(prefix+"INSTALLATION_ID"), 10, 64) + if appErr != nil || installationErr != nil || appID <= 0 || installationID <= 0 { + return nil, errors.New("token broker requires positive App and installation IDs") + } + return &tokenbroker.Config{ + Audience: os.Getenv(prefix + "URL"), PolicyFile: configuredHostPolicyPath(), + AppID: appID, InstallationID: installationID, OwnerID: os.Getenv(prefix + "OWNER_ID"), + KeyVaultSecretURL: os.Getenv(prefix + "KEY_VAULT_SECRET_URL"), + ManagedIdentityClientID: os.Getenv(prefix + "MANAGED_IDENTITY_CLIENT_ID"), + }, nil +} + +func newTokenBroker(config Config) (*tokenbroker.Service, error) { + if config.TokenBroker == nil { + return nil, nil //nolint:nilnil // Absence must not enable a credential issuer. + } + if config.HostProfile.Authentication != HostAuthenticationOAuth || !config.Proxy.RequireHTTPS || + config.HostProfile.Listener != HostListenerProcess || !config.SingleReplicaConfirmed || + !config.TokenBrokerSingleProcessConfirmed { + return nil, errors.New("token broker requires hosted HTTPS and a confirmed single process replica") + } + endpoint, err := url.Parse(config.TokenBroker.Audience) + if err != nil || endpoint.Scheme != "https" || endpoint.Hostname() == "" || endpoint.User != nil || + endpoint.Path != tokenBrokerPath || endpoint.RawPath != "" || endpoint.RawQuery != "" || + endpoint.ForceQuery || endpoint.Fragment != "" || endpoint.Port() != "" || + strings.ContainsAny(config.TokenBroker.Audience, "\\ \t\r\n") { + return nil, errors.New("token broker URL must be the exact public HTTPS /api/token-broker endpoint") + } + allowed := false + for _, host := range config.Proxy.AllowedHosts { + if strings.EqualFold(host, endpoint.Hostname()) { + allowed = true + } + } + if !allowed { + return nil, errors.New("token broker URL must use a reviewed allowed host") + } + return tokenbroker.New(*config.TokenBroker) +} + +func (a *App) exchangeBrokerToken(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "no-store") + if a.tokenBroker == nil || r.Method != http.MethodPost || !validProxyRequest(r, a.proxyPolicy()) { + w.WriteHeader(http.StatusForbidden) + return + } + a.startMu.Lock() + unavailable := a.draining + a.startMu.Unlock() + if unavailable { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + ctx, cancel := context.WithTimeout(r.Context(), 8*time.Second) + defer cancel() + sum := sha256.Sum256([]byte(a.clientIP(r))) + limit, err := a.services.RequestLimiter.TakeRateLimitToken(ctx, + "token-broker:"+hex.EncodeToString(sum[:]), 60, time.Minute) + if err != nil { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + if !limit.Allowed { + w.WriteHeader(http.StatusTooManyRequests) + return + } + controller := http.NewResponseController(w) + deadline, _ := ctx.Deadline() + if err := controller.SetReadDeadline(deadline); err != nil && !errors.Is(err, http.ErrNotSupported) { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + defer func() { _ = controller.SetReadDeadline(time.Time{}) }() + if err := controller.SetWriteDeadline(deadline); err != nil && !errors.Is(err, http.ErrNotSupported) { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + defer func() { _ = controller.SetWriteDeadline(time.Time{}) }() + http.TimeoutHandler(a.tokenBroker, time.Until(deadline), "").ServeHTTP( + &admissionResponse{ResponseWriter: w, headers: make(http.Header)}, r.WithContext(ctx)) +} diff --git a/server/internal/server/token_broker_test.go b/server/internal/server/token_broker_test.go new file mode 100644 index 0000000000..9fbd141c3e --- /dev/null +++ b/server/internal/server/token_broker_test.go @@ -0,0 +1,189 @@ +package server + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/githubnext/gh-cao/server/internal/admission" + "github.com/githubnext/gh-cao/server/internal/operational/memory" + "github.com/githubnext/gh-cao/server/internal/tokenbroker" +) + +type deniedBrokerIdentity struct{} + +type brokerDeadlineWriter struct { + *httptest.ResponseRecorder + read, write []time.Time +} + +func (w *brokerDeadlineWriter) SetReadDeadline(deadline time.Time) error { + w.read = append(w.read, deadline) + return nil +} + +func (w *brokerDeadlineWriter) SetWriteDeadline(deadline time.Time) error { + w.write = append(w.write, deadline) + return nil +} + +func (deniedBrokerIdentity) Verify(context.Context, string) (admission.Identity, error) { + return admission.Identity{}, admission.ErrDenied +} + +func (deniedBrokerIdentity) PrivateKey(context.Context) ([]byte, error) { + return nil, admission.ErrDenied +} + +func TestTokenBrokerEnvironmentOptIn(t *testing.T) { + for _, name := range []string{ + "ENABLED", "APP_ID", "INSTALLATION_ID", "OWNER_ID", "URL", + "KEY_VAULT_SECRET_URL", "MANAGED_IDENTITY_CLIENT_ID", + } { + t.Setenv("CAO_TOKEN_BROKER_"+name, "") + } + config, err := tokenBrokerConfigFromEnv() + if err != nil || config != nil { + t.Fatal("unset broker must remain disabled") + } + t.Setenv("CAO_TOKEN_BROKER_ENABLED", "yes") + if _, err := tokenBrokerConfigFromEnv(); err == nil { + t.Fatal("invalid opt-in was accepted") + } + t.Setenv("CAO_TOKEN_BROKER_ENABLED", "true") + if _, err := tokenBrokerConfigFromEnv(); err == nil { + t.Fatal("missing identifiers were accepted") + } + t.Setenv("CAO_TOKEN_BROKER_APP_ID", "123") + t.Setenv("CAO_TOKEN_BROKER_INSTALLATION_ID", "456") + t.Setenv("CAO_TOKEN_BROKER_OWNER_ID", "789") + t.Setenv("CAO_TOKEN_BROKER_URL", "https://broker.example/api/token-broker") + t.Setenv("CAO_TOKEN_BROKER_KEY_VAULT_SECRET_URL", "https://cao-vault.vault.azure.net/secrets/copilot-app") + config, err = tokenBrokerConfigFromEnv() + if err != nil || config == nil || config.AppID != 123 || config.InstallationID != 456 || + config.OwnerID != "789" || config.KeyVaultSecretURL == "" { + t.Fatal("explicit broker configuration was not loaded") + } +} + +func TestTokenBrokerRejectsUnsafeHosting(t *testing.T) { + base := Config{ + TokenBroker: &tokenbroker.Config{ + Audience: "https://broker.example/api/token-broker", PolicyFile: "absent-cao.json", + AppID: 1, InstallationID: 2, OwnerID: "3", + Verifier: deniedBrokerIdentity{}, KeyReader: deniedBrokerIdentity{}, + }, + HostProfile: hostedHostProfile(), SingleReplicaConfirmed: true, + TokenBrokerSingleProcessConfirmed: true, + Proxy: ProxyPolicy{RequireHTTPS: true, AllowedHosts: []string{"broker.example"}}, + } + if _, err := newTokenBroker(base); err != nil { + t.Fatal("safe explicit broker configuration was rejected") + } + for _, test := range []struct { + name string + change func(*Config) + }{ + {"local bearer", func(c *Config) { c.HostProfile = localHostProfile() }}, + {"plaintext", func(c *Config) { c.Proxy.RequireHTTPS = false }}, + {"external listener", func(c *Config) { c.HostProfile.Listener = HostListenerExternal }}, + {"platform listener", func(c *Config) { c.HostProfile.Listener = HostListenerPlatform }}, + {"unconfirmed replica", func(c *Config) { c.SingleReplicaConfirmed = false }}, + {"unconfirmed process", func(c *Config) { c.TokenBrokerSingleProcessConfirmed = false }}, + {"unreviewed host", func(c *Config) { c.Proxy.AllowedHosts = []string{"other.example"} }}, + } { + t.Run(test.name, func(t *testing.T) { + config := base + test.change(&config) + if _, err := newTokenBroker(config); err == nil { + t.Fatal("unsafe hosting was accepted") + } + }) + } + for _, endpoint := range []string{ + "http://broker.example/api/token-broker", + "https://broker.example/other", + "https://broker.example/api/token-broker/", + "https://broker.example/api/token-broker?x=1", + "https://broker.example/api/token-broker?", + "https://broker.example/api/token-broker#x", + "https://user@broker.example/api/token-broker", + "https://broker.example:443/api/token-broker", + } { + config := base + brokerConfig := *base.TokenBroker + brokerConfig.Audience = endpoint + config.TokenBroker = &brokerConfig + if _, err := newTokenBroker(config); err == nil { + t.Fatal("ambiguous broker endpoint was accepted") + } + } +} + +func TestTokenBrokerDisabledRouteDoesNotUseDashboardCredentials(t *testing.T) { + app := &App{} + for _, method := range []string{http.MethodPost, http.MethodGet} { + request := httptest.NewRequestWithContext(t.Context(), method, "https://broker.example/api/token-broker", + strings.NewReader(`{"token_type":"copilot-request","provider":"github","oidc_token":"invalid"}`)) + request.Header.Set("Authorization", "******") + request.Header.Set("Cookie", "cao_session=dashboard-session") + response := httptest.NewRecorder() + app.Handler().ServeHTTP(response, request) + if response.Code != http.StatusForbidden || response.Body.Len() != 0 || + response.Header().Get("Cache-Control") != "no-store" { + t.Fatal("disabled broker did not return an empty no-store denial") + } + } +} + +func TestTokenBrokerUsesOIDCInsteadOfDashboardAuthentication(t *testing.T) { + store, err := memory.New(memory.Config{}) + if err != nil { + t.Fatal(err) + } + defer func() { _ = store.Close() }() + broker, err := tokenbroker.New(tokenbroker.Config{ + Audience: "https://broker.example/api/token-broker", PolicyFile: "absent-cao.json", + AppID: 1, InstallationID: 2, OwnerID: "3", Verifier: deniedBrokerIdentity{}, + KeyReader: deniedBrokerIdentity{}, + }) + if err != nil { + t.Fatal(err) + } + app := &App{ + services: store.OperationalServices(), tokenBroker: broker, + config: Config{HostProfile: hostedHostProfile(), Proxy: ProxyPolicy{ + AllowedHosts: []string{"broker.example"}, RequireHTTPS: true, + }}, + } + for _, endpoint := range []string{ + "https://broker.example/api/token-broker", + "http://broker.example/api/token-broker", + "https://other.example/api/token-broker", + } { + request := httptest.NewRequestWithContext(t.Context(), http.MethodPost, endpoint, + strings.NewReader(`{"token_type":"copilot-request","provider":"github","oidc_token":"invalid"}`)) + request.Header.Set("Content-Type", "application/json") + request.Header.Set("Authorization", "******") + request.Header.Set("Cookie", "cao_session=dashboard-session") + response := &brokerDeadlineWriter{ResponseRecorder: httptest.NewRecorder()} + app.Handler().ServeHTTP(response, request) + want := http.StatusForbidden + if endpoint == "https://broker.example/api/token-broker" { + want = http.StatusUnauthorized + for _, deadlines := range [][]time.Time{response.read, response.write} { + if len(deadlines) != 2 || deadlines[0].IsZero() || + !deadlines[0].Before(time.Now().Add(9*time.Second)) || !deadlines[1].IsZero() { + t.Fatal("broker did not bound and reset both transport deadlines") + } + } + } + if response.Code != want || response.Body.Len() != 0 || + response.Header().Get("Cache-Control") != "no-store" { + t.Fatalf("broker authentication boundary failed: status=%d, want=%d", response.Code, want) + } + } +} diff --git a/server/internal/tokenbroker/issuer.go b/server/internal/tokenbroker/issuer.go new file mode 100644 index 0000000000..8343a7e020 --- /dev/null +++ b/server/internal/tokenbroker/issuer.go @@ -0,0 +1,142 @@ +package tokenbroker + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "net/http" + "strconv" + "time" + + "github.com/bradleyfalzon/ghinstallation/v2" + + "github.com/githubnext/gh-cao/server/internal/admission" +) + +const githubAPI = "https://api.github.com" + +var errUpstream = errors.New("credential issuer unavailable") + +func (s *Service) issue(ctx context.Context, identity admission.Identity) (credential, error) { + pem, err := s.keyReader.PrivateKey(ctx) + if err != nil { + return credential{}, errUpstream + } + defer clear(pem) + appTransport, err := ghinstallation.NewAppsTransport(s.client.Transport, s.config.AppID, pem) + if err != nil { + return credential{}, errUpstream + } + client := *s.client + client.Transport = appTransport + base := githubAPI + "/app/installations/" + strconv.FormatInt(s.config.InstallationID, 10) + body, err := upstream(ctx, &client, http.MethodGet, base, "", nil, http.StatusOK) + if err != nil { + return credential{}, err + } + var installation struct { + AppID int64 `json:"app_id"` + Account struct { + ID json.Number `json:"id"` + Type string `json:"type"` + } `json:"account"` + SuspendedAt json.RawMessage `json:"suspended_at"` + Permissions map[string]string `json:"permissions"` + } + if json.Unmarshal(body, &installation) != nil || installation.AppID != s.config.AppID || + installation.Account.Type != "Organization" || installation.Account.ID.String() != identity.OwnerID || + installation.Account.ID.String() != s.config.OwnerID || string(installation.SuspendedAt) != "null" || + installation.Permissions["copilot_requests"] != "read" { + return credential{}, errUpstream + } + repoID, err := strconv.ParseUint(identity.RepositoryID, 10, 64) + if err != nil { + return credential{}, errUpstream + } + payload, _ := json.Marshal(struct { + Permissions map[string]string `json:"permissions"` + RepositoryIDs []uint64 `json:"repository_ids"` + }{map[string]string{"copilot_requests": "read"}, []uint64{repoID}}) + body, err = upstream(ctx, &client, http.MethodPost, base+"/access_tokens", "", payload, http.StatusCreated) + if err != nil { + return credential{}, err + } + var result struct { + Token string `json:"token"` + ExpiresAt string `json:"expires_at"` + Permissions map[string]string `json:"permissions"` + Repositories []struct { + ID json.Number `json:"id"` + Owner struct { + ID json.Number `json:"id"` + } `json:"owner"` + } `json:"repositories"` + } + if json.Unmarshal(body, &result) != nil { + return credential{Token: result.Token}, errUpstream + } + expiry, parseErr := time.Parse(time.RFC3339, result.ExpiresAt) + token := credential{Token: result.Token, ExpiresAt: expiry} + now := s.now() + if parseErr != nil || !safeToken(result.Token) || + !expiry.After(now.Add(time.Minute)) || expiry.After(now.Add(time.Hour)) || + result.Permissions["copilot_requests"] != "read" || len(result.Repositories) != 1 || + result.Repositories[0].ID.String() != identity.RepositoryID || + result.Repositories[0].Owner.ID.String() != identity.OwnerID { + return token, errUpstream + } + for permission, level := range result.Permissions { + if permission != "copilot_requests" && (permission != "metadata" || level != "read") { + return token, errUpstream + } + } + return token, nil +} + +func upstream(ctx context.Context, client *http.Client, method, endpoint, token string, payload []byte, expected int) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, method, endpoint, bytes.NewReader(payload)) + if err != nil { + return nil, errUpstream + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2026-03-10") + if len(payload) > 0 { + req.Header.Set("Content-Type", "application/json") + } + if token != "" { + if !safeToken(token) { + return nil, errUpstream + } + req.Header.Set("Authorization", "Bearer "+token) + } + res, err := client.Do(req) + if err != nil { + return nil, errUpstream + } + defer func() { _ = res.Body.Close() }() + body, err := io.ReadAll(io.LimitReader(res.Body, bodyLimit+1)) + if err != nil || len(body) > bodyLimit { + return nil, errUpstream + } + if res.StatusCode != expected { + // Revocation can succeed remotely even if its response is lost. A + // subsequent 401 from this fixed endpoint proves the held credential + // no longer authenticates, so cleanup is already complete. + if method == http.MethodDelete && endpoint == githubAPI+"/installation/token" && + token != "" && expected == http.StatusNoContent && res.StatusCode == http.StatusUnauthorized { + return nil, nil + } + return nil, errUpstream + } + if expected != http.StatusNoContent && uniqueJSON(body) != nil { + return nil, errUpstream + } + return body, nil +} + +func (s *Service) revokeToken(ctx context.Context, token string) error { + _, err := upstream(ctx, s.client, http.MethodDelete, githubAPI+"/installation/token", token, nil, http.StatusNoContent) + return err +} diff --git a/server/internal/tokenbroker/issuer_test.go b/server/internal/tokenbroker/issuer_test.go new file mode 100644 index 0000000000..954e8d07c5 --- /dev/null +++ b/server/internal/tokenbroker/issuer_test.go @@ -0,0 +1,239 @@ +package tokenbroker + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + "time" +) + +func TestInstallationMustMatchTrustedTenant(t *testing.T) { + tests := []struct { + name string + body string + }{ + {"wrong-app", `{"app_id":8,"account":{"id":42,"type":"Organization"},"suspended_at":null,"permissions":{"copilot_requests":"read"}}`}, + {"wrong-owner", `{"app_id":7,"account":{"id":99,"type":"Organization"},"suspended_at":null,"permissions":{"copilot_requests":"read"}}`}, + {"personal-account", `{"app_id":7,"account":{"id":42,"type":"User"},"suspended_at":null,"permissions":{"copilot_requests":"read"}}`}, + {"suspended", `{"app_id":7,"account":{"id":42,"type":"Organization"},"suspended_at":"2026-10-30T00:00:00Z","permissions":{"copilot_requests":"read"}}`}, + {"missing-suspension-evidence", `{"app_id":7,"account":{"id":42,"type":"Organization"},"permissions":{"copilot_requests":"read"}}`}, + {"missing-permission", `{"app_id":7,"account":{"id":42,"type":"Organization"},"suspended_at":null,"permissions":{"issues":"write"}}`}, + {"write-not-read", `{"app_id":7,"account":{"id":42,"type":"Organization"},"suspended_at":null,"permissions":{"copilot_requests":"write"}}`}, + {"duplicate-security-field", `{"app_id":7,"app_id":8,"account":{"id":42,"type":"Organization"},"suspended_at":null,"permissions":{"copilot_requests":"read"}}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFixture(t) + f.installation = tt.body + if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 || f.mints != 0 { + t.Fatal("installation evidence failed open") + } + }) + } +} + +func TestIssuedTokenValidationRevokesRejectedResults(t *testing.T) { + tests := []struct { + name string + edit func(map[string]any, *fixture) + }{ + {"scope", func(result map[string]any, _ *fixture) { + result["permissions"] = map[string]string{"copilot_requests": "read", "issues": "write"} + }}, + {"missing-permission", func(result map[string]any, _ *fixture) { + result["permissions"] = map[string]string{"metadata": "read"} + }}, + {"write", func(result map[string]any, _ *fixture) { + result["permissions"] = map[string]string{"copilot_requests": "write"} + }}, + {"near-expiry", func(result map[string]any, f *fixture) { + result["expires_at"] = f.now.Add(time.Minute).Format(time.RFC3339) + }}, + {"too-long", func(result map[string]any, f *fixture) { + result["expires_at"] = f.now.Add(time.Hour + time.Second).Format(time.RFC3339) + }}, + {"unknown-expiry", func(result map[string]any, _ *fixture) { delete(result, "expires_at") }}, + {"bad-expiry", func(result map[string]any, _ *fixture) { result["expires_at"] = "not-a-time" }}, + {"missing-repository", func(result map[string]any, _ *fixture) { delete(result, "repositories") }}, + {"other-repository", func(result map[string]any, _ *fixture) { + result["repositories"] = []any{map[string]any{"id": 1002, "owner": map[string]any{"id": 42}}} + }}, + {"other-owner", func(result map[string]any, _ *fixture) { + result["repositories"] = []any{map[string]any{"id": 1001, "owner": map[string]any{"id": 99}}} + }}, + {"too-many-repositories", func(result map[string]any, _ *fixture) { + result["repositories"] = []any{ + map[string]any{"id": 1001, "owner": map[string]any{"id": 42}}, + map[string]any{"id": 1002, "owner": map[string]any{"id": 42}}, + } + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFixture(t) + original := f.result + f.result = func() string { + var result map[string]any + if err := json.Unmarshal([]byte(original()), &result); err != nil { + t.Fatal(err) + } + tt.edit(result, f) + data, _ := json.Marshal(result) + return string(data) + } + if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 || f.mints != 1 || len(f.revoked) != 1 { + t.Fatal("invalid upstream token was returned or orphaned") + } + }) + } +} + +func TestRejectedCredentialRetainedOnCleanupFailure(t *testing.T) { + f := newFixture(t) + f.result = func() string { + return `{"token":"test-rejected","expires_at":"bad","permissions":{"copilot_requests":"write"}}` + } + f.revokeFails = true + if w := f.call(exchangeBody); w.Code != 503 { + t.Fatal(w.Code) + } + state := f.service.contexts[executionKey(f.identity)] + if len(state.tokens) != 1 { + t.Fatal("failed cleanup lost rejected token") + } + f.revokeFails = false + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("rejected credential cleanup was not retried") + } +} + +func TestUpstreamTransportFailsClosed(t *testing.T) { + for _, status := range []int{301, 302, 307, 400, 401, 403, 429, 500, 503} { + t.Run(http.StatusText(status), func(t *testing.T) { + f := newFixture(t) + calls := 0 + f.service.client.Transport = transportFunc(func(r *http.Request) (*http.Response, error) { + calls++ + if r.URL.Host != "api.github.com" { + t.Error("followed attacker redirect") + } + res := testResponse(status, "untrusted upstream response") + res.Header.Set("Location", "https://evil.example/steal") + return res, nil + }) + if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 || calls != 1 { + t.Fatal("upstream failure leaked or redirected") + } + }) + } + f := newFixture(t) + f.service.client.Transport = transportFunc(func(*http.Request) (*http.Response, error) { + return testResponse(200, strings.Repeat("x", bodyLimit+1)), nil + }) + if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 { + t.Fatal("oversized upstream response accepted") + } + f.service.client.Transport = transportFunc(func(*http.Request) (*http.Response, error) { + return nil, errors.New("private upstream error") + }) + if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 { + t.Fatal("transport error leaked") + } +} + +func TestUpstreamUsesRequestDeadline(t *testing.T) { + f := newFixture(t) + f.service.client.Transport = transportFunc(func(r *http.Request) (*http.Response, error) { + deadline, ok := r.Context().Deadline() + if !ok || time.Until(deadline) > requestTimeout { + t.Error("issuer not bounded by request deadline") + } + <-r.Context().Done() + return nil, r.Context().Err() + }) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + cancel() + if _, err := f.service.issue(ctx, f.identity); err == nil { + t.Fatal("canceled context minted credential") + } +} + +func TestCompleteEscapedCredentialResponseIsBounded(t *testing.T) { + for _, character := range []string{`"`, `\`, `<`} { + for _, boundary := range []string{"fits", "exceeds", "token-limit"} { + t.Run(character+"/"+boundary, func(t *testing.T) { + f := newFixture(t) + empty, err := json.Marshal(credential{ExpiresAt: f.now.Add(50 * time.Minute)}) + if err != nil { + t.Fatal(err) + } + unit, err := json.Marshal(character) + if err != nil { + t.Fatal(err) + } + length := (bodyLimit - len(empty)) / (len(unit) - 2) + switch boundary { + case "exceeds": + length++ + case "token-limit": + length = bodyLimit / 2 + } + token := strings.Repeat(character, length) + original := f.result + f.result = func() string { + var result map[string]any + if err := json.Unmarshal([]byte(original()), &result); err != nil { + t.Fatal(err) + } + result["token"] = token + body, err := json.Marshal(result) + if err != nil { + t.Fatal(err) + } + return string(body) + } + w := f.call(exchangeBody) + if boundary == "fits" { + if w.Code != 200 || w.Body.Len() > bodyLimit { + t.Fatalf("bounded escaped credential failed: %d bytes=%d", w.Code, w.Body.Len()) + } + return + } + if w.Code != 503 || w.Body.Len() != 0 || len(f.revoked) != 1 || f.revoked[0] != token { + t.Fatal("oversized encoded credential was returned or orphaned") + } + }) + } + } +} + +func TestUnauthorizedHeldCredentialCompletesIdempotentRevocation(t *testing.T) { + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal(w.Code) + } + transport := f.service.client.Transport + revocations := 0 + f.service.client.Transport = transportFunc(func(r *http.Request) (*http.Response, error) { + if r.Method == http.MethodDelete { + revocations++ + if r.URL.String() != githubAPI+"/installation/token" || r.Header.Get("Authorization") == "" { + t.Fatal("cleanup did not authenticate the exact held credential") + } + return testResponse(http.StatusUnauthorized, `{"message":"Bad credentials"}`), nil + } + return transport.RoundTrip(r) + }) + if w := f.call(invalidateBody); w.Code != 204 { + t.Fatal("already-invalid held token prevented cleanup") + } + if len(f.service.contexts[executionKey(f.identity)].tokens) != 0 { + t.Fatal("already-invalid held token remained tracked") + } + if w := f.call(invalidateBody); w.Code != 204 || revocations != 1 { + t.Fatal("already-invalid token cleanup was not idempotent") + } +} diff --git a/server/internal/tokenbroker/policy.go b/server/internal/tokenbroker/policy.go new file mode 100644 index 0000000000..ac5f4f10b7 --- /dev/null +++ b/server/internal/tokenbroker/policy.go @@ -0,0 +1,102 @@ +package tokenbroker + +import ( + "encoding/json" + "io" + "os" + "regexp" + "strings" + + "github.com/githubnext/gh-cao/server/internal/admission" +) + +var ( + stableID = regexp.MustCompile(`^[1-9][0-9]{0,19}$`) + revision = regexp.MustCompile(`^[a-f0-9]{40}$`) + workflow = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9-]*/[A-Za-z0-9][A-Za-z0-9._-]*/\.github/workflows/[A-Za-z0-9_-][A-Za-z0-9._-]*\.ya?ml@refs/(heads|tags)/[A-Za-z0-9_-]+([./][A-Za-z0-9_-]+)*$`) +) + +type workflowRule struct { + RepositoryID string `json:"repository-id"` + OwnerID string `json:"owner-id"` + WorkflowRef string `json:"workflow-ref"` + WorkflowSHA string `json:"workflow-sha"` +} + +func (s *Service) authorized(i admission.Identity) bool { + if !stableID.MatchString(i.RepositoryID) || !stableID.MatchString(i.OwnerID) || + !stableID.MatchString(i.RunID) || !stableID.MatchString(i.RunAttempt) || + !stableID.MatchString(i.CheckRunID) || i.EnvironmentPresent || i.OwnerID != s.config.OwnerID || + i.Issuer != admission.Issuer || !revision.MatchString(i.WorkflowSHA) || + !validWorkflowRef(i.WorkflowRef) || + !strings.HasPrefix(i.WorkflowRef, i.Repository+"/.github/workflows/") { + return false + } + _, ref, ok := strings.Cut(i.WorkflowRef, "@") + owner, repository, _ := strings.Cut(i.Repository, "/") + legacySubject := "repo:" + i.Repository + ":ref:" + ref + immutableSubject := "repo:" + owner + "@" + i.OwnerID + "/" + repository + "@" + i.RepositoryID + ":ref:" + ref + if !ok || i.Ref != ref || (i.Subject != legacySubject && i.Subject != immutableSubject) { + return false + } + switch i.Event { + case "workflow_dispatch", "schedule", "push": + default: + return false + } + // Read on every operation. A changed or removed mounted policy immediately + // denies authority instead of retaining a stale authorization snapshot. + f, err := os.Open(s.config.PolicyFile) + if err != nil { + return false + } + defer func() { _ = f.Close() }() + data, err := io.ReadAll(io.LimitReader(f, (1<<20)+1)) + if err != nil || len(data) > 1<<20 || uniqueJSON(data) != nil { + return false + } + var root, control, broker, requests map[string]json.RawMessage + var workflows []json.RawMessage + if json.Unmarshal(data, &root) != nil || root == nil || + json.Unmarshal(root["control-plane"], &control) != nil || control == nil || + json.Unmarshal(control["token-broker"], &broker) != nil || len(broker) != 1 || + json.Unmarshal(broker["copilot-requests"], &requests) != nil || len(requests) != 1 || + json.Unmarshal(requests["workflows"], &workflows) != nil || len(workflows) > 128 { + return false + } + allowed := false + seen := make(map[workflowRule]bool, len(workflows)) + for _, raw := range workflows { + var rule workflowRule + var fields map[string]json.RawMessage + if json.Unmarshal(raw, &fields) != nil || len(fields) != 4 || + fields["repository-id"] == nil || fields["owner-id"] == nil || + fields["workflow-ref"] == nil || fields["workflow-sha"] == nil || + json.Unmarshal(raw, &rule) != nil || seen[rule] { + return false + } + seen[rule] = true + if !stableID.MatchString(rule.RepositoryID) || !stableID.MatchString(rule.OwnerID) || + !validWorkflowRef(rule.WorkflowRef) || !revision.MatchString(rule.WorkflowSHA) { + return false + } + if rule.RepositoryID == i.RepositoryID && rule.OwnerID == i.OwnerID && + rule.WorkflowRef == i.WorkflowRef && rule.WorkflowSHA == i.WorkflowSHA { + allowed = true + } + } + return allowed +} + +func validWorkflowRef(value string) bool { + if !workflow.MatchString(value) || strings.Contains(value, "..") { + return false + } + _, ref, _ := strings.Cut(value, "@") + for _, segment := range strings.Split(ref, "/") { + if strings.HasSuffix(segment, ".lock") { + return false + } + } + return true +} diff --git a/server/internal/tokenbroker/policy_test.go b/server/internal/tokenbroker/policy_test.go new file mode 100644 index 0000000000..cb5800f0fa --- /dev/null +++ b/server/internal/tokenbroker/policy_test.go @@ -0,0 +1,197 @@ +package tokenbroker + +import ( + "encoding/json" + "os" + "strconv" + "strings" + "testing" + "time" +) + +func TestMalformedPolicyAlwaysDenies(t *testing.T) { + tests := []string{ + `{}`, `null`, `[]`, `{`, + `{"control-plane":{"token-broker":{"copilot-requests":{"workflows":null}}}}`, + `{"control-plane":{"token-broker":{"copilot-requests":{"workflows":[],"workflows":[]}}}}`, + `{"control-plane":{"token-broker":{"copilot-requests":{"workflows":[null]}}}}`, + strings.Repeat(" ", (1<<20)+1), + } + f := newFixture(t) + for _, body := range tests { + if err := os.WriteFile(f.policy, []byte(body), 0600); err != nil { + t.Fatal(err) + } + if w := f.call(exchangeBody); w.Code != 403 || f.keyReads != 0 { + t.Fatal("malformed policy failed open") + } + } +} + +func TestPolicyRuleRejectsUnsafeAuthority(t *testing.T) { + f := newFixture(t) + for _, edit := range []func(map[string]string){ + func(r map[string]string) { r["repository-id"] = "*" }, + func(r map[string]string) { r["repository-id"] = "0" }, + func(r map[string]string) { r["repository-id"] = "01" }, + func(r map[string]string) { r["owner-id"] = strings.Repeat("1", 21) }, + func(r map[string]string) { r["workflow-sha"] = strings.Repeat("A", 40) }, + func(r map[string]string) { + r["workflow-ref"] = "acme/project/.github/workflows/../triage.lock.yml@refs/heads/main" + }, + func(r map[string]string) { + r["workflow-ref"] = "acme/project/.github/workflows/triage.lock.yml@refs/heads/feature..evil" + }, + func(r map[string]string) { + r["workflow-ref"] = "acme/project/.github/workflows/*.lock.yml@refs/heads/main" + }, + func(r map[string]string) { r["extra-authority"] = "ignored" }, + } { + rule := map[string]string{ + "repository-id": f.identity.RepositoryID, "owner-id": f.identity.OwnerID, + "workflow-ref": f.identity.WorkflowRef, "workflow-sha": f.identity.WorkflowSHA, + } + edit(rule) + data, _ := json.Marshal(map[string]any{"control-plane": map[string]any{"token-broker": map[string]any{"copilot-requests": map[string]any{"workflows": []any{rule}}}}}) + if err := os.WriteFile(f.policy, data, 0600); err != nil { + t.Fatal(err) + } + if w := f.call(exchangeBody); w.Code != 403 || f.keyReads != 0 { + t.Fatal("unsafe policy rule authorized issuance") + } + } +} + +func TestExactTagRefProfile(t *testing.T) { + f := newFixture(t) + f.identity.Ref = "refs/tags/release-1.0" + f.identity.Subject = "repo:acme/project:ref:" + f.identity.Ref + f.identity.WorkflowRef = "acme/project/.github/workflows/triage.lock.yml@" + f.identity.Ref + f.identity.Event = "push" + f.writePolicy(t, f.identity) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal("enrolled exact safe tag denied") + } +} + +func TestBoundedContextsAndTenantLimits(t *testing.T) { + f := newFixture(t) + for n := range maxContexts { + f.service.contexts[strconv.Itoa(n)] = &execution{until: f.now.Add(time.Hour)} + } + if w := f.call(exchangeBody); w.Code != 429 || f.keyReads != 0 { + t.Fatal("context capacity did not fail closed") + } + clear(f.service.contexts) + f.service.tenantWindow = f.now + f.service.tenantMints = 60 + if w := f.call(exchangeBody); w.Code != 429 || f.keyReads != 0 { + t.Fatal("tenant mint rate was not bounded") + } + f.service.tenantMints = 0 + state := f.service.contexts[executionKey(f.identity)] + state.tokens = make([]heldToken, maxTokens) + for n := range state.tokens { + state.tokens[n] = heldToken{sealed: f.service.seal(executionKey(f.identity), "test"), expires: f.now.Add(time.Hour)} + } + if w := f.call(exchangeBody); w.Code != 429 || f.keyReads != 0 { + t.Fatal("per-context credential capacity did not fail closed") + } +} + +func TestPolicyRuleCountAndDuplicatesFailClosed(t *testing.T) { + f := newFixture(t) + for _, count := range []int{2, 129} { + rules := make([]workflowRule, count) + for n := range rules { + rules[n] = workflowRule{ + RepositoryID: f.identity.RepositoryID, OwnerID: f.identity.OwnerID, + WorkflowRef: f.identity.WorkflowRef, WorkflowSHA: f.identity.WorkflowSHA, + } + if count > 128 && n != 0 { + rules[n].RepositoryID = strconv.Itoa(n + 1) + } + } + data, _ := json.Marshal(map[string]any{"control-plane": map[string]any{"token-broker": map[string]any{"copilot-requests": map[string]any{"workflows": rules}}}}) + if err := os.WriteFile(f.policy, data, 0600); err != nil { + t.Fatal(err) + } + if w := f.call(exchangeBody); w.Code != 403 || f.keyReads != 0 { + t.Fatal("duplicate or over-capacity policy authorized issuance") + } + } +} + +func TestWorkflowRefMatchesSharedPolicyContract(t *testing.T) { + for _, value := range []string{ + "acme/project/.github/workflows/triage.yml@refs/heads/main", + "acme/project/.github/workflows/triage.yaml@refs/heads/feature/topic", + "acme/project/.github/workflows/_triage.lock.yml@refs/tags/release/1.2", + "acme/project/.github/workflows/-triage.yml@refs/heads/feature-1", + "acme/project/.github/workflows/triage.lock.yaml@refs/tags/a.locked", + } { + if !validWorkflowRef(value) { + t.Errorf("shared-contract valid ref rejected: %s", value) + } + } + for _, value := range []string{ + "acme/project/.github/workflows/triage.yml@refs/heads/foo.lock", + "acme/project/.github/workflows/triage.yml@refs/tags/foo.lock/v1", + "acme/project/.github/workflows/triage.yml@refs/heads/main.", + "acme/project/.github/workflows/triage.yml@refs/heads/main//foo", + "acme/project/.github/workflows/triage.yml@refs/heads/main..foo", + "acme/project/.github/workflows/.triage.yml@refs/heads/main", + "acme/project/.github/workflows/triage.yml@refs/heads/main\n", + } { + if validWorkflowRef(value) { + t.Errorf("shared-contract invalid ref accepted: %s", value) + } + } + f := newFixture(t) + f.identity.Ref = "refs/heads/feature/topic" + f.identity.Subject = "repo:acme/project:ref:" + f.identity.Ref + f.identity.WorkflowRef = "acme/project/.github/workflows/_triage.yaml@" + f.identity.Ref + f.writePolicy(t, f.identity) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal("schema-valid enrolled YAML branch was denied") + } +} + +func TestBrokerPolicyRejectsUnknownAndCaseVariantFields(t *testing.T) { + f := newFixture(t) + rule := map[string]string{ + "repository-id": f.identity.RepositoryID, "owner-id": f.identity.OwnerID, + "workflow-ref": f.identity.WorkflowRef, "workflow-sha": f.identity.WorkflowSHA, + } + for _, change := range []func(map[string]any, map[string]any, map[string]string){ + func(b, _ map[string]any, _ map[string]string) { b["extra"] = true }, + func(_, r map[string]any, _ map[string]string) { r["extra"] = true }, + func(b, _ map[string]any, _ map[string]string) { + b["Copilot-Requests"] = b["copilot-requests"] + delete(b, "copilot-requests") + }, + func(_, r map[string]any, _ map[string]string) { + r["Workflows"] = r["workflows"] + delete(r, "workflows") + }, + func(_, _ map[string]any, r map[string]string) { + r["Repository-ID"] = r["repository-id"] + delete(r, "repository-id") + }, + } { + entry := make(map[string]string, len(rule)) + for key, value := range rule { + entry[key] = value + } + requests := map[string]any{"workflows": []any{entry}} + broker := map[string]any{"copilot-requests": requests} + change(broker, requests, entry) + data, _ := json.Marshal(map[string]any{"control-plane": map[string]any{"token-broker": broker}}) + if err := os.WriteFile(f.policy, data, 0600); err != nil { + t.Fatal(err) + } + if w := f.call(exchangeBody); w.Code != 403 || f.keyReads != 0 { + t.Fatal("unknown or case-variant authority fields failed open") + } + } +} diff --git a/server/internal/tokenbroker/protocol.go b/server/internal/tokenbroker/protocol.go new file mode 100644 index 0000000000..8c75dd9629 --- /dev/null +++ b/server/internal/tokenbroker/protocol.go @@ -0,0 +1,147 @@ +package tokenbroker + +import ( + "bytes" + "encoding/json" + "errors" + "io" + "mime" + "net/http" + "strings" + "unicode/utf8" +) + +const bodyLimit = 65536 + +var errInvalid = errors.New("invalid broker input") + +type request struct { + tokenType, provider, identity string + invalidate bool +} + +// uniqueJSON rejects duplicate members at every nesting depth, not just at the +// top level. It is also used for reviewed policy and upstream security fields. +func uniqueJSON(data []byte) error { + if !utf8.Valid(data) { + return errInvalid + } + d := json.NewDecoder(bytes.NewReader(data)) + d.UseNumber() + var value func(int) error + value = func(depth int) error { + if depth > 64 { + return errInvalid + } + t, err := d.Token() + if err != nil { + return errInvalid + } + delim, ok := t.(json.Delim) + if !ok { + return nil + } + switch delim { + case '{': + seen := map[string]bool{} + for d.More() { + k, err := d.Token() + s, ok := k.(string) + if err != nil || !ok || seen[s] { + return errInvalid + } + seen[s] = true + if err := value(depth + 1); err != nil { + return err + } + } + case '[': + for d.More() { + if err := value(depth + 1); err != nil { + return err + } + } + default: + return errInvalid + } + _, err = d.Token() + return err + } + if err := value(0); err != nil { + return errInvalid + } + if _, err := d.Token(); !errors.Is(err, io.EOF) { + return errInvalid + } + return nil +} + +func decodeRequest(w http.ResponseWriter, r *http.Request) (request, int) { + media, params, err := mime.ParseMediaType(r.Header.Get("Content-Type")) + if err != nil || media != "application/json" || (params["charset"] != "" && !strings.EqualFold(params["charset"], "utf-8")) { + return request{}, http.StatusUnsupportedMediaType + } + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, bodyLimit)) + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + return request{}, http.StatusRequestEntityTooLarge + } + if err != nil || uniqueJSON(body) != nil { + return request{}, http.StatusBadRequest + } + var fields map[string]json.RawMessage + if json.Unmarshal(body, &fields) != nil || fields == nil { + return request{}, http.StatusBadRequest + } + read := func(name string) string { + var s string + _ = json.Unmarshal(fields[name], &s) + return s + } + out := request{tokenType: read("token_type"), provider: read("provider"), identity: read("oidc_token")} + if out.identity == "" { + return request{}, http.StatusBadRequest + } + if _, exists := fields["operation"]; exists { + if read("operation") != "invalidate" || len(fields) != 2 { + return request{}, http.StatusBadRequest + } + out.invalidate = true + return out, 0 + } + if out.tokenType == "" { + return request{}, http.StatusBadRequest + } + switch out.tokenType { + case "copilot-request": + if len(fields) != 3 || out.provider == "" { + return request{}, http.StatusBadRequest + } + case "copilot-task": + if len(fields) != 2 { + return request{}, http.StatusBadRequest + } + default: + // Unknown types still have to satisfy the string-member envelope. + for name, raw := range fields { + var s string + if (name != "token_type" && name != "provider" && name != "oidc_token") || json.Unmarshal(raw, &s) != nil { + return request{}, http.StatusBadRequest + } + } + return request{}, http.StatusUnprocessableEntity + } + return out, 0 +} + +func safeToken(token string) bool { + if len(token) == 0 || len(token) > bodyLimit/2 { + return false + } + for _, c := range token { + if c < '!' || c > '~' { + return false + } + } + return true +} diff --git a/server/internal/tokenbroker/protocol_test.go b/server/internal/tokenbroker/protocol_test.go new file mode 100644 index 0000000000..20360f014d --- /dev/null +++ b/server/internal/tokenbroker/protocol_test.go @@ -0,0 +1,25 @@ +package tokenbroker + +import ( + "strings" + "testing" +) + +func TestUniqueJSONIsDepthBoundedAndStrict(t *testing.T) { + for _, value := range []string{ + `{"a":1,"a":2}`, + `{"a":{"b":1,"b":2}}`, + `{"a":1,"\u0061":2}`, + `{} {}`, + `{"a":}`, + string([]byte{'"', 0xff, '"'}), + strings.Repeat("[", 66) + "0" + strings.Repeat("]", 66), + } { + if uniqueJSON([]byte(value)) == nil { + t.Fatal("duplicate, invalid, or excessively nested JSON accepted") + } + } + if err := uniqueJSON([]byte(`{"a":[1,{"b":true}],"c":null}`)); err != nil { + t.Fatal("bounded unique JSON rejected") + } +} diff --git a/server/internal/tokenbroker/service.go b/server/internal/tokenbroker/service.go new file mode 100644 index 0000000000..c968dfaab7 --- /dev/null +++ b/server/internal/tokenbroker/service.go @@ -0,0 +1,354 @@ +// Package tokenbroker implements a request-only, single-process GitHub Actions +// credential broker. Deployment must not share an endpoint across replicas. +package tokenbroker + +import ( + "context" + "crypto/aes" + "crypto/cipher" + "crypto/rand" + "crypto/sha256" + "encoding/binary" + "encoding/hex" + "encoding/json" + "errors" + "net/http" + "net/url" + "strings" + "time" + "unicode" + + "github.com/githubnext/gh-cao/server/internal/admission" +) + +type IdentityVerifier interface { + Verify(context.Context, string) (admission.Identity, error) +} + +type PrivateKeyReader interface { + PrivateKey(context.Context) ([]byte, error) +} + +type Config struct { + Audience, PolicyFile string + AppID, InstallationID int64 + OwnerID, KeyVaultSecretURL, ManagedIdentityClientID string + Verifier IdentityVerifier + KeyReader PrivateKeyReader + HTTPClient *http.Client + Now func() time.Time +} + +const ( + requestTimeout = 8 * time.Second + maxContexts = 4096 + maxTokens = 16 +) + +type credential struct { + Token string `json:"token"` + ExpiresAt time.Time `json:"expires_at"` +} + +type heldToken struct { + sealed []byte + expires time.Time +} + +type execution struct { + tokens []heldToken + until time.Time + window time.Time + requests int + invalidated bool +} + +type Service struct { + config Config + verifier IdentityVerifier + keyReader PrivateKeyReader + client *http.Client + now func() time.Time + gate chan struct{} + contexts map[string]*execution + cipher cipher.AEAD + sequence uint64 + closed bool + tenantWindow time.Time + tenantMints int +} + +func New(config Config) (*Service, error) { + u, err := url.Parse(config.Audience) + if err != nil || u.Scheme != "https" || u.Hostname() == "" || u.User != nil || + u.Fragment != "" || u.RawQuery != "" || u.ForceQuery || u.Path != "/api/token-broker" || + len(config.Audience) > 2048 || strings.ContainsAny(config.Audience, "\\#") || + strings.ContainsFunc(config.Audience, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) || + strings.Contains(config.Audience, "${{") || config.PolicyFile == "" || + config.AppID <= 0 || config.InstallationID <= 0 || !stableID.MatchString(config.OwnerID) { + return nil, errors.New("invalid token broker configuration") + } + verifier := config.Verifier + if verifier == nil { + verifier, err = admission.NewVerifier(config.Audience) + if err != nil { + return nil, errors.New("invalid token broker verifier configuration") + } + } + reader := config.KeyReader + if reader == nil { + vaultURL, err := url.Parse(config.KeyVaultSecretURL) + if err != nil || strings.Contains(config.KeyVaultSecretURL, "#") || !safeVaultURL(vaultURL) { + return nil, errors.New("invalid token broker key configuration") + } + reader = KeyVault{SecretURL: config.KeyVaultSecretURL, ClientID: config.ManagedIdentityClientID} + } + client := &http.Client{Timeout: requestTimeout} + if config.HTTPClient != nil { + *client = *config.HTTPClient + } + if client.Timeout <= 0 || client.Timeout > requestTimeout { + client.Timeout = requestTimeout + } + client.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse } + client.Jar = nil + if client.Transport == nil { + client.Transport = http.DefaultTransport + } + now := config.Now + if now == nil { + now = time.Now + } + var key [32]byte + if _, err := rand.Read(key[:]); err != nil { + return nil, errors.New("cannot initialize token protection") + } + block, err := aes.NewCipher(key[:]) + clear(key[:]) + if err != nil { + return nil, err + } + aead, err := cipher.NewGCM(block) + if err != nil { + return nil, err + } + return &Service{ + config: config, verifier: verifier, keyReader: reader, client: client, now: now, + gate: make(chan struct{}, 1), contexts: map[string]*execution{}, cipher: aead, + }, nil +} + +func executionKey(i admission.Identity) string { + data, _ := json.Marshal([]string{ + i.Issuer, i.OwnerID, i.RepositoryID, i.Repository, i.WorkflowRef, + i.WorkflowSHA, i.Ref, i.Event, i.RunID, i.RunAttempt, i.CheckRunID, + }) + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + +func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("X-Content-Type-Options", "nosniff") + if r.URL.Path != "/api/token-broker" { + w.WriteHeader(http.StatusNotFound) + return + } + if r.Method != http.MethodPost { + w.Header().Set("Allow", http.MethodPost) + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + ctx, cancel := context.WithTimeout(r.Context(), requestTimeout) + defer cancel() + controller := http.NewResponseController(w) + _ = controller.SetReadDeadline(time.Now().Add(requestTimeout)) + defer func() { _ = controller.SetReadDeadline(time.Time{}) }() + input, status := decodeRequest(w, r) + if status != 0 { + w.WriteHeader(status) + return + } + identity, err := s.verifier.Verify(ctx, input.identity) + if err != nil { + status = http.StatusUnauthorized + if errors.Is(err, admission.ErrUnavailable) { + status = http.StatusServiceUnavailable + } + w.WriteHeader(status) + return + } + // Serialize policy evaluation, issuance and cleanup, including shutdown, so + // invalidation cannot race an in-flight mint or policy evaluation. + select { + case s.gate <- struct{}{}: + defer func() { <-s.gate }() + case <-ctx.Done(): + w.WriteHeader(http.StatusServiceUnavailable) + return + } + if !s.authorized(identity) { + w.WriteHeader(http.StatusForbidden) + return + } + if !input.invalidate && input.tokenType == "copilot-task" { + w.WriteHeader(http.StatusNotImplemented) + return + } + if !input.invalidate && input.provider != "github" { + w.WriteHeader(http.StatusForbidden) + return + } + now := s.now() + s.prune(now) + key := executionKey(identity) + state := s.contexts[key] + if s.closed { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + if state == nil { + if len(s.contexts) >= maxContexts { + w.WriteHeader(http.StatusTooManyRequests) + return + } + state = &execution{until: now.Add(70 * time.Minute)} + s.contexts[key] = state + } + if now.Sub(state.window) >= time.Minute { + state.window, state.requests = now, 0 + } + state.requests++ + if state.requests > 30 { + w.WriteHeader(http.StatusTooManyRequests) + return + } + if input.invalidate { + state.invalidated = true + if s.revoke(ctx, key, state) != nil { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusNoContent) + return + } + if state.invalidated { + w.WriteHeader(http.StatusForbidden) + return + } + if state.requests > 6 || len(state.tokens) >= maxTokens { + w.WriteHeader(http.StatusTooManyRequests) + return + } + if now.Sub(s.tenantWindow) >= time.Minute { + s.tenantWindow, s.tenantMints = now, 0 + } + if s.tenantMints >= 60 { + w.WriteHeader(http.StatusTooManyRequests) + return + } + s.tenantMints++ + result, issueErr := s.issue(ctx, identity) + var response []byte + if issueErr == nil { + response, issueErr = json.Marshal(result) + if len(response) > bodyLimit { + issueErr = errUpstream + } + } + if result.Token != "" { + expiry := result.ExpiresAt + if expiry.IsZero() { + expiry = now.Add(time.Hour) + } + // Record every returned credential before any attempt to send it. Even + // rejected upstream scope/expiry results must be revoked or retained. + state.tokens = append(state.tokens, heldToken{ + sealed: s.seal(key, result.Token), expires: expiry, + }) + state.until = now.Add(70 * time.Minute) + } + if issueErr != nil { + _ = s.revoke(ctx, key, state) + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(response); err != nil { + // Delivery may have been partial. Retain the credential until cleanup. + return + } +} + +func (s *Service) seal(key, token string) []byte { + nonce := make([]byte, s.cipher.NonceSize()) + // All calls hold the service gate. A counter supplies unique nonces for + // the random process-local key without persisting any credential state. + s.sequence++ + binary.BigEndian.PutUint64(nonce[len(nonce)-8:], s.sequence) + return s.cipher.Seal(nonce, nonce, []byte(token), []byte(key)) +} + +func (s *Service) prune(now time.Time) { + for key, state := range s.contexts { + remaining := state.tokens[:0] + for _, token := range state.tokens { + if token.expires.After(now) { + remaining = append(remaining, token) + } + } + clear(state.tokens[len(remaining):]) + state.tokens = remaining + if len(state.tokens) == 0 && !state.until.After(now) { + delete(s.contexts, key) + } + } +} + +func (s *Service) revoke(ctx context.Context, key string, state *execution) error { + var failed bool + remaining := state.tokens[:0] + for _, token := range state.tokens { + n := s.cipher.NonceSize() + plain, err := s.cipher.Open(nil, token.sealed[:n], token.sealed[n:], []byte(key)) + if err == nil { + err = s.revokeToken(ctx, string(plain)) + } + clear(plain) + if err != nil { + failed = true + remaining = append(remaining, token) + } + } + clear(state.tokens[len(remaining):]) + state.tokens = remaining + if failed { + return errors.New("credential cleanup unavailable") + } + return nil +} + +// Close stops issuance and attempts to revoke all outstanding credentials. +// Failed cleanup remains in memory so callers can retry Close with a fresh context. +func (s *Service) Close(ctx context.Context) error { + ctx, cancel := context.WithTimeout(ctx, requestTimeout) + defer cancel() + select { + case s.gate <- struct{}{}: + defer func() { <-s.gate }() + case <-ctx.Done(): + return errors.New("token broker shutdown timed out") + } + s.closed = true + s.prune(s.now()) + var failed bool + for key, state := range s.contexts { + if s.revoke(ctx, key, state) != nil { + failed = true + } + } + if failed { + return errors.New("token broker cleanup unavailable") + } + return nil +} diff --git a/server/internal/tokenbroker/service_test.go b/server/internal/tokenbroker/service_test.go new file mode 100644 index 0000000000..3600d6d983 --- /dev/null +++ b/server/internal/tokenbroker/service_test.go @@ -0,0 +1,492 @@ +package tokenbroker + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/hex" + "encoding/json" + "encoding/pem" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync" + "testing" + "time" + + "github.com/golang-jwt/jwt/v4" + + "github.com/githubnext/gh-cao/server/internal/admission" +) + +type verifierFunc func(context.Context, string) (admission.Identity, error) + +func (f verifierFunc) Verify(ctx context.Context, raw string) (admission.Identity, error) { + return f(ctx, raw) +} + +type keyFunc func(context.Context) ([]byte, error) + +func (f keyFunc) PrivateKey(ctx context.Context) ([]byte, error) { return f(ctx) } + +type transportFunc func(*http.Request) (*http.Response, error) + +func (f transportFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +type fixture struct { + service *Service + identity admission.Identity + policy string + now time.Time + mints int + keyReads int + apiReads int + revoked []string + revokeFails bool + installation string + result func() string +} + +func newFixture(t *testing.T) *fixture { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + privateKey := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) + f := &fixture{ + now: time.Now().UTC().Truncate(time.Second), + identity: admission.Identity{ + Issuer: admission.Issuer, Subject: "repo:acme/project:ref:refs/heads/main", + Repository: "acme/project", RepositoryID: "1001", OwnerID: "42", + RunID: "1234", RunAttempt: "2", CheckRunID: "8001", Ref: "refs/heads/main", + Event: "workflow_dispatch", Actor: "octocat", ActorID: "101", + WorkflowRef: "acme/project/.github/workflows/triage.lock.yml@refs/heads/main", + WorkflowSHA: strings.Repeat("a", 40), + }, + installation: `{"app_id":7,"account":{"id":42,"type":"Organization"},"suspended_at":null,"permissions":{"copilot_requests":"read","issues":"write"}}`, + } + var suffix [16]byte + if _, err := rand.Read(suffix[:]); err != nil { + t.Fatal(err) + } + f.policy = ".tokenbroker-test-" + hex.EncodeToString(suffix[:]) + ".json" + t.Cleanup(func() { _ = os.Remove(f.policy) }) + f.writePolicy(t, f.identity) + f.result = func() string { + body, _ := json.Marshal(map[string]any{ + "token": "test-issued-" + strings.Repeat("x", f.mints), + "expires_at": f.now.Add(50 * time.Minute).Format(time.RFC3339), + "permissions": map[string]string{"copilot_requests": "read", "metadata": "read"}, + "repositories": []any{map[string]any{"id": 1001, "owner": map[string]any{"id": 42}}}, + }) + return string(body) + } + client := &http.Client{Transport: transportFunc(func(r *http.Request) (*http.Response, error) { + if r.URL.Scheme != "https" || r.URL.Host != "api.github.com" { + t.Errorf("untrusted upstream destination: %s", r.URL) + } + if r.Header.Get("X-GitHub-Api-Version") != "2026-03-10" { + t.Error("issuer did not use the approved GitHub API version") + } + status, body := 200, f.installation + if r.Method == http.MethodDelete { + if r.URL.Path != "/installation/token" { + t.Error("invalid revocation endpoint") + } + if f.revokeFails { + status = 503 + } else { + status = 204 + f.revoked = append(f.revoked, strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ")) + } + return testResponse(status, ""), nil + } + f.apiReads++ + raw := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + parsed, err := jwt.Parse(raw, func(*jwt.Token) (any, error) { return &key.PublicKey, nil }, jwt.WithValidMethods([]string{"RS256"})) + if err != nil || !parsed.Valid || parsed.Claims.(jwt.MapClaims)["iss"] != "7" { + t.Error("issuer did not use configured App-signed JWT") + } + if r.Method == http.MethodPost { + if r.URL.Path != "/app/installations/11/access_tokens" { + t.Error("invalid token endpoint") + } + payload, err := io.ReadAll(r.Body) + if err != nil || string(payload) != `{"permissions":{"copilot_requests":"read"},"repository_ids":[1001]}` { + t.Errorf("excessive or invalid mint scope: %s", payload) + } + f.mints++ + status, body = 201, f.result() + } else if r.URL.Path != "/app/installations/11" { + t.Error("invalid installation endpoint") + } + return testResponse(status, body), nil + })} + config := Config{ + Audience: "https://broker.example/api/token-broker", PolicyFile: f.policy, + AppID: 7, InstallationID: 11, OwnerID: "42", HTTPClient: client, + Verifier: verifierFunc(func(_ context.Context, raw string) (admission.Identity, error) { + if raw == "unavailable" { + return admission.Identity{}, admission.ErrUnavailable + } + if raw != "valid" { + return admission.Identity{}, admission.ErrDenied + } + return f.identity, nil + }), + KeyReader: keyFunc(func(context.Context) ([]byte, error) { + f.keyReads++ + return append([]byte(nil), privateKey...), nil + }), + Now: func() time.Time { return f.now }, + } + f.service, err = New(config) + if err != nil { + t.Fatal(err) + } + return f +} + +func testResponse(status int, body string) *http.Response { + return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: make(http.Header)} +} + +func (f *fixture) writePolicy(t *testing.T, i admission.Identity) { + t.Helper() + rule := workflowRule{i.RepositoryID, i.OwnerID, i.WorkflowRef, i.WorkflowSHA} + data, _ := json.Marshal(map[string]any{"control-plane": map[string]any{"token-broker": map[string]any{"copilot-requests": map[string]any{"workflows": []workflowRule{rule}}}}}) + if err := os.WriteFile(f.policy, data, 0600); err != nil { + t.Fatal(err) + } +} + +func (f *fixture) call(body string) *httptest.ResponseRecorder { + r := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/token-broker", strings.NewReader(body)) + r.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + f.service.ServeHTTP(w, r) + return w +} + +const exchangeBody = `{"token_type":"copilot-request","provider":"github","oidc_token":"valid"}` +const invalidateBody = `{"operation":"invalidate","oidc_token":"valid"}` + +func TestExchangeAndInvalidation(t *testing.T) { + f := newFixture(t) + w := f.call(exchangeBody) + if w.Code != 200 || w.Header().Get("Cache-Control") != "no-store" || w.Header().Get("Content-Type") != "application/json" { + t.Fatalf("exchange status/headers: %d %v", w.Code, w.Header()) + } + var fields map[string]string + if json.Unmarshal(w.Body.Bytes(), &fields) != nil || len(fields) != 2 || !safeToken(fields["token"]) || fields["expires_at"] == "" { + t.Fatal("invalid success response") + } + if f.mints != 1 || f.keyReads != 1 { + t.Fatal("missing mint or fresh key read") + } + key := executionKey(f.identity) + if strings.Contains(string(f.service.contexts[key].tokens[0].sealed), fields["token"]) { + t.Fatal("credential stored unencrypted") + } + if w = f.call(invalidateBody); w.Code != 204 || w.Body.Len() != 0 || len(f.revoked) != 1 || f.revoked[0] != fields["token"] { + t.Fatal("exact credential invalidation failed") + } + if w = f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("invalidation not idempotent") + } + if w = f.call(exchangeBody); w.Code != 403 || f.mints != 1 { + t.Fatal("invalidated job may mint again") + } +} + +func TestProtocolRejectsBeforeIssuance(t *testing.T) { + f := newFixture(t) + tests := []struct { + body string + code int + }{ + {`{`, 400}, + {`[]`, 400}, + {`null`, 400}, + {`{"token_type":"copilot-task","oidc_token":"valid","provider":"github"}`, 400}, + {`{"token_type":"copilot-request","oidc_token":"valid"}`, 400}, + {`{"token_type":"copilot-request","provider":5,"oidc_token":"valid"}`, 400}, + {`{"token_type":"copilot-request","provider":"github","oidc_token":"valid","extra":""}`, 400}, + {`{"token_type":"copilot-task","oidc_token":"valid","oidc_token":"valid"}`, 400}, + {`{"operation":"invalidate","oidc_token":"valid","token_type":"copilot-task"}`, 400}, + {`{"operation":"unknown","oidc_token":"valid"}`, 400}, + {`{"token_type":"unknown","oidc_token":"valid"}`, 422}, + {`{"token_type":"unknown","oidc_token":4}`, 400}, + {`{"token_type":"unknown","oidc_token":"valid","extra":""}`, 400}, + {`{"token_type":"copilot-task","oidc_token":"invalid"}`, 401}, + {`{"token_type":"copilot-task","oidc_token":"valid"}`, 501}, + {`{"token_type":"copilot-task","oidc_token":"unavailable"}`, 503}, + {`{"token_type":"copilot-request","provider":"https://evil.example","oidc_token":"valid"}`, 403}, + {`{"token_type":"copilot-task","oidc_token":"` + string([]byte{0xff}) + `"}`, 400}, + {strings.Repeat(" ", bodyLimit+1), 413}, + } + for _, tt := range tests { + w := f.call(tt.body) + if w.Code != tt.code || w.Body.Len() != 0 || w.Header().Get("Cache-Control") != "no-store" { + t.Errorf("rejection: got %d want %d", w.Code, tt.code) + } + } + if f.mints != 0 || f.apiReads != 0 || f.keyReads != 0 { + t.Fatal("pre-issuance rejection contacted issuer") + } + for _, media := range []string{"", "text/plain", "application/json; charset=utf-16"} { + r := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/token-broker", strings.NewReader(exchangeBody)) + r.Header.Set("Content-Type", media) + w := httptest.NewRecorder() + f.service.ServeHTTP(w, r) + if w.Code != 415 { + t.Errorf("media rejected with %d", w.Code) + } + } +} + +func TestEndpointAndAuthenticationHeaderCannotSubstituteIdentity(t *testing.T) { + f := newFixture(t) + tests := []struct { + method, path, body string + status int + }{ + {http.MethodGet, "/api/token-broker", "", 405}, + {http.MethodPost, "/api/token-broker/", exchangeBody, 404}, + {http.MethodPost, "/another-route", exchangeBody, 404}, + {http.MethodPost, "/api/token-broker", `{"token_type":"copilot-task","oidc_token":""}`, 400}, + } + for _, tt := range tests { + r := httptest.NewRequestWithContext(context.Background(), tt.method, tt.path, strings.NewReader(tt.body)) + r.Header.Set("Authorization", "******") + r.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + f.service.ServeHTTP(w, r) + if w.Code != tt.status || w.Header().Get("Cache-Control") != "no-store" { + t.Errorf("endpoint: got %d want %d", w.Code, tt.status) + } + } + if f.mints != 0 || f.keyReads != 0 { + t.Fatal("invalid route or substituted identity minted") + } +} + +func TestJobAndPolicyBinding(t *testing.T) { + changes := []struct { + name string + edit func(*admission.Identity) + }{ + {"missing-job", func(i *admission.Identity) { i.CheckRunID = "" }}, + {"invalid-job", func(i *admission.Identity) { i.CheckRunID = "job-name" }}, + {"owner", func(i *admission.Identity) { i.OwnerID = "99" }}, + {"repository", func(i *admission.Identity) { i.RepositoryID = "99" }}, + {"sha", func(i *admission.Identity) { i.WorkflowSHA = strings.Repeat("b", 40) }}, + {"ref", func(i *admission.Identity) { i.Ref = "refs/heads/evil" }}, + {"subject", func(i *admission.Identity) { i.Subject = "repo:acme/project:environment:prod" }}, + {"environment", func(i *admission.Identity) { i.EnvironmentPresent = true }}, + {"pull-request", func(i *admission.Identity) { i.Event = "pull_request" }}, + {"pull-request-target", func(i *admission.Identity) { i.Event = "pull_request_target" }}, + {"run", func(i *admission.Identity) { i.RunID = "" }}, + {"attempt", func(i *admission.Identity) { i.RunAttempt = "0" }}, + {"repository-name", func(i *admission.Identity) { i.Repository = "other/project" }}, + {"reusable-path", func(i *admission.Identity) { + i.WorkflowRef = "other/repo/.github/workflows/triage.lock.yml@refs/heads/main" + }}, + } + for _, tt := range changes { + t.Run(tt.name, func(t *testing.T) { + f := newFixture(t) + tt.edit(&f.identity) + if w := f.call(exchangeBody); w.Code != 403 || f.keyReads != 0 { + t.Fatalf("bad binding minted: %d", w.Code) + } + }) + } + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal(w.Code) + } + if err := os.WriteFile(f.policy, []byte(`{"control-plane":{"token-broker":{"copilot-requests":{"workflows":[]}}}}`), 0600); err != nil { + t.Fatal(err) + } + if w := f.call(exchangeBody); w.Code != 403 || f.mints != 1 { + t.Fatal("policy change did not revoke issuance authority") + } + if err := os.Remove(f.policy); err != nil { + t.Fatal(err) + } + if w := f.call(exchangeBody); w.Code != 403 { + t.Fatal("missing policy allowed mint") + } +} + +func TestRevocationIsolationAndRetry(t *testing.T) { + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal(w.Code) + } + job := f.identity.CheckRunID + f.identity.CheckRunID = "8002" + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 0 { + t.Fatal("another job revoked credential") + } + f.identity.CheckRunID = job + f.revokeFails = true + if w := f.call(invalidateBody); w.Code != 503 || len(f.service.contexts[executionKey(f.identity)].tokens) != 1 { + t.Fatal("failed revocation discarded credential") + } + f.revokeFails = false + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("failed revocation not retryable") + } +} + +func TestCloseRetriesAndStopsIssuance(t *testing.T) { + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal(w.Code) + } + f.revokeFails = true + if f.service.Close(context.Background()) == nil { + t.Fatal("shutdown hid failed revocation") + } + f.revokeFails = false + if err := f.service.Close(context.Background()); err != nil || len(f.revoked) != 1 { + t.Fatal("shutdown retry failed") + } + if w := f.call(exchangeBody); w.Code != 503 || f.mints != 1 { + t.Fatal("closed broker issued credential") + } +} + +func TestConcurrentMintAndInvalidate(t *testing.T) { + f := newFixture(t) + var wg sync.WaitGroup + for range 6 { + wg.Go(func() { + w := f.call(exchangeBody) + if w.Code != 200 && w.Code != 403 && w.Code != 429 { + t.Errorf("concurrent issuance: %d", w.Code) + } + }) + } + wg.Go(func() { + if w := f.call(invalidateBody); w.Code != 204 { + t.Errorf("concurrent invalidation: %d", w.Code) + } + }) + wg.Wait() + if len(f.revoked) != f.mints || len(f.service.contexts[executionKey(f.identity)].tokens) != 0 { + t.Fatal("mint raced cleanup and orphaned a credential") + } +} + +func TestRateBoundsAndFreshKeys(t *testing.T) { + f := newFixture(t) + for range 6 { + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal(w.Code) + } + } + if w := f.call(exchangeBody); w.Code != 429 || f.mints != 6 || f.keyReads != 6 { + t.Fatal("rate or fresh-key requirements violated") + } + f.now = f.now.Add(time.Minute) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal("rate window did not reset") + } + f.now = f.now.Add(2 * time.Hour) + f.service.prune(f.now) + if len(f.service.contexts) != 0 { + t.Fatal("expired state was not pruned") + } +} + +func TestNewFailsClosed(t *testing.T) { + for _, config := range []Config{ + {}, {Audience: "http://broker.example/api/token-broker"}, + {Audience: "https://user@broker.example/api/token-broker"}, + {Audience: "https://broker.example/api/token-broker#"}, + } { + if _, err := New(config); err == nil { + t.Fatal("invalid configuration accepted") + } + } + f := newFixture(t) + f.service.keyReader = keyFunc(func(context.Context) ([]byte, error) { return nil, errors.New("private upstream error") }) + if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 || f.apiReads != 0 { + t.Fatal("key failure leaked or failed open") + } +} + +func TestSafeTokenAlphabet(t *testing.T) { + for _, token := range []string{"", "test token", "test\n", "test\r", "\x00", "test-é", "\x7f", strings.Repeat("x", bodyLimit)} { + if safeToken(token) { + t.Fatal("unsafe credential accepted") + } + } + if !safeToken("!test-token~") { + t.Fatal("safe opaque credential denied") + } +} + +func TestNewRejectsNoncanonicalEndpoint(t *testing.T) { + f := newFixture(t) + for _, audience := range []string{ + "https://broker.example/api/token-broker?tenant=42", + "https://broker.example/api/token-broker?", + "https://broker.example/api/token-broker#", + "https://user@broker.example/api/token-broker", + "https://broker.example/api/token-broker/", + "https://broker.example/api/another-route", + "http://broker.example/api/token-broker", + } { + config := f.service.config + config.Audience = audience + if _, err := New(config); err == nil { + t.Errorf("unsafe public endpoint accepted: %s", audience) + } + } +} + +func TestNewRejectsEmptyVaultFragment(t *testing.T) { + f := newFixture(t) + config := f.service.config + config.KeyReader = nil + config.KeyVaultSecretURL = "https://cao-broker.vault.azure.net/secrets/signing-key" + if _, err := New(config); err != nil { + t.Fatal("valid protected key URL rejected") + } + config.KeyVaultSecretURL += "#" + if _, err := New(config); err == nil { + t.Fatal("empty Key Vault fragment accepted") + } +} + +func TestImmutableSubjectMatchesSameExactJobContext(t *testing.T) { + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal("legacy subject exchange denied") + } + key := executionKey(f.identity) + f.identity.Subject = "repo:acme@42/project@1001:ref:refs/heads/main" + if executionKey(f.identity) != key { + t.Fatal("equivalent subject profiles split exact-job cleanup state") + } + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("immutable subject could not invalidate its exact job") + } + f.identity.Subject = "repo:acme@99/project@1001:ref:refs/heads/main" + if w := f.call(invalidateBody); w.Code != 403 { + t.Fatal("mismatched immutable owner subject was accepted") + } + f.identity.Subject = "repo:acme@42/project@1002:ref:refs/heads/main" + if w := f.call(invalidateBody); w.Code != 403 { + t.Fatal("mismatched immutable repository subject was accepted") + } +} diff --git a/server/internal/tokenbroker/vault.go b/server/internal/tokenbroker/vault.go new file mode 100644 index 0000000000..21a0e57ac0 --- /dev/null +++ b/server/internal/tokenbroker/vault.go @@ -0,0 +1,196 @@ +package tokenbroker + +import ( + "context" + "encoding/json" + "errors" + "io" + "net" + "net/http" + "net/url" + "os" + "regexp" + "strconv" + "strings" + "time" +) + +const vaultResponseLimit = 64 * 1024 + +var ( + vaultHostPattern = regexp.MustCompile(`^[a-z][a-z0-9-]{1,22}[a-z0-9]\.vault\.azure\.net$`) + vaultPathPattern = regexp.MustCompile(`^/secrets/[a-zA-Z0-9-]{1,127}(/[a-zA-Z0-9-]{1,127})?$`) +) + +// KeyVault reads a private key secret using Azure managed identity. +type KeyVault struct { + SecretURL string + ClientID string + HTTPClient *http.Client +} + +// PrivateKey reads the current secret value without caching credentials or keys. +func (v KeyVault) PrivateKey(ctx context.Context) ([]byte, error) { + secretURL, err := url.Parse(v.SecretURL) + if err != nil || strings.Contains(v.SecretURL, "#") || !safeVaultURL(secretURL) { + return nil, errors.New("invalid Key Vault secret URL") + } + + client := http.Client{Timeout: 15 * time.Second} + if v.HTTPClient != nil { + client = *v.HTTPClient + if client.Timeout == 0 { + client.Timeout = 15 * time.Second + } + } + // Copy the client so even an injected client's redirect policy cannot send + // the managed identity token or deployment header to a different endpoint. + client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + client.Jar = nil + + token, err := v.managedIdentityToken(ctx, &client) + if err != nil { + return nil, err + } + query := secretURL.Query() + query.Set("api-version", "7.4") + secretURL.RawQuery = query.Encode() + request, err := http.NewRequestWithContext(ctx, http.MethodGet, secretURL.String(), nil) + if err != nil { + return nil, errors.New("cannot create Key Vault request") + } + request.Header.Set("Authorization", "Bearer "+token) + body, err := vaultRequest(&client, request) + if err != nil { + return nil, errors.New("key vault secret request failed") + } + var secret struct { + Value string `json:"value"` + } + if json.Unmarshal(body, &secret) != nil || strings.TrimSpace(secret.Value) == "" { + return nil, errors.New("invalid Key Vault secret response") + } + return []byte(secret.Value), nil +} + +func safeVaultURL(u *url.URL) bool { + return u != nil && u.Scheme == "https" && u.Opaque == "" && + u.User == nil && u.Port() == "" && u.Host == u.Hostname() && + vaultHostPattern.MatchString(strings.ToLower(u.Hostname())) && + !strings.Contains(u.Hostname(), "--") && + u.RawPath == "" && vaultPathPattern.MatchString(u.Path) && + u.RawQuery == "" && !u.ForceQuery && u.Fragment == "" && u.RawFragment == "" +} + +func (v KeyVault) managedIdentityToken(ctx context.Context, client *http.Client) (string, error) { + endpoint, configured := os.LookupEnv("IDENTITY_ENDPOINT") + header := os.Getenv("IDENTITY_HEADER") + apiVersion := "2018-02-01" + if configured || header != "" { + u, err := url.Parse(endpoint) + if err != nil || strings.Contains(endpoint, "#") || !safeIdentityEndpoint(u) || header == "" || strings.ContainsAny(header, "\r\n") { + return "", errors.New("invalid managed identity configuration") + } + apiVersion = "2019-08-01" + } else { + endpoint = "http://169.254.169.254/metadata/identity/oauth2/token" + } + u, err := url.Parse(endpoint) + if err != nil { + return "", errors.New("invalid managed identity configuration") + } + query := u.Query() + query.Set("api-version", apiVersion) + query.Set("resource", "https://vault.azure.net") + if v.ClientID != "" { + query.Set("client_id", v.ClientID) + } + u.RawQuery = query.Encode() + request, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil) + if err != nil { + return "", errors.New("cannot create managed identity request") + } + if configured { + request.Header.Set("X-IDENTITY-HEADER", header) + } else { + request.Header.Set("Metadata", "true") + } + body, err := vaultRequest(client, request) + if err != nil { + return "", errors.New("managed identity request failed") + } + var response struct { + AccessToken string `json:"access_token"` + TokenType string `json:"token_type"` + ExpiresOn json.RawMessage `json:"expires_on"` + } + if json.Unmarshal(body, &response) != nil || + !validIdentityToken(response.AccessToken) || + (response.TokenType != "" && !strings.EqualFold(response.TokenType, "Bearer")) { + return "", errors.New("invalid managed identity response") + } + expiry := string(response.ExpiresOn) + if strings.HasPrefix(expiry, `"`) { + if json.Unmarshal(response.ExpiresOn, &expiry) != nil { + return "", errors.New("invalid managed identity response") + } + } + expiresOn, err := strconv.ParseInt(expiry, 10, 64) + if err != nil || expiresOn <= time.Now().Unix() { + return "", errors.New("invalid managed identity response") + } + return response.AccessToken, nil +} + +func safeIdentityEndpoint(u *url.URL) bool { + if u == nil || u.Scheme != "http" || u.Opaque != "" || u.User != nil || + u.RawQuery != "" || u.ForceQuery || u.Fragment != "" || u.RawFragment != "" || u.RawPath != "" { + return false + } + ip := net.ParseIP(u.Hostname()) + if ip == nil { + return false + } + // App Service can expose its identity endpoint on this platform-owned + // link-local address rather than loopback. Do not admit its neighbors. + if !ip.IsLoopback() && (u.Hostname() != "169.254.255.2" || u.Path != "/msi/token") { + return false + } + if port := u.Port(); port != "" { + n, err := strconv.Atoi(port) + return err == nil && n > 0 && n <= 65535 + } + return u.Host == u.Hostname() || u.Host == "["+u.Hostname()+"]" +} + +func validIdentityToken(token string) bool { + if token == "" { + return false + } + for _, c := range token { + if c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || + c >= '0' && c <= '9' || strings.ContainsRune("-._~+/=", c) { + continue + } + return false + } + return true +} + +func vaultRequest(client *http.Client, request *http.Request) ([]byte, error) { + response, err := client.Do(request) + if err != nil { + return nil, errors.New("request failed") + } + defer func() { _ = response.Body.Close() }() + if response.StatusCode != http.StatusOK { + return nil, errors.New("unexpected response status") + } + body, err := io.ReadAll(io.LimitReader(response.Body, vaultResponseLimit+1)) + if err != nil || len(body) > vaultResponseLimit { + return nil, errors.New("invalid response body") + } + return body, nil +} diff --git a/server/internal/tokenbroker/vault_test.go b/server/internal/tokenbroker/vault_test.go new file mode 100644 index 0000000000..ad8b23d419 --- /dev/null +++ b/server/internal/tokenbroker/vault_test.go @@ -0,0 +1,267 @@ +package tokenbroker + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "os" + "strings" + "testing" + "time" +) + +type vaultTestTransport func(*http.Request) (*http.Response, error) + +func (f vaultTestTransport) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +} + +func vaultTestResponse(status int, body string) *http.Response { + return &http.Response{StatusCode: status, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(body))} +} + +func vaultTestToken() string { + return fmt.Sprintf(`{"access_token":"identity-token","token_type":"Bearer","expires_on":"%d"}`, time.Now().Add(time.Hour).Unix()) +} + +func TestKeyVaultPrivateKey(t *testing.T) { + for _, appService := range []bool{false, true} { + t.Run(fmt.Sprint(appService), func(t *testing.T) { + // A missing endpoint selects IMDS, whereas an explicitly empty one + // is an invalid App Service configuration. + unsetVaultIdentityEnvironment(t) + if appService { + t.Setenv("IDENTITY_ENDPOINT", "http://127.0.0.1:8081/identity/token") + t.Setenv("IDENTITY_HEADER", "deployment-header") + } + calls := 0 + client := &http.Client{Transport: vaultTestTransport(func(r *http.Request) (*http.Response, error) { + calls++ + if calls%2 == 1 { + if r.URL.Query().Get("resource") != "https://vault.azure.net" || r.URL.Query().Get("client_id") != "selected-client" { + t.Fatalf("incorrect identity query: %v", r.URL) + } + if appService { + if r.URL.Host != "127.0.0.1:8081" || r.Header.Get("X-IDENTITY-HEADER") != "deployment-header" || r.URL.Query().Get("api-version") != "2019-08-01" { + t.Fatal("incorrect App Service request") + } + } else if r.URL.Host != "169.254.169.254" || r.URL.Path != "/metadata/identity/oauth2/token" || r.Header.Get("Metadata") != "true" || r.URL.Query().Get("api-version") != "2018-02-01" { + t.Fatal("incorrect IMDS request") + } + return vaultTestResponse(200, vaultTestToken()), nil + } + if r.URL.String() != "https://example.vault.azure.net/secrets/signing-key/version-1?api-version=7.4" || r.Header.Get("Authorization") != "Bearer "+"identity-token" || r.Header.Get("X-IDENTITY-HEADER") != "" { + t.Fatal("incorrect secret request") + } + return vaultTestResponse(200, fmt.Sprintf(`{"value":"key-%d\n"}`, calls)), nil + })} + vault := KeyVault{SecretURL: "https://example.vault.azure.net/secrets/signing-key/version-1", ClientID: "selected-client", HTTPClient: client} // #nosec G101 -- Offline fixture, not a credential. + for _, expected := range []string{"key-2\n", "key-4\n"} { + key, err := vault.PrivateKey(context.Background()) + if err != nil || string(key) != expected { + t.Fatalf("PrivateKey = %q, %v", key, err) + } + } + if calls != 4 { + t.Fatalf("expected fresh identity and secret reads: %d", calls) + } + if client.CheckRedirect != nil || client.Timeout != 0 { + t.Fatal("caller client was mutated") + } + }) + } +} + +func TestKeyVaultRejectsUnsafeSecretURLs(t *testing.T) { + for _, secretURL := range []string{ + "", "http://example.vault.azure.net/secrets/key", "https://evil.example/secrets/key", + "https://example.vault.azure.net.evil.example/secrets/key", + "https://" + "user" + "@" + "example.vault.azure.net/secrets/key", + "https://example.vault.azure.net:443/secrets/key", "https://example.vault.azure.net:/secrets/key", + "https://example.vault.azure.net/secrets/key?x=y", "https://example.vault.azure.net/secrets/key?", + "https://example.vault.azure.net/secrets/key#fragment", "https://example.vault.azure.net/keys/key", + "https://example.vault.azure.net/secrets/key#", + "https://example.vault.azure.net/secrets/key/", "https://example.vault.azure.net/secrets/../key", + "https://example.vault.azure.net/secrets/%6bey", "https://example.vault.azure.net/secrets/key/version/extra", + "https://bad--vault.vault.azure.net/secrets/key", "https://127.0.0.1/secrets/key", + } { + t.Run(secretURL, func(t *testing.T) { + calls := 0 + vault := KeyVault{SecretURL: secretURL, HTTPClient: &http.Client{Transport: vaultTestTransport(func(*http.Request) (*http.Response, error) { + calls++ + return nil, errors.New("must not request") + })}} + if key, err := vault.PrivateKey(context.Background()); err == nil || key != nil || calls != 0 { + t.Fatalf("unsafe URL accepted: calls=%d err=%v", calls, err) + } + }) + } +} + +func TestKeyVaultRejectsUnsafeIdentityEndpoints(t *testing.T) { + for _, endpoint := range []string{ + "", "https://127.0.0.1/token", "http://localhost/token", "http://169.254.169.254/token", + "http://evil.example/token", "http://127.0.0.1.evil.example/token", "http://user@127.0.0.1/token", + "http://127.0.0.1/token?x=y", "http://127.0.0.1/token?", "http://127.0.0.1/token#x", + "http://127.0.0.1/token#", + "http://127.0.0.1:0/token", "http://127.0.0.1:65536/token", "http://127.0.0.1:/token", + "http://169.254.255.3:8081/msi/token", "http://169.254.255.2:8081/other", + "http://10.0.0.1:8081/msi/token", "http://192.168.1.1:8081/msi/token", + "http://8.8.8.8:8081/msi/token", "http://[::ffff:169.254.255.2]:8081/msi/token", + } { + t.Run(endpoint, func(t *testing.T) { + t.Setenv("IDENTITY_ENDPOINT", endpoint) + t.Setenv("IDENTITY_HEADER", "deployment-header") + calls := 0 + vault := KeyVault{SecretURL: "https://example.vault.azure.net/secrets/key", HTTPClient: &http.Client{Transport: vaultTestTransport(func(*http.Request) (*http.Response, error) { // #nosec G101 -- Offline fixture, not a credential. + calls++ + return nil, errors.New("must not request") + })}} + if _, err := vault.PrivateKey(context.Background()); err == nil || calls != 0 { + t.Fatalf("unsafe identity endpoint accepted: calls=%d err=%v", calls, err) + } + }) + } +} + +func TestKeyVaultIdentityConfiguration(t *testing.T) { + for _, test := range []struct { + name string + endpoint string + header string + valid bool + }{ + {name: "IPv6 loopback", endpoint: "http://[::1]:8080/identity/token", header: "header", valid: true}, + {name: "loopback without port", endpoint: "http://127.0.0.1/token", header: "header", valid: true}, + {name: "App Service platform endpoint", endpoint: "http://169.254.255.2:8081/msi/token", header: "deployment-header", valid: true}, + {name: "missing header", endpoint: "http://127.0.0.1/token"}, + {name: "missing endpoint", header: "header"}, + {name: "invalid header", endpoint: "http://127.0.0.1/token", header: "header\r\nbad"}, + } { + t.Run(test.name, func(t *testing.T) { + unsetVaultIdentityEnvironment(t) + if test.endpoint != "" { + t.Setenv("IDENTITY_ENDPOINT", test.endpoint) + } + t.Setenv("IDENTITY_HEADER", test.header) + calls := 0 + client := &http.Client{Transport: vaultTestTransport(func(r *http.Request) (*http.Response, error) { + calls++ + if calls == 1 { + if r.URL.Query().Has("client_id") { + t.Fatal("unexpected client selection") + } + if r.URL.Scheme+"://"+r.URL.Host+r.URL.Path != test.endpoint || + r.Header.Get("X-IDENTITY-HEADER") != test.header || + r.URL.Query().Get("resource") != "https://vault.azure.net" || + r.URL.Query().Get("api-version") != "2019-08-01" { + t.Fatal("incorrect App Service endpoint or authentication") + } + return vaultTestResponse(200, fmt.Sprintf(`{"access_token":"token","expires_on":%d}`, time.Now().Add(time.Hour).Unix())), nil + } + return vaultTestResponse(200, `{"value":"key"}`), nil + })} + v := KeyVault{SecretURL: "https://example.vault.azure.net/secrets/key", HTTPClient: client} // #nosec G101 -- Offline fixture, not a credential. + key, err := v.PrivateKey(context.Background()) + if test.valid { + if err != nil || string(key) != "key" || calls != 2 { + t.Fatalf("valid configuration failed: key=%q err=%v calls=%d", key, err, calls) + } + } else if err == nil || key != nil || calls != 0 { + t.Fatalf("invalid configuration accepted: err=%v calls=%d", err, calls) + } + }) + } +} + +func TestKeyVaultFailsClosed(t *testing.T) { + for _, test := range []struct { + name string + identity string + secret string + identityErr bool + secretErr bool + status int + secretStage bool + }{ + {name: "malformed identity JSON", identity: "sensitive upstream message"}, + {name: "empty token", identity: `{"access_token":"","expires_on":9999999999}`}, + {name: "whitespace token", identity: `{"access_token":"token value","expires_on":9999999999}`}, + {name: "header injection token", identity: `{"access_token":"token\r\nInjected: true","expires_on":9999999999}`}, + {name: "expired token", identity: `{"access_token":"token","expires_on":1}`}, + {name: "missing expiry", identity: `{"access_token":"token"}`}, + {name: "malformed expiry", identity: `{"access_token":"token","expires_on":"tomorrow"}`}, + {name: "wrong token type", identity: `{"access_token":"token","token_type":"Basic","expires_on":9999999999}`}, + {name: "oversized identity", identity: strings.Repeat("x", vaultResponseLimit+1)}, + {name: "identity transport error", identityErr: true}, + {name: "secret transport error", secretErr: true, secretStage: true}, + {name: "malformed secret JSON", secret: "sensitive upstream message", secretStage: true}, + {name: "empty secret", secret: `{"value":""}`, secretStage: true}, // #nosec G101 -- Empty response fixture, not a credential. + {name: "missing secret", secret: `{}`, secretStage: true}, + {name: "oversized secret", secret: `{"value":"` + strings.Repeat("x", vaultResponseLimit) + `"}`, secretStage: true}, + {name: "identity denied", status: 403}, + {name: "secret denied", status: 403, secretStage: true}, + {name: "identity redirect", status: 302}, + {name: "secret redirect", status: 307, secretStage: true}, + } { + t.Run(test.name, func(t *testing.T) { + unsetVaultIdentityEnvironment(t) + calls := 0 + client := &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { + t.Fatal("injected redirect policy must not run") + return nil + }, + Transport: vaultTestTransport(func(r *http.Request) (*http.Response, error) { + calls++ + if calls > 2 || r.URL.Host == "evil.example" { + t.Fatal("unexpected request or followed redirect") + } + isSecret := calls == 2 + if !isSecret && test.secretStage { + return vaultTestResponse(200, vaultTestToken()), nil + } + if test.identityErr || test.secretErr { + return nil, errors.New("sensitive upstream message") + } + body := test.identity + if isSecret { + body = test.secret + } + status := test.status + if status == 0 { + status = 200 + } + response := vaultTestResponse(status, body) + response.Header.Set("Location", "https://evil.example/steal") + return response, nil + }), + } + vault := KeyVault{SecretURL: "https://example.vault.azure.net/secrets/key", HTTPClient: client} // #nosec G101 -- Offline fixture, not a credential. + key, err := vault.PrivateKey(context.Background()) + if err == nil || key != nil || strings.Contains(err.Error(), "sensitive") || strings.Contains(err.Error(), "identity-token") { + t.Fatalf("expected sanitized failure, got %q, %v", key, err) + } + expectedCalls := 1 + if test.secretStage { + expectedCalls = 2 + } + if calls != expectedCalls { + t.Fatalf("requests = %d, want %d", calls, expectedCalls) + } + }) + } +} + +func unsetVaultIdentityEnvironment(t *testing.T) { + t.Helper() + t.Setenv("IDENTITY_ENDPOINT", "") + t.Setenv("IDENTITY_HEADER", "") + // t.Setenv restores the original values even though this test needs the + // endpoint absent, rather than present with an empty value. + if err := os.Unsetenv("IDENTITY_ENDPOINT"); err != nil { + t.Fatal(err) + } +} diff --git a/server/spec/README.md b/server/spec/README.md index 27d50ecd1a..83349e6d2d 100644 --- a/server/spec/README.md +++ b/server/spec/README.md @@ -4,10 +4,14 @@ `main.tsp` imports independent domains: shared response/auth types, Dashboard Language queries and SSE, health, agent discovery, hosted sessions, canonical entities, repository memory, ingestion/administration, explicit workflow-value -publication, and the optional MCP HTTP +publication, the optional organization Copilot requests token broker, and MCP HTTP transport. MCP protocol messages and tool schemas, and static dashboard assets, are intentionally out of scope. +`token-broker.tsp` specifies the OIDC-in-body exchange and job invalidation +protocol, independently of dashboard session authentication and activation +admission. It supports `copilot-request` only; no task credential is issued. + `storage.tsp` is a separate, **fresh-only** physical Postgres contract. `storage.tspconfig.yaml` runs the repository-owned `postgres-emitter.mjs` to generate `../internal/postgresx/schema.sql` and `schema.gen.go`. The SQL file is diff --git a/server/spec/contract.test.mjs b/server/spec/contract.test.mjs index d662bfb799..ecebc40c20 100644 --- a/server/spec/contract.test.mjs +++ b/server/spec/contract.test.mjs @@ -51,7 +51,7 @@ test('agent discovery describes public GET and body-free HEAD responses', () => test('the generated contract describes the implemented security and wire formats', () => { assert.equal(openapi.openapi, '3.1.0') - assert.equal(openapi.info.version, '2.12.0') + assert.equal(openapi.info.version, '2.13.0') for (const path of ['/api/runs/{id}/jobs', '/api/runs/{id}/sessions', '/api/sessions/{id}/events']) { assert.equal(openapi.paths[path], undefined) } @@ -111,6 +111,20 @@ test('the generated contract describes the implemented security and wire formats ) }) +test('token broker exposes only the OIDC exchange and exact credential response', () => { + const operation = openapi.paths['/api/token-broker'].post + assert.equal(operation.security, undefined) + assert.ok(operation.requestBody.content['application/json']) + for (const status of ['200', '204', '400', '401', '403', '413', '415', '422', '429', '501', '503']) { + assert.deepEqual(operation.responses[status].headers['cache-control'].schema.enum, ['no-store']) + if (status !== '200') assert.equal(operation.responses[status].content, undefined) + } + assert.deepEqual(Object.keys(openapi.components.schemas.BrokerCredential.properties).sort(), ['expires_at', 'token']) + assert.equal(openapi.components.schemas.CopilotRequestExchange.properties.token_type.enum[0], 'copilot-request') + assert.equal(openapi.components.schemas.CopilotRequestExchange.properties.provider.enum[0], 'github') + assert.equal(openapi.components.schemas.TokenBrokerInvalidation.properties.operation.enum[0], 'invalidate') +}) + test('private archive queries preserve explicit full-content authority and the shared event contract', () => { const status = openapi.paths['/api/v1/session-archive/status'].get const query = openapi.paths['/api/v1/session-archive/query'].post diff --git a/server/spec/generated/openapi.json b/server/spec/generated/openapi.json index 353e62be02..37ff7c61df 100644 --- a/server/spec/generated/openapi.json +++ b/server/spec/generated/openapi.json @@ -2,7 +2,7 @@ "openapi": "3.1.0", "info": { "title": "CAO dashboard server", - "version": "2.12.0", + "version": "2.13.0", "description": "The Go dashboard server HTTP contract. Local mode uses a loopback bearer capability; hosted mode uses an authorized GitHub OAuth session and X-CSRF-Token on protected mutations. The optional MCP HTTP transport is covered, but its protocol messages and tool schemas are owned by the MCP contract; dashboard static assets are outside this contract." }, "tags": [ @@ -41,6 +41,9 @@ }, { "name": "Private session archive" + }, + { + "name": "Token broker" } ], "paths": { @@ -1223,6 +1226,199 @@ ] } }, + "/api/token-broker": { + "post": { + "operationId": "TokenBroker_exchange", + "description": "Optional, request-only organization GitHub App broker implementing the gh-aw exchange shape. Authentication is exclusively the oidc_token in the JSON body, not OAuth, CSRF, a bearer header, or admission. The exact configured HTTPS endpoint is the OIDC audience. Enrolled immutable owner/repository IDs, workflow reference and SHA, trusted event/ref and signed job identity must match before issuance. App private keys are read from Azure Key Vault using managed identity. All responses are no-store with empty failure bodies. This initial single-process profile does not claim integrated-deployment conformance; actual Copilot use and renewal require operator acceptance.", + "parameters": [], + "responses": { + "200": { + "description": "The request has succeeded.", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + }, + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BrokerCredential" + } + } + } + }, + "204": { + "description": "There is no content to send for this request, but the headers may be useful. ", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "400": { + "description": "The server could not understand the request due to invalid syntax.", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "401": { + "description": "Access is unauthorized.", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "403": { + "description": "Access is forbidden.", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "413": { + "description": "Client error", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "415": { + "description": "Client error", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "422": { + "description": "Client error", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "429": { + "description": "Client error", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "501": { + "description": "Server error", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + }, + "503": { + "description": "Service unavailable.", + "headers": { + "cache-control": { + "required": true, + "schema": { + "type": "string", + "enum": [ + "no-store" + ] + } + } + } + } + }, + "tags": [ + "Token broker" + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "anyOf": [ + { + "$ref": "#/components/schemas/CopilotRequestExchange" + }, + { + "$ref": "#/components/schemas/UnsupportedCopilotTaskExchange" + }, + { + "$ref": "#/components/schemas/TokenBrokerInvalidation" + } + ] + } + } + } + } + } + }, "/api/v1/diagnostics": { "get": { "operationId": "Queries_diagnostics", @@ -4328,6 +4524,27 @@ } } }, + "BrokerCredential": { + "type": "object", + "required": [ + "token", + "expires_at" + ], + "properties": { + "token": { + "type": "string", + "minLength": 1 + }, + "expires_at": { + "type": "string", + "format": "date-time" + } + }, + "unevaluatedProperties": { + "not": {} + }, + "description": "Only the credential and its actual upstream expiry are returned; no provider endpoint or other authority is conveyed." + }, "CollectionStatus": { "type": "object", "required": [ @@ -4626,6 +4843,37 @@ } ] }, + "CopilotRequestExchange": { + "type": "object", + "required": [ + "token_type", + "provider", + "oidc_token" + ], + "properties": { + "token_type": { + "type": "string", + "enum": [ + "copilot-request" + ] + }, + "provider": { + "type": "string", + "enum": [ + "github" + ] + }, + "oidc_token": { + "type": "string", + "minLength": 1, + "description": "Signed GitHub Actions identity with the exact public exchange URL as its string audience. Never a GitHub API token." + } + }, + "unevaluatedProperties": { + "not": {} + }, + "description": "GitHub Actions OIDC exchange for organization-billed Copilot inference. Additional and duplicate members are rejected." + }, "DataState": { "type": "object", "required": [ @@ -6448,6 +6696,29 @@ } } }, + "TokenBrokerInvalidation": { + "type": "object", + "required": [ + "operation", + "oidc_token" + ], + "properties": { + "operation": { + "type": "string", + "enum": [ + "invalidate" + ] + }, + "oidc_token": { + "type": "string", + "minLength": 1 + } + }, + "unevaluatedProperties": { + "not": {} + }, + "description": "Cleanup obtains a fresh identity for the same verified job; it never includes an issued credential." + }, "Trace.Archive.Event": { "type": "object", "required": [ @@ -7177,6 +7448,29 @@ } ] }, + "UnsupportedCopilotTaskExchange": { + "type": "object", + "required": [ + "token_type", + "oidc_token" + ], + "properties": { + "token_type": { + "type": "string", + "enum": [ + "copilot-task" + ] + }, + "oidc_token": { + "type": "string", + "minLength": 1 + } + }, + "unevaluatedProperties": { + "not": {} + }, + "description": "A known but unsupported capability; identity and enrollment are verified before returning 501." + }, "UsageArtifactID": { "type": "string", "minLength": 1, diff --git a/server/spec/main.tsp b/server/spec/main.tsp index ba8b259ffa..67fd8bfd20 100644 --- a/server/spec/main.tsp +++ b/server/spec/main.tsp @@ -15,11 +15,12 @@ import "./value.tsp"; import "./mcp.tsp"; import "./admission.tsp"; import "./session-archive.tsp"; +import "./token-broker.tsp"; using TypeSpec.Http; using TypeSpec.OpenAPI; @service(#{ title: "CAO dashboard server" }) -@info(#{ version: "2.12.0" }) +@info(#{ version: "2.13.0" }) @doc("The Go dashboard server HTTP contract. Local mode uses a loopback bearer capability; hosted mode uses an authorized GitHub OAuth session and X-CSRF-Token on protected mutations. The optional MCP HTTP transport is covered, but its protocol messages and tool schemas are owned by the MCP contract; dashboard static assets are outside this contract.") namespace Cao; diff --git a/server/spec/token-broker.tsp b/server/spec/token-broker.tsp new file mode 100644 index 0000000000..9cd6449ab9 --- /dev/null +++ b/server/spec/token-broker.tsp @@ -0,0 +1,55 @@ +import "@typespec/http"; + +using TypeSpec.Http; +namespace Cao; + +/** GitHub Actions OIDC exchange for organization-billed Copilot inference. Additional and duplicate members are rejected. */ +model CopilotRequestExchange { + ...Record; + token_type: "copilot-request"; + provider: "github"; + /** Signed GitHub Actions identity with the exact public exchange URL as its string audience. Never a GitHub API token. */ + @minLength(1) oidc_token: string; +} + +/** A known but unsupported capability; identity and enrollment are verified before returning 501. */ +model UnsupportedCopilotTaskExchange { + ...Record; + token_type: "copilot-task"; + @minLength(1) oidc_token: string; +} + +/** Cleanup obtains a fresh identity for the same verified job; it never includes an issued credential. */ +model TokenBrokerInvalidation { + ...Record; + operation: "invalidate"; + @minLength(1) oidc_token: string; +} + +/** Only the credential and its actual upstream expiry are returned; no provider endpoint or other authority is conveyed. */ +model BrokerCredential { + ...Record; + @minLength(1) token: string; + expires_at: utcDateTime; +} + +@tag("Token broker") +namespace TokenBroker { + /** Optional, request-only organization GitHub App broker implementing the gh-aw exchange shape. Authentication is exclusively the oidc_token in the JSON body, not OAuth, CSRF, a bearer header, or admission. The exact configured HTTPS endpoint is the OIDC audience. Enrolled immutable owner/repository IDs, workflow reference and SHA, trusted event/ref and signed job identity must match before issuance. App private keys are read from Azure Key Vault using managed identity. All responses are no-store with empty failure bodies. This initial single-process profile does not claim integrated-deployment conformance; actual Copilot use and renewal require operator acceptance. */ + @route("/api/token-broker") + @post op exchange( + @header("content-type") contentType: "application/json", + @body body: CopilotRequestExchange | UnsupportedCopilotTaskExchange | TokenBrokerInvalidation + ): + { @statusCode code: 200; @header("cache-control") cacheControl: "no-store"; @body body: BrokerCredential } + | { @statusCode code: 204; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 400; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 401; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 403; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 413; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 415; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 422; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 429; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 501; @header("cache-control") cacheControl: "no-store" } + | { @statusCode code: 503; @header("cache-control") cacheControl: "no-store" }; +} diff --git a/tests/unit/control-token-broker.test.mjs b/tests/unit/control-token-broker.test.mjs new file mode 100644 index 0000000000..bad5f32957 --- /dev/null +++ b/tests/unit/control-token-broker.test.mjs @@ -0,0 +1,172 @@ +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import test from "node:test"; +import { controlSettings, parsePolicy, PolicyError } from "../../.github/workflows/shared/policy.mjs"; + +const schema = JSON.parse(readFileSync(new URL("../../.github/workflows/shared/cao.schema.json", import.meta.url), "utf8")); +const brokerSchema = schema.$defs.tokenBroker; +const workflow = { + "repository-id": "123", + "owner-id": "456", + "workflow-ref": "owner/repo/.github/workflows/name.lock.yml@refs/heads/main", + "workflow-sha": "0123456789abcdef0123456789abcdef01234567", +}; + +function document(broker) { + return { + version: 1, + "gh-aw-version": "v1.0.1", + "control-plane": { "token-broker": broker }, + }; +} + +function broker(workflows = [workflow]) { + return { "copilot-requests": { workflows } }; +} + +// Evaluate the small schema vocabulary used by the broker, without another dependency. +function matchesSchema(value, definition) { + switch (definition.type) { + case "object": + return value !== null && typeof value === "object" && !Array.isArray(value) + && definition.required.every(key => Object.hasOwn(value, key)) + && Object.entries(value).every(([key, child]) => + Object.hasOwn(definition.properties, key) + && matchesSchema(child, definition.properties[key])); + case "array": + return Array.isArray(value) && value.length <= definition.maxItems + && value.every(child => matchesSchema(child, definition.items)) + && (!definition.uniqueItems || value.every((child, index) => + !value.slice(0, index).some(previous => + Object.keys(child).every(key => child[key] === previous[key])))); + case "string": + return typeof value === "string" && new RegExp(definition.pattern).test(value); + default: + throw new Error(`unexpected schema type ${definition.type}`); + } +} + +function accepts(value) { + assert.equal(matchesSchema(value, brokerSchema), true, JSON.stringify(value)); + const input = document(value); + assert.deepEqual(parsePolicy(JSON.stringify(input)), input); +} + +function rejects(value) { + assert.equal(matchesSchema(value, brokerSchema), false, JSON.stringify(value)); + assert.throws(() => parsePolicy(JSON.stringify(document(value))), PolicyError); +} + +test("token broker schema is opt-in and closed at every mapping level", () => { + assert.equal(schema.$defs.controlPlane.properties["token-broker"].$ref, "#/$defs/tokenBroker"); + assert.equal(schema.$defs.controlPlane.required?.includes("token-broker") ?? false, false); + assert.equal(brokerSchema.additionalProperties, false); + assert.deepEqual(brokerSchema.required, ["copilot-requests"]); + const requests = brokerSchema.properties["copilot-requests"]; + assert.equal(requests.additionalProperties, false); + assert.deepEqual(requests.required, ["workflows"]); + assert.equal(requests.properties.workflows.maxItems, 128); + assert.equal(requests.properties.workflows.uniqueItems, true); + const entry = requests.properties.workflows.items; + assert.equal(entry.additionalProperties, false); + assert.deepEqual(entry.required, Object.keys(workflow)); + assert.deepEqual(Object.keys(entry.properties), Object.keys(workflow)); +}); + +test("omitted and empty token broker authorizations remain disabled without changing rollout settings", () => { + const missing = { version: 1, "gh-aw-version": "v1.0.1", "control-plane": {} }; + assert.deepEqual(parsePolicy(JSON.stringify(missing)), missing); + accepts(broker([])); + const populated = document(broker()); + populated["control-plane"].scope = { "allowed-repositories": ["different/control"] }; + assert.deepEqual(parsePolicy(JSON.stringify(populated)), populated); + const baseline = structuredClone(populated); + delete baseline["control-plane"]["token-broker"]; + assert.deepEqual(controlSettings(populated, "different/control"), controlSettings(baseline, "different/control")); + const checkedIn = parsePolicy(readFileSync(new URL("../../.github/workflows/cao.json", import.meta.url), "utf8")); + assert.deepEqual(checkedIn["control-plane"]["token-broker"], broker([])); +}); + +test("token broker accepts exact branch/tag workflow pins and preserves all fields", () => { + accepts(broker()); + accepts(broker([{ + ...workflow, + "repository-id": "12345678901234567890", + "owner-id": "1", + "workflow-ref": "some-owner/some.repo/.github/workflows/build_test.yaml@refs/tags/releases/v1.2-rc_1", + }])); + accepts(broker(Array.from({ length: 128 }, (_, index) => ({ + ...workflow, "repository-id": String(index + 1), + })))); + accepts(broker([workflow, { ...workflow, "workflow-sha": "f".repeat(40) }])); +}); + +test("token broker rejects missing fields, unknown capabilities, and malformed mappings", () => { + for (const value of [ + null, [], "disabled", {}, { "copilot-requests": null }, + { "copilot-requests": [] }, { "copilot-requests": {} }, + { "copilot-requests": { workflows: null } }, + { "copilot-requests": { workflows: {} } }, + { ...broker(), "copilot-tasks": {} }, + { ...broker(), enabled: true }, + { "copilot-requests": { workflows: [], token: "forbidden" } }, + broker([null]), broker([[]]), broker(["workflow"]), + broker([{ ...workflow, mode: "live" }]), + broker([{ ...workflow, "copilot-tasks": true }]), + ]) rejects(value); + for (const key of Object.keys(workflow)) { + const incomplete = { ...workflow }; + delete incomplete[key]; + rejects(broker([incomplete])); + } +}); + +test("token broker enforces canonical positive decimal IDs and lowercase full SHAs", () => { + for (const key of ["repository-id", "owner-id"]) { + for (const value of [0, 123, null, "", "0", "01", "-1", "+1", "1.0", "1e2", "1 ", " 1", "1\n", "1\r", "1".repeat(21), "${{ github.repository_id }}"]) { + rejects(broker([{ ...workflow, [key]: value }])); + } + } + for (const value of [null, 123, "", "a".repeat(39), "a".repeat(41), "A".repeat(40), "g".repeat(40), `${"a".repeat(40)}\n`, "${{ github.sha }}"]) { + rejects(broker([{ ...workflow, "workflow-sha": value }])); + } +}); + +test("token broker rejects ambiguous, unsafe, or unpinned workflow references", () => { + const prefix = "owner/repo/.github/workflows/name.lock.yml@"; + for (const value of [ + null, 123, "", "*", "owner/repo/.github/workflows/name.lock.yml", + `${prefix}main`, `${prefix}${"a".repeat(40)}`, `${prefix}refs/pull/1/merge`, + `${prefix}refs/heads/`, `${prefix}refs/tags/`, `${prefix}refs/heads/*`, + `${prefix}refs/heads/../main`, `${prefix}refs/heads/feature/../../main`, + `${prefix}refs/heads/.hidden`, `${prefix}refs/heads/feature/.hidden`, + `${prefix}refs/heads/main.`, `${prefix}refs/heads/main.lock`, + `${prefix}refs/heads/main.lock/other`, `${prefix}refs/heads/a..b`, + `${prefix}refs/heads/feature//main`, `${prefix}refs/heads/main/`, + `${prefix}refs/heads/main\n`, `${prefix}refs/heads/main\r`, + `${prefix}refs/heads/main\u0000`, `${prefix}refs/heads/main\u007f`, + `${prefix}refs/heads/main\u0085`, `${prefix}refs/heads/main\u2028`, + `${prefix}refs/heads/a b`, `${prefix}refs/heads/a?b`, + `${prefix}refs/heads/a[b`, `${prefix}refs/heads/a~b`, + `${prefix}refs/heads/a^b`, `${prefix}refs/heads/a:b`, + `${prefix}refs/heads/a\\b`, `${prefix}refs/heads/a@{b`, + `${prefix}refs/heads/\${{ github.ref }}`, + "owner/*/.github/workflows/name.yml@refs/heads/main", + "owner/../.github/workflows/name.yml@refs/heads/main", + "owner/repo/.github/workflows/../name.yml@refs/heads/main", + "owner/repo/.github/workflows/nested/name.yml@refs/heads/main", + "owner/repo/.github/workflows/name.md@refs/heads/main", + "owner/repo/.github/workflows/name.yml@refs/heads/main@refs/tags/v1", + "https://github.com/owner/repo/.github/workflows/name.yml@refs/heads/main", + ]) rejects(broker([{ ...workflow, "workflow-ref": value }])); +}); + +test("token broker bounds the allowlist and rejects duplicate objects regardless of key order", () => { + rejects(broker(Array.from({ length: 129 }, (_, index) => ({ + ...workflow, "repository-id": String(index + 1), + })))); + rejects(broker([workflow, { ...workflow }])); + rejects(broker([workflow, Object.fromEntries(Object.entries(workflow).reverse())])); + const duplicateKey = JSON.stringify(document(broker())).replace('"repository-id":"123"', '"repository-id":"123","repository-id":"123"'); + assert.throws(() => parsePolicy(duplicateKey), /duplicate mapping key/); +}); From 00ece5d7f6f09bde9bbeaec8f63be5b9f69c4bee Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 31 Oct 2026 00:30:41 +0000 Subject: [PATCH 2/2] Harden broker cleanup, policy validation, and response deadlines Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- server/internal/server/token_broker.go | 5 +- server/internal/server/token_broker_test.go | 11 ++- server/internal/tokenbroker/issuer.go | 9 ++- server/internal/tokenbroker/issuer_test.go | 2 +- server/internal/tokenbroker/lifecycle_test.go | 79 +++++++++++++++++++ server/internal/tokenbroker/policy.go | 9 ++- server/internal/tokenbroker/service.go | 14 ++-- 7 files changed, 117 insertions(+), 12 deletions(-) create mode 100644 server/internal/tokenbroker/lifecycle_test.go diff --git a/server/internal/server/token_broker.go b/server/internal/server/token_broker.go index 224bcc1ccf..e2e3d11bf7 100644 --- a/server/internal/server/token_broker.go +++ b/server/internal/server/token_broker.go @@ -104,7 +104,10 @@ func (a *App) exchangeBrokerToken(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusServiceUnavailable) return } - defer func() { _ = controller.SetWriteDeadline(time.Time{}) }() + defer func() { + _ = controller.Flush() + _ = controller.SetWriteDeadline(time.Time{}) + }() http.TimeoutHandler(a.tokenBroker, time.Until(deadline), "").ServeHTTP( &admissionResponse{ResponseWriter: w, headers: make(http.Header)}, r.WithContext(ctx)) } diff --git a/server/internal/server/token_broker_test.go b/server/internal/server/token_broker_test.go index 9fbd141c3e..3e29de7b2d 100644 --- a/server/internal/server/token_broker_test.go +++ b/server/internal/server/token_broker_test.go @@ -17,7 +17,13 @@ type deniedBrokerIdentity struct{} type brokerDeadlineWriter struct { *httptest.ResponseRecorder - read, write []time.Time + read, write []time.Time + boundedFlush bool +} + +func (w *brokerDeadlineWriter) Flush() { + w.boundedFlush = len(w.write) > 0 && !w.write[len(w.write)-1].IsZero() + w.ResponseRecorder.Flush() } func (w *brokerDeadlineWriter) SetReadDeadline(deadline time.Time) error { @@ -174,6 +180,9 @@ func TestTokenBrokerUsesOIDCInsteadOfDashboardAuthentication(t *testing.T) { want := http.StatusForbidden if endpoint == "https://broker.example/api/token-broker" { want = http.StatusUnauthorized + if !response.boundedFlush { + t.Fatal("broker cleared its transport deadline before flushing the response") + } for _, deadlines := range [][]time.Time{response.read, response.write} { if len(deadlines) != 2 || deadlines[0].IsZero() || !deadlines[0].Before(time.Now().Add(9*time.Second)) || !deadlines[1].IsZero() { diff --git a/server/internal/tokenbroker/issuer.go b/server/internal/tokenbroker/issuer.go index 8343a7e020..9f11013c3b 100644 --- a/server/internal/tokenbroker/issuer.go +++ b/server/internal/tokenbroker/issuer.go @@ -15,7 +15,10 @@ import ( "github.com/githubnext/gh-cao/server/internal/admission" ) -const githubAPI = "https://api.github.com" +const ( + githubAPI = "https://api.github.com" + githubAPIBodyLimit = 256 << 10 +) var errUpstream = errors.New("credential issuer unavailable") @@ -116,8 +119,8 @@ func upstream(ctx context.Context, client *http.Client, method, endpoint, token return nil, errUpstream } defer func() { _ = res.Body.Close() }() - body, err := io.ReadAll(io.LimitReader(res.Body, bodyLimit+1)) - if err != nil || len(body) > bodyLimit { + body, err := io.ReadAll(io.LimitReader(res.Body, githubAPIBodyLimit+1)) + if err != nil || len(body) > githubAPIBodyLimit { return nil, errUpstream } if res.StatusCode != expected { diff --git a/server/internal/tokenbroker/issuer_test.go b/server/internal/tokenbroker/issuer_test.go index 954e8d07c5..16e19d6d76 100644 --- a/server/internal/tokenbroker/issuer_test.go +++ b/server/internal/tokenbroker/issuer_test.go @@ -131,7 +131,7 @@ func TestUpstreamTransportFailsClosed(t *testing.T) { } f := newFixture(t) f.service.client.Transport = transportFunc(func(*http.Request) (*http.Response, error) { - return testResponse(200, strings.Repeat("x", bodyLimit+1)), nil + return testResponse(200, strings.Repeat("x", githubAPIBodyLimit+1)), nil }) if w := f.call(exchangeBody); w.Code != 503 || w.Body.Len() != 0 { t.Fatal("oversized upstream response accepted") diff --git a/server/internal/tokenbroker/lifecycle_test.go b/server/internal/tokenbroker/lifecycle_test.go new file mode 100644 index 0000000000..d28572fc7c --- /dev/null +++ b/server/internal/tokenbroker/lifecycle_test.go @@ -0,0 +1,79 @@ +package tokenbroker + +import ( + "os" + "strings" + "testing" +) + +func TestPreviouslyAuthorizedCleanupSurvivesPolicyRevocation(t *testing.T) { + for _, change := range []string{"remove-policy", "empty-enrollment", "rotate-sha"} { + t.Run(change, func(t *testing.T) { + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal("initial authorized exchange failed") + } + switch change { + case "remove-policy": + if err := os.Remove(f.policy); err != nil { + t.Fatal(err) + } + case "empty-enrollment": + if err := os.WriteFile(f.policy, []byte(`{"control-plane":{"token-broker":{"copilot-requests":{"workflows":[]}}}}`), 0600); err != nil { + t.Fatal(err) + } + case "rotate-sha": + current := f.identity + current.WorkflowSHA = strings.Repeat("b", 40) + f.writePolicy(t, current) + } + if w := f.call(exchangeBody); w.Code != 403 || f.mints != 1 { + t.Fatal("revoked enrollment still permitted issuance") + } + job := f.identity.CheckRunID + f.identity.CheckRunID = "8002" + if w := f.call(invalidateBody); w.Code != 403 || len(f.revoked) != 0 { + t.Fatal("another untracked job bypassed current policy") + } + f.identity.CheckRunID = job + f.identity.EnvironmentPresent = true + if w := f.call(invalidateBody); w.Code != 403 || len(f.revoked) != 0 { + t.Fatal("unsupported signed profile bypassed cleanup authorization") + } + f.identity.EnvironmentPresent = false + subject := f.identity.Subject + f.identity.Subject = "repo:acme@99/project@1001:ref:refs/heads/main" + if w := f.call(invalidateBody); w.Code != 403 || len(f.revoked) != 0 { + t.Fatal("mismatched signed subject bypassed cleanup authorization") + } + f.identity.Subject = subject + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("policy change prevented exact previously authorized cleanup") + } + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("previously authorized cleanup was not idempotent") + } + if w := f.call(exchangeBody); w.Code != 403 || f.mints != 1 { + t.Fatal("cleanup restored revoked issuance authority") + } + }) + } +} + +func TestCleanupRetrySurvivesPolicyRemoval(t *testing.T) { + f := newFixture(t) + if w := f.call(exchangeBody); w.Code != 200 { + t.Fatal(w.Code) + } + if err := os.Remove(f.policy); err != nil { + t.Fatal(err) + } + f.revokeFails = true + if w := f.call(invalidateBody); w.Code != 503 || len(f.service.contexts[executionKey(f.identity)].tokens) != 1 { + t.Fatal("failed cleanup lost a revoked-policy credential") + } + f.revokeFails = false + if w := f.call(invalidateBody); w.Code != 204 || len(f.revoked) != 1 { + t.Fatal("revoked-policy cleanup could not be retried") + } +} diff --git a/server/internal/tokenbroker/policy.go b/server/internal/tokenbroker/policy.go index ac5f4f10b7..ba088e5c29 100644 --- a/server/internal/tokenbroker/policy.go +++ b/server/internal/tokenbroker/policy.go @@ -23,7 +23,7 @@ type workflowRule struct { WorkflowSHA string `json:"workflow-sha"` } -func (s *Service) authorized(i admission.Identity) bool { +func (s *Service) authorizedIdentity(i admission.Identity) bool { if !stableID.MatchString(i.RepositoryID) || !stableID.MatchString(i.OwnerID) || !stableID.MatchString(i.RunID) || !stableID.MatchString(i.RunAttempt) || !stableID.MatchString(i.CheckRunID) || i.EnvironmentPresent || i.OwnerID != s.config.OwnerID || @@ -44,6 +44,13 @@ func (s *Service) authorized(i admission.Identity) bool { default: return false } + return true +} + +func (s *Service) authorized(i admission.Identity) bool { + if !s.authorizedIdentity(i) { + return false + } // Read on every operation. A changed or removed mounted policy immediately // denies authority instead of retaining a stale authorization snapshot. f, err := os.Open(s.config.PolicyFile) diff --git a/server/internal/tokenbroker/service.go b/server/internal/tokenbroker/service.go index c968dfaab7..988f12b0a8 100644 --- a/server/internal/tokenbroker/service.go +++ b/server/internal/tokenbroker/service.go @@ -187,7 +187,15 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusServiceUnavailable) return } - if !s.authorized(identity) { + now := s.now() + s.prune(now) + key := executionKey(identity) + state := s.contexts[key] + // Policy revocation stops minting, not cleanup of a context this process + // previously authorized. The verified profile and complete execution key + // still have to match; an untracked context requires current enrollment. + trackedCleanup := input.invalidate && state != nil && s.authorizedIdentity(identity) + if !trackedCleanup && !s.authorized(identity) { w.WriteHeader(http.StatusForbidden) return } @@ -199,10 +207,6 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusForbidden) return } - now := s.now() - s.prune(now) - key := executionKey(identity) - state := s.contexts[key] if s.closed { w.WriteHeader(http.StatusServiceUnavailable) return