[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [Elist Home]
Subject: core-12 comments
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:email@example.com 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).
Powered by eList eXpress LLC