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

Separate token and presentation formats #374

Closed
bifurcation opened this issue Nov 13, 2023 · 13 comments · Fixed by #394
Closed

Separate token and presentation formats #374

bifurcation opened this issue Nov 13, 2023 · 13 comments · Fixed by #394
Labels

Comments

@bifurcation
Copy link
Contributor

This document actually defines two object formats, with distinct formats and validation processes:

  1. An object that is sent from Issuer to Holder, containing the Issuer JWT and zero or more disclosures
    • Format: IssuerJWT~disclosure~...~disclosure~
    • Validation: Validate the Issuer JWT, validate that all disclosures are present in the Issuer JWT claims
  2. An object that is sent from Holder to Verifier, containing the Issuer JWT, zero or more disclosures, and a Key Binding JWT
    • Format: Type1Object+KBJWT = IssuerJWT~disclosure~...~disclosure~KBJWT
    • Validation: Validate the KB JWT and cnf, then validate the type-(1) object

Right now, the document refers to only one format, SD-JWT, which may or may not have a KB-JWT. The document should name these two things differently and make the different processes clear. In my notes, I have called these a "token" and a "presentation", respectively, but I don't have strong feelings about terminology.

@bifurcation
Copy link
Contributor Author

To be clear, I don't think we need actual technical changes here (aside from maybe moving a ~ character cf. #375). I'm more concerned that both of these things are called "SD-JWT" right now, so when you have, for example, an API that expects an SD-JWT, that API has to deal with whether it's getting a key binding or not. The reason for proposing separate names and processes here is to make it clear that implementors should handle them differently, and avoid "cross-contamination", in particular someone thinking they got a non-replayable thing when they did not.

(This is an exact parallel to "alg": "none" in JWS, where the same syntax could encode very different security properties. The fact that JWS allowed that cross-contamination has led to a significant number of security bugs.)

So my proposal here would basically be to change some terminology and rearrange some text:

  • Make separate terms for two data distinct formats:
    • "IssuerJWT + disclosures"
    • "IssuerJWT + disclosures + KB-JWT"
    • These can be syntactically as close as they are now, but they need separate names and definitions, not a single optional field, that optionality is like the optionality of the signature in JWS(alg=none)
  • For "IssuerJWT + disclosures", the available algorithms are:
    • Validate: Validate Issuer JWT, validate that disclosures all appear (transitively) in Issuer JWT
    • "Forget": Remove a disclosure and all of its transitively dependent disclosures
    • "Reveal": Apply the disclosures to obtain the maximally unredacted claims (may be coalesced with Validate)
    • "Sign": Create an "IssuerJWT + disclosures + KB-JWT" using the key referenced in cnf
  • For "IssuerJWT + disclosures + KB-JWT", the available algorithms are
    • Validate: Validate KB JWT, then validate the "IssuerJWT + disclosures"
    • "Reveal": implicitly from the IssuerJWT + disclosures (maybe coalesced)

I admit that I have not done the work to see how the current text would map to this structure. If people are on board in principle, I could work up a PR that would make a more concrete proposal.

@bc-pi
Copy link
Collaborator

bc-pi commented Nov 13, 2023

There are indeed similarities to to "alg": "none" but I don't think it's an exact parallel.

There's a lot of text about the verifier policy decisions with respect to requiring key binding at https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-06.html#name-key-binding and also some push to loosing (or contextualize) it #368

We actually worked to consolidate the validation rules for Holder and Verifier here because there's so much commonality. With the key binding check here being the one part that's verifier specific (and yes, whether or not Key Binding is required is a policy decision of the verifier). Consolidation of the format and calling that SD-JWT was also part of that. Not saying that was necessarily the right or best thing to do but it was intentional with some thought behind it.

@bc-pi
Copy link
Collaborator

bc-pi commented Nov 14, 2023

I admit that I have not done the work to see how the current text would map to this structure. If people are on board in principle, I could work up a PR that would make a more concrete proposal.

On the general principle at a high level I think we have quite similar end goals of avoiding "cross-contamination" type confusion. I might quibble with some of the details and names and stuff. And I suspect the current text doesn't map easily at all. I'm not necessarily opposed to trying something here. But need to be conscientiousness of lots of (sometimes competing) factors including the current content of the whole draft and especially not breaking things. I realize that's not a very concrete statement - sorry! - but it's the best I could articulate.

@danielfett
Copy link
Member

I think this is not very comparable to the alg none "flaw" at all. The problem with alg none is that it seemingly is a signature, its just a "bad" algorithm. When the KB-JWT is missing, a major part of the verification would not work.

@selfissued
Copy link
Collaborator

I think that the current encoding design, which had a lot of thought put into it, does a good job balancing many real engineering tradeoffs. It would be unnecessarily disruptive to change it at this point.

With respect to "alg": "none", every security issue that's arisen from it is a consequence of implementations not following normative requirements in the specification. Those requirements, from https://www.rfc-editor.org/rfc/rfc7515.html#section-5.2, are:

Finally, note that it is an application decision which algorithms may be used in a given context. Even if a JWS can be successfully validated, unless the algorithm(s) used in the JWS are acceptable to the application, it SHOULD consider the JWS to be invalid.

It's the JWS library's job to determine if a JWS is syntactically correct. It's the application's job to determine if it's semantically correct. Applications that don't do this are abdicating their security responsibilities.

@bifurcation
Copy link
Contributor Author

@danielfett The specific text that is freaking me out here is:

Determine if Key Binding is to be checked according to the Verifier's policy for the use case at hand. This decision MUST NOT be based on whether a Key Binding JWT is provided by the Holder or not. Refer to Section 11.6 for details.

This invites people to write code that is insecure unless the operator writes a policy that corrects for the deficiencies of the code. As @selfissued points out, when things are structured that way, people will fail to write those policies in a safe way, and you get security defects.

To frame it in terms of API design:

# What the document has now
def verify_sd_jwt(sd_jwt, require_kb)

# What the document ought to have
def verify_sd_jwt(sd_jwt) # KB MUST NOT be present
def verify_sd_jwt_with_key_binding(sd_jwt) # KB MUST be present and valid

The point being that the require_kb flag has significant security implications (much like a putative "accept alg:none flag), so we shouldn't express things in those terms. You would never write a data import method that is like import_data(data, signed), because the first time someone calls it with signed=false in the wrong context, you have a security incident. Instead, you would write a separate import_data_raw_danger_danger_danger(data) method, and be very careful about where you use it. If an application doesn't care about key binding, it should explicitly transform the SD-JWT to strip it.

Net of all that ranting, though, all I'm saying here is that we should split the current verification algorithm in two, where one algorithm covers validation of the base SD-JWT properties (issuer JWT valid, all disclosures accounted for), and the other first validates the key binding, then calls the first algorithm.

@Sakurann
Copy link
Collaborator

we should split the current verification algorithm in two, where one algorithm covers validation of the base SD-JWT properties (issuer JWT valid, all disclosures accounted for), and the other first validates the key binding, then calls the first algorithm.

I think that makes sense. as it sounds like we agree that "verifier can have both def verify_sd_jwt(sd_jwt) # KB MUST NOT be present and def verify_sd_jwt_with_key_binding(sd_jwt) # KB MUST be present and valid and calling one or the other based on the credential/use-case?

@bifurcation
Copy link
Contributor Author

Yeah, I think I can live with that. It still makes me a little queasy to have the Verifier accepting documents with two different sets of security properties, but I'll take an antacid :) We might want to split the Holder->Verifier arrow in Figure 1 into two arrows, one for each type of object.

I would also still prefer we had separate terms for "SD-JWT without KB-JWT" and "SD-JWT with KB-JWT", since these have distinct formats and algorithms.

@bifurcation
Copy link
Contributor Author

Thinking on this more, in light of #356, I'm feeling increasingly confident that Token and Presentation are good terms for "SD-JWT-without-KB" and "SD-JWT-with-KB", respectively. Or perhaps to be more streamlined, "SD-JWT" for "without KB" and "[SD-JWT] Presentation" for "with KB".

Token / SD-JWT makes sense for "without-KB" because it has basically the same dynamics as a JWT. It's something the Issuer hands to the Holder, and the Holder passes on to other people. And importantly, it shares with JWT the property of replayability. The only difference here is that the Holder can drop some disclosures.

Presentation makes sense for "with-KB" because the Holder needs to take an additional operation, making a new thing using the Token and the cnf-bound private key.

You can still call the Holder--Verifier interaction "presentation", you just say that the Holder presents either a Presentation or a bare Token. The slightly awkwardness of the latter construction is productive, because it tacitly implies that you really ought to be using a Presentation to get the full security properties.

                   +------------+
                   |            |
                   |   Issuer   |
                   |            |
                   +------------+
                         |
                    Issues SD-JWT
              including all Disclosures
                         |
                         v
                   +------------+
                   |            |
                   |   Holder   |
                   |            |
                   +------------+
                       |    |
   Presents Token with |    | Presents Presentation
  selected disclosures | or | with selected disclosures
                       |    |
                       v    v
                   +-------------+
                   |             |+
                   |  Verifiers  ||+
                   |             |||
                   +-------------+||
                    +-------------+|
                     +-------------+

@bc-pi
Copy link
Collaborator

bc-pi commented Nov 28, 2023

split the current verification algorithm in two, where one algorithm covers validation of the base SD-JWT properties (issuer JWT valid, all disclosures accounted for), and the other first validates the key binding, then calls the first algorithm.

In the common/typical/expected case the key needed to verify the key binding JWT is in the cnf of the Issuer-signed JWT so that order of validation steps doesn't work very well.

@bc-pi
Copy link
Collaborator

bc-pi commented Nov 28, 2023

feeling increasingly confident that Token and Presentation are good terms for "SD-JWT-without-KB" and "SD-JWT-with-KB", respectively.

Strongly disagree with Token and Presentation being good terms in this context.

Or perhaps to be more streamlined, "SD-JWT" for "without KB" and "[SD-JWT] Presentation" for "with KB".

Maybe. If we decide to have separate terms. Which we have not.

@bifurcation
Copy link
Contributor Author

bifurcation commented Nov 28, 2023

split the current verification algorithm in two, where one algorithm covers validation of the base SD-JWT properties (issuer JWT valid, all disclosures accounted for), and the other first validates the key binding, then calls the first algorithm.

In the common/typical/expected case the key needed to verify the key binding JWT is in the cnf of the Issuer-signed JWT so that order of validation steps doesn't work very well.

Sorry for the ambiguity, I was including "check that the bound public key matches cnf" within "validate the key binding".

@bc-pi
Copy link
Collaborator

bc-pi commented Nov 28, 2023

Sorry for the ambiguity, I was including "check that the bound public key matches cnf" within "validate the key binding".

The small point I was trying make there is that typically the "validation of the base SD-JWT properties" needs to happen first because the bound public key needed for the "validate the key binding" comes out of the former process. So the order of "first validates the key binding, then calls the first algorithm" probably needs to be reversed.

The meta-point is that such a suggestion to "split the current verification algorithm" may not resonate well with those who already question its necessity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants