Again thanks for the comments and questions Hannes and apologies for the
delayed response on this one.  I've tried to address your comments inline
below. Some new questions were raised too.


On Fri, May 25, 2012 at 1:22 AM, Tschofenig, Hannes (NSN - FI/Espoo) <
hannes.tschofenig@nsn.com> wrote:

> **
>
> Here a few minor comments:
>
> The specification does not provide a lot of hints for the client when an
> error occurs. For example, Section 4.1.1 only says “invalid_client” is
> something goes wrong with the assertion processing in case of client
> authentication. The same is true for the authorization grant error
> response in Section 4.2.1.
>
> What about errors like?
>
> ·       Assertion was not fresh – replay detected (based on the assertion
> ID)
>
> ·       Issuer unknown or not trusted
>
> ·       Assertion couldn’t be parsed
>
> ·       The assertion format is unknown
>
> ·       Signature covering the assertion couldn’t be verified
>
> ·       Audience does not match
>
> ·       Assertion expired (based on ‘expired at’ element)
>
> ·       Missing mandatory elements in the assertion
>

T intent was to utilize the "error_description" field to provide such
information (at the discretion of the AS).  §4.1.1 and §4.2.1 have the
text, 'The authorization server MAY include additional information
regarding the reasons the assertion was considered invalid using the
"error_description" or "error_uri" parameters.' The use of "invalid_client"
and "invalid_grant" error codes in this document are consistent with their
definition and usage in the OAuth Framework spec and the description is
available to provide more detailed troubleshooting information. I didn't
want to attempt to iterate all possible error conditions and register them
as OAuth error codes nor did I feel there would be value in doing so. There
may also be AS implementations/deployments that, for security reasons,
don't want to provide such detailed information on error conditions.


 There are a lot of “SHOULDs” in the specification and I was wondering why
> this has to be the case.
>

I believe that in general that is largely because this document is an
abstract meta specification that attempts to provide guidance and common
functionality to the concrete specs that will derive from it (currently the
SAML and JWT profiles are the only ones) without dictating too much.


> Typically, there has to be a good reason why there is a SHOULD rather than
> a MUST or at least an explanation in case there are different processing
> alternatives.
>
> For example, Section 5.2 says:
>
> “When the client is acting on behalf of itself, the
>
>       Principal SHOULD be the "client_id".
>
> “
>
> When the client is acting on his own behalf then it would be good to say
> in what cases the principal element does not contain the client_id. In
> general, it seems that the client_id and the principal are pretty much the
> same thing (the fields just appear twice).
>
 The same issue regarding the “SHOULD” shows up in other places as well,
> such as with the issuer in the same section: “If an assertion is
> self-asserted, the
>
>       Issuer SHOULD be the "client_id".”
>

In some cases these entities may be identified by means other than their
OAuth specific identifier and I believe part of the intent in these SHOULDs
is to allow for that while having some other kind of association between
that identifier and the client id. And also leaving this meta spec as
somewhat general.

Perhaps the wording around those items could be improved however.  What
about something like this, for example, "If an assertion is self-issued,
the Issuer MUST unambiguously identify the client to the AS and MAY be the
'client_id'."?


>  Again same section: “The Audience SHOULD be the URL of the Authorization
>
>       Server's Token Endpoint.”
>
> In case it is not an URL what else should it be? When can this other case
> happen?
>

The audience can be used as a restriction of where there assertion is
intended to be sent (which is the URL case) but also as an indication of
whom it was sent to (which is case that might have a more abstract value
identifying the AS).  If you look in the SAML Profile, for example, you'll
see a lot of wording trying to account for different possibilities of how
the Audience is represented in how it talks about the SAML
AudienceRestriction element and the Recipient attribute.

>  You also write: “The assertion MUST contain an Issuer.”
>
> That’s great but what is the relationship if the assertion already
> contains an issuer as part of the signature that covers the party that signed
> the assertion (which is the case in SAML). Do they have to match?
>

I'm not sure I understand this exactly?  The intent is to say that whatever
the token/assertion is, it needs to have a field/element/claim that says
who "issued" it.  In SAML there is the Issuer element which is covered by
the signature but is not part of the signature. The same is true of JWT but
it's a claim named "iss".


>  Section 6.3 and Section 6.4 say:
>
> “
>
>    o  The grant_type HTTP request parameter MUST indicate the assertion
>
>       format.
>
> “
>
> Is this correct? Shouldn’t it be rather
>
> “
>
>    o  The "client_assertion_type" HTTP parameter MUST identify the
>
>       assertion format.
>
> “
>
>
>

The "client acting on behalf of" cases are done by using an assertion as an
authorization grant so the use of "grant_type" in 6.3 and 6.4 is correct.


> Difference between Section 6.3 and 6.3: anonymous user or not
>
> These two sections are identical with one exception: the content of the
> principal element.
>
> Wouldn’t it make sense to merge the two cases and then to indicate that
> the content of the principal element varies?
>

Yes, that probably would make sense as well as shorten up the draft a bit.
I'll work on it.


>   In Section 6.2 you write:
>
> When a client is accessing resources on behalf of itself, it SHOULD
>
>    do so in a manner analogous to the Client Credentials flow defined in
>
>    *Section 4.4*<http://tools.ietf.org/html/draft-ietf-oauth-assertions-03>of OAuth 2.0 [
> *I-D.ietf-oauth-v2*<http://tools.ietf.org/html/draft-ietf-oauth-assertions-03>
> ].
>
> Use  “Client Credentials Grant flow” instead of “Client Credentials flow”
>

I've made that change in my local copy of the xml source. At what point in
this process can I or should I post a new revision?



>  In Section 6.3 you write:
>
> “
>
> When a client is accessing resources on behalf of a user, it SHOULD
>
>    be treated as using an assertion as an Authorization Grant according
>
>    to *Section 4.2*<http://tools.ietf.org/html/draft-ietf-oauth-assertions-03>.
>
>
> “
>
> This is a confusing sentence. I believe what you are trying to say is that
> the description in Section 4.2 MUST be followed.
>
> Agree that the sentence is confusing and could be worded better. But I
don't necessarily want to say MUST as the core framework spec allows for
the "on behalf of" case using the client credentials grant.

How about, "When a client is accessing resources on behalf of a user, it
SHOULD use an assertion as an Authorization Grant according to Section
4.2."?


>
>
> IANA consideration section: This section is essentially the communication
> you have with IANA to request values to be added to existing registries. It
> helps them to be specific about what information you want to get added to
> what registry.
>
> For example, Section 8.1 registers an additional parameter called “
> assertion”. It would be useful to just say that this is a new entry to the
> “OAuth Parameters Registry” established in Section 11.2 of [
> I-D.ietf-oauth-v2<http://tools.ietf.org/html/draft-ietf-oauth-assertions-03>
> ].
>

Other than not using the actual section number, doesn't it basically say
that already?


>  As a minor nit here The parameter usage location indicates “client
> authentication, token request”. Client authentication is not a valid
> location per Section 11.2.1.
>
> The same comment applies to Section 8.2 and Section 8.3.
>

I've removed the "client authentication" from the param usage location
bullet of each of those three sections.



> Ciao
>
> Hannes
>
>
> _______________________________________________
> OAuth mailing list
> OAuth@ietf.org
> https://www.ietf.org/mailman/listinfo/oauth
>
>