From b32c3cf5493a415d9e09c14e91211216bd8f27f2 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Fri, 4 Sep 2026 09:41:05 +0200 Subject: [PATCH 1/3] fix(vcr): don't permanently cache an empty OpenID4VCI base-URL identifier 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 --- vcr/openid4vci/identifiers.go | 7 +++++-- vcr/openid4vci/identifiers_test.go | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/vcr/openid4vci/identifiers.go b/vcr/openid4vci/identifiers.go index 37c2f4bed3..567bad7e31 100644 --- a/vcr/openid4vci/identifiers.go +++ b/vcr/openid4vci/identifiers.go @@ -100,8 +100,11 @@ type tlsIdentifierResolver struct { } func (t tlsIdentifierResolver) Resolve(id did.DID) (string, error) { + // Only a successfully resolved (non-empty) identifier is a valid cache hit; an empty result means + // resolution hasn't succeeded yet (e.g. the DID document is still missing its base URL service), and + // must be retried on every call rather than being cached forever. cached := t.cachedIdentifier.Load() - if cached != nil { + if cached != nil && *cached != "" { return *cached, nil } @@ -118,7 +121,7 @@ func (t tlsIdentifierResolver) Resolve(id did.DID) (string, error) { lastAttempt := time.Now() t.lastAttempt.Store(&lastAttempt) identifier, err = t.resolveFromCertificate(id) - if err == nil { + if err == nil && identifier != "" { t.cachedIdentifier.Store(&identifier) } return identifier, err diff --git a/vcr/openid4vci/identifiers_test.go b/vcr/openid4vci/identifiers_test.go index 1ae1232e14..24fed963cf 100644 --- a/vcr/openid4vci/identifiers_test.go +++ b/vcr/openid4vci/identifiers_test.go @@ -163,6 +163,23 @@ func TestTLSIdentifierResolver(t *testing.T) { require.NoError(t, err) require.Equal(t, "", actual) }) + t.Run("empty result is not cached forever", func(t *testing.T) { + ctrl := gomock.NewController(t) + underlying := NewMockIdentifierResolver(ctrl) + // Called twice: an empty result must not short-circuit future calls, so the DID document + // (the underlying resolver) is checked again every time, until it actually resolves. + underlying.EXPECT().Resolve(gomock.Any()).Times(2).Return("", nil) + + resolver := NewTLSIdentifierResolver(underlying, tlsConfig) + + actual, err := resolver.Resolve(id) + require.NoError(t, err) + require.Equal(t, "", actual) + + actual, err = resolver.Resolve(id) + require.NoError(t, err) + require.Equal(t, "", actual) + }) t.Run("ok - resolved from underlying resolver", func(t *testing.T) { ctrl := gomock.NewController(t) underlying := NewMockIdentifierResolver(ctrl) From 255452b9870b84de8b8b5cbec927d27680a0f600 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Fri, 4 Sep 2026 10:08:46 +0200 Subject: [PATCH 2/3] fix(vcr): bound negative identifier caching, guard against empty offers 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 --- vcr/issuer/issuer.go | 4 ++ vcr/issuer/issuer_test.go | 31 +++++++++++++++ vcr/openid4vci/identifiers.go | 44 ++++++++++++++-------- vcr/openid4vci/identifiers_test.go | 60 ++++++++++++++++++++++++++++-- vcr/vcr.go | 3 ++ vcr/vcr_test.go | 12 ++++++ 6 files changed, 134 insertions(+), 20 deletions(-) diff --git a/vcr/issuer/issuer.go b/vcr/issuer/issuer.go index 82f3cfdea7..744916f2e6 100644 --- a/vcr/issuer/issuer.go +++ b/vcr/issuer/issuer.go @@ -219,6 +219,10 @@ func (i issuer) issueUsingOpenID4VCI(ctx context.Context, credential vc.Verifiab } issuerDID, _ := did.ParseDID(credential.Issuer.String()) // can't fail, already created openidIssuer, err := i.openidHandlerFn(ctx, *issuerDID) + if errors.Is(err, openid4vci.ErrIdentifierNotConfigured) { + // Issuer not (yet) configured for OpenID4VCI (e.g. missing node-http-services-baseurl service) + return false, nil + } if err != nil { return false, fmt.Errorf("unable to discover issuer identifier: %w", err) } diff --git a/vcr/issuer/issuer_test.go b/vcr/issuer/issuer_test.go index 36a038fea9..88ab43563a 100644 --- a/vcr/issuer/issuer_test.go +++ b/vcr/issuer/issuer_test.go @@ -456,6 +456,37 @@ func Test_issuer_Issue(t *testing.T) { Public: false, }) + require.NoError(t, err) + assert.NotNil(t, result) + }) + t.Run("ok - OpenID4VCI issuer identifier not (yet) configured - fallback to network", func(t *testing.T) { + ctrl := gomock.NewController(t) + walletResolver := openid4vci.NewMockIdentifierResolver(ctrl) + walletResolver.EXPECT().Resolve(holderDID).AnyTimes().Return(walletIdentifier, nil) + publisher := NewMockPublisher(ctrl) + publisher.EXPECT().PublishCredential(gomock.Any(), gomock.Any(), gomock.Any()) + keyResolverMock := resolver.NewMockKeyResolver(ctrl) + keyResolverMock.EXPECT().ResolveKey(issuerDID, nil, resolver.AssertionMethod).Return(issuerKeyID, issuerKey, nil) + store := NewMockStore(ctrl) + store.EXPECT().StoreCredential(gomock.Any()) + sut := issuer{ + keyResolver: keyResolverMock, + store: store, + jsonldManager: jsonldManager, + trustConfig: trust.NewConfig(path.Join(io.TestDirectory(t), "trust.config")), + keyStore: nutsCryptoInstance, + walletResolver: walletResolver, + openidHandlerFn: func(_ context.Context, _ did.DID) (OpenIDHandler, error) { + return nil, openid4vci.ErrIdentifierNotConfigured + }, + networkPublisher: publisher, + } + + result, err := sut.Issue(ctx, template, CredentialOptions{ + Publish: true, + Public: false, + }) + require.NoError(t, err) assert.NotNil(t, result) }) diff --git a/vcr/openid4vci/identifiers.go b/vcr/openid4vci/identifiers.go index 567bad7e31..2a605ab6bf 100644 --- a/vcr/openid4vci/identifiers.go +++ b/vcr/openid4vci/identifiers.go @@ -20,6 +20,7 @@ package openid4vci import ( "crypto/tls" + "errors" "fmt" "github.com/nuts-foundation/go-did/did" "github.com/nuts-foundation/nuts-node/core" @@ -42,6 +43,11 @@ type IdentifierResolver interface { Resolve(id did.DID) (string, error) } +// ErrIdentifierNotConfigured is returned by callers wrapping an IdentifierResolver when resolution +// completed without error but yielded an empty identifier, meaning the DID isn't (yet) usable over +// OpenID4VCI (e.g. it's missing its node-http-services-baseurl service). +var ErrIdentifierNotConfigured = errors.New("no OpenID4VCI identifier configured for DID") + var _ IdentifierResolver = DIDIdentifierResolver{} var _ IdentifierResolver = NoopIdentifierResolver{} @@ -84,7 +90,12 @@ func NewTLSIdentifierResolver(underlying IdentifierResolver, config *tls.Config) return result } -const tlsAttemptInterval = time.Minute +// tlsAttemptInterval bounds how often the (expensive) TLS-certificate-derived resolution is attempted, +// and, since Resolve() can be called on every OpenID4VCI request, doubles as how long an empty result is +// cached for: long enough to protect against repeated resolution attempts under load, short enough that a +// later fix (e.g. a missing base URL service being added) is picked up automatically, without requiring a +// node restart. +var tlsAttemptInterval = time.Minute var tlsIdentifierResolverPort = 443 @@ -95,17 +106,21 @@ type tlsIdentifierResolver struct { config *tls.Config cachedIdentifier *atomic.Pointer[string] // lastAttempt is the time at which the last attempt to resolve the identifier from the TLS certificate was made. - // It is used to prevent spamming the local node, since it could be called on each OpenID4VCI request. lastAttempt *atomic.Pointer[time.Time] } func (t tlsIdentifierResolver) Resolve(id did.DID) (string, error) { - // Only a successfully resolved (non-empty) identifier is a valid cache hit; an empty result means - // resolution hasn't succeeded yet (e.g. the DID document is still missing its base URL service), and - // must be retried on every call rather than being cached forever. cached := t.cachedIdentifier.Load() - if cached != nil && *cached != "" { - return *cached, nil + if cached != nil { + if *cached != "" { + return *cached, nil + } + // An empty result stays cached only until the next TLS-certificate resolution attempt is due + // (the same throttle guarding that attempt below), so it doesn't take a node restart to pick up + // a later fix. + if time.Since(*t.lastAttempt.Load()) < tlsAttemptInterval { + return "", nil + } } identifier, err := t.underlying.Resolve(id) @@ -117,16 +132,13 @@ func (t tlsIdentifierResolver) Resolve(id did.DID) (string, error) { } // Could not load from DID document, try to derive from TLS certificate - if time.Since(*t.lastAttempt.Load()) > tlsAttemptInterval { - lastAttempt := time.Now() - t.lastAttempt.Store(&lastAttempt) - identifier, err = t.resolveFromCertificate(id) - if err == nil && identifier != "" { - t.cachedIdentifier.Store(&identifier) - } - return identifier, err + lastAttempt := time.Now() + t.lastAttempt.Store(&lastAttempt) + identifier, err = t.resolveFromCertificate(id) + if err == nil { + t.cachedIdentifier.Store(&identifier) } - return "", nil + return identifier, err } func (t tlsIdentifierResolver) resolveFromCertificate(id did.DID) (string, error) { diff --git a/vcr/openid4vci/identifiers_test.go b/vcr/openid4vci/identifiers_test.go index 24fed963cf..f31e1b5a07 100644 --- a/vcr/openid4vci/identifiers_test.go +++ b/vcr/openid4vci/identifiers_test.go @@ -19,7 +19,12 @@ package openid4vci import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" "errors" ssi "github.com/nuts-foundation/go-did" "github.com/nuts-foundation/go-did/did" @@ -28,14 +33,35 @@ import ( "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "math/big" "net/http" "net/http/httptest" "net/url" "strconv" "strings" "testing" + "time" ) +// selfSignedCertWithSAN generates a minimal self-signed certificate with a single DNS SAN, so tests +// relying on it don't also trigger resolution attempts against unrelated real hostnames. +func selfSignedCertWithSAN(t *testing.T, dnsName string) tls.Certificate { + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: dnsName}, + DNSNames: []string{dnsName}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + require.NoError(t, err) + leaf, err := x509.ParseCertificate(der) + require.NoError(t, err) + return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key, Leaf: leaf} +} + var issuerDID = did.MustParseDID("did:nuts:B8PUHs2AUHbFF1xLLK4eZjgErEcMXHxs68FteY7NDtCY") var issuerIdentifier = "http://example.com/n2n/identity/" + issuerDID.String() var issuerService = did.Service{ @@ -163,19 +189,45 @@ func TestTLSIdentifierResolver(t *testing.T) { require.NoError(t, err) require.Equal(t, "", actual) }) - t.Run("empty result is not cached forever", func(t *testing.T) { + t.Run("empty result is cached briefly, not forever", func(t *testing.T) { + // A local server that always says "not found", using a single-SAN certificate so resolution + // only ever tries this local server (no real network dial timeouts against unrelated hosts) - + // this test is about caching behavior, not network latency. + localCert := selfSignedCertWithSAN(t, "localhost") + localTLSConfig := &tls.Config{Certificates: []tls.Certificate{localCert}, InsecureSkipVerify: true} + httpServer := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + httpServer.TLS = localTLSConfig.Clone() + httpServer.StartTLS() + t.Cleanup(httpServer.Close) + serverURL, _ := url.Parse(httpServer.URL) + tlsIdentifierResolverPort, _ = strconv.Atoi(serverURL.Port()) + + originalInterval := tlsAttemptInterval + tlsAttemptInterval = 50 * time.Millisecond + t.Cleanup(func() { tlsAttemptInterval = originalInterval }) + ctrl := gomock.NewController(t) underlying := NewMockIdentifierResolver(ctrl) - // Called twice: an empty result must not short-circuit future calls, so the DID document - // (the underlying resolver) is checked again every time, until it actually resolves. + // Called twice: once for the initial (empty) resolution, and once more after the next + // TLS-attempt is due, proving a later fix (e.g. a missing service being added) is picked up + // without requiring a restart. underlying.EXPECT().Resolve(gomock.Any()).Times(2).Return("", nil) - resolver := NewTLSIdentifierResolver(underlying, tlsConfig) + resolver := NewTLSIdentifierResolver(underlying, httpServer.TLS) actual, err := resolver.Resolve(id) require.NoError(t, err) require.Equal(t, "", actual) + // Immediately calling again must be a cache hit (no additional underlying.Resolve call yet). + actual, err = resolver.Resolve(id) + require.NoError(t, err) + require.Equal(t, "", actual) + + time.Sleep(100 * time.Millisecond) + actual, err = resolver.Resolve(id) require.NoError(t, err) require.Equal(t, "", actual) diff --git a/vcr/vcr.go b/vcr/vcr.go index 7804c5f1e1..b4e1ddec67 100644 --- a/vcr/vcr.go +++ b/vcr/vcr.go @@ -141,6 +141,9 @@ func (c *vcr) resolveOpenID4VCIIdentifier(ctx context.Context, id did.DID) (stri StatusCode: http.StatusNotFound, } } + if identifier == "" { + return "", openid4vci.ErrIdentifierNotConfigured + } return identifier, nil } diff --git a/vcr/vcr_test.go b/vcr/vcr_test.go index affb404c1a..36a2e0118e 100644 --- a/vcr/vcr_test.go +++ b/vcr/vcr_test.go @@ -336,6 +336,18 @@ func Test_vcr_GetOIDCIssuer(t *testing.T) { require.Error(t, err) assert.Nil(t, actual) }) + t.Run("found DID, owned, but no identifier configured", func(t *testing.T) { + ctx := newMockContext(t) + ctx.documentOwner.EXPECT().IsOwner(gomock.Any(), id).Return(true, nil) + identifierResolver := openid4vci.NewMockIdentifierResolver(ctx.ctrl) + identifierResolver.EXPECT().Resolve(id).Return("", nil) + ctx.vcr.localWalletResolver = identifierResolver + + actual, err := ctx.vcr.GetOpenIDIssuer(context.Background(), id) + + require.ErrorIs(t, err, openid4vci.ErrIdentifierNotConfigured) + assert.Nil(t, actual) + }) } func Test_vcr_GetOIDCWallet(t *testing.T) { From 05a2685bf9e9c83fce100adaced3d3d8891d2b69 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Fri, 4 Sep 2026 10:14:51 +0200 Subject: [PATCH 3/3] feat(vcr): warn operator when local DID isn't configured for OpenID4VCI 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 --- vcr/issuer/issuer.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vcr/issuer/issuer.go b/vcr/issuer/issuer.go index 744916f2e6..4f2caed3b6 100644 --- a/vcr/issuer/issuer.go +++ b/vcr/issuer/issuer.go @@ -220,7 +220,11 @@ func (i issuer) issueUsingOpenID4VCI(ctx context.Context, credential vc.Verifiab issuerDID, _ := did.ParseDID(credential.Issuer.String()) // can't fail, already created openidIssuer, err := i.openidHandlerFn(ctx, *issuerDID) if errors.Is(err, openid4vci.ErrIdentifierNotConfigured) { - // Issuer not (yet) configured for OpenID4VCI (e.g. missing node-http-services-baseurl service) + // Unlike an unsupported wallet (the other party's problem), this is something the operator of + // *this* node needs to act on: fix the DID document so the local node can be discovered. + log.Logger(). + WithField(core.LogFieldDID, issuerDID.String()). + Warn("Local DID document is not properly configured for OpenID4VCI issuance; search the node documentation for 'node-http-services-baseurl' to fix it. Falling back to publishing over the Nuts network for now.") return false, nil } if err != nil {