[ https://issues.apache.org/jira/browse/MESOS-5405?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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)