[Openid-specs-ab] Review of openid-connect-4-identity-assurance-04

Sat Jun 1 00:16:51 UTC 2019

Thanks for working on this, Torsten and Daniel. I'm excited about the possibilities that this spec opens up!  I read https://openid.net/specs/openid-connect-4-identity-assurance-04.html cover-to-cover.  The issues that I'd like to see addressed follow, broken into substantive and editorial issue sets.

SUBSTANTIVE ISSUES

All Sections:  Generalize kinds of verified claims.  The most important issue is to generalize the goal of the document from defining how to use "verified person data" to defining how to use "verified data".  This work isn't happening in a vacuum.  There are other efforts to define representations of verified claims in the industry, including https://w3c.github.io/vc-data-model/, that take this more general approach, but propose much more complicated data representations that are not based on JWTs.  It would be highly beneficial to have a simple general JWT-based "verified data" representation that is general-purpose.  Indeed, that's the possibility that excites me about this work.  Don't get me wrong - I believe that all the particulars for verified people data can and should remain.  The main concrete change needed is to rename "verified_person_data" to "verified_data".  And of course, the descriptive text needs to be similarly generalized (while still pointing out that specific data structures for verified person data are defined by the specification).

Many Sections:  Enable signed evidence.  I believe that there will be use cases in which it will be important to be able to have portions of the representations be signed by a party other than the OP.  Note that you already have an "issuer" in some cases.  It would be good if the issuer could sign the pertinent data.  For instance, there are electronic driver's license standards in the works using JWTs.  It would be good to be able to carry an electronic driver's license JWT as part of the verification data.  (Might we able to do this by using aggregated claims, or something close to that?)

Status of This Memo:  Incorrect IPR language.  The document contains an IETF IPR statement, rather than an OpenID one.  Please include ipr="none" in the <rfc ...> element to turn off the IETF IPR language and add the missing  "Notices" section containing the OpenID Foundation copyright, etc.

1. Introduction:  Generalize "strong identity verification of a natural person" language along the lines of "strong verification of identity information, including information about natural persons".  Similarly generalize the abstract.

1. Introduction, etc.:  The exposition uses the term "authentication assurance", which is not defined, and for which the meaning isn't clear.  Either add the term to the terminology section or reword to not use that term.

1. Introduction, etc.:  Add "and for the OP to represent the identity assurance data" at the end of the last sentence of the section.

2. Scope and Requirements: Misuse of "SHALL", "MAY", etc.  The RFC 2119 keywords such as "SHALL" are intended to be used to define testable requirements of implementations.  In this section, "SHALL" is instead used to express the intent of the specification - not requirements on implementations.  Please remove all such uses of RFC 2119 keywords.  For instance, reword "The extension specified in this document SHALL" to "The extension specified in this document is intended to".  Similarly, reword "SHALL allow" to "allows", etc.

2. Scope and Requirements: Add a citation after the term "Anti Money Laundering Law".

3. User Claims:  Replace "birth_name" with "birth_family_name", "birth_given_name", and "birth_middle_name".  For an example of why this is needed, my eldest daughter Gwen legally changed her name from "Gweneth Lynn Jones" to "Gwendolyn Rose Jones" before she got her first driver's license.  And my youngest daughter will get married later this year, changing her last name as a result.

3.2 transaction_id:  Replace all uses of "transaction_id" with "txn" - a JWT claim already registered at https://www.iana.org/assignments/jwt/jwt.xhtml#claims for exactly this purpose.

4.1. verification Element: Add a reference for "eIDAS".

4.1. verification Element: Add a reference for "ISO 8601:2004".

4.1.1.1. id_document: The term "CONDITIONALLY REQUIRED" is not defined and the intended meaning isn't clear.  Reword in the way the OpenID Connect Core expresses such conditional requirements.  For example, "REQUIRED if the Authorization Request included the state parameter".

5.2. Defining constraints on requested data: Change ""max_age":"63113852"" to ""max_age":63113852" (removing the quotation marks around the value), since max_age is a number - not a string.

8.  Transaction-specific Purpose:  The internationalization characteristics of the "purpose" string should be described.  For instance, what locale/language is the string may the string be in?  How is the language/locale determined between the OP and the RP?  For instance is the "ui_locale" parameter used and if so, is it required in the request?

13.  JSON Schema, etc.:  A clear statement should be made where data structures are being defined that values that are not understood MUST be ignored.  This is what will enable extensibility of the JSON over time.  The JSON Schema should reflect this as well, if it is retained.

13.  JSON Schema:  I would suggest deleting the JSON Schema entirely, as having two normative definitions of the same data structures can only lead to conflicts and inconsistencies as the specification evolves.  Note that other OpenID Connect specifications manage to unambiguously define normative data structures without the need for JSON Schema.  I don't think it adds value here as well.  (And if it does, anything expressed in the schema that is not expressed in the prose should also be normatively stated in the prose.)

EDITORIAL ISSUES

Specification version identifier:  OpenID documents typically include "1.0" in the title and represent the version number with "-1_0" in the document identifier.  Please change the document identifier to "openid-connect-4-identity-assurance-1_0" and add "1.0" to the end of the title.

1. Introduction, etc.:  The English plural of "evidence" is "evidence" (without an "s").  It's incredibly jarring to see "evidences" where what is meant is "evidence".  Please change all occurrences of "evidences" to "evidence".  (Another such example of this in English is the word "knowledge" - which is used in both singular and plural contexts.)  This occurs in many sections of the document.

1. Introduction:  Change "that is defined in this specification" to ", which is defined in this specification".

1. Introduction:  Change "a certain OpenID Connect transaction" to "an OpenID Connect transaction".

1. Introduction:  Put commas around the phrase "as defined in Section 2 of the OpenID Connect specification [OpenID]<https://openid.net/specs/openid-connect-4-identity-assurance-04.html#OpenID>".

2. Scope and Requirements: Change the comma to a semicolon in "This extension MAY introduce new user claims to cover user attributes not currently covered by OpenID Connect, one example would be the place of birth." so that it is no longer a run-on sentence.  (And fix the incorrect use of "MAY".)

2. Scope and Requirements: Change "User Info" to "UserInfo".

2. Scope and Requirements: There should always be a comma after "For example".

2. Scope and Requirements: Change "in case of eIDAS" to "in the case of eIDAS".

2. Scope and Requirements: Delete "basically", as it doesn't add to the meaning of the sentence.

2. Scope and Requirements: Change "Note: data transfer" to "Note: Data transfer".

3.1 User Claims:  Change "User Claims" to "Claims About the End-User" in the section title.  Likewise change all occurrences of "user data" to "data about the end-user" and all occurrences of "user claims" to "Claims about the End-User".  This aligns the terminology with that used by OpenID Connect.

3.2 transaction_id: Replace "the claims parameter" with "the <spanx style="verb">claims</spanx> parameter" so that the protocol element "claims" is appropriately formatted in the output.  Likewise, add spanx style verb to all other places where parameter names appear in the document.

4. Verified Data Representation: Change "mixup" to "mix up".

4. Verified Data Representation: Change "section JSON Schema" to "<xref target="JSONSchema"/>" (or whatever the right anchor tag is) so that xml2rfc emits a "Section 13" section link.  (Use the Markdown syntax "# JSON Schema # {#JSONSchema}" to generate the anchor tag for the section title.)  Likewise, change all other ASCII section name references to xrefs so that section number links are emitted.

4.1.  verification Element: "defined in Trust Frameworks" is another example of a section reference that needs to use xref.

4.1.  verification Element: Change "id" to "verification_process" or another suitable identifier that is not "id".  The "id" identifier is widely used by the Facebook graph and other JSON data structures as the primary key for database lookup of the object.  (This is so widely used that we should probably mark it as reserved in the JWT Claims registry.)  Attempting to reuse this identifier would only lead to trouble.

4.1.1.1. id_document: This is a run-on sentence: "The method used to verify the id document, predefined values are given in Verification Methods<https://openid.net/specs/openid-connect-4-identity-assurance-04.html#predefined_values_vm>".  Change the comma to a period or semicolon and add a period to the end.  The sentence after "type" likewise needs to change the comma to a period.

5. Requesting Verified Person Data: Change the title of this section to "Requesting Verified Claims".

5.1. Requesting User Claims: Change the title of this section to "Requesting Verified Claims About the End-User".

6. Examples: Change "The third section illustrate" to "The third section illustrates".

6.1.  id_document:  Add a "region" value to the place_of_birth example.  Likewise in 6.3.

8.  Transaction-specific Purpose:  Change instances of "IDP" to "OP".

12.  Predefined Values:  A blank line is needed after each (Identifier, Definition) pair to increase readability of the tables.

                                                       Thanks,
                                                       -- Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-ab/attachments/20190601/8df24f36/attachment.html>