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=40pingidentity....@dmarc.ietf.org> 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
<michael_b_jo...@hotmail.com> 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 -- oauth@ietf.org
To unsubscribe send an email to oauth-le...@ietf.org
/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 -- oauth@ietf.org
To unsubscribe send an email to oauth-le...@ietf.org
_______________________________________________
OAuth mailing list --oauth@ietf.org
To unsubscribe send an email tooauth-le...@ietf.org
