Subject: Gen-ART Telechat Review of draft-ietf-kitten-sasl-saml-08

From: Ben Campbell <[email-masked]>
Date: Fri, 13 Jan 2012 16:00:08 -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 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.

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.

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

> -- section 5, general:
> 
> The section seems to need further elaboration or references

I did not see a response to this comment.

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

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.

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

> -- section 3.1, first paragraph:
> 
> Is "authorization identifier" the same as "IdP-Identifier"?

The paragraph has been updated, but I still have the question. 

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

> -- 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…".

> -- section 7 
> 
> Does the GSS-API description introduce security considerations? If not, please say so.
> 

I did not see a response to this comment.

---

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

> I did not see a response to this comment.

I missed this in my last e-mail.  I propose we add another sub-section
of the security considerations like this:

7.5. GSS-API specific security considerations

   Security issues inherent in GSS-API (RFC 2743) and GS2 (RFC 5801)
   apply to the SAML GSS-API mechanism defined in this document.
   Further, and as discussed in section 4, proper TLS server identity
   verification is critical to the security of the mechanism.

I believe this should cover the relevant security considerations.  Of
course, having more implementation experience with the SAML mechanism
used as a GSS-API mechanism may help to identify further security
considerations for the GSS-API mechanism.  However, I don't believe that
is a show-stopper that prevent publication now.

/Simon

---

From: Klaas Wierenga <[email-masked]>
Date: Wed, 18 Jan 2012 20:42:21 +0100

Hi Ben,

> 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 previ…

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.

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

> I did not see a response to this comment.

see Simon's comments

> I did not see a response to this comment.

idem

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

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

> Partially fixed. I suggest s/"needs to be"/"is"

ok, will do

> The paragraph has been updated, but I still have the question. 

It is not, I will add text on that

> 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

> 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."

> I did not see a response to this comment.

see response Simon.

Klaas

> 
> _______________________________________________
> Ietf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ietf