[Openid-specs-ab] openid-connect-messages-1_0-15 review

Mike Jones Michael.Jones at microsoft.com
Fri Feb 1 09:00:43 UTC 2013


Hi Amanda,

I've checked in updates for all your comments on Messages.  Any chance you can do similarly detailed reviews of the other specs? :)

                                                            Thanks a bunch,
                                                            -- Mike

From: openid-specs-ab-bounces at lists.openid.net [mailto:openid-specs-ab-bounces at lists.openid.net] On Behalf Of Anganes, Amanda L
Sent: Friday, January 25, 2013 12:12 PM
To: openid-specs-ab at lists.openid.net
Subject: [Openid-specs-ab] openid-connect-messages-1_0-15 review


Messages release candidate review. Comments are ordered by section number.



1.2 Authentication context definition: "...before it makes an entitlement_s_ decision" => "...before it makes an entitlement decision"



1.3 step 5: "_The_ UserInfo Endpoint..." All other steps start with "the".



2. opening sentence is awkward, suggest rewording: "The OpenID Connect protocol defines several endpoints, which the RP interacts with in order to accomplish its goal of obtaining claims from the OP."



2. step 1 last sentence: "When id_token was specified" => "If id_token was specified" OR "When id_token is specified"



2.1.1 3rd sentence "Section 4.1.1 and 4.2.2 of OAuth 2.0 defines ..." => "Sections 4.11 and 4.22 of OAuth 2.0 define"



2.1.1.1.2 2nd paragraph "Following Claims" => "The following Claims"



2.1.1.1.3 "The Client may request additional Claims on voluntary basis that it requires to perform other tasks offered to the user." What is this sentence adding? Grammar is off but I'm not sure what it is trying to add to the section. Remove it?



Formatting for member values of "null" or "A JSON Object" is off. Not formatted like other definition lists in the document.



2.1.2.1 auth_time definition mentions Request Object semantics (If requested with {"essential" : true} then the claim is REQUIRED). Why does this optional claim get this clause while none of the other optional claims do? Pull it out or add it to all other OPTIONAL Claims.



2.2 "..to obtain Access Token Response" => "...to obtain _an_ Access Token Response"



2.2.1 client_secret_jwt definition, 3rd sentence "The Client Authenticates" => "The Client authenticates"



The paragraph at the end of client_secret_jwt definition specifies value of "client_assertion_type" and "client_assertion", which are NOT included in the list of required claims in this document. The claims listing currently states "The JWT MUST contain the following REQUIRED Claims and MAY contain the following OPTIONAL Claims:" but should instead say "In addition to any REQUIRED and OPTIONAL claims specified by OAuth JWT Bearer Token Profiles and OAuth 2.0 Assertion Profile, the JWT MUST contain the following..." It might be worthwhile to additionally specify on the "client_assertion" and "client_assertion_type" claim requirements that those claims are specified in the referenced docs.



The private_key_jwt section has the same problem.



2.4 offline_access definition "...access token that grants access _to_ the End-User's UserInfo endpoint..."



3.1 "Each parameter MAY have JSON Structure as its value." Should this be "Each parameter MAY have a JSON structure as its value"?



4.2 Delete fragment "The related elements are:" from the first line.

Provider x509_url, x509_encryption_url, and Client x509_encryption_url definitions are all missing end periods.



5.1.1 "MUST decode the JWT in accordance with _the_ JSON Web Encryption specification"



5.1.3 step 1 change "the unsupported Claims" to "any unsupported Claims".



5.2 step 10 "...a nonce Claim MUST be present and its value of the checked to verify" => "a nonce Claim MUST be present and its value checked to verify"



6. "The user MUST always explicitly consent to the return of a Refresh Token that enables offline access", but the Authorization Server "SHOULD explicitly receive user consent for all clients when the registered application_type is native". This seems contradictory; am I missing something?



9.1 "...a request may be disclosed to an attacker posing security and privacy threat" => "...a request may be disclosed to an attacker, posing a security and privacy threat"



"This works even against a compromised user-agent in the case of indirect request." => "This protects against even a compromised user-agent in the case of an indirect request."



9.6 "Since...malicious Client to send _a_ request to a wrong party"

"To mitigate...require that the request to be digitally signed...using" => "To mitigate...require that the request be digitally signed...using"



9.8 is an Authorization Code really an example of possible token reuse, as it is not a token (maybe a question of semantics) and is already required by OAuth 2.0 section 10.5 to be single-use only?



9.9 OAuth SC needs to be called out with a proper reference/hyperlink. "it" in the first sentence should be replaced with "an authorization code", and/or the whole thing could be rewritten. It reads like an unfinished thought.



9.10 2nd paragraph "Responses to token requests is bound" => "Responses to token requests are bound"



9.11

"A timing attack is an attack that allows the attacker to obtain an unnecessary large amount of information" => "A timing attack allows an attacker to obtain an unnecessarily large amount of information"



2nd paragraph, should "instance of the finding error" be "instance of finding the error"?



9.16 comma in title is unnecessary



If providing refresh token revocation (which is not mentioned anywhere else in this document) is required, shouldn't the revocation document be referenced? It feels odd for a MUST to be placed here w/o further information, as things like a revocation endpoint and revocation request message(s) are not described by this document. Maybe it should have the phrase "The details of such a mechanism are out of scope of this document"?



Appendix A:  Breno's name is mis-placed, should be above Casper for alphabetical order by first name



--Amanda










-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-ab/attachments/20130201/5f8de858/attachment-0001.html>


More information about the Openid-specs-ab mailing list