Skip to content
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

Remove connect SocketPermissions from core #22797

Merged
merged 13 commits into from
Feb 3, 2017

Conversation

Tim-Brooks
Copy link
Contributor

This permission is relegated to these modules/plugins:

  • transport-netty4 module
  • reindex module
  • repository-url module
  • discovery-azure-classic plugin
  • discovery-ec2 plugin
  • discovery-gce plugin
  • repository-azure plugin
  • repository-gcs plugin
  • repository-hdfs plugin
  • repository-s3 plugin

And for tests:

  • mocksocket jar
  • rest client
  • httpcore-nio jar
  • httpasyncclient jar

This is related to elastic#22116. Core no longer needs `SocketPermission`
`connect`.

This permission is relegated to these modules/plugins:
- transport-netty4 module
- reindex module
- repository-url module
- discovery-azure-classic plugin
- discovery-ec2 plugin
- discovery-gce plugin
- repository-azure plugin
- repository-gcs plugin
- repository-hdfs plugin
- repository-s3 plugin

And for tests:
- mocksocket jar
- rest client
- httpcore-nio jar
- httpasyncclient jar
@Tim-Brooks Tim-Brooks added :Core/Infra/Core Core issues without another label :Distributed Coordination/Network Http and internode communication implementations >enhancement review v6.0.0-alpha1 labels Jan 26, 2017
@Tim-Brooks
Copy link
Contributor Author

A couple notes:

  1. This depends on Add doPrivilege blocks for socket connect ops in repository-hdfs #22793
  2. It's a little messy that I need to give permissions to rest client, httpcore-nio, and httpasyncclient in tests. But this is necessary for the rest integration tests. The doPrivileged block is at the rest client level. So the http libraries (which are called by rest) also need the permissions.
  3. Due to the way tests permission are applied, some issues can be obscured. For ex: reindex needs connect. But even if I did not give it connect, the tests would still pass. This is because it depends on transport-netty4 which has connect. And the way privileges are applied in tests, reindex dependencies get any permissions transport-netty4 when the transport-netty4 security policy is applied in reindex tests. I'm not sure if there is a clear way around this without changing the build significantly.

@Tim-Brooks Tim-Brooks changed the title Officially remove connect Remove connect SocketPermissions from core Jan 26, 2017
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -55,8 +55,8 @@ grant {
// third party code, to safeguard these against unprivileged code like scripts.
permission org.elasticsearch.SpecialPermission;

// Allow connecting to the internet anywhere
permission java.net.SocketPermission "*", "connect,resolve";
// Allow host/ip name service lookups
Copy link
Contributor

Choose a reason for hiding this comment

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

w00t

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left one comment.

permission java.net.SocketPermission "*", "connect";
};

grant codeBase "${codebase.httpcore-nio-4.4.5.jar}" {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add comments to the places where these dependencies are defined that if the version is changed then the version needs to be updated here too? (This comment applies to all of the dependencies for which we have to supply these permissions.)

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left one more comment.

};


grant codeBase "${codebase.rest-6.0.0-alpha1-SNAPSHOT.jar}" {
Copy link
Member

Choose a reason for hiding this comment

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

Does this one deserve a comment too, above the elasticsearch version in version.properties?

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@Tim-Brooks Tim-Brooks merged commit f70188a into elastic:master Feb 3, 2017
@Tim-Brooks Tim-Brooks deleted the officially_remove_connect branch February 3, 2017 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label :Distributed Coordination/Network Http and internode communication implementations >enhancement v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants