Comments on SAML 2.0 Core draft-19...

["Conor P. Cahill" <concahill@aol.com>]

So, I've been spending some time reading through the latest core spec
(yes, Jeff, it's about time) and have the following
feedback/commnets/question (line numbers from the *NON* diff pdf file):

• General:  It would be really helpful to one reading the spec to have some examples showing implementation of the various schemas (i.e. a sample NameIdentifier element).  This is especially helpful to one who is not an XML Schema expert.
• General: Timestamps are treated with different prose in different places throughout the document (Lines 634, 636, 688, 691, 755-758, etc.).  Suggest that we have one definition of timestamp values (as we do in section 1.2.2) and the remaining places all refer to that section.   So line 634 (and 688) would be along the lines of "A time instant (see Section 1.2.2)...."  and the prose on lines 755-758 would be deleted.  
  
• General:  Why do we have separate attributes for MajorVersion and MinorVersion?   Can't we have a single attribute called version with a fixed decimal format (2.0).  Also, can't we make MinorVersion optional so that if it isn't present it defaults to zero.  This applies to the assertion itself, as well as the RequestAbstractType.
  
• Line 407 & 470 - if no Format valie us provided the *identifier* is in effect.  I suggest changing the word identifier to be "value" since we are talking about a value here and this is in the "Name Identifiers" section which makes the word identifier a bit confusing.
• Lines 560-565:  The way this is laid out in the schema, I can have 0 or more of the 4 statements in a random order.  I would rather see this done by removing the choice and then having the 4 statements listed with min=0, max=unbounded.  That way the statements of the same type all come together.
  
• Lines 573-603: I'm a bit confused here.  Lines 575 says that the Subject "contains a name identifier, a series of one or more subject confirmations or both".  The schema says that you either have a name identifier with zero or more confirmations or you have one or more confirmations (in other words, you could have just a name identifier, or you could have just a confirmation or you can have both).
• Lines 573-603: What's really confusing to me is what the heck is a Subject without an identifier but which has a confirmation?  That should be explained in here somewhere.
• Lines 722-731: These make statements as to the validity of the assertion based upon the conditions.  I think that it should refer to the condition check step in the validation process rather than saying that the assertion is valid.   So, if the condition is not met, the condition check step fails.  If the conditions are not specified, empty or all are met, the condition check step is passed.    I don't like the current statement on line 724-5 that says 'the assertion is considered to be Valid".  Similarly, I would say that conditions are met or not met (as opposed to valid or invalid -- the conditions themselves are (almost) always valid, it's just whether or not they are met that is of interest here).
• Section 2.3.3.5.4 (One Time Use): I think we should have some prose in here about the fact that the consuming entity should only accept such an assertion once (e.g. if the same assertion is presented again, even within the validity window, it should be refused).  It would probably also be interesting to have some form of a recommended timeframe to keep track of these (perhaps twice the validity period or something like that).
• Lines 949-950: "That is, it MUST NOT be used in subsequent assertions about the same principal" probably should be clarified to be "That is, it MUST NOT be used in subsequent assertions given to different relying parties about the same principal".
• Line 1083: "then the" at the end of the line should be just "the"
• Line 1430:  Status should not be required.  If not present, it should be interpreted as:

> > <Status>
> >         <StatusCode Value="urn:oasis:names:tc:SAML:20.:status:Success"
> > />
> >     </Status>

• (Note that I think this awful lot of stuff to just say "OK".)
  
• Line 1483:  Do we really mean "permissible" or do we mean something along the lines of pre-defined.   I don't think we should limit the status codes to the ones we can think of right now.
  
• Lines 1484-1536: Do we really need these to be urn's?   Can't we just reserve unqualified strings as our own and let people who want to extend this list to define qualified strings?  It's not like we expect our internal status codes to collide.
• Line 1832: Suggest changing "it" to "the Identity Provider" to make it clear what "it" is.
  
• Lines 1831-1836: This reads like the subject must authenticate and then submit the AuthnRequest.  That isn't the case.  The security context/Authentication can be established *after* the AuthnRequest is received by the IdP.
• Line 1873: Strongly recommend that IsPassive default to "false" not True.   The passive request is the exception to the rule and the default should be set accordingly.
• Lines 1918-1919:  The ForceAuthn and IsPassive attributes don't have defaults specified in the schema.  Is this on purpose?  I would think if we set a default we should define it in the schema.
• Lines 1937-1941: I can't tell what's supposed to go in this attribute.  I presume it specifies the namespace of the name identifier in the resulting assertion, but that isn't clear here.
• Lines 1942-1946: I thing this is a bit watered down from what it should be.    Essentially this is specifying that the Requesting Entity is asking the Identity Provider to establish a new relationship identifier (federation identifier) for the subject at the relying party. I also think the default should be "False".  Suggested wording:
• A boolean value used to indicate that the requesting party would like the Identity provider to create a new federated name identifier (if one doesn't already exist) for the subject at the relying party.  Defaults to "false".  When "false" the AuthnRequest should fail unles an existing federated name identifier exists for the user at the relying party.
• Lines 1955-1956:  I found the current wording confusing. Suggested alternative:
• Regardless of the Format in the <AuthnRequest>, the IdP may return an <EncryptedID> in the resulting assertion if the policies in effect at the IdP or the SP require <EncryptedID>s.
• Line 2010: Where is GetComplete defined?  I presume section 3.4.1.3.2.
• Line 2014: I presume this should be 3.4.1.3.1 as it is a sub-element of IdPList.
• Lines 2047-2048:  Suggest adding "or if prevented from providing an assertion by policies in effect at the IdP (such as the Subject has prohibited providing assertions to the relying parti)" as a condition resulting in this error.
• Lines 2064-2066: This makes me think that the relying party and the request issuer must be the same entity.    I would think that only the Relying party MUST be included.
• Lines 2138-2139:  There are other very reasonable reasons for using artificacts other than size.  For example it allows one to use non-signed assertions if they are presented on a secure channel between the IdP and the Relying party.
• Lines 2177-2178: This makes it look like the response must be signed (at least it could be read that way by an uncareful reader).  Suggest that we add something that says it's OK if the message is not signed when it is presented over a secure channel (yes, the current prose covers this with the "otherwise" clause, it's just not clear to someone who hasn't been working on this stuff for a while).
• Lines 2194-2196: If the status must be present and it MUST have a specific value, why require it at all.   It doesn't add any semantic value here.  I would rather see it just absent.
  
• Line 2195: There's no code value "Success" (although I think there should be). 
  
• Lines 2242-2263: Suggest making "terminate" an optional boolean attribute and not an element.  If we're gonna keep it as an element, we need to define the type (right now on line 2263 it's an empty complex type).
• Line 2272: There's no guidance as to whether or not it's ok to mix a NameID with an NewEncryptedID or an EncryptedID with a NewID.  If it is OK, we need to mention it so that both parties can prepare for that case.  If it's not OK, we should document it as such.
• Lines 2331-2333, 2396-2397, and 2427-2428: what does it mean to have a logout request expire?  Does that mean that the SP shoud not log the user out any further?  This makes no sense to me.  I think that this is supposed to refer to the original NotOnOrAfter that was in the original assertion in order to handle the possible race condition where the assertion hadn't gotten to the SP yet so the SP should remember the logout (which is an assertion cancellation) until that time so that if the assertion is subsequently presented, it should be refused. 
  

Conor