Skip to content

Commit b1d51eb

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request openshift#821 from ankitathomas/OCP25341-4.14
[release-4.14] OCPBUGS-36949: [CARRY] perform operator apiService certificate validity checks directly
2 parents 8d9b39e + 83af49d commit b1d51eb

File tree

21 files changed

+614
-361
lines changed

21 files changed

+614
-361
lines changed

‎staging/operator-lifecycle-manager/pkg/controller/install/certresources.go

Lines changed: 75 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
apierrors "k8s.io/apimachinery/pkg/api/errors"
1313
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1414
"k8s.io/apimachinery/pkg/util/intstr"
15+
"k8s.io/apimachinery/pkg/util/sets"
1516

1617
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1718
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs"
@@ -155,6 +156,14 @@ func ServiceName(deploymentName string) string {
155156
return deploymentName + "-service"
156157
}
157158

159+
func HostnamesForService(serviceName, namespace string) []string {
160+
return []string{
161+
fmt.Sprintf("%s.%s", serviceName, namespace),
162+
fmt.Sprintf("%s.%s.svc", serviceName, namespace),
163+
fmt.Sprintf("%s.%s.svc.cluster.local", serviceName, namespace),
164+
}
165+
}
166+
158167
func (i *StrategyDeploymentInstaller) getCertResources() []certResource {
159168
return append(i.apiServiceDescriptions, i.webhookDescriptions...)
160169
}
@@ -221,15 +230,74 @@ func (i *StrategyDeploymentInstaller) CertsRotated() bool {
221230
return i.certificatesRotated
222231
}
223232

224-
func ShouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
233+
// shouldRotateCerts indicates whether an apiService cert should be rotated due to being
234+
// malformed, invalid, expired, inactive or within a specific freshness interval (DefaultCertMinFresh) before expiry.
235+
func shouldRotateCerts(certSecret *corev1.Secret, hosts []string) bool {
225236
now := metav1.Now()
226-
if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
237+
caPEM, ok := certSecret.Data[OLMCAPEMKey]
238+
if !ok {
239+
// missing CA cert in secret
240+
return true
241+
}
242+
certPEM, ok := certSecret.Data["tls.crt"]
243+
if !ok {
244+
// missing cert in secret
245+
return true
246+
}
247+
248+
ca, err := certs.PEMToCert(caPEM)
249+
if err != nil {
250+
// malformed CA cert
251+
return true
252+
}
253+
cert, err := certs.PEMToCert(certPEM)
254+
if err != nil {
255+
// malformed cert
227256
return true
228257
}
229258

259+
// check for cert freshness
260+
if !certs.Active(ca) || now.Time.After(CalculateCertRotatesAt(ca.NotAfter)) ||
261+
!certs.Active(cert) || now.Time.After(CalculateCertRotatesAt(cert.NotAfter)) {
262+
return true
263+
}
264+
265+
// Check validity of serving cert and if serving cert is trusted by the CA
266+
for _, host := range hosts {
267+
if err := certs.VerifyCert(ca, cert, host); err != nil {
268+
return true
269+
}
270+
}
230271
return false
231272
}
232273

274+
func (i *StrategyDeploymentInstaller) ShouldRotateCerts(s Strategy) (bool, error) {
275+
strategy, ok := s.(*v1alpha1.StrategyDetailsDeployment)
276+
if !ok {
277+
return false, fmt.Errorf("failed to install %s strategy with deployment installer: unsupported deployment install strategy", strategy.GetStrategyName())
278+
}
279+
280+
hasCerts := sets.New[string]()
281+
for _, c := range i.getCertResources() {
282+
hasCerts.Insert(c.getDeploymentName())
283+
}
284+
for _, sddSpec := range strategy.DeploymentSpecs {
285+
if hasCerts.Has(sddSpec.Name) {
286+
certSecret, err := i.strategyClient.GetOpLister().CoreV1().SecretLister().Secrets(i.owner.GetNamespace()).Get(SecretName(ServiceName(sddSpec.Name)))
287+
if err == nil {
288+
if shouldRotateCerts(certSecret, HostnamesForService(ServiceName(sddSpec.Name), i.owner.GetNamespace())) {
289+
return true, nil
290+
}
291+
} else if apierrors.IsNotFound(err) {
292+
return true, nil
293+
} else {
294+
return false, err
295+
}
296+
}
297+
}
298+
return false, nil
299+
}
300+
233301
func CalculateCertExpiration(startingFrom time.Time) time.Time {
234302
return startingFrom.Add(DefaultCertValidFor)
235303
}
@@ -248,7 +316,8 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
248316
Selector: depSpec.Selector.MatchLabels,
249317
},
250318
}
251-
service.SetName(ServiceName(deploymentName))
319+
serviceName := ServiceName(deploymentName)
320+
service.SetName(serviceName)
252321
service.SetNamespace(i.owner.GetNamespace())
253322
ownerutil.AddNonBlockingOwner(service, i.owner)
254323

@@ -274,10 +343,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
274343
}
275344

276345
// Create signed serving cert
277-
hosts := []string{
278-
fmt.Sprintf("%s.%s", service.GetName(), i.owner.GetNamespace()),
279-
fmt.Sprintf("%s.%s.svc", service.GetName(), i.owner.GetNamespace()),
280-
}
346+
hosts := HostnamesForService(serviceName, i.owner.GetNamespace())
281347
servingPair, err := certGenerator.Generate(expiration, Organization, ca, hosts)
282348
if err != nil {
283349
logger.Warnf("could not generate signed certs for hosts %v", hosts)
@@ -321,10 +387,10 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
321387

322388
// Attempt an update
323389
// TODO: Check that the secret was not modified
324-
if existingCAPEM, ok := existingSecret.Data[OLMCAPEMKey]; ok && !ShouldRotateCerts(i.owner.(*v1alpha1.ClusterServiceVersion)) {
390+
if !shouldRotateCerts(existingSecret, HostnamesForService(serviceName, i.owner.GetNamespace())) {
325391
logger.Warnf("reusing existing cert %s", secret.GetName())
326392
secret = existingSecret
327-
caPEM = existingCAPEM
393+
caPEM = existingSecret.Data[OLMCAPEMKey]
328394
caHash = certs.PEMSHA256(caPEM)
329395
} else {
330396
if _, err := i.strategyClient.GetOpClient().UpdateSecret(secret); err != nil {

‎staging/operator-lifecycle-manager/pkg/controller/install/resolver.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ type Strategy interface {
2121
type StrategyInstaller interface {
2222
Install(strategy Strategy) error
2323
CheckInstalled(strategy Strategy) (bool, error)
24+
ShouldRotateCerts(strategy Strategy) (bool, error)
2425
CertsRotateAt() time.Time
2526
CertsRotated() bool
2627
}
@@ -79,3 +80,7 @@ func (i *NullStrategyInstaller) CertsRotateAt() time.Time {
7980
func (i *NullStrategyInstaller) CertsRotated() bool {
8081
return false
8182
}
83+
84+
func (i *NullStrategyInstaller) ShouldRotateCerts(s Strategy) (bool, error) {
85+
return false, nil
86+
}

‎staging/operator-lifecycle-manager/pkg/controller/operators/olm/apiservices.go

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
7878
}
7979

8080
serviceName := install.ServiceName(desc.DeploymentName)
81-
service, err := a.lister.CoreV1().ServiceLister().Services(csv.GetNamespace()).Get(serviceName)
81+
_, err = a.lister.CoreV1().ServiceLister().Services(csv.GetNamespace()).Get(serviceName)
8282
if err != nil {
8383
logger.WithField("service", serviceName).Warnf("could not retrieve generated Service")
8484
errs = append(errs, err)
@@ -94,17 +94,12 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
9494

9595
// Check if CA is Active
9696
caBundle := apiService.Spec.CABundle
97-
ca, err := certs.PEMToCert(caBundle)
97+
_, err = certs.PEMToCert(caBundle)
9898
if err != nil {
9999
logger.Warnf("could not convert APIService CA bundle to x509 cert")
100100
errs = append(errs, err)
101101
continue
102102
}
103-
if !certs.Active(ca) {
104-
logger.Warnf("CA cert not active")
105-
errs = append(errs, fmt.Errorf("found the CA cert is not active"))
106-
continue
107-
}
108103

109104
// Check if serving cert is active
110105
secretName := install.SecretName(serviceName)
@@ -114,17 +109,12 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
114109
errs = append(errs, err)
115110
continue
116111
}
117-
cert, err := certs.PEMToCert(secret.Data["tls.crt"])
112+
_, err = certs.PEMToCert(secret.Data["tls.crt"])
118113
if err != nil {
119114
logger.Warnf("could not convert serving cert to x509 cert")
120115
errs = append(errs, err)
121116
continue
122117
}
123-
if !certs.Active(cert) {
124-
logger.Warnf("serving cert not active")
125-
errs = append(errs, fmt.Errorf("found the serving cert not active"))
126-
continue
127-
}
128118

129119
// Check if CA hash matches expected
130120
caHash := hashFunc(caBundle)
@@ -134,18 +124,6 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
134124
continue
135125
}
136126

137-
// Check if serving cert is trusted by the CA
138-
hosts := []string{
139-
fmt.Sprintf("%s.%s", service.GetName(), csv.GetNamespace()),
140-
fmt.Sprintf("%s.%s.svc", service.GetName(), csv.GetNamespace()),
141-
}
142-
for _, host := range hosts {
143-
if err := certs.VerifyCert(ca, cert, host); err != nil {
144-
errs = append(errs, fmt.Errorf("could not verify cert: %s", err.Error()))
145-
continue
146-
}
147-
}
148-
149127
// Ensure the existing Deployment has a matching CA hash annotation
150128
deployment, err := a.lister.AppsV1().DeploymentLister().Deployments(csv.GetNamespace()).Get(desc.DeploymentName)
151129
if apierrors.IsNotFound(err) || err != nil {

‎staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2108,7 +2108,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
21082108
}
21092109

21102110
// Check if it's time to refresh owned APIService certs
2111-
if install.ShouldRotateCerts(out) {
2111+
if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
2112+
logger.WithError(err).Info("cert validity check")
2113+
return
2114+
} else if shouldRotate {
21122115
logger.Debug("CSV owns resources that require a cert refresh")
21132116
out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "CSV owns resources that require a cert refresh", now, a.recorder)
21142117
return
@@ -2218,7 +2221,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
22182221
}
22192222

22202223
// Check if it's time to refresh owned APIService certs
2221-
if install.ShouldRotateCerts(out) {
2224+
if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
2225+
logger.WithError(err).Info("cert validity check")
2226+
return
2227+
} else if shouldRotate {
22222228
logger.Debug("CSV owns resources that require a cert refresh")
22232229
out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "owned APIServices need cert refresh", now, a.recorder)
22242230
return

‎staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ func (i *TestInstaller) CheckInstalled(s install.Strategy) (bool, error) {
100100
return true, nil
101101
}
102102

103+
func (i *TestInstaller) ShouldRotateCerts(s install.Strategy) (bool, error) {
104+
return false, nil
105+
}
106+
103107
func (i *TestInstaller) CertsRotateAt() time.Time {
104108
return time.Time{}
105109
}
@@ -528,6 +532,7 @@ func tlsSecret(name, namespace string, certPEM, privPEM []byte) *corev1.Secret {
528532
}
529533
secret.SetName(name)
530534
secret.SetNamespace(namespace)
535+
secret.SetLabels(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue})
531536

532537
return secret
533538
}
@@ -1919,26 +1924,26 @@ func TestTransitionCSV(t *testing.T) {
19191924
},
19201925
clientObjs: []runtime.Object{defaultOperatorGroup},
19211926
apis: []runtime.Object{
1922-
apiService("a1", "v1", "v1-a1", namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
1927+
apiService("a1", "v1", install.ServiceName("a1"), namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
19231928
},
19241929
objs: []runtime.Object{
19251930
deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
19261931
install.OLMCAHashAnnotationKey: expiredCAHash,
19271932
})),
1928-
withAnnotations(keyPairToTLSSecret("v1.a1-cert", namespace, signedServingPair(time.Now().Add(24*time.Hour), expiredCA, []string{"v1-a1.ns", "v1-a1.ns.svc"})), map[string]string{
1933+
withAnnotations(keyPairToTLSSecret(install.SecretName(install.ServiceName("a1")), namespace, signedServingPair(time.Now().Add(24*time.Hour), expiredCA, install.HostnamesForService(install.ServiceName("a1"), "ns"))), map[string]string{
19291934
install.OLMCAHashAnnotationKey: expiredCAHash,
19301935
}),
1931-
service("v1-a1", namespace, "a1", 80),
1936+
service(install.ServiceName("a1"), namespace, "a1", 80),
19321937
serviceAccount("sa", namespace),
1933-
role("v1.a1-cert", namespace, []rbacv1.PolicyRule{
1938+
role(install.SecretName(install.ServiceName("a1")), namespace, []rbacv1.PolicyRule{
19341939
{
19351940
Verbs: []string{"get"},
19361941
APIGroups: []string{""},
19371942
Resources: []string{"secrets"},
1938-
ResourceNames: []string{"v1.a1-cert"},
1943+
ResourceNames: []string{install.SecretName(install.ServiceName("a1"))},
19391944
},
19401945
}),
1941-
roleBinding("v1.a1-cert", namespace, "v1.a1-cert", "sa", namespace),
1946+
roleBinding(install.SecretName(install.ServiceName("a1")), namespace, install.SecretName(install.ServiceName("a1")), "sa", namespace),
19421947
role("extension-apiserver-authentication-reader", "kube-system", []rbacv1.PolicyRule{
19431948
{
19441949
Verbs: []string{"get"},
@@ -1947,7 +1952,7 @@ func TestTransitionCSV(t *testing.T) {
19471952
ResourceNames: []string{"extension-apiserver-authentication"},
19481953
},
19491954
}),
1950-
roleBinding("v1.a1-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
1955+
roleBinding(fmt.Sprintf("%s-auth-reader", install.ServiceName("a1")), "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
19511956
clusterRole("system:auth-delegator", []rbacv1.PolicyRule{
19521957
{
19531958
Verbs: []string{"create"},
@@ -1960,15 +1965,15 @@ func TestTransitionCSV(t *testing.T) {
19601965
Resources: []string{"subjectaccessreviews"},
19611966
},
19621967
}),
1963-
clusterRoleBinding("v1.a1-system:auth-delegator", "system:auth-delegator", "sa", namespace),
1968+
clusterRoleBinding(fmt.Sprintf("%s-system:auth-delegator", install.ServiceName("a1")), "system:auth-delegator", "sa", namespace),
19641969
},
19651970
crds: []runtime.Object{
19661971
crd("c1", "v1", "g1"),
19671972
},
19681973
},
19691974
expected: expected{
19701975
csvStates: map[string]csvState{
1971-
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed, reason: v1alpha1.CSVReasonAPIServiceResourceIssue},
1976+
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed, reason: v1alpha1.CSVReasonNeedsCertRotation},
19721977
},
19731978
},
19741979
},
@@ -1988,26 +1993,26 @@ func TestTransitionCSV(t *testing.T) {
19881993
},
19891994
clientObjs: []runtime.Object{defaultOperatorGroup},
19901995
apis: []runtime.Object{
1991-
apiService("a1", "v1", "v1-a1", namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
1996+
apiService("a1", "v1", install.ServiceName("a1"), namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
19921997
},
19931998
objs: []runtime.Object{
19941999
deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
19952000
install.OLMCAHashAnnotationKey: expiredCAHash,
19962001
})),
1997-
withAnnotations(keyPairToTLSSecret("v1.a1-cert", namespace, signedServingPair(time.Now().Add(24*time.Hour), expiredCA, []string{"v1-a1.ns", "v1-a1.ns.svc"})), map[string]string{
2002+
withAnnotations(keyPairToTLSSecret(install.SecretName(install.ServiceName("a1")), namespace, signedServingPair(time.Now().Add(24*time.Hour), expiredCA, install.HostnamesForService(install.ServiceName("a1"), "ns"))), map[string]string{
19982003
install.OLMCAHashAnnotationKey: expiredCAHash,
19992004
}),
2000-
service("v1-a1", namespace, "a1", 80),
2005+
service(install.ServiceName("a1"), namespace, "a1", 80),
20012006
serviceAccount("sa", namespace),
2002-
role("v1.a1-cert", namespace, []rbacv1.PolicyRule{
2007+
role(install.SecretName(install.ServiceName("a1")), namespace, []rbacv1.PolicyRule{
20032008
{
20042009
Verbs: []string{"get"},
20052010
APIGroups: []string{""},
20062011
Resources: []string{"secrets"},
2007-
ResourceNames: []string{"v1.a1-cert"},
2012+
ResourceNames: []string{install.SecretName(install.ServiceName("a1"))},
20082013
},
20092014
}),
2010-
roleBinding("v1.a1-cert", namespace, "v1.a1-cert", "sa", namespace),
2015+
roleBinding(install.SecretName(install.ServiceName("a1")), namespace, install.SecretName(install.ServiceName("a1")), "sa", namespace),
20112016
role("extension-apiserver-authentication-reader", "kube-system", []rbacv1.PolicyRule{
20122017
{
20132018
Verbs: []string{"get"},
@@ -2016,7 +2021,7 @@ func TestTransitionCSV(t *testing.T) {
20162021
ResourceNames: []string{"extension-apiserver-authentication"},
20172022
},
20182023
}),
2019-
roleBinding("v1.a1-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
2024+
roleBinding(fmt.Sprintf("%s-auth-reader", install.ServiceName("a1")), "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
20202025
clusterRole("system:auth-delegator", []rbacv1.PolicyRule{
20212026
{
20222027
Verbs: []string{"create"},
@@ -2029,15 +2034,15 @@ func TestTransitionCSV(t *testing.T) {
20292034
Resources: []string{"subjectaccessreviews"},
20302035
},
20312036
}),
2032-
clusterRoleBinding("v1.a1-system:auth-delegator", "system:auth-delegator", "sa", namespace),
2037+
clusterRoleBinding(fmt.Sprintf("%s-system:auth-delegator", install.ServiceName("a1")), "system:auth-delegator", "sa", namespace),
20332038
},
20342039
crds: []runtime.Object{
20352040
crd("c1", "v1", "g1"),
20362041
},
20372042
},
20382043
expected: expected{
20392044
csvStates: map[string]csvState{
2040-
"csv1": {exists: true, phase: v1alpha1.CSVPhasePending, reason: v1alpha1.CSVReasonAPIServiceResourcesNeedReinstall},
2045+
"csv1": {exists: true, phase: v1alpha1.CSVPhasePending, reason: v1alpha1.CSVReasonNeedsCertRotation},
20412046
},
20422047
},
20432048
},

‎staging/operator-lifecycle-manager/pkg/controller/operators/openshift/suite_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,5 +108,5 @@ var _ = BeforeSuite(func() {
108108
var _ = AfterSuite(func() {
109109
By("tearing down the test environment")
110110
close(syncCh)
111-
Expect(testEnv.Stop()).To(Succeed())
111+
testEnv.Stop()
112112
})

‎staging/operator-lifecycle-manager/pkg/controller/operators/suite_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,7 @@ var _ = AfterSuite(func() {
162162
ctx.Done()
163163

164164
By("tearing down the test environment")
165-
err := testEnv.Stop()
166-
Expect(err).ToNot(HaveOccurred())
165+
testEnv.Stop()
167166
})
168167

169168
func newOperator(name string) *decorators.Operator {

0 commit comments

Comments
 (0)