-
Notifications
You must be signed in to change notification settings - Fork 90
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
Support Selective Disclosure SD-JWT #1268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too convinced with how the validator has changed. Having to explicitly pass the optional SdObjectDecoder
doesn't feel really confortable. Don't you think it might be worth having SdObjectDecoder
work behind the scene? The APIs for JwtCredentialValidator
remains the same, but internally use SdObjectDecoder
if the JWT received as input is a SD-JWT. If a user knows for sure that they are working with a SD-JWT then they can use the SdObjectDecoder
directly, maybe through some utility functions or even an extension trait provided by the library to make things more ergonomic.
Probably we can discuss a better way to handle this. What concerns me is that this won't scale well if we want to handle the validation of even more JWT types (say ZK-JWT for instance), as it could require to add yet another optional parameter to every method call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now you have created an SdJwtValidator
that only validates SD-JWT encoded credential. Even more of a reason to avoid polluting the APIs of JwtCredentialValidator
if the two have different concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library's API didn't really change, since none of the public methods changed. validate_extended
is not public.
I agree with you that this does not scale well. But If we want to separate the concerns, we pretty much have to duplicate the majority of the JwtCredentialValidator
.
Don't you think it might be worth having SdObjectDecoder work behind the scene?
It doesn't work, it's tightly coupled with the signature. You can not reuse JwtCredentialValidator
if you do the decoding yourself. You have to verify the signature first, which is a deep down part of the JwtCredentialValidator
.
So all in all, we either have to duplicate JwtCredentialValidator
, or we have to rewrite it completely to make it more generic (not solved problem), or we keep the current solution and make it a future problem :D, But for ZK, I don't think it would use any of the JwtCredentialValidator
. I don't think it would have a big overlap for that to make sense.
My concern is only that the sd_jwt_payload
is not an optional dependency if users only want JwtCredentialValidator
. But I kept all selective disclosure concerns under a flag, in case we want to rewrite or rethink the architecture of the internal implementation of the JwtCredentialValidator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulmth the last thing you mentioned is definitely something worth discussing. I couldn't have thought about it, thanks for pointing it out.
identity_credential/src/validator/sd_jwt/kb_validation_options.rs
Outdated
Show resolved
Hide resolved
We might want to further split some validation steps into smaller reusable and composable functions which probably better belong to |
@@ -704,7 +704,39 @@ impl WasmIotaDocument { | |||
/// | |||
/// Upon success a string representing a JWS encoded according to the Compact JWS Serialization format is returned. | |||
/// See [RFC7515 section 3.1](https://www.rfc-editor.org/rfc/rfc7515#section-3.1). | |||
/// | |||
/// @deprecated Use `createJws()` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in the latest release. I added a new method and deprecated this one
We can do that in a separate PR, this one is getting a bit too big. I already adapted your PR here, we can close that one I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Great job with the example!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, I left a couple of minor suggestions. Please also add the new example to bindings/wasm/examples/src/tests
to run it in CI.
|
||
/** | ||
* Demonstrates how to create a selective disclosure verifiable credential and validate it | ||
* using the standard [Selective Disclosure for JWTs (SD-JWT)](https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-07.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* using the standard [Selective Disclosure for JWTs (SD-JWT)](https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-07.html). | |
* using the [Selective Disclosure for JWTs (SD-JWT)](https://www.ietf.org/archive/id/draft-ietf-oauth-selective-disclosure-jwt-07.html) specification. |
// disclosures. | ||
const strDisclosures = disclosures.map(disclosure => disclosure.toEncodedString()); | ||
|
||
let sdJwt = new SdJwt(jws.toString(), strDisclosures, undefined).presentation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we omit the third parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I kept it originally to hint that users can optionally add the KB. but it's demonstrated further down in the code so it's fine.
const kbJwt = await aliceDocument.createJws(aliceStorage, aliceFragment, bindingClaims.toString(), options); | ||
|
||
// Create the final SD-JWT. | ||
let finalSdJwt = new SdJwt(sdJwtReceived.jwt().toString(), toBeDisclosed, kbJwt.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe sdJwtWithKb
@@ -215,6 +218,26 @@ impl WasmCredential { | |||
pub fn set_proof(&mut self, proof: Option<WasmProof>) { | |||
self.0.set_proof(proof.map(|wasm_proof| wasm_proof.0)) | |||
} | |||
|
|||
/// Serializes the `Credential` as a JWT claims set | |||
/// in accordance with [VC Data Model v1.1](https://www.w3.org/TR/vc-data-model/#json-web-token). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know Data Model 2.0 will be a thing, should we somehow anticipate that already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't have it in the Data Model 2.0, it's in Securing Verifiable Credentials using JOSE and COSE.
I'm not sure what we can do here, it affects our JWT implementation for credentials everywhere, not only for SD JWT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't clear. I was going for do we have any dependencies on Data Model 1.1 that we should abstract out already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, it can deal with any JSON value, so that wouldn't be an issue AFAIK.
Would there ever be an additional Proof in the credential? Is that something that we need to demonstrate? |
It's possible to add a proof property to a credential, but how is that relevant to SD JWT? We use JWT to validate the signature here so we don't need a proof property since it's not relevant to SD JWT 🤷🏻 |
That makes sense, thank you! Then I think there is nothing for us to demonstrate or additionally consider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, awesone job 💪
Add Rust and Wasm support for verifying selective disclosures JWT and key binding JWT using https://github.com/iotaledger/sd-jwt-payload.