Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] metadata grains in AWS fail when IMDSv2 is required #65233

Open
edlitmus opened this issue Sep 19, 2023 · 7 comments · May be fixed by #65269
Open

[BUG] metadata grains in AWS fail when IMDSv2 is required #65233

edlitmus opened this issue Sep 19, 2023 · 7 comments · May be fixed by #65269
Labels
Bug broken, incorrect, or confusing behavior Duplicate Duplicate of another issue or PR - will be closed needs-triage

Comments

@edlitmus
Copy link

Description

The current metadata grain module does not account for IMDSv2 in AWS.

Setup

Running salt on instances in EC2 with metadata grains enabled fails when IMDSv2 is required by the underlying AMI. This results in errors and IMDSv2 requires the use of a token when making metadata requests.

Steps to Reproduce the behavior

Launch an EC2 instances using an AMI that has been produced which enforces the use of IMDSv2. Install salt and try to access metadata grains.

Expected behavior

The metadata grains module should retry requests if IMDSv1 requests return a 403, or there should be an AWS specific metadata module that uses IMDSv2 as this should be universally available in AWS.

Versions Report

This has been tested in 3004.2 as well as 3006.2

Additional context

We replaced the use of the built in metadata grain with this custom module, combining parts of the existing codebase that account for IMDSv2 in our AWS salt systems:

import os

import salt.utils.aws as metadata
import salt.utils.data
import salt.utils.json
import salt.utils.stringutils

def _search(prefix="latest/"):
    """
    Recursively look up all grains in the metadata server
    """
    ret = {}
    result = metadata.get_metadata(prefix)
    body = result.text
    for line in body.split("\n"):
        if line.endswith("/"):
            ret[line[:-1]] = _search(prefix=os.path.join(prefix, line))
        elif prefix == "latest/":
            # (gtmanfred) The first level should have a forward slash since
            # they have stuff underneath. This will not be doubled up though,
            # because lines ending with a slash are checked first.
            ret[line] = _search(prefix=os.path.join(prefix, line + "/"))
        elif line.endswith(("dynamic", "meta-data")):
            ret[line] = _search(prefix=os.path.join(prefix, line))
        elif "=" in line:
            key, value = line.split("=")
            ret[value] = _search(prefix=os.path.join(prefix, key))
        else:
            retdata = metadata.get_metadata(os.path.join(prefix, line)).text
            # (gtmanfred) This try except block is slightly faster than
            # checking if the string starts with a curly brace
            if isinstance(retdata, bytes):
                try:
                    ret[line] = salt.utils.json.loads(
                        salt.utils.stringutils.to_unicode(retdata)
                    )
                except ValueError:
                    ret[line] = salt.utils.stringutils.to_unicode(retdata)
            else:
                ret[line] = retdata
    return salt.utils.data.decode(ret)

def main():
    ret = {}
    ret['dynamic'] = _search('dynamic')
    ret['meta-data'] = _search('meta-data')
    return ret
@edlitmus edlitmus added Bug broken, incorrect, or confusing behavior needs-triage labels Sep 19, 2023
@OrangeDog
Copy link
Contributor

Duplicates #60668

@OrangeDog OrangeDog added the Duplicate Duplicate of another issue or PR - will be closed label Sep 20, 2023
@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 20, 2023

@edlitmus are you willing to submit a PR with test coverage?

@esilva-everbridge
Copy link

esilva-everbridge commented Sep 20, 2023

I could, if I can get someone to advise on how best to handle it.

Should this be added as a new grain module specific to AWS (metadata_aws)?

@esilva-everbridge
Copy link

Sorry for the two accounts, BTW (work and personal)

@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 21, 2023

It would be ideal if both IMDSv1 and IMDSv2 are able to be supported in the same grain

@edlitmus
Copy link
Author

My understanding of the salt.utils.aws:get_metadata() method is that it should be able to work with either, as it falls back on IMDSv2 requests only if it encounters a 401 response, so the code in this ticket should already be sufficient for both.

I need to take a deeper dive into the test coverage to see if there are existing tests or if there is a way to actually test this.

@edlitmus
Copy link
Author

I was able to get an EC2 instance with IMDSv2 required set up and running my salt from my fork with the new code and the meta-data grains work just fine. I can't find any unit tests specifically for the metadata grains, but I'm not sure how that would even work without a lot of mocking up of the metadata end points.

I'll submit a PR with my changes after I finish running all the unit tests just to make sure nothing wonky is going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Duplicate Duplicate of another issue or PR - will be closed needs-triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants