Subject: [OAUTH-WG] Comments on Assertions draft 00 by Yaron Goland

From: Mike Jones <[email-masked]>
Date: Wed, 10 Aug 2011 21:39:58 +0000

Author List:  Change "MSFT" to "Microsoft" (twice).

Author List:  Change "Yaron Goland" to "Yaron Y. Goland".

2.  Overview:  Change "privliged" to "privileged".

2.  Overview:  Change "messsage" to "message".

3.  Authentication vs. Authorization:  Add a period after "vs" so the title becomes "Authentication vs. Authorization"

3.  Authentication vs. Authorization:  Comment on words "single assertion":  "This sentence implies that a system could issue two tokens, one for authentication and a separate one for authorization. While this is certainly possible does anyone actually do that? If not then perhaps it's not worth calling out?"

4.1. Using Assertions for Client Authentication:  Comment on "client_id":  "It seems like a bad idea to have the client_id outside of the assertion. It's either redundant or insecure."

4.1. Using Assertions for Client Authentication:  Change "a Authorization Code" to "an Authorization Code".

4.2. Using Assertions as Authorization Grants:  Delete ", without direct user approval", per comment "This fragment sounds slimy and isn't all that relevant so I would suggest omitting it."

4.2. Using Assertions as Authorization Grants:  Comment on "client_id":  "This seems like a bug. Why is there a client_id? In any scenario I can come up with that would use an assertion all data needed to identifying the caller is provided (securely) in the assertion itself. So at best requiring client_id is just redundant and redundancy in protocols just opens up new places to have problems.  So I would suggest that we require that assertions MUST identify the client and therefore the requests MUST NOT include client_id."

5.  In title, change "Proccessing" to "Processing".

5.1. Assertion Metamodel:  Audience:  Change "the Authorization Server as the intended audience" to "the party intended to process the token", per the comment "It's generally not considered good form to write a definition that contains the word being defined.".

5.2. General Assertion Format and Processing Rules:  Change "a Assertion ID" to "an Assertion ID" and change "algortihm" to "algorithm" and change "generate assertion" to "generate the assertion".

6.1. Client authentication:  Change "as in a format" to "in a format".

6.1. Client authentication:  Comment on last 4 bullets:  "This is all redundant with section 5.2. I think it's not a great idea to repeat normative requirements as it just opens the door for confusion and inconsistency. So I would urge that we remove these lines and just point to section 5.2."

6.1. Client authentication:  Change "a client authenticating" to "a client authentication" and change "a Authorization Code" to "an Authorization Code".

6.2. Client acting on behalf of itself:  Change "analagous" to "analogous".

6.2. Client acting on behalf of itself:  Comment on last 4 bullets:  "Again, I would just point to section 5.2."

6.3. Client acting on behalf of a user:  Comment on last 4 bullets:  "Same comment as before".

6.3. Client acting on behalf of a user:  Change "a Authorization Code" to "an Authorization Code".

6.4. Client acting on behalf of an anonymous user: Change "authorizaion" to "authorization".

7.  Error Responses:  Comment on "Audience validation failed": "Isn't returning this kind of information just helping an attacker to hone their attack by trying various values and seeing what errors they get? I'm not sure how serious this threat is though. But I get nervous any time we leak data about security validation failures."

---

From: Brian Campbell <[email-masked]>
Date: Fri, 26 Aug 2011 12:24:52 -0600

Couple comments on the comments inline:

On Wed, Aug 10, 2011 at 3:39 PM, Mike Jones <[email-masked]> wrote:
>
> 4.1. Using Assertions for Client Authentication:  Comment on “client_id”:
> “It seems like a bad idea to have the client_id outside of the assertion.
> It’s either redundant or insecure.”
>

I tend to agree with this.  Now that treatment of client_id seems to
have stabilized in the core spec, this draft is probable due for some
reconciliatory changes around the parameter.

>
> 7.  Error Responses:  Comment on “Audience validation failed”: “Isn’t
> returning this kind of information just helping an attacker to hone their
> attack by trying various values and seeing what errors they get? I’m not
> sure how serious this threat is though. But I get nervous any time we leak
> data about security validation failures.”

Trying to walk the line between security and having useful feedback
available for troubleshooting during the early stages of deployments.
Given that there's a signature over the assertion, providing details
about other semantic validation issues doesn't seem like an issue to
me.  But I know that such information disclosure is usually considered
a no no in security related contexts.  Anyway, the error_description
parameter is optional so individual implementation/deployments can
make their own decisions about what, if anything, to put there and/or
when to put info there.