fix: enroll 写出守护进程会拒绝的配置,install 写出与自身路径不符的单元
四处"校验在错误的层"的缺陷,都属于同一类:每个组件单独看都对,接缝错了。 1. `msa-agent enroll --relay http://relay.example` / `relay.example` / `https://relay.example/v1` 都会写出配置文件,install 报成功,守护进程在**下一次启动**才 拒绝,日志里只有一行 "msa-agent exiting"。规则原本散在三处(config 只查非空、transport 查真实规则、enroll 的注册客户端另有一份),现在收进 `internal/relayurl` 一份,三处都调它。 `config.File.Validate` 因此也补上了 https/host/无路径/无 query 的检查。 2. `enroll` 不再自己手写字段检查,改为构造真正的 `config.File` 并调用真正的 `Validate`—— 一份"什么是可用配置"的定义,结构上无法再漂移。`config.Marshal` 让 enroll 与 Save 用同一个 编码器。 3. `install --config /srv/msa/config.json` 会给该文件改属主、交给服务用户,然后写一个 ExecStart 指向 `/etc/msa-agent/config.json` 的单元——服务起来找不到配置就退出,而它需要的 文件就在别处、属主正确、从未被读。`enroll --audit /srv/audit/audit.log` 同理:单元的 `ReadWritePaths` 只覆盖 `/var/log/msa-agent`,在 `ProtectSystem=strict` 下 agent 的 `MkdirAll` 报 EROFS,进程启动即退出,报错既不提那个 flag 也不提那条指令。现在单元是模板, `RenderedUnit` 按实际布局渲染,`--print-unit` 打印的就是会被写入的那一份;默认布局渲染结果 与仓库里被 review 过的文件逐字节相同,所以 vendored 副本的漂移检查仍然有意义。 测试:新增 `internal/relayurl`,`config`/`enroll`/`install` 各补回归测试。每条都做了变异验证 (把修复回退,对应测试必须失败)。
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 = "" },
|
||||
|
||||
+38
-33
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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"},
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user