Fix container egress loss after localhost DNS changes (#2256)

Sources/Services/ContainerAPIService/Client/PacketFilter.swift

@@ -20,17 +20,26 @@ import DNSServer
 import Foundation
 import SystemPackage
 
-public struct PacketFilter {
-    public static let anchor = "com.apple.container"
+public struct PacketFilter: Sendable {
+    public static let anchor = "com.apple/container"
     public static let defaultConfigPath = FilePath("/etc/pf.conf")
     public static let defaultAnchorsPath = FilePath("/etc/pf.anchors")
 
+    private static let legacyAnchor = "com.apple.container"
+    private static let anchorFileName = "com.apple.container"
+
     private let configPath: FilePath
     private let anchorsPath: FilePath
+    private let run: @Sendable ([String]) throws -> Int32
 
     public init(configPath: FilePath = Self.defaultConfigPath, anchorsPath: FilePath = Self.defaultAnchorsPath) {
+        self.init(configPath: configPath, anchorsPath: anchorsPath, run: Self.runPFCTL)
+    }
+
+    init(configPath: FilePath, anchorsPath: FilePath, run: @escaping @Sendable ([String]) throws -> Int32) {
         self.configPath = configPath
         self.anchorsPath = anchorsPath
+        self.run = run
     }
 
     public func createRedirectRule(from: IPAddress, to: IPAddress, domain: DNSName) throws {
@@ -40,7 +49,7 @@ public struct PacketFilter {
 
         let fm: FileManager = FileManager.default
 
-        let anchorPath = self.anchorsPath.appending(Self.anchor)
+        let anchorPath = self.anchorsPath.appending(Self.anchorFileName)
 
         let inet: String
         switch from {
@@ -52,9 +61,8 @@ public struct PacketFilter {
         var content = ""
         if fm.fileExists(atPath: anchorPath.string) {
             content = try String(contentsOfFile: anchorPath.string, encoding: .utf8)
-        } else {
-            try addAnchorToConfig()
         }
+        try updateConfig(removing: false)
 
         var lines = content.components(separatedBy: .newlines)
         if !content.contains(redirectRule) {
@@ -71,7 +79,7 @@ public struct PacketFilter {
 
         let fm: FileManager = FileManager.default
 
-        let anchorPath = self.anchorsPath.appending(Self.anchor)
+        let anchorPath = self.anchorsPath.appending(Self.anchorFileName)
 
         let inet: String
         switch from {
@@ -81,6 +89,7 @@ public struct PacketFilter {
         let redirectRule = "rdr \(inet) from any to \(from.description) -> \(to.description) # \(domain.pqdn)"
 
         guard fm.fileExists(atPath: anchorPath.string) else {
+            try updateConfig(removing: true)
             return
         }
 
@@ -93,112 +102,89 @@ public struct PacketFilter {
 
         if removedLines == [""] {
             try fm.removeItem(atPath: anchorPath.string)
-            try removeAnchorFromConfig()
+            try updateConfig(removing: true)
         } else {
             try removedLines.joined(separator: "\n").write(toFile: anchorPath.string, atomically: true, encoding: .utf8)
+            try updateConfig(removing: false)
         }
     }
 
-    private func addAnchorToConfig() throws {
+    private func updateConfig(removing: Bool) throws {
         let fm: FileManager = FileManager.default
 
-        let anchorPath = self.anchorsPath.appending(Self.anchor)
+        let anchorPath = self.anchorsPath.appending(Self.anchorFileName)
 
-        /* PF requires strict ordering of anchors:
-           scrub-anchor, nat-anchor, rdr-anchor, dummynet-anchor, anchor, load anchor
-         */
-        let anchorKeywords = ["scrub-anchor", "nat-anchor", "rdr-anchor", "dummynet-anchor", "anchor", "load anchor"]
+        let anchorKeywords = ["scrub-anchor", "nat-anchor", "rdr-anchor", "dummynet-anchor", "anchor"]
         let loadAnchorText = "load anchor \"\(Self.anchor)\" from \"\(anchorPath.string)\""
+        let ownedLines =
+            anchorKeywords.map { "\($0) \"\(Self.legacyAnchor)\"" } + [
+                "load anchor \"\(Self.legacyAnchor)\" from \"\(anchorPath.string)\"",
+                loadAnchorText,
+            ]
 
         var content: String = ""
-        var lines: [String] = []
         if fm.fileExists(atPath: self.configPath.string) {
             content = try String(contentsOfFile: self.configPath.string, encoding: .utf8)
         }
-        lines = content.components(separatedBy: .newlines)
-
-        for (i, keyword) in anchorKeywords[..<(anchorKeywords.endIndex - 1)].enumerated() {
-            let anchorText = "\(keyword) \"\(Self.anchor)\""
-
-            if content.contains(anchorText) {
-                continue
+        var lines = content.components(separatedBy: .newlines).filter { !ownedLines.contains($0) }
+        if !removing {
+            if lines.last != "" {
+                lines.append("")
             }
-
-            let idx = lines.firstIndex { l in
-                anchorKeywords[i...].map { k in l.starts(with: k) }.contains(true)
-            }
-            lines.insert(anchorText, at: idx ?? lines.endIndex - 1)
-        }
-
-        if !content.contains(loadAnchorText) {
             lines.insert(loadAnchorText, at: lines.endIndex - 1)
         }
 
-        do {
-            try lines.joined(separator: "\n").write(toFile: self.configPath.string, atomically: true, encoding: .utf8)
-        } catch {
-            throw ContainerizationError(.invalidState, message: "failed to write \"\(self.configPath.string)\"")
-        }
-    }
-
-    private func removeAnchorFromConfig() throws {
-        let fm: FileManager = FileManager.default
-
-        guard fm.fileExists(atPath: configPath.string) else {
+        let updatedContent = lines.joined(separator: "\n")
+        guard updatedContent != content else {
             return
         }
 
-        let content = try String(contentsOfFile: configPath.string, encoding: .utf8)
-        let lines = content.components(separatedBy: .newlines)
-
-        let removedLines = lines.filter { l in !l.contains(Self.anchor) }
-
         do {
-            try removedLines.joined(separator: "\n").write(toFile: configPath.string, atomically: true, encoding: .utf8)
+            try updatedContent.write(toFile: configPath.string, atomically: true, encoding: .utf8)
         } catch {
             throw ContainerizationError(.invalidState, message: "failed to write \"\(configPath.string)\"")
         }
     }
 
     public func reinitialize() throws {
-        let null = FileHandle.nullDevice
-
-        let checkProcess = Foundation.Process()
-        var checkStatus: Int32
-        checkProcess.executableURL = URL(fileURLWithPath: "/sbin/pfctl")
-        checkProcess.arguments = ["-n", "-f", configPath.string]
-        checkProcess.standardOutput = null
-        checkProcess.standardError = null
+        let anchorPath = self.anchorsPath.appending(Self.anchorFileName)
+        let path = FileManager.default.fileExists(atPath: anchorPath.string) ? anchorPath.string : "/dev/null"
 
+        let checkStatus: Int32
         do {
-            try checkProcess.run()
+            checkStatus = try run(["-n", "-a", Self.anchor, "-f", path])
         } catch {
             throw ContainerizationError(.internalError, message: "pfctl rule check exec failed: \"\(error)\"")
         }
 
-        checkProcess.waitUntilExit()
-        checkStatus = checkProcess.terminationStatus
         guard checkStatus == 0 else {
-            throw ContainerizationError(.internalError, message: "invalid pf config \"\(configPath.string)\"")
+            throw ContainerizationError(.internalError, message: "invalid pf config \"\(path)\"")
         }
 
-        let reloadProcess = Foundation.Process()
-        var reloadStatus: Int32
-
-        reloadProcess.executableURL = URL(fileURLWithPath: "/sbin/pfctl")
-        reloadProcess.arguments = ["-f", configPath.string]
-        reloadProcess.standardOutput = null
-        reloadProcess.standardError = null
+        try loadRules(anchor: Self.anchor, path: path)
+        try loadRules(anchor: Self.legacyAnchor, path: "/dev/null")
+    }
 
+    private func loadRules(anchor: String, path: String) throws {
+        let reloadStatus: Int32
         do {
-            try reloadProcess.run()
+            reloadStatus = try run(["-a", anchor, "-f", path])
         } catch {
             throw ContainerizationError(.internalError, message: "pfctl reload exec failed: \"\(error)\"")
         }
-        reloadProcess.waitUntilExit()
-        reloadStatus = reloadProcess.terminationStatus
         guard reloadStatus == 0 else {
-            throw ContainerizationError(.invalidState, message: "pfctl -f \"\(configPath.string)\" failed with status \(reloadStatus)")
+            throw ContainerizationError(.invalidState, message: "pfctl -a \"\(anchor)\" -f \"\(path)\" failed with status \(reloadStatus)")
         }
     }
+
+    private static func runPFCTL(_ arguments: [String]) throws -> Int32 {
+        let process = Foundation.Process()
+        process.executableURL = URL(fileURLWithPath: "/sbin/pfctl")
+        process.arguments = arguments
+        process.standardOutput = FileHandle.nullDevice
+        process.standardError = FileHandle.nullDevice
+        try process.run()
+        process.waitUntilExit()
+        return process.terminationStatus
+    }
 }

Tests/ContainerAPIClientTests/PacketFilterTest.swift

@@ -18,6 +18,7 @@ import ContainerizationError
 import ContainerizationExtras
 import DNSServer
 import Foundation
+import Synchronization
 import SystemPackage
 import Testing
 
@@ -25,7 +26,174 @@ import Testing
 
 struct PacketFilterTest {
     @Test
-    func testRedirectRuleUpdate() async throws {
+    func testRedirectRuleLifecycle() throws {
+        try withTemporaryDirectory { tempPath in
+            let configPath = tempPath.appending("pf.conf")
+            let anchorPath = tempPath.appending("com.apple.container")
+            try String(Self.config.dropLast()).write(toFile: configPath.string, atomically: true, encoding: .utf8)
+            let commands = Mutex<[[String]]>([])
+            let pf = PacketFilter(configPath: configPath, anchorsPath: tempPath) { arguments in
+                commands.withLock { $0.append(arguments) }
+                return 0
+            }
+            let from1 = try IPAddress("203.0.113.113")
+            let from2 = try IPAddress("203.0.113.114")
+            let to = try IPAddress("127.0.0.1")
+            let domain1 = try DNSName("aaa.com")
+            let domain2 = try DNSName("bbb.com")
+            let rule1 = "rdr inet from any to \(from1) -> \(to) # \(domain1.pqdn)\n"
+            let rule2 = "rdr inet from any to \(from2) -> \(to) # \(domain2.pqdn)\n"
+            let configured = Self.config + "load anchor \"com.apple/container\" from \"\(anchorPath.string)\"\n"
+            let reloadCommands = [
+                ["-n", "-a", "com.apple/container", "-f", anchorPath.string],
+                ["-a", "com.apple/container", "-f", anchorPath.string],
+                ["-a", "com.apple.container", "-f", "/dev/null"],
+            ]
+
+            try pf.createRedirectRule(from: from1, to: to, domain: domain1)
+            try pf.createRedirectRule(from: from1, to: to, domain: domain1)
+            try pf.createRedirectRule(from: from2, to: to, domain: domain2)
+            try pf.reinitialize()
+
+            #expect(try String(contentsOfFile: anchorPath.string, encoding: .utf8) == rule1 + rule2)
+            #expect(try String(contentsOfFile: configPath.string, encoding: .utf8) == configured)
+            #expect(commands.withLock { $0 } == reloadCommands)
+
+            try pf.removeRedirectRule(from: from1, to: to, domain: domain1)
+            try pf.reinitialize()
+
+            #expect(try String(contentsOfFile: anchorPath.string, encoding: .utf8) == rule2)
+            #expect(try String(contentsOfFile: configPath.string, encoding: .utf8) == configured)
+            #expect(commands.withLock { $0 } == reloadCommands + reloadCommands)
+
+            try pf.removeRedirectRule(from: from2, to: to, domain: domain2)
+            try pf.reinitialize()
+
+            #expect(!FileManager.default.fileExists(atPath: anchorPath.string))
+            #expect(try String(contentsOfFile: configPath.string, encoding: .utf8) == Self.config)
+            #expect(commands.withLock { $0 } == reloadCommands + reloadCommands + Self.emptyReloadCommands)
+        }
+    }
+
+    @Test(arguments: [false, true])
+    func testLegacyRulesMigration(deleting: Bool) throws {
+        try withTemporaryDirectory { tempPath in
+            let configPath = tempPath.appending("pf.conf")
+            let anchorPath = tempPath.appending("com.apple.container")
+            try Self.legacyConfig(anchorPath: anchorPath).write(toFile: configPath.string, atomically: true, encoding: .utf8)
+            let from = try IPAddress("203.0.113.113")
+            let to = try IPAddress("127.0.0.1")
+            let domain = try DNSName("aaa.com")
+            let rule = "rdr inet from any to \(from) -> \(to) # \(domain.pqdn)\n"
+            let retainedRule = "rdr inet from any to 203.0.113.114 -> 127.0.0.1 # bbb.com\n"
+            let originalRules = deleting ? rule + retainedRule : retainedRule
+            try originalRules.write(toFile: anchorPath.string, atomically: true, encoding: .utf8)
+            let commands = Mutex<[[String]]>([])
+            let pf = PacketFilter(configPath: configPath, anchorsPath: tempPath) { arguments in
+                commands.withLock { $0.append(arguments) }
+                return 0
+            }
+
+            if deleting {
+                try pf.removeRedirectRule(from: from, to: to, domain: domain)
+            } else {
+                try pf.createRedirectRule(from: from, to: to, domain: domain)
+            }
+            try pf.reinitialize()
+
+            let expectedRules = deleting ? retainedRule : retainedRule + rule
+            let expectedConfig = Self.config + "load anchor \"com.apple/container\" from \"\(anchorPath.string)\"\n"
+            #expect(try String(contentsOfFile: anchorPath.string, encoding: .utf8) == expectedRules)
+            #expect(try String(contentsOfFile: configPath.string, encoding: .utf8) == expectedConfig)
+            #expect(
+                commands.withLock { $0 } == [
+                    ["-n", "-a", "com.apple/container", "-f", anchorPath.string],
+                    ["-a", "com.apple/container", "-f", anchorPath.string],
+                    ["-a", "com.apple.container", "-f", "/dev/null"],
+                ])
+        }
+    }
+
+    @Test(arguments: [false, true])
+    func testLegacyLastRuleDeletion(missingFile: Bool) throws {
+        try withTemporaryDirectory { tempPath in
+            let configPath = tempPath.appending("pf.conf")
+            let anchorPath = tempPath.appending("com.apple.container")
+            try Self.legacyConfig(anchorPath: anchorPath).write(toFile: configPath.string, atomically: true, encoding: .utf8)
+            let from = try IPAddress("203.0.113.113")
+            let to = try IPAddress("127.0.0.1")
+            let domain = try DNSName("aaa.com")
+            if !missingFile {
+                let rule = "rdr inet from any to \(from) -> \(to) # \(domain.pqdn)\n"
+                try rule.write(toFile: anchorPath.string, atomically: true, encoding: .utf8)
+            }
+            let commands = Mutex<[[String]]>([])
+            let pf = PacketFilter(configPath: configPath, anchorsPath: tempPath) { arguments in
+                commands.withLock { $0.append(arguments) }
+                return 0
+            }
+
+            try pf.removeRedirectRule(from: from, to: to, domain: domain)
+            try pf.reinitialize()
+
+            #expect(!FileManager.default.fileExists(atPath: anchorPath.string))
+            #expect(try String(contentsOfFile: configPath.string, encoding: .utf8) == Self.config)
+            #expect(commands.withLock { $0 } == Self.emptyReloadCommands)
+        }
+    }
+
+    @Test(arguments: [0, 1, 2])
+    func testReinitializeStopsOnFailure(failingCommand: Int) throws {
+        try withTemporaryDirectory { tempPath in
+            let commands = Mutex<[[String]]>([])
+            let pf = PacketFilter(configPath: tempPath.appending("pf.conf"), anchorsPath: tempPath) { arguments in
+                commands.withLock { commands in
+                    commands.append(arguments)
+                    return commands.count - 1 == failingCommand ? 1 : 0
+                }
+            }
+
+            #expect {
+                try pf.reinitialize()
+            } throws: { error in
+                guard let error = error as? ContainerizationError else {
+                    return false
+                }
+                return error.code == (failingCommand == 0 ? .internalError : .invalidState)
+            }
+            #expect(commands.withLock { $0 } == Array(Self.emptyReloadCommands.prefix(failingCommand + 1)))
+        }
+    }
+
+    private static let config = """
+        # Preserve com.apple.container configuration owned by other services.
+        scrub-anchor "com.apple/*"
+        nat-anchor "com.apple/*"
+        rdr-anchor "com.apple/*"
+        rdr-anchor "com.apple.container.other"
+        dummynet-anchor "com.apple/*"
+        anchor "com.apple/*"
+        load anchor "com.apple" from "/etc/pf.anchors/com.apple"
+        # load anchor "com.apple.container" from "/etc/pf.anchors/custom"
+
+        """
+
+    private static let emptyReloadCommands = [
+        ["-n", "-a", "com.apple/container", "-f", "/dev/null"],
+        ["-a", "com.apple/container", "-f", "/dev/null"],
+        ["-a", "com.apple.container", "-f", "/dev/null"],
+    ]
+
+    private static func legacyConfig(anchorPath: FilePath) -> String {
+        var config = Self.config
+        for keyword in ["scrub-anchor", "nat-anchor", "rdr-anchor", "dummynet-anchor", "anchor"] {
+            let wildcard = "\(keyword) \"com.apple/*\""
+            config = config.replacingOccurrences(of: "\n\(wildcard)\n", with: "\n\(keyword) \"com.apple.container\"\n\(wildcard)\n")
+        }
+        return config + "load anchor \"com.apple.container\" from \"\(anchorPath.string)\"\n"
+    }
+
+    private func withTemporaryDirectory(_ body: (FilePath) throws -> Void) throws {
         let fm = FileManager.default
         let tempURL = try fm.url(
             for: .itemReplacementDirectory,
@@ -33,61 +201,7 @@ struct PacketFilterTest {
             appropriateFor: .temporaryDirectory,
             create: true
         )
-        let tempPath = FilePath(tempURL.path)
-        defer { try? FileManager.default.removeItem(at: tempURL) }
-        let configPath = tempPath.appending("pf.conf")
-
-        let pf = PacketFilter(configPath: configPath, anchorsPath: tempPath)
-        let from1 = try! IPAddress("203.0.113.113")
-        let domain1 = try! DNSName("aaa.com")
-        let to = try! IPAddress("127.0.0.1")
-        try pf.createRedirectRule(from: from1, to: to, domain: domain1)
-
-        let anchorPath = tempPath.appending("com.apple.container")
-        var actualAnchorText = try String(contentsOfFile: anchorPath.string, encoding: .utf8)
-        var expectedAnchorTest = """
-            rdr inet from any to \(from1) -> \(to) # \(domain1.pqdn)\n
-            """
-
-        #expect(actualAnchorText == expectedAnchorTest)
-
-        let from2 = try! IPAddress("172.31.72.1")
-        let domain2 = try! DNSName("bbb.com")
-        try pf.createRedirectRule(from: from2, to: to, domain: domain2)
-
-        actualAnchorText = try String(contentsOfFile: anchorPath.string, encoding: .utf8)
-        expectedAnchorTest += """
-            rdr inet from any to \(from2) -> \(to) # \(domain2.pqdn)\n
-            """
-        #expect(actualAnchorText == expectedAnchorTest)
-
-        let actualConfigText = try String(contentsOfFile: configPath.string, encoding: .utf8)
-        let expectedConfigText = try Regex(
-            #"""
-            scrub-anchor "([^"]+)"
-            nat-anchor "([^"]+)"
-            rdr-anchor "([^"]+)"
-            dummynet-anchor "([^"]+)"
-            anchor "([^"]+)"
-            load anchor "([^"]+)" from "[^"]+"
-            """#
-        )
-
-        #expect(actualConfigText.contains(expectedConfigText))
-
-        try pf.removeRedirectRule(from: from1, to: to, domain: domain1)
-        try pf.removeRedirectRule(from: from2, to: to, domain: domain2)
-
-        #expect(!fm.fileExists(atPath: anchorPath.string))
-        let configText = try String(contentsOfFile: configPath.string, encoding: .utf8)
-        #expect(configText == "")
-    }
-
-    @Test
-    func testPacketFilterReinitialize() async throws {
-        let pf = PacketFilter()
-        #expect(throws: ContainerizationError.self) {
-            try pf.reinitialize()
-        }
+        defer { try? fm.removeItem(at: tempURL) }
+        try body(FilePath(tempURL.path))
     }
 }