oauth-wg / oauth-selective-disclosure-jwt

https://datatracker.ietf.org/doc/draft-ietf-oauth-selective-disclosure-jwt/
Other
54 stars 27 forks source link

Distinguish SD-JWT from SD-JWT+KB #394

Closed bifurcation closed 3 weeks ago

bifurcation commented 7 months ago

This PR limits the scope of the term "SD-JWT" so that it refers only to an Issuer-signed JWT and a set of disclosures. We introduce the term "Fnord" to refer to an SD-JWT with key binding. As a bonus, we are able to clean up stale uses of Presentation, which mostly, but not always align with Fnord.

"Fnord" is deliberately nonsensical, of course. We should replace this with a more intuitive word. We could reintroduce "Presentation" for this, with the allowance that one might "present" either a Presentation or an SD-JWT. Or we could pick another term, suggestions welcome.

I have tried to be fairly minimal in the changes here. In particular, I have not changed any of the algorithms, though I suspect they would be clearer if a bit more separate. I have rewritten a few paragraphs, especially close to the top and in the Security Considerations.

Fixes #374 Fixes #392

OR13 commented 7 months ago

Its very confusing when communicating regarding SD-JWT.

You really can't ever use the term by itself.

You must use the term with:

"Issuer Signed SD-JWT" and "Holder Disclosed SD-JWT".

If you don't make this clear, its easy to imagine scenarios where implementers accidentally disclose more than they should.

Sakurann commented 6 months ago

I find it immensely challenging to read and review this PR with Fnord being used as a term. please replace it with some specific term that we can bikeshed.

Sakurann commented 5 months ago

@bifurcation can you please replace Fnord with SD-JWT/KB, which is a short form for SD-JWT with Key Binding (SD-JWT+KB could have worked too, but we were just worried it will be confused with media types, etc.), so that reviewing is easier.

bifurcation commented 5 months ago

@Sakurann - Changed to SD-JWT-KB, since the SD-JWT/KB looked weird to me (division seemed like the wrong metaphor) and SD-JWT~KB seemed a little too cute. Also merged master while I was at it.

Sakurann commented 4 months ago

I think this PR as-is now also addresses #356

danielfett commented 4 months ago

@Sakurann - Changed to SD-JWT-KB, since the SD-JWT/KB looked weird to me (division seemed like the wrong metaphor) and SD-JWT~KB seemed a little too cute. Also merged master while I was at it.

I think I like the aesthethics of SD-JWT+KB or SD-JWT/KB or SD-JWT|KB more than SD-JWT-KB.

bc-pi commented 4 months ago

@Sakurann - Changed to SD-JWT-KB, since the SD-JWT/KB looked weird to me (division seemed like the wrong metaphor) and SD-JWT~KB seemed a little too cute. Also merged master while I was at it.

I think I like the aesthethics of SD-JWT+KB or SD-JWT/KB or SD-JWT|KB more than SD-JWT-KB.

I think aesthethics is spelled aesthetics but otherwise agree (I'm usually the one misspelling things so happy not to be this time...).

I'd lean towards SD-JWT+KB (but did smile at SD-JWT~KB despite being a little too cute).

bifurcation commented 3 months ago

@selfissued - The point of this PR is that Key Binding isn't just an optional feature -- it's a feature that causes a security failure if you forget to check that it's enabled. Just like it's a basic security principle that one API endpoint should never accept both signed and unsigned data (because of the trivial downgrade risk, see "alg": "none"), there should never be a verification process that will accept both SD-JWT and SD-JWT-KB. Thus, we should talk about them as separate things.

bc-pi commented 3 months ago

Note for posterity that the meeting notes https://notes.ietf.org/notes-ietf-119-oauth# have some more of the discussion around this and related topics that happened on the Wednesday of IETF 119. Although I don't think there's anything particularity novel there that's not been expressed in this PR or the linked issue(s).

selfissued commented 3 months ago

@selfissued - The point of this PR is that Key Binding isn't just an optional feature -- it's a feature that causes a security failure if you forget to check that it's enabled. Just like it's a basic security principle that one API endpoint should never accept both signed and unsigned data (because of the trivial downgrade risk, see "alg": "none"), there should never be a verification process that will accept both SD-JWT and SD-JWT-KB. Thus, we should talk about them as separate things.

@bifurcation, I believe you're missing my point. To correctly accept a key-bound SD-JWT, the recipient MUST have code that:

  1. Verifies that a key binding is present in the SD-JWT.
  2. Verifies the key binding signature.

Only then can the recipient know that the SD-JWT is key bound.

Introducing a separate media type asserting that the SD-JWT is key bound doesn't remove the need for the recipient to verify the presence and signature of the key binding as above, if it wants a key-bound SD-JWT. If anything, introducing a key-bound-SD-JWT media type could increase the probability that recipients would be lazy thinking that "The media type says it's key-bound so that must be true." and not do steps 1 and 2 above - which is what actually needs to be done to securely accept a key-bound SD-JWT.

Introducing a separate media type doesn't help from a security perspective, and in practice, it may hurt. For that reason, I am opposed to introducing a separate media type.

...

For what it's worth, @ve7jtb made similar comments at the microphone during the OAuth WG meeting at IETF 119 in Brisbane.

bc-pi commented 3 months ago

@selfissued, would removing the `application/sd-jwt-kb' media type from this PR sufficiently alleviate your concern? Or is it more than that?

(note, I'm hopeful you and @danielfett / @Sakurann can discuss this further in person at OSW and/or IIW soon but I'm unfortunately not going to be at either so just trying to nudge the conversation)

danielfett commented 2 months ago

Edit: I still think we should call it "SD-JWT+KB", which nicely captures that the KB is an "add-on".

I would appreciate if the PR could change SD-JWT-KB to SD-JWT+KB.

bc-pi commented 2 months ago

Edit: I still think we should call it "SD-JWT+KB", which nicely captures that the KB is an "add-on".

I would appreciate if the PR could change SD-JWT-KB to SD-JWT+KB.

He did ask nicely.

selfissued commented 2 months ago

@selfissued, would removing the `application/sd-jwt-kb' media type from this PR sufficiently alleviate your concern? Or is it more than that?

Yes, my objection is to having two media types. Once the extra media type is removed, I should be fine with the updated description.

Sakurann commented 2 months ago

I am ok with changing SD-JWT-KB to SD-JWT+KB

bc-pi commented 2 months ago

@selfissued, would removing the `application/sd-jwt-kb' media type from this PR sufficiently alleviate your concern? Or is it more than that?

Yes, my objection is to having two media types. Once the extra media type is removed, I should be fine with the updated description.

The extra media type removed with 98383e665a17fff50654a0a096eaf5e8cfe001dc, which I'm perfectly okay with and coming around to even thinking is the right thing. But I am struck by the irony of doing this on the same day you've (I know, on behalf of the WG) requested registration of:

application/vc+ld+json application/vp+ld+json application/vc+ld+json+jwt application/vp+ld+json+jwt application/vc+ld+json+sd-jwt application/vp+ld+json+sd-jwt application/vc+ld+json+cose application/vp+ld+json+cose +ld+json+jwt +json+jwt +ld+json+sd-jwt +json+sd-jwt +ld+json+cose +json+cose

bc-pi commented 2 months ago

Edit: I still think we should call it "SD-JWT+KB", which nicely captures that the KB is an "add-on".

I would appreciate if the PR could change SD-JWT-KB to SD-JWT+KB.

92d20f848299bb2d93f99d4d392f663341e08c9d made that change

bc-pi commented 2 months ago

Not entirely sure about the SD-JWT-KB vs SD-JWT+KB part. Should the kb-jwt then also be renamed to kb+jwt (even outside the typ)?

I'm gonna say no.

bc-pi commented 1 month ago

@bifurcation and @danielfett and @Sakurann to hopefully do a re-review soon(ish) and get this one merged soon(ish)

Sakurann commented 3 weeks ago

Thank you, Richard "fnord" Barnes

bc-pi commented 3 weeks ago

Thank you, Richard "fnord" Barnes

It's the little things that keep me going