[ 
https://issues.apache.org/jira/browse/MESOS-5405?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15308700#comment-15308700
 ] 

Till Toenshoff edited comment on MESOS-5405 at 5/31/16 9:46 PM:
----------------------------------------------------------------

[~tillt] [~adam-mesos] [~mcypark]
This breaks some assumptions of the current `authorized` interface which assume 
`subject` and `object` are set (see below).

In order to accomodate for this these new optional fields i would propose the 
following 
1. Change getObjectApprover's signatures to accept Option<subject>, Option 
<action>
2. Change objectApprover->approved() signature to accept an Option<object>
(and adapt the logic in approved for the LocalAuthorizerObjectApprover to deal 
with the None -> Any conversion)

{noformat}
  Future<bool> authorized(const authorization::Request& request)
  {
    return getObjectApprover(request.subject(), request.action())
      .then([=](const Owned<ObjectApprover>& objectApprover) -> Future<bool> {
        ObjectApprover::Object object(request.object());
        Try<bool> result = objectApprover->approved(object);
        if (result.isError()) {
          return Failure(result.error());
        }
        return result.get();
      });
  }
{noformat}



was (Author: js84):
[~tillt] [~adam-mesos] [~mcypark]
This breaks some assumptions of the current `authorized` interface which assume 
`subject` and `object` are set (see below).

In order to accomodate for this these new optional fields i would propose the 
following 
1. Change getObjectApprover's signatures to accept Option<subject>, Option 
<action>
2. Change objectApprover->approved() signature to accept an Option<object>
(and adapt the logic in approved for the LocalAuthorizerObjectApprover to deal 
with the None -> Any conversion)

```
  Future<bool> authorized(const authorization::Request& request)
  {
    return getObjectApprover(request.subject(), request.action())
      .then([=](const Owned<ObjectApprover>& objectApprover) -> Future<bool> {
        ObjectApprover::Object object(request.object());
        Try<bool> result = objectApprover->approved(object);
        if (result.isError()) {
          return Failure(result.error());
        }
        return result.get();
      });
  }
```

> Make fields in authorization::Request protobuf optional.
> --------------------------------------------------------
>
>                 Key: MESOS-5405
>                 URL: https://issues.apache.org/jira/browse/MESOS-5405
>             Project: Mesos
>          Issue Type: Bug
>            Reporter: Alexander Rukletsov
>            Assignee: Till Toenshoff
>            Priority: Blocker
>              Labels: mesosphere, security
>             Fix For: 1.0.0
>
>
> Currently {{authorization::Request}} protobuf declares {{subject}} and 
> {{object}} as required fields. However, in the codebase we not always set 
> them, which renders the message in the uninitialized state, for example:
>  * 
> https://github.com/apache/mesos/blob/0bfd6999ebb55ddd45e2c8566db17ab49bc1ffec/src/common/http.cpp#L603
>  * 
> https://github.com/apache/mesos/blob/0bfd6999ebb55ddd45e2c8566db17ab49bc1ffec/src/master/http.cpp#L2057
> I believe that the reason why we don't see issues related to this is because 
> we never send authz requests over the wire, i.e., never serialize/deserialize 
> them. However, they are still invalid protobuf messages. Moreover, some 
> external authorizers may serialize these messages.
> We can either ensure all required fields are set or make both {{subject}} and 
> {{object}} fields optional. This will also require updating local authorizer, 
> which should properly handle the situation when these fields are absent. We 
> may also want to notify authors of external authorizers to update their code 
> accordingly.
> It looks like no deprecation is necessary, mainly because we 
> already—erroneously!—treat these fields as optional.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to