a few tests implemented

Signed-off-by: Sean Teeling <[email protected]>

Author: steeling

pkg/certificate/certificate.go

@@ -26,10 +26,9 @@ const (
 func (c *Certificate) newMergedWithRoot(root pem.RootCertificate) *Certificate {
 	cert := *c
 
-	buf := make([]byte, 0, len(root)+len(c.IssuingCA)+1)
-	buf = append(buf, root...)
-	buf = append(buf, '\n')
+	buf := make([]byte, 0, len(root)+len(c.IssuingCA))
 	buf = append(buf, c.IssuingCA...)
+	buf = append(buf, root...)
 	cert.IssuingCA = buf
 	return &cert
 }

pkg/certificate/fake_manager.go

@@ -7,6 +7,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/openservicemesh/osm/pkg/apis/config/v1alpha2"
+	"github.com/openservicemesh/osm/pkg/certificate/pem"
 )
 
 var (
@@ -34,15 +35,22 @@ func (c *fakeMRCClient) List() ([]*v1alpha2.MeshRootCertificate, error) {
 func (c *fakeMRCClient) AddEventHandler(cache.ResourceEventHandler) {}
 
 type fakeIssuer struct {
-	err error
+	err bool
+	id  string
 }
 
 // IssueCertificate is a testing helper to satisfy the certificate client interface
 func (i *fakeIssuer) IssueCertificate(cn CommonName, validityPeriod time.Duration) (*Certificate, error) {
+	if i.err {
+		return nil, fmt.Errorf("%s failed", i.id)
+	}
 	return &Certificate{
 		CommonName: cn,
 		Expiration: time.Now().Add(validityPeriod),
-	}, i.err
+		// simply used to distinguish the private/public key from other issuers
+		IssuingCA:  pem.RootCertificate(i.id),
+		PrivateKey: pem.PrivateKey(i.id),
+	}, nil
 }
 
 func (i *fakeIssuer) GetRootCertificate() *Certificate {

pkg/certificate/manager.go

@@ -1,13 +1,13 @@
 package certificate
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
 
 	"github.com/openservicemesh/osm/pkg/announcements"
-	"github.com/openservicemesh/osm/pkg/certificate/pem"
 	"github.com/openservicemesh/osm/pkg/errcode"
 	"github.com/openservicemesh/osm/pkg/k8s/events"
 	"github.com/openservicemesh/osm/pkg/messaging"
@@ -28,10 +28,12 @@ func NewManager(mrcClient MRCClient, serviceCertValidityDuration time.Duration,
 		return nil, err
 	}
 
+	c := &issuer{Issuer: client, ID: clientID}
+
 	m := &Manager{
 		// The root certificate signing all newly issued certificates
-		clients:                     map[string]Issuer{clientID: client},
-		keyIssuer:                   clientID,
+		keyIssuer:                   c,
+		pubIssuer:                   c,
 		serviceCertValidityDuration: serviceCertValidityDuration,
 		msgBroker:                   msgBroker,
 	}
@@ -96,57 +98,41 @@ func (m *Manager) getFromCache(cn CommonName) *Certificate {
 	log.Trace().Msgf("Certificate found in cache SerialNumber=%s", cert.GetSerialNumber())
 	if cert.ShouldRotate() {
 		log.Trace().Msgf("Certificate found in cache but has expired SerialNumber=%s", cert.GetSerialNumber())
+		return nil
 	}
 	return cert
 }
 
 // IssueCertificate implements Manager and returns a newly issued certificate from the given client.
 func (m *Manager) IssueCertificate(cn CommonName, validityPeriod time.Duration) (*Certificate, error) {
-	var additionalRoot pem.RootCertificate
-
+	// var additionalRoot pem.RootCertificate
+	var err error
 	cert := m.getFromCache(cn) // Don't call this while holding the lock
 
-	m.mu.Lock()
+	m.mu.RLock()
 	pubIssuer := m.pubIssuer
 	keyIssuer := m.keyIssuer
+	m.mu.RUnlock()
 
-	if pubIssuer != "" && (cert == nil || cert.pubIssuer != pubIssuer) {
-		pubCert, err := m.clients[keyIssuer].IssueCertificate(cn, validityPeriod)
+	start := time.Now()
+	if cert == nil || cert.keyIssuer != keyIssuer.ID || cert.pubIssuer != pubIssuer.ID {
+		fmt.Println("issuing!")
+		cert, err = keyIssuer.IssueCertificate(cn, validityPeriod)
 		if err != nil {
 			return nil, err
 		}
-		additionalRoot = pubCert.GetIssuingCA()
-	}
-
-	m.mu.Unlock()
-
-	// this can be nil
-	switch {
-	case cert == nil:
-		break
-	case cert.keyIssuer != keyIssuer:
-		break
-	case cert.pubIssuer == pubIssuer:
-		return cert, nil
-	case cert.pubIssuer != pubIssuer:
-		// copy to avoid mutating original. The copied fields may be pointers as well, but those shouldn't be modified
-		// later, and the field we mutate is completely changed.
-		certCopy := *cert
-		cert = cert.newMergedWithRoot(additionalRoot)
-		cert = &certCopy
-		m.cache.Store(cn, &cert)
-		return cert, nil
-	}
+		if pubIssuer.ID != keyIssuer.ID {
+			pubCert, err := pubIssuer.IssueCertificate(cn, validityPeriod)
+			if err != nil {
+				return nil, err
+			}
 
-	start := time.Now()
-	cert, err := m.clients[keyIssuer].IssueCertificate(cn, validityPeriod)
-	if err != nil {
-		return nil, err
-	}
+			cert = cert.newMergedWithRoot(pubCert.GetIssuingCA())
+		}
 
-	if pubIssuer != "" {
-		// Add the other issuing CA
-		cert = cert.newMergedWithRoot(additionalRoot)
+		cert.keyIssuer = keyIssuer.ID
+		cert.pubIssuer = m.pubIssuer.ID
+		fmt.Println("issued!", cert.keyIssuer, keyIssuer.ID)
 	}
 
 	m.cache.Store(cn, cert)
@@ -225,5 +211,5 @@ func (m *Manager) GetCertificate(cn CommonName) (*Certificate, error) {
 // GetRootCertificate returns the root
 func (m *Manager) GetRootCertificate() *Certificate {
 	// TODO(#4502): determine client(s) to use based on the rotation stage(s) of the client(s)
-	return m.clients[m.keyIssuer].GetRootCertificate()
+	return m.keyIssuer.GetRootCertificate()
 }

pkg/certificate/manager_test.go

@@ -1,7 +1,6 @@
 package certificate
 
 import (
-	"fmt"
 	"testing"
 	time "time"
 
@@ -10,6 +9,7 @@ import (
 	tassert "github.com/stretchr/testify/assert"
 
 	"github.com/openservicemesh/osm/pkg/announcements"
+	"github.com/openservicemesh/osm/pkg/certificate/pem"
 	"github.com/openservicemesh/osm/pkg/messaging"
 )
 
@@ -70,11 +70,8 @@ func TestRotor(t *testing.T) {
 	assert.NoError(err)
 	certRotateChan := msgBroker.GetCertPubSub().Sub(announcements.CertificateRotated.String())
 
-	start := time.Now()
-	// Wait for one certificate rotation to be announced and terminate
+	// Wait for two certificate rotations to be announced and terminate
 	<-certRotateChan
-
-	fmt.Printf("It took %+v to rotate certificate %s\n", time.Since(start), cn)
 	newCert, err := certManager.IssueCertificate(cn, validityPeriod)
 	assert.NoError(err)
 	assert.NotEqual(certA.GetExpiration(), newCert.GetExpiration())
@@ -216,7 +213,10 @@ func TestListCertificate(t *testing.T) {
 func TestGetRootCertificate(t *testing.T) {
 	assert := tassert.New(t)
 
-	manager := &Manager{clients: map[string]Issuer{"fake-issuer-1": &fakeIssuer{}}}
+	manager := &Manager{
+		keyIssuer: &issuer{ID: "fake-1", Issuer: &fakeIssuer{}},
+		pubIssuer: &issuer{ID: "fake-1", Issuer: &fakeIssuer{}},
+	}
 
 	got := manager.GetRootCertificate()
 
@@ -227,103 +227,113 @@ func TestIssueCertificate(t *testing.T) {
 	cn := CommonName("fake-cert-cn")
 	assert := tassert.New(t)
 
-	cm := &Manager{
-		// The root certificate signing all newly issued certificates
-		clients:   map[string]Issuer{"id1": &fakeIssuer{}},
-		keyIssuer: "id1",
-	}
-
 	t.Run("single key issuer", func(t *testing.T) {
+		cm := &Manager{
+			// The root certificate signing all newly issued certificates
+			keyIssuer: &issuer{ID: "id1", Issuer: &fakeIssuer{id: "id1"}},
+			pubIssuer: &issuer{ID: "id1", Issuer: &fakeIssuer{id: "id1"}},
+		}
 		// single keyIssuer, not cached
-		cert, err := cm.IssueCertificate(cn, time.Minute)
+		cert1, err := cm.IssueCertificate(cn, time.Minute)
 		assert.NoError(err)
-		assert.NotNil(cert)
-		assert.Equal(cert.keyIssuer, "id1")
-		assert.Empty(cert.pubIssuer)
+		assert.NotNil(cert1)
+		assert.Equal(cert1.keyIssuer, "id1")
+		assert.Equal(cert1.pubIssuer, "id1")
+		assert.Equal(cert1.GetIssuingCA(), pem.RootCertificate("id1"))
 
 		// single keyIssuer cached
-		cert1, err := cm.IssueCertificate(cn, time.Minute)
+		cert2, err := cm.IssueCertificate(cn, time.Minute)
 		assert.NoError(err)
-		assert.Equal(cert, cert1)
+		assert.Equal(cert1, cert2)
 
 		// single key issuer, old version cached
 		// TODO: could use informer logic to test mrc updates instead of just manually making changes.
-		cm.clients["id2"] = &fakeIssuer{}
-		delete(cm.clients, "id1")
-		cm.keyIssuer = "id2"
+		cm.keyIssuer = &issuer{ID: "id2", Issuer: &fakeIssuer{id: "id2"}}
+		cm.pubIssuer = &issuer{ID: "id2", Issuer: &fakeIssuer{id: "id2"}}
 
 		cert3, err := cm.IssueCertificate(cn, time.Minute)
 		assert.NoError(err)
-		assert.NotNil(cert)
-		assert.Equal(cert.keyIssuer, "id2")
-		assert.NotEqual(cert1, cert3)
-		assert.Empty(cert.pubIssuer)
+		assert.NotNil(cert3)
+		assert.Equal(cert3.keyIssuer, "id2")
+		assert.Equal(cert3.pubIssuer, "id2")
+		assert.NotEqual(cert2, cert3)
+		assert.Equal(cert3.GetIssuingCA(), pem.RootCertificate("id2"))
 	})
 
 	t.Run("2 issuers", func(t *testing.T) {
-		// {
-		// 	name: "2 issuers, nothing cached",
-		// 	test: func(a *tassert.Assertions) {
-
-		// 	},
-		// },
-		// {
-		// 	name: "2 issuers, old version cached",
-		// 	test: func(a *tassert.Assertions) {
-
-		// 	},
-		// },
-		// {
-		// 	name: "2 issuers, 1 cached",
-		// 	test: func(a *tassert.Assertions) {
-
-		// 	},
-		// },
-		// {
-		// 	name: "2 issuers, both cached",
-		// 	test: func(a *tassert.Assertions) {
-
-		// 	},
-		// },
-	})
-
-	testCases := []struct {
-		name string
-		test func(*tassert.Assertions)
-	}{
-		{
-			name: "",
-			test: func(a *tassert.Assertions) {
-
-			},
-		},
-		{
-			name: "single keyIssuer, cached",
-			test: func(a *tassert.Assertions) {
+		cm := &Manager{
+			// The root certificate signing all newly issued certificates
+			keyIssuer: &issuer{ID: "id1", Issuer: &fakeIssuer{id: "id1"}},
+			pubIssuer: &issuer{ID: "id2", Issuer: &fakeIssuer{id: "id2"}},
+		}
 
-			},
-		},
-		{
-			name: "single keyIssuer, old version cached",
-			test: func(a *tassert.Assertions) {
+		// Not cached
+		cert1, err := cm.IssueCertificate(cn, time.Minute)
+		assert.NoError(err)
+		assert.NotNil(cert1)
+		assert.Equal(cert1.keyIssuer, "id1")
+		assert.Equal(cert1.pubIssuer, "id2")
+		assert.Equal(cert1.GetIssuingCA(), pem.RootCertificate("idid2"))
 
-			},
-		},
+		// cached
+		cert2, err := cm.IssueCertificate(cn, time.Minute)
+		assert.NoError(err)
+		assert.Equal(cert1, cert2)
 
-		// 2 keyissuers
+		// cached, but pubIssuer is removed
+		cm.pubIssuer = cm.keyIssuer
+		cert3, err := cm.IssueCertificate(cn, time.Minute)
+		assert.NoError(err)
+		assert.NotEqual(cert1, cert3)
+		assert.Equal(cert3.keyIssuer, "id1")
+		assert.Equal(cert3.pubIssuer, "id1")
+		assert.Equal(cert3.GetIssuingCA(), pem.RootCertificate("id1"))
 
-		{
-			name: "bad key issuer",
-			test: func(a *tassert.Assertions) {
+		// cached, but keyIssuer is old
+		cm.keyIssuer = &issuer{ID: "id2", Issuer: &fakeIssuer{id: "id2"}}
+		cert4, err := cm.IssueCertificate(cn, time.Minute)
+		assert.NoError(err)
+		assert.NotEqual(cert3, cert4)
+		assert.Equal(cert4.keyIssuer, "id2")
+		assert.Equal(cert4.pubIssuer, "id1")
+		assert.Equal(cert4.GetIssuingCA(), pem.RootCertificate("idid1"))
+
+		// cached, but pubIssuer is old
+		cm.pubIssuer = &issuer{ID: "id3", Issuer: &fakeIssuer{id: "id3"}}
+		cert5, err := cm.IssueCertificate(cn, time.Minute)
+		assert.NoError(err)
+		assert.NotEqual(cert4, cert5)
+		assert.Equal(cert5.keyIssuer, "id2")
+		assert.Equal(cert5.pubIssuer, "id3")
+		assert.Equal(cert5.GetIssuingCA(), pem.RootCertificate("idid3"))
+	})
 
-			},
-		},
-		{
-			name: "bad pub issuer",
-			test: func(a *tassert.Assertions) {
+	t.Run("bad issuers", func(t *testing.T) {
+		cm := &Manager{
+			// The root certificate signing all newly issued certificates
+			keyIssuer: &issuer{ID: "id1", Issuer: &fakeIssuer{id: "id1", err: true}},
+			pubIssuer: &issuer{ID: "id2", Issuer: &fakeIssuer{id: "id2", err: true}},
+		}
 
-			},
-		},
-	}
+		// bad private key
+		cert, err := cm.IssueCertificate(cn, time.Minute)
+		assert.Nil(cert)
+		assert.EqualError(err, "id1 failed")
+
+		// bad public key
+		cm.keyIssuer = &issuer{ID: "id3", Issuer: &fakeIssuer{id: "id3"}}
+		cert, err = cm.IssueCertificate(cn, time.Minute)
+		assert.Nil(cert)
+		assert.EqualError(err, "id2 failed")
+
+		// insert a cached cert
+		cm.pubIssuer = cm.keyIssuer
+		cert, err = cm.IssueCertificate(cn, time.Minute)
+		assert.NoError(err)
 
+		// bad public key on an existing cached cert, because the pubIssuer is new
+		cm.pubIssuer = &issuer{ID: "id1", Issuer: &fakeIssuer{id: "id1", err: true}}
+		cert, err = cm.IssueCertificate(cn, time.Minute)
+		assert.EqualError(err, "id1 failed")
+	})
 }