ssi-sd-jwt implementation #529
Conversation
|
|
tristanmiller-spruceid
changed the title
tristanmiller-spruceid
requested review from
justAnIdentity and
JacobHealy-Spruce
ssi-sd-jwt/src/lib.rs
Outdated
| pub use verify::verify_sd_disclosures_array; | ||
|
|
||
| #[derive(thiserror::Error, Debug)] | ||
| pub enum Error { |
There was a problem hiding this comment.
ssi has a history of having massive bundles of errors, but I don't think it's necessarily nice. Could you split the errors between encoding and decoding?
There was a problem hiding this comment.
I can definitely do that. I trend towards that thinking generally; I just be default was matching ssi.
| Ok(Disclosure { encoded, hash }) | ||
| } | ||
|
|
||
| pub fn encode_sign<Claims: Serialize>( |
There was a problem hiding this comment.
I'm not familiar with the specs, could you explain why it's not generating the disclosures from the claims struct? Is it because each disclosure can have a different encoding? If so, why not still use the values in the object and pass the encoding in some other way? It's just a bit confusing to pass an object that's barely used (at least from what I see in the examples)
There was a problem hiding this comment.
The main issue is that the return value array of encoded disclosures were very difficult to match up to the various disclosures, since very complex sets of disclosures are possible including nesting. True, the object is not used much from the encode side except for the base jwt (so common claims like iss, exp, etc. that will always be disclosed), but in exchange you do get to keep the same claim struct on the decode and encode side.
ssi-sd-jwt/src/decode.rs
Outdated
| pub fn decode_verify<Claims: DeserializeOwned>( | ||
| jwt: &str, | ||
| key: &JWK, | ||
| disclosures: &[&str], |
There was a problem hiding this comment.
Do you think it could improve the API if this was using the DecodedDisclosure type? To force the user to "validate" the disclosures before doing anything, and also to prevent silly mistakes like passing the jwt as a disclosure
There was a problem hiding this comment.
Practically, the main decode pathway should be going through decode_verify_string_format AFAIUI the normal use cases. That format is sort of like how jwt/jws/jwe components are separated by '.', this nests that in a structure separated by tildes to also hold the list of disclosures. At that point, I was hoping that those using the 'decode_verify' not have to call other functions just to call decode_verify when it could give them those errors pretty much immediately anyway.
Perhaps to make this clearer, would you like to see 'decode_verify_string_format' be named 'decode_verify' and the current 'decode_verify' be named something like 'decode_verify_disclosure_array' to direct downstream library users to the easiest path?
tristanmiller-spruceid
changed the title
| ) -> Result<String, serde_json::Error> { | ||
| let disclosure = match claim_name { | ||
| Some(claim_name) => serde_json::json!([salt, claim_name, claim_value]), | ||
| None => serde_json::json!([salt, claim_value]), |
There was a problem hiding this comment.
I think the claim_name is mandatory for a disclosure right?
There was a problem hiding this comment.
No, array style disclosures don't require a name.
| let _ = base_claims_obj.insert(claim_name.clone(), serde_json::json!([])); | ||
| } | ||
|
|
||
| let array = base_claims_obj.get_mut(claim_name).unwrap(); |
There was a problem hiding this comment.
The unwrap is because claim_name is explicitly added in all cases in the couple lines above, but this absolutely needed a comment justifying this. That has been added.
|
Am I correct thinking this implementation takes the Flat SD-JWT approach for nested data? |
|
Do you plan integration with |
I imagined that SD-JWT + VC would be a separate pull request, but I can integrate that here if that's required. |
The highlevel API only supports Flat SD-JWT, but an example of using the lower level API to parse a more complex JWT that is both nested and contains recursive disclosures is given in https://github.com/tristanmiller-spruceid/ssi/blob/5bfe8af2d9ffabe3b7b68db79f5943ffd3c4fb10/ssi-sd-jwt/tests/rfc_examples.rs#L181 and to create and parse a nested JWT is here https://github.com/tristanmiller-spruceid/ssi/blob/5bfe8af2d9ffabe3b7b68db79f5943ffd3c4fb10/ssi-sd-jwt/tests/full_pathway.rs#L119 |
This new crate implements the sd-jwt specification as found here:
https://datatracker.ietf.org/doc/draft-ietf-oauth-selective-disclosure-jwt/
This is an RFC PR to provide a base for discussion, particularly around the shape of the public API.
TODO