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

[Brian Campbell <[e-mail]>]

Michael B. Jones, Sir,

I have endeavored to reply inline below to the remaining comments
and/or addressing them with further updates to the source of the draft,
which will appear in the -11 revision that hopefully will be published
soon.

I have the honor to be Your Obedient Servant,
B. Cam

On Sat, Aug 17, 2024 at 12:06 PM Michael Jones <[e-mail]>
wrote:

> Thanks very much for addressing the majority of my comments and for
> replying to the rest of them.  I’m replying inline to the remaining ones
> that I believe should still be addressed.  I’ve deleted the ones from this
> thread that I’m satisfied with the responses to.
>
>
>
>                                                                 Best
> wishes,
>
>                                                                 -- Mike
>
>
>
> *From:* Kristina Yasuda <[e-mail]>
> *Sent:* Wednesday, August 14, 2024 6:07 AM
> *To:* Brian Campbell <bcampbell=[e-mail]>
> *Cc:* Michael Jones <[e-mail]>; [e-mail]
> *Subject:* Re: [OAUTH-WG] Re: Review of
> draft-ietf-oauth-selective-disclosure-jwt-10
>
>
>
> 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
>
>
>
> *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.*
>
>
>
> Since “Holder” is sometimes used in the spec to refer to the Wallet
> software and sometimes used to refer to the person using the Wallet, I
> believe that readers would be well served to make a clear terminological
> distinction between the two.
>

We've intentionally avoided using the term "Wallet" to date and should
remain consistent with that. But will expand the text defining Holder to
better accommodate its use throughout the document.


 *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".*
>
>
>
> The fact that a misunderstanding is possible and occurred leads me to
> believe that a clarification of this phrase is needed.  Readers won’t
> necessarily know where the “array elements” being referred to occur in the
> data structures.  That was certainly my experience, hence my initial
> comment.  Possible wording that’s simpler than my initial proposed wording
> could be “array elements occurring within Claim values”.  Or clarify it
> with different wording of your choosing.
>

While I am personally quite sympathetic to the "that a misunderstanding is
possible" line of reasoning, the use of the words "array elements" to
mean array elements is the most concise and meaningful terminology the
document authors were able to conceive of to convey the concept of elements
in arrays. I believe it would be a disservice to readers to embellish the
wording.



> *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".*
>
>
>
> Same response as my previous comment.
>

Same response as my previous comment but I can't help but wonder if there's
a different misunderstanding at play here...



>  *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.*
>
>
>
> Yes, the definition is clear, but it’s in the wrong place.  Definitions
> belong in the Terms and Definitions section.  The current location leaves
> it strangely hang in a different section by itself, separate from the other
> definitions in the spec.
>

The editors consider the current location, a section called "Conventions
and Terminology", sufficient and appropriate for the mention of a piece of
terminology established by convention in a previously published RFC.


*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.*
>
>
>
> The current wording is a sentence fragment – not a complete sentence.  The
> fact that that error slipped through in DPoP isn’t a reason not to correct
> it here.
>

There is indeed a misunderstanding on this one. The comparable wording in
DPoP/RFC9449 is actually a complete sentence and this draft will be fixed
so that it will also have a complete sentence.


 *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.*
>
>
>
> OK.  Please address this then by adding this normative sentence at an
> appropriate place in the specification.  “When the SD-JWT contains a cnf
> value with a jwk member, this key MUST be used to very the Key Binding JWT.”
>

https://github.com/oauth-wg/oauth-selective-disclosure-jwt/pull/435 made
the cnf claim the way to enable key binding with that increased
interoperability as a primary aim.


>  *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.*
>
>
>
> The fact remains that the current wording is factually incorrect and
> therefore needs to be corrected.  If this can be done by updating the
> tooling, that’s actually good, since it means that it will be consistently
> corrected everywhere the error occurs.
>

The statement that the current wording is factually incorrect is itself
factually incorrect. The current wording might not be as descriptive as
you'd like but it is correct. In what way does the encoding make it not a
SHA-256 Hash? Asking rhetorically.


> *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.*
>
>
>
> Sure, it’s an example, but the examples should nonetheless reflect best
> practices.  Please delete “nbf” here.
>

I'd be curious to see some documentation or citation, beyond your personal
opinion, that supports the assertion that any of that is "best practice".

There are two occurrences of "[like or such as] nbf, iat, and exp" in the
draft.

In
https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-11.4-8
they are mentioned in the context of "claims carrying time information",
which is what they all are.

In
https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-8.1-4.6
they are mentioned in the context of claims to check regarding the validity
of the SD-JWT. The "nbf" claim, as you likely know from (I think?) having
written the text in RFC7519
<https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.5>, has
normative requirements around when the "JWT MUST NOT be accepted for
processing." As such, it is entirely appropriate in the context of
mentioning some validity-controlling claims. The only normative requirement
on the "iat" claim
<https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.6>, however, is
about the syntax of its value. Rejecting a token based on the "iat" value
can be required by a specific profile of JWT, such as was done in DPoP, but
is otherwise something that is at the discretion of an
implementation/deployment and something that has apparently been the cause
of some interoperability problems
<https://mailarchive.ietf.org/arch/msg/oauth/Qkz2HqOzdVM0oyDPSeOdo-iLN8E/>.
Arguably "iat" shouldn't be in that list.


 *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.*
>
>
>
> As above, the examples should reflect best practices.  Please at least add
> “iat” to this list, even if you choose to retain “nbf”.
>

As above, but in the context of the content in that section, "iat" does not
belong in this list.



> 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 wouldn’t say that there’s a misunderstanding in how disclosure in arrays
> work.  Rather, I’d say that the current exposition of array disclosure can
> leave readers wondering what is meant, and could be improved by the
> suggested clarifications above.
>
>
>
> Thank you for explaining that “…” is intended to be understood as the
> ellipsis character, and the intuition behind it.  Its usage in that way is
> slightly odd, since the ellipsis character is used in place of omitted
> content, whereas in this case it’s used with the opposite meaning to
> introduce content that is NOT omitted (content that is disclosed).
>

I don't see how it is at all odd; the “…” and disclosure digest are used in
place of content omitted from the main payload. The ellipsis is often used
in place of content omitted from the context at hand.



>
> Be that as it may, I understand very well the reasons for keeping the
> claim name as it is.  Nonetheless, I think it would be helpful to readers
> to add a comment when the claim name is introduced about why this otherwise
> non-obvious claim name was chosen.  The sentence you add could be something
> like “The claim name “…” was chosen because the ellipsis character,
> typically entered as three period characters, is used in contexts where
> content is omitted (not disclosed).”  Or choose your own wording.  Give
> readers the intuition behind the choice.
>

Will add something along those lines to the draft.



>
> Finally, this comment was incompletely addressed:
>
> Section *11.1. *
> <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#section-11.1>*Storage
> of Signed User Data*
> <https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-10.html#name-storage-of-signed-user-data>:
> Please change “OpenID Connect” to “OpenID Connect [OpenID.Core]” (adding
> the missing citation).
>

Will add that missing citation.



>
>
>                                                                 Best
> wishes,
>
>                                                                 -- Mike
>
>
>

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