Subject: [kitten] AD review of draft-ietf-kitten-sasl-openid

From: Stephen Farrell <[email-masked]>
Date: Wed, 20 Jul 2011 16:27:04 +0100

Hi all,

I've had a read of this and reckon its nearly ok. While
I've a bunch of comments, I would expect they can be
handled fairly easily.

S.

Not-quite-major-but-close:

1) The use of TLS needs to be clarified throughout.
  - 5th para of intro says "It is anticipated that existing security
    layers, such as Transport Layer Security (TLS) [RFC5246], will
    continued to be used."
  - Figure 1 should I think show which channels MUST/SHOULD/MAY
    use TLS
  - 1.2 is ambiguous as to which channels MUST use TLS
  - bullet 9 on p7 says "over HTTP" - not clear if TLS needed
    here
  - Unnumbered figure before 2.1 says "- - - = HTTP or HTTPS"
    are both ok?
  - 5th para of section 4 says "applications MUST match the
    TLS server identity..."
  - the example in section 5 uses mixed http: and https: URIs
  - 6.2 RECOMMENDs that only "http or https" schemas (sic?) be
    accepted

Not all of those things need to change, but some may and it needs to be
crystal clear when TLS is a MUST|SHOULD|MAY to implement and to use and
when its ok to not implement or use TLS when its a SHOULD if there are
any of those. I think you probably only need to say once that
server-auth is a MUST when TLS is used, but you need to say that that
covers all uses of TLS, if that is in fact the case.

2) The fact that the GSS-API part is OPTIONAL needs to be more clearly
stated. The intro does that right I think but not in 2119 langauge
(which is only introduced in 1.1 anyway). Section 4 says "INFORMATIVE"
and "NORMATIVE" but those are not 2119 terms.

3) section 2, bullet 3 in 1st list, do clients here have to support DH
with the OP or not? Even if OpenID punts on this (I'm not sure) I think
you need to say here. (Text currently says "optionally.") Same point
for bullet 3 in 2nd list in section 2.

4) section 2, bullet 8 in 2nd list - as per (3) saying "optionally"
isn't clear enough. This entire list ought to be using 2119 terms
wherever possible I think. (Now, its a mixture.)

5) section 2, bullet 11, in 2nd list - this is the first time SREG is
mentioned, some description and a reference would be good even if this
is just an openid concept. Is SREG support really a MUST though?

6) section 2.1 is vague - saying "it may be necessary" doesn't make it
clear to me what the RP has to implement or use. How are they supposed
to know?

7) Last para of 2.2, are implementers supposed to add a transaction id
to a return_to URL or not? Who needs to decide/know/check? I think this
needs another sentence or two.

8) Do you really need both URI and XRI here? (3.1) If its not needed in
reality, why not just allow URIs? If it is needed, is it really clear
how to implement that? (I don't know, so I'm asking:-)

9) 3.2 says the SASL server MUST add a transaction id. That seemed to
be optional earlier - which is it? When is it ok for that to be
guessable or not resistant to attack? (I'd prefer a MUST there if that
was ok, hard to code up a SHOULD properly I'd have thought.)

10) 2nd last para of 3.2 - who's doing the rejection that the client
MUST handle here? (The OP I assume, but its not quite clear.)

11) 2nd last para of section 4 - I just don't get the mapping from the
openid to the GSS-API name. Maybe just adding examples would clarify
this enough.

12) Section 6, 1st para - "other literature" is too vague. Please add
informative references. (Nice-to-have: repeat the references to SASL
and GSS-API in this para too.)

13) In 6.3 would a reference to CORS help against CSRF? In any case, I
think some references are needed to help the programmer.

14) Is OpenID directed identity support a MUST or what? (6.5) If so,
what'd have to change in the client or RP? I think you need to say if
you're mentioning this as a mitigation for colluding RPs.

15) Please give URLs for the references that are not RFCs, mainly
OpenID, SREG1.0 and (if it stays) XRI2.0. (I apologise in advance if
the RFC editor makes you take 'em out again, but I think they should be
stable enough to include and are very useful.)

Places where I went "huh?"

H1) Intro: "This specification is appropriate for use when a browser is
available." Browsers have been continually available since ~1995, so I
think you need to state what you mean more clearly.

H2) 2nd last para of 2.2 - I don't get what kind of "alternative" you
mean - was this something considered but dropped during design or does
it need an if-statement in someone's code? (I can guess, but I don't
think I should have to guess.)

H3) All of 3.4;-) Why is something 4422 "explicitly prohibits" done
here? What does "lock step" mean? When does the client send a second
"=" message? (If that's what it says.) What is an "application level
outcome"?

H4) 5th para of section 4. I think you need to clarify who's playing
which TLS roles here, but I'm not quite sure that's all.

H5) Is the mitigation in 6.1 clear enough? What code wouuld I write
here? Maybe you mean that the mitigation depends on the OP
implementation but that'd not be ideal really.

H6) Last sentence of 6.3 needs something. (Maybe a re-write:-)

H7) All of section 7. I just don't get that, but is it worth including
in any case?

Nits:

N1) section 2, bullet 2 in 1st list, what is the "it" in "discovery on
it"

N2) section 2, 1st para after 1st list, suggest "must change their
code" to "have to change their code" to avoid 2119 confusion. Good to
do that throughout if you can and generally avoid "must," "should" and
"may" except for when you mean the 2119 terms.

N3) section 2, 2nd list intro - the steps are not "shown from" they are
listed.

N4) last para of section 2 (after the unnumbered figure) what is the
"it" in "Specifially, it processes..."

N5) s/insure/ensure/

N6) s/The astute/The astute [something]/

N7) s/shall/SHALL/ in 2nd para 3.3

N8) s/ambersand/ampersand/

N9) Add a reference for URL encoding in 3.3

N10) 4th para of section 4 is missing some MUSTs - "always have" is not
2119 language and s/alway /always /

N11) section 5, p16, where's the Line break that was added in Eliot's
email address for clarity? (I do see the full-stop that seems to have
been added at the end of that for obscurity:-)

N12) 6.2 s/should/SHOULD/ twice

N13) s/schemas/schemes/ in 6.2?

N14) 6.4 - s/will be track/will be able to track/

N15) 6.5 - 1st sentence - who is "you"?

N16) 6.5 - 2nd sentence - s/possible/made easier/

N17) id-nits doesn't like "http://openid.example" (I think) and
mentions an obsolete RFC being referenced, if you can check again
that'd be good. (3920->6120 is probably right to change.)