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

Support setting EC2 instance metadata to require token (IMDSv2) #5137

Closed
geoffroyrenaud opened this issue Nov 20, 2019 · 15 comments · Fixed by #16051 or #16052
Closed

Support setting EC2 instance metadata to require token (IMDSv2) #5137

geoffroyrenaud opened this issue Nov 20, 2019 · 15 comments · Fixed by #16051 or #16052
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@geoffroyrenaud
Copy link

Following the announce on the aws security blog about improving SSRF vuln : https://aws.amazon.com/blogs/security/defense-in-depth-open-firewalls-reverse-proxies-ssrf-vulnerabilities-ec2-instance-metadata-service/

Use Case

EC2 instances can be set to require IMDSv2 only.

Proposed Solution

This can be done with running instances or by using correct IAM policy, documentation : https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html#configuring-instance-metadata-options


This is a 🚀 Feature Request

@geoffroyrenaud geoffroyrenaud added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Nov 20, 2019
@SomayaB SomayaB added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Nov 21, 2019
@rix0rrr rix0rrr added the effort/small Small work item – less than a day of effort label Jan 23, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Mar 5, 2020
@rix0rrr rix0rrr added the p2 label Aug 12, 2020
@ericzbeard ericzbeard added p1 and removed p2 labels Oct 23, 2020
@mauricioharley
Copy link

Hello, team. What's the progress on this? Does CDK already support directly specifying IMDSv2 for new instances, or do we need to use custom resources?

If custom is the answer, is there any example to follow?

Thanks.

@jumi-dev
Copy link
Contributor

@mauricioharley As long as it is not supported in the ec2 instance construct in CDK, you can use the AwsCustomResource to call SDK method modifyInstanceMetadataOptions. There, you can set the metadata options to enforce IMDSv2.

new cr.AwsCustomResource(this, "InstanceMetadataOptions", {
  onUpdate: {
    service: "EC2",
    action: "modifyInstanceMetadataOptions",
    parameters: {
      InstanceId: myInstance.instanceId,
      HttpEndpoint: 'enabled',
      HttpTokens: 'required'
    },
    region: this.region,
    physicalResourceId: cr.PhysicalResourceId.of(myInstance.instanceId),
  },
  policy: cr.AwsCustomResourcePolicy.fromSdkCalls({
    resources: [`arn:aws:ec2:${this.region}:${this.account}:instance/${myInstance.instanceId}`],
  }),
});

@jumi-dev
Copy link
Contributor

The CloudFormation feature is missing to specifiy the instance metadata. A request is already created:
aws-cloudformation/cloudformation-coverage-roadmap#655

This issue could be flagged with label needs-cfn.

@skinny85 skinny85 added the needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. label Dec 29, 2020
@skinny85
Copy link
Contributor

The CloudFormation feature is missing to specifiy the instance metadata. A request is already created:
aws-cloudformation/aws-cloudformation-coverage-roadmap#655

This issue could be flagged with label needs-cfn.

Sir yes sir! 😃

@richarddotcodes
Copy link
Contributor

Can this issue be updated with the current status? I'm interested in using higher level constructs (like the ASG) to set metadata options, but I'm not seeing a path forward after navigating through the various linked issues.

Is this still blocked by CloudFormation? The following docs appear to show it's now supported:

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-launchconfig.html#cfn-autoscaling-launchconfig-metadataoptions

@rix0rrr rix0rrr added good first issue Related to contributions. See CONTRIBUTING.md and removed needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. labels Jul 19, 2021
@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 19, 2021

@richarddotcodes you are right this can now be implemented. This means you can now use the regular Escape Hatching mechanism of CDK to configure these options, or someone can submit a PR to add the feature.

Example escape hatching code:

const launchConfig = autoScalingGroup.node.tryFindChild('LaunchConfig') as autoscaling.CfnLaunchConfiguration;

launchConfig.metadataOptions = { httpTokens: 'required' };
// OR
launchConfig.addPropertyOverride('MetadataOptions', {
  HttpTokens: 'required',
});

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 19, 2021

An Aspect that applies the change for all ASGs and EC2 instances would be a welcome addition, but those could equally well be two different PRs.

@richarddotcodes
Copy link
Contributor

I'd be happy to tackle this one. I agree that applying the change at larger scopes could be different PRs, so if all goes well, maybe I can take those later too.

@alexjfisher
Copy link

Is this still blocked by CloudFormation? The following docs appear to show it's now supported:

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-launchconfig.html#cfn-autoscaling-launchconfig-metadataoptions

That looks like for EC2s created by ASGs only. Regular EC2 instances seem to still be missing this feature and aws-cloudformation/cloudformation-coverage-roadmap#655 is still open.

@alexjfisher
Copy link

I've been able to create an EC2 with IMDSv2 using launch templates.
CDK could possibly make this easier, but the following seems to work for me.

const launchTemplate = new ec2.LaunchTemplate(this, 'LaunchTemplate', {});

// The LaunchTemplate construct doesn't support setting the metadata options, so escape hatch time...
const CfnLaunchTemplate = launchTemplate.node.defaultChild as ec2.CfnLaunchTemplate;

CfnLaunchTemplate.launchTemplateData = {
  metadataOptions: {
    httpTokens: 'required',
  }
}

const myEC2= new ec2.Instance(this, 'Instance', {
  vpc,
  // etc.
}

// The Instance construct doesn't support setting the launch template, so break out _again_...
const CfnMyEC2 = myEC2.node.defaultChild as ec2.CfnInstance;

CfnMyEC2.launchTemplate = {
  launchTemplateId: launchTemplate.launchTemplateId,
  version: launchTemplate.versionNumber,
}

@richarddotcodes
Copy link
Contributor

I've got a change ready that allows setting the metadata options in the autoscaling group construct. I created an interface to wrap the options and am passing it in via the asg props. It made sense to put this interface in the instance construct in aws-ec2, since it's a property of the instances and not asg. The issue I'm facing is that the ec2 tests fail awslint because I'm exporting this interface (for use in aws-autoscaling), but not using it directly in the instance class in instance.ts.

Any thoughts on my approach?

@richarddotcodes
Copy link
Contributor

Checking in again. If I could get some feedback to unblock this change, it would be much appreciated. Thanks!

@jericht
Copy link
Contributor

jericht commented Aug 12, 2021

I also need this for AutoScalingGroup, Instance, and LaunchTemplate as well. It would be great to have options for these directly in the construct interfaces instead of having to escape hatch into the underlying L1 constructs.

An Aspect that applies the change for all ASGs and EC2 instances would be a welcome addition, but those could equally well be two different PRs.

@rix0rrr I'm currently implementing an Aspect that would achieve this for the AutoScalingGroup, Instance, and LaunchTemplate constructs, but I'm not sure where this Aspect should go. It's going to be dependent on a few of the service construct libraries (@aws-cdk/aws-ec2 and @aws-cdk/aws-autoscaling), so it probably shouldn't go in @aws-cdk/core. Does it make sense to make a new package like @aws-cdk/aspects for this? or maybe just a generic @aws-cdk/utils package?

Thought about this again, maybe it makes more sense to have different Aspects that achieve this per construct supported. Then, they could just exist in the corresponding service construct library. Instead of my original idea of having a single Aspect to handle all constructs that support the option to require IMDSv2, we'll just have one for each construct listed above. I think I'll start with this idea and create a PR, then go from there.

mergify bot pushed a commit that referenced this issue Oct 19, 2021
Partially fixes: #5137
Related PR: #16051

**Note:** I have some concerns about duplicated code between this and the above linked PR. Please see that PR for more details.

### Changes
Adds an aspect that can enable/disable IMDSv1 on AutoScalingGroups

### Testing
Added unit tests

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@Miradorn
Copy link

As i see it this is only fixed for ASG as of #16052 and not for ec2 (see #16051 for that). Correct?

mergify bot pushed a commit that referenced this issue Oct 20, 2021
Partially fixes: #5137
Related PR: #16052

**Note:** This PR and the above related PR have common code that has been duplicated across these two PRs because I decided it made more sense for these Aspects to be in the same package with the constructs they work with. However, it means I had to duplicate some of the base class code across the two PRs. Looking for an opinion on what's better here:
- Should we keep it as is (2 PRs) so these Aspects are cleanly separated? or,
- Does it make sense to either combine them in some way (e.g. a new package `@aws-cdk/aspects`) or have one reference the other (maybe the AutoScalingGroup aspect can reference the code in this PR since it already depends on this package).

### Changes
Adds an aspect that can enable/disable IMDSv1 on Instances and Launch Templates.

### Testing
Added unit tests

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
Partially fixes: aws#5137
Related PR: aws#16051

**Note:** I have some concerns about duplicated code between this and the above linked PR. Please see that PR for more details.

### Changes
Adds an aspect that can enable/disable IMDSv1 on AutoScalingGroups

### Testing
Added unit tests

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
Partially fixes: aws#5137
Related PR: aws#16052

**Note:** This PR and the above related PR have common code that has been duplicated across these two PRs because I decided it made more sense for these Aspects to be in the same package with the constructs they work with. However, it means I had to duplicate some of the base class code across the two PRs. Looking for an opinion on what's better here:
- Should we keep it as is (2 PRs) so these Aspects are cleanly separated? or,
- Does it make sense to either combine them in some way (e.g. a new package `@aws-cdk/aspects`) or have one reference the other (maybe the AutoScalingGroup aspect can reference the code in this PR since it already depends on this package).

### Changes
Adds an aspect that can enable/disable IMDSv1 on Instances and Launch Templates.

### Testing
Added unit tests

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet