OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

security-services message

[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:


- As suggested in this message


can you modify the namespace prefix used throughout?

- Did you consider including language such as that in the following message?


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*

- 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

- [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

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]