I like the implementation; I think it walks a good line between being uncumbersome but quickly auditable for missing authz on services.
- Dan On Tue, Nov 1, 2016 at 11:51 AM, Brock Noland <[email protected]> wrote: > This looks really straightforward IMO. I think as a dev if you are > modifying the wire protocol, understanding this should be no sweat. > > On Mon, Oct 31, 2016 at 9:24 PM, Todd Lipcon <[email protected]> wrote: > > > I whipped up a patch for something somewhat like option 3 here: > > http://gerrit.cloudera.org:8080/4897 > > > > Please take a look if you're interested. > > > > -Todd > > > > On Mon, Oct 31, 2016 at 4:41 PM, Todd Lipcon <[email protected]> wrote: > > > > > Hey folks, > > > > > > After https://gerrit.cloudera.org/#/c/4765/ finishes code review, the > > > Kudu daemons will have support for Kerberized RPC. If we start the > > servers > > > with --server_require_kerberos, then they'll require that all RPC > clients > > > authenticate themselves via Kerberos. > > > > > > Of course authentication isn't that useful on its own. I think the next > > > step along this direction is to add basic "internal service" > > authorization: > > > > > > - the master should only allow tablet server service principals to > > > heartbeat/register/etc > > > - the tserver should only allow master service principals to > > create/delete > > > tablets, etc. (with the possible extension to also allow 'root' users > to > > do > > > so) > > > > > > I think there are a couple patterns we could follow here to enforce the > > > authorization > > > > > > *Option 1) Try to enforce ACLs on a per-RPC-service level* > > > > > > This is more or less the approach that Hadoop takes. We can make sure > > that > > > different RPC use cases are separated into different RPC "services", > and > > > apply an ACL on each service. For example, we can separate the current > > > 'MasterService' into 'TserverToMasterService' and > > 'ClientToMasterService', > > > and then register ACLs on each such service when we start the RPC > server. > > > > > > Pros: > > > - when adding a new method, it "automatically" gets the right ACL > without > > > having to remember to add special checks > > > - enforces some nice separation and grouping of RPCs > > > > > > Cons: > > > - doesn't handle special cases well, like 'DeleteTablet is usually > called > > > by the master, but we also want to allow admins to call the same method > > for > > > use in manual recovery tools' > > > - would require a bunch of RPC methods to be moved around, and likely > > > break wire compatibility (I believe the RPC header currently specifies > > the > > > actual service name for each method call) > > > > > > *Option 2) Entirely ad-hoc authorization checks* > > > > > > At the top of each RPC method implementation, we can manually check the > > > authorization. Basically, imagine something like this: > > > > > > bool CheckTabletServerRequest(RpcContext* rpc) { > > > // if 'rpc' is from a valid tablet server, return true > > > // otherwise, respond to the RPC with an authz error and return false > > > } > > > > > > void MasterServiceImpl::TSHeartbeat(const TSHeartbeatRequestPB* req, > > > TSHeartbeatResponsePB* resp, > > > rpc::RpcContext* rpc) { > > > if (!CheckTabletServerRequest(rpc)) return; > > > } > > > > > > > > > Pros: > > > - 100% flexible - easily allows for request-specific special cases > > > - relies on no "magic", it's obvious when you look at the code how the > > > authz check is applied. > > > > > > Cons: > > > - easy to forget to check authorization when adding new RPCs. I recall > a > > > CVE or two in the past where we forgot to add the appropriate check in > > > various internal Hadoop RPCs. > > > > > > > > > *Option 3) Per-method authorization checks, declared in .proto file* > > > > > > In the .proto file describing a service, we can add a custom option > like: > > > > > > rpc TSHeartbeat(TSHeartbeatRequestPB) returns > (TSHeartbeatResponsePB) > > { > > > option (kudu.authz = "tablet-servers-only") > > > }; > > > > > > When we start the RPC system, we can bind these magic strings to actual > > > method implementations, and give an error if any RPC method either > > mentions > > > an invalid authz policy, or doesn't mention an authz policy at all. > > > > > > Pros: > > > - we can require that all methods specify a policy (even if they > specify > > > 'none'), making it impossible to forget to authorize > > > - still retains flexibility: we can always create an exotic policy > > > implementation for an exotic method as necessary (or specify 'none' and > > do > > > the custom check from option 2) > > > - we can allow the same protobuf option to be specified as a > service-wide > > > 'default' for the cases where an entire service has the same authz > policy > > > > > > Cons: > > > - maybe a little more confusing to developers who have to go looking at > > > the .proto file to see which authorization policy is being applied to a > > > method? > > > > > > *Option X? Any other options I missed?* > > > - I only spent 30m or so thinking about this, so maybe I missed a > better > > > option? Feedback appreciated. > > > > > > > > > I'm leaning towards option 3 here, but would appreciate feedback > before I > > > go too far down that path. > > > > > > -Todd > > > -- > > > Todd Lipcon > > > Software Engineer, Cloudera > > > > > > > > > > > -- > > Todd Lipcon > > Software Engineer, Cloudera > > >
