Skip to content
This repository has been archived by the owner on Mar 5, 2024. It is now read-only.

Support IMDS v2 #381

Merged
merged 2 commits into from
Mar 16, 2020
Merged

Support IMDS v2 #381

merged 2 commits into from
Mar 16, 2020

Conversation

rbvigilante
Copy link
Contributor

@rbvigilante rbvigilante commented Mar 3, 2020

fixes #359

  • Prevent the reverse proxy adding an X-Forwarded-For header
  • Allow PUT requests to */api/token without consulting the API whitelist

@rbvigilante rbvigilante changed the title Support imds v2 Support IMDS v2 Mar 3, 2020
@Joseph-Irving
Copy link
Contributor

Joseph-Irving commented Mar 3, 2020

the tests are failing as you're using a http method (request.Clone) not found in go1.12, I've updated the go version, so if you rebase with master it should fix it

This is required for support of the instance metadata API v2
(Regardless of whether the path is in the whitelist or not)
@rbvigilante
Copy link
Contributor Author

Ah, sorry. Rebase done.

@Joseph-Irving
Copy link
Contributor

Hey, I've been trying to look over this, the first two commits seem pretty self-explanatory. It's the third one that I keep getting stuck on, can you give some more detail about what you're trying to achieve here? Is it actually needed for this to work or is it an extra set of security you're adding? If it's the latter perhaps we should split this up.

@rbvigilante
Copy link
Contributor Author

The reasoning behind the third commit went something like this:

If I'm on an AWS instance (without Kubernetes or Kiam), I can now (successfully) get credentials out of the metadata API in two ways:

  • I can just hit 169.254.169.254/latest/meta-data/iam/security-credentials/<role> with no headers, and get the credentials through IMDSv1 (unless this has been explicitly disabled)
  • I can get a token, then hit the same URL with a session token header to get the credentials through IMDSv2

I'm pretty sure the first two commits here (whitelisting the token endpoint and removing the x_forwarded_for header) will allow IMDSv2 to function more or less as intended for every endpoint Kiam isn't taking over. For the ones it does take over (/iam/*), though, we'd be potentially accepting a session token and completely ignoring it (and handing out a role regardless).

The third commit is my attempt to respect the session control aspect of IMDSv2 without needing to keep track of sessions within Kiam. Totally happy to remove it and deal with it later if you like.

@Joseph-Irving
Copy link
Contributor

Right ok, I think I follow, so you're having kiam check the token before handing out a role to the requester as an additional security layer.
That seems like a nice thing to have but I would be keen to have this separated out of this PR and potentially put behind a feature flag, I'm worried about this interacting with certain users' setups in a strange way and causing kiam to totally stop working for them.

@rbvigilante
Copy link
Contributor Author

Sure. I'll put it into another branch.

@rbvigilante
Copy link
Contributor Author

Done

Copy link
Contributor

@Joseph-Irving Joseph-Irving left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, that's great thanks
lgtm!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to access new AWS metadata api
2 participants