fix(vcr): bound negative OpenID4VCI identifier caching, guard against empty offers

vcr/issuer/issuer.go

@@ -219,6 +219,14 @@ 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) {
+		// 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 {
 		return false, fmt.Errorf("unable to discover issuer identifier: %w", err)
 	}

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)
 		})

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/v6/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,14 +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) {
 	cached := t.cachedIdentifier.Load()
 	if cached != nil {
-		return *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)
@@ -114,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 {
-			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) {

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,6 +189,49 @@ func TestTLSIdentifierResolver(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, "", actual)
 	})
+	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: 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, 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)
+	})
 	t.Run("ok - resolved from underlying resolver", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
 		underlying := NewMockIdentifierResolver(ctrl)

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
 }
 

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) {