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