> On March 4, 2014, 7:29 p.m., Benjamin Hindman wrote: > > include/mesos/mesos.proto, lines 481-485 > > <https://reviews.apache.org/r/18730/diff/2/?file=509427#file509427line481> > > > > In line with Niklas' and Adam's comments, I agree we should be making > > ACLs more explicit. How about defining a message for each action? For > > example: > > > > message ACL { > > message RunAsUser { > > required string user = 1; > > } > > > > message AllocateFromRole { > > required string role = 1; > > } > > > > enum Action { > > RUN_AS_USER = 1, > > ALLOCATE_FROM_ROLE = 2 > > }; > > > > required string subject = 1; > > required Action action = 2; > > > > optional RunAsUser run_as_user = 3; > > optional AllocateFromRole allocate_from_role = 4; > > } > > > > This should still give us lots of flexibility while also being explicit > > (and type safe!). > > > > I can also imagine doing the same thing for 'subject', e.g.: > > > > message ACL { > > message Subject { > > optional string principal = 1; > > optional string user = 2; > > // Add more subjects as necessary. > > } > > required Subject subject = 1; > > ... > > } > > > > We could accomplish the same with another enum as well, but it's > > probably cleaner to use a nested message. > > > > Also, feel free to suggest different naming! > > Vinod Kone wrote: > I played around with the ACL format a bit and came up with the below. > This is inspire by how ZooKeeper implements ACLs > (https://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_ZooKeeperAccessControl). > > message ACL { > > message Object { > enum Type { > USER = 0; // For launching tasks as. > ROLE = 1; // For getting allocations for. > WWW = 2; // For accessing a web endpoint. > } > > required Type type = 1; > required string id = 2; > } > > message Subject { > enum Type { > PRINCIPAL = 0; > IP = 1; > HOST = 2; > ANYONE = 3; > } > > required Type type = 1; > optional string id = 2; // Can be optional iff type is ANYONE. > } > > enum Action { > LAUNCH_USER = 0; > ALLOCATE_ROLE = 1; > GET_WWW = 2; > PUT_WWW = 3; > } > > required Object object = 1; > required Subject subject = 2; > required Action action = 3; > } > > > I am assuming we can set up ACLs on Objects as follows: > > Object Subject Action > --------------------------------------------------------------------- > WWW:/index.html ANYONE GET_WWW > WWW:/stats.json IP:"10.0.0.0/24" GET_WWW > WWW:/framework/<foo>/tasks.json PRINCIPAL:"foo-user" GET_WWW > WWW:/drain.json PRINCIPAL:"admin" PUT_WWW > > ROLE:ads PRINCIPAL:"marathon" > ALLOCATE_ROLE > ROLE:* ANYONE > ALLOCATE_ROLE > > USER:root PRINCIPAL:"aurora" LAUNCH_USER > > > It's not yet clear how we validate that ACLs that are being set up don't > conflict with each other. > > Also, authorization itself might be a bit involved depending on the > formats we allow for "Subject.id". > > Authorizer->authorize(WWW:/stats.json, "10.0.0.12", "GET_WWW") --> > returns true > > Let me know what you think? > > > > > Niklas Nielsen wrote: > +1, Looks good. I don't think you can avoid conflicts all together; a > well known order in which the rules are applied should be enough? Conflict > detection will probably be a complicated piece of code. > > Benjamin Hindman wrote: > I like the explicitness Vinod, but I think we should "merge" the action > and object. I'm concerned that some combinations won't "make sense", and it > would be nice to enforce that with types. For example, right now we can do > subject PRINCIPAL:"aurora" action GET_WWW object ROLE:"aurora", which is an > error by construction. By merging the action and object we can get type > safety, i.e., > > message ACL { > message GetWWW { > repeated string url = 1; > } > > enum Action { > GET_WWW = 1; > ... > } > } > > Moreover, the ACLs should be easier to construct (setting one less > field). In fact, extending this all the way to avoid bad subject > action/object combinations would be beneficial too. As of now all subjects > might be candidates for all actions but I doubt that will be the case in the > future. > > Vinod Kone wrote: > message ACL { > > message GET_WWW { > message Subject { > enum Type { > IP = 1; > HOST = 2; > ANYONE = 3; > NONE = 4; > } > required Type type = 1; > optional string id = 2; // Required for IP/HOST. > } > > required Subject subject = 1; > repeated string urls = 2; > } > > message PUT_WWW { > message Subject { > enum Type { > IP = 1; > HOST = 2; > ANYONE = 3; > NONE = 4; > } > required Type type = 1; > optional string id = 2; // Required for IP/HOST. > } > > required Subject subject = 1; > repeated string urls = 2; > } > > message ALLOCATE_ROLE { > message Subject { > enum Type { > PRINCIPAL = 1; > ANYONE = 2; > NONE = 3; > } > required Type type = 1; > optional string id = 2; // Required for PRINCIPAL. > } > > required Subject subject = 1; > repeated string roles = 2; > } > > message LAUNCH_USER { > message Subject { > enum Type { > PRINCIPAL = 1; > ANYONE = 2; > NONE = 3; > } > required Type type = 1; > optional string id = 2; // Required for PRINCIPAL. > } > > required Subject subject = 1; > repeated string users = 2; > } > > repeated GET_WWW get_www = 1; > repeated PUT_WWW put_www = 2; > repeated ALLOCATE_ROLE allocate_role = 3; > repeated LAUNCH_USER launch_user = 4; > } > > > > This is what I came up with after feedback. Let me know what you think. > > I imagine we would have different overloads for Authorizer::authorize() > as follows: > > Authorizer::authorize(const ACL::GET_WWW& get_www); > Authorizer::authorize(const ACL::PUT_WWW& put_www); > Authorizer::authorize(const ACL::ALLOCATE_ROLE& allocate_role); > Authorizer::authorize(const ACL::LAUNCH_USER& launch_user); > > > Any suggestions for naming are also welcome. I particularly don't like > the repeated fields at the bottom to have singular names. One option is: > > repeated GET_WWW gets = 1; > repeated PUT_WWW puts = 2; > repeated ALLOCATE_ROLE allocates = 3; > repeated LAUNCH_USER launches = 4; > >
Actually lets have the design discussion in JIRA. I think it's more appropriate. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18730/#review36150 ----------------------------------------------------------- On March 4, 2014, 11:27 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18730/ > ----------------------------------------------------------- > > (Updated March 4, 2014, 11:27 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen. > > > Bugs: MESOS-911 > https://issues.apache.org/jira/browse/MESOS-911 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > include/mesos/mesos.proto 37f8a7fcd23d467b1274c46c405b836510afbd49 > src/Makefile.am 61d832b89132be2cc5b8ae9bbf743685464f78a4 > src/authorizer/authorizer.hpp PRE-CREATION > src/tests/authorization_tests.cpp PRE-CREATION > src/tests/master_contender_detector_tests.cpp > 8da7420e18c7a960b566fae13a5975857eb777ee > > Diff: https://reviews.apache.org/r/18730/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
