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 <[email protected]>
Sent: Wednesday, August 14, 2024 6:07 AM
To: Brian Campbell <[email protected]>
Cc: Michael Jones <[email protected]>; [email protected]
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.
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.
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.
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.
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.
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.”
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.
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.
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”.
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).
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.
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).
Best wishes,
-- Mike
_______________________________________________
OAuth mailing list -- [email protected]
To unsubscribe send an email to [email protected]
