Merge pull request #49 from irbekrm/fix_renewal

Fix default renewal time calculation

Author: jetstack-bot

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/container-storage-interface/spec v1.7.0
 	github.com/go-logr/logr v1.2.3
 	github.com/kubernetes-csi/csi-lib-utils v0.13.0
+	github.com/stretchr/testify v1.8.1
 	golang.org/x/net v0.7.0
 	google.golang.org/grpc v1.53.0
 	k8s.io/apimachinery v0.26.1
@@ -44,6 +45,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_golang v1.14.0 // indirect
 	github.com/prometheus/client_model v0.3.0 // indirect
 	github.com/prometheus/common v0.39.0 // indirect

manager/manager.go

@@ -95,7 +95,7 @@ type Options struct {
 // resume managing them if any already exist.
 func NewManager(opts Options) (*Manager, error) {
 	if opts.Client == nil {
-		return nil, errors.New("Client must be set")
+		return nil, errors.New("client must be set")
 	}
 	if opts.ClientForMetadata == nil {
 		opts.ClientForMetadata = func(_ metadata.Metadata) (cmclient.Interface, error) {
@@ -122,7 +122,7 @@ func NewManager(opts Options) (*Manager, error) {
 		}
 	}
 	if opts.Log == nil {
-		return nil, errors.New("Log must be set")
+		return nil, errors.New("log must be set")
 	}
 	if opts.MetadataReader == nil {
 		return nil, errors.New("MetadataReader must be set")
@@ -413,16 +413,13 @@ func (m *Manager) issue(ctx context.Context, volumeID string) error {
 		return fmt.Errorf("waiting for request: %w", err)
 	}
 
-	// Default the renewal time to be 2/3rds through the certificate's duration.
+	// Calculate the default next issuance time.
 	// The implementation's writeKeypair function may override this value before
 	// writing to the storage layer.
-	block, _ := pem.Decode(req.Status.Certificate)
-	crt, err := x509.ParseCertificate(block.Bytes)
+	renewalPoint, err := calculateNextIssuanceTime(req.Status.Certificate)
 	if err != nil {
-		return fmt.Errorf("parsing issued certificate: %w", err)
+		return fmt.Errorf("calculating next issuance time: %w", err)
 	}
-	duration := crt.NotAfter.Sub(crt.NotBefore)
-	renewalPoint := crt.NotBefore.Add(duration * (2 / 3))
 	meta.NextIssuanceTime = &renewalPoint
 
 	if err := m.writeKeypair(meta, key, req.Status.Certificate, req.Status.CA); err != nil {
@@ -722,3 +719,20 @@ func (m *Manager) Stop() {
 		delete(m.managedVolumes, k)
 	}
 }
+
+// calculateNextIssuanceTime will return the default time at which the certificate
+// should be renewed by the driver- 2/3rds through its lifetime (NotAfter -
+// NotBefore).
+func calculateNextIssuanceTime(chain []byte) (time.Time, error) {
+	block, _ := pem.Decode(chain)
+	crt, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		return time.Time{}, fmt.Errorf("parsing issued certificate: %w", err)
+	}
+
+	actualDuration := crt.NotAfter.Sub(crt.NotBefore)
+
+	renewBeforeNotAfter := actualDuration / 3
+
+	return crt.NotAfter.Add(-renewBeforeNotAfter), nil
+}

manager/manager_test.go

@@ -2,13 +2,20 @@ package manager
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"encoding/pem"
 	"fmt"
+	"math/big"
 	"testing"
 	"time"
 
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 	cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
 	"github.com/go-logr/logr/testr"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -383,6 +390,44 @@ func TestManager_cleanupStaleRequests(t *testing.T) {
 	}
 }
 
+func Test_calculateNextIssuanceTime(t *testing.T) {
+	notBefore := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)
+	notAfter := time.Date(1970, time.January, 4, 0, 0, 0, 0, time.UTC)
+	pk, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	template := x509.Certificate{
+		SerialNumber:          new(big.Int).Lsh(big.NewInt(1), 128),
+		NotBefore:             notBefore,
+		NotAfter:              notAfter,
+		BasicConstraintsValid: true,
+	}
+
+	derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &pk.PublicKey, pk)
+	require.NoError(t, err)
+	certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: derBytes})
+
+	tests := map[string]struct {
+		expTime time.Time
+		expErr  bool
+	}{
+		"if no attributes given, return 2/3rd certificate lifetime": {
+			expTime: notBefore.AddDate(0, 0, 2),
+			expErr:  false,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			renewTime, err := calculateNextIssuanceTime(certPEM)
+			assert.Equal(t, test.expErr, err != nil)
+			assert.Equal(t, test.expTime, renewTime)
+		})
+	}
+}
+
 func cr(crName, crNamespace, nodeID, volumeID string) *cmapi.CertificateRequest {
 	return &cmapi.CertificateRequest{
 		ObjectMeta: metav1.ObjectMeta{