use DNS names instead of IP addresses in permissions (done, but what to call it?) #150

Open
bspratt opened this Issue Oct 17, 2012 · 7 comments

Comments

Projects
None yet
2 participants
@bspratt

bspratt commented Oct 17, 2012

I've implemented an alternative to cidr_ip that lets you lock down a port range to a DNS name. It resolves DNS and populates cidr_ip at startup (and yes, the cidr value becomes stale if the DNS record changes during cluster lifetime, but there you have it - AWS doesn't support DNS names in security groups).

Working name is "cidr_dns", which I don't love - anyone have an alternate suggestion?

@jtriley

This comment has been minimized.

Show comment Hide comment
@jtriley

jtriley Oct 18, 2012

Owner

@bspratt Awesome! Do you have the code in a branch somewhere I could take a look at? I'm assuming you always set the cidir range to /32 when populating cidr_ip?

I need to double check but I believe it's possible to add multiple cidr_ip ranges to a given security group rule and if that's the case we could allow cidr_ip to be a list and then make your new DNS option a list as well and call it 'allow_hosts' or something. What do you think?

Owner

jtriley commented Oct 18, 2012

@bspratt Awesome! Do you have the code in a branch somewhere I could take a look at? I'm assuming you always set the cidir range to /32 when populating cidr_ip?

I need to double check but I believe it's possible to add multiple cidr_ip ranges to a given security group rule and if that's the case we could allow cidr_ip to be a list and then make your new DNS option a list as well and call it 'allow_hosts' or something. What do you think?

@jtriley

This comment has been minimized.

Show comment Hide comment
@jtriley

jtriley Oct 18, 2012

Owner

or 'require_hosts'

Owner

jtriley commented Oct 18, 2012

or 'require_hosts'

@bspratt

This comment has been minimized.

Show comment Hide comment
@bspratt

bspratt Oct 18, 2012

Correct, you end up with a rule like this: Allow 80 (HTTP) 173.157.204.231/32

Yeah, I think "require_hosts" is good.

I like the idea of a cidr_ip list, although I think that might actually have to result in multiple rules - one CIDR per rule IIRC.

As long as we're messing with cidr_ip, what if it was allowed to accept DNS names (and we ditched the require_hosts thing)? They'd have to be structured somehow so that they weren't mistaken for security groups - maybe something like uri:mydomain.org .

I'll go ahead and push my current "cidr_dns" code up to my Github fork. Then I guess I have to enable you for poking around in there, I'll have to go figure out how that's done.

bspratt commented Oct 18, 2012

Correct, you end up with a rule like this: Allow 80 (HTTP) 173.157.204.231/32

Yeah, I think "require_hosts" is good.

I like the idea of a cidr_ip list, although I think that might actually have to result in multiple rules - one CIDR per rule IIRC.

As long as we're messing with cidr_ip, what if it was allowed to accept DNS names (and we ditched the require_hosts thing)? They'd have to be structured somehow so that they weren't mistaken for security groups - maybe something like uri:mydomain.org .

I'll go ahead and push my current "cidr_dns" code up to my Github fork. Then I guess I have to enable you for poking around in there, I'll have to go figure out how that's done.

@jtriley

This comment has been minimized.

Show comment Hide comment
@jtriley

jtriley Oct 18, 2012

Owner

@bspratt I just experimented and was able to add another CIDR to an existing rule so I think multiple CIDR's per rule is OK but I'll need to do some more testing. I'm fine with extending cidr_ip and not adding another option so long as the current implementation works for existing users. Given that we're resolving the DNS ourselves it should be easy to distinguish an IP from a dns name (via regex) from the config so I dont think the "uri:" prefix is necessary but I could be wrong.

Just simply pushing your branch/code is enough so that I can take a look at it. No need to enable anything...

Owner

jtriley commented Oct 18, 2012

@bspratt I just experimented and was able to add another CIDR to an existing rule so I think multiple CIDR's per rule is OK but I'll need to do some more testing. I'm fine with extending cidr_ip and not adding another option so long as the current implementation works for existing users. Given that we're resolving the DNS ourselves it should be easy to distinguish an IP from a dns name (via regex) from the config so I dont think the "uri:" prefix is necessary but I could be wrong.

Just simply pushing your branch/code is enough so that I can take a look at it. No need to enable anything...

@bspratt

This comment has been minimized.

Show comment Hide comment
@bspratt

bspratt Oct 18, 2012

Yeah, I hadn't checked to see if my fork was public or not, it is: https://github.com/bspratt/StarCluster.git

My only concern is that we can reliably tell URIs from security groups. I suppose security groups never have . in them, and URIs always do, and an actual IP CIDR is easy to match.

bspratt commented Oct 18, 2012

Yeah, I hadn't checked to see if my fork was public or not, it is: https://github.com/bspratt/StarCluster.git

My only concern is that we can reliably tell URIs from security groups. I suppose security groups never have . in them, and URIs always do, and an actual IP CIDR is easy to match.

@bspratt

This comment has been minimized.

Show comment Hide comment
@bspratt

bspratt Oct 18, 2012

OK, cidr_ip now handles actual CIDR references as well as URIs, and it handles multiple entries as a space seperated list. Pushed to my fork on github.

bspratt commented Oct 18, 2012

OK, cidr_ip now handles actual CIDR references as well as URIs, and it handles multiple entries as a space seperated list. Pushed to my fork on github.

@jtriley

This comment has been minimized.

Show comment Hide comment
@jtriley

jtriley Jan 4, 2013

Owner

This should be closed when #182 gets merged

Owner

jtriley commented Jan 4, 2013

This should be closed when #182 gets merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment