Re: [OAUTH-WG] AD review of draft-ietf-oauth-bearer-13

[Mike Jones <[e-mail]>]

Thanks for the careful read, Stephen.  I've attached a proposed draft -14 that addresses your comments.  (Per your other note, if you let me know how I can submit this during the submission blackout period based upon your approval as an Area Director, I will gladly do so.)

Replies are inline below.


-----Original Message-----
From: [e-mail]<mailto:[e-mail]> [mailto:[e-mail]]<mailto:[mailto:[e-mail]]> On Behalf Of Stephen Farrell
Sent: Wednesday, November 02, 2011 9:45 AM
To: [e-mail]<mailto:[e-mail]>
Subject: [OAUTH-WG] AD review of draft-ietf-oauth-bearer-13





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?

Agreed, this sentence was unnecessary - deleted.


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.

Done


3) Where's it say what to do with a scope attribute presented by a server?

Added "In some cases, the scope value will be used when requesting a new access token with sufficient scope of access to utilize the protected resource."


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;-)

Spec now describes the realm parameter and states that it is used as defined by HTTPbis-p7.


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.

The spec does not allow this, as realm (and the other parameters) are defined as a quoted strings.


6) 3.1, invalid_token - the client MAY retry, SHOULD it do that in an infinite loop? Probably not;-)

The spec says that "The client MAY request a new access token and retry the protected resource request."  So this isn't just a mechanistic re-do; multiple protocol messages would be exchanged to request and present a new access token and retry and user interactions may occur in the process, so it's not likely that this is going to be placed into a simple "while" loop.  While you're right that a badly written program might retry ad infinitum, I don't think it's likely;  therefore, I'm not sure that it's the place of the spec to warn developers against writing an infinite loop in this particular case, as competent developers will avoid them in all cases.  But if you strongly disagree, please provide specific suggested language to include.


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.

Good - done


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.

Added statement that "TLS is mandatory to implement and use with this specification" to the introduction.  (It was already present in 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.

Done - added "As a further defense against token disclosure, the client MUST validate the TLS certificate chain when making requests to protected resources" to 4.2.


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.

Clarified that putting a validity time field inside the protected part of the token is one means, but not the only means, of limiting the lifetime of the token.


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.

Agreed - Dropped the confusing phrase "for instance, through the use of TLS" from the sentence about confidentiality protection of the exchanges in in the 2nd to last paragraph.


12) Why reference 2818 instead of 6125?

Changed, per your suggestion.


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;-)

Added this new paragraph to the Threat Mitigation section: "In some deployments, including those utilizing load balancers, the TLS connection to the resource server terminates prior to the actual server that provides the resource.  This could leave the token unprotected between the front end server where the TLS connection terminates and the back end server that provides the resource.  In such deployments, sufficient measures MUST be employed to ensure confidentiality of the token between the front end and back end servers; encryption of the token is one possible such measure."


14) Why are cookies first mentioned in 4.3? Seems like that should have been done earlier.

Added this new paragraph to the Threat Mitigation section: "Cookies are typically transmitted in the clear.  Thus, any information contained in them is at risk of disclosure.  Therefore, bearer tokens MUST NOT be stored in cookies that can be sent in the clear."


nits:



abstract: maybe s/granted resources/the associated resources/?

Done


abstract: s/the bearer token needs to/bearer tokens need to/?

Done


1.2: s/abstraction layer/abstraction/ I don't see any layer there

Done


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.

Done (although the reference "Representation and Verification of Domain-Based Application Service Identity within Internet Public Key Infrastructure Using X.509 (PKIX) Certificates in the Context of Transport Layer Security (TLS) [RFC6125]" is quite a mouthful!)


2.1: s/follows/as follows/

Done


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.

Statements about when each of the methods SHOULD be used are made in the last paragraph of each of 2.1, 2.2, and 2.3.  I looked into moving all of them to the first paragraphs to keep the presentation parallel, but it hurt readability for 2.2 and 2.3.  Thus, I made no change here.


end of p7, s/attribute MUST NOT/attributes MUST NOT/

Done


4.2, s/recommended/RECOMMENDED/ is better but they mean the same already!

Done

_______________________________________________

OAuth mailing list

[e-mail]<mailto:[e-mail]>

https://www.ietf.org/mailman/listinfo/oauth

                                                                Thanks again,
                                                                -- Mike