Re: [Gen-art] Gen-ART LC Review of draft-ietf-kitten-sasl-saml-06

[Stephen Farrell <[e-mail]>]

Hi Klaas,

Looks like those would justify a revised ID. If you've
time to do that in the next week then please go ahead
and shoot out a new version. IESG folk won't have
started reviewing it before then probably and this
review would probably generate a discuss or comment
we'd want to fix anyway.

Cheers,
S.

PS: I cc'd Sam Hartman who's the assigned secdir
reviewer. Be good if you can let him know if you
plan to do a new rev before he does his review.

On 01/06/2012 09:53 AM, Klaas Wierenga wrote:
> Ben,
>
> Thanks for your thorough review!
>
> Klaas
>
> On Jan 5, 2012, at 11:32 PM, Ben Campbell wrote:
>
>>
>> I am the assigned Gen-ART reviewer for this draft. For background on Gen-ART, please see the FAQ at<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>>
>> Please resolve these comments along with any other Last Call comments you may receive.
>>
>> Document: draft-ietf-kitten-sasl-saml-06
>> Reviewer: Ben Campbell
>> Review Date: 2012-01-05
>> IETF LC End Date:2012-01-05
>> IESG Telechat date: (if known)
>>
>> Summary: This draft is on the right track for publication as a proposed standard. However, there are a few minor issues, and sufficient editorial issues to make the document difficult to understand.
>>
>> *** I noticed shortly before sending this that the authors submitted version 07 today. I have not reviewed that version--this review refers to version 06.
>>
>> Major issues:
>>
>> None
>>
>> Minor issues:
>>
>> -- section 3.2, last paragraph:  "… needs to correlate the TCP session from the SASL client with the SAML authentication."
>>
>> Please elaborate on this correlation
>>
>> -- section 4, 2nd paragraph: "The SAML Idenity Provider does not have a role in GSS-API, and is considered an internal matter for the OpenID mechanism."
>>
>> Please elaborate, as this is the only mention of OpenID in the draft. It seems out of context.  (If OpenID was in fact intended, please include a citation?)
>>
>> -- section 4, 3rd and 4th paragraph (paragraph a and b)
>>
>> These seem like protocol affecting differences. If so, they need elaboration, such as normative statements and formal definitions, or references to such.
>>
>> -- section 5, general:
>>
>> The section seems to need further elaboration or references
>>
>> -- section 6.1, step 5 (alt)
>>
>> Please describe the circumstances where the alternative step would occur. (also, please spell out "alternative")
>>
>> -- section 6.1, step 6: "The client now sends the URL to a browser for processing."
>>
>> Isn't that a question of software design rather than protocol? (unless you mean something more abstract by "browser", in which case it would help to say so.)
>>
>> Also, this section compresses the interaction with the identity provider. Why not show the details for those steps like the others? (If you mean them to be out of scope, then why give as much detail as you do?)
>>
>> -- section 6.2:
>>
>> The draft does not provide the decoding of the Base64 encoded parts like it did in 6.1, Without the decodes, the example is not very illuminating.
>>
>> -- section 7.4: "This is an option the user has to understand and decide to use if the IdP is supporting it."
>>
>> I doubt end users will read this spec. What does this mean for implementors?
>>
>>
>>
>> Nits/editorial comments:
>>
>> -- General:
>>
>> There are a lot of editorial and organizational issues, some of which created difficulty in understanding the authors' intent. I noted a number of these below, but I doubt I caught everything. I suggest this draft have another detailed proofreading and editing pass before it progresses.
>>
>> -- Pagination is strange throughout the document. (Mostly blank pages, etc.) It's worse in the PDF version, but still not right in the text version.
>>
>> -- section 1.1, 2nd paragraph:
>>
>> Repeating the SAML 2.0 citation here would be helpful.
>>
>> -- section 1.2, 1st paragraph
>>
>> The sentence structure makes it ambiguous whether the other side of the or should be just certificate validation, or TLS with cert validation.
>>
>> -- section 2, list starting in 2nd paragraph:
>>
>> Please leave a blank line between list items. Without it, you get a wall-of-text effect that's difficult to read.
>>
>> -- section 2, list item 3:
>>
>> Is "service provider" a well defined term as used here?
>>
>> -- section 2, paragraph after 1st numbered list: "This will be discussed below. The steps are shown from below:"
>>
>> Redundant sentences. Also, I assume the discussion has already been written, so "will be" is not correct.
>>
>> -- section 2, 2nd paragraph after 2nd numbered list: "… flow appears as follows:"
>>
>> Please explicitly mention the figure number. E.g. "… flow appears as shown in Figure XX."
>>
>> -- section 2, figure 2:
>>
>> The numbers in parentheses are confusing. We just saw a numbered list that sort of corresponds to this flow, but the numbers don't match up. I realize later sections refer back to these numbers, but without some mention of that, a reader is almost certainly going to try to correlate this to the previous list.
>>
>> -- section 3, 1st paragraph: "Recall section 5 of [RFC4422] for what needs to be described here."
>>
>> That reads like an author's "to do" note to himself. Has the needed description been completed, or does it still need to be described?
>>
>> -- section 3, 2nd paragraph: "The name of this mechanism "SAML20"."
>>
>> Missing word?
>>
>> -- "(via "gs2-header")"
>>
>> Missing word?
>>
>> -- section 3, 3rd paragraph: "The first mechanism message from the client to the server is the "initial-response" described below."
>>
>> Please explicitly mention section.
>>
>> -- section 3, 4th paragraph: "The second mechanism message is from the server to the client, the "authentication- request" described below."
>>
>> I can't parse this sentence--are there missing words? Also, please mention section instead of saying "below".
>>
>> -- section 3.1, first paragraph:
>>
>> Is "authorization identifier" the same as "IdP-Identifier"?
>>
>> -- section 3.1, ABNF
>>
>> Is this the authoritative definition for "initial-response"? If so, please mention that in the text. The including section does not mention "initial-response"
>>
>> -- section 3.1, last paragraph:
>>
>> Used as follows where, the rest of this paragraph? If so, then the "used as follows" clause is redundant. Also, the paragraph would be much easier to read if you broke it into a bulleted list.
>>
>> The 3rd sentence is awkward. I suggest something like "The … field is used as described in section 5."
>>
>> Is there a word missing from the forth sentence?
>>
>> -- section 3.2, 1st paragraph:
>>
>> s/ (re)directs/ redirects . Also, what gets redirected (i.e. what is the direct object of "redirects"?)
>>
>> -- section 3.2, 3rd paragraph (2nd note)
>>
>> I can't parse this.
>>
>> -- section 3.2, BNF
>>
>> Is this the formal definition of authentication-request? The next paragraph suggests it is defined in a referenced document. If so, then the repeated definition creates confusion about which document is authoritative on the matter. (if the definition is repeated here as a courtesy, please say so explicitly)
>>
>> -- section 3.2, 6th paragraph: "The client now sends…"
>>
>> s/now/then. Also, It seems like these sections jump back and forth between message definitions and a sequence of steps. I find that confusing. It would be better to keep the "sequence of steps" and the "message definitions" separate. But if you want each section to represent one or more steps in a sequence, then please add some context (e.g. "Once the widget completes the steps in section XXX, it then …" ). Keep in mind readers often jump directly to a section from the table of contents.
>>
>> -- section 3.3, 2nd paragraph:  "and SHALL be used to set state in the server accordingly, and it shall be used by the server"
>>
>> Is this new normative language, or a repeat of language from the SAML spec?
>>
>> -- section 4, 1st paragraph:
>>
>> I have difficulty parsing this.
>>
>> -- section 4, 2nd paragraph, final sentence:
>>
>> sentence fragment.
>>
>> -- section 5, 1st sentence: 'The "gs2-cb-flag" MUST use "n" '
>>
>> … MUST be set to "n"? (Or even better, 'the [actor] MUST set the [flag] to "n" '
>>
>> -- section 7 " This section will address…"
>>
>> Addresses ( unless you haven't addressed it yet)
>>
>> Also, does the GSS-API description introduce security considerations? If not, please say so.
>>
>> -- section 7.1, "… unless a client always verify the server identity… "
>>
>> s/verify/verifies
>>
>> -- section 7.3: "SASL servers should be aware that SAML IdPs will track - to some extent - user access to their services."
>>
>> I'm not sure what it means for a server to be aware of this sort of thing. You are talking about people, not servers, right? What actions do you propose implementors take to mitigate this?
>>
>> -- section 7.4:
>>
>> s/you/users
>>
>> "By using the same identifier to log into every Relying Party, collusion between Relying Parties is possible."
>>
>> But this isn't the Relying Party's decision, it is? I think you mean to say, if the client (or user) does this, it create a vulnerability where the Relying Parties can collude…
>>
>> -- section 8:
>>
>> I suggest breaking each IANA requested action into its own subsection. Otherwise the second action looks like an afterthought, or a comment on the first.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>