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

[Kristina Yasuda <[e-mail]>]

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