-----Original Message-----
From: oauth-boun...@ietf.org [mailto:oauth-boun...@ietf.org] On Behalf
Of Stephen Farrell
Sent: Thursday, October 13, 2011 10:13 AM
Suggested non-trivial clarifications:
-------------------------------------
(1) 1.3.4 - "previously arranged" might trigger discusses on the document
since it implies that this spec might not be suited for broad use. I think that
making it clear that the normal mode for client developers is to work against
an existing service (AS and resource server) would help to clarify that such
arrangements are ok here.
Added new 'Interoperability' section to the introduction:
OAuth 2.0 provides a rich authorization framework with well-defined
security properties.
However, as a rich and highly extensible framework with many
optional components, this
specification is likely to produce a wide range of non-interoperable
implementations.
In addition, this specification leaves a few required components
partially or fully
undefined (e.g. client registration, authorization server
capabilities, endpoint
discovery).
This protocol was design with the clear expectation that future work
will define
prescriptive profiles and extensions necessary to achieve full
web-scale
interoperability.
There is no way to sugar coat reality and hopefully by being blunt about it
upfront, we will avoid a prolonged debate about the protocol's failings in that
regard.
(2) p11, in step (F) is there a way to distinguish between an access token that
is invalid due to expiry vs. e.g. data corruption? Section 6 refers to 5.2 for
the
error codes but its not clear to me which one is returned for this case. I think
clarifying that in section 6 or 5.2 is needed.
That depends on the token specification. Steps C-F are outside the scope of
this document. I'll note that.
(3) p13, 2.2 and 2.3 - these things happens at registration time right? I think
making that clear is needed since we don't specify a registration protocol
here.
The entire section 2 is 'Client Registration' which is described as out of
scope for implementation details.
(4) 2.3.1 uses the term "token endpoint" without definition (its defined in
section 3) and in particular without making it clear if both access and refresh
token issuance is covered (I guess it is).
Changed to 'when sending requests using password authentication'.
(5) The same text about x-www-form-urlencoded is repeated various times,
it'd be better to do that once and refer to it where necessary.
Not enough to be worth the change.
(6) 3.1.2.2 states the rules for when AS'es are to require registration of
redirection URIs. I think you need to clarify that some. First, you use the term
"redirection_uri" for both a "complete" URI and for a scheme/authority/path
triple that can be added to via a query component which is confusing.
Second, its overall a very complex rule with a MUST, two MAYs and 3
SHOULDs. I do think it could be made clearer by putting the MUST up front
and separating issues related to complete URI and triples separately from the
when something MUST be registered.
New text:
The authorization server MUST require the following clients to
register their redirection endpoint:
o Public clients.
o Confidential clients utilizing the implicit grant type.
The authorization server SHOULD require all clients to register their
redirection endpoint prior to utilizing the authorization endpoint
The authorization server SHOULD require the client to provide the
complete redirection URI (the client MAY use the "state" request
parameter to achieve per-request customization). If requiring the
registration of the complete redirection URI is not possible, the
authorization server SHOULD require the registration of the URI
scheme, authority, and path (allowing the client to dynamically vary
only the query component of the redirection URI when requesting
authorization).
The authorization server MAY allow the client to register multiple
redirection endpoints.
Lack of a redirection URI registration requirement can enable an
attacker to use the authorization endpoint as open redirector as
described in Section 10.15.
(7) 4.2.1 and elsewhere - refers back to 3.1.2 for the way in which the
redirection-uri is OPTIONAL - I'm not sure that's sufficiently clear, 3.1.2 is
quite long and discusses a bunch of things - couldn't it be made clearer when
the messages are defined?
The reference is not for the OPTIONAL definition. I changed the coma to a
period.
More generally, is there no way to avoid the
extensive cross-referencing in the message field definitions? E.g. 4.2.2 has
references to 7.1 and 3.3, and others are similar. Organizing the text for the
benefit of the reader is a good thing so would it be worthwhile to do an
editing pass for just this purpose - to reduce the number of forward
references and minimize the number of pointers in general?
We've gone back and forth on this for 22 drafts and this is the best balance
between readability and consistency we found. I'm inclined to avoid another
reshuffle.
On a similar note, 4.1& 4.2 call out specific errors, but 4.3 defers to 5.2,
why?
Be good if that could be made more consistent at the TOC level.
Because 4.3 doesn't use the authorization endpoint.
(8) How can the AS protect against brute force attacks in 4.3.2? I think you
could give a bit more guidance, e.g. say to rate-limit or generate alerts or
whatever, but not as normative text, just good hints.
Ok.
(9) In 10.12 you say how a client can protect against CSRF via the state field
but you don't say how the AS can do the same in order to satisfy the MUST in
the last para of 10.12. Can you not add a hint or reference here?
The WG sentiment was that server developers do not require as much hand holding
as the client developers. If the authors of the original text want to propose
text, I'm happy to include it.
Some nits: (Stuff that seemed more serious at first:-)
---------
(1) In 2.3.1 I think you're ruling out putting the client_id and client_secret
in
the request URL - is that right? If so, that's good, but I think it needs
calling
out since people do that all the time and it'd be good to say why its bad to do
that.
Added:
The parameters can only be transmitted in the request body and MUST
NOT be included in the
request URI
(2) The redirection endpoint is introduced in 3.2.1 but is not listed at the
start
of section 3 which only lists two endpoints.
I think you mean 3.1.2. Added.
(3) In 4.1.2 what does it mean to "attempt" to revoke tokens? Why can't the
AS just revoke them? (Where revoke == not accept them when they are
next presented, right?)
Can't revoke self-encoded tokens (e.g. stateless on the server). Changed
'attempt' to 'when possible'
(4) I think this is just language but wanna check. 4.1.2.1 and
4.2.2.1 errors have a state field. Text says: "If a valid "state"
parameter was present..." which would imply that state variables are either
valid or invalid according to the AS. I don't think that's the case, and nor
should it be. (If it were, I'd have a security concern that I could use
otherwise
crap requests to probe for good state values.) s/valid// I think?
Yep.
(5) I don't get the benefit of saying the client SHOULD ignore unknown fields
in the response in 4.2.2 - what's effectively the difference between that and
"MUST ignore" - I don't get it, and hence don't see why you don't say MUST
ignore.
Changed all 'ignore' to MUST.
(6) Why say "MUST NOT issue a refresh token" in 4.2.2? Are you making an
assumption that access& refresh tokens are distinguishable to anyone other
than the issuer? If not, then is this just saying "don't make a mistake"?
No. It's a security concern that developer might decide to be flexible and
provider a refresh token in addition to an access token using this flow.
(7) 5.1 says that the client SHOULD ignore "unrecognized response
parameters" - does that mean unrecognized parameters in the JSON entity
body? Is it clear enough as-is that those are "response parameters"?
Changed 'parameters' to 'value names in the response'.
(8) How does the use of TLS on endpoints used for end-user interaction
"reduce the risk of phishing attacks"? I don't get that. Maybe you mean that
TLS+users actually checking server identity reduces the risk of successful
phishes? I think that's a bit different. (I do like the MUST though.)
Dropped 'phishing'. If the authors of the original text would like to explain,
I'll put it back in.
(To be continued...)
EHL
