RE: [security-services] Comments on SAML 2.0 Core draft-19...

["Scott Cantor" <cantor.2@osu.edu>]

Some quick reactions...

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

It also encourages something you've noted in the past, people treating
examples as normative instead of the schema, which is very bad, and one
argument for pushing examples to another document.

But mostly, it just takes a lot of time, and I don't have it. If somebody
else is willing, have at it...

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

+1, personally. This predates my involvement with 1.0

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

+1

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

It means you're describing the conditions/methods by which something can act
as the subject without actually naming it. It allows making an assertion
about, for example, a public key.

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

I'm not sure there's a difference. An assertion *is* valid if its conditions
are valid. It may not be acceptable for some use or some profile, but that
doesn't make it invalid in terms of core. Maybe I'm missing an edge case,
but that's how it seems to me.

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

I dunno, this bugs me, but I can't quite say why. It's not ambiguous or
anything though.

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

No, this was always explicit. The top level codes are those 4, period, end
of story. The subcodes can be anything.

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

I'm not in favor of it, personally. I understand why you are, though. I
think making it optional for success would be a decent compromise (ie, this
bugs me more than that does).

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

+1, I copied this from ID-FF, but I was a little confused by it myself. I
didn't think true was the more common case, but I thought maybe this had
been concluded otherwise in Liberty discussions.

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

I asked this a while back, I've assumed Eve or Phill had some reason to
avoid defaults in the past. I personally would like to add them.

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

Yes.

> *	Lines 1942-1946: I thing this is a bit watered down 
> from what it should be.

I watered it down on purpose. It says to me nothing more than "you can
create a new identifier". There's no implication beyond that that I can see.

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

The identifier may well be global and not specific to the relying party. It
definitely could be "federated" in the sense that you mean it, but that's
not a requirement, so the text doesn't say that.

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

The intro sentence to the bullets says that the rules here are specific to
the case that there are no other relying parties specified (meaning no
Conditions element in the request). So there's no basis to determine a
relying party other than the request issuer (the simple/common case).

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

I preferred it as an element, because it's so central to the action being
requested, and because it's easier than wasting time with it as a boolean
when it makes no sense for it to be false. As far as the schema, people sort
of need to make up their mind. ;-)

First I made it an empty sequence (<sequence/>) and that was confusing, so
now I just made it an empty type, which is the same thing. There's no other
way to do it, so which is preferred?

Thanks (though it's going to be tough to do all this in a week),
Scott