[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [Elist Home]
Subject: [security-services] comments on (most of) core-25
All, The attached are my comments on the first part of core-25. More to follow as I get time. My overall impression is that more work is needed and that that work shouldn't result in something that looks exactly like its been done by committee (which the current documents certainly do, probably unavoidably). I'd suggest sending some off to the wilderness with all the comments received and seeing what they come back with. Not all the work is editorial, I don't believe the current documents are sufficient for someone (actually *anyone*) to implement from and achieve interop, in fact that's probably some way away. I know this is a variation of the "issues" document approach, but IMO it'd produce a better result assuming a reasonable editor. (As an aside, I found the issues document unusable since it doesn't seem to correlate with the document structure - let me know if I missed out on the secret chant that makes it magically work:-) Regards, 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: - "IDType" (defined on 285) capatalization is likely to result in bugs ("IdType", "IDtype", etc.) Get rid of abbrevations from all element names. - I can't see much use for indeterminate assertions or conditions. Why not simplify to just ok/deny? - Why allow "unbounded" anywhere? I see no reason why 10000000000 statements MUST be supported, which is what seems to be implied. Suggest including a max value that implementations MUST support, to be the same for all cases of "unbounded". Either incorporate this into the schema (e.g. "maxOccurs=1000") or into text (considering how versioning is currently done). - Versioning. This should use the namespace - specific elements are unnecessary. - NotOnOrAfter. This is different from most end-date types specified elsewhere, in particular the notAfter field in many ASN.1 structures. There is no justification given for this semantic change which will cause new boundary conditions and hence new (probably broken) code. For example, if an issuer has an X.509 certificate with a notAfter of 20021231235959Z then what is the latest NotOnOrAfter value that should result in a valid assertion? What is the first NotOnOrAfter value that should result in an assertion being invalidated for this reason? I don't know the answers. Gratuitous changes are bad things. This is one such. - The specification is still *very* vague when it comes to comparing values, which implementations will have to do, e.g. no rules for dateTime *comparisons* are given at all, which will *definitely* mean non-interop for assertions, esp when >1 timezone is involved (which is often). The same is true for the Subject element which will also cause major problems, e.g. is compare("Fred","Fred or Bill")=true/valid? - Inclusion of both Audience and Target conditions is pointless and broken. Delete one, or show they're different. Specific: 189: Why's ds:KeyInfo of "particular interest". Delete, since its not that interesting. 200: What's an "Internet domain", suggest "DNS domain" and reference DNS. 206: "...checking and revoking of crentials...out of scope..." I think this just confuses and prompts silly questions like "what's the difference between an assertion and a credential". Finish the sentence at "previously." 209 s/provides a/defines an abstract/ 212 "It is possible to...in this case." Delete. Doesn't add anything. 215 Delete the paragraph. Doesn't add anything. 218 If the SAML Domain model is important enough to warrant a 1/2 page, then I would have thought that some text would be provided. None is really, other than the disclaimer on 218/9. Either provide text that says that this (normatively) defines the scope of SAML, or delete the picture and 218/9. (Note: I've no problem with the picture, it certainly was useful during development of SAML, it just isn't useful as presented in core-25). 222 s/center/goal/ (Never heared that usage ever before) 222 Delete the paragraph. Doesn't say much. 226 Delete the paragraph and move whatever wasn't already said at line 114 to there. 237 Wouldn't that be better ending with "#rwedc" rather than "/rwedc"? (That is if you're using the document name, which itself seems strange. I'd suggest "saml:#rwedc" would be better than including "core-25" in the name.) 245 Huh? Means (and adds) nothing to me. 248 "7"? 250 s/to support extension/to be extensible/ (natural language please:-) 252 s/and the/and therefore the/ and s/carefully considered/avoided/ 263 s/contains/contain/ 277 Delete - depends on location of the saml schema 278-280 Delete - "neither use nor ornament" as me granny used to say. 288 The use of "attributes" is ambiguous - I guess it means SAML attributes and not XML attributes. 290 s/assign/accidentially assign/ 291 s/is/it/ 295 Doesn't the birthday paradox apply here? This means that if IDtypes are 128 bits long, then given 2^64 values, probability of a collision exceeds 50%. Suggest just saying "use at least 128 bits, preferably 160 bits" and avoid the issue. [http://www.howstuffworks.com/question261.htm] 297 (Said this in my earlier comments, but here goes again...") A "referencable" boolean attribute would be useful. If not, just say that IDType are not referencable. 311 What use case calls for Indeterminate? If none, delete. Justification for this is that this seems to be a case of purity winning over clarity - I can't see a PDP implementation that really returns this value for a decision, they'd always return "deny" instead. Similarly, a PEP will (I guess) always treat this the same as "deny". 339 This is entirely unclear. Does it mean that the value is de-referencable, either for sure or in principle? Clarify properly. 346 Delete "(in order)" - they're not, according to the schema fragment. (at least modulo some weird element/attribute ordering rule) 393 The text makes no mention of this ds:Signature. It MUST be explained or deleted. 404 "The validity of an..." Is this considered true? What if the signature is broken? Suggest re-phrasing all this (402-411) with some "other things being equal..." type text. 412 In order to be useful the specification has to define what "understood" means. (See the current PKIX/TSP extensions discussion to know why.) In this case, since we can make use of the Advice element in the assertion I think we can be strict for Conditions, i.e. "understand" = "be able to fully process, and have code to handle, all the relevant semantics". However, we should also encourage people not to extend conditions since it degrades interop. 414 s/contains/MAY contain/ 419 & 421 What's the difference between "zero or more" and "any number"? (Can I have "pi" AudienceRestrictionConditions :-) 421 & 423 I don't understand the difference between these two and its not explained in the text. Show me why they're different or delete one please. 424 s/TargetRestiction/TargetRestrictionCondition/ 438 Are there any C14N issues with dateTime that we should note? E.g. don't add a "00" seconds if it wasn't there? Why do we want timezones? What TZ flexibility is REQUIRED for implementations (e.g. do I have to handle "+7:14" or any other nasties? I assume that the text requires me to know that "200201212301-2" is in 20020122 in GMT when I'm comparing dateTime values? If so say so, if not, say so. 461-510 I'm sorry, but there's still no visible difference between Audience and Target. Both seem the same as the ACPROF targetting concept and even have the same syntax. 494 s/true/valid/ and possibly s/if/iff/ ? 516 What, exactly, is "lax schema validation" (cf encryption list) 566 & 575 contradict one another, 1st says "one or more", 2nd says "SHOULD NOT" be >1 (which is not the same as MUST NOT!!!!). Barring a weird definition of subject vs. prinicipal (why use both terms btw?) this is wrong. I assume that "exactly one" is what's intended. 579 This syntax is over complex for the semantics allowed according to the text description. Simplify it. 603 Why is SecurityDomain mandatory to include? That's wrong since many name forms include sufficient information (e.g. rfc2822 addresses in most contexts). Should be minOccurs=0. 616 Shouldn't this be stated to be a public key (or cert) where the corresponding private key is held by the Subject? 617 I see no reason why the conf. data shouldn't be a choice between string and KeyInfo. However, I'm ok with what's there now. Might be useful to explain what additional (to the KeyInfo) data could be included. Maybe that's later on? 641 s/system entity that was/system entity from which the Subject was apparently/ 653 Why is the AuthorityBinding not mentioned above when the other fields are? I assume this is just a typo? 664 IPAddress. Need to specify v4/v6 requirements. Suggest that both v4 and v6 (string formats) be REQUIRED for support. Need to reference the relevant RFCs for the string formats. Also need to state whether /24 type notation is to be supported or not and if so how that's to be compared to a single address. 666 (eek!) says DNSName is required but the schema says optional. Fix schema I guess? What if there is no DNS name for the entity? Are you supposed to include a reverse IP address form? 680 s/authenticationand/authentication and/ 681 Isn't this redundant since it has to occur within an assertion that already says what kind it is? Delete it. 683 State whether this address is supposed to be resolvable and if so, for what. 705 Should state whether wildcards are allowed or not and if so, in what scopes (e.g. "*://foo", "*:/*/foo", "http://foo.*.com/", "/blah*/" "/blah/bar#*foo*") etc. 710 Why is "Actions" required? In some cases, there may only one be action defined (e.g. GET for an apache server & a static resource, maybe object execution for a corba object), so any action is unnecesary. This should be zero or more, with zero meaning that any and all actions defined are granted/denied. 739 Better not to say "by default" in connection with a namespace. Just delete the offending words. 779 I still see no reason why DSML attributes aren't a better choice for use in SAML (given that many/most PDPs use LDAP servers for storage of most attribute values). A case of NIH? 825 One too many levels of indirection! There's no point in having a namespace on attribute types and on instances of an attribute value.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [Elist Home]
Powered by eList eXpress LLC