diff --git a/cmd/msa-agent/main.go b/cmd/msa-agent/main.go index 3b00563..1383502 100644 --- a/cmd/msa-agent/main.go +++ b/cmd/msa-agent/main.go @@ -485,10 +485,14 @@ func runInstallCommand(args []string) int { if err := flags.Parse(args); err != nil { return 2 } + paths := install.DefaultPaths() + paths.ConfigPath = *configPath + opts.Paths = paths if *printUnit { - // The unit is embedded, so this is how an operator reads it before it is installed — which is the - // point of embedding rather than fetching: the file that will be installed is readable first. - if err := install.PrintUnit(os.Stdout); err != nil { + // The unit is embedded and rendered for this layout, so this is how an operator reads what will be + // installed — which is the point of embedding rather than fetching: the file that will be installed + // is readable first, including the parts that follow --config. + if err := install.PrintUnit(os.Stdout, paths); err != nil { fmt.Fprintln(os.Stderr, "msa-agent install:", err) return 1 } @@ -500,9 +504,6 @@ func runInstallCommand(args []string) int { return 1 } } - paths := install.DefaultPaths() - paths.ConfigPath = *configPath - opts.Paths = paths opts.Log = func(format string, args ...any) { fmt.Printf(format+"\n", args...) } if err := install.Install(opts); err != nil { fmt.Fprintln(os.Stderr, "msa-agent install:", err) diff --git a/internal/config/config.go b/internal/config/config.go index 2798d0a..fe4eee9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,8 @@ import ( "os" "regexp" "syscall" + + "fun.chick/msa-agent/internal/relayurl" ) var nodePattern = regexp.MustCompile(`^nd_[A-Za-z0-9_-]{8,64}$`) @@ -84,9 +86,16 @@ func (c File) Validate(pairMode bool) error { if !nodePattern.MatchString(c.NodeID) { return errors.New("nodeId must look like nd_xxxxxxxx") } + // The URL is checked for shape here, not merely for being present. It used to be checked for + // presence alone, and the transport refused the real rule at dial time — so `enroll` would write a + // config, the daemon would refuse it at the next startup, and the operator would find out after + // the install had already reported success. if c.RelayURL == "" { return errors.New("relayUrl is required") } + if err := relayurl.Validate(c.RelayURL); err != nil { + return err + } if c.AgentToken == "" { return errors.New("agentToken is required") } @@ -159,6 +168,13 @@ func loadFile(path string) (File, error) { return cfg, nil } +// Marshal renders a File the way Save writes it, so a caller that needs the bytes without writing them +// — `enroll`, which validates before it writes — gets the same encoding rather than its own +// json.MarshalIndent that could drift from this one. +func Marshal(cfg File) ([]byte, error) { + return json.MarshalIndent(cfg, "", " ") +} + // Save writes the config back with 0600, atomically, preserving the file's owner. // // Atomic (temp file + rename) because the pairing flow writes the device list at moments when the @@ -176,7 +192,7 @@ func Save(path string, cfg File) error { if err := cfg.Validate(true); err != nil { return fmt.Errorf("config: refusing to save an invalid file: %w", err) } - encoded, err := json.MarshalIndent(cfg, "", " ") + encoded, err := Marshal(cfg) if err != nil { return err } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f2835b8..b217b2a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -123,6 +123,33 @@ func TestSaveRefusesAnInvalidFile(t *testing.T) { } } +// The relay URL used to be checked only for being non-empty, so a shape the transport refuses was +// accepted here and refused later — at dial time in the daemon, or at the next startup for a config +// `enroll` had already written and reported as success. These are the values that reached real nodes. +func TestValidateRefusesARelayURLTheDaemonCannotDial(t *testing.T) { + for _, raw := range []string{ + "relay.example", + "http://relay.example", + "https://relay.example/v1", + "https://relay.example?x=1", + "https://", + } { + cfg := valid() + cfg.RelayURL = raw + if err := cfg.Validate(false); err == nil { + t.Errorf("accepted relayUrl %q", raw) + } + if err := cfg.Validate(true); err == nil { + t.Errorf("pair mode accepted relayUrl %q", raw) + } + } + cfg := valid() + cfg.RelayURL = "https://relay.example/" + if err := cfg.Validate(false); err != nil { + t.Fatalf("refused a trailing slash: %v", err) + } +} + func TestValidateNamesEveryMissingField(t *testing.T) { cases := map[string]func(*File){ "nodeId": func(c *File) { c.NodeID = "" }, diff --git a/internal/enroll/enroll.go b/internal/enroll/enroll.go index 0c4329e..f6021cc 100644 --- a/internal/enroll/enroll.go +++ b/internal/enroll/enroll.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" + "fun.chick/msa-agent/internal/config" "fun.chick/msa-agent/internal/wire" ) @@ -69,17 +70,12 @@ type Registration struct { } // Generate mints the material and renders both artefacts. +// +// Every input rule is enforced by config.File.Validate below rather than restated here. The three +// hand-written checks that used to stand here — relay URL non-empty, port in range, audit path present +// — were a second, weaker copy of rules the daemon already had, and the relay URL one was the weakest +// of the three: it accepted anything non-empty, including values the transport refuses at dial time. func Generate(opts Options) (Result, error) { - if opts.RelayURL == "" { - return Result{}, fmt.Errorf("enroll: relay URL is required") - } - if opts.LocalPort <= 0 || opts.LocalPort > 65535 { - return Result{}, fmt.Errorf("enroll: local port %d is out of range", opts.LocalPort) - } - if opts.AuditPath == "" { - return Result{}, fmt.Errorf("enroll: audit path is required") - } - // The signing key is Ed25519: it answers the relay's handshake challenge and signs responses. signingPublic, signingKey, err := ed25519.GenerateKey(rand.Reader) if err != nil { @@ -109,34 +105,43 @@ func Generate(opts Options) (Result, error) { pairingSecret = randomHex(16) } - config := map[string]any{ - "nodeId": nodeID, - "relayUrl": opts.RelayURL, - "agentToken": token, - "localPort": opts.LocalPort, - "signKey": base64.StdEncoding.EncodeToString(signingKey), - "kxKey": base64.StdEncoding.EncodeToString(kxKey), - "auditPath": opts.AuditPath, - // Only used to authorise the pairing window on an unprotected node. Deleting it after pairing - // is fine; nothing else reads it. - "provisionSecret": pairingSecret, + // The config is built as the type the daemon loads, and validated by the daemon's own rules before it + // is written. + // + // This used to be a map[string]any with a handful of hand-written checks, and it produced configs the + // daemon then refused: `--relay relay.example` (no scheme), `--relay https://relay.example/v1` (a + // path), `--node node-1` (a malformed id) were all written happily, and the failure appeared later as + // "msa-agent exiting" on a node the operator had just installed. Two places that must agree about + // what a valid config is, with nothing checking the agreement — the same shape of defect as the + // install command that skipped the service user. + // + // Building the real type and calling the real Validate makes the agreement structural: there is one + // definition of a usable config and this cannot drift from it. + file := config.File{ + NodeID: nodeID, + RelayURL: opts.RelayURL, + AgentToken: token, + LocalPort: opts.LocalPort, + SignKey: base64.StdEncoding.EncodeToString(signingKey), + KxKey: base64.StdEncoding.EncodeToString(kxKey), + AuditPath: opts.AuditPath, + // Only used to authorise the pairing window on an unprotected node. Deleting it after pairing is + // fine; nothing else reads it. + ProvisionSecret: pairingSecret, + NodePassword: opts.NodePassword, + RelayCAFile: opts.RelayCAFile, + SelfEnrolled: opts.SelfEnrolled, // Empty on purpose: devices are added by pairing (`msa-agent --pair`), which is the only path // that should ever be able to introduce a trusted phone. - "devices": []any{}, + Devices: []config.Device{}, } - // Absent rather than empty when unset, so a config that has not been told about a password does not - // claim the node has none. - if opts.NodePassword != "" { - config["nodePassword"] = opts.NodePassword - } - if opts.RelayCAFile != "" { - config["relayCAFile"] = opts.RelayCAFile - } - if opts.SelfEnrolled { - config["selfEnrolled"] = true + // pairMode relaxes exactly the empty-device rule, which is the state enrollment produces: the node + // has keys and no phone yet. + if err := file.Validate(true); err != nil { + return Result{}, fmt.Errorf("enroll: refusing to write a config the agent would not load: %w", err) } - encoded, err := json.MarshalIndent(config, "", " ") + encoded, err := config.Marshal(file) if err != nil { return Result{}, fmt.Errorf("enroll: encoding the config: %w", err) } diff --git a/internal/enroll/enroll_test.go b/internal/enroll/enroll_test.go index ec16f21..3bcee0d 100644 --- a/internal/enroll/enroll_test.go +++ b/internal/enroll/enroll_test.go @@ -129,6 +129,25 @@ func TestTwoEnrollmentsDiffer(t *testing.T) { } } +// Enrollment is where a bad relay URL is cheapest to catch and where it used to be missed entirely. +// It built a map[string]any with a handful of hand-written checks, so `--relay relay.example` and +// `--relay https://relay.example/v1` both produced a config file, the install reported success, and +// the daemon refused the file at the next startup. The values below are the ones that were measured +// being written. +func TestARelayURLTheDaemonWouldRefuseIsNeverWritten(t *testing.T) { + for _, raw := range []string{ + "relay.example", + "http://relay.example", + "https://relay.example/v1", + "https://relay.example?x=1", + "https://", + } { + if _, err := Generate(Options{RelayURL: raw, LocalPort: 3000, AuditPath: "/a"}); err == nil { + t.Errorf("wrote a config for relay %q", raw) + } + } +} + func TestMissingInputIsRefused(t *testing.T) { cases := map[string]Options{ "no relay": {LocalPort: 3000, AuditPath: "/a"}, diff --git a/internal/enroll/register.go b/internal/enroll/register.go index fcf501d..0ba0cb2 100644 --- a/internal/enroll/register.go +++ b/internal/enroll/register.go @@ -9,10 +9,11 @@ import ( "fmt" "io" "net/http" - "net/url" "os" "strings" "time" + + "fun.chick/msa-agent/internal/relayurl" ) // Register announces this node to the relay, using a one-time code an operator generated in the app. @@ -43,14 +44,11 @@ type RegisterOptions struct { // The failure vocabulary is deliberately small and each case names the next action, because this runs // during an install and the operator is looking at one line of output. func Register(ctx context.Context, opts RegisterOptions) error { - parsed, err := url.Parse(opts.RelayURL) - if err != nil { - return fmt.Errorf("enroll: relay url: %w", err) - } - if parsed.Scheme != "https" { - // https is not negotiable here for the same reason the agent refuses it: a registration sent in - // the clear can be replayed by anyone who sees it, and the code is single-use. - return fmt.Errorf("enroll: relay url must be https, got %q", parsed.Scheme) + // The same rule the daemon applies, from the same package. This check is not redundant with the + // config validation in Generate: registration happens *before* the config is written, so a URL this + // rejected late would already have been sent to a host the operator did not mean to reach. + if err := relayurl.Validate(opts.RelayURL); err != nil { + return fmt.Errorf("enroll: %w", err) } if strings.TrimSpace(opts.Code) == "" { return fmt.Errorf("enroll: the enrollment code is empty") diff --git a/internal/install/service.go b/internal/install/service.go index 114c82c..8cd9c7f 100644 --- a/internal/install/service.go +++ b/internal/install/service.go @@ -14,10 +14,20 @@ package install import ( _ "embed" + "encoding/json" "fmt" "io" "os" "path/filepath" + "strings" +) + +// The two literals in the embedded unit that have to follow the install's own paths. They are named +// here rather than parsed out of the file so a unit edit that removes one is a visible failure in +// RenderedUnit's tests rather than a silent no-op substitution. +const ( + defaultConfigPath = "/etc/msa-agent/config.json" + defaultWritablePaths = "/var/log/msa-agent" ) // The vendored unit. It is copied from agent/systemd/ by the Makefile, and @@ -31,6 +41,72 @@ var embeddedUnit []byte // UnitFile returns the unit this binary installs, for tests and for `install --print-unit`. func UnitFile() []byte { return embeddedUnit } +// RenderedUnit is the unit as it will actually be written for a given layout. +// +// The embedded file is a template, and two of its literals have to follow the paths the install was +// given. Both were measured drifting: +// +// - `install --config /srv/msa/config.json` chowned that file, gave it to the service user, and then +// wrote a unit whose ExecStart named `/etc/msa-agent/config.json` — a path that on that node holds +// nothing, so the service fails with "no such file or directory" while the config it needs sits +// elsewhere, correctly owned and never read. +// - `enroll --audit /srv/audit/audit.log` puts that path in the config, and the unit's +// `ReadWritePaths=/var/log/msa-agent` does not cover it. Under `ProtectSystem=strict` the agent's +// `os.MkdirAll` for the parent directory fails with EROFS and the daemon exits at startup, with a +// permission error that names neither the flag nor the directive. +// +// The default layout renders byte-for-byte the embedded file, which is what keeps the vendored copy +// and the drift test meaningful: the template is still the thing that was reviewed. +func RenderedUnit(paths Paths, writableDirs []string) []byte { + unit := string(embeddedUnit) + // Only substituted when it differs, so the common install produces exactly the reviewed file and a + // diff of `install --print-unit` against the repository copy stays empty. + if paths.ConfigPath != defaultConfigPath { + unit = strings.Replace(unit, "--config "+defaultConfigPath, "--config "+paths.ConfigPath, 1) + } + dirs := uniqueDirs(append([]string{paths.LogDir}, writableDirs...)) + if len(dirs) > 0 && strings.Join(dirs, " ") != defaultWritablePaths { + unit = strings.Replace(unit, "ReadWritePaths="+defaultWritablePaths, "ReadWritePaths="+strings.Join(dirs, " "), 1) + } + return []byte(unit) +} + +// uniqueDirs keeps the first occurrence of each path, in order, so a log directory that is also an +// audit directory appears once and the rendering is stable. +func uniqueDirs(dirs []string) []string { + seen := map[string]bool{} + out := make([]string, 0, len(dirs)) + for _, dir := range dirs { + dir = filepath.Clean(dir) + if dir == "" || dir == "." || dir == "/" || seen[dir] { + continue + } + seen[dir] = true + out = append(out, dir) + } + return out +} + +// auditDirOf reports the directory the config's audit log needs to be writable, or "" when the config +// cannot be read. +// +// An unreadable config is not an error here: `install --dry-run` is run on nodes where `enroll` has not +// happened yet, and the real run refuses a missing config a few lines above. What it must not do is +// invent a path — a wrong ReadWritePaths entry is a sandbox hole, and a missing one is the EROFS above. +func auditDirOf(configPath string) string { + raw, err := os.ReadFile(configPath) + if err != nil { + return "" + } + var parsed struct { + AuditPath string `json:"auditPath"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return "" + } + return filepath.Dir(parsed.AuditPath) +} + // InstallOptions configures an install. type InstallOptions struct { Paths Paths @@ -114,12 +190,16 @@ func Install(opts InstallOptions) error { } } - // 4. The unit, from this binary. + // 4. The unit, from this binary. The audit log's directory comes from the config that `enroll` wrote, + // because the unit's ReadWritePaths is the only thing that makes it writable under + // ProtectSystem=strict — and a config pointing outside it is a daemon that exits at startup with + // EROFS, naming neither the flag nor the directive. if stepWith(opts.DryRun, opts.Log, "writing %s", paths.UnitPath) { if err := os.MkdirAll(filepath.Dir(paths.UnitPath), 0o755); err != nil { return fmt.Errorf("install: creating the unit directory: %w", err) } - if err := os.WriteFile(paths.UnitPath, embeddedUnit, 0o644); err != nil { + unit := RenderedUnit(paths, []string{auditDirOf(paths.ConfigPath)}) + if err := os.WriteFile(paths.UnitPath, unit, 0o644); err != nil { return fmt.Errorf("install: writing the unit: %w", err) } } @@ -171,8 +251,13 @@ func chownTo(path, user string, opts InstallOptions) error { return nil } -// PrintUnit writes the embedded unit to w, so an operator can read what will be installed before it is. -func PrintUnit(w io.Writer) error { - _, err := w.Write(embeddedUnit) +// PrintUnit writes the unit as it would be installed for a layout to w, so an operator can read what +// will be installed before it is. +// +// It renders rather than dumping the embedded bytes, because the two are not the same thing once the +// install has been given a non-default config path or an audit log outside the log directory — and the +// command's whole promise is that what it prints is what gets written. +func PrintUnit(w io.Writer, paths Paths) error { + _, err := w.Write(RenderedUnit(paths, []string{auditDirOf(paths.ConfigPath)})) return err } diff --git a/internal/install/unitrender_test.go b/internal/install/unitrender_test.go new file mode 100644 index 0000000..297cec5 --- /dev/null +++ b/internal/install/unitrender_test.go @@ -0,0 +1,158 @@ +package install + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "fun.chick/msa-agent/internal/config" +) + +// The embedded unit is a template, and the install's own paths have to reach it. +// +// Two literals were measured drifting, and both fail on the node rather than here: +// +// 1. `install --config /srv/msa/config.json` chowned that file and gave it to the service user, then +// wrote a unit whose ExecStart named the default path. The service starts, cannot find its config, +// and exits — with the file it needs sitting elsewhere, correctly owned and never read. +// 2. `enroll --audit /srv/audit/audit.log` writes that path into the config, and the unit's +// ReadWritePaths covered only /var/log/msa-agent. Under ProtectSystem=strict the agent's MkdirAll +// for the parent directory fails with EROFS and the daemon exits at startup, naming neither the +// flag nor the directive. +// +// The rendering is deliberately identity-preserving for the default layout: the file a reviewer read +// is still byte-for-byte what a default install writes, so the vendored-copy drift test keeps its +// meaning. +func TestRenderedUnitFollowsTheLayoutItWasGiven(t *testing.T) { + // (a) The default layout renders exactly the embedded file. + if got := string(RenderedUnit(DefaultPaths(), nil)); got != string(UnitFile()) { + t.Fatal("the default layout does not render the reviewed unit file") + } + + // (b) A config path elsewhere reaches ExecStart. + custom := DefaultPaths() + custom.ConfigPath = "/srv/msa/config.json" + unit := string(RenderedUnit(custom, nil)) + if !strings.Contains(unit, "ExecStart=/opt/msa-agent/msa-agent --config /srv/msa/config.json") { + t.Fatalf("ExecStart does not name the config the install was given:\n%s", execLines(unit)) + } + if strings.Contains(unit, defaultConfigPath) { + t.Fatalf("the default config path is still in the unit:\n%s", execLines(unit)) + } + + // (c) An audit log outside the log directory becomes writable, and the default entry stays. + unit = string(RenderedUnit(DefaultPaths(), []string{"/srv/audit"})) + if !strings.Contains(unit, "ReadWritePaths=/var/log/msa-agent /srv/audit") { + t.Fatalf("the audit directory is not writable:\n%s", readWriteLines(unit)) + } + // (d) A duplicate — an audit log that already lives in the log directory — is collapsed, so the + // rendered file does not accumulate the same path on every re-install. + unit = string(RenderedUnit(DefaultPaths(), []string{"/var/log/msa-agent", "/var/log/msa-agent/"})) + if !strings.Contains(unit, "ReadWritePaths=/var/log/msa-agent\n") { + t.Fatalf("a duplicate writable path was rendered:\n%s", readWriteLines(unit)) + } +} + +// The install reads the audit path out of the config `enroll` wrote, so the two agree without anyone +// passing the flag twice. An unreadable config yields nothing rather than a guess: a wrong +// ReadWritePaths entry is a sandbox hole, and a missing one is the EROFS above. +func TestTheAuditDirectoryComesFromTheConfig(t *testing.T) { + directory := t.TempDir() + path := filepath.Join(directory, "config.json") + cfg := config.File{ + NodeID: "nd_7f3a9c2e", RelayURL: "https://relay.example", AgentToken: "tok", + LocalPort: 3000, SignKey: "AAAA", KxKey: "AAAA", + AuditPath: "/srv/audit/audit.log", Devices: []config.Device{}, + } + if err := config.Save(path, cfg); err != nil { + t.Fatal(err) + } + if got := auditDirOf(path); got != "/srv/audit" { + t.Fatalf("auditDirOf = %q, want /srv/audit", got) + } + if got := auditDirOf(filepath.Join(directory, "absent.json")); got != "" { + t.Fatalf("auditDirOf invented %q for a config that is not there", got) + } + if err := os.WriteFile(path, []byte("not json"), 0o600); err != nil { + t.Fatal(err) + } + if got := auditDirOf(path); got != "" { + t.Fatalf("auditDirOf invented %q for an unparseable config", got) + } +} + +// And the whole install writes the rendered unit, not the embedded bytes. This is the assertion that +// would fail if the two ever came apart again. +func TestInstallWritesAUnitForItsOwnPaths(t *testing.T) { + root := t.TempDir() + paths := Paths{ + ServiceName: "msa-agent", + UnitPath: filepath.Join(root, "etc/systemd/system/msa-agent.service"), + ConfigPath: filepath.Join(root, "srv/msa/config.json"), + LogDir: filepath.Join(root, "var/log/msa-agent"), + BinaryPath: filepath.Join(root, "opt/msa-agent/msa-agent"), + User: "msa-agent", + } + for _, path := range []string{paths.BinaryPath, paths.ConfigPath} { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(paths.BinaryPath, []byte("binary"), 0o755); err != nil { + t.Fatal(err) + } + cfg := config.File{ + NodeID: "nd_7f3a9c2e", RelayURL: "https://relay.example", AgentToken: "tok", + LocalPort: 3000, SignKey: "AAAA", KxKey: "AAAA", + AuditPath: filepath.Join(root, "srv/audit/audit.log"), Devices: []config.Device{}, + } + if err := config.Save(paths.ConfigPath, cfg); err != nil { + t.Fatal(err) + } + + runner := &recorder{failOn: "getent"} + if err := Install(InstallOptions{Paths: paths, Runner: runner, Log: func(string, ...any) {}}); err != nil { + t.Fatalf("Install: %v", err) + } + written, err := os.ReadFile(paths.UnitPath) + if err != nil { + t.Fatal(err) + } + unit := string(written) + if !strings.Contains(unit, "--config "+paths.ConfigPath) { + t.Fatalf("the installed unit does not name the config the install was given:\n%s", execLines(unit)) + } + if !strings.Contains(unit, "ReadWritePaths="+paths.LogDir+" "+filepath.Join(root, "srv/audit")) { + t.Fatalf("the installed unit does not make the audit log writable:\n%s", readWriteLines(unit)) + } +} + +// PrintUnit is the operator's preview of the install, so it has to be the same rendering the install +// writes. Dumping the embedded bytes would show a default-layout unit to someone installing with +// --config, which is a preview of a different file. +func TestPrintUnitShowsWhatTheInstallWouldWrite(t *testing.T) { + paths := DefaultPaths() + paths.ConfigPath = "/srv/msa/config.json" + var printed strings.Builder + if err := PrintUnit(&printed, paths); err != nil { + t.Fatal(err) + } + if printed.String() != string(RenderedUnit(paths, []string{auditDirOf(paths.ConfigPath)})) { + t.Fatal("--print-unit and the install would write different files") + } +} + +func execLines(unit string) string { return linesStartingWith(unit, "ExecStart") } + +func readWriteLines(unit string) string { return linesStartingWith(unit, "ReadWritePaths") } + +func linesStartingWith(unit, prefix string) string { + var out []string + for _, line := range strings.Split(unit, "\n") { + if strings.HasPrefix(line, prefix) { + out = append(out, line) + } + } + return strings.Join(out, "\n") +} diff --git a/internal/relayurl/relayurl.go b/internal/relayurl/relayurl.go new file mode 100644 index 0000000..5ea6106 --- /dev/null +++ b/internal/relayurl/relayurl.go @@ -0,0 +1,56 @@ +// Package relayurl holds the one definition of an acceptable relay URL. +// +// Three components need to agree on it — the config validator that decides whether the daemon starts +// at all, the transport that dials the relay, and the enrollment client that registers a node before +// either of them runs — and they did not. `msa-agent enroll --relay relay.example` (no scheme) and +// `--relay https://relay.example/v1` (a path) both wrote a config happily; the daemon then refused it +// at startup, on a node the operator had just finished installing, minutes later and in a different +// process. The error arrived as "msa-agent exiting" in the journal. +// +// The rule is short enough to state once and nowhere else: https, a host, and nothing after it. +package relayurl + +import ( + "errors" + "fmt" + "net/url" +) + +// Validate reports whether raw can be used as a relay base URL. +// +// The path rule is not cosmetic. The transport builds every request URL by concatenation +// (baseURL + "/v1/hello"), so a base URL carrying a query would produce +// `https://relay.example?x=1/v1/hello` — an empty path and a query of "/v1/hello". A base URL with a +// path would silently address endpoints under it. Both fail somewhere far from the typo. +func Validate(raw string) error { + if raw == "" { + // Named separately because the empty string is what a flag that was never given a value looks + // like, and "must be https, got """ is a worse thing to read at 2am than "is required". + return errors.New("relay url is required") + } + parsed, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("relay url: %w", err) + } + if parsed.Scheme != "https" { + // Not negotiable. The relay is untrusted by design, so the channel's job is to stop a third + // party from substituting a different ciphertext; over an unauthenticated channel the + // end-to-end encryption above it is decorative. + return fmt.Errorf("relay url must be https, got %q", parsed.Scheme) + } + if parsed.Host == "" { + return errors.New("relay url has no host") + } + if parsed.Path != "" && parsed.Path != "/" { + return fmt.Errorf("relay url must not carry a path, got %q", parsed.Path) + } + if parsed.RawQuery != "" || parsed.Fragment != "" { + return errors.New("relay url must not carry a query or a fragment") + } + if parsed.User != nil { + // Credentials in the URL would be sent on every request and are not how this authenticates: + // the agent signs a challenge with its own key, and a bearer in the URL would end up in logs. + return errors.New("relay url must not carry credentials") + } + return nil +} diff --git a/internal/relayurl/relayurl_test.go b/internal/relayurl/relayurl_test.go new file mode 100644 index 0000000..777dad1 --- /dev/null +++ b/internal/relayurl/relayurl_test.go @@ -0,0 +1,37 @@ +package relayurl + +import "testing" + +// The list is the union of every malformed value that reached a real node: the two that were written +// into a config and refused only at the next startup, and the ones the transport already refused. +func TestValidateRefusesWhatTheDaemonCannotUse(t *testing.T) { + for _, raw := range []string{ + "", // the flag was never given a value + "relay.example", // no scheme: url.Parse reads this as a path + "http://relay.example", // plaintext, so the ciphertext above it is decorative + "ws://relay.example", // a scheme the transport cannot dial + "https://relay.example/v1", // a path the request builder would silently append to + "https://relay.example?x=1", + "https://relay.example#f", + "https://user:pw@relay.example", + "https://", // no host + "not a url", // a space is not a valid URL byte + } { + if err := Validate(raw); err == nil { + t.Fatalf("accepted %q", raw) + } + } +} + +func TestValidateAcceptsTheFormsAnOperatorActuallyTypes(t *testing.T) { + for _, raw := range []string{ + "https://relay.example", + "https://relay.example/", // a trailing slash is trimmed by the transport, not an error + "https://relay.example:8443", + "https://203.0.113.9", + } { + if err := Validate(raw); err != nil { + t.Fatalf("refused %q: %v", raw, err) + } + } +} diff --git a/internal/transport/transport.go b/internal/transport/transport.go index 4ba6a1f..8fe2998 100644 --- a/internal/transport/transport.go +++ b/internal/transport/transport.go @@ -21,10 +21,11 @@ import ( "fmt" "io" "net/http" - "net/url" "strconv" "strings" "time" + + "fun.chick/msa-agent/internal/relayurl" ) // Paths on the relay. Versioned, because an agent and a relay will not be upgraded on the same day. @@ -135,18 +136,11 @@ type Config struct { // stop a third party from substituting a different ciphertext; an unauthenticated channel would make // the endpoint-to-endpoint encryption above it decorative. func New(cfg Config) (*Client, error) { - parsed, err := url.Parse(cfg.BaseURL) - if err != nil { - return nil, fmt.Errorf("transport: relay url: %w", err) - } - if parsed.Scheme != "https" { - return nil, fmt.Errorf("transport: relay url must be https, got %q", parsed.Scheme) - } - if parsed.Host == "" { - return nil, errors.New("transport: relay url has no host") - } - if parsed.Path != "" && parsed.Path != "/" { - return nil, fmt.Errorf("transport: relay url must not carry a path, got %q", parsed.Path) + // The rule lives in internal/relayurl and is applied here, in config.File.Validate and in the + // enrollment client. Three copies of it is how `enroll` came to write configs this constructor + // refused. + if err := relayurl.Validate(cfg.BaseURL); err != nil { + return nil, fmt.Errorf("transport: %w", err) } if len(cfg.Signer) != ed25519.PrivateKeySize { return nil, errors.New("transport: agent signing key is the wrong size")