Skip to content

Commit 42d01eb

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #836 from ankitathomas/OCP25341-4.13
[release-4.13] OCPBUGS-38254: [CARRY] perform operator apiService certificate validity checks directly
2 parents ebbc15f + c7ddc4f commit 42d01eb

File tree

22 files changed

+687
-372
lines changed

22 files changed

+687
-372
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.NewString()
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
@@ -2104,7 +2104,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
21042104
}
21052105

21062106
// Check if it's time to refresh owned APIService certs
2107-
if install.ShouldRotateCerts(out) {
2107+
if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
2108+
logger.WithError(err).Info("cert validity check")
2109+
return
2110+
} else if shouldRotate {
21082111
logger.Debug("CSV owns resources that require a cert refresh")
21092112
out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "CSV owns resources that require a cert refresh", now, a.recorder)
21102113
return
@@ -2214,7 +2217,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
22142217
}
22152218

22162219
// Check if it's time to refresh owned APIService certs
2217-
if install.ShouldRotateCerts(out) {
2220+
if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
2221+
logger.WithError(err).Info("cert validity check")
2222+
return
2223+
} else if shouldRotate {
22182224
logger.Debug("CSV owns resources that require a cert refresh")
22192225
out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "owned APIServices need cert refresh", now, a.recorder)
22202226
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
@@ -97,6 +97,10 @@ func (i *TestInstaller) CheckInstalled(s install.Strategy) (bool, error) {
9797
return true, nil
9898
}
9999

100+
func (i *TestInstaller) ShouldRotateCerts(s install.Strategy) (bool, error) {
101+
return false, nil
102+
}
103+
100104
func (i *TestInstaller) CertsRotateAt() time.Time {
101105
return time.Time{}
102106
}
@@ -489,6 +493,7 @@ func tlsSecret(name, namespace string, certPEM, privPEM []byte) *corev1.Secret {
489493
}
490494
secret.SetName(name)
491495
secret.SetNamespace(namespace)
496+
secret.SetLabels(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue})
492497

493498
return secret
494499
}
@@ -1880,26 +1885,26 @@ func TestTransitionCSV(t *testing.T) {
18801885
},
18811886
clientObjs: []runtime.Object{defaultOperatorGroup},
18821887
apis: []runtime.Object{
1883-
apiService("a1", "v1", "v1-a1", namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
1888+
apiService("a1", "v1", install.ServiceName("a1"), namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
18841889
},
18851890
objs: []runtime.Object{
18861891
deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
18871892
install.OLMCAHashAnnotationKey: expiredCAHash,
18881893
})),
1889-
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{
1894+
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{
18901895
install.OLMCAHashAnnotationKey: expiredCAHash,
18911896
}),
1892-
service("v1-a1", namespace, "a1", 80),
1897+
service(install.ServiceName("a1"), namespace, "a1", 80),
18931898
serviceAccount("sa", namespace),
1894-
role("v1.a1-cert", namespace, []rbacv1.PolicyRule{
1899+
role(install.SecretName(install.ServiceName("a1")), namespace, []rbacv1.PolicyRule{
18951900
{
18961901
Verbs: []string{"get"},
18971902
APIGroups: []string{""},
18981903
Resources: []string{"secrets"},
1899-
ResourceNames: []string{"v1.a1-cert"},
1904+
ResourceNames: []string{install.SecretName(install.ServiceName("a1"))},
19001905
},
19011906
}),
1902-
roleBinding("v1.a1-cert", namespace, "v1.a1-cert", "sa", namespace),
1907+
roleBinding(install.SecretName(install.ServiceName("a1")), namespace, install.SecretName(install.ServiceName("a1")), "sa", namespace),
19031908
role("extension-apiserver-authentication-reader", "kube-system", []rbacv1.PolicyRule{
19041909
{
19051910
Verbs: []string{"get"},
@@ -1908,7 +1913,7 @@ func TestTransitionCSV(t *testing.T) {
19081913
ResourceNames: []string{"extension-apiserver-authentication"},
19091914
},
19101915
}),
1911-
roleBinding("v1.a1-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
1916+
roleBinding(fmt.Sprintf("%s-auth-reader", install.ServiceName("a1")), "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
19121917
clusterRole("system:auth-delegator", []rbacv1.PolicyRule{
19131918
{
19141919
Verbs: []string{"create"},
@@ -1921,15 +1926,15 @@ func TestTransitionCSV(t *testing.T) {
19211926
Resources: []string{"subjectaccessreviews"},
19221927
},
19231928
}),
1924-
clusterRoleBinding("v1.a1-system:auth-delegator", "system:auth-delegator", "sa", namespace),
1929+
clusterRoleBinding(fmt.Sprintf("%s-system:auth-delegator", install.ServiceName("a1")), "system:auth-delegator", "sa", namespace),
19251930
},
19261931
crds: []runtime.Object{
19271932
crd("c1", "v1", "g1"),
19281933
},
19291934
},
19301935
expected: expected{
19311936
csvStates: map[string]csvState{
1932-
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed, reason: v1alpha1.CSVReasonAPIServiceResourceIssue},
1937+
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed, reason: v1alpha1.CSVReasonNeedsCertRotation},
19331938
},
19341939
},
19351940
},
@@ -1949,26 +1954,26 @@ func TestTransitionCSV(t *testing.T) {
19491954
},
19501955
clientObjs: []runtime.Object{defaultOperatorGroup},
19511956
apis: []runtime.Object{
1952-
apiService("a1", "v1", "v1-a1", namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
1957+
apiService("a1", "v1", install.ServiceName("a1"), namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
19531958
},
19541959
objs: []runtime.Object{
19551960
deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
19561961
install.OLMCAHashAnnotationKey: expiredCAHash,
19571962
})),
1958-
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{
1963+
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{
19591964
install.OLMCAHashAnnotationKey: expiredCAHash,
19601965
}),
1961-
service("v1-a1", namespace, "a1", 80),
1966+
service(install.ServiceName("a1"), namespace, "a1", 80),
19621967
serviceAccount("sa", namespace),
1963-
role("v1.a1-cert", namespace, []rbacv1.PolicyRule{
1968+
role(install.SecretName(install.ServiceName("a1")), namespace, []rbacv1.PolicyRule{
19641969
{
19651970
Verbs: []string{"get"},
19661971
APIGroups: []string{""},
19671972
Resources: []string{"secrets"},
1968-
ResourceNames: []string{"v1.a1-cert"},
1973+
ResourceNames: []string{install.SecretName(install.ServiceName("a1"))},
19691974
},
19701975
}),
1971-
roleBinding("v1.a1-cert", namespace, "v1.a1-cert", "sa", namespace),
1976+
roleBinding(install.SecretName(install.ServiceName("a1")), namespace, install.SecretName(install.ServiceName("a1")), "sa", namespace),
19721977
role("extension-apiserver-authentication-reader", "kube-system", []rbacv1.PolicyRule{
19731978
{
19741979
Verbs: []string{"get"},
@@ -1977,7 +1982,7 @@ func TestTransitionCSV(t *testing.T) {
19771982
ResourceNames: []string{"extension-apiserver-authentication"},
19781983
},
19791984
}),
1980-
roleBinding("v1.a1-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
1985+
roleBinding(fmt.Sprintf("%s-auth-reader", install.ServiceName("a1")), "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
19811986
clusterRole("system:auth-delegator", []rbacv1.PolicyRule{
19821987
{
19831988
Verbs: []string{"create"},
@@ -1990,15 +1995,15 @@ func TestTransitionCSV(t *testing.T) {
19901995
Resources: []string{"subjectaccessreviews"},
19911996
},
19921997
}),
1993-
clusterRoleBinding("v1.a1-system:auth-delegator", "system:auth-delegator", "sa", namespace),
1998+
clusterRoleBinding(fmt.Sprintf("%s-system:auth-delegator", install.ServiceName("a1")), "system:auth-delegator", "sa", namespace),
19941999
},
19952000
crds: []runtime.Object{
19962001
crd("c1", "v1", "g1"),
19972002
},
19982003
},
19992004
expected: expected{
20002005
csvStates: map[string]csvState{
2001-
"csv1": {exists: true, phase: v1alpha1.CSVPhasePending, reason: v1alpha1.CSVReasonAPIServiceResourcesNeedReinstall},
2006+
"csv1": {exists: true, phase: v1alpha1.CSVPhasePending, reason: v1alpha1.CSVReasonNeedsCertRotation},
20022007
},
20032008
},
20042009
},

‎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)