RE: [security-services-comment] Public Comment

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

Sorry for not responding sooner, I'm finally getting back to reviewing
comments and starting the last round of editoral changes.

> Here are some comments on the public review version of 
> Assertions and Protocols from the perspective of a naive 
> outsider. Note that I skipped section 3 and just skimmed 
> sections 8 and 9.

Thanks for the thorough review. Note that I think some of the confusion is
from not reading section 3, which is sort of "applied SAML". Also, there's
eventually going to be some kind of implementation guidelines document that
provides a much better forum for overview and explanatory text than the core
would. As the spec gets bigger, room for lots of explanation gets smaller. I
wish we had time to get it done concurrently, but the resources haven't been
there.

> - Line 314. I assume the requirements here apply not just to 
> pseudorandom numbers but also to true random numbers.

S'pose so.

> - Line 357. Schema shows elementFormDefault="unqualified". 
> Since all types are declared globally I don't think the value 
> for elementFormDefault even matters, but "unqualified" is 
> pretty misleading since all elements in an instance document 
> end up namespace qualified, don't they?

In SAML, yes. We don't use locally defined elements, so yes, it's irrelevant
what this is defined to be.

> - Line 435 - The default for format on line 435 is 
> contradicted by line 490, where a different default is 
> defined for Issuer. Maybe 435 should start "If no Format 
> value is provided and no default is otherwise defined, the value...".

They can't really contradict, since they aren't the same element. They share
a type, but the two elements are directly defined, and don't share
underlying descriptive text based on that. I'll see if I can improve it. I
think it's best to call out the difference in the Issuer definition.

> - Line 468 says "The encrypted content MUST contain an 
> element that has a type that is derived from 
> BaseIDAbstractType, NameIDType or AssertionType." I was 
> confused by "derived from"; it makes it sound like an element 
> with a type of NameIDType, for example, wouldn't satisfy the 
> requirement. Maybe something like, "The encrypted content 
> MUST contain an element that either a) has a type of 
> NameIDType or AssertionType or b) has a type that is derived 
> from BaseIDAbstractType, NameIDType or AssertionType. Similar 
> comment for line 591 / AssertionType and line 1181 / AttributeType.

Thanks, added.

> - Line 469 - The fact that the content of an EncryptedID can 
> be an assertion is surprising. What does it mean to use an 
> assertion as an identifier? Is the assertion's subject the 
> thing identified? If using an assertion for an identifier is 
> useful, why isn't it allowed in unencrypted form?

It is (just derive a new ID type from BaseIDAbstractType), but there's not
much point, since if it's unencrypted you could just stick that assertion's
Subject in directly. The purpose of encrypting an assertion is to allow a
signed, bounded or conditioned claim to be used, exactly as you state, while
still preserving privacy. It's sort of like a principal referencing ticket
that can expire.

This is discussed in the name ID mapping profile, but since that's in a
second document, I'll add something here. I think we need to point out that
if the encrypted "identifying" assertion is invalid, then it pretty clearly
invalidates the enclosing one. I think this is the only rational
interpretation, but we should codify it.

> - Lines 475 to 477. This is a judgment call, but I think the 
> MUST here is too strong. To me this seems like a desired 
> feature rather than an absolute requirement for 
> interoperability or operation.

I would disagree with that, I think. Moreover, XML Encryption basically
insists on this anyway. It's a basic rule that you always insure the
ciphertext is different for each operation by using a unique IV.

Also, we bake in such constraints because we *are* creating interoperable
privacy controls. That's the entire point of encrypted identifiers, along
with many other spec features derived from Liberty and/or Shibboleth. When
privacy isn't a requirement, there are ample alternative features available.

Just MHO anyway. The TC should address this on the next voting call.

> - Line 504. First, "resolving" is a confusing word to use 
> with URIs, since 2396 and 2396bis both use "resolve" to mean, 
> roughly, "take a relative URI and make it absolute". Second, 
> must it _always_ be possible to retrieve the assertion via 
> the AssertionURIRef, or is it legal for the URI reference to 
> identify the assertion without necessarily providing a means 
> of retrieval? I'd suggest it should be legal to simply 
> identify. If that's the case, I'd just remove the sentence 
> that begins "Resolving a URI reference..." and leave it to 
> the binding or profile to describe the operations (like 
> retrieval) that can be performed on the referenced assertion.

My inclination is that this is pretty subtle in semantics (apart from the
point you make about the word "resolve"), but the TC should discuss. What's
the appropriate word to use to describe the act of turning a URI into a
resource representation?

> - Line 532. Normative MUST is confusing, since Conditions are 
> required to be "taken into account" but there's no guidance 
> for dealing with an assertion that turns out to be Invalid or 
> Indeterminate. What's the conformance test for "taken into account"?

How about "evaluated" and a reference to section 2.5 where we define
conditions?

I don't think you're going to find us telling relying parties what to do
with an Invalid or Indeterminate assertion though. That's been the case back
to version 1.0.

> - Line 549. The line that starts "If omitted..." is confusing 
> for a couple of reasons. First, I think statements "apply to" 
> subjects rather than "identify" subjects.

The sentence doesn't really say that, though, it says "the statements
identify the subject(s) to which they apply".

> Second, if there's 
> no subject, then the subject is implicit by definition, isn't 
> it?

No. It could be explicit, but not in a syntax defined by SAML core. XACML,
for example.

> I'd change that line to "If <Subject> is omitted, then 
> the statements in an assertion apply to a subject or subjects 
> as identified in an application- or profile-specific manner."

I'll try and work that in.
 
> - Line 561. Normative MAY seems odd. A "relying party" gets a 
> SAML assertion. It's signed, so he MUST validate the 
> signature. The signature is valid, so he SHOULD look to see 
> who signed it (but no guidance about correlating the signer 
> and the issuer).

Definitely intentional. There is no such relationship defined in core. When
we say "evaluate the signature to determine the identity of the issuer", we
mean whatever anybody wants it to mean. Basically, you're moving from SAML
Issuer to XML Sig signer, and that trust cross-walk is not part of core.

I'll propose a small change:

evaluate the signature to determine the identity *and appropriateness* of
the issuer

> That done, the relying party "MAY process 
> the assertion in accordance with this specification and as it 
> deems appropriate". Is it really true that interoperability 
> and operational requirements end with signature validation? 
> At a minimum, isn't the relying party required to evaluate 
> the Conditions block, the version, required elements etc.?

I think "continue to process" is a better way to say it. We kept talking
about adding a general section about "assertion processing rules", but
nobody supplied one.

I'll wordsmith a bit.

> - Line 682 and 687. I was stumped by the intent of Recipient 
> and Address in SubmectConfirmationData until I looked at the 
> example for Bearer in Profiles. I think these could both use 
> a little elaboration. I may have this wrong, but my 
> understanding for Recipient is that it "Specifies the entity 
> or location to which an entity can present the assertion 
> while confirming itself. In other words, confirmation of the 
> subject can only be performed by the entity or location 
> identified by the URI in this attribute." For Adress, 
> "Specifies the network address from which an entity can 
> present the assertion while authenticating itself. In other 
> words, confirmation of the subject should only be performed 
> if the assertion was obtained from the network address 
> identified by the URI in this attribute."

This is all true except that Address isn't necessarily a URI (it's often
just an IP address, which is what the original attribute was called).
Address permits more generality, but the schema type is just string.

I will try and add some additional text, but in general SubjectConfirmation
is really a profile thing and I may best just point at profiles and say
here's an example. It can't be really understood well in isolation because
the "use" of an assertion is essentially only defined in profiles. SSO for
example being the only real case we have today. There are literally no other
examples today in which we say "you give an assertion to X to accomplish Y".
I think this is the single hardest thing for people to grasp. They want to
know what they can do with SAML assertions, and the answer is very little,
unless a profile says you can.

> - Lines 790 to 808. The rules for evaluating Conditions 
> appear to contradict the text that follows. From rules 2 and 
> 3 it seems like an invalid condition has a higher priority 
> than an indeterminate condition. The text that follows, 
> however, makes it sound like an indeterminate condition has a 
> higher priority than an invalid condition. I'm not sure, for 
> example, whether an assertion that contains both an invalid 
> condition and a condition that's not understood is Invalid or 
> Indeterminate. Applying the rules in order, I think, would 
> make it Invalid, but it seems like the text that follows 
> requires it to be Indeterminate.

Agree, this needs to be clarified. It dates to SAML 1.0. The TC will discuss
and clarify.
 
> - Lines 816 - "and if any other conditions" should be "and if 
> all other conditions". Same comment for line 820.

More old text, good catch.
 
> - Lines 849 to 850 are interesting. If there are two 
> AudienceRestriction elements and I'm included in one but not 
> the other, I should consider the assertion Invalid. Another 
> way of looking at it is that multiple Audience elements 
> within an AudienceRestriction are OR'd, while multiple 
> AudienceRestriction elements are AND'd. I suppose that could 
> be useful, but it should probably be called out a little more 
> explicitly in the text.

This has always been the "effect" of the definition, and has been noted a
few times in the past, including by me. I guess it's past time to just say
something in the text.

> - Line 904. The "MAY" here does not have the meaning defined 
> by 2119. It should appear in lower case. Same comment for line 907.

Agree, but I think we need stronger language then a lower case may. Will
respin.

> - Line 995 - 996. "Contains a reference to an authentication 
> context class, an authentication context declaration, 
> declaration reference, or both." I didn't understand this 
> until I read the equivalent description on line 1045. I'd 
> follow the form there, and say "Contains a reference to an 
> authentication context class, an authentication context 
> declaration or declaration reference, or both."

Agreed.

> - Line 1000. The "MUST NOT" says this is an absolute 
> prohibition of the spec, but it only applies "when privacy is 
> a consideration". In my opinion, the phrase "when privacy is 
> a consideration" is way too underdefined to support a 
> normative "MUST NOT". I'd drop the caps there. The 
> "RECOMMENDED" on line 1002 is fine.

This text is again the product of significant review and discussion. We
really, really meant MUST NOT. Partly it's worded so that if you're not sure
whether privacy is a consideration, well, it is. The TC should re-affirm
this.

> - Line 2609 - Boolean algebra symbols are impressive, but OR 
> and AND would be much more accessible to a general reader.

Changed.

> - Line 2700+ - This may not be a contradiction, but I found 
> it confusing. Lines 2633 and 2653 say "There is explicitly NO 
> relationship between the assertion [or protocol] version and 
> the target XML namespace specified for the schema definitions 
> for that assertion [or protocol] version."

It's definitely not a contradiction.

> Line 2700, 
> however, says, "The major and minor version in the URI MUST 
> correspond to the major and minor version of the 
> specification set in which the namespace is first introduced 
> and defined" and goes on to say that this "is intended to 
> communicate the relationship between the specification set 
> and the namespaces it defines."

Right. There is a relationship between the namespace and the spec set that
introduces it, but not between the namespace and the assertion/protocol
versions, which match the spec in lockstep.

That's why (e.g.) SAML 2.1 assertions would have a version of 2.1 but a
namespace with a "2.0" in it.

> - Line 2775 - Some of the rules in the XML Signature Profile 
> seem to contradict lines 2770 - 2774. For example, Line 2785 
> says, "SAML assertions and protocols MUST use enveloped 
> signatures when signing assertions and protocol messages." 
> Line 2700+, however, makes it clear (at least I think it's 
> clear) that a SAML Assertion is allowed to inherit a detached 
> or enveloping signature if a profile says it's ok. In 
> addition, line 2754 explicitly allows a profile to define an 
> alternative to enveloped signatures. Maybe you just need a 
> sentence in section 5.4 that says this XML signature profile 
> only applies to signatures that appear inside a SAML 
> assertion, a SAML request or a SAML response, and the rules 
> may be overridden by a SAML profile. 

Will add this.

> - Line 2793 - Is an example of using a relative reference 
> (#foo) sufficient to override the requirement in section 
> 1.2.2 that all URIs must be absolute?

I s'pose so, we should be clearer that the 1.2.2 rule is about the use of
URIs as identifiers within SAML defined elements, and not in contexts
defined by other specifications.

> - Just an FYI on 2396bis. The notion of "absolute URI" 
> changes somewhat in RFC2396bis in that it doesn't allow a 
> fragment.

I haven't looked at it at all, frankly. When you say it doesn't allow a
fragment, do you mean that a fragment can't be absolute by itself, or that
no absolute URI can contain a fragment? I assume the former, although I
guess I didn't think that was ever considered absolute. If the latter, I
guess I'll have to read it soon, since that makes no sense to me...

> When that spec officially obsoletes 2396, line 295 
> will need to change to something like "and are REQUIRED to 
> match the URI production defined in [XXXX]."  Line 1200+ will 
> also be affected since RFC2396bis has much more to say about 
> equivalence than RFC2396 did.

I'll bring it up to the TC, thanks for pointing it out.

Thanks again for the thorough review. I will send a follow up with any
resulting decisions the TC makes pertaining to your comments.

-- Scott