Subject: [abfab] Review of draft-ietf-abfab-aaa-saml-09

From: tshields <[email-masked]>
Date: Tue, 12 Aug 2014 21:15:57 -0600

I don't have any major notes or concerns addressing the draft as a
whole as it was concise and easy to read through. Below are notes on
my questions and suggestions for improvement on some specific areas.

Section 2, Paragraph 5
> This document aspires to the guidelines stipulated by...

"Aspires" seems like a weird word choice, consider replacing with
"adheres".

Section 4, Paragraph 1

> The RADIUS SAML binding defined by this binding Section 5 uses two 
> attributes to convey SAML assertions and protocol messages 
> respectively [OASIS.saml-core-2.0-os] .

This sentence does not compute and can be reworded to be more
meaningful.

Section 4, Paragraph 1

> The Length of both of these attributes is >=5.

Greater than or equal to 5 what?

Section 5.2, Paragraph 2

> Support for fragmentation over UDP is not mandatory.

Is support for fragmentation over UDP RECOMMENDED or OPTIONAL? May be
beneficial to be more explicit here on what this draft's opinion is.
If the draft doesn't care at all, then consider removing this sentence
entirely.

Section 5.3.2, Paragraph 2 and Paragraph 4

> The following methods are sufficient:
> 
> o  NAS identity in trusted digitally signed request.
> 
> o  NAS identity in trusted SAML federation metadata.

> The following methods are sufficient:
> 
> o  RADIUS realm in trusted digitally signed request.
> 
> o  RADIUS realm in trusted SAML federation metadata.

The wording here makes it unclear whether one of these alone is
sufficient or whether both are required. Rewording to say something
like "Satisfying the following two conditions is sufficient:" is more
clear.

Section 5.3.3, Paragraph 1

> This bindings calls...

Correct to "This binding calls..."

Section 7, Paragraph 1

> The Relying Party, acting as a NAS, attempts to validate the 
> Principal's credentials against a RADIUS server acting the 
> Principal's Identity Provider.

Correct to "...Principal's credentials against a RADIUS server acting
as the Principal's Identity Provider."

Section 7.3.3, Paragraph 1

> If the ForceAuthn attribute on the <samlp:AuthnRequest> element
> (if sent by the requester) is present and true, the Identity
> Provider MUST freshly establish this identity rather than relying
> on any existing session state it may have with the Principal (for
> example, TLS state that may be used for session resumption).

Consider breaking this sentence up more and explaining why the
Identity Provider must freshly establish the identity.

Section 7.3.4, Paragraph 1

> The Identity Provider MUST conclude the authentication in a manner 
> consistent with the RADIUS authentication result, and MAY issue a 
> <samlp:Response> message to the Relying Party consisent with the 
> authentication result and as described in [OASIS.saml-core-2.0-os] 
> and delivered to the Relying Party using the SAML RADIUS binding.

Break this sentence up and fix the typo in "consistent".

Something like, "The Identity Provider MUST conclude the
authentication in a manner consistent with the RADIUS authentication
result. The Identity Provider MAY issue a <samlp:Response> message to
the Relying Party that is consistent with the authentication result,
as described in [OASIS.saml-core-2.0-os]."

(The last part of this sentence doesn't make sense to me, so reword
that as necessary).

Section 7.3.5, Paragraph 1

> Any subsequent use of the <saml:Assertion> elements is at the 
> discretion of the Relying Party, subject to any restrictions on
> use contained within the assertions themselves or previously 
> established out-of-band policy governing interactions between the 
> Identity Provider and the Relying Party.

Reword to something like: "Any subsequent use of the <saml:Assertion>
elements is at the discretion of the Relying Party, subject to any
restrictions contained within the assertions themselves or from any
previously established out-of-band policy that governs the interaction
between the Identity Provider and the Relying Party."

Section 7.4.2, Paragraph 2

> ... the <samlp:Response> element MUST conform to the following:
> 
> o  It MAY be signed.

Extremely minor, but I found it a little distracting/funny that the
first bullet in a list of MUSTs is a MAY. My recommendation is to
reword the introduction to this: "... the <samlp:Response> element is
subject to the following conditions."

Section 7.4.3, Bullet 3

> If a <saml:AuthnStatement> used to establish a security context for
> the Principal contains a SessionNotOnOrAfter attribute, the 
> security context SHOULD be discarded once this time is reached, 
> unless the service provider reestablishes the Principal's identity 
> by repeating the use of this profile.

Why is this a SHOULD and not a MUST? If it is a SHOULD merely to allow
for the situation described above (where the service provider
reestablishes the identity), then it actually should be a MUST. "The
security context MUST be discarded unless the service provider
reestablishes the Principal's identity" is a perfectly sound requirement.

- -- 
Troy Shields
Developer of Awesome | Jive Communications, Inc.
Jive.com | [email-masked]

---

From: Sam Hartman <[email-masked]>
Date: Wed, 13 Aug 2014 09:45:47 -0400

>>>>> "tshields" == tshields  <[email-masked]> writes:
Hi.
most of your comments were editorial wording comments and can be
addressed in editing the document.
We appreciate both the editorial comments and the more technical
comments I'm responding to in this message.

    tshields> Section 5.3.2, Paragraph 2 and Paragraph 4

    >> o RADIUS realm in trusted SAML federation metadata.

    tshields> The wording here makes it unclear whether one of these
    tshields> alone is sufficient or whether both are
    tshields> required. Rewording to say something like "Satisfying the
    tshields> following two conditions is sufficient:" is more clear.

Sticking the information either in metadata or in the SAML message is
sufficient, both is not required.

    tshields> Section 7.4.3, Bullet 3

    >> If a <saml:AuthnStatement> used to establish a security context
    >> for the Principal contains a SessionNotOnOrAfter attribute, the
    >> unless the service provider reestablishes the Principal's
    >> identity by repeating the use of this profile.

    tshields> Why is this a SHOULD and not a MUST? If it is a SHOULD
    tshields> merely to allow for the situation described above (where
    tshields> the service provider reestablishes the identity), then it
    tshields> actually should be a MUST. "The security context MUST be
    tshields> discarded unless the service provider reestablishes the
    tshields> Principal's identity" is a perfectly sound requirement.

Well, we've found in the GSS community that the user experience of
ending some sorts of sessions prematurely is sufficiently bad that
everyone has gone out of their way not to do it.  For example if you're
in the middle of downloading a file, it's far better to finish that than
to kill your session because it reached an authentication time out.
We're hoping to make the text in this draft consistent with what people
will do rather than an ideal that would break stuff.

The Kerberos community went through a lot of pain convincing themselves
to do this over the years, and so that's where we're taking this from.
It's certainly something that we can discuss if we think our
requirements are different.