> On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 1388
> > <https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1388>
> >
> >     You can check whether the primitive field is set, which will 
> > distinguish between default int value and an explicitly set value.
> >     
> >     I think it makes sense to reject zero at this layer.

Done.


> 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);
> 
> Maxim Khutornenko wrote:
>     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.
> 
> Kevin Sweeney wrote:
>     Operations scoped to an object owned by a role will not be enforcable 
> with pure AOP, as many will typically require a storage lookup (usually by 
> ID) to determine the owner of the object being operated on.
>     
>     Operations that happen on "global" like `"snapshot:create"` will be 
> enforceable with pure AOP.

Refactored to use UPDATE_COORDINATOR role with a user auth as fallback. The 
authorization is matching the provided credentials (SessionKey) against the 
operational intent (JobUpdateKey). The validation of JobUpdateKey matching the 
actual stored data is deferred to application layer where a missing content 
will generate an INVALID_REQUEST response (or JobUpdatePulseStatus.FINISHED as 
in the current case).


- 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