Open Stack

On 2/21/19 3: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).

I'm all for spending extra time to work through this if needed. Thanks
for raising it to the list.

>
> * 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.

I struggled with this name, but I didn't have a worth-while replacement
to suggest. I like using "access rules" but...

>
> 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.

... I think the final proposal here is a great idea, using "access
rules" for both with one having a slightly more specific derivative,
tailored either for operators or users. The fact we have an association
between the two via naming is better than what we had with "template"
and "capability".

> * 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.

I agree about the awkwardness and complexity, but I do want to clarify.
Using the example above, going with * and ** would mean that tokens
generated from that application credential would be actionable on any
project tag for the project the application credential was created for
and not just 'foobar'.

For an initial implementation, I think that's fine. Sure, creating an
application credential specific to a single server is ideal, but at
least we're heading in the right direction by limiting its usage to a
single API. If we get that right - we should be able to iteratively add
filtering later*. I wouldn't mind re-raising this particular point after
we have more feedback from the user community.

* iff we need to

>
> * 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?

Not that I can think of, but I'd like to hear an operator or user chime
in. The example brought up in IRC advocating for the attribute was:

- I need to create an application credential that "something" can use to
create a server, but that "something" can't create ephemeral storage

Otherwise, maybe we can meet in the middle by deciding a sane default
behavior and implementing the toggle later? Regarding the name, it's not
really clear what we're chaining. But, after re-reading the details in
the specification, it's clear this was specific to "service chaining".
I'll have to dig my naming hat out of the closet to come up with
something more useful...

>
> 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.

It would have taken me way longer to distill this from Gerrit. Thanks
again for taking the time to write it up.

>
> 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
>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20190221/f4834365/attachment.sig>