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

Torsten Lodderstedt torsten at lodderstedt.net
Thu Jun 20 15:05:20 UTC 2019


Hi Mike,

> On 1. Jun 2019, at 02:16, Mike Jones <Michael.Jones at microsoft.com> wrote:
> 
> Thanks for working on this, Torsten and Daniel.

Thank you for your detailed review and substantial feedback! I incorporated it into the new version -05.

> 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).


Makes sense - I changed it to “verified_claims” based on your recent feedback on the list. 

>  
> 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?)

In my opinion, aggregated claims can serve exactly this purpose. 

Do you have more information about those ongoing initiatives? I would like to do a cross check.

>  
> 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.

done 

>  
> 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.

Will do when I change the name of the claim. 

>  
> 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.

It kind of is defined in NIST SP 800-63B, but the term there is "Authenticator Assurance Level". But I feel introduction of this terminology would be a bit too much and therefore rephrased the introduction using more general terminology. 

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

done

>  
> 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.

Good point - I have completely re-written this section. 

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

I’m not sure what a reasonable resource would be. I could refer to the respective German Act or the EU’s AML Directive, …. Any suggestion is appreciated.

>  
> 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.

Good point! done. 

>  
> 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”.

done

>  
> 4.1. verification Element: Add a reference for “ISO 8601:2004”.

Done and added references to ISO 3166-1 and ISO 3166-3

BTW: I noticed the URL in the ISO 3166-1 reference of the OIDC Core Spec points to http://www.w3.org/WAI/ER/IG/ert/iso639.htm, a page containing ISO 639 Language Codes instead of country codes. Might be worth changing this in the Errata?
 
>  
> 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”.

done. 

>  
> 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.

good catch - done 

>  
> 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?

Very good question. I see the ways: (1) either the RP requests a certain locale using ui_locales and also sends the purpose in that locale or (2) there is additional metadata associated with the purpose parameter. I think (1) is sufficient and added this sentence 

"In order to ensure a consistent UX, the RP MAY send the `purpose` in a certain language and request the OP to use the same language using the `ui_locales` parameter."

>  
> 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.  

Good point. The respective JSON Schema definitions already had “additionalProperties” set to false 

Added notes to the respective sections.

>  
> 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.)

The JSON scheme proofed very useful for ma as it allowed me to automatically validate all examples. I think implementors will appreciate it for similar reasons and it could be an ingredient of conformance tests.

I agree, spec text and JSON scheme must not contradict. I don’t see an issue if the Schema is more detailed and precise than the prose as long as the Schema is a normative part of the spec. 

>  
> 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.

done 

>  
> 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.

I’m sorry to have caused a jarring experience ;-) I should have checked in advance. I wiped all occurrences of evidences (including the respective sub-element). 

>  
> 1. Introduction:  Change “that is defined in this specification” to “, which is defined in this specification”.

rephrased introduction 

>  
> 1. Introduction:  Change “a certain OpenID Connect transaction” to “an OpenID Connect transaction”.

done

>  
> 1. Introduction:  Put commas around the phrase “as defined in Section 2 of the OpenID Connect specification [OpenID]”.

done

>  
> 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”.)

done 

>  
> 2. Scope and Requirements: Change “User Info” to “UserInfo”.

done 

>  
> 2. Scope and Requirements: There should always be a comma after “For example”.

done

>  
> 2. Scope and Requirements: Change “in case of eIDAS” to “in the case of eIDAS”.

done 
>  
> 2. Scope and Requirements: Delete “basically”, as it doesn’t add to the meaning of the sentence.

Removed during rewriting :-)

>  
> 2. Scope and Requirements: Change “Note: data transfer” to “Note: Data transfer”.

Incorporated the statement into the existing text in the Privacy Considerations section. 

>  
> 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.

done 

>  
> 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.

Fixed - I think you discovered one of the rare places where I hadn’t specially formatted a parameter or Claim :-)

>  
> 4. Verified Data Representation: Change “mixup” to “mix up”.

done 

>  
> 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.
>  

done 

> 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.

Changed it to verification_process.

Not really editorial, right ;-)? 

>  
> 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”.  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.

done 

>  
> 5. Requesting Verified Person Data: Change the title of this section to “Requesting Verified Claims”.

done 
>  
> 5.1. Requesting User Claims: Change the title of this section to “Requesting Verified Claims About the End-User”.

done

>  
> 6. Examples: Change “The third section illustrate” to “The third section illustrates”.

done 

>  
> 6.1.  id_document:  Add a “region” value to the place_of_birth example.  Likewise in 6.3.

Region value are not used in German id document. 

I would be happy to include another example from the US including a region. 

>  
> 8.  Transaction-specific Purpose:  Change instances of “IDP” to “OP”.

done 

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

Transformed definitions into tables. 

best regards,
Torsten. 

>  
>                                                        Thanks,
>                                                        -- Mike

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3923 bytes
Desc: not available
URL: <http://lists.openid.net/pipermail/openid-specs-ab/attachments/20190620/1ec41224/attachment.p7s>


More information about the Openid-specs-ab mailing list