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.

Sign up for GitHub

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

Add key Signer trait in place of bearer_did+key_selector #208 #213

Closed
wants to merge 2 commits into from
Closed

Add key Signer trait in place of bearer_did+key_selector #208 #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c0e3a2
Add key Signer trait in place of bearer_did+key_selector #208
KendallWeihe May 16, 2024
d0447db
Undo temporary bindings code
KendallWeihe May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/credentials/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ license-file.workspace = true
base64 = { workspace = true }
chrono = { workspace = true }
keys = { path = "../keys" }
dids = { path = "../dids" }
hex = "0.4"
jsonpath-rust = "0.5.1"
jsonschema = "0.17.1"
Expand All @@ -25,6 +24,7 @@ uuid = { version = "1.8.0", features = ["v4"] }

[dev-dependencies]
crypto = { path = "../crypto" }
dids = { path = "../dids" }
serde_canonical_json = "1.0.0"
tokio = { version = "1.34.0", features = ["macros", "test-util"] }
test-helpers = { path = "../test-helpers" }
22 changes: 15 additions & 7 deletions crates/credentials/src/vc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use core::fmt;
use dids::{bearer::BearerDid, document::KeySelector};
use jws::JwsError;
use jws::{JwsError, JwsHeader};
use jwt::{
jws::Jwt,
{Claims, JwtError, RegisteredClaims},
};
use keys::key_manager::Signer;
use serde::{Deserialize, Serialize};
use std::{
collections::HashMap,
Expand Down Expand Up @@ -107,8 +107,8 @@ impl VerifiableCredential {

pub fn sign(
&self,
bearer_did: &BearerDid,
key_selector: &KeySelector,
signer: Arc<dyn Signer>,
jws_header: JwsHeader,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love this byproduct of this design, wherein we have to pass the jws_header to the VerifiableCredential's sign() method, and the same is true for the Jwt's sign() method. Previously, we were able to construct the default JwsHeader from the provided bearer_did+key_selector but since we don't have those anymore, we have to push that concept up the stack. For this reason, you'll see a new method I created in the jws crate, JwsHeader::from_did_document() which constructs the default JwsHeader for the given DID Doc + key selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please weigh in if you have alternative ideas. We're prioritizing modularity here over convenience, which is one of the guiding principals, but oof this one kind of hurts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has triggered a requirement I hadn't previously considered, I don't think our crate design should require the existence of peer dependencies. Moving this PR to a draft stage while I think this through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #215 and leaving this code snippet here for now, because this'll probably be what we end up doing here

pub use keys::{Signer, KeyManagerError};

pub trait CredentialSigner {
    fn sign(&self, payload: &[u8]) -> Result<Vec<u8>, CredentialError>;
}

impl<T> CredentialSigner for T
where
    T: Signer,
{
    fn sign(&self, payload: &[u8]) -> Result<Vec<u8>, CredentialError> {
        T::sign(self, payload).map_err(|e| CredentialError::from(e))
    }
}

The idea being: both re-export the underlying keys crate in the case the developer wants to use an implementation of the Signer from the keys crate, and also create a wrapper trait CredentialSigner which is compatible with keys::Signer but also enables developers to bring-their-own-signer.

Copy link
Member

Choose a reason for hiding this comment

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

hmm could you show a sample usage here? I'm not clear on why header parameters need to be passed to a generic credential sign - if we are following v1.1. then the decisions have been made for us...and v2 to a lesser degree

maybe the same is not true for other formats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, if you see the code here

impl JwsHeader {
    pub fn from_did_document(
        document: &Document,
        key_selector: &KeySelector,
    ) -> Result<Self, JwsError> {
        let verification_method = document.get_verification_method(key_selector)?;

        Ok(Self {
            alg: verification_method.public_key_jwk.alg.clone(),
            kid: verification_method.id.clone(),
            typ: "JWT".to_string(),
        })
    }
}

We're creating a VC-JWT with those three JOSE Header's set, two of which originate from the DID Document. Looking at the spec I see the language:

specific JWS-registered header parameter names

@decentralgabe what're your thoughts on what's needed in the VC-JWT JOSE Header's?

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, I would imagine we construct a signer that takes those parameters as inputs
since any JWT signer with a DID would have the iss and kid fields set as you described

) -> Result<String, CredentialError> {
let claims = VcJwtClaims {
registered_claims: RegisteredClaims {
Expand All @@ -122,7 +122,7 @@ impl VerifiableCredential {
vc: self.clone(),
};

let jwt = Jwt::sign(bearer_did, key_selector, None, &claims)?;
let jwt = Jwt::sign(signer, jws_header, &claims)?;

Ok(jwt)
}
Expand Down Expand Up @@ -162,7 +162,8 @@ mod test {
use super::*;
use crypto::Curve;
use dids::{
document::VerificationMethodType,
bearer::BearerDid,
document::{KeyIdFragment, KeySelector, VerificationMethodType},
methods::{
jwk::{DidJwk, DidJwkCreateOptions},
Create,
Expand Down Expand Up @@ -313,7 +314,14 @@ mod test {
let key_selector = KeySelector::MethodType {
verification_method_type: VerificationMethodType::VerificationMethod,
};
let vcjwt = vc.sign(&bearer_did, &key_selector).unwrap();

let key_id = bearer_did.document.verification_method[0].id.clone();
let key_alias = KeyIdFragment(key_id.clone()).splice_key_alias();
let signer = bearer_did.key_manager.get_signer(&key_alias).unwrap();

let jws_header = JwsHeader::from_did_document(&bearer_did.document, &key_selector).unwrap();

let vcjwt = vc.sign(signer, jws_header).unwrap();
assert!(!vcjwt.is_empty());

let verified_vc = VerifiableCredential::verify(&vcjwt).await.unwrap();
Expand Down
47 changes: 5 additions & 42 deletions crates/dids/src/bearer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{
document::{Document, DocumentError, KeyIdFragment, KeySelector},
document::{Document, DocumentError},
identifier::{Identifier, IdentifierError},
resolver::{ResolutionError, Resolver},
};
Expand Down Expand Up @@ -49,31 +49,17 @@ impl BearerDid {
.ok_or(ResolutionError::NotFound)?,
})
}

pub fn sign(
&self,
key_selector: &KeySelector,
payload: &[u8],
) -> Result<Vec<u8>, BearerDidError> {
let verification_method = self.document.get_verification_method(key_selector)?;
let key_alias = KeyIdFragment(verification_method.id.clone()).splice_key_alias();
let signature = self.key_manager.sign(&key_alias, payload)?;
Ok(signature)
}
}

#[cfg(test)]
mod test {
use super::*;
use crate::{
document::VerificationMethodType,
methods::{
jwk::{DidJwk, DidJwkCreateOptions},
Create,
},
use crate::methods::{
jwk::{DidJwk, DidJwkCreateOptions},
Create,
};
use crypto::Curve;
use keys::{key::PublicKey, key_manager::local_key_manager::LocalKeyManager};
use keys::key_manager::local_key_manager::LocalKeyManager;

#[tokio::test]
async fn test_from_key_manager() {
Expand All @@ -97,27 +83,4 @@ mod test {
bearer_did_private_keys[0].jwk().unwrap().d
);
}

#[test]
fn test_sign() {
let key_manager = Arc::new(LocalKeyManager::new());
let options = DidJwkCreateOptions {
curve: Curve::Ed25519,
};
let bearer_did = DidJwk::create(key_manager.clone(), options).unwrap();

let payload = b"hello world";
let key_selector = KeySelector::MethodType {
verification_method_type: VerificationMethodType::VerificationMethod,
};
let signature = bearer_did.sign(&key_selector, payload).unwrap();

assert_ne!(0, signature.len());

let vm = bearer_did
.document
.get_verification_method(&key_selector)
.unwrap();
vm.public_key_jwk.verify(payload, &signature).unwrap();
}
}
1 change: 1 addition & 0 deletions crates/jws/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ license-file.workspace = true
base64 = { workspace = true }
crypto = { path = "../crypto" }
dids = { path = "../dids" }
keys = { path = "../keys" }
serde = { workspace = true }
serde_json = { workspace = true }
thiserror = { workspace = true }
Expand Down
Loading
Loading