From 1cfaa69e618201a310b24fdf0511ebcc1217d193 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Wed, 28 Oct 2026 12:59:39 -0400 Subject: [PATCH 1/5] Decline DNS management on macOS without root The dynamic store, /etc/resolver, and the lock directory all require root on macOS. A non-root Manager was still built around the live backend, so a startup Clear failed on the identity lock, retried with backoff for the Manager's lifetime, and Close warned that cleanup failed: several warnings per start on a host with nothing to do. New now gives a non-root darwin Manager the declined mode: the no-op backend and no locker, Set refused with ErrConfigurationDeclined and the reason, Clear converging without touching the host, and one Info line in place of the construction warning. Errors still name the darwin backend. --- backend_darwin.go | 16 ++++++ backend_other.go | 3 + backend_windows.go | 3 + declined_darwin_test.go | 73 ++++++++++++++++++++++++ doc.go | 8 ++- errors.go | 10 +++- internal/backend/darwin/conflict_test.go | 2 +- internal/backend/darwin/converge_test.go | 1 - internal/backend/darwin/darwin.go | 10 ---- internal/backend/darwin/darwin_test.go | 43 -------------- manager.go | 31 ++++++++-- searchlimit_darwin_test.go | 1 + 12 files changed, 134 insertions(+), 67 deletions(-) create mode 100644 declined_darwin_test.go diff --git a/backend_darwin.go b/backend_darwin.go index f197ec8..abfd248 100644 --- a/backend_darwin.go +++ b/backend_darwin.go @@ -2,6 +2,8 @@ package dnsconfig import ( "context" + "errors" + "os" "github.com/DefinedNet/dnsconfig/internal/backend" "github.com/DefinedNet/dnsconfig/internal/backend/darwin" @@ -20,3 +22,17 @@ func newNativeBackend(ctx context.Context, kind BackendKind, opts Options) (back } return b, nil } + +// isRoot is replaced by tests so the declined path does not depend on the +// test runner's user. +var isRoot = func() bool { return os.Geteuid() == 0 } + +// nativeDeclined reports why this process cannot manage kind's DNS, or nil. +// The dynamic store, /etc/resolver, and the lock directory all require root, +// so a non-root darwin Manager could only fail and retry every attempt. +func nativeDeclined(kind BackendKind) error { + if kind == BackendDarwin && !isRoot() { + return errors.New("darwin: requires root") + } + return nil +} diff --git a/backend_other.go b/backend_other.go index 0f1ff66..3e1555c 100644 --- a/backend_other.go +++ b/backend_other.go @@ -11,3 +11,6 @@ import ( func newNativeBackend(_ context.Context, kind BackendKind, _ Options) (backend.Backend, error) { return nil, errBackendUnsupported(kind) } + +// nativeDeclined reports why this process cannot manage kind's DNS, or nil. +func nativeDeclined(BackendKind) error { return nil } diff --git a/backend_windows.go b/backend_windows.go index f60579c..012381f 100644 --- a/backend_windows.go +++ b/backend_windows.go @@ -22,3 +22,6 @@ func newNativeBackend(ctx context.Context, kind BackendKind, opts Options) (back } return b, nil } + +// nativeDeclined reports why this process cannot manage kind's DNS, or nil. +func nativeDeclined(BackendKind) error { return nil } diff --git a/declined_darwin_test.go b/declined_darwin_test.go new file mode 100644 index 0000000..db2e169 --- /dev/null +++ b/declined_darwin_test.go @@ -0,0 +1,73 @@ +package dnsconfig + +import ( + "bytes" + "context" + "log/slog" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/DefinedNet/dnsconfig/internal/backend/noop" +) + +// withRoot answers the process root check for the duration of t. +func withRoot(t *testing.T, root bool) { + t.Helper() + prev := isRoot + isRoot = func() bool { return root } + t.Cleanup(func() { isRoot = prev }) +} + +// TestNew_DarwinWithoutRootDeclines checks that a non-root macOS process, +// which can write none of the store, /etc/resolver, or the lock directory, +// gets a declined Manager instead of one that retries failing attempts. +func TestNew_DarwinWithoutRootDeclines(t *testing.T) { + withRoot(t, false) + var logs bytes.Buffer + m, err := New(context.Background(), Options{ + Identity: "facade-declined", + ForceBackend: BackendDarwin, + Logger: slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelDebug})), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = m.Close() }) + + assert.Equal(t, BackendDarwin, m.kind) + assert.Nil(t, m.locker, "a declined Manager takes no host locks") + assert.IsType(t, &noop.Backend{}, m.backend, "the darwin backend is never built") + + err = m.Set(sampleConfig(t)) + require.ErrorIs(t, err, ErrConfigurationDeclined) + assert.ErrorContains(t, err, "requires root") + var e *Error + require.ErrorAs(t, err, &e) + assert.Equal(t, BackendDarwin, e.Backend) + s := m.Status() + assert.Equal(t, GoalNone, s.Goal) + assert.Same(t, err, s.LastError) + + require.NoError(t, m.Clear()) + s = waitStatus(t, m, func(s Status) bool { return s.Converged }) + assert.Equal(t, GoalClear, s.Goal) + assert.True(t, s.LastChangedAt.IsZero(), "a declined Clear never changes the host") + assert.NoError(t, s.LastError) + + require.NoError(t, m.Close()) + out := logs.String() + assert.Equal(t, 1, strings.Count(out, "level=INFO"), out) + assert.Contains(t, out, "DNS management declined") + assert.NotContains(t, out, "level=WARN") +} + +// TestNew_DarwinWithRootBuildsBackend pins that root keeps the live backend. +func TestNew_DarwinWithRootBuildsBackend(t *testing.T) { + withRoot(t, true) + m, err := New(context.Background(), Options{Identity: "facade-root", ForceBackend: BackendDarwin}) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, m.Close()) }) + assert.NotNil(t, m.locker) + assert.Nil(t, m.declined) +} diff --git a/doc.go b/doc.go index b868f9b..d0fd7f3 100644 --- a/doc.go +++ b/doc.go @@ -10,9 +10,11 @@ // are expected to run as LocalSystem so Group Policy changes can be notified // directly; elevated Administrator operation falls back to a full policy // refresh; see "Convergence and repair". -// New logs a warning through Options.Logger when the macOS process is not root, -// or when the Windows process is not LocalSystem or its identity cannot be -// checked; construction still succeeds. +// On macOS, New declines DNS management for a process that is not root: it +// logs one Info line through Options.Logger, Set returns +// [ErrConfigurationDeclined], and Clear converges without touching the host. +// New logs a warning when the Windows process is not LocalSystem or its +// identity cannot be checked; construction still succeeds. // The OS tools the backends invoke are resolved by absolute path and never // through PATH. Unsupported platforms select [BackendNoop], which refuses // [Manager.Set] unless explicitly forced. diff --git a/errors.go b/errors.go index 000372f..f46a008 100644 --- a/errors.go +++ b/errors.go @@ -56,9 +56,13 @@ var ( // ErrConfigurationDeclined is returned by Set when the active backend // applies nothing and the caller did not ask for that. Detection answers - // BackendNoop on unsupported platforms. Set records no goal. Explicitly - // selecting BackendNoop through Options.ForceBackend permits a goal that - // converges without touching the host. + // BackendNoop on unsupported platforms, and New declines the macOS backend + // for a process that is not root, since every write it would attempt + // needs root. A declined macOS Manager names BackendDarwin in its errors, + // its refusal says why, and its Clear converges without touching the host or + // taking locks. Set records no goal. Explicitly selecting BackendNoop + // through Options.ForceBackend permits a goal that converges without + // touching the host. // // Retrying on the same Manager cannot change this condition. ErrConfigurationDeclined = errors.New("backend applies no DNS configuration") diff --git a/internal/backend/darwin/conflict_test.go b/internal/backend/darwin/conflict_test.go index e30345b..2faea73 100644 --- a/internal/backend/darwin/conflict_test.go +++ b/internal/backend/darwin/conflict_test.go @@ -31,7 +31,7 @@ func (h *sharedHost) backend(identity string) *Backend { if b, ok := h.backends[identity]; ok { return b } - b, err := New(context.Background(), Options{Identity: identity, FS: h.fs, Runner: h.fake, Logger: discardLogger(), IsRoot: asRoot}) + b, err := New(context.Background(), Options{Identity: identity, FS: h.fs, Runner: h.fake, Logger: discardLogger()}) require.NoError(h.t, err) useFS(b, h.fs) h.t.Cleanup(func() { _ = b.Close() }) diff --git a/internal/backend/darwin/converge_test.go b/internal/backend/darwin/converge_test.go index f4d1efc..c812b7a 100644 --- a/internal/backend/darwin/converge_test.go +++ b/internal/backend/darwin/converge_test.go @@ -830,7 +830,6 @@ func TestConvergeLogsWhatChanged(t *testing.T) { FS: fs, Runner: fake, Logger: slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo})), - IsRoot: asRoot, }) require.NoError(t, err) useFS(b, fs) diff --git a/internal/backend/darwin/darwin.go b/internal/backend/darwin/darwin.go index 72d15b1..27c7ddd 100644 --- a/internal/backend/darwin/darwin.go +++ b/internal/backend/darwin/darwin.go @@ -62,9 +62,6 @@ type Options struct { Runner runner.Runner // Logger receives non-fatal warnings. It is required. Logger *slog.Logger - // IsRoot reports whether the process runs as root, for the startup - // diagnostic. Nil checks the effective user ID. - IsRoot func() bool } type stateKeyConfig struct { @@ -121,13 +118,6 @@ func New(_ context.Context, opts Options) (*Backend, error) { if fs == nil { fs = backend.DiskFS{} } - isRoot := opts.IsRoot - if isRoot == nil { - isRoot = func() bool { return os.Geteuid() == 0 } - } - if !isRoot() { - opts.Logger.Warn("darwin: DNS configuration requires root for the dynamic store, /etc/resolver, and the lock directory; without it every Set and Clear attempt is refused before writing") - } r := opts.Runner if r == nil { r = runner.ExecRunner{} diff --git a/internal/backend/darwin/darwin_test.go b/internal/backend/darwin/darwin_test.go index 10b1f19..6c1d186 100644 --- a/internal/backend/darwin/darwin_test.go +++ b/internal/backend/darwin/darwin_test.go @@ -522,9 +522,6 @@ func discardLogger() *slog.Logger { return slog.New(slog.DiscardHandler) } -// asRoot keeps the startup diagnostic independent of the test runner's user. -func asRoot() bool { return true } - func newTestBackend(t *testing.T, fs dirFS, fake runner.Runner) *Backend { t.Helper() b, err := New(context.Background(), Options{ @@ -532,51 +529,12 @@ func newTestBackend(t *testing.T, fs dirFS, fake runner.Runner) *Backend { FS: fs, Runner: fake, Logger: discardLogger(), - IsRoot: asRoot, }) require.NoError(t, err) t.Cleanup(func() { _ = b.Close() }) return useFS(b, fs) } -func TestNewWarnsWithoutRoot(t *testing.T) { - for _, tc := range []struct { - name string - root bool - want string - }{ - {name: "root", root: true}, - {name: "not root", want: "requires root"}, - } { - t.Run(tc.name, func(t *testing.T) { - var logs bytes.Buffer - fs := newTestFS(t) - fake := newScutilFake() - calls := 0 - b, err := New(context.Background(), Options{ - Identity: testIdentity, - FS: fs, - Runner: fake, - Logger: slog.New(slog.NewTextHandler(&logs, nil)), - IsRoot: func() bool { - calls++ - return tc.root - }, - }) - require.NoError(t, err) - t.Cleanup(func() { _ = b.Close() }) - assert.Equal(t, 1, calls) - assertQuiet(t, observeHost(fs, fake), "construction must not change DNS") - if tc.want == "" { - assert.Empty(t, logs.String()) - return - } - assert.Equal(t, 1, strings.Count(logs.String(), "level=WARN")) - assert.Contains(t, logs.String(), tc.want) - }) - } -} - var ( newGoal = backend.Attempt{NewGoal: true} repeat = backend.Attempt{} @@ -990,7 +948,6 @@ func TestClearLeavesKeysItCannotAttribute(t *testing.T) { FS: fs, Runner: fr, Logger: discardLogger(), - IsRoot: asRoot, }) require.NoError(t, err) useFS(b, fs) diff --git a/manager.go b/manager.go index 81a2947..b201907 100644 --- a/manager.go +++ b/manager.go @@ -74,8 +74,10 @@ type Manager struct { logger *slog.Logger kind BackendKind backend backend.Backend - // declined distinguishes unsupported hosts from an explicitly forced noop. - declined bool + // declined, when non-nil, is the ErrConfigurationDeclined refusal for a + // host this process cannot manage. It distinguishes those hosts from an + // explicitly forced noop. + declined error // locker issues the per-identity lifetime lock and the host lock. It is // nil for noop, which never touches the host. @@ -177,6 +179,18 @@ func New(ctx context.Context, opts Options) (*Manager, error) { kind = Detect() } + // A host this process cannot write gets the no-op backend and no locks, + // so Clear converges without attempts that could only fail and retry. + if reason := nativeDeclined(kind); reason != nil { + opts.Logger.Info("dnsconfig: DNS management declined", + slog.String("backend", string(kind)), slog.Any("reason", reason)) + b, _ := newBackend(ctx, BackendNoop, opts) + m := newWithBackend(opts, kind, b, nil) + m.declined = wrapSentinel(ErrConfigurationDeclined, reason) + m.start() + return m, nil + } + b, err := newBackend(ctx, kind, opts) if err != nil { return nil, &Error{Op: "New", Backend: kind, Err: err} @@ -199,12 +213,16 @@ func newWithBackend(opts Options, kind BackendKind, b backend.Backend, locker ho if opts.ShutdownTimeout == 0 { opts.ShutdownTimeout = DefaultShutdownTimeout } + var declined error + if kind == BackendNoop && opts.ForceBackend != BackendNoop { + declined = ErrConfigurationDeclined + } return &Manager{ opts: opts, logger: opts.Logger, kind: kind, backend: b, - declined: kind == BackendNoop && opts.ForceBackend != BackendNoop, + declined: declined, locker: locker, clock: realClock{}, hostLockTimeout: hostLockAcquireTimeout, @@ -237,7 +255,8 @@ func wrapSentinel(sentinel, err error) error { // alone, are returned directly // without an [Error] wrapper. The other refusals record no goal: // [ErrClosed] after Close, [ErrConfigurationDeclined] when -// detection selected the no-op backend, and [ErrInterfaceRequired] on Windows +// detection selected the no-op backend or the process lacks the privilege +// the backend needs, and [ErrInterfaceRequired] on Windows // without Options.InterfaceName. The last two are also recorded in // Status.LastError. // @@ -259,8 +278,8 @@ func (m *Manager) Set(cfg Config) error { if m.st.closed { return &Error{Op: "Set", Backend: m.kind, Err: ErrClosed} } - if m.declined { - return m.refuseSetLocked(ErrConfigurationDeclined) + if m.declined != nil { + return m.refuseSetLocked(m.declined) } // Recovery records identify the adapter for Clear, but a Set has no // adapter to write without the caller naming one. diff --git a/searchlimit_darwin_test.go b/searchlimit_darwin_test.go index f5516be..3ab9959 100644 --- a/searchlimit_darwin_test.go +++ b/searchlimit_darwin_test.go @@ -16,6 +16,7 @@ import ( // backend refuses a search list that does not fit one scutil line before a // goal is recorded, so nothing is applied and Close has nothing to revert. func TestSetRejectsSearchListOverScutilLine(t *testing.T) { + withRoot(t, true) m, err := New(context.Background(), Options{Identity: "facade-search-limit", ForceBackend: BackendDarwin}) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, m.Close()) }) From 62a1a085a62329830819fb8b30d19f1d7743fbe0 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Wed, 28 Oct 2026 13:01:02 -0400 Subject: [PATCH 2/5] Warn about non-LocalSystem on Windows only when writing The LocalSystem diagnostic ran at construction, so every Windows process that built a Manager logged it, including one whose startup Clear found nothing and never wrote. The warning matters only for a process that changes the host, where Group Policy rules can stay effective after cleanup. The check now runs once per backend, before the first registry write of a Set or of a Clear that found artifacts to remove. --- doc.go | 5 ++-- internal/backend/windows/converge.go | 3 ++ internal/backend/windows/windows.go | 35 +++++++++++++++++------- internal/backend/windows/windows_test.go | 34 ++++++++++++----------- 4 files changed, 49 insertions(+), 28 deletions(-) diff --git a/doc.go b/doc.go index d0fd7f3..6ad9de1 100644 --- a/doc.go +++ b/doc.go @@ -13,8 +13,9 @@ // On macOS, New declines DNS management for a process that is not root: it // logs one Info line through Options.Logger, Set returns // [ErrConfigurationDeclined], and Clear converges without touching the host. -// New logs a warning when the Windows process is not LocalSystem or its -// identity cannot be checked; construction still succeeds. +// On Windows, the first attempt that writes to the host logs a warning when +// the process is not LocalSystem or its identity cannot be checked; the +// attempt proceeds. // The OS tools the backends invoke are resolved by absolute path and never // through PATH. Unsupported platforms select [BackendNoop], which refuses // [Manager.Set] unless explicitly forced. diff --git a/internal/backend/windows/converge.go b/internal/backend/windows/converge.go index 4b06911..11aca43 100644 --- a/internal/backend/windows/converge.go +++ b/internal/backend/windows/converge.go @@ -34,6 +34,9 @@ type mutation struct { // notification for a write to the GP store. func (x *mutation) begin(base string) { + if !x.started { + x.b.warnIdentityOnce() + } x.started = true x.b.flushOwed = true if base == gpoNrptBase { diff --git a/internal/backend/windows/windows.go b/internal/backend/windows/windows.go index 58ad382..8bf2ce7 100644 --- a/internal/backend/windows/windows.go +++ b/internal/backend/windows/windows.go @@ -225,7 +225,8 @@ type Options struct { // Logger receives structured diagnostics. It is required. Logger *slog.Logger - // IsLocalSystem checks the execution identity for the startup diagnostic. + // IsLocalSystem checks the execution identity for the diagnostic logged + // before the first write. // Nil uses the process token on Windows and skips the check elsewhere. IsLocalSystem func() (bool, error) @@ -278,6 +279,10 @@ type Backend struct { // The fields from here to mu are touched only by Set and Clear, which // the Manager never runs concurrently. + // checkIdentity is the LocalSystem check warnIdentityOnce runs, nil once + // it has run or where no check exists. + checkIdentity func() (bool, error) + // adapterTarget is the adapter whose NameServer and SearchList this // instance still owes cleanup: one it wrote to, or a recorded // predecessor's adapter whose clear failed. It survives external record @@ -319,15 +324,6 @@ func New(_ context.Context, opts Options) (*Backend, error) { if checkIdentity == nil { checkIdentity = newDefaultLocalSystemCheck() } - if checkIdentity != nil { - system, err := checkIdentity() - switch { - case err != nil: - logger.Warn("windows: could not verify execution identity; DNS configuration expects LocalSystem", "error", err) - case !system: - logger.Warn("windows: DNS configuration expects LocalSystem; running without it is best effort, and Group Policy DNS rules may remain effective after cleanup") - } - } r := opts.Runner if r == nil { r = runner.ExecRunner{} @@ -363,10 +359,29 @@ func New(_ context.Context, opts Options) (*Backend, error) { resolveInterface: resolve, notifier: notifier, domainJoined: domainJoined, + checkIdentity: checkIdentity, changes: make(chan struct{}, 1), }, nil } +// warnIdentityOnce logs the execution-identity diagnostic before the first +// write. Every Windows process builds a backend, but only one that writes +// can leave Group Policy rules effective after cleanup. +func (b *Backend) warnIdentityOnce() { + check := b.checkIdentity + if check == nil { + return + } + b.checkIdentity = nil + system, err := check() + switch { + case err != nil: + b.logger.Warn("windows: could not verify execution identity; DNS configuration expects LocalSystem", "error", err) + case !system: + b.logger.Warn("windows: DNS configuration expects LocalSystem; running without it is best effort, and Group Policy DNS rules may remain effective after cleanup") + } +} + // Changes returns the coalesced change signal. It is closed by Close. func (b *Backend) Changes() <-chan struct{} { return b.changes } diff --git a/internal/backend/windows/windows_test.go b/internal/backend/windows/windows_test.go index 02293df..c8dbad7 100644 --- a/internal/backend/windows/windows_test.go +++ b/internal/backend/windows/windows_test.go @@ -57,7 +57,10 @@ func discardLogger() *slog.Logger { return slog.New(slog.DiscardHandler) } -func TestExecutionIdentityWarningDoesNotAffectLifecycle(t *testing.T) { +// TestExecutionIdentityWarningAtFirstWrite checks that the LocalSystem +// diagnostic waits for the first write, so a process that only constructs a +// backend or clears an empty host logs nothing, and that it is logged once. +func TestExecutionIdentityWarningAtFirstWrite(t *testing.T) { for _, tc := range []struct { name string system bool @@ -83,25 +86,24 @@ func TestExecutionIdentityWarningDoesNotAffectLifecycle(t *testing.T) { }) require.NoError(t, err) t.Cleanup(func() { _ = b.Close() }) - assert.Empty(t, reg.writes, "construction must not change DNS") - if tc.want == "" { - assert.Empty(t, logs.String()) - } else { - assert.Contains(t, logs.String(), "level=WARN") - assert.Contains(t, logs.String(), tc.want) - if tc.err != nil { - assert.Contains(t, logs.String(), tc.err.Error()) - } - } - startup := logs.String() - logs.Reset() + require.NoError(t, b.clearGoal(ctx)) + assert.Empty(t, reg.writes, "an empty host gives Clear nothing to write") + assert.Zero(t, calls, "nothing written, nothing checked") + assert.NotContains(t, logs.String(), "LocalSystem") + require.NoError(t, b.setGoal(ctx, catchAll(t))) require.NoError(t, b.clearGoal(ctx)) + require.NoError(t, b.setGoal(ctx, catchAll(t))) require.NoError(t, b.Close()) assert.Equal(t, 1, calls) - assert.NotContains(t, logs.String(), "expects LocalSystem") - if tc.want != "" { - assert.Equal(t, 1, strings.Count(startup, "level=WARN")) + if tc.want == "" { + assert.NotContains(t, logs.String(), "level=WARN") + return + } + assert.Equal(t, 1, strings.Count(logs.String(), "level=WARN"), logs.String()) + assert.Contains(t, logs.String(), tc.want) + if tc.err != nil { + assert.Contains(t, logs.String(), tc.err.Error()) } }) } From c669caef56cce91fe84c11b2d58bcf99ad0c3d42 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Wed, 28 Oct 2026 15:51:49 -0400 Subject: [PATCH 3/5] Keep the declined refusal in Status after Clear converges --- declined_darwin_test.go | 14 +++++++++++-- doc.go | 2 ++ errors.go | 4 +++- manager.go | 12 ++++++++++- manager_test.go | 44 +++++++++++++++++++++++++++++++++++++++++ status.go | 10 +++++++--- 6 files changed, 79 insertions(+), 7 deletions(-) diff --git a/declined_darwin_test.go b/declined_darwin_test.go index db2e169..5583635 100644 --- a/declined_darwin_test.go +++ b/declined_darwin_test.go @@ -35,6 +35,11 @@ func TestNew_DarwinWithoutRootDeclines(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { _ = m.Close() }) + s := m.Status() + require.ErrorIs(t, s.LastError, ErrConfigurationDeclined, "the refusal is visible from New on") + assert.ErrorContains(t, s.LastError, "requires root") + assert.False(t, s.LastConvergedCleared) + assert.Equal(t, BackendDarwin, m.kind) assert.Nil(t, m.locker, "a declined Manager takes no host locks") assert.IsType(t, &noop.Backend{}, m.backend, "the darwin backend is never built") @@ -45,7 +50,7 @@ func TestNew_DarwinWithoutRootDeclines(t *testing.T) { var e *Error require.ErrorAs(t, err, &e) assert.Equal(t, BackendDarwin, e.Backend) - s := m.Status() + s = m.Status() assert.Equal(t, GoalNone, s.Goal) assert.Same(t, err, s.LastError) @@ -53,9 +58,14 @@ func TestNew_DarwinWithoutRootDeclines(t *testing.T) { s = waitStatus(t, m, func(s Status) bool { return s.Converged }) assert.Equal(t, GoalClear, s.Goal) assert.True(t, s.LastChangedAt.IsZero(), "a declined Clear never changes the host") - assert.NoError(t, s.LastError) + assert.ErrorIs(t, s.LastError, ErrConfigurationDeclined, "a declined Clear verified nothing") + assert.False(t, s.LastConvergedCleared) require.NoError(t, m.Close()) + s = m.Status() + assert.ErrorIs(t, s.LastError, ErrConfigurationDeclined) + assert.ErrorContains(t, s.LastError, "requires root") + assert.False(t, s.LastConvergedCleared) out := logs.String() assert.Equal(t, 1, strings.Count(out, "level=INFO"), out) assert.Contains(t, out, "DNS management declined") diff --git a/doc.go b/doc.go index 6ad9de1..5b47b1f 100644 --- a/doc.go +++ b/doc.go @@ -13,6 +13,8 @@ // On macOS, New declines DNS management for a process that is not root: it // logs one Info line through Options.Logger, Set returns // [ErrConfigurationDeclined], and Clear converges without touching the host. +// Status.LastError keeps that refusal for the Manager's life, so a converged +// Clear does not read as a verified clean host. // On Windows, the first attempt that writes to the host logs a warning when // the process is not LocalSystem or its identity cannot be checked; the // attempt proceeds. diff --git a/errors.go b/errors.go index f46a008..75903a2 100644 --- a/errors.go +++ b/errors.go @@ -60,7 +60,9 @@ var ( // for a process that is not root, since every write it would attempt // needs root. A declined macOS Manager names BackendDarwin in its errors, // its refusal says why, and its Clear converges without touching the host or - // taking locks. Set records no goal. Explicitly selecting BackendNoop + // taking locks. Set records no goal. Status.LastError holds this error + // from New on and LastConvergedCleared stays false, since a converged + // Clear checked nothing. Explicitly selecting BackendNoop // through Options.ForceBackend permits a goal that converges without // touching the host. // diff --git a/manager.go b/manager.go index b201907..ddb4fa7 100644 --- a/manager.go +++ b/manager.go @@ -317,7 +317,9 @@ func (m *Manager) refuseSetLocked(cause error) error { // recovery does not need InterfaceName or a saved snapshot, and clears that // adapter's DNS values rather than restoring older ones. Discovering nothing // attributable is a successful Clear, so a setting whose ownership evidence -// was lost stays in place. +// was lost stays in place. A Manager that declined DNS management converges +// a Clear without touching the host; Status keeps [ErrConfigurationDeclined] +// in LastError and LastConvergedCleared false. // // Clear returns [ErrClosed] after Close and otherwise nil. Calling it while // Clear is already the goal is a no-op. @@ -831,6 +833,14 @@ func (m *Manager) Status() Status { LastChangedAt: m.st.lastChangedAt, LastConvergedCleared: m.st.lastConvergedCleared, } + // A declined Manager verifies nothing, so a converged Clear must not read + // as a clean host; the refusal stays visible for the Manager's life. + if m.declined != nil { + if s.LastError == nil { + s.LastError = &Error{Op: "New", Backend: m.kind, Err: m.declined} + } + s.LastConvergedCleared = false + } if m.st.goal == GoalSet { s.Desired = cloneConfig(m.st.desired) } diff --git a/manager_test.go b/manager_test.go index bf3e06c..983a705 100644 --- a/manager_test.go +++ b/manager_test.go @@ -70,6 +70,50 @@ func TestSet_ForcedNoopConverges(t *testing.T) { require.NoError(t, m.Close()) } +// TestStatus_DeclinedKeepsRefusal pins that a Manager whose backend applies +// nothing, unless the caller forced BackendNoop, never reports a verified +// clean host: its Clear converges, but LastError keeps the refusal and +// LastConvergedCleared stays false through Close. +func TestStatus_DeclinedKeepsRefusal(t *testing.T) { + for _, tc := range []struct { + name string + opts Options + declined bool + }{ + {name: "detected noop", declined: true}, + {name: "forced noop", opts: Options{ForceBackend: BackendNoop}}, + } { + t.Run(tc.name, func(t *testing.T) { + h := newHarness(t, BackendNoop, tc.opts) + check := func(when string) { + t.Helper() + s := h.m.Status() + if tc.declined { + assert.ErrorIs(t, s.LastError, ErrConfigurationDeclined, when) + assert.False(t, s.LastConvergedCleared, when) + } else { + assert.NoError(t, s.LastError, when) + } + } + check("after New") + var e *Error + if tc.declined { + require.ErrorAs(t, h.m.Status().LastError, &e) + assert.Equal(t, "New", e.Op) + } + + require.NoError(t, h.m.Clear()) + h.converged("clear") + assert.True(t, h.m.Status().Converged, "callers waiting for convergence still see it") + check("after Clear") + assert.Equal(t, !tc.declined, h.m.Status().LastConvergedCleared) + + h.closeAnswering() + check("after Close") + }) + } +} + func TestSet_ValidationReturnsDirectly(t *testing.T) { for _, tc := range []struct { name string diff --git a/status.go b/status.go index 966d5a4..0f5e89b 100644 --- a/status.go +++ b/status.go @@ -41,8 +41,11 @@ type Status struct { // Op "Converge", or of Close, or a Set refusal that Set also returned. // After a Close that left a live holder's configuration in place it // includes [ErrManagerAlreadyRunning] with Op "Close", which Close - // itself does not return. It is nil after a successful attempt; - // errors.Is and errors.As reach the cause. + // itself does not return. It is nil after a successful attempt, except + // on a Manager that declined DNS management: from New on it holds + // [ErrConfigurationDeclined], as Op "New" or a Set refusal, because + // nothing on the host was checked. errors.Is and errors.As reach the + // cause. // Before Close the worker retries a failed attempt with backoff; Set // refusals and Close's failure are not retried. LastError error @@ -55,6 +58,7 @@ type Status struct { // LastConvergedCleared reports that the most recent successful attempt, // including Close's final cleanup, was a Clear. It is false before any - // attempt succeeds. + // attempt succeeds and always false on a Manager that declined DNS + // management, which removes nothing. LastConvergedCleared bool } From fb1a34ccdb4d77b365d38ca88b82e7fea887e6ef Mon Sep 17 00:00:00 2001 From: John Maguire Date: Wed, 28 Oct 2026 16:00:48 -0400 Subject: [PATCH 4/5] Warn about non-LocalSystem before a reload-only Set and document the declined path --- example_test.go | 3 +++ internal/backend/windows/windows.go | 7 ++++-- internal/backend/windows/windows_test.go | 29 ++++++++++++++++++++++++ manager.go | 6 +++-- options.go | 5 +++- status.go | 3 ++- 6 files changed, 47 insertions(+), 6 deletions(-) diff --git a/example_test.go b/example_test.go index ccf4f42..cf7de15 100644 --- a/example_test.go +++ b/example_test.go @@ -79,6 +79,9 @@ func ExampleManager_Clear_recovery() { for { s := m.Status() switch { + case errors.Is(s.LastError, dnsconfig.ErrConfigurationDeclined): + log.Printf("DNS management declined; nothing was checked: %v", s.LastError) + return case s.Converged && s.LastChangedAt.IsZero(): log.Print("no DNS configuration attributable to myapp was found") return diff --git a/internal/backend/windows/windows.go b/internal/backend/windows/windows.go index 8bf2ce7..59f1ea2 100644 --- a/internal/backend/windows/windows.go +++ b/internal/backend/windows/windows.go @@ -226,7 +226,7 @@ type Options struct { Logger *slog.Logger // IsLocalSystem checks the execution identity for the diagnostic logged - // before the first write. + // before the first registry write, including a reload trigger. // Nil uses the process token on Windows and skips the check elsewhere. IsLocalSystem func() (bool, error) @@ -365,7 +365,7 @@ func New(_ context.Context, opts Options) (*Backend, error) { } // warnIdentityOnce logs the execution-identity diagnostic before the first -// write. Every Windows process builds a backend, but only one that writes +// registry write, from mutation.begin and reload. Every Windows process builds a backend, but only one that writes // can leave Group Policy rules effective after cleanup. func (b *Backend) warnIdentityOnce() { check := b.checkIdentity @@ -469,6 +469,9 @@ func (b *Backend) reload(ctx context.Context, mut *mutation) error { if err := ctx.Err(); err != nil { return err } + // A reload can be an attempt's only write, when a predecessor's artifacts + // already match. + b.warnIdentityOnce() if err := b.reg.SetString(nrptBase, reloadTriggerValue, "1"); err != nil { return fmt.Errorf("windows: trigger adapter reload: %w", err) } diff --git a/internal/backend/windows/windows_test.go b/internal/backend/windows/windows_test.go index c8dbad7..4d408a9 100644 --- a/internal/backend/windows/windows_test.go +++ b/internal/backend/windows/windows_test.go @@ -109,6 +109,35 @@ func TestExecutionIdentityWarningAtFirstWrite(t *testing.T) { } } +// TestExecutionIdentityWarningOnReloadOnlySet covers a restart whose +// predecessor's artifacts already match, so the reload trigger is the only +// write and must still be preceded by the diagnostic. +func TestExecutionIdentityWarningOnReloadOnlySet(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + require.NoError(t, h.setGoal(ctx, catchAll(t))) + require.NoError(t, h.Close()) + + var logs bytes.Buffer + b, err := newTestBackend(ctx, Options{ + Registry: h.reg, + Logger: slog.New(slog.NewTextHandler(&logs, nil)), + IsLocalSystem: func() (bool, error) { return false, nil }, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = b.Close() }) + h.reg.writes = nil + res, err := b.Set(ctx, catchAll(t), backend.Attempt{NewGoal: true}) + require.NoError(t, err) + assert.Equal(t, backend.Result{}, res, "matching values need no write") + assert.Equal(t, []string{ + "SetString " + nrptBase + " " + reloadTriggerValue, + "DeleteValue " + nrptBase + " " + reloadTriggerValue, + }, h.reg.writes, "the only writes are one reload trigger") + assert.Equal(t, 1, strings.Count(logs.String(), "level=WARN"), logs.String()) + assert.Contains(t, logs.String(), "running without it is best effort") +} + func TestNewRequiresLogger(t *testing.T) { _, err := New(context.Background(), Options{Identity: testIdentity, Registry: newFakeRegistry()}) require.Error(t, err) diff --git a/manager.go b/manager.go index ddb4fa7..abb0bbc 100644 --- a/manager.go +++ b/manager.go @@ -80,7 +80,7 @@ type Manager struct { declined error // locker issues the per-identity lifetime lock and the host lock. It is - // nil for noop, which never touches the host. + // nil for noop and a declined Manager, which never touch the host. locker hostlock.Locker // lifetime is held from the first attempt at a goal until a Clear // converges or Close finishes. Only the worker touches it while running; @@ -165,7 +165,9 @@ type attemptJob struct { // Invalid Options are returned as-is. Construction failures are wrapped in // *Error, with ErrInvalidOptions for a backend absent on this OS. No backend // touches the host at construction or needs Options.InterfaceName; Windows -// checks it at Set. +// checks it at Set. A host this process cannot manage, such as macOS without +// root, gets a declined Manager instead of an error; see +// [ErrConfigurationDeclined]. func New(ctx context.Context, opts Options) (*Manager, error) { if err := opts.Validate(); err != nil { return nil, err diff --git a/options.go b/options.go index 3bebd0c..0bdbc2c 100644 --- a/options.go +++ b/options.go @@ -63,7 +63,10 @@ type Options struct { // ForceBackend selects a specific backend; the zero value requests // detection. New never falls back to a different backend: an undeclared // kind fails Validate, and a kind with no implementation on this OS fails - // New with ErrInvalidOptions. See [BackendNoop] for forcing the no-op one. + // New with ErrInvalidOptions. The exception is a declined host, such as + // BackendDarwin in a process that is not root, which gets a Manager that + // refuses Set with [ErrConfigurationDeclined]. See [BackendNoop] for + // forcing the no-op one. ForceBackend BackendKind // InterfaceName is the friendly name of a Windows adapter, such as a diff --git a/status.go b/status.go index 0f5e89b..2d50677 100644 --- a/status.go +++ b/status.go @@ -31,7 +31,8 @@ type Status struct { // Converged reports that the most recent attempt at the current goal // passed its artifact checks and completed every owed side-effect - // request, and no newer goal has been recorded since. Later drift is + // request, and no newer goal has been recorded since. A declined + // Manager checks nothing; see LastError. Later drift is // found by a change signal or periodic check. Close's final cleanup, // when it runs, counts as an attempt: afterwards Converged is false for // a Set goal and reports whether that cleanup succeeded for a Clear goal. From 3b833e0a938388a9397809f74a62998718ec9729 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Wed, 28 Oct 2026 16:07:51 -0400 Subject: [PATCH 5/5] Skip the darwin case of the native construction test --- native_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/native_test.go b/native_test.go index df96c54..cd552b0 100644 --- a/native_test.go +++ b/native_test.go @@ -16,7 +16,8 @@ func TestNewConstructsNativeBackend(t *testing.T) { var kind BackendKind switch runtime.GOOS { case "darwin": - kind = BackendDarwin + // A non-root test user would get a declined Manager here. + t.Skip("covered by TestNew_DarwinWithRootBuildsBackend, which forces the root check") case "windows": kind = BackendWindows default: