nsqadmin: X-Forwarded-User based ACL #914

chen-anders · 2017-07-11T06:43:57Z

This PR introduces a small access-control-list mechanism via the X-Forwarded-User HTTP headers.

NSQAdmin can be started with one or more -authorized-user=<user> flags and NSQAdmin will be in a view-only mode for non-admin users. Only authorized admin users can perform administrative actions (e.g. pause/unpause/create/delete topics or channels).

If no -authorized-user=<user> is specified, we default to allowing any user to perform administrative actions.

This approach assumes that there is some authenticating proxy in front of nsqadmin that will strip out incoming X-Forwarded-User headers.

freerobby · 2017-07-11T11:26:13Z

Is there an assumption here that the proxy is performing some authentication before passing on the username? If so there may be a security hole where an end-user passes in a (non-standard) X-Forwarded-User header that a proxy doesn't know to strip out, and now the end-user can perform what the developer thinks are locked down actions.

(It's also possible I'm misunderstanding the auth mechanism use case).

mreiferson · 2017-07-11T14:41:11Z

Yes, and should be documented as such. This approach is only useful if you can trust your proxy and internal network, e.g. oauth2_proxy strips any incoming X-Forwarded-User.

ploxiln · 2017-07-11T15:48:57Z

apps/nsqadmin/main.go

@@ -55,6 +56,7 @@ var (
 func init() {
 	flagSet.Var(&nsqlookupdHTTPAddresses, "lookupd-http-address", "lookupd HTTP address (may be given multiple times)")
 	flagSet.Var(&nsqdHTTPAddresses, "nsqd-http-address", "nsqd HTTP address (may be given multiple times)")
+	flagSet.Var(&allowAuthorizedUsers, "authorized-user", "Authorized Users (for performing admin actions); Any user will have admin privileges if this field is empty.")


maybe admin-user would be a more straightforward name for this

mreiferson · 2017-07-11T15:59:03Z

nsqadmin/http.go

@@ -755,6 +784,21 @@ func (s *httpServer) doConfig(w http.ResponseWriter, req *http.Request, ps httpr
 	return v, nil
 }

+func (s *httpServer) authorizedAdminRequest(req *http.Request) (bool) {


can we call this fun isAuthorizedAdminRequest?

mreiferson · 2017-07-11T15:59:23Z

nsqadmin/http.go

+	adminUsers := s.ctx.nsqadmin.getOpts().AdminUsers
+	if len(adminUsers) == 0 {
+		return true
+	} else {


don't need this else since the above block returns

mreiferson · 2017-07-11T15:59:37Z

nsqadmin/http.go

+	if len(adminUsers) == 0 {
+		return true
+	} else {
+		user := req.Header.Get("X-FORWARDED-USER")


since we're in here, let's make this header user-configurable?

default to X-Forwarded-User

mreiferson · 2017-07-11T16:00:16Z

nsqadmin/static/js/main.js

@@ -14,8 +14,18 @@ require('./lib/ajax_setup');
 require('./lib/handlebars_helpers');

 var start = function() {
-    new AppView();
-    Router.start();
+    $.ajax({


there should be a way for us to inject this data into the template rather than create a new endpoint and have to perform this AJAX request

mreiferson · 2017-07-11T16:17:44Z

nsqadmin/http.go

@@ -100,6 +99,8 @@ func NewHTTPServer(ctx *Context) *httpServer {
 	router.Handle("POST", "/api/topics", http_api.Decorate(s.createTopicChannelHandler, log, http_api.V1))
 	router.Handle("POST", "/api/topics/:topic", http_api.Decorate(s.topicActionHandler, log, http_api.V1))
 	router.Handle("POST", "/api/topics/:topic/:channel", http_api.Decorate(s.channelActionHandler, log, http_api.V1))
+
+


👮 whitespace police, let's drop this change

mreiferson · 2017-07-11T16:17:49Z

nsqadmin/http.go

@@ -73,7 +73,6 @@ func NewHTTPServer(ctx *Context) *httpServer {
 	}

 	router.Handle("GET", "/ping", http_api.Decorate(s.pingHandler, log, http_api.PlainText))
-


👮 whitespace police, let's drop this change

ploxiln · 2017-07-11T18:04:03Z

apps/nsqadmin/main.go

@@ -47,6 +47,8 @@ var (
 	httpClientTLSKey                = flagSet.String("http-client-tls-key", "", "path to key file for the HTTP client")

 	allowConfigFromCIDR = flagSet.String("allow-config-from-cidr", "127.0.0.1/8", "A CIDR from which to allow HTTP requests to the /config endpoint")
+	aclHttpHeader = flagSet.String("acl-http-header", "X-Forwarded-User", "HTTP header to check for authenticated users (default: X-Forwarded-User)")


I don't think you have to note the default in the help string yourself, if you specify the default in the flag declaration as you have done. For example, allow-config-from-cidr is described like so in nsqadmin --help:

-allow-config-from-cidr string A CIDR from which to allow HTTP requests to the /config endpoint (default "127.0.0.1/8")

(We only manually note the default in help strings when we cannot set the default in the flag declaration.)

EDIT: set -> cannot set

ploxiln · 2017-07-11T18:05:28Z

This is looking pretty good to me, thanks for being so responsive to review comments!

mreiferson · 2017-07-22T18:13:09Z

@chen-anders anything else you want to do here? If not, can you squash this down to one commit and prefix it with nsqadmin: ? Thanks!

chen-anders · 2017-07-23T05:27:36Z

@mreiferson Complete.

mreiferson · 2017-07-23T18:39:43Z

Pushed a commit to update bindata and tweak the flag help text.

ploxiln · 2017-07-23T19:24:12Z

👍

mreiferson added the feature label Jul 11, 2017

mreiferson changed the title ~~View-Only NSQAdmin Dashboard via X-Forwarded-User access control~~ nsqadmin: X-Forwarded-User based ACL Jul 11, 2017

ploxiln reviewed Jul 11, 2017

View reviewed changes

chen-anders force-pushed the anders/admin-control-via-xff branch from 27d590f to 09c03a0 Compare July 11, 2017 15:58

mreiferson reviewed Jul 11, 2017

View reviewed changes

chen-anders force-pushed the anders/admin-control-via-xff branch from e6a6f01 to e58afcf Compare July 11, 2017 16:19

ploxiln reviewed Jul 11, 2017

View reviewed changes

chen-anders force-pushed the anders/admin-control-via-xff branch from 02e04f6 to fb5103b Compare July 23, 2017 05:27

chen-anders and others added 2 commits July 23, 2017 11:33

nsqadmin: X-Forwarded-User based ACL

37e7cd0

nsqadmin: update bindata

bc39218

mreiferson force-pushed the anders/admin-control-via-xff branch from fb5103b to bc39218 Compare July 23, 2017 18:39

ploxiln merged commit c3f1bc4 into nsqio:master Jul 23, 2017

chen-anders mentioned this pull request Jul 25, 2017

nsqadmin: Update bindata for admin-user ACL changes #918

Merged

chen-anders deleted the anders/admin-control-via-xff branch March 17, 2019 02:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsqadmin: X-Forwarded-User based ACL #914

nsqadmin: X-Forwarded-User based ACL #914

chen-anders commented Jul 11, 2017 •

edited

Loading

freerobby commented Jul 11, 2017

mreiferson commented Jul 11, 2017

ploxiln Jul 11, 2017

mreiferson Jul 11, 2017

mreiferson Jul 11, 2017

mreiferson Jul 11, 2017

mreiferson Jul 11, 2017

mreiferson Jul 11, 2017

mreiferson Jul 11, 2017

mreiferson Jul 11, 2017

ploxiln Jul 11, 2017 •

edited

Loading

ploxiln commented Jul 11, 2017

mreiferson commented Jul 22, 2017

chen-anders commented Jul 23, 2017

mreiferson commented Jul 23, 2017

ploxiln commented Jul 23, 2017

		@@ -73,7 +73,6 @@ func NewHTTPServer(ctx Context) httpServer {
		}

		router.Handle("GET", "/ping", http_api.Decorate(s.pingHandler, log, http_api.PlainText))

nsqadmin: X-Forwarded-User based ACL #914

nsqadmin: X-Forwarded-User based ACL #914

Conversation

chen-anders commented Jul 11, 2017 • edited Loading

freerobby commented Jul 11, 2017

mreiferson commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

ploxiln commented Jul 11, 2017

mreiferson commented Jul 22, 2017

chen-anders commented Jul 23, 2017

mreiferson commented Jul 23, 2017

ploxiln commented Jul 23, 2017

chen-anders commented Jul 11, 2017 •

edited

Loading

ploxiln Jul 11, 2017 •

edited

Loading