feat: Attested Signatures + UCAN Principal Clarification#30
Conversation
This makes it practical to use on its own, outside of `ValidateInvocation` (for validating a delegation, for instance).
No reason to have a registry to parse different verification method types when they're just bags of fields. It's the verifier that uses it that's interesting.
They weren't really being used outside of one pair of tests.
No need to filter them up front, just try them all. It's safe, and there's rarely more than one anyway.
- did/web: new did:web resolver - did/utilresolvers: ByMethod, Chain, Cached, WellKnown resolver combinators - verification: replace global init()-based registry with explicit Factory/Registry types; validator now takes verifierFactories via DefaultFactories() + WithVerifierFactory() - multikey: add NewIssuer, richer Signer/Verifier interfaces (Code, PublicKey, Raw, PrivateKey) - did: add New(), MustParse(), ValidateMethod(), UnsupportedMethodError
alanshaw
left a comment
There was a problem hiding this comment.
Not finished review - just submitting what I have :)
| client, _ := ed25519.Generate() | ||
| service, _ := ed25519.Generate() | ||
| client, _ := ed25519.GenerateIssuer() | ||
| service, _ := ed25519.GenerateIssuer() |
There was a problem hiding this comment.
Is there a Generate... function that does not create an issuer? I wonder if this should be Generate() and reduce churn to just a import change?
There was a problem hiding this comment.
Generate() generates a signer, since that's more fundamental. GenerateIssuer() is a convenience wrapper that makes a multikey.KeyIssuer() out of it.
| Material: did.GenericMap{did.MultikeyPublicKeyMultibaseProp: d.Identifier()}, | ||
| } | ||
|
|
||
| if err := doc.VerificationMethods.Add(vm); err != nil { |
There was a problem hiding this comment.
nit: we're Adding here to a pluralised VerificationMethods but the others below are singular.
There was a problem hiding this comment.
Well, not exactly. The others aren't nouns, they're verbs. "CapabilityDelegation" doesn't contain capability delegations, it contains verification methods which can be used for the delegation of capabilities. I have pluralized VerificationMethods where the actual JSON key is verificationMethod, because…it just felt really weird not to. So we're already messing with the names a bit here, and we could mess with them more, but pluralizing the others wouldn't make sense. WDYT? What would read best to you?
| // error if it cannot resolve the DID so the next tier can be tried. If all | ||
| // tiers fail, the error from each tier is aggregated and returned in a single | ||
| // error. | ||
| type Chain []did.Resolver |
There was a problem hiding this comment.
IMO "tiered" is a better description of what this is...
There was a problem hiding this comment.
Tier sounded like it carried a lot of semantic baggage to me; I was going for the Chain-of-responsibility pattern here. But I'm not against changing it back. Do you have other examples of "tier" being used this way? It seemed novel to me, but that might just be my own experience.
| ) | ||
|
|
||
| // WellKnown is a simple resolver that looks up DIDs in a local mapping. | ||
| type WellKnown map[did.DID]did.Document |
There was a problem hiding this comment.
👏 but this is a better name than "map" resolver
| } | ||
| } | ||
|
|
||
| func WithInsecure() Option { |
There was a problem hiding this comment.
FWIW, I find it easier to pass a bool here than have to conditionally add the option.
There was a problem hiding this comment.
Ooh, yes, that's much better.
There was a problem hiding this comment.
Ah, I think I must have done that because I remembered it from the otel code. That has a WithInsecure() option. But I agree, passing the explicit boolean (and being able to override it back with another option) is better.
|
|
||
| // Method returns the DID method name (e.g. "key", "web") parsed from the | ||
| // scheme. Returns "" for an undefined DID. | ||
| // scheme. Returns "" for an undefined |
There was a problem hiding this comment.
…I have no explanation. 😅 revert
There was a problem hiding this comment.
Can we just call this package resolver?
There was a problem hiding this comment.
Oh, yeah, that does sound nice. 👍🏻
| return f(ctx, d) | ||
| } | ||
|
|
||
| type ResolverMap map[string]Resolver |
There was a problem hiding this comment.
Is this a duplicate of the ByMethod resolver?
There was a problem hiding this comment.
Oh, dang, it is. That was supposed to go away. It's only used in tests. Lemme get that…
| return resolver.Resolve(ctx, d) | ||
| } | ||
|
|
||
| type MethodNotSupportedError struct { |
There was a problem hiding this comment.
Do you think we should put the code related to DID documents in a document package?
There was a problem hiding this comment.
I had one for a bit, but it was clunky. There were some dependency issues, and a document was a document.Document instead of a did.Document. Fundamentally, I realized that DID documents are pretty core to what DIDs are, so it made sense to put them in the same package. We could split them out if it's just too crowded and alias things for the outside world to use from the main did package, but I don't think it actually makes it any easier to work with.
This is part of a set of PRs across the
fil-forgerepos. They all work together using ago.work. I'm not sure how to make CI do anything useful, though.Attested Signatures + UCAN Principal Clarification
This PR (along with coordinated changes in sibling repos) reimplements attested signatures. This was written less thoroughly in sprue#15, but it needed to be more central for everything in the network to use it.
Pulling on that thread unraveled a whole bunch of latent issues with the domain model in UCAN. Nothing fundamentally wrong, just some things that were conflated that are now teased apart to reflect some subtle but important distinctions.
Attested Signatures
To recap:
did:mailto:DIDs have no native key material. Rather than signing directly, a mailto DID delegates to a trusted authority (the signing service), which issues a/ucan/attest/proofinvocation over a SHA2-256 hash of the message. That invocation is the signature. On the verify side, the signature is decoded as an invocation, the hash is checked against the message, and the invocation is validated against the authority.DID Documents as First-Class Objects
Previously, we "resolved" DIDs directly to verifiers. Now DID documents are a first-class concept in the
didpackage, rather than being reimplemented in multiple codebases.The previous code assumed that there was a one-to-one mapping between DIDs and signers/verifiers. That's a natural asssumption when most things are
did:key:s, where that's true. But in general, a DID document can have multipleverificationMethods, and that's what actually maps to a signer/verifier pair.Now we can resolve a DID to a document, and turn a verification method within it into a verifier. We can also make a "multi-verifier" (which succeeds if any of them succeed) out of all of the verification methods in the document, or out of just those with a particular verification relationship (notably
CapabilityInvocationandCapabilityDelegation).did.Documentis now a real typed struct with verification relationships. Resolvers return documents; verifier derivation is a separate step via a pluggable factory registry. This is what makes custom verification method types (likeAuthorityAttestation) possible.For
did:mailto:,didmailto.Resolvergenerates synthetic DID documents on-the-fly. Each document contains anAuthorityAttestationverification method naming the authority, and the registered factory for that type produces anAttestedVerifier.UCAN 1.0 Terminology
The refactor also tightens up the principal/signing model:
Principalremains "something with a DID()".SignerandVerifierare no longerPrincipals. They only deal in signatures. A verification method exists independently of a DID, and a signer or verifier exists independently for the same reason. But they're often tied together, so…Issueris now the noun for "aSignertied to aPrincipal". In many cases,Issuersimply replaces the existing use ofSigner. Notably, there are lot of variables already namedissuerwhich wereucan.Signerand are nowucan.Issuer, which gives me some confidence that this is correct.multikeyis now a specific family ofSigners andVerifiers which are based on cryptographic keys which the DID document represents asMultikey-type verification methods, specifically Ed25519 and secp256k1 currently. There's a set of enhancedmultikey.Issuer,multikey.Signer, andmultikey.Verifiertypes which know about the keys they represent.did:key:s are not keys and vice versa. Because of the conflation of signers/verifiers and identities, keys were often represented as the correspondingdid:key:s. But these are different things: a key is a method of signing and verification, while a DID is something that identifies a subject. The subject of adid:key:is, generally speaking, the entity which holds the private key—which is different from being the key itself! Now that's clearer.did:web:signer by creating adid:key:signer and wrapping it with adid:web:so that it reported that as its DID. Now they're separate concerns. The equivalent of "wrapping" ismultikey.NewIssuer(did.DID, multikey.Signer) multikey.Issuer. The equivalent of unwrapping isanIssuer.PrivateKey()—which returns an actual key, not a principal.identity.Identity. This was promoted fromsprueto solve the same problem in other modules.Identitysimply wraps anIssuerto provide aDIDDocument()factory, which can then be used both to serve the DID document on the web and to resolve one's own DID.Identityis intended to be used for "our" identity in any given service. The fact that it's such a simple wrapper seems like a smell to me, but it's useful and I didn't want to mess with it too much. It might want a little massaging in the future.Questions for Reviewers
attested, the verification needs a context, because it needs to recursively validate the invocation that is the attestation signature. ButVerify()doesn't take a context right now, so the verifier holds a context given at creation. That's a bit odd. ShouldVerify()change to take a context, or should we keep doing this?