KantaraInitiative / wg-uma

This is the repository of all specifications related to the User Managed Access Group
http://kantarainitiative.org/confluence/display/uma/
Other
27 stars 21 forks source link

Editorial items from Justin #323

Closed xmlgrrl closed 7 years ago

xmlgrrl commented 7 years ago

@jricher suggests (this is from a larger list, out of which I've already broken out and submitted the items that are non-editorial and that may need to be discussed):

uma grant

1. "protected-resource access" probably shouldn't be hyphenated

1.3

the first diagram doesn't make sense w/o the diagram from 6749, probably better to remove the referential aspects ("replaces resource owner", eg)

the second diagram is very hard to read with all the text and (ex.) bits throughout --maybe it's trying to show too much in this format? consider making this one into a couple stripped down alternates

"client is not aware of permissions" should be "permissions are part of the authorization server's process and are opaque to the client"

add forward reference to 1.3.1 from authorization process bullet -- though these seem to contradict. The definition is about the client's request, the subsection is about the AS's internal process.

including "the impetus for the client's request" seems weirdly over philosophical to me, I would drop this bullet and incorporate the first bullet into the text

1.3.1

Seems weird having claims gathering happen first especially given diagrams above. Better to move it later and note that it could happen first as an exception. Better yet to get rid of the "phase" terminology with looping and ordering all together.

2

Defining the "claims_Redirect_uri" requirement underneath the "Registration_endpoint" bit is confusing structure. That should be in its own section or at least paragraph

3.3.1

claim_token text should read: "MUST be base64url encoded unless specified otherwise by the claim token format"

claim_token_Format examples aren't examples of the values in question, but rather what they would represent -- we should define the strings representing these if we want to use them here

3.3.3

mention of PCT is awkward, suggest rewording: "The authorization server MAY request authorization from the RqP for persisting claims across authorization processes. Such persisted claims will be represented by a PCT issued to the client in a subsequent step."

3.3.4

All the "Note:"s in the second bullet make it impossible to follow the thread, suggest pulling them out into discussion text

RPT upgrading should be its own section, maybe?

remove fedauthz reference fromthe middle of the example

the default deny text has been mangled and needs to be rewritten: key point, don't default a new policy to "no claims needed" and then have "no claims provided" be sufficient for it. This might be better in a forward reference to a security consideration

3.3.5

needs to explicitly call out the 6749 token endpoint response here at the beginning

3.3.6

invalid_scope should also cover cases where the client asked for a scope it's not registered for

"engage, or engage the requesting party," is redundant, suggest simplifying to "option to perform claims gathering as specified in [3.3.2]"

3.4

explicitly say to use 6750 here for bearer tokens and any other presentation mechanism for other token types

3.5

section title makes it sound like RPT is in response, reword to say "authorized request" or "rpt-enabled request" or something?

multiple references to fedauthz here, need at most one

remove "this concludes"

5.1.1

the PCT attack is important but gets buried here, I'd put it in the front of the paragraph and follow it with the explanation. "The authorization server has no way of confirming which user is in charge of a client at a given time, and a PCT issued for one user could be used for a different user." There's no mitigation server side, it's the client's responsibility to ensure that the right user is doing things. This is the same as the client using the "wrong" access token for a given user. It's the client's job to make sure it's doing the right thing in both cases.

5.1.2

PCT doesn't "carry extra power" so much as "allow non-interactive issuance of RPTs"

5.2

suggest removing the philosophical discussion of bearer and pop, just stating that non-bearer tokens are outside this specification but are assumed to be used in a manner similar to their oauth counterparts, just like bearer tokens are

5.4.1

OIDC IdP case could be simplified into: "Some deployments could have exceptional circumstances allowing the AS to validate claim tokens. For example, if the UMA AS is also the IdP for the RqP, then the AS would be able to validate any ID tokens it issued to the client and ensure they were issued to the client presenting the claim token."

6.1

I don't think there's any "primary" privacy duty -- it either respects privacy or it doesn't.

========

FedAuthz

1

Requirement to "use in its entirety" should be redundant from the spec's point of view -- by default you do the whole spec or you don't care

1.5

offline access mode should go with the lifetime management discussion

3.1

move description of uri-based scope requirements entirely to 3.1.1

3.2

"authorization server-assigned" should probably be "authorization-server-assigned"

reiterate here that all calls are protected by a PAT and all examples use bearer tokens in 6750 format

example of "rreguri/" mismatches examples of "/rs/" below

3.2.1

normative text doesn't mention the location header

normative text doesn't mention 201/created

3.2.4

normative text doesn't mention 204/no-content

3.2.5

get rid of braces around _id, makes it look like a json object

3.5

this example is incredibly detailed, is all of this really necessary? all the normative steps already have examples. This feels more like it should be a blog post than in the spec.

"standard OAuth resource registration API" should be "UMA resource..."

4

we say "may have zero scopes" here but previously resources must have "one or more" scopes associated with them

4.2

explicitly point out that a single ticket is returned in all cases, even for multiple permissions

5.1

example path of /rs/status clashes with examples in 3.2.x, suggest we just use /introspect here

we should be clear about allowing a server to support both UMA-extended and non-UMA introspection requests and responses

5.1.1

instead of "the former overrides the latter", be explicit: "the token level value takes precedence"

6

this section doesn't define format. you don't add "parameters" to an entity body, but you can add members to a JSON object that's carried in an entity body.

8.1

this should be more explicit about leaking user-identifying data. The example of "it's a health record API and it's pointing to Alice's personal AS" is one that I found resonates with people.

xmlgrrl commented 7 years ago

Per UMA telecon 2017-05-18: Incorporate.

xmlgrrl commented 7 years ago

1.3

the first diagram doesn't make sense w/o the diagram from 6749, probably better to remove the referential aspects ("replaces resource owner", eg)

the second diagram is very hard to read with all the text and (ex.) bits throughout --maybe it's trying to show too much in this format? consider making this one into a couple stripped down alternates

In rev 04 this was already pretty heavily edited based on similar feedback I was getting, so I haven't done anything more in 05. There's now a single diagram with a rationalized swimlane, and the first diagram is gone.

xmlgrrl commented 7 years ago

1.3.1

Seems weird having claims gathering happen first especially given diagrams above. Better to move it later and note that it could happen first as an exception. Better yet to get rid of the "phase" terminology with looping and ordering all together.

This has always been a tough section. I elided the "phase" bits (which contained nothing normative or new anyway), and that helped. In concert with the new swimlane above, which now highlights the permission ticket rotation, I think this section has better added value.

xmlgrrl commented 7 years ago

3.3.1

_claim_tokenFormat examples aren't examples of the values in question, but rather what they would represent -- we should define the strings representing these if we want to use them here

I've revised this wording to say these are examples of potential types of claim token formats. We already agreed to keep our OIDC code example below the text as non-normative.

xmlgrrl commented 7 years ago

3.3.6

_invalidscope should also cover cases where the client asked for a scope it's not registered for

Our set math simply eliminates this as an option for being included in RequestedScopes; we haven't defined this as an error condition per se. In other words, we need not have gone to the trouble of specifying the (ClientRequested ∩ ClientRegistered) portion in Sec 3.3.4 if we added this to the error. So I think no action is needed for this item.

xmlgrrl commented 7 years ago

5.1.1

the PCT attack is important but gets buried here, I'd put it in the front of the paragraph and follow it with the explanation. "The authorization server has no way of confirming which user is in charge of a client at a given time, and a PCT issued for one user could be used for a different user." There's no mitigation server side, it's the client's responsibility to ensure that the right user is doing things. This is the same as the client using the "wrong" access token for a given user. It's the client's job to make sure it's doing the right thing in both cases.

(Note that some reorganization has resulted in this being a different section number, Sec 5.2 so far. CSRF is now Sec 5.1.) I'm thinking that requesting party switching within a single authorization process is pretty well covered by the second and third bullets we've got (involving the AS), while the PCT attack is definitely in the realm of the client's responsibility.

Finally, the little bit that we say about RPT (access token) exposure to the requesting party might need a little more said about it -- at least to point out that it's more dangerous when the RqP isn't the same party as the RO, but in fact we provide no particular mitigations for it (as we agreed when we discussed it a month or so back, since OAuth doesn't). Thoughts?

xmlgrrl commented 7 years ago

5.2

suggest removing the philosophical discussion of bearer and pop, just stating that non-bearer tokens are outside this specification but are assumed to be used in a manner similar to their oauth counterparts, just like bearer tokens are

(This is now Sec 5.4.) Have added a reference to 6750 (with a companion mention of doing the right thing if you're using non-bearer tokens) earlier, as you'd suggested, I see the point of this. But I'm wondering if it now needs to be said at all here in the security considerations, being fairly obvious.

xmlgrrl commented 7 years ago

Per UMA telecon 2017-05-25:

Issue #323: Regarding 3.3.6: invalid_scope should also cover cases where the client asked for a scope it's not registered for: In Sec 3.3.4, mention that if the client requests a scope that it didn't pre-register for, it's not an error (at the RPT request stage) because, during the authorization assessment process, the RequestedScopes might include scopes requested on the client's behalf by the RS. However, it is not included as a requested scope. Issue #323: s/other entity other/any entity other/ Issue #323: PoP: Let's keep as is.