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 permission checks before reading from HDFS stream #26716

Merged
merged 3 commits into from
Sep 21, 2017

Conversation

jbaiera
Copy link
Member

@jbaiera jbaiera commented Sep 19, 2017

Adding permission checks before and limiting the allowed permissions for privileged code blocks that read stream data from HDFS. This is a forward port of some enhancements from #26714, as not all of that PR is relevant for the master branch.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM, but is there a way we can extend/modify the existing hdfs integ tests to cover this case?

@jbaiera
Copy link
Member Author

jbaiera commented Sep 20, 2017

@rjernst Your comment does make me realize that there's a new test case in #26714 that should probably be forward ported as well though. As for the changes in this PR, they are more for observing best practices around guarding privileged blocks from scripting exploits. Not sure if there's an easy way to test that.

MiniHDFS will now start with an existing repository with a single snapshot contained within.
Readonly Repository is created in tests and attempts to list the snapshots within this repo.
Correcting typos...
@rjernst
Copy link
Member

rjernst commented Sep 20, 2017

LGTM, thanks for porting the test.

Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

LGTM

@jbaiera
Copy link
Member Author

jbaiera commented Sep 21, 2017

Failing test looks unrelated to this PR. Dug through the logs and it seems that the tests affected by this change are ok. I'll move ahead with merging it.

@jbaiera jbaiera merged commit c760eec into elastic:master Sep 21, 2017
@jbaiera jbaiera deleted the jbaiera-specialperm-hdfs branch September 21, 2017 15:55
jbaiera added a commit that referenced this pull request Sep 21, 2017
Add checks for special permissions before reading hdfs stream data. Also adds test from 
readonly repository fix. MiniHDFS will now start with an existing repository with a single snapshot 
contained within. Readonly Repository is created in tests and attempts to list the snapshots 
within this repo.
jbaiera added a commit that referenced this pull request Sep 21, 2017
Add checks for special permissions before reading hdfs stream data. Also adds test from 
readonly repository fix. MiniHDFS will now start with an existing repository with a single snapshot 
contained within. Readonly Repository is created in tests and attempts to list the snapshots 
within this repo.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 21, 2017
* master:
  Add permission checks before reading from HDFS stream (elastic#26716)
  muted test
  [Docs] Fixed typo of *configuration* (elastic#25058)
  Add azure storage endpoint suffix elastic#26432 (elastic#26568)
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@clintongormley clintongormley added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository HDFS labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants