[security-services] comments on (most of) core-25

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

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:stephen.farrell@baltimore.ie
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.