Mime-Version: 1.0 (Apple Message framework v1251.1)
Content-Type: text/plain; charset=windows-1252
From: Klaas Wierenga <klaas@cisco.com>
In-Reply-To: <5FBCE42B-679F-4BD5-B30B-A11664734A0B@nostrum.com>
Date: Wed, 18 Jan 2012 20:42:21 +0100
Content-Transfer-Encoding: quoted-printable
Message-Id: <9C1CF025-78C6-47F4-9BFD-77F853C72EF9@cisco.com>
References: <5FBCE42B-679F-4BD5-B30B-A11664734A0B@nostrum.com>
To: Ben Campbell <ben@nostrum.com>
Cc: "gen-art@ietf.org Review Team" <gen-art@ietf.org>, The IETF <ietf@ietf.org>,
 draft-ietf-kitten-sasl-saml.all@tools.ietf.org
Subject: Re: [Gen-art] Gen-ART Telechat Review of
 draft-ietf-kitten-sasl-saml-08
Precedence: list

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>.
>=20
> Please wait for direction from your document shepherd
> or AD before posting a new version of the draft.
>=20
> Document: draft-ietf-kitten-sasl-saml-08
> Reviewer: Ben Campbell
> Review Date: 2012-01-13
> IESG Telechat date: 2012-01-19
>=20
> Summary: This draft is almost ready for publication as a proposed =
standard. There are a few minor issues that should be considered first.
>=20
> 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.

>=20
> Major issues:
>=20
> None
>=20
>=20
> Minor issues:
>=20
>> -- section 3.2, last paragraph:  "=85 needs to correlate the TCP =
session from the SASL client with the SAML authentication."
>>=20
>> Please elaborate on this correlation
>=20
> 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"

>=20
>> -- section 4, 3rd and 4th paragraph (paragraph a and b)
>>=20
>> These seem like protocol affecting differences. If so, they need =
elaboration, such as normative statements and formal definitions, or =
references to such.
>=20
> I did not see a response to this comment.

see Simon's comments

>=20
>> -- section 5, general:
>>=20
>> The section seems to need further elaboration or references
>=20
> I did not see a response to this comment.
>=20

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?)
>>=20
>=20
> 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?

>=20
> Nits/editorial comments:
>=20
>> -- 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.
>=20
> 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.=20

>=20
>> -- section 3, 1st paragraph: "Recall section 5 of [RFC4422] for what =
needs to be described here."
>>=20
>> 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?
>=20
> Partially fixed. I suggest s/"needs to be"/"is"

ok, will do

>=20
>> -- section 3.1, first paragraph:
>>=20
>> Is "authorization identifier" the same as "IdP-Identifier"?
>=20
> The paragraph has been updated, but I still have the question.=20

It is not, I will add text on that

>=20
>> -- section 3.3, 2nd paragraph:  "and SHALL be used to set state in =
the server accordingly, and it shall be used by the server"
>>=20
>> Is this new normative language, or a repeat of language from the SAML =
spec?
>>=20
>=20
> 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, "=85. SHALL=85."'
>=20
> 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

>=20
>> -- section 4, 1st paragraph:
>>=20
>> I have difficulty parsing this.
>=20
> 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=85".

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=20
>>=20
>> Does the GSS-API description introduce security considerations? If =
not, please say so.
>>=20
>=20
> I did not see a response to this comment.

see response Simon.

Klaas

>=20
> _______________________________________________
> Ietf mailing list
> Ietf@ietf.org
> https://www.ietf.org/mailman/listinfo/ietf