crypto,all: allow to choose which crypto key algo are allowed#46
crypto,all: allow to choose which crypto key algo are allowed#46MichaelMure wants to merge 1 commit into
Conversation
662854b to
345e151
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors public-key decoding to be policy-driven via a pluggable crypto.KeySet, allowing callers to control which key algorithms (and for RSA, modulus sizes) are permitted during DID resolution. It replaces the previous “decode everything” approach (crypto/_allkeys) with opt-in registration (including a crypto/all convenience package) and wires the policy into did:key and did:plc resolution paths.
Changes:
- Introduces
crypto.KeySet/crypto.KeyType, an initially-emptycrypto.DefaultKeySet, and acrypto/allside-effect import to register all supported algorithms. - Adds
did.WithKeySet(...)and updatesdid:key+did:plcresolution to decode public keys through the selected key set (movingdid:keyvalidation from parse-time toDocument()). - Switches RSA
publicKeyMultibaseencoding/decoding to PKCS#1 RSAPublicKey DER (from prior PKIX/X.509 encoding).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| verifiers/did-web/web.go | Documents that did:web does not apply WithKeySet during resolution. |
| verifiers/did-plc/plc.go | Decodes did:plc embedded did:key public keys via the resolution KeySet. |
| verifiers/did-plc/plc_test.go | Updates tests to opt into all algorithms via crypto/all. |
| verifiers/did-plc/document_test.go | Adds coverage verifying WithKeySet enforcement during did:plc resolution. |
| verifiers/did-key/key.go | Defers key decoding to Document() using the resolution KeySet; parse no longer decodes/validates keys. |
| verifiers/did-key/key_test.go | Updates tests for new did:key behavior and adds WithKeySet coverage. |
| verifiers/_methods/multikey/multikey.go | Switches Multikey JSON decoding to use crypto.PublicKeyFromMultibase (DefaultKeySet-governed). |
| options.go | Adds ResolutionOpts.keySet, accessor KeySet(), and did.WithKeySet(...). |
| document/document_test.go | Ensures tests register algorithms via crypto/all. |
| did_test.go | Adds WithKeySet tests and registers algorithms via crypto/all. |
| crypto/x25519/key.go | Adds x25519.KeyType() for KeySet registration. |
| crypto/secp256k1/key.go | Adds secp256k1.KeyType() for KeySet registration. |
| crypto/rsa/public.go | Changes RSA multibase decode/encode to PKCS#1 RSAPublicKey DER. |
| crypto/rsa/key.go | Adds rsa.KeyType(sizes...) to encode RSA size policy into a KeySet. |
| crypto/p521/key.go | Adds p521.KeyType() for KeySet registration. |
| crypto/p384/key.go | Adds p384.KeyType() for KeySet registration. |
| crypto/p256/key.go | Adds p256.KeyType() for KeySet registration. |
| crypto/keyset.go | Introduces KeySet, KeyType, DefaultKeySet, and helper decode APIs. |
| crypto/keyset_test.go | Adds tests for algorithm restriction and RSA size policy enforcement. |
| crypto/ed25519/key.go | Adds ed25519.KeyType() for KeySet registration. |
| crypto/all/all.go | Adds a kitchen-sink side-effect import to register all algorithms into DefaultKeySet. |
| crypto/_allkeys/allkeys.go | Removes the previous monolithic “all algorithms” decoder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // WithKeySet provides a KeySet, a set of cryptographic key algorithm as | ||
| // allowed during resolution. If the DID to resolve has an algorithm not included | ||
| // in this set, resolution will fail. | ||
| // This is a way for the caller to control what algorithm are deemed safe or expected. |
| // The decode functions return a nil PublicKey/PrivateKey together with an error on failure; a nil | ||
| // function means that form is not supported for this algorithm (for example RSA has no raw private | ||
| // bytes form). |
| return "web" | ||
| } | ||
|
|
||
| // Does not support WithKeySet. |
345e151 to
b6ba90a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b6ba90a. Configure here.
| ) | ||
| require.Error(t, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Tests omit key registration
Medium Severity
Resolution now decodes keys in Document() via DefaultKeySet, which starts empty unless algorithms are registered. These in-package tests call Document() without WithKeySet or a blank crypto/all import, so they fail where they previously passed because parsing used to decode keys eagerly.
Reviewed by Cursor Bugbot for commit b6ba90a. Configure here.
| } | ||
|
|
||
| m.pubkey, err = allkeys.PublicKeyFromPublicKeyMultibase(aux.PublicKeyMultibase) | ||
| m.pubkey, err = crypto.PublicKeyFromMultibase(aux.PublicKeyMultibase) |
There was a problem hiding this comment.
Multikey ignores KeySet policy
Medium Severity
Multikey JSON unmarshaling still calls crypto.PublicKeyFromMultibase, which always uses DefaultKeySet. WithKeySet on DID resolution only affects paths like did:key and did:plc, so a restricted resolution policy can be bypassed when verification methods are loaded from JSON via Multikey.
Reviewed by Cursor Bugbot for commit b6ba90a. Configure here.


Note
High Risk
Changes cryptographic acceptance policy and default behavior (empty DefaultKeySet), alters when did:key keys are validated, and fixes RSA multibase wire format—callers may break or see interoperability shifts until they register algorithms or pass WithKeySet.
Overview
Replaces the monolithic
crypto/_allkeysdecoder with aKeySet/KeyTyperegistry so callers choose which algorithms (and RSA modulus sizes) are accepted. Eachcrypto/<algo>package exposesKeyType();crypto/allblank-import registers everything intoDefaultKeySet, which starts empty until youRegisteror importall.DID resolution gains
did.WithKeySet(...):did:keyanddid:plcdecode embeddedpublicKeyMultibaseonly through the selected set, so mismatched policies fail resolution.did:keyparse no longer validates the key untilDocument(). Multikey unmarshaling usescrypto.PublicKeyFromMultibase(default set).RSA
did:keymultibase encoding/decoding switches from PKIX DER to PKCS#1 RSAPublicKey DER, aligned with the spec;KeyTypecan restrict allowed RSA bit lengths.Tests add
_ "crypto/all"where full algorithm support is needed and cover KeySet restriction fordid:key,did:plc, and package-levelcryptobehavior.Reviewed by Cursor Bugbot for commit b6ba90a. Bugbot is set up for automated code reviews on this repo. Configure here.