[jira] [Comment Edited] (MESOS-5405) Make fields in authorization::Request protobuf optional.
[
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, Option
2. Change objectApprover->approved() signature to accept an Option
(and adapt the logic in approved for the LocalAuthorizerObjectApprover to deal
with the None -> Any conversion)
{noformat}
Future authorized(const authorization::Request& request)
{
return getObjectApprover(request.subject(), request.action())
.then([=](const Owned& objectApprover) -> Future {
ObjectApprover::Object object(request.object());
Try 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, Option
2. Change objectApprover->approved() signature to accept an Option
(and adapt the logic in approved for the LocalAuthorizerObjectApprover to deal
with the None -> Any conversion)
```
Future authorized(const authorization::Request& request)
{
return getObjectApprover(request.subject(), request.action())
.then([=](const Owned& objectApprover) -> Future {
ObjectApprover::Object object(request.object());
Try 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)
[jira] [Comment Edited] (MESOS-5405) Make fields in authorization::Request protobuf optional.
[
https://issues.apache.org/jira/browse/MESOS-5405?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15288029#comment-15288029
]
Till Toenshoff edited comment on MESOS-5405 at 5/18/16 1:36 AM:
While my above approach would fix things, it would have a deep impact on the
conceptual design of the authorizer. It would also make us lose the ability to
add alternative {{value}} types - e.g. having a union styled {{object}}.
As an alternative, we could do the following:
1. fix all of those missing {{subject}} / {{object}} initializings.
2. {{CHECK(request->has_subject())}} and {{CHECK(request->has_object())}} to
{{authorizer:: authorized}} to make sure people do not fall into that trap
anymore.
was (Author: tillt):
While my above approach would fix things, it would have a deep impact on the
conceptual design of the authorizer. It would also make us lose the ability to
add alternative {{value}} types - e.g. having a union styled {{object}}.
As an alternative, we could do the following:
1. fix all of those missing {{subject}} / {{object}} initializings.
2. {{CHECK(request->has_subject())}} and {{CHECK(request->has_object())}} to
all {{authorizer:: authorized}} overloads to make sure people do not fall into
that trap anymore.
> 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: 0.29.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)
[jira] [Comment Edited] (MESOS-5405) Make fields in authorization::Request protobuf optional.
[
https://issues.apache.org/jira/browse/MESOS-5405?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15287889#comment-15287889
]
Till Toenshoff edited comment on MESOS-5405 at 5/17/16 11:53 PM:
-
Seems the correct solution here is to make {{subject}} and {{object}} optional
within the proto definition.
Keeping them mandatory would e.g. mandate a rather ugly fix for the first
example:
{noformat}
if (principal.isSome()) {
authRequest.mutable_subject()->set_value(principal.get());
} else {
authRequest.mutable_subject();
}
{noformat}
I firmly believe that this is not what we want for expressing {{ANY}} as used
by our internal representation whenever an empty / missing {{subject}} or
{{object}} got supplied.
Note that this bug was already present in the authorizer design document:
https://docs.google.com/document/d/1-XARWJFUq0r_TgRHz_472NvLZNjbqE4G8c2JL44OSMQ/edit?pref=2&pli=1
I just added a comment to point this out.
I would like to propose the following changes to {{authorizer.proto}}:
1. make the {{subject}} and {{object}} themselves optional:
{noformat}
message Request {
optional Subject subject = 1;
optional Action action = 2;
optional Object object = 3;
}
{noformat}
2. also make the {{subject}} and {{object}} {{value}} required to simplify the
underlying implementations:
{noformat}
message Subject {
required string value = 1;
}
message Object {
required string value = 1;
}
{noformat}
was (Author: tillt):
Seems the correct solution here is to make {{subject}} and {{object}} optional
within the proto definition.
Keeping them mandatory would e.g. mandate a rather ugly fix for the first
example:
{noformat}
if (principal.isSome()) {
authRequest.mutable_subject()->set_value(principal.get());
} else {
authRequest.mutable_subject();
}
{noformat}
I firmly believe that this is not what we want for expressing {{ANY}} as used
by our internal representation whenever an empty / missing {{subject}} or
{{object}} got supplied.
> 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
>Priority: Blocker
> Labels: mesosphere, security
> Fix For: 0.29.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)
