diff --git a/auth/test.go b/auth/test.go index 26a627019..4142046cd 100644 --- a/auth/test.go +++ b/auth/test.go @@ -40,7 +40,6 @@ func testInstance(t *testing.T, cfg Config) *Auth { vcrInstance := vcr.NewTestVCRInstance(t) ctrl := gomock.NewController(t) pkiMock := pki.NewMockProvider(ctrl) - pkiMock.EXPECT().AddTruststore(gomock.Any()).AnyTimes() pkiMock.EXPECT().CreateTLSConfig(gomock.Any()).AnyTimes() vdrInstance := vdr.NewMockVDR(ctrl) vdrInstance.EXPECT().Resolver().AnyTimes() diff --git a/docs/pages/deployment/certificates.rst b/docs/pages/deployment/certificates.rst index 91e537656..048d7a10f 100644 --- a/docs/pages/deployment/certificates.rst +++ b/docs/pages/deployment/certificates.rst @@ -24,11 +24,6 @@ In ``did:x509`` the certificates are also used in the cryptographic proofs to ob This means the certificate chain now provides the root of trust and has stricter requirements than connection certificates. Trust in specific certificate CAs is configured per use-case in a :ref:`Discovery ` and :ref:`Policy ` definition file. -In addition, all trusted CA chains must also be added to the ``tls.truststorefile``. +CRLs from trusted chains (per the above definition files) are consulted when evaluating ``did:x509`` Verifiable Credentials. For certificate chains used in ``did:x509`` the Nuts-node always uses a hard-fail strategy, i.e., the ``pki.softfail`` config value is ignored during certificate validation for ``did:x509``. This means that the Nuts-node will not be able to verify a ``did:x509`` DID or Verifiable Credential signed by this DID Method if the CRL cannot be downloaded and the CRL in the cache is older than ``pki.maxupdatefailhours``. - -.. note:: - - Since the configured truststore file is now used for multiple purposes, it is no longer possible for the Nuts-node to determine what certificate chain is accepted/trusted for what purpose. - This means that all incoming TLS connections (including gRPC) must be offloaded in a proxy and validated against the expected certificate chain. \ No newline at end of file diff --git a/network/network_test.go b/network/network_test.go index 110d73a82..3bcd3f4d1 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -217,7 +217,6 @@ func TestNetwork_Configure(t *testing.T) { ctrl := gomock.NewController(t) ctx := createNetwork(t, ctrl) ctx.protocol.EXPECT().Configure(gomock.Any()) - ctx.pkiValidator.EXPECT().AddTruststore(gomock.Any()) ctx.pkiValidator.EXPECT().SetVerifyPeerCertificateFunc(gomock.Any()).Times(2) // tls.Configs: client, selfTestDialer ctx.pkiValidator.EXPECT().SubscribeDenied(gomock.Any()) ctx.network.connectionManager = nil @@ -277,7 +276,6 @@ func TestNetwork_Configure(t *testing.T) { ctrl := gomock.NewController(t) ctx := createNetwork(t, ctrl) ctx.protocol.EXPECT().Configure(gomock.Any()) - ctx.pkiValidator.EXPECT().AddTruststore(gomock.Any()) ctx.pkiValidator.EXPECT().SetVerifyPeerCertificateFunc(gomock.Any()) // selftestDialer tls.Config ctx.network.connectionManager = nil diff --git a/network/transport/grpc/config.go b/network/transport/grpc/config.go index 9940e37bd..4f89bec20 100644 --- a/network/transport/grpc/config.go +++ b/network/transport/grpc/config.go @@ -65,9 +65,6 @@ func WithTLS(clientCertificate tls.Certificate, trustStore *core.TrustStore, pki config.clientCert = &clientCertificate config.trustStore = trustStore.CertPool config.pkiValidator = pkiValidator - if err := pkiValidator.AddTruststore(trustStore.Certificates()); err != nil { - return err - } // Load TLS server certificate if the gRPC server should be started. if config.listenAddress != "" { config.serverCert = config.clientCert diff --git a/network/transport/grpc/config_test.go b/network/transport/grpc/config_test.go index 1afb50f3d..86338e200 100644 --- a/network/transport/grpc/config_test.go +++ b/network/transport/grpc/config_test.go @@ -53,7 +53,6 @@ func TestNewConfig(t *testing.T) { ts := &core.TrustStore{ CertPool: x509.NewCertPool(), } - pkiMock.EXPECT().AddTruststore(gomock.Any()) cfg, err := NewConfig(":1234", "foo", WithTLS(tlsCert, ts, pkiMock)) require.NoError(t, err) assert.Equal(t, &tlsCert, cfg.clientCert) @@ -64,7 +63,6 @@ func TestNewConfig(t *testing.T) { ts := &core.TrustStore{ CertPool: core.NewCertPool(x509Cert), } - pkiMock.EXPECT().AddTruststore(gomock.Any()) cfg, err := NewConfig(":1234", "foo", WithTLS(tlsCert, ts, pkiMock)) require.NoError(t, err) assert.Equal(t, &tlsCert, cfg.clientCert) diff --git a/network/transport/grpc/connection_manager_test.go b/network/transport/grpc/connection_manager_test.go index 89c06f1d7..e9b1e56f7 100644 --- a/network/transport/grpc/connection_manager_test.go +++ b/network/transport/grpc/connection_manager_test.go @@ -137,7 +137,6 @@ func Test_grpcConnectionManager_Connect(t *testing.T) { clientCert := testPKI.Certificate() ctrl := gomock.NewController(t) pkiMock := pki.NewMockValidator(ctrl) - pkiMock.EXPECT().AddTruststore(ts.Certificates()) pkiMock.EXPECT().SetVerifyPeerCertificateFunc(gomock.Any()) pkiMock.EXPECT().SubscribeDenied(gomock.Any()) @@ -557,7 +556,6 @@ func Test_grpcConnectionManager_Start(t *testing.T) { serverCert := testPKI.Certificate() ctrl := gomock.NewController(t) pkiMock := pki.NewMockValidator(ctrl) - pkiMock.EXPECT().AddTruststore(gomock.Any()).AnyTimes() t.Run("ok - gRPC server not bound", func(t *testing.T) { cm, err := NewGRPCConnectionManager(Config{}, nil, *nodeDID, nil) diff --git a/pki/interface.go b/pki/interface.go index 7b26e3776..ad3f429f8 100644 --- a/pki/interface.go +++ b/pki/interface.go @@ -31,7 +31,7 @@ var ( ErrCRLMissing = errors.New("crl is missing") ErrCRLExpired = errors.New("crl has expired") ErrCertRevoked = errors.New("certificate is revoked") - ErrCertUntrusted = errors.New("certificate's issuer is not trusted") + ErrUnknownIssuer = errors.New("unknown certificate issuer") // ErrDenylistMissing occurs when the denylist cannot be downloaded ErrDenylistMissing = errors.New("denylist cannot be retrieved") @@ -57,14 +57,20 @@ type Denylist interface { Subscribe(f func()) } +// Validator is used to check the revocation status of certificates on the issuer controlled CRL and the user controlled Denylist. +// It does NOT manage trust and assumes all presented certificates belong to a trusted certificate tree. type Validator interface { // CheckCRL returns an error if any of the certificates in the chain has been revoked, or if the request cannot be processed. - // ErrCertRevoked and ErrCertUntrusted indicate that at least one of the certificates is revoked, or signed by a CA that is not in the truststore. + // All certificates in the chain are considered trusted, which means that the caller has verified the integrity of the chain and appropriateness for the use-case. + // Any new CA / CRL in the chain will be added to the internal watchlist and updated periodically, so it MUST NOT be called on untrusted/invalid chains. + // The certificate chain MUST be sorted leaf to root. + // + // ErrCertRevoked and ErrUnknownIssuer indicate that at least one of the certificates is revoked, or signed by an unknown CA (so we have no key to verify the CRL). // ErrCRLMissing and ErrCRLExpired signal that at least one of the certificates cannot be validated reliably. // If the certificate was revoked on an expired CRL, it wil return ErrCertRevoked. + // // CheckCRL uses the configured soft-/hard-fail strategy // If set to soft-fail it ignores ErrCRLMissing and ErrCRLExpired errors. - // The certificate chain is expected to be sorted leaf to root. CheckCRL(chain []*x509.Certificate) error // CheckCRLStrict does the same as CheckCRL, except it always uses the hard-fail strategy. @@ -73,11 +79,6 @@ type Validator interface { // SetVerifyPeerCertificateFunc sets config.ValidatePeerCertificate to use CheckCRL. SetVerifyPeerCertificateFunc(config *tls.Config) error - // AddTruststore adds all CAs to the truststore for validation of CRL signatures. It also adds all CRL Distribution Endpoints found in the chain. - // CRL Distribution Points encountered at runtime, such as on end user certificates when calling CheckCRL, are only added to the monitored CRLs if their issuer is in the truststore. - // This fails if any of the issuers mentioned in the chain is not also in the chain or already in the truststore - AddTruststore(chain []*x509.Certificate) error - // SubscribeDenied registers a callback that is triggered everytime the denylist is updated. // This can be used to revalidate all certificates on long-lasting connections by calling CheckCRL on them again. SubscribeDenied(f func()) @@ -86,6 +87,7 @@ type Validator interface { // Provider is an interface for providing PKI services (e.g. TLS configuration, certificate validation). type Provider interface { Validator - // CreateTLSConfig creates a tls.Config for outbound connections. It returns nil (and no error) if TLS is disabled. + // CreateTLSConfig creates a tls.Config from the core.TLSConfig for outbound connections. + // It returns (nil, nil) if core.TLSConfig.Enabled() == false. CreateTLSConfig(cfg core.TLSConfig) (*tls.Config, error) } diff --git a/pki/mock.go b/pki/mock.go index e23e5b591..3411fc81d 100644 --- a/pki/mock.go +++ b/pki/mock.go @@ -135,20 +135,6 @@ func (m *MockValidator) EXPECT() *MockValidatorMockRecorder { return m.recorder } -// AddTruststore mocks base method. -func (m *MockValidator) AddTruststore(chain []*x509.Certificate) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddTruststore", chain) - ret0, _ := ret[0].(error) - return ret0 -} - -// AddTruststore indicates an expected call of AddTruststore. -func (mr *MockValidatorMockRecorder) AddTruststore(chain any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddTruststore", reflect.TypeOf((*MockValidator)(nil).AddTruststore), chain) -} - // CheckCRL mocks base method. func (m *MockValidator) CheckCRL(chain []*x509.Certificate) error { m.ctrl.T.Helper() @@ -227,20 +213,6 @@ func (m *MockProvider) EXPECT() *MockProviderMockRecorder { return m.recorder } -// AddTruststore mocks base method. -func (m *MockProvider) AddTruststore(chain []*x509.Certificate) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddTruststore", chain) - ret0, _ := ret[0].(error) - return ret0 -} - -// AddTruststore indicates an expected call of AddTruststore. -func (mr *MockProviderMockRecorder) AddTruststore(chain any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddTruststore", reflect.TypeOf((*MockProvider)(nil).AddTruststore), chain) -} - // CheckCRL mocks base method. func (m *MockProvider) CheckCRL(chain []*x509.Certificate) error { m.ctrl.T.Helper() diff --git a/pki/pki.go b/pki/pki.go index f158a650a..d5e1b2576 100644 --- a/pki/pki.go +++ b/pki/pki.go @@ -23,6 +23,7 @@ import ( "crypto/tls" "fmt" "github.com/nuts-foundation/nuts-node/core" + "os" "time" ) @@ -56,15 +57,39 @@ func (p *PKI) Config() any { return &p.config } -func (p *PKI) Configure(_ core.ServerConfig) error { +func (p *PKI) Configure(config core.ServerConfig) error { var err error p.validator, err = newValidator(p.config) if err != nil { return err } + trustStore, err := loadTrustStore(config.TLS.TrustStoreFile) + if err != nil { + return err + } + if trustStore != nil { + err = p.addCAs(trustStore.Certificates()) + if err != nil { + return err + } + } return nil } +func loadTrustStore(file string) (*core.TrustStore, error) { + if file == "" { + return nil, nil + } + if _, err := os.Stat(file); err != nil { + if os.IsNotExist(err) && file == core.NewServerConfig().TLS.TrustStoreFile { + // assume this is the default config value and ignore it + return nil, nil + } + return nil, fmt.Errorf("failed to load truststore: %w", err) + } + return core.LoadTrustStore(file) +} + func (p *PKI) Start() error { p.ctx, p.shutdown = context.WithCancel(context.Background()) p.validator.start(p.ctx) @@ -77,11 +102,11 @@ func (p *PKI) Shutdown() error { } // CreateTLSConfig creates a tls.Config based on the given core.TLSConfig for outbound connections to other Nuts nodes. -// It registers the CA certificates in the trust store in the validator which will start fetching their CRLs. -// It finally registers a VerifyPeerCertificateFunc in the tls.Config which will validate the peer certificate against the validator. +// It registers a VerifyPeerCertificateFunc in the tls.Config which will validate the peer certificate against the CRLs. // If TLS is not enabled, it returns nil (and no error). func (p *PKI) CreateTLSConfig(cfg core.TLSConfig) (*tls.Config, error) { - tlsConfig, trustStore, err := cfg.Load() + // This uses the provided truststore (truststore from config), NOT the CA list from the CRL Validator. + tlsConfig, _, err := cfg.Load() if err != nil { return nil, err } @@ -89,10 +114,6 @@ func (p *PKI) CreateTLSConfig(cfg core.TLSConfig) (*tls.Config, error) { // TLS is not enabled return nil, nil } - err = p.AddTruststore(trustStore.Certificates()) - if err != nil { - return nil, err - } _ = p.SetVerifyPeerCertificateFunc(tlsConfig) // no error can occur return tlsConfig, nil } diff --git a/pki/pki_test.go b/pki/pki_test.go index 1404102cb..5028f4601 100644 --- a/pki/pki_test.go +++ b/pki/pki_test.go @@ -54,11 +54,45 @@ func TestPKI_Configure(t *testing.T) { t.Run("ok", func(t *testing.T) { e := New() - err := e.Configure(core.ServerConfig{}) + err := e.Configure(*core.NewServerConfig()) assert.NoError(t, err) assert.NotNil(t, e.validator) }) + t.Run("loads truststore", func(t *testing.T) { + e := New() + cfg := core.NewServerConfig().TLS + cfg.TrustStoreFile = "test/truststore.pem" + cfg.CertFile = "test/A-valid.pem" + cfg.CertKeyFile = "test/A-valid.pem" + + err := e.Configure(core.ServerConfig{TLS: cfg}) + + assert.NoError(t, err) + assert.NotNil(t, e.validator) + // Assert the certificate in truststore.pem was loaded into the truststore + _, ok := e.cas.Load("CN=Intermediate A CA") + assert.True(t, ok) + }) + t.Run("no truststore", func(t *testing.T) { + e := New() + cfg := core.NewServerConfig() + cfg.TLS.TrustStoreFile = "" // remove default + + err := e.Configure(*cfg) + + assert.NoError(t, err) + assert.NotNil(t, e.validator) + }) + t.Run("invalid truststore file", func(t *testing.T) { + e := New() + cfg := core.NewServerConfig() + cfg.TLS.TrustStoreFile = "./not-the-default" + + err := e.Configure(*cfg) + + assert.ErrorContains(t, err, "failed to load truststore: stat ./not-the-default:") + }) t.Run("invalid config", func(t *testing.T) { e := New() e.config.Denylist = DenylistConfig{ @@ -66,7 +100,7 @@ func TestPKI_Configure(t *testing.T) { TrustedSigner: "definitely not valid", } - err := e.Configure(core.ServerConfig{}) + err := e.Configure(*core.NewServerConfig()) assert.Error(t, err) }) @@ -100,7 +134,7 @@ func TestPKI_CheckHealth(t *testing.T) { // Add truststore store, err := core.LoadTrustStore(truststore) // contains 1 CRL distribution point require.NoError(t, err) - require.NoError(t, e.validator.AddTruststore(store.Certificates())) + require.NoError(t, e.validator.addCAs(store.Certificates())) // Add Denylist testServer := denylistTestServer("") @@ -171,9 +205,6 @@ func TestPKI_CreateTLSConfig(t *testing.T) { assert.Equal(t, core.MinTLSVersion, tlsConfig.MinVersion) assert.NotEmpty(t, tlsConfig.Certificates) assert.NotNil(t, tlsConfig.RootCAs) - // Assert the certificate in truststore.pem was loaded into the truststore - _, ok := e.truststore.Load("CN=Intermediate A CA") - assert.True(t, ok) }) t.Run("TLS disabled", func(t *testing.T) { e := New() diff --git a/pki/validator.go b/pki/validator.go index 2ea0e523d..80db1ed63 100644 --- a/pki/validator.go +++ b/pki/validator.go @@ -47,9 +47,9 @@ type validator struct { // httpClient downloads the CRLs httpClient *http.Client - // truststore maps Certificate.Subject.String() to their certificate. - // Used for CRL signature checking. Immutable once Start() has been called. - truststore sync.Map + // cas maps a CA's Certificate.Subject.String() to their certificate. + // Used for CRL signature checking. + cas sync.Map // crls maps CRL endpoints to their x509.RevocationList crls sync.Map @@ -139,12 +139,26 @@ func (v *validator) checkCRL(chain []*x509.Certificate, softfail bool) error { var cert *x509.Certificate var err error for i := range chain { - cert = chain[len(chain)-1-i] // check in reverse order to prevent CRL expiration errors due to revoked CAs no longer issuing CRLs - if err = v.validateCert(cert); err != nil { + cert = chain[len(chain)-1-i] + err = v.validateCert(cert) + if err != nil { + // if the issuer of a cert is unknown, add entire chain to the ca/crl lists and try again + if errors.Is(err, ErrUnknownIssuer) { + if err = v.addCAs(chain); err == nil { + if err = v.validateCert(cert); err == nil { + continue + } + } + } + // handle error errOut := fmt.Errorf("%w: subject=%s, S/N=%s, issuer=%s", err, cert.Subject.String(), cert.SerialNumber.String(), cert.Issuer.String()) if softfail && (errors.Is(err, ErrCRLExpired) || errors.Is(err, ErrCRLMissing) || errors.Is(err, ErrDenylistMissing)) { // Accept the certificate even if it cannot be properly validated against the CRL or denylist + // NOTE: ErrUnknownIssuer should probably be part of the soft-fail list, but this could result in unsafe configurations for did:nuts: + // If the presented chain only contains the leaf certificate, and if the issuer of this leaf certificate is unknown, + // the node cannot check the CRL -> revocation status of the leaf cert will always succeed with soft-fail. + // This situation occurs when using the gRPC(-nuts)-network WithTLSOffloading and the configured truststore is incomplete. logger().WithError(errOut).Error("Certificate CRL check softfail bypass. Might be unsafe, find cause of failure!") continue } @@ -190,7 +204,7 @@ func (v *validator) validateCert(cert *x509.Certificate) error { var issuer *x509.Certificate issuer, ok = v.getCert(cert.Issuer.String()) if !ok { - return ErrCertUntrusted + return ErrUnknownIssuer } err := v.addEndpoints(issuer, []string{endpoint}) if err != nil { @@ -236,20 +250,20 @@ func (v *validator) validateCert(cert *x509.Certificate) error { return nil } -func (v *validator) AddTruststore(chain []*x509.Certificate) error { +func (v *validator) addCAs(chain []*x509.Certificate) error { // Add all CAs - // TODO: cert.Subject.String() is not guaranteed to be unique + // NOTE: cert.Subject.String() is not guaranteed to be unique, we rely on external validation of the chain var certificate *x509.Certificate var err error for _, certificate = range chain { - v.truststore.Store(certificate.Subject.String(), certificate) + v.cas.Store(certificate.Subject.String(), certificate) } // Add CRL distribution points, issuers should all be available now for _, certificate = range chain { issuer, ok := v.getCert(certificate.Issuer.String()) if !ok { - return fmt.Errorf("pki: certificate's issuer is not in the trust store: subject=%s, issuer=%s", certificate.Subject.String(), certificate.Issuer.String()) + return fmt.Errorf("pki: %w: subject=%s, issuer=%s", ErrUnknownIssuer, certificate.Subject.String(), certificate.Issuer.String()) } err = v.addEndpoints(issuer, certificate.CRLDistributionPoints) if err != nil { @@ -266,7 +280,7 @@ func (v *validator) SubscribeDenied(f func()) { } func (v *validator) getCert(subject string) (*x509.Certificate, bool) { - issuer, ok := v.truststore.Load(subject) + issuer, ok := v.cas.Load(subject) if !ok { return nil, false } diff --git a/pki/validator_test.go b/pki/validator_test.go index 8935c8d04..6332ce996 100644 --- a/pki/validator_test.go +++ b/pki/validator_test.go @@ -64,7 +64,7 @@ func TestValidator_Start(t *testing.T) { defer cancel() val, err := newValidatorWithHTTPClient(TestConfig(t), newClient()) require.NoError(t, err) - require.NoError(t, val.AddTruststore(store.Certificates())) + require.NoError(t, val.addCAs(store.Certificates())) // crls are empty val.crls.Range(func(key, value any) bool { @@ -86,7 +86,7 @@ func TestValidator_Start(t *testing.T) { assert.True(t, crl.lastUpdated.IsZero()) // pkiOverheid CA has expired, so is not updated } -func TestValidator_Validate(t *testing.T) { +func TestValidator_CheckCRL(t *testing.T) { val := newValidatorStarted(t) // load test certificates @@ -136,7 +136,28 @@ func TestValidator_Validate(t *testing.T) { }) t.Run("unknown issuer", func(t *testing.T) { val := &validator{} - testSoftHard(t, val, validCertA, ErrCertUntrusted, ErrCertUntrusted) + testSoftHard(t, val, validCertA, ErrUnknownIssuer, ErrUnknownIssuer) + }) + t.Run("recover from unknown issuer", func(t *testing.T) { + cert := validCertA + chain := []*x509.Certificate{cert} + val := newValidatorStarted(t) + // rebuild chain for cert from ca list and then remove the ca list + for cert.Subject.String() != cert.Issuer.String() { + val.cas.Range(func(caName, caCert any) bool { + thisCACert := caCert.(*x509.Certificate) + if thisCACert.Subject.String() == cert.Issuer.String() { + chain = append(chain, thisCACert) + cert = thisCACert + return false + } + return true + }) + } + // clear CA/CRL list so it is rebuilt at runtime + val.cas.Clear() + val.crls.Clear() + assert.NoError(t, val.CheckCRLStrict(chain)) // if strict works, the rest works too }) t.Run("missing crl", func(t *testing.T) { testSoftHard(t, val, validCertBWithRevokedCA, nil, ErrCRLMissing) @@ -176,7 +197,7 @@ func TestValidator_SetValidatePeerCertificateFunc(t *testing.T) { require.Nil(t, cfg.VerifyPeerCertificate) v := testValidator(t) - require.NoError(t, v.AddTruststore(store.Certificates())) + require.NoError(t, v.addCAs(store.Certificates())) err = v.SetVerifyPeerCertificateFunc(cfg) @@ -200,7 +221,7 @@ func TestValidator_SetValidatePeerCertificateFunc(t *testing.T) { }) } -func TestValidator_AddTruststore(t *testing.T) { +func TestValidator_addCAs(t *testing.T) { store, err := core.LoadTrustStore(truststore) require.NoError(t, err) @@ -208,7 +229,7 @@ func TestValidator_AddTruststore(t *testing.T) { val, err := newValidator(TestConfig(t)) require.NoError(t, err) - err = val.AddTruststore(store.Certificates()) + err = val.addCAs(store.Certificates()) assert.NotNil(t, val) }) @@ -218,9 +239,9 @@ func TestValidator_AddTruststore(t *testing.T) { val, err := newValidator(Config{Softfail: true}) require.NoError(t, err) - err = val.AddTruststore(noRootStore) + err = val.addCAs(noRootStore) - assert.ErrorContains(t, err, "certificate's issuer is not in the trust store") + assert.ErrorIs(t, err, ErrUnknownIssuer) }) } @@ -394,7 +415,7 @@ func testValidator(t *testing.T) *validator { require.Len(t, store.Certificates(), 4) val, err := newValidatorWithHTTPClient(DefaultConfig(), newClient()) require.NoError(t, err) - require.NoError(t, val.AddTruststore(store.Certificates())) + require.NoError(t, val.addCAs(store.Certificates())) return val } diff --git a/vcr/verifier/signature_verifier_test.go b/vcr/verifier/signature_verifier_test.go index 4180c11d6..cf8712bb8 100644 --- a/vcr/verifier/signature_verifier_test.go +++ b/vcr/verifier/signature_verifier_test.go @@ -34,7 +34,6 @@ import ( "github.com/google/uuid" "github.com/lestrrat-go/jwx/v2/cert" "github.com/lestrrat-go/jwx/v2/jwa" - "github.com/nuts-foundation/nuts-node/pki" testpki "github.com/nuts-foundation/nuts-node/test/pki" "github.com/nuts-foundation/nuts-node/vdr/didx509" "os" @@ -99,8 +98,7 @@ func TestSignatureVerifier_VerifySignature(t *testing.T) { assert.NoError(t, err) t.Run("happy flow", func(t *testing.T) { - sv, validator := x509VerifierTestSetup(t) - validator.EXPECT().CheckCRLStrict(gomock.Any()).Return(nil) + sv := x509VerifierTestSetup(t) err = sv.VerifySignature(*cred, nil) assert.NoError(t, err) }) @@ -111,19 +109,8 @@ func TestSignatureVerifier_VerifySignature(t *testing.T) { ExtractProtectedHeaders = func(jwt string) (map[string]interface{}, error) { return nil, expectedError } - sv, _ := x509VerifierTestSetup(t) + sv := x509VerifierTestSetup(t) err = sv.VerifySignature(*cred, nil) - assert.Error(t, err) - assert.ErrorIs(t, err, expectedError) - }) - t.Run("wrong ura", func(t *testing.T) { - cred, err := buildX509Credential(chain, signingCert, rootCert, signingKey, ura) - assert.NoError(t, err) - sv, validator := x509VerifierTestSetup(t) - expectedError := errors.New("wrong ura") - validator.EXPECT().CheckCRLStrict(gomock.Any()).Return(expectedError) - err = sv.VerifySignature(*cred, nil) - assert.Error(t, err) assert.ErrorIs(t, err, expectedError) }) }) @@ -370,14 +357,11 @@ func signatureVerifierTestSetup(t testing.TB) (signatureVerifier, *resolver.Mock }, keyResolver } -func x509VerifierTestSetup(t testing.TB) (signatureVerifier, *pki.MockValidator) { - ctrl := gomock.NewController(t) - pkiMock := pki.NewMockValidator(ctrl) - var keyResolver = resolver.DIDKeyResolver{ - Resolver: didx509.NewResolver(pkiMock), - } +func x509VerifierTestSetup(t testing.TB) signatureVerifier { return signatureVerifier{ - keyResolver: keyResolver, + keyResolver: resolver.DIDKeyResolver{ + Resolver: didx509.NewResolver(), + }, jsonldManager: jsonld.NewTestJSONLDManager(t), - }, pkiMock + } } diff --git a/vdr/didx509/resolver.go b/vdr/didx509/resolver.go index 4a76a4ed8..d840e704b 100644 --- a/vdr/didx509/resolver.go +++ b/vdr/didx509/resolver.go @@ -26,7 +26,6 @@ import ( ssi "github.com/nuts-foundation/go-did" "github.com/nuts-foundation/go-did/did" "github.com/nuts-foundation/nuts-node/core" - "github.com/nuts-foundation/nuts-node/pki" "github.com/nuts-foundation/nuts-node/vdr/resolver" "strings" ) @@ -61,14 +60,11 @@ var ( var _ resolver.DIDResolver = &Resolver{} // NewResolver creates a new Resolver. -func NewResolver(pkiValidator pki.Validator) *Resolver { - return &Resolver{ - pkiValidator: pkiValidator, - } +func NewResolver() *Resolver { + return &Resolver{} } type Resolver struct { - pkiValidator pki.Validator } // X509DidPolicy represents an X.509 DID policy that includes a policy name and corresponding value. @@ -94,6 +90,7 @@ type X509DidReference struct { // * Besides the "san" policies "email" / "dns" / "uri", the san policy "otherName" is also implemented. // * The policy "subject" also supports "serialNumber", besides the "CN" / "L" / "ST" / "O" / "OU" / "C" / "STREET" fields. // * The policy "eku" is not implemented. +// Resolve does NOT check the CRLs on the certificate chain. func (r Resolver) Resolve(id did.DID, metadata *resolver.ResolveMetadata) (*did.Document, *resolver.DocumentMetadata, error) { if id.Method != MethodName { return nil, nil, fmt.Errorf("unsupported DID method: %s", id.Method) @@ -132,7 +129,7 @@ func (r Resolver) Resolve(id did.DID, metadata *resolver.ResolveMetadata) (*did. chainWithoutLeaf = append(chainWithoutLeaf, curr) } trustStore := core.BuildTrustStore(chainWithoutLeaf) - verifiedChains, err := validationCert.Verify(x509.VerifyOptions{ + _, err = validationCert.Verify(x509.VerifyOptions{ Intermediates: core.NewCertPool(trustStore.IntermediateCAs), Roots: core.NewCertPool(trustStore.RootCAs), }) @@ -145,10 +142,9 @@ func (r Resolver) Resolve(id did.DID, metadata *resolver.ResolveMetadata) (*did. return nil, nil, err } - err = r.pkiValidator.CheckCRLStrict(verifiedChains[0]) - if err != nil { - return nil, nil, err - } + // Do NOT check CRLs. Checking CRLs at this point would allow unsanitized user input into the CRL checker DB. + // CRL checking should be done where the appropriateness of usage of (i.e., trust in) the certificate chain can be confirmed. + document, err := createDidDocument(id, validationCert) if err != nil { return nil, nil, err diff --git a/vdr/didx509/resolver_test.go b/vdr/didx509/resolver_test.go index 499b8ee30..05aad0dc7 100644 --- a/vdr/didx509/resolver_test.go +++ b/vdr/didx509/resolver_test.go @@ -22,25 +22,20 @@ import ( "crypto/sha1" "crypto/sha512" "encoding/base64" - "errors" "fmt" "github.com/lestrrat-go/jwx/v2/cert" "github.com/minio/sha256-simd" "github.com/nuts-foundation/go-did/did" - "github.com/nuts-foundation/nuts-node/pki" testpki "github.com/nuts-foundation/nuts-node/test/pki" "github.com/nuts-foundation/nuts-node/vdr/resolver" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" "strings" "testing" ) func TestManager_Resolve_OtherName(t *testing.T) { - ctrl := gomock.NewController(t) - validator := pki.NewMockValidator(ctrl) - didResolver := NewResolver(validator) + didResolver := NewResolver() metadata := resolver.ResolveMetadata{} otherNameValue := "A_BIG_STRING" @@ -76,7 +71,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { }) t.Run("happy flow, policy depth of 0", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s", "sha256", sha256Sum(rootCertificate.Raw))) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) @@ -88,7 +82,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { assert.NotNil(t, resolve.VerificationMethod.FindByID(*didUrl)) }) t.Run("happy flow, policy depth of 1 and primary value", func(t *testing.T) { - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) @@ -102,7 +95,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow, policy depth of 1 and secondary value", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s", "sha256", sha256Sum(rootCertificate.Raw), otherNameValueSecondary)) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) @@ -116,7 +108,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow, policy depth of 2 of type OU", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s::subject:OU:%s", "sha256", sha256Sum(rootCertificate.Raw), otherNameValue, "The%20A-Team")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) @@ -130,7 +121,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow, policy depth of 2, primary and secondary", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s::san:otherName:%s", "sha256", sha256Sum(rootCertificate.Raw), otherNameValue, otherNameValueSecondary)) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) @@ -144,7 +134,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow, policy depth of 2, secondary and primary", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s::san:otherName:%s", "sha256", sha256Sum(rootCertificate.Raw), otherNameValue, otherNameValueSecondary)) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) @@ -157,7 +146,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { }) t.Run("happy flow with only x5t header", func(t *testing.T) { delete(metadata.JwtProtectedHeaders, X509CertThumbprintS256Header) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -166,7 +154,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { }) t.Run("happy flow with only x5t#S256 header", func(t *testing.T) { delete(metadata.JwtProtectedHeaders, X509CertThumbprintHeader) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -186,7 +173,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow with alternative hash alg sha512", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s", "sha512", sha512Sum(rootCertificate.Raw), otherNameValue)) delete(metadata.JwtProtectedHeaders, X509CertThumbprintHeader) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -196,7 +182,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow with alternative hash alg sha384", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s", "sha384", sha384Sum(rootCertificate.Raw), otherNameValue)) delete(metadata.JwtProtectedHeaders, X509CertThumbprintHeader) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -206,7 +191,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { t.Run("happy flow with ca-fingerprint pointing at intermediate CA", func(t *testing.T) { subjectDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s", "sha256", sha256Sum(certs[2].Raw), otherNameValue)) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(subjectDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -287,15 +271,6 @@ func TestManager_Resolve_OtherName(t *testing.T) { _, _, err = didResolver.Resolve(rootDID, &metadata) require.ErrorContains(t, err, "did:509 certificate chain validation failed: x509: certificate signed by unknown authority") }) - t.Run("can't check CRL", func(t *testing.T) { - ctrl := gomock.NewController(t) - validator := pki.NewMockValidator(ctrl) - didResolver := NewResolver(validator) - expectedErr := errors.New("broken chain") - validator.EXPECT().CheckCRLStrict(gomock.Any()).Return(expectedErr) - _, _, err := didResolver.Resolve(rootDID, &metadata) - require.EqualError(t, err, expectedErr.Error()) - }) t.Run("wrong otherName value", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:otherName:%s", "sha256", sha256Sum(rootCertificate.Raw), "ANOTHER_BIG_STRING")) _, _, err := didResolver.Resolve(rootDID, &metadata) @@ -336,9 +311,7 @@ func TestManager_Resolve_OtherName(t *testing.T) { } func TestManager_Resolve_San_Generic(t *testing.T) { - ctrl := gomock.NewController(t) - validator := pki.NewMockValidator(ctrl) - didResolver := NewResolver(validator) + didResolver := NewResolver() metadata := resolver.ResolveMetadata{} certs, _, err := testpki.BuildCertChain([]string{}, "") @@ -371,7 +344,6 @@ func TestManager_Resolve_San_Generic(t *testing.T) { t.Run("happy SAN DNS www.example.com", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:dns:%s", "sha256", sha256Sum(rootCertificate.Raw), "www.example.com")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -386,7 +358,6 @@ func TestManager_Resolve_San_Generic(t *testing.T) { t.Run("happy SAN ip", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:ip:%s", "sha256", sha256Sum(rootCertificate.Raw), "192.1.2.3")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -401,7 +372,6 @@ func TestManager_Resolve_San_Generic(t *testing.T) { t.Run("happy SAN email", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::san:email:%s", "sha256", sha256Sum(rootCertificate.Raw), "info%40example.com")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -416,9 +386,7 @@ func TestManager_Resolve_San_Generic(t *testing.T) { } func TestManager_Resolve_Subject(t *testing.T) { - ctrl := gomock.NewController(t) - validator := pki.NewMockValidator(ctrl) - didResolver := NewResolver(validator) + didResolver := NewResolver() metadata := resolver.ResolveMetadata{} otherNameValue := "A_BIG_STRING" @@ -461,7 +429,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow CN www.example.com", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:CN:%s", "sha256", sha256Sum(rootCertificate.Raw), "www.example.com")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -475,7 +442,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow O", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:O:%s", "sha256", sha256Sum(rootCertificate.Raw), "NUTS%20Foundation")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -483,7 +449,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow O and CN", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:O:%s::subject:CN:%s", "sha256", sha256Sum(rootCertificate.Raw), "NUTS%20Foundation", "www.example.com")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -491,7 +456,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow O and CN and OU", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:O:%s::subject:CN:%s::subject:OU:%s", "sha256", sha256Sum(rootCertificate.Raw), "NUTS%20Foundation", "www.example.com", "The%20A-Team")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -529,7 +493,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow L Amsterdam", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:L:%s", "sha256", sha256Sum(rootCertificate.Raw), "Amsterdam")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -537,7 +500,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow L Den Haag", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:L:%s", "sha256", sha256Sum(rootCertificate.Raw), "The%20Hague")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -551,7 +513,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow C", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:C:%s", "sha256", sha256Sum(rootCertificate.Raw), "NL")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -565,7 +526,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow ST", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:ST:%s", "sha256", sha256Sum(rootCertificate.Raw), "Noord-Holland")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -579,7 +539,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow STREET", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:STREET:%s", "sha256", sha256Sum(rootCertificate.Raw), "Amsterdamseweg%20100")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -594,7 +553,6 @@ func TestManager_Resolve_Subject(t *testing.T) { t.Run("happy flow serialNumber", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:serialNumber:%s", "sha256", sha256Sum(rootCertificate.Raw), "32121323")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) @@ -608,7 +566,6 @@ func TestManager_Resolve_Subject(t *testing.T) { }) t.Run("happy flow OU", func(t *testing.T) { rootDID := did.MustParseDID(fmt.Sprintf("did:x509:0:%s:%s::subject:OU:%s", "sha256", sha256Sum(rootCertificate.Raw), "The%20A-Team")) - validator.EXPECT().CheckCRLStrict(gomock.Any()) resolve, documentMetadata, err := didResolver.Resolve(rootDID, &metadata) require.NoError(t, err) assert.NotNil(t, resolve) diff --git a/vdr/vdr.go b/vdr/vdr.go index 618e6eaef..e4b630460 100644 --- a/vdr/vdr.go +++ b/vdr/vdr.go @@ -163,7 +163,7 @@ func (r *Module) Configure(config core.ServerConfig) error { r.didResolver.(*resolver.DIDResolverRouter).Register(didjwk.MethodName, didjwk.NewResolver()) r.didResolver.(*resolver.DIDResolverRouter).Register(didkey.MethodName, didkey.NewResolver()) - r.didResolver.(*resolver.DIDResolverRouter).Register(didx509.MethodName, didx509.NewResolver(r.pkiValidator)) + r.didResolver.(*resolver.DIDResolverRouter).Register(didx509.MethodName, didx509.NewResolver()) // Register DID resolver and DID methods we can resolve r.ownedDIDResolver = didsubject.Resolver{DB: db}