Merge pull request #1028 from smallstep/mariano/cleanup-certificates

Refactor the Cleanup method

Author: maraino

kms/apiv1/options.go

@@ -76,15 +76,15 @@ type CertificateDeleter interface {
 	DeleteCertificate(req *DeleteCertificateRequest) error
 }
 
-// CleaningCertificateManager is an optional interface for KMS implementations
-// that support cleaning up expired certificates from a certificate store.
+// CredentialsCleaner is an optional interface for KMS implementations that
+// support cleaning up expired certificates from a certificate store.
 //
 // # Experimental
 //
 // Notice: This API is EXPERIMENTAL and may be changed or removed in a later
 // release.
-type CleaningCertificateManager interface {
-	Cleanup(req *CleanupCertificatesRequest) error
+type CredentialsCleaner interface {
+	CleanupCredentials(req *CleanupCredentialsRequest) error
 }
 
 // NameValidator is an interface that KeyManager can implement to validate a

kms/apiv1/requests.go

@@ -351,16 +351,16 @@ type DeleteCertificateRequest struct {
 	Name string
 }
 
-// CleanupCertificatesRequest is the parameter used in the Cleanup method of a
-// CleaningCertificateManager. It identifies a certificate-store scope (issuer,
-// store location, store name) and a subject for which expired certificates
-// should be removed.
+// CleanupCredentialsRequest is the parameter used in the CleanupCredentials
+// method of a CredentialsCleaner. It identifies a certificate-store scope
+// (issuer, store location, store name) and a subject for which expired
+// certificates should be removed.
 //
 // # Experimental
 //
 // Notice: This API is EXPERIMENTAL and may be changed or removed in a later
 // release.
-type CleanupCertificatesRequest struct {
+type CleanupCredentialsRequest struct {
 	Issuer        string
 	StoreLocation string
 	Store         string

kms/capi/capi.go

@@ -13,17 +13,17 @@ import (
 	"crypto/x509"
 	"encoding/binary"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"io"
 	"math/big"
 	"net/url"
 	"reflect"
 	"strconv"
 	"strings"
+	"time"
 	"unsafe"
 
-	"github.com/pkg/errors"
-
 	"golang.org/x/crypto/cryptobyte"
 	"golang.org/x/crypto/cryptobyte/asn1"
 	"golang.org/x/sys/windows"
@@ -1140,6 +1140,41 @@ func (k *CAPIKMS) DeleteCertificate(req *apiv1.DeleteCertificateRequest) error {
 	}
 }
 
+// CleanupCredentials implements [apiv1.CredentialsCleaner]. It finds all
+// certificates in the Windows certificate store issued to the subject in req by
+// the issuer in req, and deletes any that have already expired.
+func (k *CAPIKMS) CleanupCredentials(req *apiv1.CleanupCredentialsRequest) error {
+	certs, err := k.FindCertificatesByIssuer(&apiv1.LoadCertificateRequest{
+		Name: uri.New("capi", url.Values{
+			"issuer":         []string{req.Issuer},
+			"store-location": []string{req.StoreLocation},
+			"store":          []string{req.Store},
+		}).String(),
+	}, req.RawSubject)
+	if err != nil {
+		return fmt.Errorf("failed loading certificates by issuer %q: %w", req.Issuer, err)
+	}
+
+	var deleteErrors []error
+	now := time.Now()
+	for _, cert := range certs {
+		if cert.NotAfter.Before(now) {
+			deleteURI := uri.New("capi", url.Values{
+				"store-location": []string{req.StoreLocation},
+				"store":          []string{req.Store},
+				"issuer":         []string{req.Issuer},
+				"serial":         []string{"0x" + cert.SerialNumber.Text(16)},
+			}).String()
+
+			if err := k.DeleteCertificate(&apiv1.DeleteCertificateRequest{Name: deleteURI}); err != nil {
+				deleteErrors = append(deleteErrors, fmt.Errorf("failed deleting expired certificate (serial %s): %w", cert.SerialNumber.Text(16), err))
+			}
+		}
+	}
+
+	return errors.Join(deleteErrors...)
+}
+
 func (k *CAPIKMS) getKeyFlags(u *uriAttributes) (uint32, error) {
 	keyFlags := uint32(0)
 

kms/platform/kms.go

@@ -93,6 +93,8 @@ var (
 	_ apiv1.KeyManager              = (*KMS)(nil)
 	_ apiv1.CertificateManager      = (*KMS)(nil)
 	_ apiv1.CertificateChainManager = (*KMS)(nil)
+	_ apiv1.SearchableKeyManager    = (*KMS)(nil)
+	_ apiv1.CredentialsCleaner      = (*KMS)(nil)
 )
 
 type KMS struct {
@@ -291,9 +293,9 @@ func (k *KMS) SearchKeys(req *apiv1.SearchKeysRequest) (*apiv1.SearchKeysRespons
 	return nil, apiv1.NotImplementedError{}
 }
 
-func (k *KMS) Cleanup(req *apiv1.CleanupCertificatesRequest) error {
-	if km, ok := k.backend.(apiv1.CleaningCertificateManager); ok {
-		return km.Cleanup(req)
+func (k *KMS) CleanupCredentials(req *apiv1.CleanupCredentialsRequest) error {
+	if km, ok := k.backend.(apiv1.CredentialsCleaner); ok {
+		return km.CleanupCredentials(req)
 	}
 
 	return apiv1.NotImplementedError{}

kms/platform/kms_darwin_test.go

@@ -1,9 +1,12 @@
 package platform
 
 import (
+	"crypto/x509"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	"go.step.sm/crypto/kms/apiv1"
 )
 
 func mustPlatformKMS(t *testing.T) *KMS {
@@ -68,3 +71,35 @@ func Test_transformFromMacKMS(t *testing.T) {
 		})
 	}
 }
+
+func TestKMS_CleanupCredentials_mackms(t *testing.T) {
+	platformKMS := mustPlatformKMS(t)
+	// Use an expired certificate
+	chain := mustCreatePlatformCertificate(t, platformKMS, withTemplateModifier(func(c *x509.Certificate) *x509.Certificate {
+		c.NotBefore = time.Now().Add(-time.Minute).Truncate(time.Second)
+		c.NotAfter = time.Now().Add(-time.Second).Truncate(time.Second)
+		return c
+	}))
+
+	type args struct {
+		req *apiv1.CleanupCredentialsRequest
+	}
+	tests := []struct {
+		name      string
+		kms       *KMS
+		args      args
+		assertion assert.ErrorAssertionFunc
+	}{
+		{"not implemented", platformKMS, args{&apiv1.CleanupCredentialsRequest{
+			Issuer:     chain[0].Issuer.CommonName,
+			RawSubject: chain[0].RawSubject,
+		}}, func(tt assert.TestingT, err error, i ...interface{}) bool {
+			return assert.ErrorIs(tt, err, apiv1.NotImplementedError{})
+		}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.assertion(t, tt.kms.CleanupCredentials(tt.args.req))
+		})
+	}
+}

kms/platform/kms_other_test.go

@@ -3,9 +3,12 @@
 package platform
 
 import (
+	"crypto/x509"
 	"net/url"
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/assert"
 	"go.step.sm/crypto/kms/apiv1"
 	"go.step.sm/crypto/kms/uri"
 	"go.step.sm/crypto/tpm/available"
@@ -26,3 +29,37 @@ func mustPlatformKMS(t *testing.T) *KMS {
 func (k *KMS) SkipTests() bool {
 	return k.Type() == apiv1.DefaultKMS
 }
+
+func TestKMS_CleanupCredentials_other(t *testing.T) {
+	platformKMS := mustPlatformKMS(t)
+	// Use an expired certificate
+	chain := mustCreatePlatformCertificate(t, platformKMS, withTemplateModifier(func(c *x509.Certificate) *x509.Certificate {
+		c.NotBefore = time.Now().Add(-time.Minute).Truncate(time.Second)
+		c.NotAfter = time.Now().Add(-time.Second).Truncate(time.Second)
+		return c
+	}))
+
+	type args struct {
+		req *apiv1.CleanupCredentialsRequest
+	}
+	tests := []struct {
+		name      string
+		kms       *KMS
+		args      args
+		assertion assert.ErrorAssertionFunc
+	}{
+		{"not implemented", platformKMS, args{&apiv1.CleanupCredentialsRequest{
+			RawSubject: chain[0].RawSubject,
+		}}, func(tt assert.TestingT, err error, i ...interface{}) bool {
+			if platformKMS.Type() == apiv1.TPMKMS {
+				return assert.NoError(t, err)
+			}
+			return assert.ErrorIs(tt, err, apiv1.NotImplementedError{})
+		}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.assertion(t, tt.kms.CleanupCredentials(tt.args.req))
+		})
+	}
+}

kms/platform/kms_test.go

@@ -20,6 +20,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -101,21 +102,27 @@ func mustReadSigner(t *testing.T, path string) crypto.Signer {
 	return signer
 }
 
-func mustCertificate(t *testing.T, path string) []*x509.Certificate {
+func mustCertificate(t *testing.T, path string, opts ...createFuncOption) []*x509.Certificate {
 	t.Helper()
 
+	o := new(createOptions)
+	o.templateModifier = func(c *x509.Certificate) *x509.Certificate { return c }
+	for _, fn := range opts {
+		fn(o)
+	}
+
 	ca, err := minica.New()
 	require.NoError(t, err)
 
 	signer, err := keyutil.GenerateDefaultSigner()
 	require.NoError(t, err)
 
-	cert, err := ca.Sign(&x509.Certificate{
+	cert, err := ca.Sign(o.templateModifier(&x509.Certificate{
 		KeyUsage:    x509.KeyUsageDigitalSignature,
 		ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 		PublicKey:   signer.Public(),
 		DNSNames:    []string{"example.com"},
-	})
+	}))
 	require.NoError(t, err)
 
 	if path != "" {
@@ -186,6 +193,7 @@ type createOptions struct {
 	name                 string
 	noCleanup            bool
 	noCleanupCertificate bool
+	templateModifier     func(*x509.Certificate) *x509.Certificate
 }
 
 type createFuncOption func(*createOptions)
@@ -209,6 +217,12 @@ func withNoCleanupCertificate() createFuncOption {
 	}
 }
 
+func withTemplateModifier(fn func(*x509.Certificate) *x509.Certificate) createFuncOption {
+	return func(co *createOptions) {
+		co.templateModifier = fn
+	}
+}
+
 func mustCreatePlatformKey(t *testing.T, km *KMS, opts ...createFuncOption) *apiv1.CreateKeyResponse {
 	t.Helper()
 
@@ -242,6 +256,7 @@ func mustCreatePlatformCertificate(t *testing.T, km *KMS, opts ...createFuncOpti
 	t.Helper()
 
 	o := new(createOptions)
+	o.templateModifier = func(c *x509.Certificate) *x509.Certificate { return c }
 	o.name = platformCertName
 	for _, fn := range opts {
 		fn(o)
@@ -257,12 +272,12 @@ func mustCreatePlatformCertificate(t *testing.T, km *KMS, opts ...createFuncOpti
 	}
 
 	key := mustCreatePlatformKey(t, km, opts...)
-	cert, err := ca.Sign(&x509.Certificate{
+	cert, err := ca.Sign(o.templateModifier(&x509.Certificate{
 		KeyUsage:    x509.KeyUsageDigitalSignature,
 		ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 		PublicKey:   key.PublicKey,
 		DNSNames:    []string{"example.com"},
-	})
+	}))
 	require.NoError(t, err)
 
 	require.NoError(t, km.StoreCertificateChain(&apiv1.StoreCertificateChainRequest{
@@ -1289,6 +1304,39 @@ func TestKMS_SearchKeys(t *testing.T) {
 	}
 }
 
+func TestKMS_CleanupCredentials(t *testing.T) {
+	dir := t.TempDir()
+	softKMS := mustKMS(t, "kms:backend=softkms")
+	// Create an expired certificate
+	chain := mustCertificate(t, filepath.Join(dir, "chain.crt"), withTemplateModifier(func(c *x509.Certificate) *x509.Certificate {
+		c.NotBefore = time.Now().Add(-time.Minute).Truncate(time.Second)
+		c.NotAfter = time.Now().Add(-time.Second).Truncate(time.Second)
+		return c
+	}))
+
+	type args struct {
+		req *apiv1.CleanupCredentialsRequest
+	}
+	tests := []struct {
+		name      string
+		kms       *KMS
+		args      args
+		assertion assert.ErrorAssertionFunc
+	}{
+		{"not implemented", softKMS, args{&apiv1.CleanupCredentialsRequest{
+			Issuer:     chain[0].Issuer.CommonName,
+			RawSubject: chain[0].RawSubject,
+		}}, func(tt assert.TestingT, err error, i ...interface{}) bool {
+			return assert.ErrorIs(tt, err, apiv1.NotImplementedError{})
+		}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.assertion(t, tt.kms.CleanupCredentials(tt.args.req))
+		})
+	}
+}
+
 func Test_getBackend(t *testing.T) {
 	type args struct {
 		opts apiv1.Options