Add support for IAM external ID #51
Conversation
|
We're making travis real sad right now, but getting closer... |
|
Woohoo. Clean build. Though, it looks like travis is going to konk out since this is coming from a fork, and therefore no docker login secrets. Ok, now that the build is passing, hopefully this also works. |
internal/docker.go
Outdated
| @@ -105,6 +105,11 @@ func findDockerContainerIAMRole(container *docker.Container, request *Request) ( | |||
| return "", fmt.Errorf("Could not find IAM_ROLE in the container ENV config") | |||
| } | |||
|
|
|||
| func findDockerContainerExternalId(container *docker.Container, request *Request) (string, error) { | |||
There was a problem hiding this comment.
I would drop the error return in this function, since we just discard it anyway :)
| func findDockerContainerExternalId(container *docker.Container, request *Request) (string, error) { | |
| func findDockerContainerExternalId(container *docker.Container, request *Request) string { |
internal/aws.go
Outdated
| RoleArn: aws.String(arn), | ||
| RoleSessionName: aws.String("go-metadataproxy"), | ||
| }) | ||
| req := stsService.AssumeRoleRequest(constructAssumeRoleInput(arn, externalId, "go-metadataproxy")) |
There was a problem hiding this comment.
Would move go-metadataproxy into being hardcoded in the RoleSessionName since its static and users can't change it anyway, fewer arguments to pass around=win
| req := stsService.AssumeRoleRequest(constructAssumeRoleInput(arn, externalId, "go-metadataproxy")) | |
| req := stsService.AssumeRoleRequest(constructAssumeRoleInput(arn, externalId)) |
internal/http_helper.go
Outdated
| return nil, "", err | ||
| } | ||
|
|
||
| externalId, err := findDockerContainerExternalId(container, request) |
There was a problem hiding this comment.
since this can never fail (see the other comment on removing the err) the error handling can just go away here
|
@byronwolfman don't worry about the build failing with The code looks solid, a couple of simple nits in my review to simplify it a bit and then we can ship it - thank you! |
|
Thanks for the review! All points should be (hopefully) addressed. |
|
Woohoo, thanks! Master build seems a bit borked so no dockerhub artifact yet. Hopefully that's just a (solvable) result of the fork... |
This PR adds support for sts external IDs to bring go-metadataproxy into closer parity with lyft/metadataproxy. Specifically, a container may define the environment variable
IAM_EXTERNAL_IDwhich, if present, go-metadataproxy should use in the sts role assumption.Unfortunately I hardly have a golang environment so speak of or a way to test this. I also write maybe a dozen lines of golang a year, so I'm open to review feedback if something here doesn't smell right.