KantaraInitiative / wg-uma

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

No error for bad request body in FedAuthz RReg #354

Closed mrpotes closed 7 years ago

mrpotes commented 7 years ago

In section 3.2 of FedAuthz there is no error defined for when the request body cannot be handled - i.e. some form of 400 response.

mrpotes commented 7 years ago

There is also no statement of expected behaviour if the PAT is invalid/expired/missing/etc.

IDmachines commented 7 years ago

Is there a PAT validation endpoint?

mrpotes commented 7 years ago

The introspection endpoint could be used to validate the PAT, but I'm not sure why you ask...?

IDmachines commented 7 years ago

So what does that endpoint say when presented an invalid or expired PAT and might this be an example of expected behaviour.

mrpotes commented 7 years ago

I don't think so - the token introspection doesn't return errors for invalid tokens, it just reports {"active":false}

xmlgrrl commented 7 years ago

The FedAuth spec says 1) that the authorization server's protection API is OAuth-protected, 2) requires a valid PAT, and 3) that it has a security consideration about being susceptible to OAuth threats because of the PAT, so this implies/pulls in all of OAuth for success and error conditions surrounding how PAT handling works. Is that insufficient, do you think? We have a diagram that we've published on the wiki (from the UMA1 era) showing the relationship of OAuth endpoints to UMA-specific endpoints in the AS. We could update it for the UIG.

xmlgrrl commented 7 years ago

Responding on the "bad request" point now: In FedAuthz Sec 3.2, you're right, we define only how to respond to not_found and unsupported_method_type error conditions. Do we need to specifya corresponding 400 + bad_request error? Are we missing any others?

mrpotes commented 7 years ago

I wonder if we just need to reference the Bearer Token Usage spec?

ciseng commented 7 years ago

In Section 6, in the example we return 400 (meaning Bad Request). To represent missing resource_scopes in resource description, for example, I agree with James that we can adopt “invalid_request”. We can also rely on the other error codes from RFC 6750.

xmlgrrl commented 7 years ago

The example in Sec 6 is meant to illustrate an error that happens to be defined for permission requests, which gets a 400. The errors defined for resource registration happen to get only 404 or 405, but nothing is said (yet) about bad-request errors, which is the problem. So, so far I'm with you. But I'm not sure why it would be necessary to reference 6750 instead of 6749 (e.g. 6749 Sec 5.2) to pull in Bad Request semantics. In any case, can I ask for someone to propose wording?

And, is everyone satisfied on the PAT/OAuth question?

ciseng commented 7 years ago

I am fine with falling back on either.

Looking at RFC 6750, it has also an invalid_token error code. AS can use this error code, if the RS provided an invalid PAT. Since PAT is provided by RS to AS - token introspection returning active: false does not apply. PAT is not a token RS is sending to AS to check its validity. But, I am with you Eve, if you just want to say: "so this implies/pulls in all of OAuth for success and error conditions surrounding how PAT handling works”. This is fine with me too.

mrpotes commented 7 years ago

@xmlgrrl 6749 is about issuing oauth tokens, which is not what we are doing in the FedAuthz spec - instead we're using an access token (the PAT) as a bearer token, which is exactly what 6750 is about - we should actually be specifying this for both resource registration endpoint and permission endpoint, which both use PAT as Bearer tokens.

xmlgrrl commented 7 years ago

Aren't we using the PAT as, well, an access token for protecting the rreg and permission (and token introspection) endpoints, regardless of whether it's a bearer-style token (extremely likely because of its prevalence and standardization) or some other kind, like PoP?

I think we're mixing the two sub-issues mentioned in this issue a little bit. Just to be obnoxiously clear and prescriptive:

  1. This is the problem of the AS knowing what to do if the rreg endpoint (specifically) gets a request message with a bad/broken request body; this is a "semantic" error condition.
  2. This is the security-layer problem, protection API-wide, of whether we've said enough about how the AS should handle PATs.

Regarding sub-issue no. 2, we should of course be clear that it is the AS's responsibility to use OAuth properly to ensure that the client uses only valid PATs at all three endpoints and to return an error as appropriate otherwise. The existing spec language (as noted in my first comment upthread) does actually "pull in all of OAuth", but if it doesn't accomplish this, we could add a belt to the suspenders, perhaps in FedAuthz Sec 1.5 to the effect that:

"... The AS MUST return the appropriate [RFC6749] or [RFC6750] error in the case of an invalid or expired PAT."

Regarding sub-issue no. 1, this is about the request body during rreg, not about the PAT at all. Even though invalid_request is indeed both a 6749 error code and a 6750 error code, I'm thinking the right thing to do is "borrow" the name and invent a new error code for FedAuthz Sec 3.2, in a new bullet like this:

_"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed, the authorization server MUST respond with the HTTP 400 (Bad Request) status code and MAY respond with an invalid_request error message."_

mrpotes commented 7 years ago

Yes, that's the text I'm suggesting for subissue [2], although I'm just saying that RFC6749 isn't relevant here, only 6750, so:

"... The AS MUST return the appropriate [RFC6750] error in the case of an invalid or expired PAT."

Yes, agree with the proposed text to satisfy subissue [1].

xmlgrrl commented 7 years ago

James, let's please talk tomorrow so I can understand why the PAT has to be a bearer token. Otherwise, I think we're bearing down on closure. (I may only be available starting in the early evening UK time -- will do my best to be awake earlier!)

xmlgrrl commented 7 years ago

Okay, I plan to implement sub-issue no. 1 for the group to take a look at; it is a relatively "cheap" addition because FedAuthz errors don't have to be turned into registration requests or anything.

James and Cigdem and I have just discussed sub-issue no. 2 and I understand the challenge now. In UMA1, we defined metadata for pat_profiles_supported with a requirement to support (and document) bearer; in UMA2 so far, we removed that requirement, and only stated non-normatively that our examples show bearer-style PATs. Thus, we removed a requirement inadvertently, making the specs less interoperable. OAuth metadata has replacements for lots of the metadata we removed, but not for token types, and it certainly doesn't mandate bearer tokens. We need to add that back (in FedAuthz, because that's where the PAT lives now), as follows:

"The authorization server and the resource server (as an OAuth client) MUST support bearer usage of the PAT, as defined in [RFC6750]."

This doesn't prevent someone from putting, say, PoP support on top if they want to mutually negotiate it or whatever.