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

Add doPrivilege blocks for socket connect operations in plugins #22534

Merged
merged 9 commits into from Jan 18, 2017

Conversation

Projects
None yet
3 participants
@tbrooks8
Copy link
Contributor

commented Jan 10, 2017

This is related to #22116. A number of plugins (discovery-azure-classic, discovery-ec2, discovery-gce, repository-azure, repository-gcs, and repository-s3) open socket connections. As SocketPermissions are transitioned out of core, these plugins will require connect permission. This pull request wraps operations that require these permissions in doPrivileged blocks.

@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

With this pull request if I do the following the test package passes:

  • Move connect out of core
  • Put connect individually in all of the modified plugins
  • Silence modules requiring connect (netty, etc) that have not been modified by this PR

This will require a careful review to see if I caught all the places where connections occur in plugins (there were a lot, so it is possible I missed some).

@s1monw
Copy link
Contributor

left a comment

I left a bunch of comments. I think it's hard to really make sure we catch everything by review. We have to rely on the testing and the integration testing with these 3rd party storage backends must be improved. I think we are on that.

import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;

public class SocketAccess {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

can this class be final and abstract then notbody can instantiate it. We also need javadocs on this and all the methods

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

also explain why it's not in core?

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 11, 2017

Author Contributor

I can make it final and abstract and add javadocs.

I did not put it in core because then it seems that core would require the SocketPermissions. That is also why I need versions in each plugin.

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

yeah I can see why I was just asking to put this info on the class so folks see immediately why we duplicate code

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 11, 2017

Author Contributor

Ohhh. Yeah. I'll add docstrings to all of the classes with explanations.

throw new RuntimeException(e);
}
return null;
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

can you leave a comment why this is needed in this particular place?

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 11, 2017

Author Contributor

Yes - I can add a comment.

@@ -194,14 +191,14 @@ static Settings getAvailabilityZoneNodeAttributes(Settings settings, String azMe
try {
url = new URL(azMetadataUrl);
logger.debug("obtaining ec2 [placement/availability-zone] from ec2 meta-data url {}", url);
urlConnection = url.openConnection();

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

I wonder if we can leverage forbidden APIs to restrict the use of openConnection and add a dedicated method that does it with suppressforbidden on it?

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 17, 2017

Contributor

shall we spin off anotehr issue for this?

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 17, 2017

Author Contributor

I thought that would be good. On #22116 I left a comment a few days ago that maybe we should add another task on that issue to mark relevant APIs as forbidden?

urlConnection.setConnectTimeout(2000);
} catch (IOException e) {
// should not happen, we know the url is not malformed, and openConnection does not actually hit network
throw new UncheckedIOException(e);
}

try (InputStream in = urlConnection.getInputStream();
try (InputStream in = SocketAccess.doPrivilegedIOException(urlConnection::getInputStream);

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

same here...

* GCE's http client changes access levels. Additionally remote calls need SocketPermissions for 'connect'.
* This class wraps these operations in {@link AccessController#doPrivileged(PrivilegedAction)} blocks.
*/
public class Access {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

can we make these classes consistent naming wise etc?

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 11, 2017

Author Contributor

I specifically name this one differently, because it is used for other permissions than just socket permissions. The GCE plugin requires additional permissions.

// needed because of problems in gce
  permission java.lang.RuntimePermission "accessDeclaredMembers";
  permission java.lang.RuntimePermission "setFactory";
  permission java.lang.reflect.ReflectPermission "suppressAccessChecks";

And some of the usages of this class are unrelated to socket permissions. These usages preexist my PR.

In the other plugins I named the helper classes SocketAccess because that is the only thing I use them for.

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

ok can you add alls these infos into this class

import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;

public class SocketAccess {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

same here javadocs and naming consistency

}

@FunctionalInterface
public interface VoidOpException {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

maybe name this StorageRunnable ?

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 11, 2017

Author Contributor

Sure

if (blobs.hasNext() == false || deletions.size() == MAX_BATCHING_REQUESTS) {
try {
// Deletions are executed using a batch request
BatchRequest batch = client.batch();

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

are we sure nothing else in this block needs a privileged block? I'd feel better if we kept the existing wrap

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jan 11, 2017

Author Contributor

Yeah now that I take another look - I think client.objects().delete and delete.query need permissions. I can go back to wrapping the whole block.

import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;

public class SocketAccess {

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 11, 2017

Contributor

annoying :) I'd be nice if we could share it somehow

@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

Thanks for the review @s1monw. There are some immediate changes that you pointed out that I can work on implementing.

I responded to your comments with some specifics.

@s1monw

s1monw approved these changes Jan 17, 2017

Copy link
Contributor

left a comment

LGTM I left a question about forbidden APIs

@jasontedor
Copy link
Member

left a comment

I noticed one thing.

private static void checkSpecialPermission() {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(new SpecialPermission());

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 17, 2017

Member

I think we can use a singleton instance?

private static void checkSpecialPermission() {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(new SpecialPermission());

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 17, 2017

Member

Can we use a singleton instance?

private static void checkSpecialPermission() {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(new SpecialPermission());

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 17, 2017

Member

Singleton instance?

private static void checkSpecialPermission() {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(new SpecialPermission());

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 17, 2017

Member

One more opportunity to use a singleton instance?

private static void checkSpecialPermission() {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(new SpecialPermission());

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 17, 2017

Member

Last one? 😄

@jasontedor

This comment has been minimized.

I think this should be final and probably named SPECIAL_PERMISSION like it's a constant. This comment applies throughout.

@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

That was my bad. I just used field extract in IDEA without thinking about making it a constant. I updated with new commit.

@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

@jasontedor I made the changes to make the permission a constant and the build is passing. Let me know if you are now okay with me merging this.

@jasontedor
Copy link
Member

left a comment

LGTM.

@tbrooks8 tbrooks8 merged commit 2766b08 into elastic:master Jan 18, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@tbrooks8 tbrooks8 deleted the tbrooks8:migrate_connect branch Jan 18, 2017

tbrooks8 added a commit to tbrooks8/elasticsearch that referenced this pull request Feb 21, 2017

Wrap getCredentials() in a doPrivileged() block
This commit fixes an issue that was missed in elastic#22534.
`AWSCredentialsProvider.getCredentials()` appears to potentially open a
socket connect. This operation needed to be wrapped in `doPrivileged()`.

This should fix issue elastic#23271.

tbrooks8 added a commit that referenced this pull request Feb 23, 2017

Wrap getCredentials() in a doPrivileged() block (#23297)
This commit fixes an issue that was missed in #22534.
`AWSCredentialsProvider.getCredentials()` appears to potentially open a
socket connect. This operation needed to be wrapped in `doPrivileged()`.

This should fix issue #23271.

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Mar 29, 2018

Use fixture to test repository-s3 plugin
This commit adds a new fixture that emulates a S3 service in order to
improve the existing integration tests. This is very similar to what has
 been made for Google Cloud Storage in elastic#28788, and such tests would have
 helped a lot to catch bugs like elastic#22534.

The AmazonS3Fixture is brittle and only implements the very necessary
stuff for the S3 repository to work, but at least it works and can be
adapted for specific tests needs.

tlrx added a commit that referenced this pull request Apr 3, 2018

Use fixture to test repository-s3 plugin (#29296)
This commit adds a new fixture that emulates a S3 service in order to
improve the existing integration tests. This is very similar to what has
 been made for Google Cloud Storage in #28788, and such tests would 
have helped a lot to catch bugs like #22534.

The AmazonS3Fixture is brittle and only implements the very necessary
stuff for the S3 repository to work, but at least it works and can be
adapted for specific tests needs.

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Apr 6, 2018

Use fixture to test repository-azure plugin
This commit adds a new fixture that emulates an
Azure Storage service in order to improve the
existing integration tests. This is very similar
to what has been made for Google Cloud Storage
in elastic#28788 and for Amazon S3 in elastic#29296, and it
would have helped a lot to catch bugs like elastic#22534.

tlrx added a commit that referenced this pull request Apr 6, 2018

Use fixture to test repository-azure plugin (#29347)
This commit adds a new fixture that emulates an
Azure Storage service in order to improve the
existing integration tests. This is very similar
to what has been made for Google Cloud Storage
in #28788 and for Amazon S3 in #29296, and it
would have helped a lot to catch bugs like #22534.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.