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.

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

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Dec 1, 2023

motivation: The 'network' plugin permission in SwiftPM allows a plugin to request external network access to all or to a set of ports, but the command line flag does not allow to specify ports forcing users to allow all

changes:

  • support passing ports to local and all --allow-network-connections lfag
  • write parsing logic to extract ports from local:port,port,port and all:port,port,port
  • update remedy description to accuratly specify the usage
  • update tests

rdar://116241125

motivation: The 'network' plugin permission in SwiftPM allows a plugin to request external network access to all or to a set of ports, but the command line flag does not allow to specify ports forcing users to allow all

changes:
* support passing ports to local and all --allow-network-connections lfag
* write parsing logic to extract ports from local:port,port,port and all:port,port,port
* update remedy description to accuratly specify the usage
*  update tests

rdar://116241125
@tomerd
Copy link
Contributor Author

tomerd commented Dec 1, 2023

@swift-ci test

@@ -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

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.

@tomerd tomerd merged commit a5771ec into swiftlang:main Dec 6, 2023
MaxDesiatov added a commit that referenced this pull request Dec 11, 2023
MaxDesiatov pushed a commit that referenced this pull request Dec 11, 2023
…7151)

motivation: The 'network' plugin permission in SwiftPM allows a plugin
to request external network access to all or to a set of ports, but the
command line flag does not allow to specify ports forcing users to allow
all

changes:
* support passing ports to `local` and `all` --allow-network-connections flag
* write parsing logic to extract ports from `local:port,port,port` and
`all:port,port,port`
* update remedy description to accurately specify the usage
* update tests

rdar://116241125
tomerd added a commit to tomerd/swift-package-manager that referenced this pull request Dec 11, 2023
…wiftlang#7151)

motivation: The 'network' plugin permission in SwiftPM allows a plugin
to request external network access to all or to a set of ports, but the
command line flag does not allow to specify ports forcing users to allow
all

changes:
* support passing ports to `local` and `all` --allow-network-connections flag
* write parsing logic to extract ports from `local:port,port,port` and
`all:port,port,port`
* update remedy description to accurately specify the usage
* update tests

rdar://116241125
MaxDesiatov added a commit that referenced this pull request Dec 11, 2023
…untime (#7178)

Cherry-pick of #7151

motivation: The 'network' plugin permission in SwiftPM allows a plugin to request external network access to all or to a set of ports, but the command line flag does not allow to specify ports forcing users to allow all

changes:
* support passing ports to `local` and `all` --allow-network-connections flag
* write parsing logic to extract ports from `local:port,port,port` and `all:port,port,port`
* update remedy description to accurately specify the usage
* update tests

rdar://116241125

Co-authored-by: tomer doron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants