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

Refactor #508

Open
wants to merge 105 commits into
base: main
Choose a base branch
from
Open

Refactor #508

wants to merge 105 commits into from

Conversation

timothee-haudebourg
Copy link
Contributor

@timothee-haudebourg timothee-haudebourg commented Apr 13, 2023

This PR aims at refactoring ssi to make better use of Rust traits.

  • Abstract the credential/presentation types (using traits) to accept user defined types.
  • Unify verifiable claim types (JWT, W3C VC). Have a uniform Verifable<Claims> type family for verifiable claims, independent of the underlying implementation.
  • TreeLDR integration (although it was one of the main motivation for this refactor, integration with TreeLDR will be de facto possible using the new abstractions. ssi will not depend on TreeLDR).

This is a work in early progress.

Status

Most of the work is completed. There are still some DID methods that needs to be reintroduced. Most of the remaining work is testing everything (and that's a lot).

Below are all the parts that needs to be added or changed in ssi:

  • Generate VC datamodel with TreeLDR
  • JWT VC (new dedicated ssi-vc-jwt library)
    • JWT+LD decoding
    • verification
    • JWT+LD signing
    • encoding
  • Add an independent library for Verification Methods (ssi-verification-methods library), with VMs clearly defined.
    • API design
    • Verification methods implementation
      • EcdsaSecp256k1RecoveryMethod2020
      • EcdsaSecp256k1VerificationKey2019
      • EcdsaSecp256r1VerificationKey2019
      • Ed25519VerificationKey2018
      • Ed25519VerificationKey2020
      • JsonWebKey2020
      • Multikey
      • RsaVerificationKey2018
      • Ed25519PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021
      • P256PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021
      • TezosMethod2021
      • AleoMethod2021
      • BlockchainVerificationMethod2021
      • Eip712Method2021
  • DIDs (ssi-dids library)
  • Data Integrity (in the ssi-vc-ldp library)
    • VC decoding from JSON-LD
    • VC encoding
    • CryptographicSuite trait design
    • Cryptosuites implementations
      • EcdsaSecp256k1Signature2019
      • EcdsaSecp256r1Signature2019
      • Ed25519Signature2018
      • Ed25519Signature2020
      • eddsa-2022
      • EthereumEip712Signature2021
      • JsonWebSignature2020
      • RsaSignature2018
      • EcdsaSecp256k1RecoverySignature2020
      • AleoSignature2021
      • Eip712Signature2021
      • EthereumPersonalSignature2021
      • SolanaSignature2021
    • Any VC API
  • Testing

ssi-crypto/src/verification.rs Outdated Show resolved Hide resolved
ssi-crypto/src/verification.rs Outdated Show resolved Hide resolved
ssi-crypto/src/verification.rs Outdated Show resolved Hide resolved
ssi-dids/src/did.rs Outdated Show resolved Hide resolved
///
/// See: <https://www.w3.org/TR/did-core/#did-controller>
#[serde(skip_serializing_if = "Option::is_none")]
pub controller: Option<OneOrMany<DIDBuf>>,
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to move to a Vec and deserialise as https://docs.rs/serde_with/latest/serde_with/struct.OneOrMany.html. Using OneOrMany directly is a pain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that's a good idea. Also, when None is semantically equivalent to Some(empty_collection), I think we should remove the Option wrapper. It doesn't add anything.

ssi-dids/src/resolution/http.rs Outdated Show resolved Hide resolved
ssi-jws/src/lib.rs Outdated Show resolved Hide resolved
ssi-vc-ldp/src/suite.rs Outdated Show resolved Hide resolved
@timothee-haudebourg
Copy link
Contributor Author

This is finally happening! Everything compiles, every test, doc test, example can run fine, no warnings, no broken doc links. I've fixed, tested and documented features. Now it is not perfect, there are a lot of ugly things living in ssi_jwk, ssi_jws for instance, but I'll consider it out of scope. Other issues are directly related to the refactor:

  • Before, the various verification functions would return a VerificationResult containing warnings and errors. There was no distinction between verification failure (the signature/proof is not valid), and verification errors (we could not verify the signature for some reason, it may or may not be valid). I tried to do the distinction using a more fine grain result type Result<ProofValidity, VerificationError> where ProofValidity captures the validity of the proof (it may be invalid) and VerificationError for when the verification process failed. I think its a good idea overall but
    • I haven't done it right everywhere. Some errors that should raise ProofValidity::Invalid instead throw a VerificationError
    • Ideally ProofValidity::Invalid would also gives the reason why the verification failed, but for now there is none
    • Warnings are lost, but we could add them to the ProofValidity::Valid variant
  • I've added a Validate trait to validate claims independently of the proof/signature. It can be used to check the expiration date of a credential for instance. I think this trait could be improved so we can pass an "environment" to the validation function. This is required for some advanced validation checks, such as validating the aud (audience) claim of a JWT against the application domain. To do it correctly the validation function needs to know the application domain, which could be passed through this "environment".

These are points we can discuss but I think overall the refactor is ready to be reviewed.

@timothee-haudebourg timothee-haudebourg marked this pull request as ready for review February 23, 2024 17:09
@itsbalamurali
Copy link

@sbihel any ETA on merging this?

Most libraries are disabled for now.
`ssi-vc` now dedicated to VC Data Model.
`ssi-vc` is mostly generated by TreeLDR.
`ssi-ldr` now dedicated to Data Integrity.
The dependency relation in between is reversed.
Most of the code in both has been removed for now.
Remove `SignerProvider` trait.
Remove `VerifierProvider` trait.
Remove useless `Algorithm` type.
Add `async` marker to `Verifier::verify` function.
Add safe constructor to `CompactJWS`.
Add some handy methods to the `Header` type.
Add a `sec.ttl` schema for the `sec:multibase` datatype.
Align the verification method types.
Impl `Verifier` for `DIDResolver`.
Impl `ControllerProvider` for `DIDResolver`.
Still need to use `ssi-verification-methods`.
Added `RsaSignature2018` & `RsaVerificationKey2018`
Added `JsonWebSignature2020` & `JsonWebKey2020`
Added `Multikey`
Added `EcdsaSecp256k1Signature2019` & `EcdsaSecp256k1VerificationKey2019`
Added `EcdsaSecp256r1Signature2019` & `EcdsaSecp256r1VerificationKey2019`
@sbihel
Copy link
Member

sbihel commented Apr 17, 2024

For context I'm in the process of rewriting the VC API in DIDKit spruceid/didkit#384, and I'm currently stuck with lifetime bound issues, but if you could have a look to see if I'm doing anything wrong that would be great.

@ianhamilton87
Copy link

@timothee-haudebourg @clehner @sbihel - any ETA on the completion of this PR?
Reason I ask is because merging this PR is blocking one of mine: #545

Thanks.

@sbihel
Copy link
Member

sbihel commented May 9, 2024

There's is no ETA but we're hoping to get it merged and published within two months.

Validation returns `Result`.
Add validation environment.
Rework jwt claims, impl `Validation` trait for claim sets.
Remove type parameters in error types.
Use the standard `SignatureError` type for all signature functions.
Move the signature protocol API out of `ssi-crypto` and into `ssi-verification-methods` where it belongs.
More doc.
@timothee-haudebourg
Copy link
Contributor Author

timothee-haudebourg commented May 10, 2024

I've made some changes to follow the reviews. With a few days away from ssi I had some fresh eyes, so I've fixed some other issues that were necessary in my opinion:

  • Normalized all the claim signature/verification error types. There is now one SignatureError and one ProofValidationError used across all the sign/verify functions, whatever the claim type (Data-Integrity, JWS, JWT, etc.).
  • Provided an environment to the claim validation method Validate::validate. This can be used to deterministically set the time of verification for instance, used to check the expiration/issuance date claims. The function now returns a result, giving an insight on why the validation failed (if the claim expired for instance).
  • Normalized the verification/signature API across claim types. Now besides Data-Integrity claims, the same verification pipeline is (can be) used to verify JWS, JWT, etc. I've added some example in the crate-level documentation of ssi-jws and ssi-jwt. I've also added documentation of this "verification pipeline" here: crates/claims/core/src/verification/mod.rs (maybe not the best place since this won't appear in the generated doc).
  • Changed the way we encode JWT registered claims. It is now very similar to cose-rs, which we will probably use in the future to verify CBOR-encoded claims. It is also more memory efficient.
  • Completely separated JWS/JWT from the ssi-verification-methods crate, just so it stays reasonable to use the ssi-jws/ss-jwt crates alone, without the rest of ssi.

Add `NoClaim` empty type.
Add documentation on `Claim` trait.
Removes the need for dedicated types for each type of claims, like `JsonVerifiableCredential` which can become `DataIntegrity<JsonCredential>`.
@sbihel
Copy link
Member

sbihel commented May 23, 2024

There are still some error types that are missing Send and would probably benefit from moving to a String or anyhow::Error -- at least MessageSignatureError . Have you tried compiling the VC API in DIDKit in spruceid/didkit#384?

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.

None yet

5 participants