Skip to content

Add PIMONITOR_API_KEY env var and warn about -api-key exposure - #90

Merged
LarsLaskowski merged 1 commit into
mainfrom
claude/issue-68-xx72ue
Aug 17, 2026
Merged

Add PIMONITOR_API_KEY env var and warn about -api-key exposure#90
LarsLaskowski merged 1 commit into
mainfrom
claude/issue-68-xx72ue

Conversation

@LarsLaskowski

@LarsLaskowski LarsLaskowski commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Pull Request

📖 Description

A process's command line is world-readable through /proc/<pid>/cmdline, so passing the REST API key via the -api-key flag leaks the secret to every local user on the box — unlike the config file, which install.sh keeps at mode 640 root:pimonitor. README and docs/API.md previously mentioned the flag without any warning.

This is a security-hardening fix (no breaking change):

  • New PIMONITOR_API_KEY environment variable as the middle ground suggested in the issue: systemd can deliver it via EnvironmentFile= from a root-only file, so the secret never touches a command line. It resolves between the config file and the CLI flags in precedence (defaults → YAML → env → flags). An unset or empty value is ignored, mirroring how the empty flag default is treated, so exporting PIMONITOR_API_KEY="" cannot silently disable an api_key configured in the file.
  • -api-key documented as development-only in the flag's own usage text, README.md, SECURITY.md, docs/API.md, docs/ARCHITECTURE.md, and packaging/pimonitor.example.yaml.
  • packaging/pimonitor.service gains a commented-out EnvironmentFile=/etc/pimonitor/pimonitor.env example plus a note on the required permissions (systemd reads it as PID 1, before dropping to the pimonitor user).

Load now delegates to an unexported load(args, lookupEnv) so the precedence rules can be tested without mutating the process environment. The exported Load signature and behavior are unchanged for existing callers.

🎫 Issues

Closes #68

👩‍💻 Reviewer Notes

  • Main logic change is the ~10 lines in internal/config/config.go that apply the env var between the YAML merge and the flag overrides; everything else is documentation and packaging comments.
  • Worth a second opinion on the "empty env var is a no-op" choice: it is consistent with the existing flag handling and fails safe (keeps authentication on), at the cost of not offering an env-only way to unset a configured key.
  • No API surface change — /api/v1/... response shapes are untouched; docs/API.md only gains guidance on where to put the key.
  • Smoke test: bin/pimonitor -config packaging/pimonitor.example.yaml with PIMONITOR_API_KEY=secret exported, then confirm curl -s localhost:8080/api/v1/metrics returns 401 and curl -H 'X-Api-Key: secret' ... returns 200.

📑 Test Plan

New unit tests in internal/config/config_test.go, following docs/TESTS.md (Test<Subject>_<Scenario> naming, no real environment or /proc access — the environment is injected via a lookupEnv stub):

  • TestLoad_APIKeyFromEnv — the env var sets the key.
  • TestLoad_APIKeyEnvOverridesConfigFile — env beats api_key from the YAML file.
  • TestLoad_APIKeyFlagOverridesEnv-api-key still wins over the env.
  • TestLoad_APIKeyEmptyEnvKeepsConfigFileValue — an exported-but-empty variable does not disable configured authentication.
  • TestLoad_ReadsAPIKeyEnvVar — covers the exported Load's wiring to the real process environment via t.Setenv, which the injected-stub tests deliberately bypass.

go build ./..., go vet ./..., go test ./... -race -cover (config package at 95.7%), and golangci-lint run (0 issues) all pass locally. Nothing here depends on real Pi hardware, so this is fully verified.

✅ Checklist

General

  • I have added/updated tests for my changes (go test ./... -race -cover passes locally).
  • go vet ./... and golangci-lint run are clean.
  • I have tested my changes.
  • I have read the CONTRIBUTING documentation and followed the project's code style guidelines.
  • I have updated ARCHITECTURE.md if this changes a documented design decision.

REST API / configuration / packaging

  • I have updated docs/API.md to reflect a REST API change.
  • No breaking change to /api/v1/... response shapes, or a new API version (/api/v2/...) was introduced instead.
  • I have updated README.md / packaging/pimonitor.example.yaml to reflect a new or changed configuration option.
  • I have updated packaging/install.sh or the systemd units if this changes installation/packaging, and kept the unprivileged/privileged service split intact (see SECURITY.md).

⏭ Next Steps

The -api-key flag is kept for development convenience. If it should disappear entirely, deprecating it (a startup warning first, removal in a later release) would be a natural follow-up.

A process's command line is world-readable via /proc/<pid>/cmdline, so
passing the REST API key with -api-key leaks it to every local user —
unlike the config file, which install.sh restricts to 640 root:pimonitor.

Add PIMONITOR_API_KEY as a middle ground that systemd can deliver through
EnvironmentFile= from a root-only file. It resolves between the config
file and the flags, and an unset or empty value is ignored so exporting it
blank cannot silently disable an api_key set in the config file. Load now
delegates to an unexported load(args, lookupEnv) so the precedence rules
are testable without mutating the process environment.

Document the flag as development-only in the flag usage text, README,
SECURITY.md, docs/API.md, docs/ARCHITECTURE.md, the example config, and
the systemd unit (which gains a commented-out EnvironmentFile= example).
@sonarqubecloud

Copy link
Copy Markdown

@LarsLaskowski
LarsLaskowski merged commit ebe7b4b into main Aug 17, 2026
5 checks passed
@LarsLaskowski
LarsLaskowski deleted the claude/issue-68-xx72ue branch August 17, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: -api-key CLI flag exposes the secret in the process list

2 participants