Summary
Follow-up to #104 (and PR #120), which stopped PIMONITOR_API_KEY from being copied into the environment of child processes (apt, vcgencmd) by building an explicit, minimal cmd.Env instead of inheriting the full parent environment.
A stronger, but more invasive, follow-up was called out as explicitly out of scope there: config.Load could call os.Unsetenv("PIMONITOR_API_KEY") after reading the value, so the key disappears from the main pimonitor process's own environment (/proc/self/environ) entirely, not just from subprocess environments.
Why this is separate from #104
This is a bigger behavioural change than the subprocess fix:
- It affects anything else in the process that might read
PIMONITOR_API_KEY later (unlikely today, but a behavioural change nonetheless).
os.Unsetenv does not necessarily shrink or scrub the original environ block on all platforms/libc implementations — on Linux/glibc it typically does rewrite the block in place, but this should be verified rather than assumed, and the exact guarantee should be documented rather than implied.- It's a design decision worth reviewing on its own rather than bundling into a subprocess-environment hardening PR.
Suggested approach
- In
internal/config (wherever PIMONITOR_API_KEY is currently read via os.Getenv/os.LookupEnv), call os.Unsetenv("PIMONITOR_API_KEY") immediately after reading it into config, once it's no longer needed from the environment.
- Confirm (and note in a comment /
SECURITY.md) what guarantee this actually provides on Linux — i.e. whether /proc/self/environ is actually scrubbed afterward — rather than assuming it based on os.Unsetenv's general contract.
- Add a test verifying that after
config.Load runs (with PIMONITOR_API_KEY set via t.Setenv), os.LookupEnv("PIMONITOR_API_KEY") no longer finds it — being mindful that t.Setenv and manual unsetting inside the same test need to interact correctly (parallel tests and t.Setenv don't mix; note this in the test).
- Update
SECURITY.md's "Threat Model / Design Notes" section to mention this additional hardening step once implemented.
References
Summary
Follow-up to #104 (and PR #120), which stopped
PIMONITOR_API_KEYfrom being copied into the environment of child processes (apt,vcgencmd) by building an explicit, minimalcmd.Envinstead of inheriting the full parent environment.A stronger, but more invasive, follow-up was called out as explicitly out of scope there:
config.Loadcould callos.Unsetenv("PIMONITOR_API_KEY")after reading the value, so the key disappears from the mainpimonitorprocess's own environment (/proc/self/environ) entirely, not just from subprocess environments.Why this is separate from #104
This is a bigger behavioural change than the subprocess fix:
PIMONITOR_API_KEYlater (unlikely today, but a behavioural change nonetheless).os.Unsetenvdoes not necessarily shrink or scrub the originalenvironblock on all platforms/libc implementations — on Linux/glibc it typically does rewrite the block in place, but this should be verified rather than assumed, and the exact guarantee should be documented rather than implied.Suggested approach
internal/config(whereverPIMONITOR_API_KEYis currently read viaos.Getenv/os.LookupEnv), callos.Unsetenv("PIMONITOR_API_KEY")immediately after reading it into config, once it's no longer needed from the environment.SECURITY.md) what guarantee this actually provides on Linux — i.e. whether/proc/self/environis actually scrubbed afterward — rather than assuming it based onos.Unsetenv's general contract.config.Loadruns (withPIMONITOR_API_KEYset viat.Setenv),os.LookupEnv("PIMONITOR_API_KEY")no longer finds it — being mindful thatt.Setenvand manual unsetting inside the same test need to interact correctly (parallel tests andt.Setenvdon't mix; note this in the test).SECURITY.md's "Threat Model / Design Notes" section to mention this additional hardening step once implemented.References