fix(vcr): bound negative OpenID4VCI identifier caching, guard against empty offers - #4470
Open
reinkrul wants to merge 4 commits into
Open
fix(vcr): bound negative OpenID4VCI identifier caching, guard against empty offers#4470reinkrul wants to merge 4 commits into
reinkrul wants to merge 4 commits into
Conversation
…fier tlsIdentifierResolver cached a resolution result as soon as it succeeded without erroring, including an empty string when no base-URL service or TLS-derived candidate could be found. Once that happened, every later Resolve() call for that DID short-circuited on the cached empty value for the life of the process, even after the missing node-http-services-baseurl service was added, permanently breaking OpenID4VCI credential-offer delivery until restart. Only cache a non-empty identifier, and only treat a non-empty cached value as a hit, so resolution is retried on every call until it actually succeeds. Assisted by AI
14 tasks
Contributor
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (3)
🛟 Help
|
Address review feedback on the previous commit: caching nothing at all for an unresolved identifier would re-run resolution (including the DID document lookup) on every single call for a DID that never gets fixed, since this resolver can be invoked on every OpenID4VCI request. That negative caching was likely deliberate, not a bug. Reuse the existing lastAttempt/tlsAttemptInterval throttle (already used to rate-limit the expensive TLS-certificate-derived resolution) to also bound how long an empty result is treated as cached, instead of adding a separate cache window. A successful (non-empty) identifier is still cached indefinitely, unchanged. Also close the actual sending-side gap: nothing previously stopped an OpenID4VCI credential offer from being sent with an empty `credential_issuer`, which is what produced the receiver-side "empty Credential Issuer Identifier" rejection in the first place. Introduce openid4vci.ErrIdentifierNotConfigured, returned when resolution succeeds but yields no identifier, and have issueUsingOpenID4VCI treat it like an unsupported wallet (quiet fallback to the network, no error) instead of constructing and sending a broken offer. Assisted by AI
reinkrul
changed the title
Unlike an unsupported wallet (the other party's problem, no action for this operator), a missing node-http-services-baseurl service is this node's own misconfiguration and needs the operator's attention. Log a Warn pointing them at what to search the documentation for, rather than staying silent like the unsupported-wallet case. Assisted by AI
reinkrul
marked this pull request as ready for review
September 4, 2026 08:22
reinkrul
requested review from
Dirklectisch,
JorisHeadease,
gerardsn,
stevenvegt and
woutslakhorst
as code owners
September 4, 2026 08:22
3 tasks
reinkrul
added a commit
that referenced
this pull request
Sep 7, 2026
…nHammer mechanism - Lead with "server-to-server issuance" as the durable concept rather than OpenID4VCI, since the underlying protocol may be swapped out later; frame the gRPC/Nuts network as the (soon to be legacy) fallback we're moving away from, rather than centering the explanation on it. - List the concrete /n2n endpoints server-to-server issuance needs reachable, and note the whole /n2n prefix must be proxied through, not just these specific paths. - Correct the automatic-registration description: GoldenHammer does not use config.URL (that's v6+; GoldenHammer predates it, from v5.4). It actually probes each hostname in the node's own TLS certificate with a HEAD request to <hostname>/n2n/identity/<did>/.well-known/openid-credential-issuer, registering the first one that responds 200 with a JSON content type (verified against golden_hammer/module.go's tryResolveURL and vcr/openid4vci/identifiers.go's resolveFromCertificate/testIdentifier). - Dropped the closing line quoting #4470's warning message: that PR hasn't merged to master yet, so the message doesn't exist there. Assisted by AI
reinkrul
added a commit
that referenced
this pull request
Sep 7, 2026
…etup step (#4475) * docs(deployment): document node-http-services-baseurl as a required setup step Nothing currently documents this service at all, despite it being required for a did:nuts DID to be reachable over OpenID4VCI - it's only ever set automatically by GoldenHammer, a periodic best-effort auto-heal module that's scheduled for removal (#2318) and can't guarantee the service is present the moment it's actually needed. A network-wide audit (see #4469) found ~92% of DID documents missing it. Explains what it's for, how to register it via the DIDMan API for both a vendor DID (concrete URL) and a subject DID referencing a vendor's NutsComm service (matching reference, not a duplicate URL), and the log warning (#4470) that names an affected DID - which itself now points operators at this section by name. Verified with a clean sphinx build (0 new warnings; the 4 pre-existing ones are unrelated, in release_notes.rst/discovery.rst). Assisted by AI * docs(deployment): reframe as server-to-server issuance, correct GoldenHammer mechanism - Lead with "server-to-server issuance" as the durable concept rather than OpenID4VCI, since the underlying protocol may be swapped out later; frame the gRPC/Nuts network as the (soon to be legacy) fallback we're moving away from, rather than centering the explanation on it. - List the concrete /n2n endpoints server-to-server issuance needs reachable, and note the whole /n2n prefix must be proxied through, not just these specific paths. - Correct the automatic-registration description: GoldenHammer does not use config.URL (that's v6+; GoldenHammer predates it, from v5.4). It actually probes each hostname in the node's own TLS certificate with a HEAD request to <hostname>/n2n/identity/<did>/.well-known/openid-credential-issuer, registering the first one that responds 200 with a JSON content type (verified against golden_hammer/module.go's tryResolveURL and vcr/openid4vci/identifiers.go's resolveFromCertificate/testIdentifier). - Dropped the closing line quoting #4470's warning message: that PR hasn't merged to master yet, so the message doesn't exist there. Assisted by AI * docs(deployment): cross-reference node-http-services-baseurl from the /n2n endpoint description Point operators reading about the /n2n endpoint (Legacy Endpoints section) at the node-http-services-baseurl setup section, since server-to-server credential issuance for did:nuts DIDs runs over that same endpoint. Assisted by AI * docs(deployment): move node-http-services-baseurl section below Strict mode Moved out from under "Server options" (where it read as a sub-topic of the did:nuts/gRPC options table) to its own top-level section after Strict mode, matching Secrets/Strict mode's heading level. Assisted by AI * docs(deployment): address review feedback from #4475 - "service" -> "service endpoint"/"DID Document service endpoint" throughout, per review: "service" alone read like an actual service you need to run, not a DID Document term. - Reworded the automatic-registration paragraph per review: leads with what's added automatically (to ease configuration) and why it can fail, rather than "normally registered automatically... as long as...". - Corrected the gRPC/DAG fallback description. The reviewer's suggested "publishes an encrypted reference to a credential" isn't accurate either - what's actually encrypted is the transaction's recipient list (PAL), not the credential or a reference to it; the credential payload itself is never published to the network, it's fetched separately afterward over an authenticated connection between the specific nodes involved. Assisted by AI
reinkrul
added a commit
that referenced
this pull request
Sep 7, 2026
…etup step (#4475) (#4480) * docs(deployment): document node-http-services-baseurl as a required setup step Nothing currently documents this service at all, despite it being required for a did:nuts DID to be reachable over OpenID4VCI - it's only ever set automatically by GoldenHammer, a periodic best-effort auto-heal module that's scheduled for removal (#2318) and can't guarantee the service is present the moment it's actually needed. A network-wide audit (see #4469) found ~92% of DID documents missing it. Explains what it's for, how to register it via the DIDMan API for both a vendor DID (concrete URL) and a subject DID referencing a vendor's NutsComm service (matching reference, not a duplicate URL), and the log warning (#4470) that names an affected DID - which itself now points operators at this section by name. Verified with a clean sphinx build (0 new warnings; the 4 pre-existing ones are unrelated, in release_notes.rst/discovery.rst). Assisted by AI * docs(deployment): reframe as server-to-server issuance, correct GoldenHammer mechanism - Lead with "server-to-server issuance" as the durable concept rather than OpenID4VCI, since the underlying protocol may be swapped out later; frame the gRPC/Nuts network as the (soon to be legacy) fallback we're moving away from, rather than centering the explanation on it. - List the concrete /n2n endpoints server-to-server issuance needs reachable, and note the whole /n2n prefix must be proxied through, not just these specific paths. - Correct the automatic-registration description: GoldenHammer does not use config.URL (that's v6+; GoldenHammer predates it, from v5.4). It actually probes each hostname in the node's own TLS certificate with a HEAD request to <hostname>/n2n/identity/<did>/.well-known/openid-credential-issuer, registering the first one that responds 200 with a JSON content type (verified against golden_hammer/module.go's tryResolveURL and vcr/openid4vci/identifiers.go's resolveFromCertificate/testIdentifier). - Dropped the closing line quoting #4470's warning message: that PR hasn't merged to master yet, so the message doesn't exist there. Assisted by AI * docs(deployment): cross-reference node-http-services-baseurl from the /n2n endpoint description Point operators reading about the /n2n endpoint (Legacy Endpoints section) at the node-http-services-baseurl setup section, since server-to-server credential issuance for did:nuts DIDs runs over that same endpoint. Assisted by AI * docs(deployment): move node-http-services-baseurl section below Strict mode Moved out from under "Server options" (where it read as a sub-topic of the did:nuts/gRPC options table) to its own top-level section after Strict mode, matching Secrets/Strict mode's heading level. Assisted by AI * docs(deployment): address review feedback from #4475 - "service" -> "service endpoint"/"DID Document service endpoint" throughout, per review: "service" alone read like an actual service you need to run, not a DID Document term. - Reworded the automatic-registration paragraph per review: leads with what's added automatically (to ease configuration) and why it can fail, rather than "normally registered automatically... as long as...". - Corrected the gRPC/DAG fallback description. The reviewer's suggested "publishes an encrypted reference to a credential" isn't accurate either - what's actually encrypted is the transaction's recipient list (PAL), not the credential or a reference to it; the credential payload itself is never published to the network, it's fetched separately afterward over an authenticated connection between the specific nodes involved. Assisted by AI
reinkrul
added a commit
that referenced
this pull request
Sep 8, 2026
…etup step (#4475) (#4481) * docs(deployment): document node-http-services-baseurl as a required setup step Nothing currently documents this service at all, despite it being required for a did:nuts DID to be reachable over OpenID4VCI - it's only ever set automatically by GoldenHammer, a periodic best-effort auto-heal module that's scheduled for removal (#2318) and can't guarantee the service is present the moment it's actually needed. A network-wide audit (see #4469) found ~92% of DID documents missing it. Explains what it's for, how to register it via the DIDMan API for both a vendor DID (concrete URL) and a subject DID referencing a vendor's NutsComm service (matching reference, not a duplicate URL), and the log warning (#4470) that names an affected DID - which itself now points operators at this section by name. Verified with a clean sphinx build (0 new warnings; the 4 pre-existing ones are unrelated, in release_notes.rst/discovery.rst). Assisted by AI * docs(deployment): reframe as server-to-server issuance, correct GoldenHammer mechanism - Lead with "server-to-server issuance" as the durable concept rather than OpenID4VCI, since the underlying protocol may be swapped out later; frame the gRPC/Nuts network as the (soon to be legacy) fallback we're moving away from, rather than centering the explanation on it. - List the concrete /n2n endpoints server-to-server issuance needs reachable, and note the whole /n2n prefix must be proxied through, not just these specific paths. - Correct the automatic-registration description: GoldenHammer does not use config.URL (that's v6+; GoldenHammer predates it, from v5.4). It actually probes each hostname in the node's own TLS certificate with a HEAD request to <hostname>/n2n/identity/<did>/.well-known/openid-credential-issuer, registering the first one that responds 200 with a JSON content type (verified against golden_hammer/module.go's tryResolveURL and vcr/openid4vci/identifiers.go's resolveFromCertificate/testIdentifier). - Dropped the closing line quoting #4470's warning message: that PR hasn't merged to master yet, so the message doesn't exist there. Assisted by AI * docs(deployment): cross-reference node-http-services-baseurl from the /n2n endpoint description Point operators reading about the /n2n endpoint (Legacy Endpoints section) at the node-http-services-baseurl setup section, since server-to-server credential issuance for did:nuts DIDs runs over that same endpoint. Assisted by AI * docs(deployment): move node-http-services-baseurl section below Strict mode Moved out from under "Server options" (where it read as a sub-topic of the did:nuts/gRPC options table) to its own top-level section after Strict mode, matching Secrets/Strict mode's heading level. Assisted by AI * docs(deployment): address review feedback from #4475 - "service" -> "service endpoint"/"DID Document service endpoint" throughout, per review: "service" alone read like an actual service you need to run, not a DID Document term. - Reworded the automatic-registration paragraph per review: leads with what's added automatically (to ease configuration) and why it can fail, rather than "normally registered automatically... as long as...". - Corrected the gRPC/DAG fallback description. The reviewer's suggested "publishes an encrypted reference to a credential" isn't accurate either - what's actually encrypted is the transaction's recipient list (PAL), not the credential or a reference to it; the credential payload itself is never published to the network, it's fetched separately afterward over an authenticated connection between the specific nodes involved. Assisted by AI
Contributor
1 new issue
|
Resolve module path conflicts from the /v6 module rename. Assisted-by: AI
stevenvegt
force-pushed
the
fix/4469-tls-identifier-cache-empty-result
branch
from
September 9, 2026 12:49
6ae6e03 to
5c7e6ec
Compare
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.
Related: #4469
Problem
tlsIdentifierResolver(vcr/openid4vci/identifiers.go) cached its resolution result as soon as it completed without erroring — including an empty string when neither the DID document nor TLS-certificate-derived candidates yielded a base URL. Once that happened, every laterResolve()call for that DID short-circuited on the cached empty value for the life of the process, even after the missingnode-http-services-baseurlservice was added, permanently breaking OpenID4VCI credential-offer delivery until restart.Observed on the sender side (issuer), for a
credential_offersent withcredential_issuer:"":Observed on the receiver side, rejecting that same offer:
Fix
Bound the negative cache instead of removing it. An earlier version of this fix stopped caching empty results at all, but this resolver can be called on every OpenID4VCI request, so re-running resolution (including the DID document lookup) on every single call for a DID that never gets fixed isn't free — the original negative caching was likely deliberate. Instead, reuse the existing
lastAttempt/tlsAttemptIntervalthrottle (already used to rate-limit the expensive TLS-certificate-derived resolution) to also bound how long an empty result is treated as cached, rather than adding a separate cache window. A successful (non-empty) identifier is still cached indefinitely, unchanged.Guard against sending a broken offer. Nothing previously stopped an OpenID4VCI credential offer from being sent with an empty
credential_issuer— which is what produced the receiver-side "empty Credential Issuer Identifier" rejection shown above. Introducedopenid4vci.ErrIdentifierNotConfigured, returned when resolution succeeds but yields no identifier, andissueUsingOpenID4VCInow treats it like an unsupported wallet (fallback to the network) instead of constructing and sending the broken offer.Warn the operator, unlike the unsupported-wallet case. An unsupported wallet is the other party's problem — nothing this operator can act on, so it stays silent. A missing
node-http-services-baseurlservice is this node's own misconfiguration, so it now logs aWarnpointing the operator at what to search the documentation for, instead of failing the same silent way.Test plan
TestTLSIdentifierResolver/empty_result_is_cached_briefly,_not_forever(new) — asserts a cache hit on an immediate second call, then a re-check once the throttle window elapses.Test_vcr_GetOIDCIssuer/found_DID,_owned,_but_no_identifier_configured(new) — assertsErrIdentifierNotConfiguredwhen resolution yields an empty identifier.Test_issuer_Issue/OpenID4VCI/ok_-_OpenID4VCI_issuer_identifier_not_(yet)_configured_-_fallback_to_network(new) — asserts a quiet fallback to network publish, no offer sent, and the new Warn log.vcr,vcr/issuer,vcr/openid4vcisuites still pass.Assisted by AI