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. > > > > > > > > > > > > _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
