fix(vdr): reflect key store backend capability in DID document key usage - #4522
Merged
reinkrul merged 20 commits intoSep 16, 2026
Merged
Conversation
did:nuts always tagged a newly generated key's VerificationMethod with DefaultKeyFlags(), which includes KeyAgreementUsage, regardless of whether the underlying key store backend can actually use that key for decryption. Azure Key Vault EC keys can only sign; Azure doesn't support decryption/ECDH with them. A DID document could therefore advertise KeyAgreement for a key nobody can actually decrypt with, breaking gRPC network private-transaction delivery for the sender resolving the recipient's KeyAgreement key. crypto.KeyCreator.New() (and the storage.spi.Storage.NewPrivateKey() backend call it wraps) now also returns the DIDKeyFlags the generated key can actually be used for. fs, vault and the external adapter all report every usage (they hand back plain, exportable EC keys); Azure Key Vault reports AssertionKeyUsage only. did:nuts and did:web now intersect the requested key usage with what the backend actually reports before persisting a VerificationMethod's key usage, so a verification relationship is only added to a DID document when the key backing it actually supports it. Assisted by AI
…kend layer The first version of this fix had crypto/storage/spi.Storage.NewPrivateKey() (and its fs/vault/azure/external implementations) return orm.DIDKeyFlags directly, so a raw key-storage backend had to speak DID Core vocabulary (AssertionMethod, KeyAgreement) that has nothing to do with storing a key. Storage.NewPrivateKey() now reports a crypto-native spi.KeyCapability (SigningOnly or SigningAndDecryption) instead, with no storage/orm dependency. crypto.Crypto.New() is the translation boundary: it already persists orm.KeyReference, so it now also translates the reported capability into orm.DIDKeyFlags and persists it as KeyReference.KeyUsage (migration 012), rather than returning it as a separate value that would just duplicate what's already on the returned KeyReference. did:nuts and did:web read requestedFlags & keyRef.KeyUsage to decide what a VerificationMethod actually gets to claim. crypto.Migrate() corrects existing KeyReferences to sign-only for nodes configured with the Azure Key Vault backend: switching crypto storage backends for an existing node isn't supported (KeyName/Version are backend-specific and become unreachable), so every managed key under an Azure-configured node is known to have been created by Azure Key Vault. Assisted by AI
KeyCapability had two named states, SigningOnly and SigningAndDecryption, that read like a bitmask (the combined-word name) without being one - membership was checked with ==, not bitwise. Give it real bits, Signing and Decryption, combined the same way orm.DIDKeyFlags already is (Signing | Decryption), with a matching Is() helper. Every generated key sets Signing; only decryption-capable keys also set Decryption, so a key that can't sign at all (e.g. a future AES-like backend) remains representable without it. Assisted by AI
The migration previously defaulted key_reference.key_usage to 31
("everything") and crypto.Migrate() only corrected it down to sign-only
for the Azure Key Vault backend specifically. That fails open: any future
backend that also can't decrypt, but isn't recognized by that Azure-only
check, would silently keep the "everything" default and offer a
KeyAgreement verification method it can't back.
Default to 0 ("not yet determined") instead, and have Migrate() set any
row still at 0 to the currently configured backend's real usage,
unconditionally rather than only for Azure. Existing rows never end up
assuming a capability that hasn't actually been confirmed.
Assisted by AI
A DEFAULT clause on key_usage would let the database silently manufacture
a value application code never actually chose - the exact kind of hidden
default (did:nuts's DefaultKeyFlags() blindly assuming KeyAgreement) that
caused this whole issue. crypto.Crypto.New() already always sets this
value explicitly for every key it creates, so the only place that ever
needs to fill one in is a one-time migration for rows that predate this
column.
Migration 012 is now a Go migration rather than a .sql file (matching
Migration011CredentialPropValueType) so it can add the column nullable,
backfill existing rows to 31 ("everything": what every key was assumed
to support before this column existed) in one UPDATE, then set NOT NULL
- something a single .sql statement can't do on a non-empty table. As
before, that "everything" assumption is wrong for a backend like Azure
Key Vault whose keys can't actually decrypt, so crypto.Migrate() still
corrects it for that backend afterwards. SQLite has no ALTER COLUMN
syntax, so it can't add the NOT NULL constraint after the fact; the
column stays nullable there, same carve-out Migration011 already uses.
Assisted by AI
…on, drop the lingering default Two fixes to migration 012: - It only branched on dbType == "postgres", missing that this node also supports mysql, sqlserver and azuresql as configured backends (see the dialect switch in storage/engine.go), same set Migration011 CredentialPropValueType already handles. Give it the same per-dialect statement map. - Simplify to two statements instead of three: ADD COLUMN ... NOT NULL needs a DEFAULT to backfill existing rows in the first place - there's no way around that in a single statement - so add it with DEFAULT 31, then drop the default immediately after so it doesn't linger for future inserts. Postgres and MySQL support dropping a column default directly; SQL Server ties a default to a separately named constraint, so adding the column now names that constraint explicitly so it can be dropped by name afterwards. SQLite has neither ALTER COLUMN nor DROP CONSTRAINT, so it keeps the default (harmless: crypto.Crypto.New() always writes a real value explicitly). Assisted by AI
…switch
Replace the map of {addColumn, dropDefault} structs (one empty field for
SQLite) with a function returning an ordered []string of statements per
database type, grouping the dialects that share identical statements
(postgres/mysql; sqlserver/azuresql) into single switch cases instead of
repeating the same SQL text under each dialect's key. Also names the
magic backfill value (31) as keyReferenceKeyUsage012AllUsage, spelled
out as the OR of the same bit values already documented on
Migration012KeyReferenceKeyUsage.
Assisted by AI
The migration backfilled existing key_reference rows to 31
("everything"), and crypto.Crypto.Migrate() corrected rows still at 31
to sign-only for the Azure Key Vault backend. But 31 is also a
perfectly valid, real usage value - a KeyReference already correctly
set to "everything" (e.g. created under a different backend before a
node was reconfigured to use Azure Key Vault) would be indistinguishable
from one that just hadn't been migrated yet, and Migrate() would wrongly
re-correct it every time it runs.
Backfill to 0 instead: no real key usage can ever be 0, since every key
supports at least AssertionKeyUsage, so it can never collide with an
already-correct value. Migrate() now corrects any row still at 0 to the
currently configured backend's real usage, unconditionally rather than
only for Azure Key Vault, since the check no longer needs to guess which
backend a "wrong" value came from.
Assisted by AI
0 doesn't need a name the way the previous 31 backfill value did (that one only made sense spelled out as the OR of specific bit values); it's self-evidently "unset". Inline it and drop the now-unused fmt import. Assisted by AI
Assisted by AI
… actually granted did:web's NewVerificationMethod always returned the caller's requested keyUsage unchanged, rather than what the generated key can actually back. It always generated an EC key via the shared crypto.KeyStore, so this made no practical difference for AssertionMethod-family flags (every backend can sign) - the pre-existing KeyAgreementUsage rejection in didsubject.SqlManager already short-circuits before this function is ever reached for that case - but it's now consistent with did:nuts, which does the same intersection against keyRef.KeyUsage. Also renames the local variable holding that value (allowedKeyUsage -> actualKeyFlags) in both did:nuts and did:web for consistency. Assisted by AI
DefaultKeyFlags() (AssertionKeyUsage|EncryptionKeyUsage) is every bit DIDKeyFlags has; ANDing it with keyRef.KeyUsage, itself always a subset of that same domain, can never remove anything, so it was a no-op. Also drops a comment left over from before NewVerificationMethod started intersecting requestedFlags with the backend's actual capability. Assisted by AI
keyUsageForCapability unconditionally granted AssertionKeyUsage regardless of whether the backend capability included Signing, breaking symmetry with the Decryption check right below it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji5QQpcavwWb54ygCddr1h
…ersisting it Nothing ever read key_reference.key_usage back from the DB after the KeyCreator.New() call that wrote it, so the migration, its 0-vs-real-value sentinel, and the Migrate() backfill logic were pure overhead. Replace all of it with a single check, before any key is created: crypto.Crypto.New() now takes the required DIDKeyFlags and compares them against what the configured backend can back (Azure Key Vault: signing only; every other backend: signing + decryption), refusing with ErrKeyUsageNotSupported if it can't fully back them. This also drops spi.KeyCapability from the backend interface entirely: what a backend can do turned out to be a static fact of which backend is configured, not something to discover per generated key. Since a successful New()/NewVerificationMethod() call now always grants exactly what was requested, the "achieved flags" return value that threaded through KeyCreator.New(), MethodManager.NewVerificationMethod() and AddVerificationMethod()'s intersection check is gone; those signatures are back to their pre-fix shape. Behavior change: a did:nuts document requires KeyAgreement on its single key, so did:nuts document creation now fails outright on Azure Key Vault instead of silently publishing a document without KeyAgreement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji5QQpcavwWb54ygCddr1h
MockMethodManager.NewVerificationMethod still had the pre-refactor 3-return signature; nothing currently uses it as a didsubject.MethodManager so it didn't fail the build, but it was drifted, broken generated code. Regenerated via mockgen. Also collapses a goose.WithGoMigrations() call back to one line now that it only takes a single argument again, and notes on Crypto.supportedKeyUsage() why it's deliberately not backend-adapter-specific today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji5QQpcavwWb54ygCddr1h
didnuts.Manager.NewDocument ignored its keyFlags argument and always requested DefaultKeyFlags() (including KeyAgreement), regardless of what the caller actually asked for. With OpenID4VCI as an alternative to gRPC/DAG private-transaction delivery, a did:nuts document no longer strictly needs KeyAgreement, so it shouldn't be forced on backends that can't back it (e.g. Azure Key Vault) when nobody asked for it. NewDocument now honors keyFlags like didweb.Manager.NewDocument already did: the default subject-creation request (AssertionKeyUsage only, unless EncryptionKeyCreationOption is given) now reaches did:nuts too, so default did:nuts document creation on Azure Key Vault succeeds. An explicit request for KeyAgreement against a backend that can't back it still fails loudly via ErrKeyUsageNotSupported. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji5QQpcavwWb54ygCddr1h
reinkrul
marked this pull request as ready for review
September 11, 2026 14:16
reinkrul
requested review from
Dirklectisch,
JorisHeadease,
gerardsn,
stevenvegt and
woutslakhorst
as code owners
September 11, 2026 14:16
stevenvegt
approved these changes
Sep 14, 2026
stevenvegt
left a comment
stevenvegt
left a comment
Member
There was a problem hiding this comment.
Code for prevention of newly created documents looks good.
Should we also add detection of existing incompatible keys, e.g. during startup?
Member
Author
As it's only 1 vendor we know of, I think we should just contact them to get it fixed. Because it might become more complex later; RSA keys supporting decryption, EC keys not. So it could become more complex than "this key store supports encryption", making the startup check complicated and expensive. |
…V1 API didnuts.Manager.NewDocument used to ignore its keyFlags argument and always request DefaultKeyFlags() (including KeyAgreement), so the V1 CreateDID endpoint got a KeyAgreement key without ever asking for one. Now that NewDocument honors keyFlags, V1 CreateDID silently stopped creating one, breaking private-transaction (gRPC/DAG) delivery, which needs it to encrypt the PAL header. V1's documented contract is that the request body is ignored and defaults (including keyAgreement = true) are always used, so CreateDID now explicitly requests EncryptionKeyCreationOption to restore that default. Assisted by AI
Contributor
1 new issue
|
Member
Author
Consideration: V1
|
…Agreement SqlManager.Create used one shared keyFlags value for every DID method configured for a subject, and aborted creating the whole subject whenever KeyAgreement was requested and did:web was among the methods (did:web has never supported it: an RSA/SOGIS constraint, independent of key store backend). Restoring V1 CreateDID's default KeyAgreement request (previous commit) meant this now hard-failed subject creation for any party with did:web enabled alongside did:nuts. Strip KeyAgreement from did:web's flags instead of aborting: did:nuts still gets a working key (or a loud ErrKeyUsageNotSupported if its key store backend, e.g. Azure Key Vault, can't back it), while did:web's document is created without one, silently. Also documents in the V1 OpenAPI spec that CreateDID's keyAgreement default can't be overridden and always fails on a backend that can't provide it, pointing callers at the V2 API instead. Assisted by AI
…as temporary docs/_static/vdr/v1.yaml: advise against mixing V1 and V2 for the same node's DIDs/subjects; callers hitting V1's Azure Key Vault limitation should switch to V2 entirely, not use both. vdr/didsubject/manager.go: mark the method == "web" check as a temporary hardcoded workaround, since did:web's KeyAgreement restriction is only a policy choice (RSA/SOGIS, #1948), not a technical one. If that's resolved, the whole check can be removed rather than replaced with a per-method capability query. Assisted by AI
Before did:nuts started honoring its keyFlags argument, every did:nuts document got a KeyAgreement key regardless of what any caller (V1 or V2) asked for. V2's CreateSubject only ever requested one when the caller explicitly set keys.encryptionKey: true, which had no visible effect until did:nuts started honoring it: callers who never set that flag (the documented default) now silently stop getting a working KeyAgreement key on new subjects and key rotations, breaking gRPC/DAG private-transaction delivery with no error at creation time. Restore the old default: an encryption key is now requested unless the caller explicitly sets keys.encryptionKey: false. did:web keeps silently not getting one either way (SqlManager.Create already strips it); a key store backend that can't back it (Azure Key Vault) still fails loudly, whether the request came from the new default or an explicit true. Assisted by AI
Member
Author
Verified behavior matrix (did:nuts KeyAgreement / did:web)Documenting the scenarios discussed on this PR, double-checked against the code as it now stands (through the V2-default fix).
Notes:
Assisted by AI |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4518.
Problem
did:nuts's and did:web's key/VerificationMethod creation always tagged a newly generated key with whatever
DIDKeyFlagswere requested, without checking whether the key store backend can actually back them. did:web already hard-rejectsKeyAgreementUsagefor an unrelated reason (RSA not implemented there). did:nuts is where it bit: a DID document could advertise KeyAgreement for a key that can't decrypt — e.g. an Azure Key Vault key, which can only sign — silently breaking gRPC private-transaction delivery for anyone sending to it (see #4515).Scope note: only affects newly generated keys/documents going forward. Doesn't retroactively fix did:nuts documents already published with a KeyAgreement entry that can't decrypt — already anchored in the DAG. We know of (almost certainly) one affected party in production, handled directly with them; no network-wide migration planned.
Fix
crypto.Crypto.New()takes the requiredorm.DIDKeyFlagsand checks it against what the configured backend can back — Azure Key Vault: signing only; every other backend: signing + decryption — before creating any key. If unsupported, no key is created andcrypto.ErrKeyUsageNotSupportedis returned (mapped to 400 in bothvdr/api/v1andv2).NewVerificationMethod/NewDocumentpass the caller's requested flags straight through as a hard requirement instead of silently granting less than what was asked.NewDocumentno longer forcesDefaultKeyFlags()regardless of what was requested (that's what caused the original bug). It now honorskeyFlagslike did:web already did: the default subject-creation request (AssertionKeyUsageonly, unlessEncryptionKeyCreationOptionis given) reaches did:nuts too, so a default did:nuts document creates fine on Azure Key Vault (just without KeyAgreement) — this is safe now that OpenID4VCI is an alternative to gRPC/DAG delivery. An explicit request for KeyAgreement against a backend that can't back it still fails loudly.No new persisted state, no migration: nothing needs to know a key's usage after the moment it's created.
Test plan
TestCrypto_New/required_usage_not_supported_by_backend— refuses before calling the backend.TestManager_NewDocument/key_store_backend_can't_back_KeyAgreement— default document creation succeeds without KeyAgreement; explicitDefaultKeyFlags()/EncryptionKeyUsagerequests fail withErrKeyUsageNotSupported.crypto,vdr,vdr/didnuts,vdr/didweb,vdr/didsubject,network,vcrsuites still pass.Assisted by AI