> On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 1535
> > <https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535>
> >
> >     This is too restrictive.  It means that _only_ the update coordinator 
> > role may call this RPC, and that a user cannot build something to pulse 
> > their own updates.
> >     
> >     You should instead do what `isAdmin` does and fall back to the 
> > coordinator role if direct auth fails.
> >     
> >     This is an unfortunate state of affairs, and hopefully the move to 
> > shiro dramatically improves all this.
> 
> Maxim Khutornenko wrote:
>     I don't see how it's necessarily better. Pulsing can always be done under 
> UPDATE_COORDINATOR membership, which is specifically covering heartbeats 
> only. The isAdmin() requires ROOT that opens up everything else, including 
> the ability to kill anyone's tasks in the claster. Isn't that more 
> restrictive in real life? We don't expect regular users having ROOT access, 
> meaning they will unlikely to get to write their own pulse routine due to 
> that. 
>     
>     Or maybe you suggesting an approach similar to killTasks(), where an 
> admin check followed by a role validation? The problem here is that we don't 
> have a job key to extract the role for authorization. Perhaps we can change 
> the RPC to accept a job key instead but that would open up for a race we 
> don't want to see (e.g. late pulse arrives for a wrong update ID). We could 
> probably get away with both updateId and jobKey in the API to avoid 
> ambiguity, or perhpas just updateId and role? I am open to suggestions.
> 
> Kevin Sweeney wrote:
>     In Shiro this will look something like
>     
>     subject.checkPermission("update:resume:mesos:prod:labrat");
>     
>     With role update_coordinator having:
>     ```
>     update:*
>     ```
>     
>     role root having:
>     ```
>     *
>     ```
>     
>     and user mesos having:
>     ```
>     *:*:mesos
>     ```
>     
>     So they'd all be allowed.
> 
> Bill Farner wrote:
>     > Or maybe you suggesting an approach similar to killTasks(), where an 
> admin check followed by a role validation?
>     
>     Yeah, this.  I'm not asking for update heartbeats to require ROOT.
>     
>     > The problem here is that we don't have a job key to extract the role 
> for authorization.
>     
>     We have an association between {{updateId}} and role owning the job, 
> right?
>     
>         /**
>          * Fetches a read-only view of a job update.
>          *
>          * @param updateId Update ID to fetch.
>          * @return A read-only view of job update.
>          */
>         Optional<IJobUpdate> fetchJobUpdate(String updateId);

We do. However, it would require accessing the store in order to authenticate 
the call. This is a new pattern we have not tried before and it may potentially 
interfere with moving to pure AOP auth.


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/#review70655
-----------------------------------------------------------


On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30325/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2015, 5:23 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
> 
> 
> Bugs: AURORA-1009
>     https://issues.apache.org/jira/browse/AURORA-1009
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin 
> interface to support AOP capability validation. 
> 
> The RB is diffed against https://reviews.apache.org/r/30225/
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 4f603f9e7ed004e53937a45ea8edf7241e15f5cf 
>   src/main/java/org/apache/aurora/auth/CapabilityValidator.java 
> 45ef643ebe57c1517cdae373574331ea302a8b74 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  8c19f3b08135eb5f3098591ebf9931b42a086318 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  03d1fba76c23570c2c4102a48daf5ce035ecaaa3 
> 
> Diff: https://reviews.apache.org/r/30325/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to