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

HBASE-17115 Define UI admins via an ACL #936

Closed
wants to merge 10 commits into from

Conversation

joshelser
Copy link
Member

The Hadoop AccessControlList allows us to specify admins of the webUI
via a list of users and/or groups. Admins of the WebUI can mutate the
system, potentially seeing sensitive data or modifying the system.

hbase.security.authentication.spnego.admin.users is a comma-separated
list of users who are admins.
hbase.security.authentication.spnego.admin.groups is a comma-separated
list of groups whose membership are admins. Either of these
configuration properties may also contain an asterisk (*) which denotes
"anything" (any user or group).

To maintain previous semantics, the UI defaults to accepting any user as an
admin. Previously, when a user was denied from some endpoint that was
designated for admins, they received an HTTP/401. In this case, it is
more correct to return HTTP/403 as they were correctly authenticated,
but they were disallowed from fetching the given resource.

The test is based off of work by Nihal Jain in HBASE-20472.

Co-authored-by: Nihal Jain nihaljain.cs@gmail.com

@joshelser
Copy link
Member Author

Testing I've done:

  • New unit tests
  • Explicit admin user defined in configuration (hbase.security.authentication.spnego.admin.users)
  • curl with both the admin and a non-admin
  • The above along with hbase.master.ui.readonly=true as well.

If admins are set, that will limit who can interact with sensitive endpoints. Setting readonly=true will further restrict the system and disallow anyone (including admins) from modifying hbase via the UI.

I think this maintains all of the previous semantics folks would expect, while letting the security-conscious lock things down.

Copy link
Contributor

@busbey busbey left a comment

Choose a reason for hiding this comment

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

I don't see the logs, debug, or zk dump pages listed. Those should be limited to admins too?

@Apache-HBase

This comment has been minimized.

@joshelser
Copy link
Member Author

I don't see the logs, ... pages listed. Those should be limited to admins too?

ServletContextHandler logContext = new ServletContextHandler(parent, "/logs");
logContext.addServlet(AdminAuthorizedServlet.class, "/*");
logContext.setResourceBase(logDir);

The logs servlet already had the AdminAuthorizedServlet in the chain; it was just ineffective because we weren't configuring/setting an ADMIN_ACL.

I don't see the ... debug, ... pages listed. Those should be limited to admins too?

Good catch! Was missing this, will fix.

I don't see the ... zk dump pages listed. Those should be limited to admins too?

I'm not finding this, @busbey . I need to go looking to see if this is a branch-specific feature.

@Apache-HBase

This comment has been minimized.

@joshelser
Copy link
Member Author

Turns out I'm still missing the critical bits for everything besides the /logs/ http endpoint. Looking in hadoop to see how they do this because I have no idea how the existing AdminAuthorizedServlet is supposed to be used (I would have thought it should be a Filter..)

@Apache-HBase

This comment has been minimized.

@joshelser
Copy link
Member Author

I just need to write a UT to cover this stuff. Just realized I hadn't done that.

@joshelser
Copy link
Member Author

Alright, I think the last commit does this right now.

There was a problem in my previous patches in that the API I added -- trying to have privileged and unprivileged methods for adding a servlet to the HttpServer were half-baked. I have this working now so that we don't have to be injecting authz logic into every servlet we write. Just, when we add it to the HttpServer/InfoServer, we call the appropriate method to restrict (or not) access to admins only.

I added some more unit tests which show that both the contexts (e.g. /logs/) and the servlets (e.g. /dump) both work for admins and reject it for non-admins. There was some trickiness in cleaning this up: we have a bit of cruft in the HttpServer logic.

  • We need to add our "default apps"
  • Then add the filters we want to apply globally (e.g. spnego, security headers, etc)
  • Then we add all the servlets, optionally adding in the new AdminAuthorizedFilter when we register that filter
    • That new Filter is the piece which, added at the end of the filter chain (meaning, after all of our other filters we expect to run all the time), will stop callers from accessing that protected servlet if you're not an admin (HTTP/403).

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 28s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 4 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 0m 33s Maven dependency ordering for branch
+1 💚 mvninstall 5m 51s master passed
+1 💚 compile 3m 20s master passed
+1 💚 checkstyle 2m 32s master passed
+0 🆗 refguide 5m 25s branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 4s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 3m 41s master passed
+0 🆗 spotbugs 4m 16s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 20m 17s master passed
-0 ⚠️ patch 4m 37s Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 14s Maven dependency ordering for patch
+1 💚 mvninstall 5m 26s the patch passed
+1 💚 compile 3m 17s the patch passed
+1 💚 javac 3m 17s the patch passed
-1 ❌ checkstyle 2m 31s root: The patch generated 2 new + 144 unchanged - 0 fixed = 146 total (was 144)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+0 🆗 refguide 5m 29s patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 3s patch has no errors when building our shaded downstream artifacts.
+1 💚 hadoopcheck 17m 13s Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
-1 ❌ javadoc 0m 15s hbase-http generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
-1 ❌ javadoc 2m 52s root generated 1 new + 3 unchanged - 0 fixed = 4 total (was 3)
+1 💚 findbugs 20m 44s the patch passed
_ Other Tests _
-1 ❌ unit 172m 48s root in the patch failed.
+1 💚 asflicense 1m 20s The patch does not generate ASF License warnings.
292m 22s
Reason Tests
Failed junit tests hadoop.hbase.quotas.TestClusterScopeQuotaThrottle
Subsystem Report/Notes
Docker Client=19.03.4 Server=19.03.4 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/Dockerfile
GITHUB PR #936
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile refguide
uname Linux f7863de1492e 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-936/out/precommit/personality/provided.sh
git revision master / eda5df7
Default Java 1.8.0_181
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/branch-site/book.html
checkstyle https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/diff-checkstyle-root.txt
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/patch-site/book.html
javadoc https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/diff-javadoc-javadoc-hbase-http.txt
javadoc https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/diff-javadoc-javadoc-root.txt
unit https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/artifact/out/patch-unit-root.txt
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/testReport/
Max. process+thread count 4961 (vs. ulimit of 10000)
modules C: hbase-http hbase-server . U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/4/console
versions git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@joshelser
Copy link
Member Author

Poking around at a local install -- looks like the DumpServlet is now requiring admins even when auth'n for the UI is off :)

Copy link
Contributor

@busbey busbey left a comment

Choose a reason for hiding this comment

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

excellent docs addition!

joshelser and others added 9 commits January 24, 2020 15:01
The Hadoop AccessControlList allows us to specify admins of the webUI
via a list of users and/or groups. Admins of the WebUI can mutate the
system, potentially seeing sensitive data or modifying the system.

hbase.security.authentication.spnego.admin.users is a comma-separated
list of users who are admins.
hbase.security.authentication.spnego.admin.groups is a comma-separated
list of groups whose membership are admins. Either of these
configuration properties may also contain an asterisk (*) which denotes
"anything" (any user or group).

To maintain previous semantics, the UI defaults to accepting any user as an
admin. Previously, when a user was denied from some endpoint that was
designated for admins, they received an HTTP/401. In this case, it is
more correct to return HTTP/403 as they were correctly authenticated,
but they were disallowed from fetching the given resource.

The test is based off of work by Nihal Jain in HBASE-20472.

Co-authored-by: Nihal Jain <nihaljain.cs@gmail.com>
* Expand on javadoc for add[Un]PrivilegedServlet method(s)
* Expand on the securing the webUI section of the book
@joshelser
Copy link
Member Author

Rebase'd on top of master and made /metrics and /jmx privileged.

addServlet("jmx", "/jmx", JMXJsonServlet.class);
addServlet("conf", "/conf", ConfServlet.class);
addPrivilegedServlet("jmx", "/jmx", JMXJsonServlet.class);
addUnprivilegedServlet("conf", "/conf", ConfServlet.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

So /conf can also have secrets potentially? Assuming this picks up all the core-site, hdfs-site confs as well. it could technically have things not secured via hadoop credential stores.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, but in a "properly" secured system they'd all be behind Hadoop Credential Providers right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we add the conf endpoint the privileged list, we should make it optional to keep it unprivileged. I know some systems that rely on reading the conf end point to automate configuring clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure but the concerns about jmx/metrics and host/ports/usernames applies to conf.

Copy link
Member Author

Choose a reason for hiding this comment

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

host/ports/usernames applies to conf

There's a difference between well-known daemons (what should be in conf/) and HBase clients. We shouldn't be advertising to the world who is talking to HBase.

in a "properly" secured system they'd all be behind Hadoop Credential Providers right?

I agree with Busbey here -- I don't think there's a case where we should expect to have unprotected secrets in the configuration. However, I will concede that folks may do this anyways. I'll add a default-false option to protect the conf endpoint, just in case.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 27s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 4 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 0m 33s Maven dependency ordering for branch
+1 💚 mvninstall 5m 43s master passed
+1 💚 compile 3m 19s master passed
+1 💚 checkstyle 2m 34s master passed
+0 🆗 refguide 5m 20s branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 4s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 3m 38s master passed
+0 🆗 spotbugs 3m 54s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 19m 45s master passed
-0 ⚠️ patch 4m 15s Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 14s Maven dependency ordering for patch
+1 💚 mvninstall 5m 24s the patch passed
+1 💚 compile 3m 20s the patch passed
+1 💚 javac 3m 20s the patch passed
+1 💚 checkstyle 2m 33s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+0 🆗 refguide 5m 23s patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 3s patch has no errors when building our shaded downstream artifacts.
+1 💚 hadoopcheck 17m 14s Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚 javadoc 3m 42s the patch passed
+1 💚 findbugs 20m 40s the patch passed
_ Other Tests _
+1 💚 unit 237m 36s root in the patch passed.
+1 💚 asflicense 1m 43s The patch does not generate ASF License warnings.
356m 41s
Subsystem Report/Notes
Docker Client=19.03.4 Server=19.03.4 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/5/artifact/out/Dockerfile
GITHUB PR #936
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile refguide
uname Linux bc35d234db37 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-936/out/precommit/personality/provided.sh
git revision master / eda5df7
Default Java 1.8.0_181
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/5/artifact/out/branch-site/book.html
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/5/artifact/out/patch-site/book.html
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/5/testReport/
Max. process+thread count 5273 (vs. ulimit of 10000)
modules C: hbase-http hbase-server . U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/5/console
versions git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 29s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 4 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 0m 35s Maven dependency ordering for branch
+1 💚 mvninstall 5m 42s master passed
+1 💚 compile 3m 19s master passed
+1 💚 checkstyle 2m 28s master passed
+0 🆗 refguide 5m 29s branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 5s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 3m 40s master passed
+0 🆗 spotbugs 4m 10s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 20m 6s master passed
-0 ⚠️ patch 4m 31s Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 18s Maven dependency ordering for patch
+1 💚 mvninstall 5m 28s the patch passed
+1 💚 compile 3m 21s the patch passed
+1 💚 javac 3m 21s the patch passed
+1 💚 checkstyle 2m 31s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+0 🆗 refguide 5m 23s patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 5s patch has no errors when building our shaded downstream artifacts.
+1 💚 hadoopcheck 17m 28s Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚 javadoc 3m 51s the patch passed
+1 💚 findbugs 20m 51s the patch passed
_ Other Tests _
+1 💚 unit 256m 13s root in the patch passed.
+1 💚 asflicense 1m 45s The patch does not generate ASF License warnings.
376m 50s
Subsystem Report/Notes
Docker Client=19.03.4 Server=19.03.4 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/6/artifact/out/Dockerfile
GITHUB PR #936
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile refguide
uname Linux 754cf6eef5cc 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-936/out/precommit/personality/provided.sh
git revision master / 753cc99
Default Java 1.8.0_181
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/6/artifact/out/branch-site/book.html
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/6/artifact/out/patch-site/book.html
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/6/testReport/
Max. process+thread count 5368 (vs. ulimit of 10000)
modules C: hbase-http hbase-server . U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/6/console
versions git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 6s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 4 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 0m 34s Maven dependency ordering for branch
+1 💚 mvninstall 5m 52s master passed
+1 💚 compile 3m 24s master passed
+1 💚 checkstyle 2m 34s master passed
+0 🆗 refguide 5m 29s branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 6s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 3m 44s master passed
+0 🆗 spotbugs 4m 10s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 20m 20s master passed
-0 ⚠️ patch 4m 32s Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 14s Maven dependency ordering for patch
+1 💚 mvninstall 5m 33s the patch passed
+1 💚 compile 3m 17s the patch passed
+1 💚 javac 3m 17s the patch passed
+1 💚 checkstyle 2m 34s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+0 🆗 refguide 5m 26s patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 5m 7s patch has no errors when building our shaded downstream artifacts.
+1 💚 hadoopcheck 17m 29s Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚 javadoc 3m 46s the patch passed
+1 💚 findbugs 20m 47s the patch passed
_ Other Tests _
-1 ❌ unit 211m 20s root in the patch failed.
+1 💚 asflicense 1m 34s The patch does not generate ASF License warnings.
332m 55s
Reason Tests
Failed junit tests hadoop.hbase.snapshot.TestExportSnapshotNoCluster
Subsystem Report/Notes
Docker Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/7/artifact/out/Dockerfile
GITHUB PR #936
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile refguide
uname Linux 7d81d2b4a477 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-936/out/precommit/personality/provided.sh
git revision master / 1690414
Default Java 1.8.0_181
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/7/artifact/out/branch-site/book.html
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/7/artifact/out/patch-site/book.html
unit https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/7/artifact/out/patch-unit-root.txt
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/7/testReport/
Max. process+thread count 5139 (vs. ulimit of 10000)
modules C: hbase-http hbase-server . U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-936/7/console
versions git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@joshelser
Copy link
Member Author

Any more requests on this?

@joshelser
Copy link
Member Author

HBASE-20950 isn't contained on branch-2.1 which means I have to fix up TestInfoServersAcl.

@joshelser
Copy link
Member Author

Submitted to all 2.x branches and master, after getting HBASE-20950 also backported to branch-2.1.

1.x needs some help with precommit before this change can land there.

@joshelser joshelser closed this Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants