Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the optionality of the cnf claim #213

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Conversation

bc-pi
Copy link
Collaborator

@bc-pi bc-pi commented Feb 20, 2024

Clarify the optionality of the cnf claim (to fix #196)

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if there might be a difference in how a key inside cnf claim is represented (jwk, kid, etc) it should still be cnf claim for the interop purposes, no? What are you trying to account for by making cnf optional?

I think it should remain "required when cryptographic binding is used"

@bc-pi
Copy link
Collaborator Author

bc-pi commented Feb 20, 2024

Hang on. Even if there might be a difference in how a key inside cnf claim is represented (jwk, kid, etc) it should still be cnf claim for the interop purposes, no? What are you trying to account for by making cnf optional?

please see issue #196 and note that the text in this PR has "This [cnf] claim MUST be present when cryptographic Key Binding is to be supported."

Also note this PR isn't for issue #205 and it was probably a mistake on my part to have suggested text there that touches two issues. But they were both on cnf so I did what I did [no pun intended]. This PR only attempts to clarify the optionality of the cnf claim for issue #196. There doesn't yet seem to be consensus on #205.

@Sakurann
Copy link
Collaborator

@bc-pi you probably made the comment while I was updating my original comment based on re-reading the PR. I still think it should remain "required when cryptographic binding is used"

Copy link
Collaborator

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this version. CNF is more optional to me and having required as the first keyword seems less right

@Sakurann
Copy link
Collaborator

Sakurann commented Feb 21, 2024

CNF is more optional to me

what does this mean? you believe sd-jwt-vc should have more than one way to do cryptographic holder binding? or that many of the credentials will not use cryptographic holder binding?

@paulbastian
Copy link
Collaborator

No, I just mean SD-JWT VCs can be key bound, but they don't need to be. Therefore it is an optional feature.

@paulbastian
Copy link
Collaborator

I see, the difference here is that if key binding shall be used, then cnf must be used while the other formulation leaves more space?

@Sakurann
Copy link
Collaborator

If that's the intent, required when cryptographic binding is used sounds more appropriate to me. just saying optional would lead to confusion that it is optional even when cryptographic binding is used.

@awoie
Copy link
Collaborator

awoie commented Feb 21, 2024

I also like the proposed version more. The proposed language addresses #196 which is based on developer feedback that were confused by the current language that uses REQUIRED if ... .

So I think that OPTIONAL seems to solve that issue. IMO, it is also appropriate since we have the additional requirement in the paragraph that says MUST be present if cryptographic binding is required.

Copy link
Collaborator

@awoie awoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, see my comment in the discussion.

@Sakurann
Copy link
Collaborator

if key binding shall be used, then cnf must be used while the other formulation leaves more space?

Sorry, I don't understand why more space is needed here? The less optionality in key binding, more interop. with the current PR, it would be ok to use a top level sub claim and put a DID there (instead of using cnf.kid). I think it is harmful for the interop, that SD-JWT VC is hoped to bring

@awoie
Copy link
Collaborator

awoie commented Feb 23, 2024

if key binding shall be used, then cnf must be used while the other formulation leaves more space?

Sorry, I don't understand why more space is needed here? The less optionality in key binding, more interop. with the current PR, it would be ok to use a top level sub claim and put a DID there (instead of using cnf.kid). I think it is harmful for the interop, that SD-JWT VC is hoped to bring

I don't see that this optionality is allowed by the following?

This claim MUST be present when cryptographic Key Binding is to be supported.

@paulbastian
Copy link
Collaborator

if key binding shall be used, then cnf must be used while the other formulation leaves more space?

Sorry, I don't understand why more space is needed here? The less optionality in key binding, more interop. with the current PR, it would be ok to use a top level sub claim and put a DID there (instead of using cnf.kid). I think it is harmful for the interop, that SD-JWT VC is hoped to bring

I see your point. The proposed text opens a door for key binding without using the cnf claim. However, the new text is more initiative and we could add this constraint as an additional sentence or in the section on key binding

@bc-pi bc-pi closed this Feb 23, 2024
@bc-pi bc-pi reopened this Feb 23, 2024
@paulbastian
Copy link
Collaborator

Okay, it is already present. Sorry I didn't recheck the changes. Lgtm then

@bc-pi
Copy link
Collaborator Author

bc-pi commented Feb 23, 2024

I'm struggling to understand the disagreement/argument here. But I'm trying...

The text for cnf in the PR has:

OPTIONAL. Contains the confirmation method identifying the proof of possession key as defined in [@!RFC7800]. This claim MUST be present when cryptographic Key Binding is to be supported. It is RECOMMENDED that this contains a JWK as defined in Section 3.2 of [@!RFC7800]. For proof of cryptographic Key Binding, the Key Binding JWT in the presentation of the SD-JWT MUST be signed by the key identified in this claim.

which seems to me to clearly says that cnf is the required way to do key binding, if doing key binding. Especially the "This claim MUST be present when cryptographic Key Binding is to be supported" sentence.

Would changing that sentence to "This claim is REQUIRED when cryptographic Key Binding is to be supported" help? Maybe I'm missing the point of contention but that and the current PR text and the current text in the draft all say the same thing - that cnf is the one and only way to do key binding in SD-JWT VC but b/c key binding isn't itself required, the claim also isn't required.

draft-ietf-oauth-sd-jwt-vc.md Outdated Show resolved Hide resolved
@bc-pi
Copy link
Collaborator Author

bc-pi commented Feb 26, 2024

Would changing that sentence to "This claim is REQUIRED when cryptographic Key Binding is to be supported" help?

I thought that was better phrasing anyway so made the change w/ 9edae9d

@awoie
Copy link
Collaborator

awoie commented Feb 26, 2024

Would changing that sentence to "This claim is REQUIRED when cryptographic Key Binding is to be supported" help?

I thought that was better phrasing anyway so made the change w/ 9edae9d

Seems great to me.

Copy link
Member

@danielfett danielfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally fine with the change, but I think I want to be pedantic here and resolve the contradiction.

Co-authored-by: Daniel Fett <fett@danielfett.de>
@bc-pi
Copy link
Collaborator Author

bc-pi commented Feb 27, 2024

I'm generally fine with the change, but I think I want to be pedantic here and resolve the contradiction.

You're suggested pedantic wording is indeed better than what I'd had. Thanks. I've accepted it w/ c6b1380

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@awoie awoie merged commit e4e3b69 into main Feb 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider making cnf optional
5 participants