Hi Ben, On Jan 13, 2012, at 11:00 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 wait for direction from your document shepherd > or AD before posting a new version of the draft. > > Document: draft-ietf-kitten-sasl-saml-08 > Reviewer: Ben Campbell > Review Date: 2012-01-13 > IESG Telechat date: 2012-01-19 > > Summary: This draft is almost ready for publication as a proposed standard. There are a few minor issues that should be considered first. > > Note: This is incremental to my review of version 06 at last call. Version 08 is considerably improved and resolved most of my comments. But a few still remain. Quoted text below is from that previous review. Yes, should have informed you about those changes before. I was going to wait until I had also incorporated Simon's additions, but I realize that may have been confusing. > > 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 > > The author added text, but the new text is about correlating response with request. The text I mentioned was about correlating a TCP connection to a SAML authentication. ah ok, but the intention of the text really is to talk about correlating the session that the SASL client maintains with the SASL server and the SAML session that the SAML client has with the SAML server. Would you be ok with changing the wording to that extent? So: "Also, the SASL server needs to correlate the session it has with the SASL client and the SAML authentication by comparing the ID of the SAML authentication request it has issued with the one it receives in the SAML authentication statement" > >> -- 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. > > I did not see a response to this comment. see Simon's comments > >> -- section 5, general: >> >> The section seems to need further elaboration or references > > I did not see a response to this comment. > idem >> 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?) >> > > I did not see a response to this comment. I want to give enough details for implementers to understand the full flow, even though those steps are out-of-scope for this specification. I thought the [ ] brackets would convey that, do you think it is clearer to leave that part out altogether? > > Nits/editorial comments: > >> -- 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. > > Pagination is still strange. I see a few mostly blank pages, diagrams split across page breaks, etc. hmm, strange, I removed some empty lines and in my version it nicely broke on session headings, but I'll double check all generated versions next iteration. > >> -- 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? > > Partially fixed. I suggest s/"needs to be"/"is" ok, will do > >> -- section 3.1, first paragraph: >> >> Is "authorization identifier" the same as "IdP-Identifier"? > > The paragraph has been updated, but I still have the question. It is not, I will add text on that > >> -- 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? >> > > The author changed SHALL to MUST, which leads me to believe my comment was not clear. I did not object to SHALL. My concern was, with the reference to RFC4422, it was not clear if the text was introduction a new normative requirement, or just restating requirements from 4422. If the second, then it's important to make sure the reader knows which doc is authoritative. You can do that by keeping the language descriptive, or by explicitly (and strongly) attributing the language with something like 'RFC4433 says, "…. SHALL…."' > > If, on the other hand, this is truly a new normative statement, then no change is needed. right, now I get it. It is indeed intended in the 4422 sense, so I will take your suggestion > >> -- section 4, 1st paragraph: >> >> I have difficulty parsing this. > > The text is updated, but I still have trouble parsing it. In particular, I'm not sure what you mean by the phrase "...and appropriate references of it not referenced elsewhere in this document…". OK, I propose to just change it to: "This section and its sub-sections are not required for SASL implementors, but this section MUST be observed to implement the GSS- API mechanism discussed below." >> -- section 7 >> >> Does the GSS-API description introduce security considerations? If not, please say so. >> > > I did not see a response to this comment. see response Simon. Klaas > > _______________________________________________ > Ietf mailing list > Ietf@xxxxxxxx > https://www.ietf.org/mailman/listinfo/ietf _______________________________________________ Ietf mailing list Ietf@xxxxxxxx https://www.ietf.org/mailman/listinfo/ietf