nasa / utm-apis

The collection of APIs for NASA's UTM project in the form of OpenAPI documents.
55 stars 35 forks source link

Update fims-authz.yaml. #178, adding request for aud #181

Closed nasajoey closed 4 years ago

nasajoey commented 4 years ago

Used "resource" as the field name in the token request to indicate the aud that is desired in the resulting token.

This request is not standardized, so this is a design decision.

arkits commented 4 years ago

Looks good! Can you include aud in the JwtClaimsSet's schema (line 299)?

nasajoey commented 4 years ago

Thanks @arkits ! If you have a few mins, can you see if the images came out ok? They should show on the README still if you are in the right branch.

issmith1 commented 4 years ago

Joey, you can take-on mods for the test client... just an idea /fims-authz/modules/fims-authz-sig/test_client/token_from_fims_az_v1.py

nasajoey commented 4 years ago

Will leave this pull request open until USS have some time to look at it.

nasajoey commented 4 years ago

@BenjaminPelletier Sounds like two issues overall:

  1. the format of the aud claim and it's request value... currently url, suggesting hostname or nothing maybe.
  2. the request field name (currently resource, suggesting audience).

Does that summarize the comments above well enough?

BenjaminPelletier commented 4 years ago

@BenjaminPelletier Sounds like two issues overall:

  1. the format of the aud claim and it's request value... currently url, suggesting hostname or nothing maybe.
  2. the request field name (currently resource, suggesting audience).

Does that summarize the comments above well enough?

Yes, I think so, and the resource question is not important -- I tend to think audience or similar would be clearer, but resource will work perfectly fine.

An additional note on the content of the aud claim: there shouldn't be a security issue with any kind of content. As such, I would suggest no whitelist as the whitelist reduces agility and flexibility while requiring additional infrastructure to track and validate accepted values for the list. Also, I would recommend no format requirement as that provides flexibility to adopt a different convention later (without any changes to the auth server). A maximum length is probably reasonable though :)

nasajoey commented 4 years ago

Will discuss with team, but fine with updating resource to audience. Also comfortable relaxing format of aud claim value to initially being a hostname. This will match the proposed approach to naming entities within the USS Network in the future anyway.

Regarding whitelist or vetting of the audience request by the authorization server, I think it will be a larger discussion for sure. I see a couple sides to it. Maximum flexibility for a USS to define any audience makes sense for agility. However, it can lead to overuse of the auth server for its intended purposes. Also, there is an expired RFC draft that discusses audience. Obviously since it is just an old, expired RFC draft, it isn't binding, but it does note:

If the authorization server does not consider the resource server acceptable then it MUST return an error response with the error code "access_denied".

So, the current implementation will leave some of that flexibility for the current activities, but will need to be decided after that how vigilant the auth server needs to be in vetting audiences. I don't have any insight beyond that, so don't want to make the decision now.

nasajoey commented 4 years ago

will have to update the images once more due to change to audience.

issmith1 commented 4 years ago

RE: aud claim value to initially being a hostname

Should this instead be API basename? My readings indicate aud value must indicate the API, not the hostname alone. API to aud must be one-to-one. For example, one server at hostname offers both api1 and api2 (hostname/api1, hostname/api2). In particular, if api2 offers elevated privs endpoints, the aud check is a needed separation.

Just noticed @arkits has same comment as this.

nasajoey commented 4 years ago

API basename makes sense to me.

issmith1 commented 4 years ago

Here, a sidenote on API Basename and elevated privs. We found during Tcl4 testing that creating separate endpoints for elevated privs (ep) rather than overloading the same endpoint with multiple scopes, worked quite well. What would also work well is a separate API for ep endpoints; in this case aud would provide yet another nice check.