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

[Stephen Farrell <[e-mail]>]

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!