[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: comments re sstc-saml-holder-of-key-browser-sso-draft-07
SAML V2.0 Holder-of-Key Web Browser SSO Profile Document ID sstc-saml-holder-of-key-browser-sso-draft-07 (PDF version) General comments: - This profile would be easier to read and more focused if all the (out of scope) extensions and recommendations were discussed exclusively in section 4. Examples include lines 242--244, lines 250--253 in section 2.2 and ... - Change reference [DSig] to [XMLSig] throughout, for consistency with other SSTC documents. - In section 2.5.1, the 2nd and 4th bullets discuss signing of the request. Request signing is also alluded to (I think) in section 2.5.2. Combine these remarks into a single normative statement regarding signing. - As it stands, this profile permits an arbitrary combination of HoK and non-HoK assertions in a given response. Is this advisable? It leads to a more difficult implementation, does it not? - In a previous set of comments, I remarked about the illogically-name hok:Protocol attribute. Please address this issue (by changing the attribute name or recording your reasons why not). If it helps, Scott gives some reasonable examples in this message: http://lists.oasis-open.org/archives/security-services/200809/msg00046.html - As suggested in this message http://lists.oasis-open.org/archives/security-services/200808/msg00057.html can you modify the namespace prefix used throughout? - Did you consider including language such as that in the following message? http://lists.oasis-open.org/archives/security-services/200809/msg00009.html I think this needs to be spelled out. - The metadata examples in section 2.6 are difficult to read. Can this be fixed? Maybe decreasing the font size will help. - The sentence on lines 528--531 should be reworded to suggest that there may be situations where both holder-of-key and bearer subject confirmation are desirable. However, a short lifetime requirement suitable for bearer assertions is needed. (I may be responsible for the short lifetime requirement to be missing from this profile, in which case I apologize.) - The last sentence of the first bullet in section 4 is not precisely true. If the IdP supports username/password authentication, after the user "authenticates" with an X.509 credential to an evil IdP, the IdP might prompt the user for a password and/or other personally identifiable information. Holder-of-key does not *eliminate* phishing. - This profile needs a version history. Specific comments: - [line 271, 314] Wouldn't IssuerAltName be more appropriate than SubjectAltName in this context? - [lines 394--395] This sentence seems to say that if a <saml:Subject> element is included in the request, the <samlp:AuthnRequest> element MUST be signed. Is this the intent? - [lines 407--408] Are you referring to signing? I suppose transport-level security could be used in the case of artifact resolution, but in any event this requirement is not clear. The use of "and/or" in this sentence is not logically sound. - [lines 410--412] Add this sentence: SAML metadata MAY be used for this purpose. - [lines 413--414] The phrase "if the user agent cannot satisfy the <saml:SubjectConfirmation> present in the <samlp:AuthnRequest>" doesn't make sense. I thought that the <saml:SubjectConfirmation> element in the request indicated the SP's desire to obtain an assertion containing such a <saml:SubjectConfirmation> element. - [line 417] NOT is not defined in RFC 2119. Moreover, I find this requirement confusing since I'm not sure why the SP would bother including such an element in the first place. - [lines 421--422] Delete the phrase "if the request is successful or the response is not associated with a request." - [lines 433--440] This requirement should read: "The assertion containing the <saml:AuthnStatement> element MUST be a holder-of-key assertion as defined in [SAML2HoKAP]. This and every holder-of-key assertion in the response MUST adhere to section 1.4 of [SAML2HoKAP]." Add a new requirement as follows: "If there is a <saml:SubjectConfirmation> element in the <samlp:AuthnRequest>, the IdP MUST honor it or return an error. If there is no <saml:SubjectConfirmation> element in the <samlp:AuthnRequest>, the IdP MUST include a <ds:X509Certificate> element in the response, but the IdP MAY include other child elements of the <ds:X509Data> element in the response as well." - [lines 441--443] I think a reference is needed.. Where is this interpretation of multiple <saml:SubjectConfirmation> elements specified? - [lines 445--446] What is a "uniquely identifying <saml:NameID>"? - [lines 452--455] Refer to [SAML2Core]. - [lines 462--464] This requirement is apparently contradictory. - [lines 470--473] The first sentence normatively specifies three security properties while the second sentence only mentions two. Moreover, the second sentence has no normative language. Perhaps this is an oversight? - [lines 478--479] This requirement is different than ordinary Web Browser SSO (see errata). Is this intentional? - [lines 516--523] Are these two paragraphs redundant? Isn't this simply reiterating what is already written in the metadata spec? - [lines 532--535] Use the profile names rather than the profile URIs for readability. - [lines 551--552] The fact that "the public key is a de facto persistent ID" could be viewed as a benefit in some circumstances. Suggested edits:: - [line 180] s/Draft 03/Draft 04/ - [line 181] s/August/October/ - [line 181] s/sstc-saml2-holder-of-key/sstc-saml2-holder-of-key-draft-04/ - [line 193] s/OASIS SSTC/OASIS Committee Specification/ - [line 194] s/http://www.oasis-open.org/committees/security/http://docs.oasis-open.org/security/saml/Post2.0/sstc-saml-idp-discovery-cs-01.pdf/ - [line 195] Remove blank line. - [line 205, 217] s/typical requirements/above requirements/ - [line 245] s/service provider/the service provider/ - [line 269] s/through use/through the use/ - [line 270] s/such the/such as the/ - [line 290] s/certificate/certificate (or a reference to an X.509 certificate)/ - [line 297] s/spoofing/initiating/ - [line 300] s/spoofed/initiated/ - [line 311] s/through a/through one of a/ - [line 318] s/is selected/has been selected/ - [line 324] s/<samlp:AuthnRequest>/<samlp:AuthnRequest> element/ - [line 335] s/<samlp:AuthnRequest>,/<samlp:AuthnRequest>/ - [line 336] s/included as/included (or referenced) in/ - [line 336] s/<saml:SubjectConfirmation>/<saml:SubjectConfirmation> element/ - [line 344] s/the X.509/an X.509/ - [lines 345--346] s/match the one included as/be used to construct the/ - [line 366] s/certificate/X.509 data/ - [lines 442--443] s/is satisfy-any/is effectively satisfy-any/ - [line 458] s^or the response^and/or the response^ - [line 459] s/The <saml:SubjectConfirmation>/Any holder-of-key assertions/ - [line 461] s/mutual TLS authentication/TLS authentication/ - [line 482] s/profile:/profile/ - [line 484] s/attribute, they/attribute. An implementation/ - [line 486, 493, 495] s/binding/Binding/ - [line 487, 496, 532] s/SSO:browser:holder-of-key/holder-of-key:SSO:browser/ - [line 490] s/endpoint/endpoint element/ - [line 495] s/attribute/attribute value of/ - [line 502] s/SingleSignOnService/md:SingleSignOnService/ - [line 510] s/AssertionConsumerService/md:AssertionConsumerService/ - [line 507, 515] s/http/https/ - [line 519] s/one/either/ - [line 519, 522] s/present/present and set to true/ - [line 522] s/a requirement/its desire/ - [line 523] s/counterparties/the identity provider/ - [line 526] s/<saml:SubjectConfirmation>/<saml:SubjectConfirmation> element/ - [line 535] s/avoid/to avoid/ - [line 537] s/verification, in this profile/confirmation/ - [line 538] s/some// - [line 538/ s/from/than/ - [line 541] s/x.509/X.509/ - [line 548] s/the/an/ - [line 548] s/end-user X.509 certificates/end-entity certificates/ - [line 549] s/regularly/often/ - [line 551] s/fields/fields in the certificate/ - [line 560] s/validation of key possession/holder-of-key subject confirmation/ Tom Scavo NCSA
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]