core-12 comments

[Stephen Farrell <stephen.farrell@baltimore.ie>]

Folks,

Having hibernated for a long time now, I feel qualified to 
send comments on core-12 as if I were a fresh reader:-)

Overall, I think its definitely on the right track, but am 
a bit concerned that it might take a number of iterations to 
close off the details that we really have to get done (most 
of my comments being fairly detailed ones).

If there are cases where I'm re-raising something I missed
on the list, just say that and I'll shut up.

Stephen.

-- 
____________________________________________________________
Stephen Farrell         				   
Baltimore Technologies,   tel: (direct line) +353 1 881 6716
39 Parkgate Street,                     fax: +353 1 881 7000
Dublin 8.                mailto:stephen.farrell@baltimore.ie
Ireland                             http://www.baltimore.com

General:

- If [MUSTSHOULD] is being used (and it looks like it is) then it should
  be referenced up front. A number of comments below don't apply if 
  [MUSTSHOULD] is dropped.

- It might be bad XML theory, but I'm concerned at each and every
  use of maxOccurs="unbounded" since each instance creates a new 
  denial-of-service attack opportunity. I'd like to see each 
  "unbounded" explicitly justified and with guidance for implementors 
  as to a reasonable number to support and what to do if more are 
  received. It would also be a good idea to include a "number" 
  attribute in such cases, so that a server can generate an error
  early without comsuming resources.
  
- In lots of places, the specification doesn't contain text which
  explains how elements are compared. This has to be fixed if we're
  going to achieve interoperability without leaving lots of potential
  security pratfalls for implementors. Security and interoperability
  are, after all, the reason for this group. I would also argue that we 
  should be explicit about this, even if some never-heard-of-it 
  XML schema specification contains the rules as well. This applies
  nearly everwhere there's an identifying string or a time (where 
  behaviour for times in the future also have to be specified).
  
- The specification doesn't contain enough information for writing
  code - it needs to specify how clients process assertions in a 
  good bit more detail.

- Samples - some are needed, either in the spec of referenced
  from it. 

Specific comments prefixed by line number)

 83 The indentation is wrong for this paragraph.
 
 96 In XMLDSIG versioning was discussed and it was decided that this was
    best handled via the namespace. Why not do the same for SAML?
    
135 "<ds:KeyInfo>...and all of its contents": this isn't strictly true
    since other specifications are defining things that go into
    KeyInfo. Delet "and all of its contents". (And add a section to
    conformance defining which KeyInfo's MUST be supported.)

153 Why use string instead of ID? If there's a reason it'd be good to 
    state that here.

158 "MUST...be negligable": there's no way to externally test this (you'd
    have to look at code and possibly config). Better to change to a 
    SHOULD and change "third party will assign" to "third party will
    accidentally or co-incidentally assign" (I can always deliberately
    assign the same IDType).
 
165 Again "SHOULD" is better, since MUSTs (IMHO) should be easily testable.

167 There's no point in saying "MAY or MAY NOT", just use lowercase.

167 It seems wrong to allow IDType to be sometimes resolvable, sometimes
    not, without providing that information to the client. Suggest adding
    that information (probably as a boolean attribute). This should save
    lots of network timeouts!

177 A fourth decision type value would seem to be useful: "AdvisoryPermit" 
    with the meaning that iff the SAML client understands all of the advice
    elements accompanying the DecisionType then the meaning is the same 
    as "Permit", otherwise "Deny". (Incidentally, I'd rather s/AdvisoryPermit/
    Conditional/ except for the fact that "Conditions" is an element 
    already.)
    
204 What's the benefit of using xsiType (presumably defined in XML schema)
    over adding a "type" Attribute? Seems like it just makes the specification
    less clear (and I wouldn't be surprised to find it causing problems with
    some parsers). Suggest changing this.
    
213 See comment re:line 96 above. Suggest deleting this.

214 Isn't is odd to sometimes have AssertionID as an attribute and sometimes
    as an element? I don't know if this causes any practical issues (character
    sets?), but consistency would seem to be bettter (would also make XSLT and
    XPath author's lives easier).
    
229 Should specify MUSTs (for assertion creators) for the granularity of 
    timeInstant which (I'm guessing) allows for various things. Also specify
    a rule for equality when granularities differ (e.g. "20010801" is the
    same as "20010801-12:00" or not?). Maybe this is all part of the 
    definition of timeInstant, but I at least don't know the answers and 
    developers will need to (and will not go looking and will cause the 
    usual interop problems...)

231 Wouldn't it be useful to allow AssertionSpecifier to specify include a  
    digest of the assertion when its referenced? (I know we're hoping for
    IDType uniqueness, but I'd like to be able to know when I'm being
    messed about:-) Another way to do this would be to allow an IDType
    to be distinguished as containing a digest value (much like the
    earlier comment about "resolvable"). In the latter case, hardcoding
    SHA-1 is fine, since there's versioning elsewhere (either namespace
    or Version element, depending on how these comments are processed).
    
231 Is an assertion allowed to include an AssertionSpecifier that names
    itself? What about loops? Should give some guidance. Probably best
    to generally dis-allow such nasty cases, but if we really need 'em
    write it up as an exception.

249 Here's another reason not to include Version in AssertionType. The
    SubjectAssertionType extends AssertionType, but there's only one 
    version number. If I only change SubjectAssertionType (say by 
    adding ToeSmell), do I also have to bump the Version for the
    AssertionType or not? Vice versa seems bad also. Again, let's drop
    the Version element.
    
260 "by one of" is wrong, its written as "by any of"
    
265 Handling of Subject with multiple values needs more specification.
    The problems that will arise are to do with matching - the text 
    isn't clear whether all subject names independently identify the
    same subject (is "<..."fred"..."bill".../> the same as
    "<..."fred".../>" or not). Similarly, what if 100 Assertions
    are included/referenced?
    
269 X.50 attribute certificates allow a holder to be identified by a
    digest. I think this is useful functionality to support (e.g. to
    bind an assertion to an executable object) and should be 
    added as another choice for Subject. I think that hardcoding
    the digest algorithm is ok here again (as SHA-1), but we also 
    have to specify which bytes are hashed. A simple typing scheme
    should be enough. That'd end up with things like:
    
    <Subject>
    	<HashedObject type="saml:jar-file" "f592...dfd0"/>
    </Subject>
    
    We might want to include some types in section 4.

293 Section 4 has to contain the rules for what Authdata and KeyInfo
    are allowed and how they're handled for each of the protocols 
    specified. 
    
295 The structure of AuthenticatorType is puzzling. How would:
	<Authenticator>
		<Protocol="samlprot:radius"/>
		<Protocol="samlprot:ldap"/>
		<Authdata "Fred"/>
	</Authenticator>
    be interpreted? Basically, each protocol should specify how 
    the list of Authdatas are handled.
    I'd suggest changing this to:
    
    	<element name="ProtocolInfo" type="saml:ProtocolInfoType"/>
	<complexType name="ProtocolInfoType">
		<attribute name="protocol" type="uriReference"/>
		<element name="Authdata" type="string" 
			minOccurs="0" maxOccurs="100" />
		<element ref="ds:KeyInfo" 
			minOccurs="0" maxOccurs="100" />
	</complexType>
    
    	<element name="Authenticator" type="saml:AuthenticatorType"/>
	<complexType name="AuthenticatorType">
		<sequence>
			<element ref="saml:ProtocolInfo" minOccurs="0"/>
		</sequence>
	</complexType>

    
    	e.g.:
    	
    	<Authenticator>
    		<ProtocolInfo protocol="saml:radius" "fred" "password"/>
    		<ProtocolInfo protocol="saml:ldap" "freddy" "password01"/>
    	</Authenticator>

307 I don't see how we can leave the syntactic issues to implementations,
    mainly for comparisons (e.g. is "baltimore.com" the same as 
    "BaLtImOrE.CoM"?). I think we should RECOMMEND that DNS names are
    used for security domains where possible.

314 Whether security domain and user name strings are case-sensitive, allow
    whitespace and handling of leading/trailing whitespaces has caused 
    problems in every specification I've ever seen. Being silent on this
    is almost certainly not the right thing. (Though I do admit its tedious
    and error-prone, so I'm not suggesting the rules myself:-) Same comment
    will apply to "Object" and other fields later on.

346 AuthenticationCode is really unclear to me. Which of "RADIUS", "strong",
    "recent", "careless", "kerberos", "yellow" make sense? If RADIUS/Kerberos
    then identifiers should be in section 4 (which is turning into its own
    little IANA:-)

354 Typo: s/AuthenticationCode/AuthLocale/

361 Some text about NAT is needed here. We should at least say that the
    IP address SHOULD be globally routable unless the assertion is only
    supposed to be valid within a local network. Should also say how
    addresses are written, in particular for IPv6 addresses.
    
362 (Mischieveous:-) How is I18N handled here?    

411 Why can't the namespace information be included with the specific 
    actions? (Its too late to start saving bytes:-). Separating the
    namespace seems strange.
    
412 "unbounded" actions is too many.

413 The Evidence element seems to be an octet hole. I can't imagine how
    anyone is going to write down processing rules for this that make
    sense. If an octet hole is needed, stick one in the AssertionType,
    if rules can be written, write them.    
    
442 Would it be better to inherit something here from DSML? (Not that
    I've read that spec:-) Seems like we're re-inventing for no good
    reason here.

463 "unbounded" here and on line 473 reminds me of X.500 names! Why the
    double level unboundedness? Delete one or the other (or educate me
    more about schema:-)
    
487 I don't like the terminology here. "Valid" implies "ok to use" whereas
    the issuer authentication etc. will be part of that determination. I'd
    rather use some other term, but since I can't think of one, (other than
    "notInvalid":-) I suggest adding some text that clarifies the fact that
    a "Valid" assertion might still be completely bogus for many many
    reasons.
    
515 s/valid/potentially valid/

516 This should be up-front, and apply to all timeInstants (and text about
    comparisons is still needed too)
    
535 (and elsewhere) is there a reason to sometimes use "xsd:uriReference" and
    sometime "uriReference"?
    
573 I assume the bindings bit will be added in later.

613 CompletenessSpecifierType value "all". This doesn't seem to work. If the
    intent is to prevent client's peeking then they just say "any", then
    "all" and the rejection of the second message tells them there's something
    they're not allowed see. I think I'm missing the point of this. Having
    now read about the attribute query, I think it'd be better to include
    an "exhaustive" (default off) optional attribute in the attribute
    assertion.
    
637 "Error" doesn't seem sufficient - there should be room for a numeric code 
    (some of which shoud be assigned) and for a text explanation. Also, the
    difference between Failure and Error needs to be carefully explained - I've
    seen this cause unexpected client behaviour with other specs in the 
    past (e.g. where one server sends Error when out of memory and another
    sends Failure). If the distinction can't be made then the clients should
    treat them alike and only one option should be specified.
    
649 Same Version remarks as for the assertions.

648 Just checking. Is there no reason to separate out the "nonce-like" and
    "name-like" aspects of RequestID? If ever a transaction was more than
    a simple request/response this might cause a problem (where a server
    used RequestID to search in a replay buffer). I could imagine that 
    leaving it as is, but adding a note not to do that might be a good idea.

668 The text implies the only one assertion can be requested. The schema
    says as many as you like. What if I send a request for 10000000 
    assertions? 

698 If I send an authentication query for the (singular) subject with two
    names "fred" and "bill" and a server has two assertions one for fred
    and one for bill, should it give me both or neither? Line 704 implies
    that neither should be returned, which contradicts the earlier definition
    of subject (line 265).
    
740 I don't believe this element is needed, see comment on line 613 above.

757 Inclusion of Evidence here seems yet another bad thing from the point of
    view of denial-of-service. I think it makes it entirely too easy to 
    get a server to do work. I also dislike that there are no Evidence types
    specified.
    
785 Version (same again).