Default to IMDSv2 for instance metadata calls

[holmesjr]

This resolves #647

All manual metadata curl calls in the build scripts are now preceded by a call to get a token. The YAML template now defaults to enforcing this, with an option to allow older v1 calls.

The ability to turn off metadata calls entirely has not been included, as the docker GC script requires it.

[holmesjr]

Note one call here that didn't change:

elastic-ci-stack-for-aws/packer/linux/conf/bin/bk-install-elastic-stack.sh

Line 31 in c3073ad

INSTANCE_ID=$(/opt/aws/bin/ec2-metadata --instance-id | cut -d " " -f 2)

If the version of cloud-utils is recent enough, it should be using v2 calls. If not, it needs to be updated.

[yob]

Thanks @holmesjr!

We're still a bit short staffed over the summer holidays, but we'll try and give feedback by next week.

My initial thought is maybe we can default the new param to optional so this is a non-breaking change and can be merged into a 5.x release, and then we can default the param to required when a 6.x release eventually happens. Hold off on making that change until I get more eyes on this though ❤️

[pda]

FYI, I don't know if this is useful, but when I was doing IMDSv2 from bash in a branch elsewhere, it looked like this:

# global var cache for IMDSv2 token
imds_token=""

# curl options used by imds functions
imds_curlopt=(--silent --show-error --fail --connect-timeout 1 --retry 2)

imds_get_token() {
  echo >&2 "ec2-metadata: acquiring IMDSv2 token"
  imds_token="$(curl "${imds_curlopt[@]}" \
    -H "X-aws-ec2-metadata-token-ttl-seconds: 300" \
    -X PUT "http://169.254.169.254/latest/api/token")"
  if [[ -z $imds_token ]]; then
    echo >&2 "ec2-metadata: failed to acquire IMDSv2 token"
    return 1
  fi
}

imds_get() {
  local path="/latest/meta-data/$1"
  echo >&2 "ec2-metadata: getting $path"
  curl "${imds_curlopt[@]}" -H "X-aws-ec2-metadata-token: $imds_token" "http://169.254.169.254$path"
}

usage_example() {
  if ! imds_get_token; then return 1; fi
  local role; role=$(imds_get iam/security-credentials | head -n1)
}

Note I never shipped that branch, maybe it's no good, but your PR just triggered a memory of it and I figured I'd share it just in case it's useful.

[yob]

@pda does the new golang-based secrets plugin already use tokens for metadata requests? It'd be Not Fun if making tokens required broke fetching secrets from S3

[pda]

I imagine so, it's using a recent AWS SDK. I haven't explicitly checked.

[holmesjr]

Yeah, last time I looked it was just using the SDK, so it should be fine.

[holmesjr]

Sorry to be a pain - is there anything I can do to help get this reviewed?

[pda]

Sorry for being slow over here @holmesjr :(

My initial thought is maybe we can default the new param to optional so this is a non-breaking change and can be merged into a 5.x release, and then we can default the param to required when a 6.x release eventually happens.

I think I agree with @yob about making optional the default, so that existing stacks aren't forced to IMDSv2 without opting in. But I'm also wondering if we can reduce the parameter count; we're getting pretty close to the CloudFormation limit per template.

What do you think about either of these approaches?

A: Only add an IMDSv2 boolean parameter, default false. The next major elastic-stack would remove the parameter and hard-code it to true. MetadataOptions.HttpPutResponseHopLimit would be hard-coded to 1 until if/when somebody needs multiple hops.

B: Only add a IMDSHopLimit optional parameter, default "" (empty). Leaving it empty keeps IMDSv1 by passing AWS::NoValue to those MetadataOptions. Setting it to e.g. 1 forces IMDSv2.

Option A adds one parameter to elastic-stack v5 and removes it again in v6, but can't control hop limit.
Option B adds one parameter forever.

Any thoughts?

[pda]

Also — would it help you if we shipped the code that adds X-aws-ec2-metadata-token headers immediately, with a separate PR for the CloudFormation parameters to make IMDSv2 mandatory? Or is it only useful to you once it's all merged?

[holmesjr]

Option A adds one parameter to elastic-stack v5 and removes it again in v6, but can't control hop limit.
Option B adds one parameter forever.

Any thoughts?

Yeah, I understand the issues with params. While I have no need for multiple hops, I imagine someone somewhere might and we'd end up adding it in anyway. Maybe option B is better for that reason, but I'd actually prefer option A.

Also — would it help you if we shipped the code that adds X-aws-ec2-metadata-token headers immediately, with a separate PR for the CloudFormation parameters to make IMDSv2 mandatory? Or is it only useful to you once it's all merged?

In terms of the security improvement, mandatory IMDSv2 is the goal. The other part is just what was necessary to enable it. So for my use case, it's not useful until the whole lot's merged in.

[pda]

👆🏼 I've split this into two PRs; I think #788 is uncontroversial and can merge ASAP. I'll wait for a bit more feedback for #789, but I think it'll be fine to merge.

[yob]

Thanks for do the hard work on this @holmesjr, and sorry it took us a little while to get to it.

This has merged in #788 and #789, with the only change being that the new parameter defaults to optional.