Adds the ability to blacklist specific HTTP endpoints. #3252
Conversation
agent/http.go
Outdated
| @@ -194,6 +201,12 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque | |||
| fmt.Fprint(resp, errMsg) | |||
| } | |||
|
|
|||
| if s.blacklist.IsDisallowed(req.URL.Path) { | |||
| err := fmt.Errorf("Permission denied, endpoint is disabled") | |||
| handleErr(err) | |||
There was a problem hiding this comment.
This relying on a string match to make it return an appropriate status code like 403 is weird and seems fragile. Would be nice to make some predefined Err500, Err404, Err403 etc.
There was a problem hiding this comment.
Yeah this is gross - I'll factor this out to make it more explicit. The other code is a little fragile but is baked in a few different places (and complicated by the fact the the errors go over the RPC interface, which launders their types into a generic error), so we can tackle that in another change.
| @@ -757,6 +757,16 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass | |||
| <br><br> | |||
| The following sub-keys are available: | |||
|
|
|||
| * <a name="disable_endpoints"></a><a href="#disable_endpoints">`disable_endpoints`</a> | |||
There was a problem hiding this comment.
This would also disable any consul commands that use the underlying api, right? Is it worth calling out here?
There was a problem hiding this comment.
Yep - I'll add a note about that.
agent/blacklist.go
Outdated
|
|
||
| // IsDisallowed will return true if the given path is included among any of the | ||
| // disallowed prefixes. | ||
| func (b *Blacklist) IsDisallowed(path string) bool { |
agent/blacklist.go
Outdated
| // IsDisallowed will return true if the given path is included among any of the | ||
| // disallowed prefixes. | ||
| func (b *Blacklist) IsDisallowed(path string) bool { | ||
| _, _, disallowed := b.tree.LongestPrefix(path) |
agent/blacklist_test.go
Outdated
| tests := []struct { | ||
| desc string | ||
| prefixes []string | ||
| paths []pathCase |
agent/config.go
Outdated
| @@ -131,6 +131,10 @@ type DNSConfig struct { | |||
|
|
|||
| // HTTPConfig is used to fine tune the Http sub-system. | |||
| type HTTPConfig struct { | |||
| // DisableEndpoints is a list of endpoint prefixes to disable in the | |||
| // HTTP API. Any requests to these will get a 403 response. | |||
| DisableEndpoints []string `mapstructure:"disable_endpoints"` | |||
There was a problem hiding this comment.
s/DisableEndpoints/BlockEndpoints/g
I'm inclined to just name this Blacklist but that may conflict with other blacklists in the future, e.g. network ranges, user agents, ...
agent/http.go
Outdated
| @@ -194,6 +201,12 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque | |||
| fmt.Fprint(resp, errMsg) | |||
| } | |||
|
|
|||
| if s.blacklist.IsDisallowed(req.URL.Path) { | |||
| err := fmt.Errorf("Permission denied, endpoint is disabled") | |||
There was a problem hiding this comment.
We should probably type these errors instead of depending on magic words but that is probably a larger change.
agent/http_test.go
Outdated
| } | ||
|
|
||
| // Try a banned endpoint, which should get a 403. | ||
| { |
There was a problem hiding this comment.
use t.Run subtests instead of block and comment
agent/blacklist_test.go
Outdated
| "/v1/agent/self", | ||
| }, | ||
| []pathCase{ | ||
| {"/", false}, |
There was a problem hiding this comment.
I'd use generic endpoint names like /a, /b to make it clear what you're testing. You're covering /a, /aa and /a/b for the /a blacklist but that isn't obvious. Also, with the two entries you didn't cover /v1/acl
No description provided.