Restrict metadata routes #110

samb1729 · 2018-07-04T16:52:53Z

This set of changes adds a flag to kiam agent which allows the user to provide a regular expression that a request path must match to be proxied to the metadata service. The parameter doesn't affect any requests that are handled by kiam directly.

By default its value is set to .* which means all requests are allowed.

Resolves #60

pingles · 2018-07-04T17:06:35Z

Thanks for this @Sambooo. If you don't mind let's take a look at what we can do to update the testing to bring in line with the (unfortunately mammoth, sorry) changes I made in #109.

samb1729 · 2018-07-05T09:15:12Z

update the testing to bring in line with ... #109.

Sure, let's gets that PR merged and I'll rebase

samb1729 · 2018-07-06T16:04:25Z

I've just pushed having rebased onto api-v3. Updated tests still to come.

codecov · 2018-07-06T16:07:46Z

Codecov Report

Merging #110 into api-v3 will increase coverage by 1.93%.
The diff coverage is 58%.

@@            Coverage Diff             @@
##           api-v3     #110      +/-   ##
==========================================
+ Coverage   37.79%   39.73%   +1.93%     
==========================================
  Files          21       22       +1     
  Lines         799      828      +29     
==========================================
+ Hits          302      329      +27     
- Misses        469      471       +2     
  Partials       28       28

Impacted Files	Coverage Δ
pkg/aws/metadata/handler_health.go	`0% <0%> (ø)`	⬆️
pkg/aws/metadata/server.go	`5.76% <0%> (+0.31%)`	⬆️
pkg/aws/metadata/handler_credentials.go	`85% <100%> (+1.66%)`	⬆️
pkg/aws/metadata/handler_role_name.go	`90.9% <100%> (+0.9%)`	⬆️
pkg/aws/metadata/handler_proxy.go	`100% <100%> (ø)`

ewbankkit · 2018-07-11T19:31:23Z

cmd/kiam/agent.go

@@ -45,6 +47,7 @@ func (cmd *agentCommand) Bind(parser parser) {

 	parser.Flag("port", "HTTP port").Default("3100").IntVar(&cmd.port)
 	parser.Flag("allow-ip-query", "Allow client IP to be specified with ?ip. Development use only.").Default("false").BoolVar(&cmd.allowIPQuery)
+	parser.Flag("allowed-routes", "Proxy routes matching this regular expression").Default(".*").RegexpVar(&cmd.allowedRoutes)


Should the flag be plural - routes - if we only allow it to be specified once?

it's a regular expression which can match many routes

Perhaps then we could rename to specify that it's a regexp? So, change it to:

kiam server --whitelist-route-regexp=.*

Thoughts @Sambooo and @ewbankkit? Agree about the plural name though- we don't support multiple regexes so it'd be good to name it singularly to avoid confusion.

Naming is the hardest problem 😄.
No particular opinion on this one but I think some documented examples would be good.

ewbankkit · 2018-07-11T19:38:13Z

pkg/aws/metadata/handler_proxy.go

+		p.reverseProxy.ServeHTTP(writer, r)
+		return writer.status, nil
+	} else {
+		return http.StatusForbidden, fmt.Errorf("request blocked by allowedRoutes pattern %q: %s", p.allowedRoutes, route)


Is 403 the correct HTTP status code to return?
From RFC 7231:

An origin server that wishes to "hide" the current existence of a
forbidden target resource MAY instead respond with a status code of
404 (Not Found).

the request is not allowed by (does not match) the allowed-routes regular expression, so the request is forbidden by the configuration. which status code should be returned in this case?

I don't think there's a "correct" answer. Either would be valid.

Given that RFC maybe we should favour 404?

We should still log that it was blocked via a warning because of the path regexp (so that its easy for operators to record/debug) but 404 feels like it leaks less information to clients.

@uswitch/cloud any other thoughts? Feels pretty close to a coin toss though 😄

I have no issues with changing the status code here since I went with my gut and MAY means it's up to us. Flip that coin.

At the moment the flag parsing library we use will tell a user that this parameter is a regular expression, but it's possible that a migration to cobra or similar will make that help string disappear or change to 'string'. Since we don't test the output of `--help`, instead make the name reflect the type.

pingles · 2018-07-12T16:39:29Z

Awesome. Last piece of the puzzle is that we want to require metadata API routes to be whitelisted, rather than permitting all by default (this was originally raised in #60).

I think once that's changed we're good to merge!

pingles

Thanks for all this @Sambooo. Left a few comments for some final changes.

pingles · 2018-07-13T11:41:17Z

pkg/aws/metadata/handler_credentials.go

-	clientIP clientIPFunc
-	client   server.Client
+	client     server.Client
+	ipResolver clientIPFunc


I'd rename ipResolver to getClientIP

pingles · 2018-07-13T11:45:27Z

pkg/aws/metadata/handler_proxy_test.go

+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+	defer cancel()
+
+	allowedRoutes := regexp.MustCompile("^$")


If we're not going to move this statement inside newProxyHandler I'd like the expression stored in a const for something like WhitelistDefaultBlockEverything so that it can be referenced in both the package and test.

pingles · 2018-07-13T11:45:43Z

pkg/aws/metadata/handler_proxy_test.go

+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+	defer cancel()
+
+	allowedRoutes := regexp.MustCompile("foo.*")


Passing in a non-default regexp is fine though

pingles · 2018-07-13T11:46:08Z

pkg/aws/metadata/handler_role_name.go

-	client   server.Client
-	clientIP clientIPFunc
+	client     server.Client
+	ipResolver clientIPFunc


As before, I'd prefer this to be named getClientIP.

pingles · 2018-07-13T11:47:25Z

pkg/aws/metadata/server.go

 }

-func NewConfig(port int) *ServerConfig {
+func NewConfig(port int, allowIPQuery bool, whitelistRouteRegexp *regexp.Regexp) *ServerConfig {


I'd rather we didn't expand NewConfig to include all options.

To be honest, I'd prefer it didn't specify a port but set good defaults for everything and then clients set individual fields.

Otherwise, as the struct grows we're just adding more and more params to the function which isn't nice.

Maybe we should rename the func to DefaultConfig too.

* Parameterise allowed metadata API routes * Add filtered metadata routes in the same way as other handlers * Add proxyHandler tests * Add whitelist-route-regexp flag * Block all non-kiam metadata api requests by default * Extract handler constructors to put default under test * Rename client IP func to getClientIP * Embed agent server options into CLI flags

Restrict metadata routes (#110) * Parameterise allowed metadata API routes * Add filtered metadata routes in the same way as other handlers * Add proxyHandler tests * Add whitelist-route-regexp flag * Block all non-kiam metadata api requests by default * Extract handler constructors to put default under test * Rename client IP func to getClientIP * Embed agent server options into CLI flags

* merging cherry-picked from missing v3 commit Restrict metadata routes (#110) * Parameterise allowed metadata API routes * Add filtered metadata routes in the same way as other handlers * Add proxyHandler tests * Add whitelist-route-regexp flag * Block all non-kiam metadata api requests by default * Extract handler constructors to put default under test * Rename client IP func to getClientIP * Embed agent server options into CLI flags * targets in Makefile to specify bin rather than phony

samb1729 requested a review from pingles July 4, 2018 16:52

pingles mentioned this pull request Jul 5, 2018

Block Access to Other Metadata #60

Closed

samb1729 force-pushed the restrict-metadata-routes branch from 5f30658 to 93794bd Compare July 6, 2018 16:03

samb1729 changed the base branch from master to api-v3 July 6, 2018 16:03

samb1729 added 2 commits July 9, 2018 15:21

Parameterise allowed metadata API routes

f2e4e27

Add allow-routes flag to kiam agent

6ae2c26

samb1729 force-pushed the restrict-metadata-routes branch from 93794bd to 6ae2c26 Compare July 9, 2018 15:43

Add filtered metadata routes in the same way as other handlers

bbb1655

ewbankkit reviewed Jul 11, 2018

View reviewed changes

samb1729 added 2 commits July 12, 2018 17:11

Add proxyHandler tests

e451a56

samb1729 added 2 commits July 12, 2018 17:57

Block all non-kiam requests by default

68201b3

Extract handler constructors to put default under test

3ad093a

pingles suggested changes Jul 13, 2018

View reviewed changes

samb1729 added 2 commits July 13, 2018 14:40

Rename ipResolver to getClientIP

f922737

Embed agent server options into CLI flags

0a06168

pingles approved these changes Jul 13, 2018

View reviewed changes

pingles merged commit afdb221 into api-v3 Jul 13, 2018

pingles deleted the restrict-metadata-routes branch July 13, 2018 14:37

pingles mentioned this pull request Jul 13, 2018

v2 to v3 Upgrade Documentation #117

Closed

pingles mentioned this pull request Aug 2, 2018

Add option to disable user-data endpoint #63

Closed

mbarrien pushed a commit to mbarrien/helm-kiam that referenced this pull request Sep 15, 2018

Restrict metadata routes; uswitch/kiam#110.

05b8300

pingles mentioned this pull request Oct 25, 2018

Missing v3 :-( #167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict metadata routes #110

Restrict metadata routes #110

samb1729 commented Jul 4, 2018

pingles commented Jul 4, 2018

samb1729 commented Jul 5, 2018

samb1729 commented Jul 6, 2018

codecov bot commented Jul 6, 2018 •

edited

Loading

ewbankkit Jul 11, 2018

samb1729 Jul 11, 2018

pingles Jul 12, 2018

ewbankkit Jul 12, 2018

ewbankkit Jul 11, 2018

samb1729 Jul 11, 2018

ewbankkit Jul 11, 2018

pingles Jul 12, 2018

samb1729 Jul 12, 2018

pingles commented Jul 12, 2018

pingles left a comment

pingles Jul 13, 2018

pingles Jul 13, 2018

pingles Jul 13, 2018

pingles Jul 13, 2018

pingles Jul 13, 2018

pingles Jul 13, 2018

Restrict metadata routes #110

Restrict metadata routes #110

Conversation

samb1729 commented Jul 4, 2018

pingles commented Jul 4, 2018

samb1729 commented Jul 5, 2018

samb1729 commented Jul 6, 2018

codecov bot commented Jul 6, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

pingles commented Jul 12, 2018

pingles left a 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

codecov bot commented Jul 6, 2018 •

edited

Loading