fix(aws): forward whole signals.env to the collector across EC2 paths (#292) - #299
Merged
Conversation
…#292) The baked AMI signals.service unit forwarded only `-e SIGNALS_API_TOKEN` to the container, so any other SIGNALS_* var a buyer placed in /etc/signals/signals.env was silently dropped (found in the #235 AMI launch-test: the dev-only SIGNALS_ALLOW_INSECURE_PG_TLS never reached the collector, which failed closed and crash-looped). Decision (forward-all vs document-only): forward-all. The collector reads its full runtime config from SIGNALS_* env, so a buyer tuning any of those must be able to set it in signals.env without editing the baked unit. - Component + deploy/aws/terraform + deploy/aws/cloudformation now docker-run with `--env-file /etc/signals/signals.env`, forwarding the whole file. The component keeps EnvironmentFile= so systemd fails cleanly if absent. All three EC2 paths now forward env identically (INV-AMI-03 parity). - Security (R-AMI-06 / INV-AMI-04): the token is passed by reference only. The IaC paths previously put the token VALUE on the docker command line (`-e SIGNALS_API_TOKEN="$SIGNALS_API_TOKEN"`), visible in the journal / `ps`; they now write it to a root-only (0600) signals.env and pass it via `--env-file`, never on the command line and never echoed/cat'd to a log. - Records the decision + new rules (R-AMI-05/06), invariants (INV-AMI-04), and acceptance cases (TC-AMI-06/07) in the AMI spec so docs match behavior. - Adds tests/signals_ami_env_forwarding_test.go asserting the two ACs plus the secrets-never-logged invariant across all three EC2 paths; verified non-vacuous (fails on an injected `cat signals.env` leak). Closes #292
2 tasks
This was referenced Jul 22, 2026
Merged
fheikens
added a commit
that referenced
this pull request
Jul 22, 2026
…303) Bump the chart version, appVersion, and image tag from 1.0.3 to 1.1.0 in lockstep, and document the release contents in CHANGELOG.md. Since v1.0.3 main carries a HIGH grpc security fix (#301) plus two backward-compatible collector features (#295, #298), an aws env fix (#299), and a go-minor dep bump (#296), so this is a minor release. The GA tag v1.1.0 is cut by a maintainer after this merges; CI enforces that the chart version matches the tag. Refs #302
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The baked AMI
signals.serviceunit(
deploy/aws/imagebuilder/signals-collector-component.yaml) forwarded only-e SIGNALS_API_TOKENto the container. Any otherSIGNALS_*var a buyerplaced in
/etc/signals/signals.envwas silently dropped. Found in the #235AMI launch-test: the dev-only
SIGNALS_ALLOW_INSECURE_PG_TLSnever reached thecollector, which failed closed and crash-looped. Production RDS use over
verify-full(config insignals.yaml) was unaffected.Decision (recorded)
Forward-all (vs document-only), chosen by Frank. The collector reads its
full runtime config from
SIGNALS_*env, so a buyer tuning any of those must beable to set it in
signals.envwithout editing the baked unit. Recorded inspecifications/marketplace-ami-image-builder.mdas R-AMI-05 with a datedrationale, plus the security rule R-AMI-06, invariant INV-AMI-04, failure
conditions FC-AMI-04/05, and acceptance cases TC-AMI-06/07.
Change
(
deploy/aws/terraform,deploy/aws/cloudformation) now docker-run with--env-file /etc/signals/signals.env, forwarding the whole file. Thecomponent keeps
EnvironmentFile=so systemd fails cleanly if the file isabsent. All three EC2 paths now forward env identically (INV-AMI-03 parity).
CloudFormation was included because it is the third EC2 run path with the
identical defect; leaving it token-only would create doc-vs-reality drift.
only. The IaC paths previously put the token value on the docker command
line (
-e SIGNALS_API_TOKEN="$SIGNALS_API_TOKEN"), visible in the journal andps. They now write it to a root-only (0600)signals.envand pass--env-file; the value is never on the command line and never echoed/cat'd toa log.
/root/signals-api-tokenis still produced for the operator verifystep.
Proving secrets are never logged
tests/signals_ami_env_forwarding_test.goasserts, across all three EC2 paths,that no line
cat/echo/teessignals.envto a log stream, noSIGNALS_API_TOKEN=<value>appears on a docker command line, and noset -xisenabled. Verified non-vacuous: injecting a
cat /etc/signals/signals.envinto terraform and cloudformation each makes the test fail; reverting makes it
pass.
Tests / gates run locally
go test ./... -count=1-> all packages pass (tests pkg 16.8s).go test ./tests -run TestAMI_ -v-> 3/3 pass.bash scripts/preflight.sh gofmt|vet|imagebuilder-> clean.terraform fmt -check+terraform validate-> clean / valid.shellcheck(default severity) on the added shell + the extracted user-data:the only findings are pre-existing SC2154/SC2086 on the Terraform
${var.*}interpolation (not real shell); my added lines are clean.
gitleaks protect --staged(repo.gitleaks.toml) -> no leaks (the token isminted at runtime via
openssl, no literal committed).Note: this repo has no
yamllintgate; the component's guard isscripts/check-imagebuilder-component.sh, which still passes.Acceptance criteria
in parity (INV-AMI-03).
SIGNALS_*var insignals.envreaches the collector(
--env-fileforwards the whole file).Closes #292