Include IAM role in ec2 data (issue #1524)

[kcbraunschweig]

Description

Currently the ohai ec2 plugin reads from the AWS metadata service but strips out all IAM data to prevent leaking security credentials. Change modifies this filtering slightly to still include the IAM role-name which is part of the metadata at:
iam/security-credentials/role-name
see: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instancedata-data-categories.html
Don't include the data returned in the document under role-name as that includes actually includes credentials, but the name part isn't a security concern and is useful to people to know the IAM identity of the instance where chef is running.

In the solution i'm being fairly paranoid checking explicitly for the security-credentials key and that it has exactly 1 key below it. As far as I can tell 'security-credentials' will only exist at all if there's an associated role, in fact if there's no role 'iam' won't exist at all and there can only be 1 associated role at any one time ever. So in reality if the IAM key exists at all, then there should be exactly 1 role name for us to find always. But yay paranoia, happy to do it however people like.

Related Issue

#1524

Types of changes

• New feature (non-breaking change which adds functionality)

Checklist:

• I have read the CONTRIBUTING document.
• I have run the pre-merge tests locally and they pass.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commits have been signed-off for the Developer Certificate of Origin.

[kcbraunschweig]

those test failures don't seem related to this change but lmk if there's something I need to do there.

[phiggins]

This seems like a good addition, but it makes me wonder if there's more relevant information here or a more complete way to handle this. In other words I'm suspicious that this satisfies one use case and could lead to a creeping addition of more things.

[phiggins]

those test failures don't seem related to this change but lmk if there's something I need to do there.

You're correct, the CI failures are unrelated.

[kcbraunschweig]

This seems like a good addition, but it makes me wonder if there's more relevant information here or a more complete way to handle this. In other words I'm suspicious that this satisfies one use case and could lead to a creeping addition of more things.

This is the full structure:

  "iam": {
    "info": {
      "Code": "Success",
      "LastUpdated": "2020-10-07T18:44:07Z",
      "InstanceProfileArn": "arn:aws:iam::123456789:instance-profile/foo",
      "InstanceProfileId": "AAAAAAAAAAAAAFCK"
    },
    "security-credentials": {
      "foo": {
        "Code": "Success",
        "LastUpdated": "2020-10-07T18:43:37Z",
        "Type": "AWS-HMAC",
        "AccessKeyId": "VERYSECRET",
        "SecretAccessKey": "j+VERYSECRETSETUFF",
        "Token": "IQo/VERYSECRETSTUFFQ==",
        "Expiration": "2020-10-08T01:10:52Z"
      }
    }
  }

The only other bits that seem potentially interesting to me are InstanceProfileArn and InstanceProfileId. It could change if they added something in the future but I'm wary of trying to future proof because they might add something new that we want to exclude.

[jaymzh]

It feels to me like keeping all of info is harmless and useful. I agree that blacklisting is bad, they could add "additional_creds" or some such, so we should whitelist... but thoughts on grabbing info ?

[kcbraunschweig]

lmk how you like this. grabs info. moves role_name under an iam key as well. If anything additional is desired in the future, it can be added to the select.

[phiggins]

The code seems good to me 👍

In order to merge this PR all commits will need a DCO signoff though.

[jaymzh]

As @phiggins says you need to fix your DCO. Just squash them all together and forcepush, or amend your last commit and forcepush, either way.

[jaymzh]

Looks good, thanks. I'll wait for someone internal to merge, or ping me tomorrow to merge if they haven't.

[jaymzh]

Hey @lamont-granquist @tas50 @phiggins ... forgot I'm an approver, not an owner, so I don't have access to merge. Can one of you click the button?