Closed copumpkin closed 8 years ago
Hey,
First of all, thanks starting work on this :)
Would you mind giving a quick explanation of how this new feature is supposed to work so the code is easier to judge? For example, specify that we're adding a lookup on ldap for certain fields $foo and $bar and what would happen if they're not present, etc.
For example, it seems like we're getting the arns from the "url" field of the grop and that at this moment this is not optional? It sounds like this is something that at the very least we should be able to opt out (ideally opt in really).
Also, should we (could we) maybe want to provide support from some default arn(s) that all groups could access? (that would prevent having to add it to every single group)
Thanks for taking a look! Yeah, sorry for being unclear: I definitely wouldn't expect this to be merged today, since you can't turn it on/off or even configure which attribute it looks for. There are also no tests :smile:
Anyway, here's the scheme:
url
(multi) attribute in those groups to find allowed ARNsWe could easily make url
configurable (and perhaps the absence of that configuration could indicate that authorization is turned off) and I plan to do that, but I mostly wanted feedback on the general approach and perhaps how to better structure the code. I understand that there are several different ways that one might want to perform authorization and don't want this one to become the way to do it. I don't know how best to abstract over different auth backends in Go but that's ultimately what I want. I could see that being tricky both in terms of code structure and in terms of backend-specific configuration (the url
LDAP attribute name probably wouldn't be applicable to other backends)
As far as defaults, I could easily add that as another configuration option.
Thanks for the explanation! I think I started focusing on details too soon :)
I think this approach makes sense, I don't think this is going to make switching auth backends significantly harder even if we need to move things around at that point. The way I would probably structure this in terms of config is to get the config setting for the url field inside the ldap-specific configuration, and a toggle for enabling/disabling the arn checks in the general config, that way you could add non-ldap backends and reuse that part of the config.
Regarding the enumeration of arns, I think that's definitely the way to go, anything else would be prone to overcomplicating things. The pragmatic approach for the administrators case would be to add some sort of special case and account for it (like adding "*" to the list would give you access to everything)
The pragmatic approach for the administrators case would be to add some sort of special case and account for it (like adding "*" to the list would give you access to everything)
That got me thinking: a regex could be interesting:
arn:aws:iam::.*:role/Foo
: use role Foo
in all accountsarn:aws:iam::1234567890:role/.*
use any role in a specific accountAnd would allow people to get more interesting properties, obviously. Overcomplicated?
Agreed on your suggestions about the config structure! I'll try to update the PR soon.
Closed due to #62 superseding it.
This implements a simple form of the scheme I described in #14. It's still a work in progress but I figured I'd put it up here to see if anyone had any feedback on it.
To use it, set up LDAP groups with the
url
field (I'll make it configurable but it served the purpose for now and didn't require me finding a suitable OID) containing role ARNs you want members of that group to be able to assume. Now when you add people to the group, they'll be able to use the associated ARNs.I also added a little bit of cleverness around how you specify roles to make it easier to request them in different accounts. I now support these syntaxes:
hologram use foo
hologram use <amazon-account-id>:role/foo
hologram use <full-role-arn>
Please tear it to shreds! My main questions:
Obviously I still need to add tests :smile:, but I wanted some early feedback if anyone had any.