-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
AWS: Inbound IP whitelist #97
base: dev
Are you sure you want to change the base?
Conversation
New config param `instances.parameters.inboundIp`. Allows users to specify an inbound IP address or range, which is then used to whitelist all ports, including 22, in the instance's security group. By default this is set to 0.0.0.0/0 and therefore not whitelisted.
netaddr.IPNetwork used as part of the schema validation and type for inbound_ip param.
}] | ||
# add ports to the security group for the user-defined inbound IP | ||
inbound_ip = instance_config.inbound_ip | ||
cidr_ip = 'CidrIpv6' if inbound_ip == 6 else 'CidrIp' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you forgot to add a proper check for IPv6 addresses here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops! This is what happens when I don't do the unit tests - maybe the templates could to with some tests to ensure the args are at least building them correctly...
Forgot to call the .version on the IPNetwork object
This reverts commit 4fb6d74. This re-introduces spaces at the end of each markdown line
Single quotes for strings Trailing commas for multiline arrays
@apls777 Thank you for the feedback, I have made all the requested changes. |
'CidrIpv6': '::/0', | ||
'IpProtocol': 'tcp', | ||
'FromPort': port, | ||
'ToPort': port, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that now if the inboundIp
parameter is not specified, we're not opening ports for IPv6. So, I think we need to use None
for the default value instead of 0.0.0.0/0
to know when the parameter wasn't specified and open ports for both protocols.
@@ -30,6 +31,8 @@ def validate_instance_parameters(params: dict): | |||
), | |||
Optional('managedPolicyArns', default=[]): [str], | |||
Optional('instanceProfileArn', default=None): str, | |||
Optional('inboundIp', default='0.0.0.0/0'): Use(IPNetwork, error='"inboundIp" should be a valid IP ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the default value, the inbound_ip.version == 6
check will fail later in the code as the Use(IPNetwork, ...
function is not applied. It can be fixed with the IPNetwork('0.0.0.0/0')
value, but as I mentioned in another comment, we need to use None
instead to know when the value wasn't set at all.
@@ -28,6 +28,7 @@ def test_default_configuration(self): | |||
'spotInstance': False, | |||
'subnetId': '', | |||
'volumes': [], | |||
'inboundIp': '0.0.0.0/0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed as well when the default inboundIp
value is changed.
New config param
instances.parameters.inboundIp
.Allows users to specify an inbound IP address or range, which is then
used to whitelist all ports, including 22, in the instance's security
group. By default this is set to 0.0.0.0/0 and therefore not
whitelisted.
Resolves #96