[GitHub] [hbase] joshelser commented on a change in pull request #936: HBASE-17115 Define UI admins via an ACL

2020-01-24 Thread GitBox 
joshelser commented on a change in pull request #936: HBASE-17115 Define UI 
admins via an ACL
URL: https://github.com/apache/hbase/pull/936#discussion_r370885196
 
 

 ##
 File path: 
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java
 ##
 @@ -712,23 +723,24 @@ private void setContextAttributes(ServletContextHandler 
context, Configuration c
* Add default servlets.
*/
   protected void addDefaultServlets(ContextHandlerCollection contexts) throws 
IOException {
+
 // set up default servlets
-addServlet("stacks", "/stacks", StackServlet.class);
-addServlet("logLevel", "/logLevel", LogLevel.Servlet.class);
+addPrivilegedServlet("stacks", "/stacks", StackServlet.class);
+addPrivilegedServlet("logLevel", "/logLevel", LogLevel.Servlet.class);
 // Hadoop3 has moved completely to metrics2, and  dropped support for 
Metrics v1's
 // MetricsServlet (see HADOOP-12504).  We'll using reflection to load if 
against hadoop2.
 // Remove when we drop support for hbase on hadoop2.x.
 try {
-  Class clz = Class.forName("org.apache.hadoop.metrics.MetricsServlet");
-  addServlet("metrics", "/metrics", clz);
+  Class clz = Class.forName("org.apache.hadoop.metrics.MetricsServlet");
+  addPrivilegedServlet("metrics", "/metrics", 
clz.asSubclass(HttpServlet.class));
 } catch (Exception e) {
   // do nothing
 }
-addServlet("jmx", "/jmx", JMXJsonServlet.class);
-addServlet("conf", "/conf", ConfServlet.class);
+addPrivilegedServlet("jmx", "/jmx", JMXJsonServlet.class);
+addUnprivilegedServlet("conf", "/conf", ConfServlet.class);
 
 Review comment:
   > 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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] joshelser commented on a change in pull request #936: HBASE-17115 Define UI admins via an ACL

2020-01-24 Thread GitBox 
joshelser commented on a change in pull request #936: HBASE-17115 Define UI 
admins via an ACL
URL: https://github.com/apache/hbase/pull/936#discussion_r370841975
 
 

 ##
 File path: 
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java
 ##
 @@ -712,23 +723,24 @@ private void setContextAttributes(ServletContextHandler 
context, Configuration c
* Add default servlets.
*/
   protected void addDefaultServlets(ContextHandlerCollection contexts) throws 
IOException {
+
 // set up default servlets
-addServlet("stacks", "/stacks", StackServlet.class);
-addServlet("logLevel", "/logLevel", LogLevel.Servlet.class);
+addPrivilegedServlet("stacks", "/stacks", StackServlet.class);
+addPrivilegedServlet("logLevel", "/logLevel", LogLevel.Servlet.class);
 // Hadoop3 has moved completely to metrics2, and  dropped support for 
Metrics v1's
 // MetricsServlet (see HADOOP-12504).  We'll using reflection to load if 
against hadoop2.
 // Remove when we drop support for hbase on hadoop2.x.
 try {
-  Class clz = Class.forName("org.apache.hadoop.metrics.MetricsServlet");
-  addServlet("metrics", "/metrics", clz);
+  Class clz = Class.forName("org.apache.hadoop.metrics.MetricsServlet");
+  addUnprivilegedServlet("metrics", "/metrics", 
clz.asSubclass(HttpServlet.class));
 
 Review comment:
   Funny. It looks like this is an empty endpoint on Hadoop2. I didn't know we 
exposed this, but I'll mark it as privileged nonetheless :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] joshelser commented on a change in pull request #936: HBASE-17115 Define UI admins via an ACL

2020-01-24 Thread GitBox 
joshelser commented on a change in pull request #936: HBASE-17115 Define UI 
admins via an ACL
URL: https://github.com/apache/hbase/pull/936#discussion_r370818099
 
 

 ##
 File path: 
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java
 ##
 @@ -691,7 +703,6 @@ protected void addDefaultApps(ContextHandlerCollection 
parent,
   }
   logContext.setDisplayName("logs");
   setContextAttributes(logContext, conf);
-  addNoCacheFilter(webAppContext);
 
 Review comment:
   We already do it globally on the WebAppContext. This just re-added the same 
filter.
   
   
https://github.com/apache/hbase/blob/978546b2f247b29dd63bad55b17fdc2e7a31e55d/hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java#L642
   
   Meant to leave a comment stating that but forgot.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] joshelser commented on a change in pull request #936: HBASE-17115 Define UI admins via an ACL

2020-01-17 Thread GitBox 
joshelser commented on a change in pull request #936: HBASE-17115 Define UI 
admins via an ACL
URL: https://github.com/apache/hbase/pull/936#discussion_r368071622
 
 

 ##
 File path: 
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java
 ##
 @@ -770,30 +777,28 @@ public void addJerseyResourcePackage(final String 
packageName,
   }
 
   /**
-   * Add a servlet in the server.
+   * Adds a servlet in the server that any user can access.
* @param name The name of the servlet (can be passed as null)
* @param pathSpec The path spec for the servlet
* @param clazz The servlet class
*/
-  public void addServlet(String name, String pathSpec,
+  public void addUnprivilegedServlet(String name, String pathSpec,
 
 Review comment:
   > Adds a servlet in the server that any user can access.
   
   I expanded this to the Javadoc already, but I suppose I can expand some more.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [hbase] joshelser commented on a change in pull request #936: HBASE-17115 Define UI admins via an ACL

2020-01-17 Thread GitBox 
joshelser commented on a change in pull request #936: HBASE-17115 Define UI 
admins via an ACL
URL: https://github.com/apache/hbase/pull/936#discussion_r368068359
 
 

 ##
 File path: 
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java
 ##
 @@ -131,6 +131,10 @@
   "signature.secret.file";
   public static final String HTTP_AUTHENTICATION_SIGNATURE_SECRET_FILE_KEY =
   HTTP_AUTHENTICATION_PREFIX + 
HTTP_AUTHENTICATION_SIGNATURE_SECRET_FILE_SUFFIX;
+  public static final String HTTP_SPNEGO_AUTHENTICATION_ADMIN_USERS_KEY =
+  HTTP_SPNEGO_AUTHENTICATION_PREFIX + "admin.users";
 
 Review comment:
   Yes, likely.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services