[OAUTH-WG] Re: Review of draft-ietf-oauth-selective-disclosure-jwt-10

[Denis <[e-mail]>]

Hi Kristina,

I concur with Mike: the language needs to be adjusted .

>         *1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-1>Introduction
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-introduction>: 
>         The usage of “Holder” in “used a number of times by the user
>         (the "Holder" of the JWT)” inconsistent with its usage in the
>         definitions, which defines “Holder” as being “an entity”.  The
>         usage here in the introduction makes the holder into a person
>         rather than an entity.  Please adjust the language here to not
>         confuse the user who utilizes the holder with the holder itself.*
>
> *-> Holder is defined as an entity and a person can be considered an 
> entity. We also have a phrase "the user (the "Holder" of the JWT)" in 
> the introduction. Hence the editors consider the current wording to be 
> sufficient.*

The following definition is confusing:

    Holder: An entity that received SD-JWTs from the Issuer and has
    control over them.

Is this "entity" a human being or an application ? It can't be both.

It is important to make the difference between an individual (the 
holder) and an "holder application" (e.g., a digital identity wallet).

About Key Binding:

    Key Binding: Ability of the Holder to prove legitimate possession of
    an SD-JWT by proving control over a private key during the
    presentation.

An application under the control of a Holder can do it, but an 
individual is unable to perform the necessary cryptographic computations 
to prove it.

For example, in Figure 1: SD-JWT Issuance and Presentation Flow, 
"Holder" should be changed into "Holder application".

Two terms should be defined and used: "Holder" and "Holder application". 
The two following definitions are proposed as a start:

    Holder: individual that has the control over an Holder application

    Holder application: application able to receive SD-JWTs from an
    Issuer and to process them

The introduction states:

    However, while both JWT and SD-JWT have potential OAuth 2.0
    applications, their utility and application is certainly not
    constrained to OAuth 2.0.

Outside the context of the OAuth 2.0 Framework, *collusion attacks* 
between individuals will need to be considered, and
the Holder application will need to be a TA (Trusted Application) 
running in a TEE (Trusted Execution environment).
Unfortunately, the acronyms TA (Trusted Application) and TEE (Trusted 
Execution environment) appear nowhere in the document
whereas they should appear in section 10.  "Security Considerations".

When requesting a digital credential to an Issuer (which is *not* the 
topic of this document), it is the Holder application that generates key 
pairs
before sending a public key to the Issuer in its credential request. The 
Holder application will need to demonstrate to the Issuer that the key pair
has indeed been generated by a TA. An individual does not generate key 
pairs.

However, it is the individual (the person soon-to-become a Holder) that 
instructs his digital identity wallet to request digital credentials to 
an Issuer.

As the vocabulary is going to be used in other contexts / documents, it 
is important to make the difference between a "Holder" and a "Holder 
application".

Denis

> Thank you very much, Mike.
>
> Majority of your comments have been incorporated in this PR 
> https://github.com/oauth-wg/oauth-selective-disclosure-jwt/pull/452, 
> which has been merged.
> Below, in bold, please find explanations for the points that have not 
> been reflected.
>
> We appreciate your review.
>
> Best,
> Kristina
>
> On Mon, Aug 5, 2024 at 8:11 PM Brian Campbell 
> <bcampbell=[e-mail]> wrote:
>
>     Thanks Mike! The detailed review is appreciated. The document
>     authors will work on addressing and/or responding to the comments.
>     As you are no doubt aware, doing so can take some time, so in the
>     meantime I wanted to send this quick note of acknowledgement and
>     thanks.
>
>     On Sun, Aug 4, 2024 at 9:56 PM Michael Jones
>     <[e-mail]> wrote:
>
>         I read
>         https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html
>         in its entirety, resulting in these suggestions. Some are for
>         readability.  Some are for consistency with related
>         specifications, including JWT.  Some are for security and
>         correctness.  The most important comments, including those
>         addressing correctness issues, are in red.
>
>         *1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-1>Introduction
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-introduction>: 
>         The usage of “Holder” in “used a number of times by the user
>         (the "Holder" of the JWT)” inconsistent with its usage in the
>         definitions, which defines “Holder” as being “an entity”.  The
>         usage here in the introduction makes the holder into a person
>         rather than an entity. Please adjust the language here to not
>         confuse the user who utilizes the holder with the holder itself.*
>
> *-> Holder is defined as an entity and a person can be considered an 
> entity. We also have a phrase "the user (the "Holder" of the JWT)" in 
> the introduction. Hence the editors consider the current wording to be 
> sufficient.*
>
>         *1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-1>Introduction
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-introduction>: 
>         In “"Claims" here refers to both object properties (name-value
>         pairs) as well as array elements.” change “array elements” to
>         “elements in arrays that are the values of name/value pairs”
>         (or something like that).  Without saying what the array
>         elements are, readers will be confused about what’s being
>         referred to.  I’d apply this clarification in 4.1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-4.1>SD-JWT
>         and Disclosures
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-sd-jwt-and-disclosures>
>         *and other applicable locations as well.
>
> *->  There is a misunderstanding. the intended meaning here is exactly 
> what the text says: "each element in the array (regardless if it is a 
> name/value pair or not, an array element as a string counts too) can 
> be selectively disclosed".*
>
>         *1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-1>Introduction
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-introduction>: 
>         In “they hold the private key associated to the SD-JWT”,
>         change “associated to” to “associated with”.*
>
> *-> there is a historical reason behind this wording that the editors 
> would like to respectfully preserve: 
> https://github.com/oauth-wg/oauth-selective-disclosure-jwt/pull/394#discussion_r1416409939. 
> *
>
>         *1.1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-1.1>Feature
>         Summary
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-feature-summary>*: 
>         In “A format for enabling selective disclosure in nested JSON
>         data structures, supporting selectively disclosable object
>         properties (name-value pairs) and array elements”, consider
>         expanding “array elements” in the same manner as the preceding
>         comment to make the meaning more evident.
>
> *->  same as above. There is a misunderstanding. the intended meaning 
> here is exactly what the text says: "each element in the array 
> (regardless if it is a name/value pair or not, an array element as a 
> string counts too) can be selectively disclosed".*
>
>         *1.2.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-1.2>Conventions
>         and Terminology
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-conventions-and-terminology>*: 
>         I suggest moving the “base64url” definition to the Terms and
>         Definitions section and use a parallel structure to that at
>         https://www.rfc-editor.org/rfc/rfc7519.html#section-2.
>         Specifically, say “The “base64url” term denotes the URL-safe
>         base64 encoding without padding defined in Section 2 of
>         [RFC7515].”  Then introduce the rest of the definitions with
>         the phrase “These terms are defined by this specification:” as
>         is done in RFC 7519.  The current presentation is fairly jarring.
>
> *-> Current text already points to Section 2 of RFC7515. The editors 
> consider the current definition sufficient and clear enough.*
>
>
>         *5.2.4.2.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-5.2.4.2>Array
>         Elements
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-array-elements>*:
>         I suggest adding an example also enabling disclosing a second
>         array element, so that the syntax will be clear to readers. 
>         In other words, show what “unless a matching Disclosure for
>         the second element is received” would look like.  Or add a
>         forward reference showing a place that it’s done.
>
> *-> There is an example where all array elements are selectively 
> disclosed here: 
> https://drafts.oauth.net/oauth-selective-disclosure-jwt/draft-ietf-oauth-selective-disclosure-jwt.html#section-5.2.6-4, 
> which should be sufficient, since we cannot put examples for 
> everything everywhere.*
>
>         *5.3.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-5.3>Key
>         Binding JWT
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-key-binding-jwt>*: 
>         Change “MUST NOT be none.” to “It MUST NOT be "none".”.
>
> *-> Changed to “It MUST NOT be `none`.”. it is exactly the same 
> wording as in DPoP RFC.*
>
>         *5.3.2.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-5.3.2>Validating
>         the Key Binding JWT
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-validating-the-key-binding->*:
>         In “if the SD-JWT contains a cnf value with a jwk member, the
>         Verifier could parse the provided JWK and use it to verify the
>         Key Binding JWT.”, change “could” to “MUST”.  Make this
>         normative to increase interoperability!
>
> *-> the sentence you point at is an example, so we cannot add a 
> normative statement here. We changed "should" to "would" to reflect 
> the intent behind the suggestion. *
>
>         *6.1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-6.1>Issuance
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-issuance>*:
>         There are many places from here on where the label “SHA-256
>         Hash” is used, for instance “SHA-256 Hash:
>         jsu9yVulwQQlhFlM_3JlzMaSFzglhQG0DpfayQwLUK4”. Change all of
>         these to “Base64url-Encoded SHA-256 Hash” for correctness.
>
> *-> Editors consider it to be sufficiently clear from 
> the specification text that it is a base64url-endoded hash. This would 
> also require changes to the tooling and might unintentionally make the 
> examples harder to read.*
>
>         *6.1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-6.1>Issuance
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-issuance>*:
>         Change “The following Key Binding JWT payload was created and
>         signed for this presentation by the Holder:” to “The following
>         Key Binding JWT Claims Set was created and signed for this
>         presentation by the Holder:”, again, to use the standard JWT
>         claims terminology.
>
> *-> payload is a commonly used term in JWT world, and here a term 
> payload is more appropriate since it is not only the claims set we 
> want to point to. *
>
>         *8.1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-8.1>Verification
>         of the SD-JWT
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-verification-of-the-sd-jwt>*:
>         Delete “nbf” from “claims such as nbf, iat, and exp” and
>         everywhere else in the spec, other than in *10.7.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-10.7>Selectively-Disclosable
>         Validity Claims
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-selectively-disclosable-val>*,
>         where both “iat” and “nbf” should be listed, as described in
>         my comment there.  “iat” (Issued At) is a standard validity
>         claim in JWT tokens (for instance, ID Tokens), whereas “nbf”
>         (Not Before) is rarely used, since it is only useful when
>         future-dating tokens, which is rare.
>
> *-> "nbf" is used merely as an example here. the fact that it might be 
> used rarely does not mean it cannot be used as an example, since it is 
> a registered claim in JWT RFC.*
>
>         *10.7.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-10.7>Selectively-Disclosable
>         Validity Claims
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-selectively-disclosable-val>*:
>         Add “iat (Issued At) to the validity claims list before “nbf”,
>         and change “nbf (Not Before)” to “nbf (Not Before) (if
>         present)”.  “iat” (Issued At) is a standard validity claim in
>         JWT tokens (for instance, ID Tokens), whereas “nbf” (Not
>         Before) is rarely used, since it is only useful when
>         future-dating tokens, which is rare.  Change all uses of “nbf”
>         to “iat” elsewhere in the spec.
>
> *-> same as above. "nbf" is used merely as an example here. the fact 
> that it might be used rarely does not mean it cannot be used as an 
> example, since it is a registered claim in JWT RFC.*
>
>         *12.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-12>Acknowledgements
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-acknowledgements>*:
>         Consider sorting the acknowledgements alphabetically by last
>         name, which is the usual convention.
>
> *-> thank you for the suggestion. We considered reordering, but 
> ultimately decided to keep the order as-is as there does not seem to 
> be a strict rule that the order has to be by last name.*
>
>         **
>
>         *14.2.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-14.2>Informative
>         References
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-informative-references>*: 
>         Update the reference “[I-D.ietf-oauth-sd-jwt-vc]” to
>         https://datatracker.ietf.org/doc/html/draft-ietf-oauth-sd-jwt-vc-04.
>
> *-> the reference is generated automatically. when the latest -11 of 
> sd-jwt will be published, it will be updated to -04 automatically.*
>
>         *A.1.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#appendix-A.1>Simple
>         Structured SD-JWT
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-simple-structured-sd-jwt>*:
>         Change “allowing to separately disclose individual members of
>         the claim” to “allowing separate disclosure of individual
>         members of the claim”.
>
> *-> Editor's preference to keep the phrase as-is since it has the same 
> meaning.*
>
>         *A.5.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#appendix-A.5>Elliptic
>         Curve Key Used in the Examples
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-elliptic-curve-key-used-in->*:
>         Change “The public key used to validate a Key Binding JWT can
>         be found in the examples as the content of the cnf claim.” to
>         “The public key used to validate a Key Binding JWT can be
>         found in the examples as the value of the jwk member of the
>         cnf claim.”
>
> *-> Editor's preference to keep the phrase as-is since it is 
> already clear from the text from where to get the public keys.*
>
>         *Appendix B.
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#appendix-B>Disclosure
>         Format Considerations
>         <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-disclosure-format-considera>*:
>         At the end of the paragraph introduced by “1.
>         Canonicalization”, consider adding the sentence
>         “Canonicalization schemes have a long history of creating
>         interoperability problems in practice.”
>
> *-> the suggested text is true, however it is more subjective than the 
> editors deem necessary in this context. it is already clear that 
> canonicalization approach has not been chosen because it has drawbacks.*
>
>         Everywhere:  Consider changing the name “…” to something more
>         indicative of its function, such as “_sd_element” or
>         “_sd_item”.  Or alternatively, at least provide rationale for
>         why that non-obvious name was chosen.
>
> *-> we have extensively bikeshedded this claim name, and this one was 
> chosen because it is concise and natural since '...' is what is used 
> when something is omitted in a list. there seems to have been some 
> misunderstanding how disclosure in the arrays work, so hopefully with 
> the clarifications above, this claim also feels more appropriate than 
> before.*
>
>         I hope that you find this review helpful in improving the
>         specification.  I look forward to its publication as an RFC
>         and its widespread use!
>
>         -- Mike
>
>         _______________________________________________
>         OAuth mailing list -- [e-mail]
>         To unsubscribe send an email to [e-mail]
>
>
>     /CONFIDENTIALITY NOTICE: This email may contain confidential and
>     privileged material for the sole use of the intended recipient(s).
>     Any review, use, distribution or disclosure by others is strictly
>     prohibited.  If you have received this communication in error,
>     please notify the sender immediately by e-mail and delete the
>     message and any file attachments from your computer. Thank
>     you./_______________________________________________
>     OAuth mailing list -- [e-mail]
>     To unsubscribe send an email to [e-mail]
>
>
> _______________________________________________
> OAuth mailing list [e-mail]
> To unsubscribe send an email [e-mail]