BREAKING CHANGE: Change IMDS metadata defaults to more strict

[lisfo4ka]

PR o'clock

Description

This will close #1271.
Defaults were set according to the Amazon EKS security best practices

Terraform apply:

# module.eks.aws_launch_template.workers_launch_template[0] will be updated in-place
  ~ resource "aws_launch_template" "workers_launch_template" {
        id                      = "lt-05e8be158523b55cc"
      ~ latest_version          = 1 -> (known after apply)
        name                    = "test-eks-project-name-on-spot20210909233440248500000005"
        tags                    = {
            "SysOwner"     = "[email protected]"
            "user:tag"     = "test-eks"
        }
        # (13 unchanged attributes hidden)

      ~ metadata_options {
          ~ http_put_response_hop_limit = 0 -> 1
          ~ http_tokens                 = "optional" -> "required"
            # (1 unchanged attribute hidden)
        }

        # (9 unchanged blocks hidden)
    }

Checklist

• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[antonbabenko]

These changes will trigger an update for existing users unless they provide these values already.

I propose we group such changes for the next major release and make it visible in the upgrade guide.

WDYT?

[lisfo4ka]

Yes, sure, fully agree.

[antonbabenko]

Added this one to milestone v18.0.0 - https://github.com/terraform-aws-modules/terraform-aws-eks/milestone/5

@daroga0002 @lisfo4ka If you find other PRs which introduce breaking changes, let's add them there.

[daroga0002]

@lisfo4ka please rebase

[jjhidalgar]

Note that I tested this yesterday by manually setting "required", and EBS and EFS CSI Provisioner helm charts stopped working because they could not read metadata. I tried it with managed nodes, but should be the same. And probably, this can be fixed somehow, maybe in the chart values or something, I'm not expert in this instance metadata thingy.

In my case I don't mind if you set it by default because it is very easy to change to "optional", but wanted you to know that this is giving me important issues. So maybe test it, or add an important note so people know.

[ArchiFleKs]

This is a good idea to be secure by default I might be wrong but I think metadata_http_put_response_hop_limit blocks metadata access for all the pods except the one using host networking right ?

It means user should use IRSA for all the component. I'm not sure you can retrieve region or vpc id or other information without querying metadata service, that mean adding them manually I guess. For example: https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html#deploy-lb-controller

I can also think of aws ebs csi driver which also rely on metadata, and not sure if the csi-node can function without metadata access (kubernetes-sigs/aws-ebs-csi-driver#474). Anyway this is good but will require some adaptation on the user side to be able to upgrade to this major release

[daroga0002]

It means user should use IRSA for all the component.

exactly as per https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html

Because:

Even if you assign an IAM role to a Kubernetes service account, the pod still also has the 
permissions assigned to the Amazon EKS node IAM role, unless you block pod access to 
the IMDS. For more information, see Restricting access to the IMDS and Amazon EC2 
instance profile credentials.

[github-actions]

Thank you for your contribution!

The CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts.
Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request.

[lisfo4ka]

Hi folks,

Investigating a little bit deeper I suggest increasing TTL (metadata_http_put_response_hop_limit) to 2, since 1 is really incompatible with containerised applications on Kubernetes that run in a separate network namespace from the instance.
I know that it can be figured out by using host networking for the pod, but I'm not sure if it's a good recommendation to the community.

Moreover, AWS EKS best practices admit increasing the hop limit on EC2 instances to 2.

Even more, official CloudFormation templates for EKS cluster deployment are going to change the hop limit to 2 as well. It seems not yet

And of course, we can remind in a Changelog that after restricting access to the instance metadata the pod can't inherit node IAM profile anymore, so the appropriate IAM roles for application service accounts should be provided.
For instance, the first deployment which should be taken into account is Daemonset aws-node that is responsible for AWS CNI plugin work and requires AmazonEKS_CNI_Policy. The details for configuring IRSA for the CNI plugin can be found here cni-iam-role.
The same should be considered for any deployments that rely on instance IAM profile token, e.g. node termination handler, EBS and EFS CSI drivers etc.

And just let me clarify that the parameter http_tokens = "required" doesn't block IMDS, but force to use IMDSv2 instead of IMDSv1, which provides improvement for the security and adds an additional defence in depth layer. More details - Add defense in depth against open firewalls, reverse proxies, and SSRF vulnerabilities with enhancements to the EC2 Instance Metadata Service

Also, we can leave a suggestion to configure the CloudWatch dashboard based on the MetadataNoToken metric.
It'll help users to ensure that no instances are using IMDSv1 before disabling it altogether.

[antonbabenko]

This issue has been resolved in version 18.0.0 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.