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(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf
_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf