[jira] [Commented] (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=15318223#comment-15318223
]
Till Toenshoff commented on MESOS-5405:
---
{noformat}
commit 90871a48f4f1a345950862a53efb78e0b9aadedb
Author: Joerg Schad
Date: Tue Jun 7 11:34:53 2016 +0200
Fixed documentation for MESOS-5405.
As MESOS-5405 changes the fields in `Request` to optional, we need to
update the documentation.
Review: https://reviews.apache.org/r/48263/
{noformat}
> 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] [Commented] (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=15316326#comment-15316326
]
Joerg Schad commented on MESOS-5405:
Added documentation and tests:
Fixed documentation for MESOS-5405.
https://reviews.apache.org/r/48263/
Added test for optional request.object field.
https://reviews.apache.org/r/48264/
> 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] [Commented] (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=15308861#comment-15308861
]
Joerg Schad commented on MESOS-5405:
https://reviews.apache.org/r/48101/
> 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] [Commented] (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=15308765#comment-15308765
]
Till Toenshoff commented on MESOS-5405:
---
sgtm
> 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] [Commented] (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=15308764#comment-15308764
]
Till Toenshoff commented on MESOS-5405:
---
Additional work was done belonging to this issue:
Added {{Request}} sanity checks in {{LocalAuthorizer}}:
https://reviews.apache.org/r/48085/
Updated comments in authorizer.proto.: https://reviews.apache.org/r/48093/
Note that the latter tries to supercede https://reviews.apache.org/r/47876 - by
borrowing some inspirations from it - thanks [~adam-mesos]!
> 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] [Commented] (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=15308700#comment-15308700
]
Joerg Schad commented on MESOS-5405:
[~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] [Commented] (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=15307005#comment-15307005
]
Alexander Rojas commented on MESOS-5405:
Well, I share Adam's opinion. Not to mention that one of the purposes of the
design was to make the calling logic somewhat friendlier and easier to track
and going for the second approach doesn't achieve that goal.
> 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] [Commented] (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=15306930#comment-15306930
]
Alexander Rukletsov commented on MESOS-5405:
cc [~arojas]
> 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] [Commented] (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=15305242#comment-15305242
]
Adam B commented on MESOS-5405:
---
[~tillt] I think I'm favoring [~alexr]'s approach of making subject and object
optional (to handle object-less requests, like ACCESS_MESOS_LOG), leaving their
values optional (to handle union-like objects), and then fixing all the
accesses to handle missing subject/object/value correctly.
But maybe missing object doesn't mean the same as a missing object.value (even
if the other fields are also missing). We could allow each action to define
whether or not it requires an object, since some like ACCESS_MESOS_LOG might
actually want to check that there is never an object set. And then, if the
action allows/requires an object, we can define an object with no fields set
(including no optional FrameworkInfo/ExecutorInfo/TaskInfo fields) as an ANY
object.
> 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] [Commented] (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=15305238#comment-15305238
]
Adam B commented on MESOS-5405:
---
[~tillt] Should we review the above patch or [~alexr]'s preferred approach:
https://reviews.apache.org/r/47505
> 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] [Commented] (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=15304858#comment-15304858
]
Artem Harutyunyan commented on MESOS-5405:
--
[~adam-mesos] Can you take a look at this one please? It's marked as a blocker
for the release.
> 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] [Commented] (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=15288594#comment-15288594
]
Alexander Rukletsov commented on MESOS-5405:
I'd rather vote for the original solution for the following reasons:
* proto3 does not support required fields, if we want to migrate eventually
we will have to get rid of them anyway;
* the design is still error-prone: checks you've added do not protect if
local authorizer is not used
* the code is not concise; we require people to call certain function only in
order to please proto rules
To overcome the drawback of your first solution, how about you keep fields in
{{Subject}} and {{Object}} optional? This way, we can still upgrade to a union
styled {{object}} if we would like to. I don't see why this change deeply
impacts the design of the authorizer. We are allowed to express {{ANY}}, with
the change you proposed, there will be _just_ another way to express it.
Basically, we already do it everywhere in the codebase, we just legalize it.
Moreover, while absence of a {{subject}} and {{subject}} with absent {{value}}
mean the same thing now, we may even want to change it in the future, so this
gives us an extra degree of freedom.
> 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] [Commented] (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=15288136#comment-15288136
]
Till Toenshoff commented on MESOS-5405:
---
https://reviews.apache.org/r/47509
> 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] [Commented] (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=15288098#comment-15288098
]
Till Toenshoff commented on MESOS-5405:
---
Got a fix for the second variant at hand - reviewboard currently down - will
upload ASAP.
Need a shepherd.
> 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] [Commented] (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=15288046#comment-15288046
]
Vinod Kone commented on MESOS-5405:
---
Fixing the call sites sgtm.
> 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] [Commented] (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=15288029#comment-15288029
]
Till Toenshoff commented on MESOS-5405:
---
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] [Commented] (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=15287889#comment-15287889
]
Till Toenshoff commented on MESOS-5405:
---
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)
