Hi,
Good work - another one almost out the door! Thanks. However, I think this one needs a revised ID before we start IETF LC. Nothing hard to change I hope, but I think there are enough changes to make that its best done that way. I reckon items 3,5,7-11 and 13 below need fixing, but are I hope all easy. Not sure about item 4. All my other comments can be considered in conjunction with whatever other IETF LC comments we get, or now, whichever. If you want to argue that the WG already had strong consensus against a change I'm asking for, or that I'm just being dumb, (which happens all the time:-) then please do that and we can discuss it on the list and/or at the meeting. Regards, Stephen. questions/comments: 1) What does the 1st sentence of section 2 mean? What is the 2119 MAY for? Couldn't that sentence be deleted? If not, why not? 2) I think you should warn implementers in 2.3 that using the query string is fairly likely to result in the access token being logged, which is highly undesirable. (That is there later too, but I think deserves to be here.) What does "the only feasible method" mean? I think that needs to be defined, as was done in 2.2. 3) Where's it say what to do with a scope attribute presented by a server? 4) What is the realm attribute in section 3? What is a client expected to do with that? I guess it has to be different from how realm is used with e.g. Basic. (That might be my ignorance of HTTP details though;-) 5) Section 3 ABNF allows "realm=foo;realm=bar;scope=baz;error=123" is that ok? Is processing clear for all cases? I don't think it is. 6) 3.1, invalid_token - the client MAY retry, SHOULD it do that in an infinite loop? Probably not;-) 7) "Assuming" token integrity protection is wrong. You need to make it a MUST. That is, you need to say that resource servers MUST only accept tokens with strong integrity or similar. 8) I think you need to say that TLS is MTI and MUST be used, (i.e. with 2119 language) and it'd be better to put that in the introduction as well as 4.2. 9) As-is 4.2 allows anonymous D-H TLS ciphersuites. I don't think you want that, but yet you only call for ciphersuites that "offer confidentiality." I think that needs to be tightened up. 4.3 does tighten there, but I think 4.2's language also needs fixing. 10) The token validity doesn't have to be "inside." I could e.g. change a token MAC verification key every hour and limit lifetime that way. 11) Two paragraphs in 4.2 contradict one another. 3rd last para say you MUST use TLS, 2nd last para says you MUST have confidentiality "for instance...TLS." I'd ditch the second one I think, but something needs fixing. 12) Why reference 2818 instead of 6125? 13) I think you need to say something here about load balancers and other server side things that terminate TLS, before the resource server and behind which bearer tokens are unprotected. At least say that tokens MUST be protected there and provide guidance as to how to do that well. Lots of people do that badly I think. (At least at first;-) 14) Why are cookies first mentioned in 4.3? Seems like that should have been done earlier. nits: abstract: maybe s/granted resources/the associated resources/? abstract: s/the bearer token needs to/bearer tokens need to/? 1.2: s/abstraction layer/abstraction/ I don't see any layer there 2.1: I (and others) dislike the use of the reference as if it were part of the sentence, e.g. "defined by [I-D.whatever],..." Better would be "defined as part of HTTP authentication [I-D.whatever]" There are multiple occurrences of this. 2.1: s/follows/as follows/ 2.1, last para: I think the SHOULD in the last para of 2.1 and the corresponding rules in 2.2 & 2.3 would be better stated up front. end of p7, s/attribute MUST NOT/attributes MUST NOT/ 4.2, s/recommended/RECOMMENDED/ is better but they mean the same already! _______________________________________________ OAuth mailing list [email protected] https://www.ietf.org/mailman/listinfo/oauth
