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

[KendallWeihe]

Closes #208

Started to write doc comments but realized this was already a lot, so added a comment to the existing ticket for doc comments #103 (comment)

[KendallWeihe]

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.

[KendallWeihe]

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.

[KendallWeihe]

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.

[KendallWeihe]

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.

[decentralgabe]

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

[KendallWeihe]

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?

[decentralgabe]

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

[KendallWeihe]

There's also a slightly strange implication with this design change, in that, the verify() methods still assume the JWS Header was signed using a DID. Within the verify() methods we resolve the DID Document and then make semantic verifications (#182). So by introducing the change in this PR we're enabling the signing of JWS's using an agnostic key signer (AKA not necessarily associated to the DID) but then when we go to verify we assume the JWS was signed with a DID.

It seems to me if we want to move forward with the proposed changes here then we should also remove the undering DID assumption in the call to verify(). And then that raises the question, should we still enable that feature, and if so, where?