Comments regarding x509 authn based attribute profile (draft 3)

Title: Message

Rick, here are some 
comments related to both content and general edits which should probably be 
addressed prior to CD:

 

- The footer is 
incorrect (both filename, date, and copyright).

 

- Line numbers are 
per page.

 

- There are various 
references to the certificate distinguished name, or DN, (p 2 line 29, p 3 
line 13, p 3 line 18) that should probably be qualified appropriately as 
"Subject DN" as there are other DNs in the cert.

 

- (p 2 line 30) 
s/.././

 

- (p 3 line 16) 
s/integrity/verify its integrity/

 

- (p 3 line 14, p 5 
line 18) You reference the <Subject> element. Do you want to 
specifically say that the X509SubjectName format identifier should be used (it 
seems like that is what we want here). Additionally, this means mentioninig that 
the <Subject> should contain a NameID with that format whose value is the 
Subject DN from the principal's cert.

 

- (p 3 line 21) 
s/.././

 

- (p 4 line 1) s/5 
and 6/1 and 2/

 

- (Section 2 vs. 3) 
In general there are some mis-matches here due to cut/paste. I would suggest 
reworking these sections, in that Encrypted/Signing Mode IS basic mode with 
these x items applied. This will remove all the redudant wording that is there, 
like SubjConfData, Audience, having one Assertion, one Attr Statement, etc... So 
section 3 becomes more like you must sign the request, sign the assertion, 
encrypt the NameID, encrypt the Assertion, Use SSL3/TLS, <Signature> 
comments, and the Use of XXX sections. But the basics of the profile would be in 
section 2. There are items below that would apply to both section 2 and 3 (where 
the text is equivalent -- in this case, I may only decribe them related to 
section 2).  

 

- (p 5 lines 5 and 
6) replace these with "The service provider MAY authenticate to the identify 
provider using this mode." I don't think you meant to require authentication in 
basic mode (as line 5 says MUST.

 

- (p 5 line 11 and 
12) Replace with "The <AttributeQuery> and <Assertion> MAY be signed 
using this mode."

 

- (p 5 line 14 and 
15, and p 5 line 21 and 22) I think you meant to say <Attribute> and not 
<Assertion> as there is NO assertion element inside of an Attribute query. 
Also, remove the reference to section 6 in SAMLProfile as I believe it does not 
reference these in any way.  By allowing <Attribute>, the attr query 
requester can control which attributes and attribute value it wants 
returned. Is that correct? Vs. not including this and allowing the attr 
authority to control what it sends back.

 

- (p 5 line 19) 
Do you 
expect any SubjectConfirmationData in the request? Is it okay to send 
this?

 

- (p 5 line 24) So 
for any error, you just want no Assertions. The status is irrelavent -- and 
could be Success, Requester, or Responder? You may want to consider an 
UnknownPrincipal second level status (with a Success top level status) if the 
Subject DN is unknown. Also you may want to say that for the NO assertion case, 
either Requester or Responder is used to imply where the error is coming from. 
E.g., a repository error at the attr authority would imply Responder, whereas an 
invalid Subject format identifier of x509 subject dn would be 
Requester.

 

- (p 5 line 
32) SubjectConfirmationData is only in basic mode and NOT enc/signing mode? 

 
- (p 5 line 35, p 8 line 24) 
s/AudienceRestrictionCondition/AudienceRestriction/

 
- (p 5 line 27) You want to 
say that there should be only one <Assertion> and then one 
<AttributeStatement> inside the assertion -- similar to p 8 lines 
17-21.

 

- (p 5 line 29) 
s/ConfirmationMethod/Method/

 

- (p 5 line 30) It's 
unclear if the decision of conf method is solely by the attr authority or can be 
implied by the requesting SubjectConfirmationData? Are you allowing either 
case?

 

- (p 5 line 34) If 
the requester asks for holder of key subj conf data, or for some reason the 
requester (out of band) asks for holder-of-key, you are assuming that the attr 
authority MUST store the user certificate, correct? Was that the 
intent?

 

- (p 6 line 2 to 4) 
I'm not sure I follow the phrase "as requested by the service provider." I don't 
believe there is any way the SP can request other conditions?  If you are 
referring to SubjConfData or Attributes, then this should be mentioned in the 
prior bullets -- otherwise I'm not sure about this particular 
phrase.

 

- (p 6) In reference 
to othe SP having to understand and accept ALL the conditions -- I don't recall 
is this is similar to other profiles or not. I'm thinking that other profiles 
say you must adhere to condition A, condition B, etc... and other conditions are 
not relevant. For this profile you are saying that every single condition (even 
Condition extensions) must be valid or else the response must be 
rejected?

 

- (p 7 line 1) 
s/Mode Mode/Mode/

 

- (p 7 lines 5 and 
6) s/provider  MUST/provider MUST/   

 

- (p 7 lines 5 and 6) 
Remove the line "In addition....". I'm not sure why this is here. 
I would have thought that the In addition would have said "Alternatively" It 
seems like you WANT the signature and that using the soap binding technique to 
authenticate the Attr Query is NOT sufficient -- which is this?  I would 
suggest just saying "The service provider MUST authenticate to the identify 
provider as defined in [SAMLProf]." If you really want/require attr query 
signature then completely omit the soap client/server authn as this is just 
redundant (why bother?).

 

- (p 7, line 12 and 
13) As above, why are you requirinig both soap binding authn and 
<Response> signing; particularly since you are requiring 
encryption.

 

- (p 7 line 17, p 8 
line 11) add a reference to [RFC3280].   I'm not 100% sure, but isn't 
this 

 

- (p 7 line 21) As 
before is an ID possible, or do we really want a NameID with x509 
format.

 

- (p 7 line 30) 
s/previously established/previously established (out of band)/   If 
this is not the case, then how is the key established and 
maintained?

 

- (p 8 line 2) 
Delete this line -- it is 100% implied by the spec and only causes confusion as 
to why this is stated.

 

- (p 8 line 7 and 
line 34) s/that it was not modified in transit/its 
integrity/

 

- (p 8 line 23) 
s/provider./provider over the assertion./

 

- (p 8 line 35) 
Delete this line -- it is 100% implied by the spec and only causes confusion as 
to why this is stated.

 

- (p 8 and 9) Move 
section 3.2.2 after 3.2.3 (i.e., keep the order the same as in 3.1 -- encryption 
data then signature -- even though the technical processing occurs in a 
different order).

 

- (p 9 line 5) 
s/principal ./principal./

 

- (p 9 lines 10 
to 13) I would suggest syncronizing the wording with p 7 lines 
34-37.

 

- (p 10 line 10) 
s/cache user/cache, to a persistent store, user/   I assume this is 
what you mean, if not and you mean caching them in memory, then are you 
implying that the principal's identity  (e.g., subject dn) must be 
encrypted in memory?

 

- (p 11 line 3 to 9) 
Update to using the standard docs.

 

- (p 13 line 13) 
update date.

 

Tom.

 

Thomas Wisniewski
Software Architect 
Phone: (201) 
891-0524 
Cell: (201) 248-3668 

  
EntrustÒ 
Securing Digital Identities
& Information