diff --git a/README.md b/README.md index 653d4c4..d1780fb 100644 --- a/README.md +++ b/README.md @@ -396,6 +396,14 @@ or CLI flags (`-listen`, `-log-level`, `-api-key`, `-config`, `-version`). Flags take precedence over the config file, which takes precedence over built-in defaults. +The API key additionally reads the `PIMONITOR_API_KEY` environment variable, +which sits between the config file and the flags in precedence. Set the key +via `api_key` in the config file (kept at mode `640 root:pimonitor` by the +installer), or via `PIMONITOR_API_KEY` from a systemd `EnvironmentFile=` with +the same restricted permissions. **The `-api-key` flag is for local +development only** — command lines are world-readable through +`/proc//cmdline`, so any local user could read the key. + ## Development See [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md) for how the system is put together diff --git a/SECURITY.md b/SECURITY.md index 6d049a8..53c9441 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -54,6 +54,13 @@ reporters and reviewers have context: per browser and persists it in `localStorage` (an accepted trade-off — anyone with access to the browser profile can read it, and without TLS the key is visible on the wire either way). +- **Supply the API key through the config file or the environment, not the + command line.** `/etc/pimonitor/config.yaml` is kept at mode `640 + root:pimonitor` by `install.sh`; alternatively set `PIMONITOR_API_KEY`, + which systemd can load from an `EnvironmentFile=` with equally restricted + permissions. The `-api-key` flag is a development convenience only: a + process's command line is world-readable via `/proc//cmdline`, so a + key passed that way is exposed to every local user on the machine. - Shell-outs (`apt list --upgradable`, optional `vcgencmd measure_temp`) are invoked with fixed argument lists (no user input is interpolated into shell commands), to avoid command injection. diff --git a/docs/API.md b/docs/API.md index ffa345a..b67ff9a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -12,11 +12,21 @@ existing integrations against `/api/v1/...` keep working. By default, no authentication is required — PiMonitor is meant to run on a trusted local network. If you set `api_key` in `config.yaml` (or the -`-api-key` flag), every `/api/v1/...` request must include one of: +`PIMONITOR_API_KEY` environment variable), every `/api/v1/...` request must +include one of: - `Authorization: Bearer ` - `X-Api-Key: ` +The key can be supplied three ways, in increasing precedence: `api_key` in +the config file, the `PIMONITOR_API_KEY` environment variable, and the +`-api-key` flag. **Prefer the config file** — `install.sh` restricts it to +mode `640 root:pimonitor`. `PIMONITOR_API_KEY` is the deployment-friendly +alternative (systemd `EnvironmentFile=` pointing at a file with the same +restricted permissions). The `-api-key` flag is for local development only: +command lines are world-readable via `/proc//cmdline`, so every local +user on the machine can read a key passed that way. + Requests without a valid key receive `401 Unauthorized`. `GET /healthz` is never gated by the API key, so external health checks keep working regardless of authentication configuration. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2f84096..ee1a1f6 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -264,8 +264,16 @@ same warn/crit cutoffs the server-side alert engine evaluates against (`>=`), an ## Configuration (`internal/config`) `config.Load` resolves `Config` in strictly increasing precedence: **built-in defaults** -(`Default()`) → **optional YAML file** (`-config`) → **CLI flags** (`-listen`, -`-log-level`, `-api-key`). YAML decoding uses `KnownFields(true)`, so an unrecognized key +(`Default()`) → **optional YAML file** (`-config`) → **environment** +(`PIMONITOR_API_KEY`) → **CLI flags** (`-listen`, `-log-level`, `-api-key`). The +environment layer exists only for the API key: the `-api-key` flag leaks the secret into +the process list (`/proc//cmdline` is world-readable), whereas `PIMONITOR_API_KEY` +can be delivered by systemd's `EnvironmentFile=` from a root-only file, and the config +file itself is kept at mode `640 root:pimonitor` by `install.sh`. An unset *or empty* +`PIMONITOR_API_KEY` changes nothing, mirroring the empty-flag default, so exporting it +blank cannot accidentally turn off an `api_key` set in the file. `Load` delegates to an +unexported `load(args, lookupEnv)` so the precedence rules are testable without mutating +the process environment. YAML decoding uses `KnownFields(true)`, so an unrecognized key (e.g. a typo like `api_kay`) fails config loading outright at startup instead of silently falling back to a default that could be security-relevant (e.g. no authentication because the intended `api_key` was never actually applied). diff --git a/internal/config/config.go b/internal/config/config.go index 9ad5d45..e789aed 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -338,16 +338,30 @@ type Result struct { VersionRequested bool } +// APIKeyEnvVar is the environment variable that overrides the REST API key. +// It exists because the -api-key flag leaks the secret into the process +// list (any local user can read /proc//cmdline), while an environment +// variable can be supplied by systemd via EnvironmentFile= from a +// root-readable file. The config file remains the primary mechanism. +const APIKeyEnvVar = "PIMONITOR_API_KEY" + // Load resolves configuration from defaults, an optional YAML file -// (-config), and flag overrides, in that order of increasing precedence. +// (-config), the PIMONITOR_API_KEY environment variable, and flag +// overrides, in that order of increasing precedence. func Load(args []string) (Result, error) { + return load(args, os.LookupEnv) +} + +// load is Load with the environment injected, so tests can exercise the +// precedence rules without mutating the process environment. +func load(args []string, lookupEnv func(string) (string, bool)) (Result, error) { cfg := Default() fs := flag.NewFlagSet("pimonitor", flag.ContinueOnError) configPath := fs.String("config", "", "path to YAML config file") listenAddr := fs.String("listen", "", "override listen address, e.g. :8080") logLevel := fs.String("log-level", "", "override log level (debug, info, warn, error)") - apiKey := fs.String("api-key", "", "override REST API key") + apiKey := fs.String("api-key", "", "override REST API key (development only: the value is visible in the process list to every local user; use api_key in the config file or "+APIKeyEnvVar+" instead)") showVersion := fs.Bool("version", false, "print version and exit") if err := fs.Parse(args); err != nil { @@ -360,6 +374,16 @@ func Load(args []string) (Result, error) { } } + // The environment sits between the config file and the flags: it is the + // recommended override for deployments (systemd EnvironmentFile=), while + // an explicit -api-key on the command line still wins. An empty or unset + // variable changes nothing, mirroring how the empty flag default is + // treated, so exporting PIMONITOR_API_KEY="" cannot accidentally disable + // an api_key configured in the file. + if v, ok := lookupEnv(APIKeyEnvVar); ok && v != "" { + cfg.APIKey = v + } + if *listenAddr != "" { cfg.ListenAddr = *listenAddr } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 05e9fa2..f62ee98 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -207,6 +207,78 @@ func TestLoad_APIKeyOverride(t *testing.T) { } } +// envFunc builds a lookupEnv stub for load() from a fixed environment. +func envFunc(env map[string]string) func(string) (string, bool) { + return func(key string) (string, bool) { + v, ok := env[key] + return v, ok + } +} + +func TestLoad_APIKeyFromEnv(t *testing.T) { + result, err := load(nil, envFunc(map[string]string{APIKeyEnvVar: "env-secret"})) + if err != nil { + t.Fatalf("load: %v", err) + } + if result.Config.APIKey != "env-secret" { + t.Fatalf("APIKey = %q, want env-secret", result.Config.APIKey) + } +} + +func TestLoad_APIKeyEnvOverridesConfigFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + writeFile(t, path, "api_key: \"file-secret\"\n") + + result, err := load([]string{"-config", path}, envFunc(map[string]string{APIKeyEnvVar: "env-secret"})) + if err != nil { + t.Fatalf("load: %v", err) + } + if result.Config.APIKey != "env-secret" { + t.Fatalf("APIKey = %q, want env-secret (env must override the config file)", result.Config.APIKey) + } +} + +func TestLoad_APIKeyFlagOverridesEnv(t *testing.T) { + result, err := load([]string{"-api-key", "flag-secret"}, envFunc(map[string]string{APIKeyEnvVar: "env-secret"})) + if err != nil { + t.Fatalf("load: %v", err) + } + if result.Config.APIKey != "flag-secret" { + t.Fatalf("APIKey = %q, want flag-secret (flag must override the env)", result.Config.APIKey) + } +} + +func TestLoad_APIKeyEmptyEnvKeepsConfigFileValue(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + writeFile(t, path, "api_key: \"file-secret\"\n") + + // An exported-but-empty PIMONITOR_API_KEY must not silently disable the + // authentication configured in the file. + result, err := load([]string{"-config", path}, envFunc(map[string]string{APIKeyEnvVar: ""})) + if err != nil { + t.Fatalf("load: %v", err) + } + if result.Config.APIKey != "file-secret" { + t.Fatalf("APIKey = %q, want file-secret", result.Config.APIKey) + } +} + +// TestLoad_ReadsAPIKeyEnvVar covers the wiring of the exported Load to the +// real process environment, which the load() tests above deliberately bypass. +func TestLoad_ReadsAPIKeyEnvVar(t *testing.T) { + t.Setenv(APIKeyEnvVar, "env-secret") + + result, err := Load(nil) + if err != nil { + t.Fatalf("Load: %v", err) + } + if result.Config.APIKey != "env-secret" { + t.Fatalf("APIKey = %q, want env-secret", result.Config.APIKey) + } +} + func TestValidate_DefaultIsValid(t *testing.T) { if err := Default().Validate(); err != nil { t.Fatalf("Default() must pass Validate(): %v", err) diff --git a/packaging/pimonitor.example.yaml b/packaging/pimonitor.example.yaml index 8906b62..4769a47 100644 --- a/packaging/pimonitor.example.yaml +++ b/packaging/pimonitor.example.yaml @@ -57,6 +57,12 @@ pi_model_enabled: true # the config manually, apply the same permissions: # sudo chown root:pimonitor /etc/pimonitor/config.yaml # sudo chmod 640 /etc/pimonitor/config.yaml +# +# Instead of setting it here you can export PIMONITOR_API_KEY, which takes +# precedence over this value; with systemd, point EnvironmentFile= at a file +# with the same restricted permissions. Do NOT use the -api-key flag outside +# local development: a process's command line is world-readable through +# /proc//cmdline, so every local user could read the key. api_key: "" # Color-coding thresholds for the dashboard (and the load-average gauge diff --git a/packaging/pimonitor.service b/packaging/pimonitor.service index f76e0c4..0f98388 100644 --- a/packaging/pimonitor.service +++ b/packaging/pimonitor.service @@ -12,6 +12,14 @@ ExecStart=/usr/local/bin/pimonitor -config /etc/pimonitor/config.yaml Restart=on-failure RestartSec=5 +# The REST API key can be set as api_key in the config file above, or kept +# out of it entirely by uncommenting the line below and putting +# PIMONITOR_API_KEY= into a root-only file (chown root:root, chmod +# 600 - systemd reads it as PID 1, before dropping to the pimonitor user). +# Never pass the key via the -api-key flag here: the command line of a +# running process is world-readable through /proc//cmdline. +#EnvironmentFile=/etc/pimonitor/pimonitor.env + # Hardening. PiMonitor only ever reads world-readable files under /proc, # /sys/class/thermal, /etc/os-release, and the apt cache under # /var/lib/apt/lists/ (via `apt list --upgradable`), plus binds a TCP