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

From: Stephen Farrell <[email-masked]>
Date: Wed, 02 Nov 2011 16:45:21 +0000

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!

---

From: Julian Reschke <[email-masked]>
Date: Wed, 02 Nov 2011 19:02:03 +0100

On 2011-11-02 17:45, Stephen Farrell wrote:
> ...
> ...

Is it different? If it is, it MUST NOT be called "realm".

Best regards, Julian

---

From: "Manger, James H" <[email-masked]>
Date: Thu, 3 Nov 2011 10:21:52 +1100

> is that ok? Is processing clear for all cases? I don't think it

The ABNF does not allow that.
It requires commas as separators, not semi-colons.
It requires double quotes around values.
The only possible ambiguity in this example is the duplicate realms, but that parameter isn't even defined in this spec (it is defined in draft-ietf-httpbis-p7-auth)! I guess that spec could try to explicitly define behaviour in the case of this particular error, but it may have to explicitly describe a lot of other error cases as well.

--
James Manger

---

From: Julian Reschke <[email-masked]>
Date: Thu, 03 Nov 2011 09:49:51 +0100

> It requires commas as separators, not semi-colons.

Indeed.

> It requires double quotes around values.

(but maybe it should not; separate issue)

> The only possible ambiguity in this example is the duplicate realms, but that parameter isn't even defined in this spec (it is defined in draft-ietf-httpbis-p7-auth)! I guess that spec could try to…

Indeed. I have opened a ticket for that 
<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/321>. UAs disagree on 
it, so I think all that needs to be done here is to point out that it's 
invalid.

Best regards, Julian