Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Commit ecc4e67

Browse files
steelingsteeling
authored
feat(cert): cert rotation state management (#4743)
Enables the certificate handler to correctly issue certificates from the correct issuer based on the set state. We don't actually populate any additional states yet. Signed-off-by: Sean Teeling <[email protected]>
1 parent 9b11d76 commit ecc4e67

File tree

8 files changed

+240
-151
lines changed

8 files changed

+240
-151
lines changed

‎pkg/certificate/certificate.go

+13
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,19 @@ const (
2020
noiseSeconds = 5
2121
)
2222

23+
// mergeRoot will merge in the provided root CA for future calls to GetIssuingCA. It guarantees to not mutate
24+
// the underlying IssuingCA field. By doing so, we ensure that we don't need locks.
25+
// NOTE: this does not return a full copy, mutations to the other byte slices could cause data races.
26+
func (c *Certificate) newMergedWithRoot(root pem.RootCertificate) *Certificate {
27+
cert := *c
28+
29+
buf := make([]byte, 0, len(root)+len(c.IssuingCA))
30+
buf = append(buf, c.IssuingCA...)
31+
buf = append(buf, root...)
32+
cert.IssuingCA = buf
33+
return &cert
34+
}
35+
2336
// GetCommonName returns the Common Name of the certificate
2437
func (c *Certificate) GetCommonName() CommonName {
2538
return c.CommonName

‎pkg/certificate/fake_manager.go

+13-9
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,8 @@ import (
44
"fmt"
55
time "time"
66

7-
"k8s.io/client-go/tools/cache"
8-
97
"github.com/openservicemesh/osm/pkg/apis/config/v1alpha2"
8+
"github.com/openservicemesh/osm/pkg/certificate/pem"
109
)
1110

1211
var (
@@ -15,8 +14,8 @@ var (
1514

1615
type fakeMRCClient struct{}
1716

18-
func (c *fakeMRCClient) GetCertIssuerForMRC(mrc *v1alpha2.MeshRootCertificate) (Issuer, error) {
19-
return &fakeIssuer{}, nil
17+
func (c *fakeMRCClient) GetCertIssuerForMRC(mrc *v1alpha2.MeshRootCertificate) (Issuer, string, error) {
18+
return &fakeIssuer{}, "fake-issuer-1", nil
2019
}
2120

2221
// List returns the single, pre-generated MRC. It is intended to implement the certificate.MRCClient interface.
@@ -25,17 +24,22 @@ func (c *fakeMRCClient) List() ([]*v1alpha2.MeshRootCertificate, error) {
2524
return []*v1alpha2.MeshRootCertificate{{}}, nil
2625
}
2726

28-
// AddEventHandler is a no-op for the legacy client. The previous client could not handle changes, but we need this
29-
// method to implement the certificate.MRCClient interface.
30-
func (c *fakeMRCClient) AddEventHandler(cache.ResourceEventHandler) {}
31-
32-
type fakeIssuer struct{}
27+
type fakeIssuer struct {
28+
err bool
29+
id string
30+
}
3331

3432
// IssueCertificate is a testing helper to satisfy the certificate client interface
3533
func (i *fakeIssuer) IssueCertificate(cn CommonName, validityPeriod time.Duration) (*Certificate, error) {
34+
if i.err {
35+
return nil, fmt.Errorf("%s failed", i.id)
36+
}
3637
return &Certificate{
3738
CommonName: cn,
3839
Expiration: time.Now().Add(validityPeriod),
40+
// simply used to distinguish the private/public key from other issuers
41+
IssuingCA: pem.RootCertificate(i.id),
42+
PrivateKey: pem.PrivateKey(i.id),
3943
}, nil
4044
}
4145

‎pkg/certificate/manager.go

+41-53
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package certificate
33
import (
44
"time"
55

6-
"github.com/pkg/errors"
76
"github.com/rs/zerolog/log"
87

98
"github.com/openservicemesh/osm/pkg/announcements"
@@ -20,14 +19,17 @@ func NewManager(mrcClient MRCClient, serviceCertValidityDuration time.Duration,
2019
return nil, err
2120
}
2221

23-
client, err := mrcClient.GetCertIssuerForMRC(mrcs[0])
22+
client, clientID, err := mrcClient.GetCertIssuerForMRC(mrcs[0])
2423
if err != nil {
2524
return nil, err
2625
}
2726

27+
c := &issuer{Issuer: client, ID: clientID}
28+
2829
m := &Manager{
2930
// The root certificate signing all newly issued certificates
30-
clients: []Issuer{client},
31+
keyIssuer: c,
32+
pubIssuer: c,
3133
serviceCertValidityDuration: serviceCertValidityDuration,
3234
msgBroker: msgBroker,
3335
}
@@ -36,19 +38,13 @@ func NewManager(mrcClient MRCClient, serviceCertValidityDuration time.Duration,
3638

3739
// Start takes an interval to check if the certificate
3840
// needs to be rotated
39-
func (m *Manager) Start(checkInterval time.Duration, certRotation <-chan struct{}) {
40-
// iterate over the list of certificates
41-
// when a cert needs to be rotated - call RotateCertificate()
42-
if certRotation == nil {
43-
log.Error().Msgf("Cannot start certificate rotation, certRotation is nil")
44-
return
45-
}
41+
func (m *Manager) Start(checkInterval time.Duration, stop <-chan struct{}) {
4642
ticker := time.NewTicker(checkInterval)
4743
go func() {
4844
m.checkAndRotate()
4945
for {
5046
select {
51-
case <-certRotation:
47+
case <-stop:
5248
ticker.Stop()
5349
return
5450
case <-ticker.C:
@@ -59,6 +55,9 @@ func (m *Manager) Start(checkInterval time.Duration, certRotation <-chan struct{
5955
}
6056

6157
func (m *Manager) checkAndRotate() {
58+
// NOTE: checkAndRotate can reintroduce a certificate that has been released, thereby creating an unbounded cache.
59+
// A certificate can also have been rotated already, leaving the list of issued certs stale, and we re-rotate.
60+
// the latter is not a bug, but a source of inefficiency.
6261
for _, cert := range m.ListIssuedCertificates() {
6362
shouldRotate := cert.ShouldRotate()
6463

@@ -70,15 +69,21 @@ func (m *Manager) checkAndRotate() {
7069
RenewBeforeCertExpires)
7170

7271
if shouldRotate {
73-
// Remove the certificate from the cache of the certificate manager
74-
newCert, err := m.RotateCertificate(cert.GetCommonName())
72+
newCert, err := m.IssueCertificate(cert.GetCommonName(), m.serviceCertValidityDuration)
7573
if err != nil {
7674
// TODO(#3962): metric might not be scraped before process restart resulting from this error
7775
log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrRotatingCert)).
7876
Msgf("Error rotating cert SerialNumber=%s", cert.GetSerialNumber())
7977
continue
8078
}
81-
log.Trace().Msgf("Rotated cert SerialNumber=%s", newCert.GetSerialNumber())
79+
80+
m.msgBroker.GetCertPubSub().Pub(events.PubSubMessage{
81+
Kind: announcements.CertificateRotated,
82+
NewObj: newCert,
83+
OldObj: cert,
84+
}, announcements.CertificateRotated.String())
85+
86+
log.Debug().Msgf("Rotated certificate (old SerialNumber=%s) with new SerialNumber=%s", cert.SerialNumber, newCert.SerialNumber)
8287
}
8388
}
8489
}
@@ -99,16 +104,31 @@ func (m *Manager) getFromCache(cn CommonName) *Certificate {
99104

100105
// IssueCertificate implements Manager and returns a newly issued certificate from the given client.
101106
func (m *Manager) IssueCertificate(cn CommonName, validityPeriod time.Duration) (*Certificate, error) {
107+
var err error
108+
cert := m.getFromCache(cn) // Don't call this while holding the lock
109+
110+
m.mu.RLock()
111+
pubIssuer := m.pubIssuer
112+
keyIssuer := m.keyIssuer
113+
m.mu.RUnlock()
114+
102115
start := time.Now()
116+
if cert == nil || cert.keyIssuerID != keyIssuer.ID || cert.pubIssuerID != pubIssuer.ID {
117+
cert, err = keyIssuer.IssueCertificate(cn, validityPeriod)
118+
if err != nil {
119+
return nil, err
120+
}
121+
if pubIssuer.ID != keyIssuer.ID {
122+
pubCert, err := pubIssuer.IssueCertificate(cn, validityPeriod)
123+
if err != nil {
124+
return nil, err
125+
}
103126

104-
if cert := m.getFromCache(cn); cert != nil {
105-
return cert, nil
106-
}
127+
cert = cert.newMergedWithRoot(pubCert.GetIssuingCA())
128+
}
107129

108-
// TODO(#4502): determine client(s) to use based on the rotation stage(s) of the client(s)
109-
cert, err := m.clients[0].IssueCertificate(cn, validityPeriod)
110-
if err != nil {
111-
return cert, err
130+
cert.keyIssuerID = keyIssuer.ID
131+
cert.pubIssuerID = pubIssuer.ID
112132
}
113133

114134
m.cache.Store(cn, cert)
@@ -124,38 +144,6 @@ func (m *Manager) ReleaseCertificate(cn CommonName) {
124144
m.cache.Delete(cn)
125145
}
126146

127-
// RotateCertificate implements Manager and rotates an existing
128-
func (m *Manager) RotateCertificate(cn CommonName) (*Certificate, error) {
129-
start := time.Now()
130-
131-
oldObj, ok := m.cache.Load(cn)
132-
if !ok {
133-
return nil, errors.Errorf("Old certificate does not exist for CN=%s", cn)
134-
}
135-
136-
oldCert, ok := oldObj.(*Certificate)
137-
if !ok {
138-
return nil, errors.Errorf("unexpected type %T for old certificate does not exist for CN=%s", oldCert, cn)
139-
}
140-
141-
newCert, err := m.IssueCertificate(cn, m.serviceCertValidityDuration)
142-
if err != nil {
143-
return nil, err
144-
}
145-
146-
m.cache.Store(cn, newCert)
147-
148-
m.msgBroker.GetCertPubSub().Pub(events.PubSubMessage{
149-
Kind: announcements.CertificateRotated,
150-
NewObj: newCert,
151-
OldObj: oldCert,
152-
}, announcements.CertificateRotated.String())
153-
154-
log.Debug().Msgf("Rotated certificate (old SerialNumber=%s) with new SerialNumber=%s took %+v", oldCert.SerialNumber, newCert.SerialNumber, time.Since(start))
155-
156-
return newCert, nil
157-
}
158-
159147
// ListIssuedCertificates implements CertificateDebugger interface and returns the list of issued certificates.
160148
func (m *Manager) ListIssuedCertificates() []*Certificate {
161149
var certs []*Certificate

0 commit comments

Comments
 (0)