Subject: Gen-ART LC Review of draft-ietf-kitten-sasl-saml-06

From: Ben Campbell <[email-masked]>
Date: Thu, 5 Jan 2012 16:32:44 -0600

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.

---

From: Simon Josefsson <[email-masked]>
Date: Wed, 18 Jan 2012 14:23:17 +0100

Ben Campbell <[email-masked]> writes:

> 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:
>

Hello Ben.  Thank you for your review.  Klaas pointed me at this part
and I will try to work this out with you.

Re section 4 I believe your comment is based on a misunderstanding.  Let
me try to explain what the intention is, and we can work out how to fix
the text if needed.  What is described in paragraph a) and b) are the
protocol requirements that (implicitely) follow from GS2.  There is
nothing particular to this protocol in there.  Let's take a step back:

The purpose of GS2 is to map any GSS-API mechanism into a SASL
mechanism.  However, SASL-SAML is defined as a SASL mechanism (because
SASL implementers wants it that way).  The point of section 4 is to turn
this SASL mechanism into a GSS-API mechanism that, after the SAML
GSS-API mechanism is used via GS2, becomes identical to the SAML
mechanism defined in the rest of the SASL-SAML document.  This is a bit
convuleted to describe, but this is the gist of it.

(It would have been simpler to specify a SAML GSS-API mechanism and then
let GS2 convert it to SASL automatically, but some SASL people doesn't
want anything to do with GSS-API so this is a compromise to allow them
to implement SAML-SASL without touching GSS-API.)

I'm thinking that perhaps the above explanation would be useful to have
in the document, to give some background.  If you agree, I propose to
resolve your section 4 comment by making this change:

OLD:
  The SAML SASL mechanism is actually also a GSS-API mechanism.  The
  SAML user takes the role of the GSS-API Initiator and the SAML

NEW:
  This section specify a GSS-API mechanism that when used via the GS2
  bridge to SASL behave like the SASL mechanism defined in this
  document.  Thus, it can loosely be said that the SAML SASL mechanism
  is also a GSS-API mechanism.

  The SAML user takes the role of the GSS-API Initiator and the SAML

Does this resolve your concern re section 4?

Re your section 5 comment, I tend to agree.  The section is quite brief
because it was ripped out of section 3.1.  I propose that we simply
collapse this section back into 3.1 where it makes more sense.  Thus:

OLD:
      - See Section 5 for the channel binding "gs2-cb-flag" field.

NEW:
      - The "gs2-cb-flag" MUST be set to "n" because channel binding
        data cannot be integrity protected by the SAML negotiation.
        (Note: In theory channel binding data could be inserted in the
        SAML flow by the client and verified by the server, but that is
        currently not supported in SAML.)

And then remove section 5 completely.  Section 3 and in particular
section 3.1 already contains the relevant references and provides the
context where the statements make sense.

What do you think?

Thanks,
/Simon