From b682cfd0bac68baa703d9f093cfda733bf0b280a Mon Sep 17 00:00:00 2001 From: Andrew Moore Date: Sat, 8 Aug 2026 17:39:40 -0700 Subject: [PATCH] Merge nucleic/vivid-glass-urchin-xoym into main --- README.md | 2 +- Sources/RunnerCore/LocalNetworkPolicy.swift | 102 ++++++++++++++---- Sources/RunnerHost/Doctor.swift | 22 +++- .../RunnerHost/LocalNetworkPermission.swift | 61 ++++++++++- .../CommandPermissions.swift | 49 ++++++--- .../LocalNetworkPolicyTests.swift | 69 ++++++++++++ docs/setup.md | 10 +- docs/troubleshooting.md | 8 +- 8 files changed, 281 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index f9adaa0..dbcc6ff 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ Every subcommand accepts the global options `--config PATH` (`-c`, default | `service install [--executable PATH] [--grant-local-network allowlist\|prompt\|none]` | Write and load `~/Library/LaunchAgents/xyz.blakeslee.gitea-macos-vm-orchestrator.plist`. Also evicts any agent left behind under a previous label. On a terminal it offers to configure Local Network access when that is unconfigured, defaulting to no; `--grant-local-network` decides it up front. | | `service uninstall` | Unload the LaunchAgent and remove its plist. | | `service status` | Report LaunchAgent installation and run state. | -| `permissions status` | Report whether macOS Local Network access is configured, and whether the code identity is stable enough to hold an interactive grant. | +| `permissions status` | Report whether macOS Local Network access is configured, and whether the code identity is stable enough to hold an interactive grant. Run it under `sudo` to see the allowlist — it is written into root's preferences, which an ordinary login cannot read. | | `permissions grant [--method allowlist\|prompt] [--subnet CIDR ...] [--reboot\|--no-reboot]` | Grant Local Network access. `allowlist` (default) writes the subnet allowlist with sudo — all of RFC 1918 unless `--subnet` narrows it — and needs a reboot. `prompt` launches the installed `.app` so the system alert is attributed to it rather than to Terminal, and applies immediately. | | `doctor [--json] [--no-fail]` | Preflight checks. `--json` emits machine-readable results; `--no-fail` exits zero even when checks fail. | | `config init [--force] [--instance-url URL]` | Write the annotated example config. `--force` (`-f`) overwrites an existing file. | diff --git a/Sources/RunnerCore/LocalNetworkPolicy.swift b/Sources/RunnerCore/LocalNetworkPolicy.swift index 9ff3237..0146f6d 100644 --- a/Sources/RunnerCore/LocalNetworkPolicy.swift +++ b/Sources/RunnerCore/LocalNetworkPolicy.swift @@ -50,6 +50,12 @@ public enum LocalNetworkPolicy { /// The domain is written with `sudo`, so which preferences directory it /// lands in depends on whether that `sudo` preserved `HOME`. Rather than /// guess at the host's sudoers configuration, check each candidate. + /// + /// In practice the first candidate is where it lands, and `/var/root` is + /// mode 700 — so an unprivileged process cannot read back what it just + /// wrote. That is what ``Status/unreadablePaths`` exists to report, and + /// what `RunnerHost`'s `LocalNetworkPermission.observedStatus()` works + /// around by re-reading as root. public static func preferenceCandidates() -> [String] { [ "/var/root/Library/Preferences/\(domain).plist", @@ -112,47 +118,103 @@ public enum LocalNetworkPolicy { /// Whether at least one entry covers the whole vmnet range. public let coversGuestRange: Bool + /// Candidate files this process was refused permission to read. + /// + /// Not the same as "absent". `sudo defaults write` normally lands in + /// `/var/root/Library/Preferences`, which is mode 700, so an ordinary + /// user is refused before it can find out whether the file is even + /// there. An empty ``allowlist`` with a non-empty `unreadablePaths` + /// means *unknown*, not *unconfigured*, and must not be reported as + /// the latter. + public let unreadablePaths: [String] + /// Whether anything is configured at all. public var isConfigured: Bool { !allowlist.isEmpty } - public init(allowlist: [String], sourcePaths: [String], coversGuestRange: Bool) { + /// Whether nothing was found and something could not be read, so the + /// answer is genuinely unknown without administrator rights. + public var isIndeterminate: Bool { allowlist.isEmpty && !unreadablePaths.isEmpty } + + public init( + allowlist: [String], + sourcePaths: [String], + coversGuestRange: Bool, + unreadablePaths: [String] = [] + ) { self.allowlist = allowlist self.sourcePaths = sourcePaths self.coversGuestRange = coversGuestRange + self.unreadablePaths = unreadablePaths } } - /// Reads the host's current allowlist. + /// Reads the host's current allowlist with this process's own privileges. /// - /// Best effort and never fatal: an unreadable or absent preferences file - /// simply reads as "no allowlist". + /// Best effort and never fatal: an absent preferences file reads as "no + /// allowlist", and one that exists but cannot be opened is recorded in + /// ``Status/unreadablePaths`` rather than being mistaken for absent. public static func status() -> Status { - var found: [String] = [] - var sources: [String] = [] + var sources: [(path: String, data: Data)] = [] + var unreadable: [String] = [] for path in preferenceCandidates() { - guard let data = FileManager.default.contents(atPath: path), - let plist = try? PropertyListSerialization.propertyList( - from: data, options: [], format: nil) as? [String: Any] - else { continue } - - var contributed = false - for key in keys { - for entry in (plist[key] as? [String] ?? []) where !found.contains(entry) { - found.append(entry) - contributed = true - } + if let data = FileManager.default.contents(atPath: path) { + sources.append((path, data)) + } else if access(path, R_OK) != 0, errno == EACCES { + // Refused, not missing — including when the refusal is on a + // parent directory, which is exactly the /var/root case. + unreadable.append(path) } - if contributed { sources.append(path) } + } + + return status(fromContentsOf: sources, unreadablePaths: unreadable) + } + + /// Builds a ``Status`` from preferences files already read, however they + /// were obtained. + /// + /// Split out from ``status()`` so the privileged read-back in `RunnerHost` + /// — which has to shell out to `sudo` to see root's copy — shares this + /// parsing rather than reimplementing it. + public static func status( + fromContentsOf sources: [(path: String, data: Data)], + unreadablePaths: [String] = [] + ) -> Status { + var found: [String] = [] + var paths: [String] = [] + + for source in sources { + let fresh = entries(inPreferences: source.data).filter { !found.contains($0) } + guard !fresh.isEmpty else { continue } + found.append(contentsOf: fresh) + paths.append(source.path) } return Status( allowlist: found, - sourcePaths: sources, - coversGuestRange: found.contains(where: coversVMNetRange) + sourcePaths: paths, + coversGuestRange: found.contains(where: coversVMNetRange), + unreadablePaths: unreadablePaths ) } + /// Every allowlist entry in one preferences file, across both keys, in the + /// order encountered and without duplicates. Unparseable data reads empty. + public static func entries(inPreferences data: Data) -> [String] { + guard + let plist = try? PropertyListSerialization.propertyList( + from: data, options: [], format: nil) as? [String: Any] + else { return [] } + + var found: [String] = [] + for key in keys { + for entry in (plist[key] as? [String] ?? []) where !found.contains(entry) { + found.append(entry) + } + } + return found + } + /// Subnets pre-authorized for local network access on this host, if any. public static func allowlist() -> [String] { status().allowlist } diff --git a/Sources/RunnerHost/Doctor.swift b/Sources/RunnerHost/Doctor.swift index 57c4727..ddabe93 100644 --- a/Sources/RunnerHost/Doctor.swift +++ b/Sources/RunnerHost/Doctor.swift @@ -771,7 +771,7 @@ public enum Doctor { /// does not apply (Apple, TN3179). public static func localNetworkNote() -> DoctorCheck { let name = "local network access" - let status = LocalNetworkPolicy.status() + let status = LocalNetworkPermission.observedStatus() if status.isConfigured { if status.coversGuestRange { @@ -796,10 +796,26 @@ public enum Doctor { ) } + // Nothing found. On all but an unusual host that means *not visible* + // rather than *not set*: the allowlist is written as root and lands in + // /var/root, which is mode 700. Say which of the two this is, because + // "no allowlist" would otherwise be asserted on a host that has one. + let caveat = + status.isIndeterminate + ? """ + This cannot see the setting itself — it lives in \ + \(status.unreadablePaths[0]), which only root can read — so treat the above as \ + "not visible", not "not set". `sudo gitea-macos-runner permissions status` \ + answers definitively, and `permissions grant` verifies its own write. + """ + : "" + return DoctorCheck( name: name, result: .info, - detail: "guests are reached over the host-private NAT link", + detail: status.isIndeterminate + ? "no allowlist visible; guests are reached over the host-private NAT link" + : "guests are reached over the host-private NAT link", remediation: """ on macOS 15+ the first connection to a guest can be blocked by the Local Network \ privacy prompt, and frequently there is nothing able to answer it. A LaunchAgent \ @@ -810,7 +826,7 @@ public enum Doctor { `gitea-macos-runner permissions grant`, which writes a subnet allowlist that \ needs no prompt, covers every process, and survives rebuilds — then reboot. \ `permissions status` explains both routes. See docs/setup.md §2.6. - """ + """ + caveat ) } diff --git a/Sources/RunnerHost/LocalNetworkPermission.swift b/Sources/RunnerHost/LocalNetworkPermission.swift index a157fc9..87f0dbe 100644 --- a/Sources/RunnerHost/LocalNetworkPermission.swift +++ b/Sources/RunnerHost/LocalNetworkPermission.swift @@ -133,7 +133,66 @@ public enum LocalNetworkPermission { } } - return AllowlistResult(requested: subnets, observed: LocalNetworkPolicy.status()) + return AllowlistResult(requested: subnets, observed: observedStatus()) + } + + /// The host's allowlist, read with root's privileges when this process's + /// own are not enough. + /// + /// `sudo defaults write ` lands in `/var/root/Library/Preferences` + /// on a stock host, and that directory is mode 700 — so the plain read in + /// ``LocalNetworkPolicy/status()`` is refused and a write that worked + /// perfectly looks like it vanished. Re-read the refused candidates as + /// root instead. + /// + /// Always `sudo -n`, so this can never turn a status query into a password + /// prompt. Right after a write the credentials are still cached and it + /// simply works; later — after a reboot, say — it fails and the result + /// stays ``LocalNetworkPolicy/Status/isIndeterminate``, which callers + /// report as "cannot tell without root" rather than as "not configured". + public static func observedStatus() -> LocalNetworkPolicy.Status { + let unprivileged = LocalNetworkPolicy.status() + guard !unprivileged.unreadablePaths.isEmpty else { return unprivileged } + + var sources: [(path: String, data: Data)] = [] + for path in LocalNetworkPolicy.preferenceCandidates() { + if let data = FileManager.default.contents(atPath: path) { + sources.append((path, data)) + } else if let data = readAsRoot(path) { + sources.append((path, data)) + } + } + + let recovered = LocalNetworkPolicy.status(fromContentsOf: sources) + // Nothing came back from the privileged read either: keep the + // unprivileged answer, which still carries why it could not tell. + guard recovered.isConfigured else { return unprivileged } + return recovered + } + + /// `sudo -n cat `, or nil if that fails for any reason. + /// + /// Both failure modes are ordinary rather than exceptional — the candidate + /// usually does not exist, and `sudo -n` legitimately refuses when no + /// credentials are cached — so stderr is discarded instead of being shown + /// to the operator. `Process` is fine here, unlike in ``runInForeground``: + /// `-n` never touches the terminal. + private static func readAsRoot(_ path: String) -> Data? { + let process = Process() + process.executableURL = URL(fileURLWithPath: "/usr/bin/sudo") + process.arguments = ["-n", "/bin/cat", path] + + let output = Pipe() + process.standardOutput = output + process.standardError = FileHandle.nullDevice + process.standardInput = FileHandle.nullDevice + + guard (try? process.run()) != nil else { return nil } + let data = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + + guard process.terminationStatus == 0, !data.isEmpty else { return nil } + return data } /// Reboots the host. Only ever called from an explicit confirmation — the diff --git a/Sources/gitea-macos-runner/CommandPermissions.swift b/Sources/gitea-macos-runner/CommandPermissions.swift index 374b67d..a524440 100644 --- a/Sources/gitea-macos-runner/CommandPermissions.swift +++ b/Sources/gitea-macos-runner/CommandPermissions.swift @@ -47,7 +47,7 @@ struct PermissionsCommand: AsyncParsableCommand { Doctor.checkCodeSignature(), ])) - let status = LocalNetworkPolicy.status() + let status = LocalNetworkPermission.observedStatus() if !status.sourcePaths.isEmpty { print("") for path in status.sourcePaths { @@ -55,9 +55,19 @@ struct PermissionsCommand: AsyncParsableCommand { } } + if status.isIndeterminate { + print("") + print("The allowlist lives in root's preferences, which only root can read, so") + print("this cannot tell whether it is already set. For a definitive answer:") + print(" sudo gitea-macos-runner permissions status") + } + guard !status.coversGuestRange else { return } print("") - print("to fix:") + // Not visible is not the same as not set, and an unconfigured host + // looks identical to a configured one from an ordinary login — so + // offer the commands without asserting anything is broken. + print(status.isIndeterminate ? "if it is not set, either of these sets it:" : "to fix:") print(" gitea-macos-runner permissions grant # subnet allowlist, needs a reboot") print(" gitea-macos-runner permissions grant --method prompt # system prompt, takes effect at once") } @@ -185,7 +195,29 @@ enum LocalNetworkGrantFlow { // `defaults` reports success regardless of which preferences directory // the write landed in, so report what was read back rather than what // was asked for. See LocalNetworkPermission.grantViaAllowlist. - guard result.verified else { + if result.verified { + print("granted: \(result.observed.allowlist.joined(separator: ", "))") + for path in result.observed.sourcePaths { + print("written to: \(path)") + } + if !result.observed.coversGuestRange { + CLI.note(""" + warning: none of these cover the whole guest range (192.168.64.0/18), \ + so guests will still be blocked once the NAT subnet shifts + """) + } + } else if result.observed.isIndeterminate { + // The write succeeded but there is no way to look: the allowlist + // lands in root's preferences, and this host does not keep sudo + // credentials cached long enough for the read-back to use them. + // Unknown is not failure — say so plainly rather than either + // claiming success or crying wolf. + CLI.note(""" + wrote \(subnets.joined(separator: ", ")), but could not read it back to \ + confirm — that needs administrator rights this process no longer holds. \ + Check it with: sudo defaults read \(LocalNetworkPolicy.domain) + """) + } else { CLI.error(""" the write reported success but the values could not be read back. \ Check by hand: sudo defaults read \(LocalNetworkPolicy.domain) @@ -193,17 +225,6 @@ enum LocalNetworkGrantFlow { throw ExitCode(1) } - print("granted: \(result.observed.allowlist.joined(separator: ", "))") - for path in result.observed.sourcePaths { - print("written to: \(path)") - } - if !result.observed.coversGuestRange { - CLI.note(""" - warning: none of these cover the whole guest range (192.168.64.0/18), \ - so guests will still be blocked once the NAT subnet shifts - """) - } - print("") print("This is read at boot, so it does nothing until the host reboots.") diff --git a/Tests/RunnerCoreTests/LocalNetworkPolicyTests.swift b/Tests/RunnerCoreTests/LocalNetworkPolicyTests.swift index 43d4db9..28710ad 100644 --- a/Tests/RunnerCoreTests/LocalNetworkPolicyTests.swift +++ b/Tests/RunnerCoreTests/LocalNetworkPolicyTests.swift @@ -139,4 +139,73 @@ struct LocalNetworkPolicyTests { #expect(candidates.contains("/Library/Preferences/\(LocalNetworkPolicy.domain).plist")) #expect(candidates.contains(NSHomeDirectory() + "/Library/Preferences/\(LocalNetworkPolicy.domain).plist")) } + + // MARK: - Parsing preferences + + /// A preferences file carrying `entries` under both allowlist keys. + private func preferences(_ entries: [String]) throws -> Data { + try PropertyListSerialization.data( + fromPropertyList: [ + LocalNetworkPolicy.ethernetKey: entries, + LocalNetworkPolicy.wifiKey: entries, + "UnrelatedKey": "ignored", + ], + format: .xml, + options: 0) + } + + @Test("both keys are read, and the same entry in both is not counted twice") + func entriesAreUnionedAcrossKeys() throws { + let data = try preferences(["10.0.0.0/8", "192.168.0.0/16"]) + #expect(LocalNetworkPolicy.entries(inPreferences: data) == ["10.0.0.0/8", "192.168.0.0/16"]) + } + + @Test("data that is not a preferences file reads as empty rather than throwing") + func unparseablePreferencesReadEmpty() { + #expect(LocalNetworkPolicy.entries(inPreferences: Data("not a plist".utf8)).isEmpty) + #expect(LocalNetworkPolicy.entries(inPreferences: Data()).isEmpty) + } + + @Test("only files that contribute an entry are named as sources") + func sourcePathsNameOnlyContributingFiles() throws { + let empty = try preferences([]) + let real = try preferences(["192.168.0.0/16"]) + // The same entries again: a second copy of a value already seen adds + // nothing, so its path must not be reported as a source. + let duplicate = try preferences(["192.168.0.0/16"]) + + let status = LocalNetworkPolicy.status(fromContentsOf: [ + ("/first.plist", empty), + ("/second.plist", real), + ("/third.plist", duplicate), + ]) + + #expect(status.allowlist == ["192.168.0.0/16"]) + #expect(status.sourcePaths == ["/second.plist"]) + #expect(status.coversGuestRange) + } + + @Test("an unreadable candidate makes the answer unknown, not unconfigured") + func unreadableCandidatesAreIndeterminate() throws { + // The real case: `sudo defaults write` lands in /var/root, which is + // mode 700, so an ordinary user is refused before it can learn whether + // the file is even there. Reporting that as "no allowlist" is how a + // successful grant gets called a failure. + let blind = LocalNetworkPolicy.status( + fromContentsOf: [], unreadablePaths: ["/var/root/Library/Preferences/x.plist"]) + #expect(!blind.isConfigured) + #expect(blind.isIndeterminate) + + // Nothing found and nothing refused really is unconfigured. + let empty = LocalNetworkPolicy.status(fromContentsOf: []) + #expect(!empty.isConfigured) + #expect(!empty.isIndeterminate) + + // Something found outweighs a refusal elsewhere: the answer is known. + let found = LocalNetworkPolicy.status( + fromContentsOf: [("/a.plist", try preferences(["10.0.0.0/8"]))], + unreadablePaths: ["/var/root/Library/Preferences/x.plist"]) + #expect(found.isConfigured) + #expect(!found.isIndeterminate) + } } diff --git a/docs/setup.md b/docs/setup.md index c8db2bf..043d392 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -461,6 +461,13 @@ gitea-macos-runner permissions status # what is configured, and what to do abo gitea-macos-runner permissions grant # configure it ``` +One asymmetry to know about before reading any of this output: the allowlist is written as root and +lands in `/var/root/Library/Preferences/`, which is mode 700. An ordinary login cannot read it back — +so `permissions status` and `doctor` report `no allowlist visible`, which means *not visible*, not +*not set*. `sudo gitea-macos-runner permissions status` answers definitively. `permissions grant` +does not have this problem: it re-reads the file with the sudo credentials it just used, so it +confirms its own write. + `grant` has two methods. Both are one command; neither needs anything pasted. **`--method allowlist` (the default) is what a CI host wants.** It writes a subnet allowlist — the @@ -478,7 +485,8 @@ configured while silently blocking every guest — and the failure surfaces as ` (errno 65) on the SSH connection, not as a permission error. `192.168.64.0/18` spans `192.168.64.0`–`192.168.127.255`, which is the narrowest entry that covers the drift. `doctor` reports `local network access` as a **pass** once it sees an allowlist covering that span, and as a -**warning** when an allowlist exists but does not. Both keys are documented by Apple in +**warning** when an allowlist exists but does not — but only when it can see it at all, which means +running under `sudo` or straight after a grant. Both keys are documented by Apple in [TN3179](https://developer.apple.com/documentation/technotes/tn3179-understanding-local-network-privacy). **`--method prompt` takes effect immediately, with no reboot**, and is the better choice on a Mac diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 53037c6..4f461b6 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -108,10 +108,14 @@ prompt**: a LaunchAgent that was never granted permission (or was denied) cannot 2. No entry at all: check and fix the permission. ```sh - gitea-macos-runner permissions status - gitea-macos-runner permissions grant # then reboot when it offers + sudo gitea-macos-runner permissions status # sudo: the allowlist lives in root's preferences + gitea-macos-runner permissions grant # then reboot when it offers ``` + Without `sudo`, `status` reports `no allowlist visible` on every host — it is written as root into + `/var/root/Library/Preferences/`, which an ordinary login cannot read. That is not evidence the + grant is missing. `grant` itself does not have the problem: it verifies its own write. + `grant` pre-authorizes the VM subnets and offers to reboot, which is required — the allowlist is read at boot. It grants all of RFC 1918 by default; `--subnet` narrows it, but nothing narrower than `192.168.64.0/18` is safe, because the NAT subnet is chosen at runtime and slides to the next