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

Wrap rest httpclient with doPrivileged blocks #22603

Merged
merged 2 commits into from
Jan 16, 2017

Conversation

Tim-Brooks
Copy link
Contributor

This is related to #22116. A number of modules (reindex, etc) use the
rest client. The rest client opens connections using the apache http
client. To avoid throwing SecurityException when using the
SecurityManager these operations must be privileged. This is tricky
because connections are opened within the httpclient code on its
reactor thread. The way I confronted this was to wrap the creation
of the client (and creation of reactor thread) in a doPrivileged
block. The new thread inherits the existing security context.

@Tim-Brooks Tim-Brooks added :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch and removed :Core/Infra/REST API REST infrastructure and utilities labels Jan 12, 2017
@s1monw
Copy link
Contributor

s1monw commented Jan 13, 2017

the change looks good but where do we grant the permission to this client? is this coming in a different PR?

@s1monw s1monw self-assigned this Jan 13, 2017
@Tim-Brooks
Copy link
Contributor Author

This client is a dependency of the reindex module. So when I modifying the policy files to officially move connect out of core, reindex will get connect. And rest needs to have the doPrivileged block for that.

@s1monw
Copy link
Contributor

s1monw commented Jan 13, 2017

This client is a dependency of the reindex module. So when I modifying the policy files to officially move connect out of core, reindex will get connect. And rest needs to have the doPrivileged block for that.

fair enough, so nothing else does a connect in the client? are all connections established once the client is created?

@Tim-Brooks
Copy link
Contributor Author

So I have not found any other place where connections occur in the rest client. Even when you call the "synchronous" request API it is still using the async API (reactor thread) and you are just blocking on the future.

But my change here was mostly influenced by how the reindex module was using the client. Other modules (test framework, x-pack) may have different needs. So I'll take a closer look at the client today and see if I am missing anything.

@javanna
Copy link
Member

javanna commented Jan 13, 2017

@tbrooks8 that sounds about right I don't think you are missing anything.

@s1monw
Copy link
Contributor

s1monw commented Jan 16, 2017

@tbrooks8 wanna pulls this in?

@Tim-Brooks Tim-Brooks merged commit 7a8884d into elastic:master Jan 16, 2017
@Tim-Brooks Tim-Brooks deleted the add_privilege_to_rest branch January 16, 2017 15:17
@Tim-Brooks
Copy link
Contributor Author

Yep. I merged it.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 16, 2017
* master: (131 commits)
  Replace EngineClosedException with AlreadyClosedExcpetion (elastic#22631)
  Remove HttpServer and HttpServerAdapter in favor of a simple dispatch method (elastic#22636)
  move ignore parameter support from yaml test client to low level rest client (elastic#22637)
  Fix thread safety of Stempel's token filter factory (elastic#22610)
  Update profile.asciidoc
  Wrap rest httpclient with doPrivileged blocks (elastic#22603)
  Revert "Add a deprecation notice to shadow replicas (elastic#22025)"
  Revert "Don'y use `INDEX_SHARED_FS_ALLOW_RECOVERY_ON_ANY_NODE_SETTING` directly as it triggers (many) deprecation logging"
  Don'y use `INDEX_SHARED_FS_ALLOW_RECOVERY_ON_ANY_NODE_SETTING` directly as it triggers (many) deprecation logging
  Add a deprecation notice to shadow replicas (elastic#22025)
  [Docs] Fix section title in profile.asciidoc
  ProfileResult and CollectorResult should print machine readable timing information (elastic#22561)
  Add replica ops with version conflict to translog
  Replace custom Functional interface in ElasticsearchException with CheckedFunction
  Make RestChannelConsumer extend CheckedConsumer<RestChannel, Exception>
  replace ShardSearchRequest.FilterParser functional interface with CheckedFunction
  replace custom functional interface with CheckedFunction in percolate module
  [TEST] replace SizeFunction with Function<Integer, Integer>
  Expose CheckedFunction
  Expose logs base path
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >enhancement v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants