From 080de4e1b8d8c4b05bded2e8383fc5b9b4e13674 Mon Sep 17 00:00:00 2001 From: Joshua Gilman Date: Mon, 27 Apr 2026 08:09:16 -0700 Subject: [PATCH] fix(security)!: harden broker deployment boundaries Reject unsafe GitHub API URLs and repository path inputs, sanitize upstream GitHub error handling, and tighten Terraform validation for release, SSM, and KMS inputs. Remove the broken Terraform-managed Lambda Function URL surface so callers use direct Lambda invocation or provide their own validated HTTP adapter. BREAKING CHANGE: The Terraform module no longer accepts enable_function_url and no longer exposes a function_url output or function-url example. --- .github/workflows/terraform.yml | 36 +++++- docs/docs/explanation/architecture.md | 8 +- docs/docs/explanation/security-model.md | 3 +- .../use-with-github-enterprise-server.md | 2 +- docs/docs/reference/environment-variables.md | 14 +-- docs/docs/reference/errors.md | 11 +- docs/docs/reference/iam-permissions.md | 4 +- docs/docs/reference/ssm-parameter-shapes.md | 2 +- .../tutorials/deploy-your-first-broker.md | 2 +- internal/config/config.go | 33 +++-- internal/config/config_test.go | 47 +++++++- internal/githubapp/client.go | 40 ++++++- internal/githubapp/client_test.go | 81 ++++++++++++- internal/integration/broker_test.go | 2 +- terraform/README.md | 17 ++- terraform/examples/function-url/README.md | 20 ---- terraform/examples/function-url/main.tf | 36 ------ terraform/examples/function-url/outputs.tf | 9 -- .../function-url/terraform.tfvars.example | 5 - terraform/examples/function-url/variables.tf | 24 ---- terraform/main.tf | 28 ++--- terraform/outputs.tf | 5 - terraform/security_validations.tftest.hcl | 113 ++++++++++++++++++ terraform/variables.tf | 47 ++++---- 24 files changed, 400 insertions(+), 189 deletions(-) delete mode 100644 terraform/examples/function-url/README.md delete mode 100644 terraform/examples/function-url/main.tf delete mode 100644 terraform/examples/function-url/outputs.tf delete mode 100644 terraform/examples/function-url/terraform.tfvars.example delete mode 100644 terraform/examples/function-url/variables.tf create mode 100644 terraform/security_validations.tftest.hcl diff --git a/.github/workflows/terraform.yml b/.github/workflows/terraform.yml index bcdb05c..172d01f 100644 --- a/.github/workflows/terraform.yml +++ b/.github/workflows/terraform.yml @@ -52,7 +52,6 @@ jobs: directory: - terraform - terraform/examples/basic - - terraform/examples/function-url - terraform/examples/with-ssm-bootstrap steps: - name: Checkout @@ -73,6 +72,41 @@ jobs: working-directory: ${{ matrix.directory }} run: terraform validate -no-color + - name: terraform test + if: matrix.directory == 'terraform' + working-directory: terraform + run: terraform test -no-color + + - name: removed Function URL input is rejected + if: matrix.directory == 'terraform' + shell: bash + run: | + tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT + + cat > "$tmpdir/main.tf" <"$tmpdir/validate.out" 2>&1; then + echo "expected enable_function_url to be rejected" >&2 + exit 1 + fi + + grep -q "Unsupported argument" "$tmpdir/validate.out" + lint: runs-on: ubuntu-latest permissions: diff --git a/docs/docs/explanation/architecture.md b/docs/docs/explanation/architecture.md index 65f83f3..fe94e51 100644 --- a/docs/docs/explanation/architecture.md +++ b/docs/docs/explanation/architecture.md @@ -23,7 +23,7 @@ flowchart LR end GitHub["GitHub API
(github.com or GHES)"] Caller -->|InvokeFunction
empty payload| Lambda - Lambda -->|POST /app/installations/
{id}/access_tokens| GitHub + Lambda -->|GET /repos/owner/repo/installation
POST /app/installations/.../access_tokens| GitHub Lambda -->|token JSON| Caller ``` @@ -45,9 +45,11 @@ sequenceDiagram L->>S: GetParameters (3 names, WithDecryption) S-->>L: client_id, installation_id, private_key_pem L->>L: Sign RS256 JWT
(iss=client_id, iat=now-60s, exp=now+9min) + L->>G: GET /repos/{owner}/{repo}/installation + G-->>L: installation metadata + L->>L: Confirm installation id matches config L->>G: POST /app/installations/{id}/access_tokens
+ permissions, repositories G-->>L: { token, expires_at, ... } - L->>L: Validate returned repositories match config L-->>C: { token, expires_at, repositories, permissions } ``` @@ -59,7 +61,7 @@ The broker is stateless. There is no database, no cache, no disk writes. Every f ## Cold start shape -On a cold start, Go bootstraps (fast — the binary is ~7 MB statically linked) and the first invocation runs the full mint flow. There is no warmup or prefetch. Steady-state invocation latency is dominated by three network round trips: SSM `GetParameters`, GitHub `POST /access_tokens`, and the invocation return. Each is typically sub-100 ms in the same AWS region. +On a cold start, Go bootstraps (fast — the binary is ~7 MB statically linked) and the first invocation runs the full mint flow. There is no warmup or prefetch. Steady-state invocation latency is dominated by SSM `GetParameters`, GitHub repository-installation preflight, GitHub `POST /access_tokens`, and the invocation return. Each is typically sub-100 ms in the same AWS region. ## Boundaries diff --git a/docs/docs/explanation/security-model.md b/docs/docs/explanation/security-model.md index bfb5c1b..cd53d63 100644 --- a/docs/docs/explanation/security-model.md +++ b/docs/docs/explanation/security-model.md @@ -14,7 +14,7 @@ The broker's purpose is to narrow the surface over which a GitHub App's long-liv - **Token theft from caller environments.** Callers only ever see short-lived installation tokens, not the PEM. Stealing a token buys the attacker roughly one hour on one repository with one permission set — and the window closes on its own. - **Scope escalation via caller input.** The scope of a minted token — repository, permissions — is fixed at deploy time. A caller cannot request a wider scope. See [Why empty payloads are enforced](./why-empty-payloads). -- **Credential exfiltration via logs.** The broker never logs the minted token. CloudWatch Logs for the function contain only the repositories and expiration time. The PEM is never logged under any code path. +- **Credential exfiltration via logs.** The broker never logs the minted token. Success logs contain only the repositories and expiration time, and GitHub error response bodies are not copied into failure logs. The PEM is never logged under any code path. - **Casual access to the PEM at rest.** The PEM is stored as an SSM `SecureString`, encrypted with either the AWS-managed SSM key or a customer-managed KMS key. Reading the parameter value requires `ssm:GetParameters` with `WithDecryption`. ### Not defended against @@ -49,6 +49,7 @@ Both are under the AWS account's control. An attacker with the ability to modify - The PEM is never emitted to logs, traces, or response bodies. - The token is never emitted to logs, traces, or any place other than the invocation response. - IAM access to the three SSM parameters is tightly scoped — no wildcards, no `GetParameter` (singular), no write actions. See [IAM permissions](../reference/iam-permissions). +- Terraform and runtime configuration reject wildcard SSM paths; Terraform rejects wildcard KMS ARNs; the GitHub client rejects non-HTTPS API URLs except loopback `http` for local tests. ## See also diff --git a/docs/docs/how-to/use-with-github-enterprise-server.md b/docs/docs/how-to/use-with-github-enterprise-server.md index 6ef48e5..6ec2a56 100644 --- a/docs/docs/how-to/use-with-github-enterprise-server.md +++ b/docs/docs/how-to/use-with-github-enterprise-server.md @@ -6,7 +6,7 @@ description: Point the broker at a GitHub Enterprise Server instance instead of # Use with GitHub Enterprise Server -The broker targets `https://api.github.com` by default. Overriding the API base URL points it at GitHub Enterprise Server (GHES). +The broker targets `https://api.github.com` by default. Overriding the API base URL points it at GitHub Enterprise Server (GHES). The GHES API URL must use `https`; plain `http` is rejected except for loopback URLs used by local tests. ## Before you start diff --git a/docs/docs/reference/environment-variables.md b/docs/docs/reference/environment-variables.md index 4ba9c43..994e700 100644 --- a/docs/docs/reference/environment-variables.md +++ b/docs/docs/reference/environment-variables.md @@ -11,19 +11,19 @@ The broker reads all runtime configuration from environment variables. When depl | Variable | Required | Default | Notes | |---|---|---|---| | `AWS_REGION` | Yes | — | Provided by the Lambda runtime. Required for SDK initialization. | -| `GITHUB_TOKEN_BROKER_REPOSITORY_OWNER` | Yes | — | GitHub owner the minted token is scoped to. Trimmed. | -| `GITHUB_TOKEN_BROKER_REPOSITORY_NAME` | Yes | — | GitHub repository name the minted token is scoped to. Trimmed. | -| `GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM` | No | `/github-token-broker/app/client-id` | SSM parameter path for the GitHub App client ID. Must be absolute (start with `/`). | -| `GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM` | No | `/github-token-broker/app/installation-id` | SSM parameter path for the installation ID. Must be absolute. | -| `GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM` | No | `/github-token-broker/app/private-key-pem` | SSM SecureString parameter path for the private key PEM. Must be absolute. | +| `GITHUB_TOKEN_BROKER_REPOSITORY_OWNER` | Yes | — | GitHub owner the minted token is scoped to. Trimmed; may contain only letters, numbers, periods, underscores, and hyphens. | +| `GITHUB_TOKEN_BROKER_REPOSITORY_NAME` | Yes | — | GitHub repository name the minted token is scoped to. Trimmed; may contain only letters, numbers, periods, underscores, and hyphens. | +| `GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM` | No | `/github-token-broker/app/client-id` | SSM parameter path for the GitHub App client ID. Must be an absolute literal path. | +| `GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM` | No | `/github-token-broker/app/installation-id` | SSM parameter path for the installation ID. Must be an absolute literal path. | +| `GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM` | No | `/github-token-broker/app/private-key-pem` | SSM SecureString parameter path for the private key PEM. Must be an absolute literal path. | | `GITHUB_TOKEN_BROKER_PERMISSIONS` | No | `{"contents":"read"}` | JSON object of string-to-string permission entries. Must parse to a non-empty object; keys and values must be non-empty. | -| `GITHUB_TOKEN_BROKER_GITHUB_API_BASE_URL` | No | `https://api.github.com` | GitHub API base URL. Override for GitHub Enterprise Server. | +| `GITHUB_TOKEN_BROKER_GITHUB_API_BASE_URL` | No | `https://api.github.com` | GitHub API base URL. Override for GitHub Enterprise Server. Must use `https` except for loopback `http` URLs used in local tests. | | `GITHUB_TOKEN_BROKER_LOG_LEVEL` | No | `info` | One of `debug`, `info`, `warn`, `error`. Passed to `slog`. | ## Notes - `AWS_REGION` is reserved by the Lambda runtime and injected automatically. Do not set it in Terraform; the broker's configuration loader reads it from the process environment like any other variable. -- SSM parameter paths are validated at startup. A non-absolute path causes the process to exit before taking traffic. +- SSM parameter paths are validated at startup. They must start with `/` and contain only letters, numbers, periods, underscores, hyphens, and slashes; wildcard characters are rejected. - The private-key parameter **must** be `SecureString` so SSM returns it encrypted and the broker decrypts it in-flight. - An empty or missing `GITHUB_TOKEN_BROKER_PERMISSIONS` falls back to `{"contents":"read"}`. diff --git a/docs/docs/reference/errors.md b/docs/docs/reference/errors.md index 51928ab..0a6ee51 100644 --- a/docs/docs/reference/errors.md +++ b/docs/docs/reference/errors.md @@ -14,19 +14,22 @@ All broker errors are returned as Lambda function errors (not as a `200` with an | `AWS_REGION is required` | The Lambda started without a region in the environment. | Should never happen under the Lambda runtime. If reproducing locally, set `AWS_REGION`. | | `GITHUB_TOKEN_BROKER_REPOSITORY_OWNER is required` | Required config missing. | Set the Terraform module's `repository_owner` input. | | `GITHUB_TOKEN_BROKER_REPOSITORY_NAME is required` | Required config missing. | Set the Terraform module's `repository_name` input. | -| `GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM must be an absolute SSM parameter path` | SSM path does not start with `/`. Applies to all three parameter-path variables. | Use an absolute path in the module's `ssm_parameter_paths` input. | +| `GITHUB_TOKEN_BROKER_REPOSITORY_OWNER contains unsupported characters` | Owner contains path separators, escapes, or another unsupported character. | Use the literal GitHub owner only: letters, numbers, periods, underscores, and hyphens. | +| `GITHUB_TOKEN_BROKER_REPOSITORY_NAME contains unsupported characters` | Repository name contains path separators, escapes, or another unsupported character. | Use the literal GitHub repository name only: letters, numbers, periods, underscores, and hyphens. | +| `GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM must be an absolute literal SSM parameter path` | SSM path is relative or contains unsupported characters. Applies to all three parameter-path variables. | Use an absolute literal path in the module's `ssm_parameter_paths` input; do not use `*` or `?`. | +| `GitHub API base URL must use https unless the host is loopback` | `GITHUB_TOKEN_BROKER_GITHUB_API_BASE_URL` points to a non-loopback `http` URL. | Use `https` for github.com and GHES. Plain `http` is only accepted for loopback local tests. | | `GITHUB_TOKEN_BROKER_PERMISSIONS must be a JSON object of string-to-string entries` | `GITHUB_TOKEN_BROKER_PERMISSIONS` is not valid JSON or has non-string values. | Supply a JSON object like `{"contents":"read"}`. | | `missing GitHub App SSM parameters: [...]` | One or more SSM parameters do not exist at the configured paths. | Create the parameters (see [SSM parameter shapes](./ssm-parameter-shapes)), or fix the paths. | | SSM `AccessDeniedException` | The Lambda's role lacks `ssm:GetParameters` on the parameter ARNs, or `kms:Decrypt` on the CMK. | Verify the module's IAM policy matches the parameter paths and CMK. See [IAM permissions](./iam-permissions). | | JWT signing failure / PEM parse error | The private-key parameter value is not a valid PEM. Common causes: line endings mangled during `put-parameter`, wrong parameter updated, truncated file. | Re-upload the PEM with `$(cat key.pem)` to preserve content. See [rotate-github-app-private-key](../how-to/rotate-github-app-private-key). | -| GitHub `401 Unauthorized` on `/app/installations/{id}/access_tokens` | The private key does not match the App's active keys, or the client ID does not match the App. | Verify the PEM corresponds to a currently-active private key on the App, and the client ID is the App's client ID (not App ID). | -| GitHub `404 Not Found` on `/app/installations/{id}/access_tokens` | The installation ID is wrong, or the App was uninstalled from the repository. | Verify the installation ID, and that the App is installed on the target repo. | +| GitHub request failure with `status 401` | The private key does not match the App's active keys, or the client ID does not match the App. | Verify the PEM corresponds to a currently-active private key on the App, and the client ID is the App's client ID (not App ID). | +| GitHub request failure with `status 404` | The repository, installation ID, or GHES API base URL is wrong, or the App was uninstalled from the repository. | Verify the target repository, installation ID, and that the App is installed on the target repo. | | Broker error mentioning the target repository | The configured repository is not covered by the installation's repository selection. | Update the App's installation to include the repo, or change the target (see [change-target-repository](../how-to/change-target-repository)). | ## Where errors surface - **AWS CLI**: `FunctionError` on `aws lambda invoke`; the message is in the response body. -- **CloudWatch Logs**: every failure logs an `ERROR` line with the message. The token is never logged on success or failure. +- **CloudWatch Logs**: every failure logs an `ERROR` line with the message. The token is never logged on success or failure, and upstream GitHub response bodies are not copied into logs. - **Caller code**: if invoking via the AWS SDK, errors surface as `Lambda.ServiceException`-family exceptions with the message in the `FunctionError` field. ## See also diff --git a/docs/docs/reference/iam-permissions.md b/docs/docs/reference/iam-permissions.md index b1284f4..4659a4d 100644 --- a/docs/docs/reference/iam-permissions.md +++ b/docs/docs/reference/iam-permissions.md @@ -25,7 +25,7 @@ The Terraform module provisions the Lambda's execution role with a least-privile } ``` -The actual parameter paths come from the module's `ssm_parameter_paths` input and default to the paths shown above. Only `ssm:GetParameters` (plural) is granted — the broker fetches all three in one batched call. +The actual parameter paths come from the module's `ssm_parameter_paths` input and default to the paths shown above. The module accepts only absolute literal SSM paths and rejects wildcard characters so the generated ARN resources stay exact. Only `ssm:GetParameters` (plural) is granted — the broker fetches all three in one batched call. ### Decrypt the private key (conditional) @@ -40,7 +40,7 @@ Present only when the module's `kms_key_arn` variable is set: } ``` -When the SecureString parameter uses the AWS-managed SSM key (`alias/aws/ssm`), this statement is omitted; the AWS-managed key grants decrypt via SSM's service principal automatically. +When the SecureString parameter uses the AWS-managed SSM key (`alias/aws/ssm`), this statement is omitted; the AWS-managed key grants decrypt via SSM's service principal automatically. When `kms_key_arn` is set, the module requires a literal KMS key or alias ARN and rejects wildcard characters. ### Write CloudWatch logs diff --git a/docs/docs/reference/ssm-parameter-shapes.md b/docs/docs/reference/ssm-parameter-shapes.md index 1b0ed58..dde767d 100644 --- a/docs/docs/reference/ssm-parameter-shapes.md +++ b/docs/docs/reference/ssm-parameter-shapes.md @@ -14,7 +14,7 @@ The broker reads three SSM parameters in a single `GetParameters` call with `Wit | `/github-token-broker/app/installation-id` | `String` | The numeric installation ID as a string (e.g. `"12345678"`). Visible in the GitHub App's installation URL. | | `/github-token-broker/app/private-key-pem` | `SecureString` | A PEM-encoded RSA private key, starting with `-----BEGIN RSA PRIVATE KEY-----`. The entire file contents, including the `BEGIN`/`END` lines and trailing newline. | -All three paths are overridable via environment variables — see [Environment variables](./environment-variables). +All three paths are overridable via environment variables — see [Environment variables](./environment-variables). Custom paths must be absolute literal SSM parameter names using only letters, numbers, periods, underscores, hyphens, and slashes; wildcard characters are rejected because these paths are also rendered into IAM resource ARNs. ## Client ID, not App ID diff --git a/docs/docs/tutorials/deploy-your-first-broker.md b/docs/docs/tutorials/deploy-your-first-broker.md index aeb3ef1..7055994 100644 --- a/docs/docs/tutorials/deploy-your-first-broker.md +++ b/docs/docs/tutorials/deploy-your-first-broker.md @@ -142,7 +142,7 @@ The response should be your repository name. ## What just happened -On each invocation the Lambda reads the three SSM parameters in one batched call (with decryption), signs an RS256 JWT valid for 9 minutes with a 60-second backdated `iat`, exchanges the JWT for an installation token via `POST /app/installations/{id}/access_tokens`, and returns the token to you. It is stateless — nothing is cached across invocations. See [Architecture](../explanation/architecture) for the full diagram. +On each invocation the Lambda reads the three SSM parameters in one batched call (with decryption), signs an RS256 JWT valid for 9 minutes with a 60-second backdated `iat`, verifies that the configured repository belongs to the configured installation, exchanges the JWT for an installation token via `POST /app/installations/{id}/access_tokens`, and returns the token to you. It is stateless — nothing is cached across invocations. See [Architecture](../explanation/architecture) for the full diagram. ## Next steps diff --git a/internal/config/config.go b/internal/config/config.go index aff0c91..7659c2f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "os" + "regexp" "strings" ) @@ -19,6 +20,11 @@ const ( defaultPermissionLevel = "read" ) +var ( + githubLiteralNamePattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) + ssmParameterPathPattern = regexp.MustCompile(`^/[A-Za-z0-9_.\-/]+$`) +) + // Config is the runtime configuration for github-token-broker. type Config struct { // AWSRegion is the AWS region used for SDK configuration. @@ -43,9 +49,10 @@ type Config struct { // Load reads environment variables into a Config. // -// Load returns an error when required variables are missing, when SSM parameter -// paths are not absolute, or when the permissions environment variable does not -// parse into a non-empty JSON object of string-to-string entries. +// Load returns an error when required variables are missing, when repository +// names or SSM parameter paths contain unsupported characters, or when the +// permissions environment variable does not parse into a non-empty JSON object +// of string-to-string entries. func Load() (Config, error) { cfg := Config{ AWSRegion: os.Getenv("AWS_REGION"), @@ -62,26 +69,34 @@ func Load() (Config, error) { return Config{}, fmt.Errorf("AWS_REGION is required") } - if !strings.HasPrefix(cfg.ClientIDParameter, "/") { - return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM must be an absolute SSM parameter path") + if !ssmParameterPathPattern.MatchString(cfg.ClientIDParameter) { + return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM must be an absolute literal SSM parameter path") } - if !strings.HasPrefix(cfg.InstallationIDParameter, "/") { - return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM must be an absolute SSM parameter path") + if !ssmParameterPathPattern.MatchString(cfg.InstallationIDParameter) { + return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM must be an absolute literal SSM parameter path") } - if !strings.HasPrefix(cfg.PrivateKeyParameter, "/") { - return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM must be an absolute SSM parameter path") + if !ssmParameterPathPattern.MatchString(cfg.PrivateKeyParameter) { + return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM must be an absolute literal SSM parameter path") } if cfg.RepositoryOwner == "" { return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_REPOSITORY_OWNER is required") } + if !githubLiteralNamePattern.MatchString(cfg.RepositoryOwner) { + return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_REPOSITORY_OWNER contains unsupported characters") + } + if cfg.RepositoryName == "" { return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_REPOSITORY_NAME is required") } + if !githubLiteralNamePattern.MatchString(cfg.RepositoryName) { + return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_REPOSITORY_NAME contains unsupported characters") + } + if cfg.GitHubAPIBaseURL == "" { return Config{}, fmt.Errorf("GITHUB_TOKEN_BROKER_GITHUB_API_BASE_URL must not be empty") } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7ac6251..4733451 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -73,7 +73,42 @@ func TestLoadAcceptsArbitraryOwnerRepo(t *testing.T) { assert.Equal(t, "infra", cfg.RepositoryName) } -func TestLoadRejectsRelativeParameterPath(t *testing.T) { +func TestLoadRejectsUnsafeOwnerRepo(t *testing.T) { + tests := []struct { + name string + envKey string + envVar string + wantMsg string + }{ + { + name: "owner path escape", + envKey: "GITHUB_TOKEN_BROKER_REPOSITORY_OWNER", + envVar: "acme/widgets", + wantMsg: "GITHUB_TOKEN_BROKER_REPOSITORY_OWNER contains unsupported characters", + }, + { + name: "repository percent escape", + envKey: "GITHUB_TOKEN_BROKER_REPOSITORY_NAME", + envVar: "widgets%2fadmin", + wantMsg: "GITHUB_TOKEN_BROKER_REPOSITORY_NAME contains unsupported characters", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + setRequiredEnv(t) + t.Setenv(tt.envKey, tt.envVar) + + _, err := Load() + + require.Error(t, err) + assert.ErrorContains(t, err, tt.wantMsg) + }) + } +} + +func TestLoadRejectsInvalidParameterPath(t *testing.T) { tests := []struct { name string envVar string @@ -84,19 +119,19 @@ func TestLoadRejectsRelativeParameterPath(t *testing.T) { name: "client id", envVar: "github-token-broker/app/client-id", envKey: "GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM", - wantMsg: "GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM must be an absolute SSM parameter path", + wantMsg: "GITHUB_TOKEN_BROKER_CLIENT_ID_PARAM must be an absolute literal SSM parameter path", }, { name: "installation id", - envVar: "github-token-broker/app/installation-id", + envVar: "/github-token-broker/app/*", envKey: "GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM", - wantMsg: "GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM must be an absolute SSM parameter path", + wantMsg: "GITHUB_TOKEN_BROKER_INSTALLATION_ID_PARAM must be an absolute literal SSM parameter path", }, { name: "private key", - envVar: "github-token-broker/app/private-key-pem", + envVar: "/github-token-broker/app/private key", envKey: "GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM", - wantMsg: "GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM must be an absolute SSM parameter path", + wantMsg: "GITHUB_TOKEN_BROKER_PRIVATE_KEY_PARAM must be an absolute literal SSM parameter path", }, } diff --git a/internal/githubapp/client.go b/internal/githubapp/client.go index 6d5e956..ae5301f 100644 --- a/internal/githubapp/client.go +++ b/internal/githubapp/client.go @@ -15,8 +15,10 @@ import ( "encoding/pem" "fmt" "io" + "net" "net/http" "net/url" + "regexp" "strconv" "strings" "time" @@ -24,6 +26,8 @@ import ( const jwtLifetime = 9 * time.Minute +var githubLiteralName = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) + // AppConfig contains the GitHub App material needed to mint installation tokens. type AppConfig struct { // ClientID is the GitHub App client ID used as the JWT issuer. @@ -85,6 +89,10 @@ func NewClient(httpClient HTTPDoer, baseURL string, clock Clock) (*Client, error return nil, fmt.Errorf("GitHub API base URL must be absolute") } + if parsedBaseURL.Scheme != "https" && !(parsedBaseURL.Scheme == "http" && isLoopbackHost(parsedBaseURL.Hostname())) { + return nil, fmt.Errorf("GitHub API base URL must use https unless the host is loopback") + } + if clock == nil { clock = time.Now } @@ -98,9 +106,10 @@ func NewClient(httpClient HTTPDoer, baseURL string, clock Clock) (*Client, error // CreateInstallationToken mints an installation token for the given target. // -// CreateInstallationToken signs a short-lived RS256 JWT, POSTs it to -// /app/installations/{id}/access_tokens, and returns the token and its -// expiration. Errors never include the App private key material. +// CreateInstallationToken signs a short-lived RS256 JWT, verifies the target +// repository's installation, POSTs to /app/installations/{id}/access_tokens, +// and returns the token and its expiration. Errors never include the App +// private key material or raw upstream response bodies. func (c *Client) CreateInstallationToken(ctx context.Context, app AppConfig, target Target) (InstallationToken, error) { if app.ClientID == "" { return InstallationToken{}, fmt.Errorf("GitHub App client ID is required") @@ -114,10 +123,18 @@ func (c *Client) CreateInstallationToken(ctx context.Context, app AppConfig, tar return InstallationToken{}, fmt.Errorf("GitHub repository owner is required") } + if !githubLiteralName.MatchString(target.Owner) { + return InstallationToken{}, fmt.Errorf("GitHub repository owner contains unsupported characters") + } + if target.Repository == "" { return InstallationToken{}, fmt.Errorf("GitHub repository name is required") } + if !githubLiteralName.MatchString(target.Repository) { + return InstallationToken{}, fmt.Errorf("GitHub repository name contains unsupported characters") + } + jwt, err := c.signJWT(app) if err != nil { return InstallationToken{}, err @@ -164,7 +181,7 @@ func (c *Client) CreateInstallationToken(ctx context.Context, app AppConfig, tar } if resp.StatusCode < 200 || resp.StatusCode > 299 { - return InstallationToken{}, fmt.Errorf("GitHub installation-token request failed with status %d: %s", resp.StatusCode, strings.TrimSpace(string(responseBody))) + return InstallationToken{}, fmt.Errorf("GitHub installation-token request failed with status %d", resp.StatusCode) } var tokenResponse struct { @@ -213,7 +230,7 @@ func (c *Client) validateRepositoryInstallation(ctx context.Context, jwt string, } if resp.StatusCode < 200 || resp.StatusCode > 299 { - return fmt.Errorf("GitHub repository-installation request failed with status %d: %s", resp.StatusCode, strings.TrimSpace(string(responseBody))) + return fmt.Errorf("GitHub repository-installation request failed with status %d", resp.StatusCode) } var installationResponse struct { @@ -234,6 +251,19 @@ func (c *Client) validateRepositoryInstallation(ctx context.Context, jwt string, return nil } +func isLoopbackHost(host string) bool { + if host == "" { + return false + } + + if strings.EqualFold(host, "localhost") { + return true + } + + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + func (c *Client) signJWT(app AppConfig) (string, error) { privateKey, err := parsePrivateKey(app.PrivateKeyPEM) if err != nil { diff --git a/internal/githubapp/client_test.go b/internal/githubapp/client_test.go index 4d78e26..a7ee23a 100644 --- a/internal/githubapp/client_test.go +++ b/internal/githubapp/client_test.go @@ -143,7 +143,7 @@ func TestCreateInstallationTokenSurfacesGitHubErrorsWithoutPrivateKey(t *testing if req.Method == http.MethodGet { return jsonResponse(http.StatusOK, `{"id":123}`), nil } - return jsonResponse(http.StatusForbidden, `{"message":"bad credentials"}`), nil + return jsonResponse(http.StatusForbidden, `{"message":"bad credentials","jwt":"`+privateKey+`"}`), nil }) client, err := NewClient(httpClient, "https://api.github.test", nil) require.NoError(t, err) @@ -162,9 +162,58 @@ func TestCreateInstallationTokenSurfacesGitHubErrorsWithoutPrivateKey(t *testing require.Error(t, err) assert.ErrorContains(t, err, "status 403") + assert.NotContains(t, err.Error(), "bad credentials") assert.NotContains(t, err.Error(), privateKey) } +func TestCreateInstallationTokenRejectsUnsafeTarget(t *testing.T) { + privateKey := testPrivateKeyPEM(t) + httpClient := roundTripFunc(func(req *http.Request) (*http.Response, error) { + t.Fatalf("HTTP call should not be issued for an unsafe target") + return nil, nil + }) + client, err := NewClient(httpClient, "https://api.github.test", nil) + require.NoError(t, err) + + tests := []struct { + name string + target Target + wantErr string + }{ + { + name: "owner with slash", + target: Target{ + Owner: "acme/other", + Repository: "widgets", + }, + wantErr: "GitHub repository owner contains unsupported characters", + }, + { + name: "repository with percent escape", + target: Target{ + Owner: "acme", + Repository: "widgets%2fother", + }, + wantErr: "GitHub repository name contains unsupported characters", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err := client.CreateInstallationToken(context.Background(), AppConfig{ + ClientID: "Iv1.client", + InstallationID: "123", + PrivateKeyPEM: privateKey, + }, tt.target) + + require.Error(t, err) + assert.ErrorContains(t, err, tt.wantErr) + assert.NotContains(t, err.Error(), privateKey) + }) + } +} + func TestCreateInstallationTokenRejectsInvalidPrivateKey(t *testing.T) { httpClient := roundTripFunc(func(req *http.Request) (*http.Response, error) { t.Fatalf("HTTP call should not be issued for a malformed key") @@ -194,6 +243,36 @@ func TestNewClientRejectsRelativeBaseURL(t *testing.T) { assert.ErrorContains(t, err, "absolute") } +func TestNewClientValidatesBaseURLScheme(t *testing.T) { + tests := []struct { + name string + baseURL string + wantErr bool + }{ + {name: "github https", baseURL: "https://api.github.com"}, + {name: "ghes https", baseURL: "https://ghe.example.com/api/v3"}, + {name: "localhost http", baseURL: "http://localhost:8080"}, + {name: "ipv4 loopback http", baseURL: "http://127.0.0.1:8080"}, + {name: "ipv6 loopback http", baseURL: "http://[::1]:8080"}, + {name: "non-loopback http", baseURL: "http://api.github.test", wantErr: true}, + {name: "unsupported scheme", baseURL: "ftp://api.github.test", wantErr: true}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err := NewClient(nil, tt.baseURL, nil) + + if tt.wantErr { + require.Error(t, err) + assert.ErrorContains(t, err, "must use https") + return + } + require.NoError(t, err) + }) + } +} + func assertJWTClaims(t *testing.T, token string, expected map[string]any) { t.Helper() diff --git a/internal/integration/broker_test.go b/internal/integration/broker_test.go index efe1d7f..0242a41 100644 --- a/internal/integration/broker_test.go +++ b/internal/integration/broker_test.go @@ -185,7 +185,7 @@ func TestBrokerReportsGitHubTokenRejection(t *testing.T) { body := result.requireError(t) assert.Contains(t, body, "status 403") - assert.Contains(t, body, "denied") + assert.NotContains(t, body, "denied") assertNoLogLeak(t, result, key.privatePEM, github.lastJWT()) github.assertRequests(t, 2) } diff --git a/terraform/README.md b/terraform/README.md index 56f50d5..b127295 100644 --- a/terraform/README.md +++ b/terraform/README.md @@ -2,7 +2,7 @@ Deploys [`github-token-broker`](https://github.com/meigma/github-token-broker) as an AWS Lambda, sourced from a published GitHub Release asset. -The module is opinionated in what matters for supply-chain integrity (SHA256 verification on download, least-privilege IAM, `AWS_IAM`-only Function URL) and configurable everywhere else (memory, timeout, tags, log retention, permissions set, SSM parameter paths, KMS). +The module is opinionated in what matters for supply-chain integrity (SHA256 verification on download, least-privilege IAM, direct Lambda invocation only) and configurable everywhere else (memory, timeout, tags, log retention, permissions set, SSM parameter paths, KMS). ## Usage @@ -30,7 +30,7 @@ Three ways to source the Lambda zip via `lambda_artifact`: Exactly one of the three must be set; a validation rule enforces this. -See [`examples/basic`](./examples/basic) for the smallest viable config, [`examples/function-url`](./examples/function-url) for a Function URL deployment, and [`examples/with-ssm-bootstrap`](./examples/with-ssm-bootstrap) for first-time SSM parameter creation. +See [`examples/basic`](./examples/basic) for the smallest viable config and [`examples/with-ssm-bootstrap`](./examples/with-ssm-bootstrap) for first-time SSM parameter creation. ## Verification @@ -83,7 +83,7 @@ End-to-end validation requires an AWS account and a GitHub App with an installat ## Security notes - IAM policy grants only `ssm:GetParameters` on the three configured paths, `kms:Decrypt` on the explicit CMK ARN when provided, and `logs:CreateLogStream`/`logs:PutLogEvents` on the module-managed log group. No wildcards on sensitive actions. -- Function URLs are always `AWS_IAM`-authenticated. The module will not create a `NONE`-auth URL. +- The module exposes direct Lambda invocation only. Put API Gateway, Function URLs, or another HTTP adapter in front of the broker only if that adapter validates method, path, query string, and body before invoking the Lambda. - The Lambda rejects non-empty invocation payloads, so the deployed `permissions` set is the upper bound — callers cannot request more. - `AWS_REGION` is provided by the Lambda runtime automatically; the module does not set it explicitly. - `CHANGELOG.md` and the release page are the source of truth for what's in a given `release_version`. The module performs SHA256 verification against `checksums.txt`, not signature verification — use `gh attestation verify` for supply-chain assurance. @@ -123,7 +123,6 @@ No modules. | [aws_iam_role.lambda](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) | resource | | [aws_iam_role_policy.lambda](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource | | [aws_lambda_function.broker](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_function) | resource | -| [aws_lambda_function_url.broker](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_function_url) | resource | | [null_resource.fetch_release](https://registry.terraform.io/providers/hashicorp/null/latest/docs/resources/resource) | resource | | [aws_caller_identity.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) | data source | | [aws_iam_policy_document.assume_role](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source | @@ -136,19 +135,18 @@ No modules. | Name | Description | Type | Default | Required | |------|-------------|------|---------|:--------:| | [architecture](#input\_architecture) | Lambda architecture. arm64 matches the published release zip. | `string` | `"arm64"` | no | -| [enable\_function\_url](#input\_enable\_function\_url) | Create a Lambda Function URL with AWS\_IAM auth. Never creates a NONE-auth URL. | `bool` | `false` | no | | [function\_name](#input\_function\_name) | Name of the Lambda function. | `string` | n/a | yes | -| [github\_api\_base\_url](#input\_github\_api\_base\_url) | GitHub API base URL. Override for GitHub Enterprise Server. | `string` | `"https://api.github.com"` | no | -| [kms\_key\_arn](#input\_kms\_key\_arn) | KMS key ARN used by SSM to encrypt the private key parameter. Set only when the customer uses a CMK instead of the AWS-managed key. Null disables kms:Decrypt in the role policy. | `string` | `null` | no | +| [github\_api\_base\_url](#input\_github\_api\_base\_url) | GitHub API base URL. Override for GitHub Enterprise Server. The Lambda requires https except for loopback http URLs used in local tests. | `string` | `"https://api.github.com"` | no | +| [kms\_key\_arn](#input\_kms\_key\_arn) | Literal KMS key or alias ARN used by SSM to encrypt the private key parameter. Set only when the customer uses a CMK instead of the AWS-managed key. Null disables kms:Decrypt in the role policy. | `string` | `null` | no | | [lambda\_artifact](#input\_lambda\_artifact) | Source of the Lambda zip. Exactly one of the three fields must be set:

- `release_version`: a tag published on `release_repository` (e.g. "v1.0.0").
The module downloads `github-token-broker.zip` and `checksums.txt` via the
`gh` CLI on the machine running `terraform apply`, verifies the zip's
SHA256 against `checksums.txt`, and points the Lambda at the cached copy.
- `lambda_zip_path`: absolute path to a pre-downloaded zip. Used for
air-gapped workflows where `gh` is unavailable at apply time.
- `lambda_source_s3`: S3 bucket/key holding the zip. Used when the zip is
staged to S3 out-of-band (e.g. by CI).

Inline SHA256 verification is defense-in-depth against a corrupted
download. It is NOT a replacement for `gh attestation verify`, which is
the canonical supply-chain check. See `terraform/README.md` for guidance. | 
object({
    release_version = optional(string)
    lambda_zip_path = optional(string)
    lambda_source_s3 = optional(object({
      bucket = string
      key    = string
    }))
  })
| n/a | yes | | [log\_level](#input\_log\_level) | Slog level. One of debug, info, warn, error. | `string` | `"info"` | no | | [log\_retention\_days](#input\_log\_retention\_days) | CloudWatch Logs retention in days. | `number` | `30` | no | | [memory\_size](#input\_memory\_size) | Lambda memory in MB. | `number` | `128` | no | | [permissions](#input\_permissions) | Repository permissions requested on each minted token. Serialized to GITHUB\_TOKEN\_BROKER\_PERMISSIONS as JSON. | `map(string)` | 
{
  "contents": "read"
}
| no | -| [release\_repository](#input\_release\_repository) | GitHub repository to pull the release asset from when lambda\_artifact.release\_version is set. Defaults to the upstream repo. | `string` | `"meigma/github-token-broker"` | no | +| [release\_repository](#input\_release\_repository) | Literal OWNER/REPO GitHub repository to pull the release asset from when lambda\_artifact.release\_version is set. Defaults to the upstream repo. | `string` | `"meigma/github-token-broker"` | no | | [repository\_name](#input\_repository\_name) | GitHub repository the broker issues tokens for. | `string` | n/a | yes | | [repository\_owner](#input\_repository\_owner) | GitHub owner of the repository the broker issues tokens for. | `string` | n/a | yes | -| [ssm\_parameter\_paths](#input\_ssm\_parameter\_paths) | SSM parameter paths holding the GitHub App credentials. All paths must be absolute. | 
object({
    client_id       = string
    installation_id = string
    private_key     = string
  })
| 
{
  "client_id": "/github-token-broker/app/client-id",
  "installation_id": "/github-token-broker/app/installation-id",
  "private_key": "/github-token-broker/app/private-key-pem"
}
| no | +| [ssm\_parameter\_paths](#input\_ssm\_parameter\_paths) | SSM parameter paths holding the GitHub App credentials. All paths must be absolute literal SSM paths. | 
object({
    client_id       = string
    installation_id = string
    private_key     = string
  })
| 
{
  "client_id": "/github-token-broker/app/client-id",
  "installation_id": "/github-token-broker/app/installation-id",
  "private_key": "/github-token-broker/app/private-key-pem"
}
| no | | [tags](#input\_tags) | Tags applied to all resources created by this module. | `map(string)` | `{}` | no | | [timeout](#input\_timeout) | Lambda execution timeout in seconds. | `number` | `10` | no | @@ -160,7 +158,6 @@ No modules. | [function\_arn](#output\_function\_arn) | ARN of the Lambda function. | | [function\_invoke\_arn](#output\_function\_invoke\_arn) | Invoke ARN, suitable for API Gateway or EventBridge integrations. | | [function\_name](#output\_function\_name) | Name of the Lambda function. | -| [function\_url](#output\_function\_url) | Function URL when enable\_function\_url is true; null otherwise. | | [log\_group\_name](#output\_log\_group\_name) | Name of the CloudWatch Log Group backing Lambda logs. | | [role\_arn](#output\_role\_arn) | ARN of the Lambda execution role. | | [role\_name](#output\_role\_name) | Name of the Lambda execution role. | diff --git a/terraform/examples/function-url/README.md b/terraform/examples/function-url/README.md deleted file mode 100644 index c6ca784..0000000 --- a/terraform/examples/function-url/README.md +++ /dev/null @@ -1,20 +0,0 @@ -# Function URL example - -Provisions the broker with a Lambda Function URL protected by `AWS_IAM` authorization and an explicit `aws_lambda_permission` scoping which principal can call it. The module never creates `NONE`-auth URLs. - -## Usage - -```sh -cp terraform.tfvars.example terraform.tfvars -# edit terraform.tfvars — invoker_principal_arn must be the IAM role/user that will call the URL -tofu init -tofu apply -``` - -Callers must sign requests with SigV4; a typical caller uses the AWS SDK's Lambda URL signer or `awscurl`. - -```sh -awscurl --service lambda --region "$REGION" "$(tofu output -raw function_url)" -``` - -If you see `Forbidden`, confirm your caller identity matches `invoker_principal_arn` and that `aws_lambda_permission` propagated (a short eventual-consistency window is normal after first apply). diff --git a/terraform/examples/function-url/main.tf b/terraform/examples/function-url/main.tf deleted file mode 100644 index c8d8e33..0000000 --- a/terraform/examples/function-url/main.tf +++ /dev/null @@ -1,36 +0,0 @@ -terraform { - required_version = ">= 1.6" - - required_providers { - aws = { - source = "hashicorp/aws" - version = ">= 5.0, < 7.0" - } - } -} - -provider "aws" { - region = var.region -} - -module "broker" { - source = "../.." - - function_name = "github-token-broker" - repository_owner = var.repository_owner - repository_name = var.repository_name - - lambda_artifact = { - release_version = var.release_version - } - - enable_function_url = true -} - -resource "aws_lambda_permission" "invoke_url" { - statement_id = "AllowInvokeFromCallerPrincipal" - action = "lambda:InvokeFunctionUrl" - function_name = module.broker.function_name - principal = var.invoker_principal_arn - function_url_auth_type = "AWS_IAM" -} diff --git a/terraform/examples/function-url/outputs.tf b/terraform/examples/function-url/outputs.tf deleted file mode 100644 index 186a192..0000000 --- a/terraform/examples/function-url/outputs.tf +++ /dev/null @@ -1,9 +0,0 @@ -output "function_url" { - description = "HTTPS endpoint for the Lambda Function URL." - value = module.broker.function_url -} - -output "function_arn" { - description = "ARN of the Lambda function." - value = module.broker.function_arn -} diff --git a/terraform/examples/function-url/terraform.tfvars.example b/terraform/examples/function-url/terraform.tfvars.example deleted file mode 100644 index 01d5073..0000000 --- a/terraform/examples/function-url/terraform.tfvars.example +++ /dev/null @@ -1,5 +0,0 @@ -region = "us-east-1" -repository_owner = "example-org" -repository_name = "example-repo" -release_version = "v1.0.0" -invoker_principal_arn = "arn:aws:iam::123456789012:role/invoker" diff --git a/terraform/examples/function-url/variables.tf b/terraform/examples/function-url/variables.tf deleted file mode 100644 index 525eeab..0000000 --- a/terraform/examples/function-url/variables.tf +++ /dev/null @@ -1,24 +0,0 @@ -variable "region" { - description = "AWS region." - type = string -} - -variable "repository_owner" { - description = "GitHub owner the broker issues tokens for." - type = string -} - -variable "repository_name" { - description = "GitHub repository the broker issues tokens for." - type = string -} - -variable "release_version" { - description = "Upstream release tag to deploy (e.g. v1.0.0)." - type = string -} - -variable "invoker_principal_arn" { - description = "IAM principal ARN allowed to invoke the Function URL." - type = string -} diff --git a/terraform/main.tf b/terraform/main.tf index 471f359..df54191 100644 --- a/terraform/main.tf +++ b/terraform/main.tf @@ -9,14 +9,15 @@ resource "null_resource" "fetch_release" { provisioner "local-exec" { interpreter = ["/bin/bash", "-c"] - command = <<-EOT + environment = { + RELEASE_VERSION = coalesce(var.lambda_artifact.release_version, "") + RELEASE_REPOSITORY = var.release_repository + RELEASE_CACHE_DIR = coalesce(local.release_cache_dir, "") + } + command = <<-EOT set -euo pipefail - version='${var.lambda_artifact.release_version}' - repo='${var.release_repository}' - dir='${local.release_cache_dir}' - - mkdir -p "$dir" + mkdir -p "$RELEASE_CACHE_DIR" if ! command -v gh >/dev/null 2>&1; then echo "error: gh CLI is required to fetch the release asset. Install it or pre-download and pass lambda_zip_path." >&2 @@ -28,14 +29,14 @@ resource "null_resource" "fetch_release" { exit 1 fi - gh release download "$version" \ - --repo "$repo" \ + gh release download "$RELEASE_VERSION" \ + --repo "$RELEASE_REPOSITORY" \ --pattern github-token-broker.zip \ --pattern checksums.txt \ - --dir "$dir" \ + --dir "$RELEASE_CACHE_DIR" \ --clobber - (cd "$dir" && sha256sum --check --status checksums.txt) + (cd "$RELEASE_CACHE_DIR" && sha256sum --check --status checksums.txt) EOT } } @@ -73,10 +74,3 @@ resource "aws_lambda_function" "broker" { aws_iam_role_policy.lambda, ] } - -resource "aws_lambda_function_url" "broker" { - count = var.enable_function_url ? 1 : 0 - - function_name = aws_lambda_function.broker.function_name - authorization_type = "AWS_IAM" -} diff --git a/terraform/outputs.tf b/terraform/outputs.tf index 536ae8a..d9ef5b6 100644 --- a/terraform/outputs.tf +++ b/terraform/outputs.tf @@ -13,11 +13,6 @@ output "function_invoke_arn" { value = aws_lambda_function.broker.invoke_arn } -output "function_url" { - description = "Function URL when enable_function_url is true; null otherwise." - value = try(aws_lambda_function_url.broker[0].function_url, null) -} - output "role_arn" { description = "ARN of the Lambda execution role." value = aws_iam_role.lambda.arn diff --git a/terraform/security_validations.tftest.hcl b/terraform/security_validations.tftest.hcl new file mode 100644 index 0000000..0a14a3c --- /dev/null +++ b/terraform/security_validations.tftest.hcl @@ -0,0 +1,113 @@ +mock_provider "aws" { + mock_data "aws_caller_identity" { + defaults = { + account_id = "123456789012" + } + } + + mock_data "aws_partition" { + defaults = { + partition = "aws" + } + } + + mock_data "aws_region" { + defaults = { + region = "us-east-1" + } + } + + mock_data "aws_iam_policy_document" { + defaults = { + json = "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":\"sts:AssumeRole\",\"Principal\":{\"Service\":\"lambda.amazonaws.com\"}}]}" + } + } +} + +mock_provider "null" {} + +variables { + function_name = "github-token-broker" + repository_owner = "acme" + repository_name = "widgets" + + lambda_artifact = { + release_version = "v1.1.0" + } +} + +run "reject_function_name_shell_metacharacters" { + command = plan + + variables { + function_name = "broker';touch/tmp/pwn" + } + + expect_failures = [ + var.function_name, + ] +} + +run "reject_release_repository_shell_metacharacters" { + command = plan + + variables { + release_repository = "meigma/github-token-broker';touch/tmp/pwn" + } + + expect_failures = [ + var.release_repository, + ] +} + +run "reject_repository_owner_path_escape" { + command = plan + + variables { + repository_owner = "acme/widgets" + } + + expect_failures = [ + var.repository_owner, + ] +} + +run "reject_repository_name_percent_escape" { + command = plan + + variables { + repository_name = "widgets%2fadmin" + } + + expect_failures = [ + var.repository_name, + ] +} + +run "reject_wildcard_ssm_paths" { + command = plan + + variables { + ssm_parameter_paths = { + client_id = "/github-token-broker/app/client-id" + installation_id = "/github-token-broker/app/*" + private_key = "/github-token-broker/app/private-key-pem" + } + } + + expect_failures = [ + var.ssm_parameter_paths, + ] +} + +run "reject_wildcard_kms_key_arn" { + command = plan + + variables { + kms_key_arn = "arn:aws:kms:us-east-1:123456789012:key/*" + } + + expect_failures = [ + var.kms_key_arn, + ] +} diff --git a/terraform/variables.tf b/terraform/variables.tf index b4b35f4..5e95e7b 100644 --- a/terraform/variables.tf +++ b/terraform/variables.tf @@ -3,8 +3,8 @@ variable "function_name" { type = string validation { - condition = length(var.function_name) > 0 && length(var.function_name) <= 64 - error_message = "function_name must be between 1 and 64 characters." + condition = can(regex("^[A-Za-z0-9_-]{1,64}$", var.function_name)) + error_message = "function_name must be 1-64 characters and contain only letters, numbers, hyphens, and underscores." } } @@ -13,8 +13,8 @@ variable "repository_owner" { type = string validation { - condition = length(trimspace(var.repository_owner)) > 0 - error_message = "repository_owner must be non-empty." + condition = can(regex("^[A-Za-z0-9_.-]+$", var.repository_owner)) + error_message = "repository_owner must contain only letters, numbers, periods, underscores, and hyphens." } } @@ -23,8 +23,8 @@ variable "repository_name" { type = string validation { - condition = length(trimspace(var.repository_name)) > 0 - error_message = "repository_name must be non-empty." + condition = can(regex("^[A-Za-z0-9_.-]+$", var.repository_name)) + error_message = "repository_name must contain only letters, numbers, periods, underscores, and hyphens." } } @@ -82,9 +82,14 @@ variable "lambda_artifact" { } variable "release_repository" { - description = "GitHub repository to pull the release asset from when lambda_artifact.release_version is set. Defaults to the upstream repo." + description = "Literal OWNER/REPO GitHub repository to pull the release asset from when lambda_artifact.release_version is set. Defaults to the upstream repo." type = string default = "meigma/github-token-broker" + + validation { + condition = can(regex("^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$", var.release_repository)) + error_message = "release_repository must be a literal OWNER/REPO value containing only letters, numbers, periods, underscores, and hyphens." + } } variable "permissions" { @@ -106,7 +111,7 @@ variable "permissions" { } variable "ssm_parameter_paths" { - description = "SSM parameter paths holding the GitHub App credentials. All paths must be absolute." + description = "SSM parameter paths holding the GitHub App credentials. All paths must be absolute literal SSM paths." type = object({ client_id = string installation_id = string @@ -120,16 +125,16 @@ variable "ssm_parameter_paths" { validation { condition = alltrue([ - startswith(var.ssm_parameter_paths.client_id, "/"), - startswith(var.ssm_parameter_paths.installation_id, "/"), - startswith(var.ssm_parameter_paths.private_key, "/"), + can(regex("^/[A-Za-z0-9_.\\-/]+$", var.ssm_parameter_paths.client_id)), + can(regex("^/[A-Za-z0-9_.\\-/]+$", var.ssm_parameter_paths.installation_id)), + can(regex("^/[A-Za-z0-9_.\\-/]+$", var.ssm_parameter_paths.private_key)), ]) - error_message = "ssm_parameter_paths entries must be absolute (start with /)." + error_message = "ssm_parameter_paths entries must be absolute literal SSM paths containing only letters, numbers, periods, underscores, hyphens, and slashes." } } variable "github_api_base_url" { - description = "GitHub API base URL. Override for GitHub Enterprise Server." + description = "GitHub API base URL. Override for GitHub Enterprise Server. The Lambda requires https except for loopback http URLs used in local tests." type = string default = "https://api.github.com" } @@ -198,14 +203,16 @@ variable "tags" { default = {} } -variable "enable_function_url" { - description = "Create a Lambda Function URL with AWS_IAM auth. Never creates a NONE-auth URL." - type = bool - default = false -} - variable "kms_key_arn" { - description = "KMS key ARN used by SSM to encrypt the private key parameter. Set only when the customer uses a CMK instead of the AWS-managed key. Null disables kms:Decrypt in the role policy." + description = "Literal KMS key or alias ARN used by SSM to encrypt the private key parameter. Set only when the customer uses a CMK instead of the AWS-managed key. Null disables kms:Decrypt in the role policy." type = string default = null + + validation { + condition = ( + var.kms_key_arn == null || + can(regex("^arn:aws[a-zA-Z-]*:kms:[a-z0-9-]+:[0-9]{12}:(key/[A-Za-z0-9-]+|alias/[A-Za-z0-9/_-]+)$", var.kms_key_arn)) + ) + error_message = "kms_key_arn must be a literal KMS key or alias ARN without wildcard characters." + } }