Skip to content

make it possible to specify permitted network port access at runtime #7151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 6, 2023
Merged

make it possible to specify permitted network port access at runtime #7151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 68 additions & 8 deletions Sources/Commands/PackageTools/PluginCommand.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,72 @@ struct PluginCommand: SwiftCommand {
)
var additionalAllowedWritableDirectories: [String] = []

enum NetworkPermission: String, EnumerableFlag, ExpressibleByArgument {
enum NetworkPermission: EnumerableFlag, ExpressibleByArgument {
static var allCases: [PluginCommand.PluginOptions.NetworkPermission] {
return [.none, .local(ports: []), .all(ports: []), .docker, .unixDomainSocket]
}

case none
case local
case all
case local(ports: [Int])
case all(ports: [Int])
case docker
case unixDomainSocket

init?(argument: String) {
let arg = argument.lowercased()
switch arg {
case "none":
self = .none
case "docker":
self = .docker
case "unixdomainsocket":
self = .unixDomainSocket
default:
if "all" == arg.prefix(3) {
let ports = Self.parsePorts(arg)
self = .all(ports: ports)
} else if "local" == arg.prefix(5) {
let ports = Self.parsePorts(arg)
self = .local(ports: ports)
} else {
return nil
}
}
}

static func parsePorts(_ string: String) -> [Int] {
let parts = string.split(separator: ":")
guard parts.count == 2 else {
return []
}
return parts[1]
.split(separator: ",")
.compactMap{ String($0).spm_chuzzle() }
.compactMap { Int($0) }
}

var remedyDescription: String {
switch self {
case .none:
return "none"
case .local(let ports):
if ports.isEmpty {
return "local"
} else {
return "local:\(ports.map(String.init).joined(separator: ","))"
}
case .all(let ports):
if ports.isEmpty {
return "all"
} else {
return "all:\(ports.map(String.init).joined(separator: ","))"
}
case .docker:
return "docker"
case .unixDomainSocket:
return "unixDomainSocket"
}
}
}

@Option(name: .customLong("allow-network-connections"))
Expand Down Expand Up @@ -211,7 +271,7 @@ struct PluginCommand: SwiftCommand {

reasonString = reason
remedyOption =
"--allow-network-connections \(PluginCommand.PluginOptions.NetworkPermission(scope).defaultValueDescription)"
"--allow-network-connections \(PluginCommand.PluginOptions.NetworkPermission(scope).remedyDescription)"
}

let problem = "Plugin ‘\(plugin.name)’ wants permission to \(permissionString)."
Expand Down Expand Up @@ -377,8 +437,8 @@ extension PluginCommand.PluginOptions.NetworkPermission {
case .unixDomainSocket: self = .unixDomainSocket
case .docker: self = .docker
case .none: self = .none
case .all: self = .all
case .local: self = .local
case .all(let ports): self = .all(ports: ports)
case .local(let ports): self = .local(ports: ports)
}
}
}
Expand All @@ -387,8 +447,8 @@ extension SandboxNetworkPermission {
init(_ permission: PluginCommand.PluginOptions.NetworkPermission) {
switch permission {
case .none: self = .none
case .local: self = .local(ports: [])
case .all: self = .all(ports: [])
case .local(let ports): self = .local(ports: ports)
case .all(let ports): self = .all(ports: ports)
case .docker: self = .docker
case .unixDomainSocket: self = .unixDomainSocket
}
Expand Down
8 changes: 4 additions & 4 deletions Tests/CommandsTests/PackageToolTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1933,12 +1933,12 @@ final class PackageToolTests: CommandsTestCase {
permissionsManifestFragment: "[.allowNetworkConnections(scope: .all(ports: [23, 42, 443, 8080]), reason: \"internet good\")]",
permissionError: "all network connections on ports: 23, 42, 443, 8080",
reason: "internet good",
remedy: ["--allow-network-connections", "all"])
remedy: ["--allow-network-connections", "all:23,42,443,8080"])
Copy link
Contributor

@neonichu neonichu Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem wrong to me? If the manifest specifies a range of ports, that will already be enforced, why would we require users to type these in again?

I think we should instead add new tests which verify if a port-less specification is done in the manifest that the user can manually narrow it to a smaller list via the arguments.

(at least this is how I understand the request in the original radar)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neonichu this test is focused on the "remedy" output, which in this case I think is better than just the generic "all" or "local" when the plugin only requires specific ports? iow when the plugin asks for specific ports, this now says in the "remedy" output that that you can allow it by passing --allow-network-connections all:[ports] instead of the broader --allow-network-connections all so safer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry looks like I misremembered and misread the code. I was under the impression everything was based on the manifest declaration, but everything is actually based on the options, as you say. With that, your changes make sense.

try testCommandPluginNetworkingPermissions(
permissionsManifestFragment: "[.allowNetworkConnections(scope: .all(ports: 1..<4), reason: \"internet good\")]",
permissionError: "all network connections on ports: 1, 2, 3",
reason: "internet good",
remedy: ["--allow-network-connections", "all"])
remedy: ["--allow-network-connections", "all:1,2,3"])

try testCommandPluginNetworkingPermissions(
permissionsManifestFragment: "[.allowNetworkConnections(scope: .local(), reason: \"localhost good\")]",
Expand All @@ -1949,12 +1949,12 @@ final class PackageToolTests: CommandsTestCase {
permissionsManifestFragment: "[.allowNetworkConnections(scope: .local(ports: [23, 42, 443, 8080]), reason: \"localhost good\")]",
permissionError: "local network connections on ports: 23, 42, 443, 8080",
reason: "localhost good",
remedy: ["--allow-network-connections", "local"])
remedy: ["--allow-network-connections", "local:23,42,443,8080"])
try testCommandPluginNetworkingPermissions(
permissionsManifestFragment: "[.allowNetworkConnections(scope: .local(ports: 1..<4), reason: \"localhost good\")]",
permissionError: "local network connections on ports: 1, 2, 3",
reason: "localhost good",
remedy: ["--allow-network-connections", "local"])
remedy: ["--allow-network-connections", "local:1,2,3"])

try testCommandPluginNetworkingPermissions(
permissionsManifestFragment: "[.allowNetworkConnections(scope: .docker, reason: \"docker good\")]",
Expand Down