v3 API and refactorings #109

pingles · 2018-07-04T14:57:30Z

This change was focused on changes to move to a new Agent/Server interaction ahead of v3. As part of doing this I also refactored/changed a few other interfaces to streamline stuff a little.

API Changes

This adds a new Server API call: GetPodCredentials that allows credentials to be obtained by the server given an IP address. Previously the agent would make three separate calls:

Find the Pod (and it's role)
Check the Pod can assume the role
Get credentials for the role

This collapses calls together inside the server. This makes it easier to deliver #14, as well as potentially moving to other mechanisms for agents to request credentials (like the service account token introduced in #104.

Testing

I've also broken apart the metadata server tests and moved all tests into the package directories, the Drone build also now runs with coverage stats to make it easier to figure out what parts of the system need tests added.

End to End Testing

Although the tests are in better shape there's still things that aren't covered and would be nice to have some end-to-end tests to prove the whole- for now I've parked that and created #108 to track separately.

Review and Request for Comments

I'd like some review/thoughts on the protos in particular. For now I've added new API calls and left the existing, so that the same server could support both new (3.x) and old (2.x) agents. However, given Kubernetes makes it easy to run servers side-by-side (only the agents have a hard requirement on running one per node) another option is to remove the API calls and have agents talk to different server services.

* created additional GetPodCredentials operation * use Client interface in place of PodFinder and other interfaces * remove telemetry wrappers for gateway and server; will be replaced with prometheus soon

* added more tests and refactored to tidy * added coverage makefile target to track progress Acked-by: Paul Ingles <[email protected]> Signed-off-by: Paul Ingles <[email protected]>

* create unit test for credentials handler * test happy path: role and credentials are found

* added test for server error when requesting credentials * remove empty role- not possible for this handler to be called without a role in the vars

* add credentials handler test for retry * role handler install

* remove old metadata server tests, replaced with handler unit tests * add server test

samb1729 · 2018-07-04T15:37:52Z

Makefile

-test: $(SOURCES)
-	go test test/unit/*_test.go
-	go test test/functional/*_test.go
+test: $(shell find . -name '*.go')


$(SOURCES)!

pingles · 2018-07-04T17:08:16Z

@DewaldV @Sambooo @Joseph-Irving @tombooth I added you all so you could take a look.

@moofish32 I know you were also keen to see where this branch/change got to so be great to hear what you think also.

samb1729 · 2018-07-05T11:15:34Z

pkg/aws/metadata/handler_credentials.go

-	credentialsProvider sts.CredentialsProvider
-	clientIP            clientIPFunc
-	policy              server.AssumeRolePolicy
+	clientIP clientIPFunc


The name clientIP is a bit misleading in a few places. We're not passing around an ip address but rather an ip resolution function. Not massively important though.

samb1729 · 2018-07-05T11:18:08Z

pkg/aws/metadata/handler_credentials.go

+	client   server.Client
+}
+
+func (c *credentialsHandler) Install(router *mux.Router) {


I like this approach to splitting up the handlers 👍

samb1729 · 2018-07-05T11:29:16Z

pkg/aws/metadata/handler_credentials.go

-	}
-
-	credentials, err := c.credentialsForRole(ctx, requestedRole)
+	credentials, err := c.fetchCredentials(ctx, ip, requestedRole)


There's no more code that can be deleted from this handler without breaking it. Good stuff.

codecov · 2018-07-05T22:13:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@60c82f6). Click here to learn what that means.
The diff coverage is 61.84%.

@@            Coverage Diff            @@
##             master     #109   +/-   ##
=========================================
  Coverage          ?   37.79%           
=========================================
  Files             ?       21           
  Lines             ?      799           
  Branches          ?        0           
=========================================
  Hits              ?      302           
  Misses            ?      469           
  Partials          ?       28

Impacted Files	Coverage Δ
pkg/aws/metadata/server.go	`5.45% <0%> (ø)`
pkg/server/gateway.go	`0% <0%> (ø)`
pkg/server/policy.go	`51.78% <100%> (ø)`
pkg/k8s/pod_cache.go	`54.8% <100%> (ø)`
pkg/prefetch/manager.go	`53.48% <100%> (ø)`
pkg/aws/metadata/handler_role_name.go	`90% <100%> (ø)`
pkg/aws/metadata/handler_credentials.go	`83.33% <100%> (ø)`
pkg/server/server.go	`15.55% <54.54%> (ø)`

samb1729

I'm a fan of the new testing approach here @pingles. I haven't tried to figure out whether we can avoid supporting two api versions easily but otherwise I'm happy with these changes.

samb1729 · 2018-07-06T14:22:16Z

pkg/aws/metadata/handler_credentials_test.go

+		t.Error("expected json result", content)
+	}
+
+	expected := `{"Code":"","Type":"","AccessKeyId":"A1","SecretAccessKey":"S1","Token":"","Expiration":"","LastUpdated":""}`


We should probably unmarshal this into a struct with these fields as encoding/json implements RFC 7159 so there's no guarantee the keys will be marshaled in this order.

samb1729 · 2018-07-06T14:26:22Z

pkg/aws/metadata/handler_credentials_test.go

+		t.Error("unexpected status", rr.Code)
+	}
+	if !strings.Contains(rr.Body.String(), "error fetching credentials: no pod found") {
+		t.Error("unexpected error", rr.Body.String())


Unexpected lack of error, no?

samb1729 · 2018-07-06T15:29:17Z

pkg/testutil/server/client.go

+)
+
+// StubClient returns fake server responses
+type StubClient struct {


👍 to this idea

moofish32

Major +1 for the testing improvements and refactoring of the auth being controlled by the server and not split with the client. I provided some comments as interested party that hopefully help represent at least one outside potential customer.

moofish32 · 2018-07-09T21:09:28Z

pkg/aws/metadata/handler_credentials.go

@@ -90,15 +66,14 @@ func (c *credentialsHandler) Handle(ctx context.Context, w http.ResponseWriter,
 	return http.StatusOK, nil
 }

-func (c *credentialsHandler) credentialsForRole(ctx context.Context, role string) (*sts.Credentials, error) {
+func (c *credentialsHandler) fetchCredentials(ctx context.Context, ip, requestedRole string) (*sts.Credentials, error) {


this is a great improvement and reduces the ability of compromised client to just arbitrarily request roles.

moofish32 · 2018-07-09T21:16:13Z

pkg/aws/metadata/handler_role_name_test.go

+)
+
+func TestReturnRoleWhenClientResponds(t *testing.T) {
+	r, _ := http.NewRequest("GET", "/latest/meta-data/iam/security-credentials/", nil)


All of these tests use /latest/meta-data/iam/security-credentials/. However, when you curl on an ec2 instance the /latest/meta-data/iam/security-credentials in at least some situations you don't get 200:

curl -v 169.254.169.254/latest/meta-data/iam/security-credentials * About to connect() to 169.254.169.254 port 80 (#0) * Trying 169.254.169.254... * Connected to 169.254.169.254 (169.254.169.254) port 80 (#0) > GET /latest/meta-data/iam/security-credentials HTTP/1.1 > User-Agent: curl/7.29.0 > Host: 169.254.169.254 > Accept: */* > * HTTP 1.0, assume close after body < HTTP/1.0 301 Moved Permanently < Location: http://169.254.169.254/latest/meta-data/iam/security-credentials/ < Content-Length: 0 < Connection: close < Date: Mon, 09 Jul 2018 21:12:44 GMT < Server: EC2ws

I don't think the SDK actually uses this endpoint, but should we make this match?

I believe you're getting a 301 rather than 200 because you didn't including the trailing slash. See this Google blog post:

don’t both return a 200 response code, but that one version redirects to the other.

Yeah I think @moofish32 was pointing out that the metadata server they tested on 301'ed, rather than returning credentials. Kiam currently supports either form and serves 200s on both.

@moofish32 I seem to remember others mentioning this stuff varied between instance generations (so it changed between m4 and m5 for example).

Do you remember the instance type you tested this on?

Either way I suspect we should probably add an issue to change it so that it behaves the same way- 301'ing rather than successfully working on both?

I've now added #116 to track this issue separately.

moofish32 · 2018-07-09T21:18:42Z

pkg/future/future_test.go

@@ -11,17 +11,16 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package kiam
+package future


Is there something really unique about this future package? I know Capital One has this one: https://github.com/capitalone/go-future-context, I'm sure there are others. I just don't know if there is a specific reason the team wants to maintain this one. I agree it's a small amount of code right now.

moofish32 · 2018-07-09T21:30:52Z

pkg/server/policy.go

@@ -107,7 +107,7 @@ func (f *forbidden) Explanation() string {
 }

 func (p *RequestingAnnotatedRolePolicy) IsAllowedAssumeRole(ctx context.Context, role, podIP string) (Decision, error) {
-	pod, err := p.pods.GetPodByIP(ctx, podIP)
+	pod, err := p.pods.GetPodByIP(podIP)


This is a point where I'm curious if we should validate the client IP (kiam client making the request) and the HostIp in the PodStatus are a match? This would prevent being able to use a compromised host to provide IAM credentials for pods that could not be scheduled on the host. Probably a future enhancement but wanted to pose the question.

It's only the agent that directly receives requests from clients, after which point the ip address forwarded by the agent is used to identify the pod. Are you suggesting the agent also pass its host ip to the server in this api call?

I suppose it does make sense to check that as a pod shouldn't hit another host's agent. @pingles would supporting this be another major bump or just an extension of the current request struct with graceful fallback?

@moofish32 I like the suggestion as an additional security enhancement (I assume this means check that the pod's host matches the agent's client IP?)

If so- could you open a new issue to track it please? I'm not sure if we'd commit to adding for v3 but I don't think it'd change the API as-is (the Pod IP is passed to GetPodCredentials which would allow for additional policy to check against the PodStatus).

@Sambooo -- we have both actually. The agent passes the IP of the pod and GRPC enables us to track the agent IP. The issue is tracked as #105

moofish32 · 2018-07-09T21:44:15Z

pkg/server/server.go

+		return nil, ErrPolicyForbidden
+	}
+
+	creds, err := k.credentialsProvider.CredentialsForRole(ctx, req.Role)


So another potentially breaking change will come here when we want to specify the session name by pod ID. I don't know if that is important enough to roll in here or wait until later. I think the usefulness of a constant session name is fairly low.

There's a separate issue open for being able to name sessions #38.

The signature for CredentialsForRole will change but the only published API is the gRPC server's GetPodCredentials which should tolerate changes to reference pods by IP/ID etc.

So for now I'd leave this and pick it up as part of #38.

moofish32 · 2018-07-09T21:46:50Z

pkg/server/server.go

@@ -181,7 +224,7 @@ func NewServer(config *Config) (*KiamServer, error) {
 		arnResolver,
 	)
 	server.credentialsProvider = credentialsCache
-	server.manager = prefetch.NewManager(credentialsCache, server.pods, server.pods)
+	server.manager = prefetch.NewManager(credentialsCache, server.pods)


One more potential big change to consider is moving the credential cache into the pod cache. Logically if you want unique credentials per pod, then maintaining two caches is less useful. If this is potentially a major version change to implement and it's high enough priority, should we consider grouping the change here?

I'd defer picking it up until we do #38. I don't think the change would need a major version change to implement. I'd like us to sort #38 before we release v3 though- so we can still review the design for different pod and credential caches, the way sessions are named etc.

pingles added 23 commits June 29, 2018 16:35

wip: making server api chunkier

c917c77

* created additional GetPodCredentials operation * use Client interface in place of PodFinder and other interfaces * remove telemetry wrappers for gateway and server; will be replaced with prometheus soon

wip: add requested role to GetPodCredentialsRequest

f0d8e53

wip on extracting api- tidying handler tests

c744bbf

update benchmarks

a0943b9

wip: api v3 refactoring. add more tests to role name handler

d613f35

* added more tests and refactored to tidy * added coverage makefile target to track progress Acked-by: Paul Ingles <[email protected]> Signed-off-by: Paul Ingles <[email protected]>

wip: refactoring for v3

abc6291

* create unit test for credentials handler * test happy path: role and credentials are found

wip: v3 refactoring

0b87404

* added test for server error when requesting credentials * remove empty role- not possible for this handler to be called without a role in the vars

v3 refactor wip

bbc7752

* add credentials handler test for retry * role handler install

add sample access and secret keys to creds test

ae43a98

wip: v3 refactoring and testing

dbf9760

* remove old metadata server tests, replaced with handler unit tests * add server test

add happy path for server test- issuing credentials

d203e71

add some log statements to GetPodCredentials

2a5eabc

Merge branch 'master' of github.com:uswitch/kiam

a586d60

resynced with upstream master

6c9d389

update benchmark target

245b582

move prefetcher test out of test/ dir

cf5c5fd

move policy test out of test/ dir

e630471

move last test from test/ into package dir

2f0b225

delete unused rolefinder interface

a1e4e10

make FindPodForIP private: only need GetPodByIP

65eef97

add todo comment

7d58d33

add implementations for gateway talking to new api

2bf669f

move findRole into handler- only used there now

d33bcd6

samb1729 reviewed Jul 4, 2018

View reviewed changes

makefile sources

9513c2e

pingles mentioned this pull request Jul 4, 2018

Restrict metadata routes #110

Merged

pingles requested review from tombooth, DewaldV and Joseph-Irving July 4, 2018 17:07

pingles mentioned this pull request Jul 4, 2018

Record IAM errors as events #14

Closed

samb1729 reviewed Jul 5, 2018

View reviewed changes

pingles mentioned this pull request Jul 5, 2018

Block Access to Other Metadata #60

Closed

pingles added 2 commits July 5, 2018 23:07

add codecov

c74db56

make test shouldn't run with coverage, separate target

4c6c421

samb1729 reviewed Jul 6, 2018

View reviewed changes

creds handler: decode response into struct

d6bb82d

This was referenced Jul 9, 2018

Idiamond/prometheus and statsd #113

Closed

Improve gRPC telemetry integration #114

Closed

moofish32 reviewed Jul 9, 2018

View reviewed changes

DewaldV approved these changes Jul 12, 2018

View reviewed changes

pingles merged commit 47c4922 into master Jul 12, 2018

pingles mentioned this pull request Jul 18, 2018

Add leaktest back to tests #125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v3 API and refactorings #109

v3 API and refactorings #109

pingles commented Jul 4, 2018

samb1729 Jul 4, 2018

pingles commented Jul 4, 2018

samb1729 Jul 5, 2018

samb1729 Jul 5, 2018

samb1729 Jul 5, 2018

codecov bot commented Jul 5, 2018 •

edited

Loading

samb1729 left a comment

samb1729 Jul 6, 2018

samb1729 Jul 6, 2018

samb1729 Jul 6, 2018

moofish32 left a comment

moofish32 Jul 9, 2018

moofish32 Jul 9, 2018

samb1729 Jul 10, 2018

pingles Jul 10, 2018

pingles Jul 12, 2018

moofish32 Jul 9, 2018

moofish32 Jul 9, 2018

samb1729 Jul 10, 2018

pingles Jul 10, 2018

moofish32 Jul 11, 2018

moofish32 Jul 9, 2018

pingles Jul 10, 2018

moofish32 Jul 9, 2018

pingles Jul 10, 2018

v3 API and refactorings #109

v3 API and refactorings #109

Conversation

pingles commented Jul 4, 2018

API Changes

Testing

End to End Testing

Review and Request for Comments

Choose a reason for hiding this comment

pingles commented Jul 4, 2018

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 5, 2018 • edited Loading

Codecov Report

samb1729 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

moofish32 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

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

codecov bot commented Jul 5, 2018 •

edited

Loading