WIP: feat(ec2): add instance resource #3697

hoegertn · 2019-08-17T19:41:32Z

Add Instance resource and create bastion host construct based on it.

fixes #3174 and #1713

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mergify · 2019-08-17T19:42:11Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

slipdexic

Thorsten I think this is great addition.

slipdexic · 2019-08-18T13:15:43Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+    readonly allowClassicSSH?: boolean;
+}
+
+export class BastionHost extends Instance {


I wonder if the class name should reflect the operating system, e.g BastionHostLinux .
Sadly a number of us have to use Windows Bastion Hosts
That would then leave scope for someone to create a Windows Version.

Sound great. I will change it

Shouldn't Windows/Linux just be a setting?

Do we really need this to be a subclass of Instance? What value do we get from that? Why not encapsulate?

slipdexic · 2019-08-18T13:16:36Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+        this.addUserData('yum install -y https://s3.amazonaws.com/ec2-downloads-windows/SSMAgent/latest/linux_amd64/amazon-ssm-agent.rpm');
+
+        if (props.allowClassicSSH) {
+            this.connections.allowFromAnyIpv4(Port.tcp(22), 'Allow SSH access from everywhere');


I think this may be a bad idea; the end-user would be opening this instance up to the whole world by default.
The AWS Well-Architected Framework Security Pillar advises against,
There is also a rule in the CIS AWS Foundation to detect such security groups. ( Can be found in SecurityHub)
4.1 Ensure no security groups allow ingress from 0.0.0.0/0 to port 22

I fully agree with this objection. On the other hand, this is exactly what I see with many AWS users. If the server is up to date and key management is working correctly, it should not be a problem to have 22 open and it is the only way if your clients have dynamic IPs from their ISPs.

How about requiring the user to explicitly enable this via a setting which will be false by default?

slipdexic · 2019-08-18T13:24:18Z

packages/@aws-cdk/aws-ec2/lib/instance.ts

+  constructor(scope: Construct, id: string, props: InstanceProps) {
+    super(scope, id);
+
+    this.securityGroup = new SecurityGroup(this, 'InstanceSecurityGroup', {


Instead of creating a security group here, this could be passed on props, then only create a default group is one is not supplied.
This gives the user ultimate flexibility.
In an enterprise or regulated space, the SecuityGroup generally would have been created upfront by another team.

I like this idea because I do not like the SSH TCP access to the bastion server at all. So I will make the group suppliable and by default block any access. For the second part of your comment: I generally do not like the approach of other teams creating IAM roles or sec groups. The whole infrastructure should be defined and created by one team. If you want/have to have four-eye validation here I use the way of merge requests with approvals of the CISO/CSIRT or SCPs to prevent creating bad stuff. In an enterprise context, I would expect NACLs to be in place to prevent users from opening ports they are not allowed to.

I see your point agree, but I work in environments that these rules apply, compliance forbids single team.
I have just created PR for NACLs as they were missing.

I see the compliance issue with single teams, but the question is if you can solve this on an org level instead of a code level. So by using four-eyes merges instead of separated AWS roles. But this is OT of this PR. Change is on the way.

I am with @slipdexic - there are many users who simply are not allowed to create security groups or IAM roles and it makes sense to allow people to pass those in optionally.

eladb

Nice job!

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

eladb · 2019-08-18T17:59:02Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+    /**
+     * In which AZ to place the instance within the VPC
+     *
+     * @default - Random zone.


Random bit stable?

Is the order of props.vpc.selectSubnets() stable? I use the first one.

eladb · 2019-08-18T17:59:31Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+    /**
+     * Use public subnet instead of private one
+     *
+     * @default - false


Shouldn't we use the standard SubnetSelection type here?

I map this to a standard SubnetSelection so yes I could change it. I wanted to make the interface easier for users instead of handling subnet selections. Just tell me if you want it public or not. But I can just change it.

eladb · 2019-08-18T18:00:35Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+     *
+     * @default - false
+     */
+    readonly allowClassicSSH?: boolean;


Use enum with Ssh/Ssm instead of a boolean. Acronyms must be pascal cased (Ssh instead of SSH). Otherwise jsii can't delimit words.

Removed it regarding the other review comment

eladb · 2019-08-18T18:01:00Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+    readonly allowClassicSSH?: boolean;
+}
+
+export class BastionHost extends Instance {


Shouldn't Windows/Linux just be a setting?

eladb · 2019-08-18T18:02:48Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+        this.addUserData('yum install -y https://s3.amazonaws.com/ec2-downloads-windows/SSMAgent/latest/linux_amd64/amazon-ssm-agent.rpm');
+
+        if (props.allowClassicSSH) {
+            this.connections.allowFromAnyIpv4(Port.tcp(22), 'Allow SSH access from everywhere');


How about requiring the user to explicitly enable this via a setting which will be false by default?

eladb · 2019-08-18T18:03:22Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+            this.connections.allowFromAnyIpv4(Port.tcp(22), 'Allow SSH access from everywhere');
+        }
+
+        new CfnOutput(this, 'BastionHostId', {


Should this be both for SSH and SSM? (description talks about SSM)

eladb · 2019-08-18T18:04:38Z

packages/@aws-cdk/aws-ec2/lib/instance.ts

+   *      assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com')
+   *    });
+   *
+   * @default A role will automatically be created, it can be accessed via the `role` property


@default value should be -

eladb · 2019-08-18T18:06:19Z

packages/@aws-cdk/aws-ec2/lib/instance.ts

+  constructor(scope: Construct, id: string, props: InstanceProps) {
+    super(scope, id);
+
+    this.securityGroup = new SecurityGroup(this, 'InstanceSecurityGroup', {


I am with @slipdexic - there are many users who simply are not allowed to create security groups or IAM roles and it makes sense to allow people to pass those in optionally.

eladb · 2019-08-18T18:06:30Z

packages/@aws-cdk/aws-ec2/lib/instance.ts

+    });
+    this.connections = new Connections({ securityGroups: [this.securityGroup] });
+    this.securityGroups.push(this.securityGroup);
+    this.node.applyAspect(new Tag(NAME_TAG, props.instanceName || this.node.path));


Use Tag.add

I copied this from AutoScalingGroup but I changed it in this PR now.

hoegertn · 2019-08-18T18:17:34Z

@elad I subclassed Instance from an OOP perspective. BastionHostLinux is a special Instance so it is a sub-class. I think I have seen this style several times in CDK, but I can change it if it is not intended to be used.

slipdexic · 2019-08-18T18:57:36Z

Great work!

rix0rrr

Thanks for this!

Love the Instance; I wonder if we could make the bastion a little more clear and potentially flexible. What do you think?

rix0rrr · 2019-08-21T11:07:44Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+   *
+   * @default - false
+   */
+  readonly publicSubnets?: boolean;


Shouldn't this be a subnetSelection?

And shouldn't public subnets be the default? What use is a Bastion host in a private subnet?

EDIT: Oh I see, it probably has to do with whether you intend to access the bastion directly using SSH or using Session Manager. Could you at least make that clear in the documentation text? Or better yet, maybe it should be a property to select between the two?

I think there are three scenarios:

Public access using SSH -> public subnet

Access via SSM -> private subnet

Access using SSH from on-prem -> private subnet via VPN/DC

So public subnets should not be used imho...

Re use cases, we use all of these and also have use case where we have had to have a baston host, (jump box) in all 3 zones.

When conducting penetration tests we will place a (bastion) host in each zone (public, private, ..), sometime the only way to test a zone get to jump through bastions in each zone using proxy chains.
So in my imho this construct should be able to live in any zone, just default to public.

Also you should be ideally able to place this in an explicit subnet.

Yes, so I still feel we should expose the subnetSelection (and have it default to something sane, and I think PRIVATE is the best sane default we can have). It would immediately benefit from future extensions we would add to the subnetSelection mechanism (for example, selecting individual subnets such as @slipdexic requested). And it should have explicit documentation on there saying you need to select public subnets if you want to access it from the internet.

Would it make sense to have a top-level helper method for a common use case, bastion.allowSshAccessFrom(peer, peer, ...) or something? The only thing it would do is encapsulate port 22, but it might be just a little nicer.

Also, @hoegertn, could you add a section to the README about the 2 Bastion Host use cases (SSM access and SSH access)?

Sounds good. I will change it accordingly.

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

rix0rrr · 2019-08-21T11:13:35Z

packages/@aws-cdk/aws-ec2/lib/instance.ts

+  readonly availabilityZone?: string;
+
+  /**
+   * Whether the instance could initiate connections to anywhere by default


Only used if securityGroup is not supplied.

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

mergify · 2019-08-21T14:36:37Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-21T14:59:53Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

rix0rrr · 2019-08-22T07:55:28Z

packages/@aws-cdk/aws-ec2/lib/bastion-host.ts

+ *
+ * @experimental
+ */
+export class BastionHostLinux extends Instance {


Sorry for not seeing this before: I also agree with Elad that this host should probably encapsulate rather than extend Instance.

Can you elaborate on why? I still think that a bastion host is a special instance and should derive from it, so users have all the options they have with an instance but with some defaults regarding the use case. I thought OOP is one of the benefits of CDK.

We achieve OOP/polymorphism by implementing interfaces. Interfaces declare "a (this) can do the same thing as a (that)", regardless of the implementation. So if you want a Bastion Host to be-an Instance, then declare it like so:

class BastionHostLinux implements IInstance

Inheriting from a class brings both the "(this) can do the same as (that)" relationship, but it also couples one implementation to the other, and can have unintended implications for the future extensibility of the classes involved (either a change in the implementation of Instance has an unintended side effect in the implementation of BastionHost, or we now face the limitation that BastionHost can only extend one class).

In some situations implementation sharing is a perfectly valid choice (which would usually be made to make it quicker to implement a certain class since you don't have to type so much), but polymorphism through interface implementation is generally a safer choice in the long term, when it's hard to predict future changes.

In this case, I'd rather be safe than sorry later.

Ok, I will change it and see what methods and fields I want to expose from the instance class.

feat(ec2): add instance resource

ca8e12f

mergify bot added the automerge-enabled label Aug 17, 2019

feat(ec2): add bastion host construct

ffebb9b

slipdexic suggested changes Aug 18, 2019

View reviewed changes

hoegertn added 2 commits August 18, 2019 20:03

feat(ec2): PR review

b1d84f1

Merge branch 'master' into bastion-host

04ef866

eladb reviewed Aug 18, 2019

View reviewed changes

feat(ec2): rename bastion host for Linux

d0c58f1

chore: more feedback

9caa820

rix0rrr self-assigned this Aug 19, 2019

hoegertn added 3 commits August 20, 2019 11:24

Merge branch 'master' into bastion-host

1ae11d1

Merge branch 'master' into bastion-host

18c33c9

Merge branch 'master' into bastion-host

4e60ca7

rix0rrr reviewed Aug 21, 2019

View reviewed changes

Merge branch 'master' into bastion-host

d5a0fba

Merge branch 'master' into bastion-host

37bd20e

chore: review feedback

aa4d6d6

rix0rrr reviewed Aug 22, 2019

View reviewed changes

hoegertn and others added 4 commits August 22, 2019 22:51

Merge branch 'master' into bastion-host

7b07fa1

feat: progress

3ede39b

Merge branch 'master' into bastion-host

15c51ec

Small tweaks to bastion host

5f634bd

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 23, 2019

Merge branch 'master' into bastion-host

56e0c30

rix0rrr approved these changes Aug 23, 2019

View reviewed changes

rix0rrr merged commit ef09aba into aws:master Aug 23, 2019

hoegertn deleted the bastion-host branch August 23, 2019 12:29

rix0rrr mentioned this pull request Aug 28, 2019

Userdata not populated properly in CfnInstance #3419

Closed

5 tasks

clueleaf mentioned this pull request Jul 12, 2024

(ec2): BastionHostLinux should use AmazonSSMManagedInstanceCore policy #30834

Closed

2 tasks

WIP: feat(ec2): add instance resource #3697

WIP: feat(ec2): add instance resource #3697

Conversation

hoegertn commented Aug 17, 2019

mergify bot commented Aug 17, 2019

Pull Request Checklist

slipdexic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoegertn commented Aug 18, 2019

slipdexic commented Aug 18, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment