Hi Stephen, I think I'll be able to crank out a new version before the end of next week. I don't think that should stop Sam however from doing a security review as soon when he is up for it, most comments appear to be cosmetic.
Klaas On Jan 6, 2012, at 11:08 AM, Stephen Farrell wrote: > > 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. >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >> _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
