Skip to content

Config writes: OnPin bakes CLI flags into config.json, and save is not atomic #46

Description

@viminizer

Two ways the config file can end up wrong, both in the write path rather than the read path.

1. Pinning the tailnet identity writes the whole in-memory config

main.go:241:

OnPin: func(login string) {
    cfg.AllowLogin = login
    if err := config.Save(cfg); err != nil { ... }

config.Update exists precisely to prevent this, and its doc comment says so:

command line flags overwrite the in-memory Config: --port, --lines, --poll and --hostname all do. Saving the in-memory copy would bake whichever flags this process happened to start with into the file, so a development server run once on another port would move the installed service to it.

OnPin is the one remaining caller that bypasses it. Start the binary once with --port 9999 --lines 1000, and the first tailnet connection to an unpinned node writes both into config.json permanently. config.Save is documented as being for the install path, which owns the whole file - this is not that.

Fix: config.Update(func(c *config.Config) { c.AllowLogin = login }).

2. config.save is not atomic

internal/config/config.go:192
return os.WriteFile(p, append(b, '\n'), 0o600)

os.WriteFile truncates then writes. A crash, a full disk, or a kill between the two leaves a truncated or empty config.json - which holds allowLogin and the whole watchlist. Load then falls back to Default(), so the node re-opens the identity claim to whoever connects first and the watchlist is gone.

The file is written on every settings change, every watch/unwatch, and every mute, so the window is hit reasonably often for a service that runs for weeks.

Fix: write to config.json.tmp in the same directory, then os.Rename over the target. Rename within a directory is atomic on APFS.

3. Minor: Auth.Check holds its mutex across a disk write

internal/api/auth.go calls a.OnPin(login) while holding a.mu, and OnPin writes the config file. Every concurrent request blocks on the auth mutex for the duration. It happens once in the life of an install, so this is cosmetic - but the fix is to capture the login under the lock and call OnPin after releasing it.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions