Skip to content

Commit 908da0c

Browse files
ankitathomasankitathomas
authored
[OCPBUGS-25341]: perform operator apiService certificate validity checks directly (#3217)
* perform operator apiService certificate validity checks directly Signed-off-by: Ankita Thomas <ankithom@redhat.com> * use sets to track certs to install, revert to checking for installPlan timeouts after API availability checks, add service FQDN to list of hostnames. Signed-off-by: Ankita Thomas <ankithom@redhat.com> --------- Signed-off-by: Ankita Thomas <ankithom@redhat.com>
1 parent 20a3cbc commit 908da0c

8 files changed

Lines changed: 284 additions & 64 deletions

File tree

‎pkg/controller/install/certresources.go‎

Lines changed: 73 additions & 8 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
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
1617
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
1718

@@ -157,6 +158,14 @@ func ServiceName(deploymentName string) string {
157158
return deploymentName + "-service"
158159
}
159160

161+
func HostnamesForService(serviceName, namespace string) []string {
162+
return []string{
163+
fmt.Sprintf("%s.%s", serviceName, namespace),
164+
fmt.Sprintf("%s.%s.svc", serviceName, namespace),
165+
fmt.Sprintf("%s.%s.svc.cluster.local", serviceName, namespace),
166+
}
167+
}
168+
160169
func (i *StrategyDeploymentInstaller) getCertResources() []certResource {
161170
return append(i.apiServiceDescriptions, i.webhookDescriptions...)
162171
}
@@ -223,15 +232,74 @@ func (i *StrategyDeploymentInstaller) CertsRotated() bool {
223232
return i.certificatesRotated
224233
}
225234

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

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

276+
func (i *StrategyDeploymentInstaller) ShouldRotateCerts(s Strategy) (bool, error) {
277+
strategy, ok := s.(*v1alpha1.StrategyDetailsDeployment)
278+
if !ok {
279+
return false, fmt.Errorf("failed to install %s strategy with deployment installer: unsupported deployment install strategy", strategy.GetStrategyName())
280+
}
281+
282+
hasCerts := sets.New[string]()
283+
for _, c := range i.getCertResources() {
284+
hasCerts.Insert(c.getDeploymentName())
285+
}
286+
for _, sddSpec := range strategy.DeploymentSpecs {
287+
if hasCerts.Has(sddSpec.Name) {
288+
certSecret, err := i.strategyClient.GetOpLister().CoreV1().SecretLister().Secrets(i.owner.GetNamespace()).Get(SecretName(ServiceName(sddSpec.Name)))
289+
if err == nil {
290+
if shouldRotateCerts(certSecret, HostnamesForService(ServiceName(sddSpec.Name), i.owner.GetNamespace())) {
291+
return true, nil
292+
}
293+
} else if apierrors.IsNotFound(err) {
294+
return true, nil
295+
} else {
296+
return false, err
297+
}
298+
}
299+
}
300+
return false, nil
301+
}
302+
235303
func CalculateCertExpiration(startingFrom time.Time) time.Time {
236304
return startingFrom.Add(DefaultCertValidFor)
237305
}
@@ -267,10 +335,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
267335
}
268336

269337
// Create signed serving cert
270-
hosts := []string{
271-
fmt.Sprintf("%s.%s", serviceName, i.owner.GetNamespace()),
272-
fmt.Sprintf("%s.%s.svc", serviceName, i.owner.GetNamespace()),
273-
}
338+
hosts := HostnamesForService(serviceName, i.owner.GetNamespace())
274339
servingPair, err := certGenerator.Generate(expiration, Organization, ca, hosts)
275340
if err != nil {
276341
logger.Warnf("could not generate signed certs for hosts %v", hosts)
@@ -314,10 +379,10 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo
314379

315380
// Attempt an update
316381
// TODO: Check that the secret was not modified
317-
if existingCAPEM, ok := existingSecret.Data[OLMCAPEMKey]; ok && !ShouldRotateCerts(i.owner.(*v1alpha1.ClusterServiceVersion)) {
382+
if !shouldRotateCerts(existingSecret, HostnamesForService(serviceName, i.owner.GetNamespace())) {
318383
logger.Warnf("reusing existing cert %s", secret.GetName())
319384
secret = existingSecret
320-
caPEM = existingCAPEM
385+
caPEM = existingSecret.Data[OLMCAPEMKey]
321386
caHash = certs.PEMSHA256(caPEM)
322387
} else {
323388
if _, err := i.strategyClient.GetOpClient().UpdateSecret(secret); err != nil {

‎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+
}

‎pkg/controller/operators/olm/apiservices.go‎

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -93,17 +93,12 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
9393

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

108103
// Check if serving cert is active
109104
secretName := install.SecretName(serviceName)
@@ -113,17 +108,12 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion,
113108
errs = append(errs, err)
114109
continue
115110
}
116-
cert, err := certs.PEMToCert(secret.Data["tls.crt"])
111+
_, err = certs.PEMToCert(secret.Data["tls.crt"])
117112
if err != nil {
118113
logger.Warnf("could not convert serving cert to x509 cert")
119114
errs = append(errs, err)
120115
continue
121116
}
122-
if !certs.Active(cert) {
123-
logger.Warnf("serving cert not active")
124-
errs = append(errs, fmt.Errorf("found the serving cert not active"))
125-
continue
126-
}
127117

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

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

‎pkg/controller/operators/olm/operator.go‎

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

22442244
// Check if it's time to refresh owned APIService certs
2245-
if install.ShouldRotateCerts(out) {
2245+
if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
2246+
logger.WithError(err).Info("cert validity check")
2247+
return
2248+
} else if shouldRotate {
22462249
logger.Debug("CSV owns resources that require a cert refresh")
22472250
out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "CSV owns resources that require a cert refresh", now, a.recorder)
22482251
return
@@ -2352,7 +2355,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
23522355
}
23532356

23542357
// Check if it's time to refresh owned APIService certs
2355-
if install.ShouldRotateCerts(out) {
2358+
if shouldRotate, err := installer.ShouldRotateCerts(strategy); err != nil {
2359+
logger.WithError(err).Info("cert validity check")
2360+
return
2361+
} else if shouldRotate {
23562362
logger.Debug("CSV owns resources that require a cert refresh")
23572363
out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsCertRotation, "owned APIServices need cert refresh", now, a.recorder)
23582364
return

‎pkg/controller/operators/olm/operator_test.go‎

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

104+
func (i *TestInstaller) ShouldRotateCerts(s install.Strategy) (bool, error) {
105+
return false, nil
106+
}
107+
104108
func (i *TestInstaller) CertsRotateAt() time.Time {
105109
return time.Time{}
106110
}
@@ -1956,26 +1960,26 @@ func TestTransitionCSV(t *testing.T) {
19561960
},
19571961
clientObjs: []runtime.Object{defaultOperatorGroup},
19581962
apis: []runtime.Object{
1959-
apiService("a1", "v1", "v1-a1", namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
1963+
apiService("a1", "v1", install.ServiceName("a1"), namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
19601964
},
19611965
objs: []runtime.Object{
19621966
deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
19631967
install.OLMCAHashAnnotationKey: expiredCAHash,
19641968
})),
1965-
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{
1969+
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{
19661970
install.OLMCAHashAnnotationKey: expiredCAHash,
19671971
}),
1968-
service("v1-a1", namespace, "a1", 80),
1972+
service(install.ServiceName("a1"), namespace, "a1", 80),
19691973
serviceAccount("sa", namespace),
1970-
role("v1.a1-cert", namespace, []rbacv1.PolicyRule{
1974+
role(install.SecretName(install.ServiceName("a1")), namespace, []rbacv1.PolicyRule{
19711975
{
19721976
Verbs: []string{"get"},
19731977
APIGroups: []string{""},
19741978
Resources: []string{"secrets"},
1975-
ResourceNames: []string{"v1.a1-cert"},
1979+
ResourceNames: []string{install.SecretName(install.ServiceName("a1"))},
19761980
},
19771981
}),
1978-
roleBinding("v1.a1-cert", namespace, "v1.a1-cert", "sa", namespace),
1982+
roleBinding(install.SecretName(install.ServiceName("a1")), namespace, install.SecretName(install.ServiceName("a1")), "sa", namespace),
19791983
role("extension-apiserver-authentication-reader", "kube-system", []rbacv1.PolicyRule{
19801984
{
19811985
Verbs: []string{"get"},
@@ -1984,7 +1988,7 @@ func TestTransitionCSV(t *testing.T) {
19841988
ResourceNames: []string{"extension-apiserver-authentication"},
19851989
},
19861990
}),
1987-
roleBinding("v1.a1-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
1991+
roleBinding(install.AuthReaderRoleBindingName(install.ServiceName("a1")), "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
19881992
clusterRole("system:auth-delegator", []rbacv1.PolicyRule{
19891993
{
19901994
Verbs: []string{"create"},
@@ -1997,15 +2001,15 @@ func TestTransitionCSV(t *testing.T) {
19972001
Resources: []string{"subjectaccessreviews"},
19982002
},
19992003
}),
2000-
clusterRoleBinding("v1.a1-system:auth-delegator", "system:auth-delegator", "sa", namespace),
2004+
clusterRoleBinding(install.AuthDelegatorClusterRoleBindingName(install.ServiceName("a1")), "system:auth-delegator", "sa", namespace),
20012005
},
20022006
crds: []runtime.Object{
20032007
crd("c1", "v1", "g1"),
20042008
},
20052009
},
20062010
expected: expected{
20072011
csvStates: map[string]csvState{
2008-
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed, reason: v1alpha1.CSVReasonAPIServiceResourceIssue},
2012+
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed, reason: v1alpha1.CSVReasonNeedsCertRotation},
20092013
},
20102014
},
20112015
},
@@ -2025,26 +2029,26 @@ func TestTransitionCSV(t *testing.T) {
20252029
},
20262030
clientObjs: []runtime.Object{defaultOperatorGroup},
20272031
apis: []runtime.Object{
2028-
apiService("a1", "v1", "v1-a1", namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
2032+
apiService("a1", "v1", install.ServiceName("a1"), namespace, "a1", expiredCAPEM, apiregistrationv1.ConditionTrue, ownerLabelFromCSV("csv1", namespace)),
20292033
},
20302034
objs: []runtime.Object{
20312035
deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
20322036
install.OLMCAHashAnnotationKey: expiredCAHash,
20332037
})),
2034-
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{
2038+
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{
20352039
install.OLMCAHashAnnotationKey: expiredCAHash,
20362040
}),
2037-
service("v1-a1", namespace, "a1", 80),
2041+
service(install.ServiceName("a1"), namespace, "a1", 80),
20382042
serviceAccount("sa", namespace),
2039-
role("v1.a1-cert", namespace, []rbacv1.PolicyRule{
2043+
role(install.SecretName(install.ServiceName("a1")), namespace, []rbacv1.PolicyRule{
20402044
{
20412045
Verbs: []string{"get"},
20422046
APIGroups: []string{""},
20432047
Resources: []string{"secrets"},
2044-
ResourceNames: []string{"v1.a1-cert"},
2048+
ResourceNames: []string{install.SecretName(install.ServiceName("a1"))},
20452049
},
20462050
}),
2047-
roleBinding("v1.a1-cert", namespace, "v1.a1-cert", "sa", namespace),
2051+
roleBinding(install.SecretName(install.ServiceName("a1")), namespace, install.SecretName(install.ServiceName("a1")), "sa", namespace),
20482052
role("extension-apiserver-authentication-reader", "kube-system", []rbacv1.PolicyRule{
20492053
{
20502054
Verbs: []string{"get"},
@@ -2053,7 +2057,7 @@ func TestTransitionCSV(t *testing.T) {
20532057
ResourceNames: []string{"extension-apiserver-authentication"},
20542058
},
20552059
}),
2056-
roleBinding("v1.a1-auth-reader", "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
2060+
roleBinding(install.AuthReaderRoleBindingName(install.ServiceName("a1")), "kube-system", "extension-apiserver-authentication-reader", "sa", namespace),
20572061
clusterRole("system:auth-delegator", []rbacv1.PolicyRule{
20582062
{
20592063
Verbs: []string{"create"},
@@ -2066,15 +2070,15 @@ func TestTransitionCSV(t *testing.T) {
20662070
Resources: []string{"subjectaccessreviews"},
20672071
},
20682072
}),
2069-
clusterRoleBinding("v1.a1-system:auth-delegator", "system:auth-delegator", "sa", namespace),
2073+
clusterRoleBinding(install.AuthDelegatorClusterRoleBindingName(install.ServiceName("a1")), "system:auth-delegator", "sa", namespace),
20702074
},
20712075
crds: []runtime.Object{
20722076
crd("c1", "v1", "g1"),
20732077
},
20742078
},
20752079
expected: expected{
20762080
csvStates: map[string]csvState{
2077-
"csv1": {exists: true, phase: v1alpha1.CSVPhasePending, reason: v1alpha1.CSVReasonAPIServiceResourcesNeedReinstall},
2081+
"csv1": {exists: true, phase: v1alpha1.CSVPhasePending, reason: v1alpha1.CSVReasonNeedsCertRotation},
20782082
},
20792083
},
20802084
},

0 commit comments

Comments
 (0)