Empty string clears a required WireGuard private key #137

New issue

Closed

opened 2026-11-12 08:24:27 +01:00 by Alex · 0 comments

Alex commented

2026-11-12 08:24:27 +01:00

Owner

Severity: Medium to High. Found in the code review of the commits from 2026-11-04 to 2026-11-11 (main at 6076be32). Still present on main at adbeb2f4. Checked in the code; the review agent confirmed it with a test on a copy.

Context

Secrets are write-only in the API (ADR 0022). GET never returns them.
To change a secret, a client sends either the keep marker (SecretKeep) or a new value.
Fields can be tagged required (it must stay set) and nonempty. Today only the BGP password uses nonempty.

Problem

pkg/models/secret_input.go:125–141 correctly rejects null for a required field.
A present empty string "", however, reaches checkSecretInput (:118), which only rejects SecretKeep.
internal/service/wireguard.go:452 skips the key check when the key is empty.

Impact

PUT /vpn/wireguard with "private_key":"" on an existing tunnel:
- The API answers 200, and the draft commits an empty key.
- The kernel keeps the old key for now, so nothing is noticed at first.
- On the next reboot or tunnel re-creation, creation fails with private_key is required. The VPN stays down and the key is lost for good.
Any API client that sends empty form fields is affected. The dashboard is not, because it leaves out empty fields.
This is an incomplete case analysis: null was covered, "" was forgotten.

Proposed change

For required secret fields, reject a present "" like null (400). In effect, required implies nonempty.
WireGuard validation rejects an empty private key instead of skipping the check.
Check every required secret field first, so that none legitimately accepts "".
Check that the first-boot TOML import also rejects an empty required secret.
Document the 400 case in openapi.yaml.

Validation

A contract test: PUT /vpn/wireguard with "private_key":"" returns 400, and the stored key is unchanged.
A TOML import case with an empty WireGuard private key.
go vet on the changed packages.

References

Commit 849af24c, ADR 0021 and 0022.

Severity: **Medium to High**. Found in the code review of the commits from 2026-11-04 to 2026-11-11 (`main` at `6076be32`). Still present on `main` at `adbeb2f4`. Checked in the code; the review agent confirmed it with a test on a copy. ## Context - Secrets are write-only in the API (ADR 0022). GET never returns them. - To change a secret, a client sends either the keep marker (`SecretKeep`) or a new value. - Fields can be tagged `required` (it must stay set) and `nonempty`. Today only the BGP password uses `nonempty`. ## Problem - `pkg/models/secret_input.go:125–141` correctly rejects `null` for a `required` field. - A present empty string `""`, however, reaches `checkSecretInput` (`:118`), which only rejects `SecretKeep`. - `internal/service/wireguard.go:452` skips the key check when the key is empty. ## Impact - `PUT /vpn/wireguard` with `"private_key":""` on an existing tunnel: - The API answers 200, and the draft commits an empty key. - The kernel keeps the old key for now, so nothing is noticed at first. - On the next reboot or tunnel re-creation, creation fails with `private_key is required`. The VPN stays down and **the key is lost for good**. - Any API client that sends empty form fields is affected. The dashboard is not, because it leaves out empty fields. - This is an incomplete case analysis: `null` was covered, `""` was forgotten. ## Proposed change - For `required` secret fields, reject a present `""` like `null` (400). In effect, `required` implies `nonempty`. - WireGuard validation rejects an empty private key instead of skipping the check. - Check every `required` secret field first, so that none legitimately accepts `""`. - Check that the first-boot TOML import also rejects an empty required secret. - Document the 400 case in `openapi.yaml`. ## Validation - A contract test: `PUT /vpn/wireguard` with `"private_key":""` returns 400, and the stored key is unchanged. - A TOML import case with an empty WireGuard private key. - `go vet` on the changed packages. ## References - Commit `849af24c`, ADR 0021 and 0022.

Alex referenced this issue from a pull request that will close it,

2026-11-12 11:15:41 +01:00

Reject empty strings for required secrets #160

Alex referenced this issue from a commit

2026-11-12 11:15:41 +01:00

Reject empty strings for required secrets

Alex referenced this issue from a commit

2026-11-12 11:15:41 +01:00

Document that required secrets refuse empty strings

Alex closed this issue

2026-11-12 11:15:46 +01:00