Wrap getCredentials() in a doPrivileged() block #23297
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Hrm, that is not the only place |
|
I adjusted the PR to reflect @rjernst's review. |
rjernst
reviewed
Feb 22, 2017
| return new AWSCredentialsProvider() { | ||
| @Override | ||
| public AWSCredentials getCredentials() { | ||
| return SocketAccess.doPrivileged(credentials::getCredentials); |
There was a problem hiding this comment.
We only need this for the instance profile credentials. The rest are read and created statically, so definitely do not require socket access.
Tim-Brooks
added a commit
to Tim-Brooks/elasticsearch
that referenced
this pull request
Feb 23, 2017
This is fallout from elastic#23297. That commit wrapped `InstanceProfileCredentialsProvider` to ensure that the `getCredentials` and `refresh` methods had privileged access. However, it looks like there was a test ensuring that `buildCredentials` returned the correct clazz type. This commit adjusts that test to check that the correct wrapper is returned.
Tim-Brooks
added a commit
that referenced
this pull request
Feb 23, 2017
This is fallout from #23297. That commit wrapped `InstanceProfileCredentialsProvider` to ensure that the `getCredentials` and `refresh` methods had privileged access. However, it looks like there was a test ensuring that `buildCredentials` returned the correct clazz type. This commit adjusts that test to check that the correct wrapper is returned.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Feb 24, 2017
* master: (54 commits) Keep the pipeline handler queue small initially Do not create String instances in 'Strings' methods accepting StringBuilder (elastic#22907) Tests: fix AwsS3ServiceImplTests Remove abstract InternalMetricsAggregation class (elastic#23326) Add BulkRequest support to High Level Rest client (elastic#23312) Wrap getCredentials() in a doPrivileged() block (elastic#23297) Respect promises on pipelined responses Align REST specs for HEAD requests Remove unnecessary result sorting in SearchPhaseController (elastic#23321) Fix SamplerAggregatorTests to have stable and predictable docIds Tests: Ensure multi node integ tests wait on first node Relocate a comment in HttpPipeliningHandler Add comments to HttpPipeliningHandler [TEST] Fix incorrect test cluster name in cluster health doc tests Build: Change location in zip of license and notice inclusion for plugins (elastic#23316) Script: Fix value of `ctx._now` to be current epoch time in milliseconds (elastic#23175) Build: Rework integ test setup and shutdown to ensure stop runs when desired (elastic#23304) Handle long overflow when adding paths' totals Don't set local node on cluster state used for node join validation (elastic#23311) Ensure that releasing listener is called ...
clintongormley
added
:Distributed/Snapshot/Restore
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes an issue that was missed in #22534.
AWSCredentialsProvider.getCredentials()appears to potentially open asocket connect. This operation needed to be wrapped in
doPrivileged().This should fix issue #23271.