diff --git a/vcr/issuer/issuer.go b/vcr/issuer/issuer.go index 1669af167..b055a3036 100644 --- a/vcr/issuer/issuer.go +++ b/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) } diff --git a/vcr/issuer/issuer_test.go b/vcr/issuer/issuer_test.go index d73a3e5cb..32b5c4e3a 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 84ff10f10..87ba114c4 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/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) { diff --git a/vcr/openid4vci/identifiers_test.go b/vcr/openid4vci/identifiers_test.go index c20d96e01..92d65feb7 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,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) diff --git a/vcr/vcr.go b/vcr/vcr.go index 56b2de754..e4e3f8764 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 64800d2fe..479409b36 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) {