Open Stack

On Thu, Feb 21, 2019, at 10:11 PM, Colleen Murphy wrote:
> I have an initial draft of application credential capabilities 
> available for
> review[1]. The spec[2] was not straightforward to implement. There were 
> a few 
> parts that I found unintuitive, user-unfriendly, and overcomplicated. 
> The
> current proposed implementation differs from the spec in some ways that 
> I want
> to discuss with the team. Given that non-client library freeze is next 
> week, 
> and the changes we need in keystonemiddleware also require changes in 
> keystoneclient, I'm not sure there is enough time to properly flesh 
> this out
> and allow for thorough code review, but if we miss the deadline we can 
> be ready
> to get this in in the beginning of next cycle (with apologies to 
> everyone waiting
> on this feature - I just want to make sure we get it right with minimal 
> regrets).
> 
> * Naming
> 
> As always, naming is hard. In the spec, we've called the property that is
> attached to the app cred "capabilities" (for the sake of this email I'm going
> to call it user-created-rules), and we've called the operator-configured list
> of available endpoints "permissible path templates" (for the sake of this email
> I'm going to call it operator-created-rules). I find both confusing and 
> awkward.
> 
> "Permissible path templates" is not a great name because the rule is actually
> about more than just the path, it's about the request as a whole, including the 
> method. I'd like to avoid saying "template" too because that evokes a picture
> of something like a Jinja or ERB template, which is not what this is, and 
> because I'd like to avoid the whole string substitution thing - more on that
> below. In the implementation, I've renamed the operator-created-rules to
> "access rules". I stole this from Istio after Adam pointed out they have a
> really similar concept[3]. I really like this name because I think it
> well-describes the thing we're building without already being overloaded.
> 
> So far, I've kept the user-created-rules as "capabilities" but I'm not a fan of
> it because it's an overloaded word and not very descriptive, although in my
> opinion it is still more descriptive than "whitelist" which is what we were
> originally going to call it. It might make sense to relate this property
> somehow to the operator-created-rules - perhaps by calling it
> access_rules_list, or granted_access_rules. Or we could call *this* thing the 
> access rules, and call the other thing allowed_access_rules or
> permitted_access_rules.
> 
> * Substitutions
> 
> The way the spec lays out variable components of the URL paths for both
> user-created-rules and operator-created-rules is unnecessarily complex and in
> some cases faulty. The only way I can explain how complicated it is is to try 
> to give an example:
> 
> Let's say we want to allow a user to create an application credential that
> allows the holder to issue a GET request on the identity API that looks like
> /v3/projects/ef7284b4-3a75-4570-8ea8-b30214f18538/tags/foobar. The spec says
> that the string '/v3/projects/{project_id}/tags/{tag}' is what should be
> provided verbatim in the "path" attribute of a "capability", then there should
> be a "substitutions" attribute that sets {"tag": "foobar"}, then the project_id
> should be taken from the token scope at app cred usage time. When the 
> capability is validated against the operator-created-rules at app cred creation
> time, it needs to check that the path string matches exactly, that the keys of
> the "substitutions" dict matches the "user template keys" list, and that keys
> required by the "context template keys" are provided by the token context.
> 
> Taking the project ID, domain ID, or user ID from the token scope is not going
> to work because some of these APIs may actually be system-scoped APIs - it's
> just not a hard and fast rule that a project/domain/user ID in the URL maps to
> the same user and scope of the token used to create it. Once we do away with
> that, it stops making sense to have a separate attribute for the user-provided
> substitutions when they could just include that in the URL path to begin with.
> So the proposed implementation simply allows the wildcards * and ** in both the 
> operator-created-rules and user-created-rules, no python-formatting variable
> substitutions.
> 
> * UUIDs
> 
> The spec says each operator-created-rule should have its own UUID, and the end
> user needs to look up and provide this UUID when they create the app cred rule.
> This has the benefit of having a fast lookup because we've put the onus on the
> user to look up the rule themselves, but I think it is very user-unfriendly. In
> the proposed implementation, I've done away with UUIDs on the
> operator-created-rules, and instead a match is looked up based on the service
> type, request path and method, and the "allow_chained" attribute (more on that
> next). Depending on how big we think this list of APIs will get, this will have
> some impact on performance on creation time (not at token validation time).
> 
> UUIDs make sense for singleton resources that are created in a database. I
> imagine this starting as an operator-managed configuration file, and maybe some
> day in the future a catalog of this sort could be published by the services so
> that the operator doesn't have to maintain it themselves. To that end, I've
> implemented the operator-created-rules driver with a JSON file backend. But
> with this style of implementation, including UUIDs for every rule is awkward -
> it only makes sense if we're generating the resources within keystone and
> storing them in a database.
> 
> * allow_chained
> 
> The allow_chained attribute looks and feels awkward and adds complexity 
> to the code
>  Is there really a case when either the user or operator would not want 
> to allow a
> service to make a request on behalf of a user who is making a more 
> general request?
> Also, can we find a better name for it?
> 
> Those are all the major question marks for me. There are some other minor
> differentiations from the spec and I will propose an update that makes it
> consistent with reality after we have these other questions sorted out.
> 
> Colleen
> 
> [1] 
> https://review.openstack.org/#/q/topic:bp/whitelist-extension-for-app-creds
> [2] 
> http://specs.openstack.org/openstack/keystone-specs/specs/keystone/stein/capabilities-app-creds.html
> [3] 
> https://istio.io/docs/reference/config/authorization/istio.rbac.v1alpha1/#AccessRule

We discussed this a bit on IRC[4] and I've proposed a revision to the spec[5] to capture the proposed alterations.

Colleen

[4] http://eavesdrop.openstack.org/irclogs/%23openstack-keystone/%23openstack-keystone.2019-02-21.log.html#t2019-02-21T21:13:50
[5] https://review.openstack.org/639182