From e2fca99c054a664d06d1f5a762d08c7557c619c5 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Mon, 10 Feb 2020 17:29:32 -0600 Subject: [PATCH 1/4] Correct ES cert rotation --- cmd/manager/main.go | 2 ++ pkg/controller/elasticsearch/certificates/ca_reconcile.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index cf515c446a..1d7be4ba60 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -266,7 +266,9 @@ func execute() { // Verify cert validity options caCertValidity, caCertRotateBefore := ValidateCertExpirationFlags(operator.CACertValidityFlag, operator.CACertRotateBeforeFlag) + log.V(1).Info("Using certificate authority rotation parameters", operator.CACertValidityFlag, caCertValidity, operator.CACertRotateBeforeFlag, caCertRotateBefore) certValidity, certRotateBefore := ValidateCertExpirationFlags(operator.CertValidityFlag, operator.CertRotateBeforeFlag) + log.V(1).Info("Using certificate rotation parameters", operator.CertValidityFlag, certValidity, operator.CertRotateBeforeFlag, certRotateBefore) // Setup all Controllers roles := viper.GetStringSlice(operator.OperatorRolesFlag) diff --git a/pkg/controller/elasticsearch/certificates/ca_reconcile.go b/pkg/controller/elasticsearch/certificates/ca_reconcile.go index e772f1523e..d219085ece 100644 --- a/pkg/controller/elasticsearch/certificates/ca_reconcile.go +++ b/pkg/controller/elasticsearch/certificates/ca_reconcile.go @@ -77,7 +77,7 @@ func Reconcile( es.Spec.HTTP.TLS, labels, services, - caRotation, + certRotation, ) if err != nil { return nil, results.WithError(err) From 25deaf94cbe8e8c40afd178d1afa4c025b9375cb Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Mon, 10 Feb 2020 17:39:44 -0600 Subject: [PATCH 2/4] Increase CA issuing time skew allowance --- pkg/controller/apmserver/apmserver_controller.go | 2 +- pkg/controller/apmserver/certificates/reconcile.go | 9 +++++---- pkg/controller/common/certificates/ca.go | 2 +- pkg/controller/kibana/certificates/reconcile.go | 9 +++++---- pkg/controller/kibana/driver.go | 2 +- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/controller/apmserver/apmserver_controller.go b/pkg/controller/apmserver/apmserver_controller.go index 15ef67f416..f85db22eee 100644 --- a/pkg/controller/apmserver/apmserver_controller.go +++ b/pkg/controller/apmserver/apmserver_controller.go @@ -241,7 +241,7 @@ func (r *ReconcileApmServer) doReconcile(ctx context.Context, request reconcile. if err != nil { return reconcile.Result{}, err } - results := apmcerts.Reconcile(ctx, r, as, []corev1.Service{*svc}, r.CACertRotation) + results := apmcerts.Reconcile(ctx, r, as, []corev1.Service{*svc}, r.CACertRotation, r.CertRotation) if results.HasError() { res, err := results.Aggregate() k8s.EmitErrorEvent(r.recorder, err, as, events.EventReconciliationError, "Certificate reconciliation error: %v", err) diff --git a/pkg/controller/apmserver/certificates/reconcile.go b/pkg/controller/apmserver/certificates/reconcile.go index 5ae3cc0864..bc41ff58ca 100644 --- a/pkg/controller/apmserver/certificates/reconcile.go +++ b/pkg/controller/apmserver/certificates/reconcile.go @@ -27,7 +27,8 @@ func Reconcile( driver driver.Interface, as *apmv1.ApmServer, services []corev1.Service, - rotation certificates.RotationParams, + caRotation certificates.RotationParams, + certRotation certificates.RotationParams, ) *reconciler.Results { span, _ := apm.StartSpan(ctx, "reconcile_certs", tracing.SpanTypeApp) defer span.End() @@ -48,7 +49,7 @@ func Reconcile( as, labels, certificates.HTTPCAType, - rotation, + caRotation, ) if err != nil { return results.WithError(err) @@ -56,7 +57,7 @@ func Reconcile( // handle CA expiry via requeue results.WithResult(reconcile.Result{ - RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, rotation.RotateBefore), + RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, caRotation.RotateBefore), }) // discover and maybe reconcile for the http certificates to use @@ -68,7 +69,7 @@ func Reconcile( as.Spec.HTTP.TLS, labels, services, - rotation, // todo correct rotation + certRotation, ) if err != nil { return results.WithError(err) diff --git a/pkg/controller/common/certificates/ca.go b/pkg/controller/common/certificates/ca.go index 7fec88cea5..7ce8d3e7d3 100644 --- a/pkg/controller/common/certificates/ca.go +++ b/pkg/controller/common/certificates/ca.go @@ -74,7 +74,7 @@ func NewSelfSignedCA(options CABuilderOptions) (*CA, error) { certificateTemplate := x509.Certificate{ SerialNumber: serial, Subject: options.Subject, - NotBefore: time.Now().Add(-1 * time.Minute), + NotBefore: time.Now().Add(-10 * time.Minute), NotAfter: notAfter, SignatureAlgorithm: x509.SHA256WithRSA, IsCA: true, diff --git a/pkg/controller/kibana/certificates/reconcile.go b/pkg/controller/kibana/certificates/reconcile.go index bae38d15b5..6d1192a3da 100644 --- a/pkg/controller/kibana/certificates/reconcile.go +++ b/pkg/controller/kibana/certificates/reconcile.go @@ -27,7 +27,8 @@ func Reconcile( d driver.Interface, kb kbv1.Kibana, services []corev1.Service, - rotation certificates.RotationParams, + caRotation certificates.RotationParams, + certRotation certificates.RotationParams, ) *reconciler.Results { span, _ := apm.StartSpan(ctx, "reconcile_certs", tracing.SpanTypeApp) defer span.End() @@ -48,7 +49,7 @@ func Reconcile( &kb, labels, certificates.HTTPCAType, - rotation, + caRotation, ) if err != nil { return results.WithError(err) @@ -56,7 +57,7 @@ func Reconcile( // handle CA expiry via requeue results.WithResult(reconcile.Result{ - RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, rotation.RotateBefore), + RequeueAfter: certificates.ShouldRotateIn(time.Now(), httpCa.Cert.NotAfter, caRotation.RotateBefore), }) // discover and maybe reconcile for the http certificates to use @@ -68,7 +69,7 @@ func Reconcile( kb.Spec.HTTP.TLS, labels, services, - rotation, // todo correct rotation + certRotation, ) if err != nil { return results.WithError(err) diff --git a/pkg/controller/kibana/driver.go b/pkg/controller/kibana/driver.go index c48b29f5ab..c6920ad66e 100644 --- a/pkg/controller/kibana/driver.go +++ b/pkg/controller/kibana/driver.go @@ -249,7 +249,7 @@ func (d *driver) Reconcile( return results.WithError(err) } - results.WithResults(kbcerts.Reconcile(ctx, d, *kb, []corev1.Service{*svc}, params.CACertRotation)) + results.WithResults(kbcerts.Reconcile(ctx, d, *kb, []corev1.Service{*svc}, params.CACertRotation, params.CertRotation)) if results.HasError() { return results } From 67b7108c51d8c05d5a45aac6c428df07e5e70a60 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Mon, 24 Feb 2020 17:26:56 -0600 Subject: [PATCH 3/4] Requeue for rest of results --- .../apmserver/certificates/reconcile.go | 9 +++++++ .../common/certificates/expiration.go | 6 ++--- pkg/controller/common/certificates/pem.go | 16 +++++++++++- .../certificates/ca_reconcile.go | 19 +++++++++++--- .../certificates/transport/reconcile.go | 26 ++++++++++++++----- .../kibana/certificates/reconcile.go | 9 +++++++ 6 files changed, 70 insertions(+), 15 deletions(-) diff --git a/pkg/controller/apmserver/certificates/reconcile.go b/pkg/controller/apmserver/certificates/reconcile.go index bc41ff58ca..f1fe70e925 100644 --- a/pkg/controller/apmserver/certificates/reconcile.go +++ b/pkg/controller/apmserver/certificates/reconcile.go @@ -74,6 +74,15 @@ func Reconcile( if err != nil { return results.WithError(err) } + + primaryCert, err := certificates.GetPrimaryCertificate(httpCertificates.CertPem()) + if err != nil { + results.WithError(err) + } + results.WithResult(reconcile.Result{ + RequeueAfter: certificates.ShouldRotateIn(time.Now(), primaryCert.NotAfter, certRotation.RotateBefore), + }) + // reconcile http public cert secret results.WithError(http.ReconcileHTTPCertsPublicSecret(driver.K8sClient(), driver.Scheme(), as, name.APMNamer, httpCertificates)) return results diff --git a/pkg/controller/common/certificates/expiration.go b/pkg/controller/common/certificates/expiration.go index 087852bcab..5e15194feb 100644 --- a/pkg/controller/common/certificates/expiration.go +++ b/pkg/controller/common/certificates/expiration.go @@ -23,10 +23,10 @@ type RotationParams struct { } // ShouldRotateIn computes the duration after which a certificate rotation should be scheduled -// in order for the CA cert to be rotated before it expires. -func ShouldRotateIn(now time.Time, certExpiration time.Time, caCertRotateBefore time.Duration) time.Duration { +// in order for the cert to be rotated before it expires. +func ShouldRotateIn(now time.Time, certExpiration time.Time, certRotateBefore time.Duration) time.Duration { // make sure we are past the safety margin when rotating, by making it a little bit shorter - safetyMargin := caCertRotateBefore - 1*time.Second + safetyMargin := certRotateBefore - 1*time.Second requeueTime := certExpiration.Add(-safetyMargin) requeueIn := requeueTime.Sub(now) if requeueIn < 0 { diff --git a/pkg/controller/common/certificates/pem.go b/pkg/controller/common/certificates/pem.go index 1f559f2c43..97dd6e006e 100644 --- a/pkg/controller/common/certificates/pem.go +++ b/pkg/controller/common/certificates/pem.go @@ -31,7 +31,7 @@ func ParsePEMCerts(pemData []byte) ([]*x509.Certificate, error) { cert, err := x509.ParseCertificate(block.Bytes) if err != nil { - return nil, err + return nil, errors.WithStack(err) } certs = append(certs, cert) @@ -86,3 +86,17 @@ func parsePKCS8PrivateKey(block []byte) (*rsa.PrivateKey, error) { return rsaKey, nil } + +// GetPrimaryCertificate returns the primary certificate (i.e. the actual subject, not a CA or intermediate) from a PEM certificate chain +func GetPrimaryCertificate(pemBytes []byte) (*x509.Certificate, error) { + parsedCerts, err := ParsePEMCerts(pemBytes) + if err != nil { + return nil, err + } + // the primary certificate should always come first, see: + // http://tools.ietf.org/html/rfc4346#section-7.4.2 + if len(parsedCerts) < 1 { + return nil, errors.New("Expected at least one certificate") + } + return parsedCerts[0], nil +} diff --git a/pkg/controller/elasticsearch/certificates/ca_reconcile.go b/pkg/controller/elasticsearch/certificates/ca_reconcile.go index d219085ece..159dc7fa6a 100644 --- a/pkg/controller/elasticsearch/certificates/ca_reconcile.go +++ b/pkg/controller/elasticsearch/certificates/ca_reconcile.go @@ -34,7 +34,7 @@ type CertificateResources struct { HTTPCACertProvided bool } -// reconcileGenericResources reconciles the expected generic resources of a cluster. +// Reconcile reconciles the certificates of a cluster. func Reconcile( ctx context.Context, driver driver.Interface, @@ -83,6 +83,14 @@ func Reconcile( return nil, results.WithError(err) } + primaryCert, err := certificates.GetPrimaryCertificate(httpCertificates.CertPem()) + if err != nil { + return nil, results.WithError(err) + } + results.WithResult(reconcile.Result{ + RequeueAfter: certificates.ShouldRotateIn(time.Now(), primaryCert.NotAfter, certRotation.RotateBefore), + }) + // reconcile http public certs secret: if err := http.ReconcileHTTPCertsPublicSecret(driver.K8sClient(), driver.Scheme(), &es, esv1.ESNamer, httpCertificates); err != nil { return nil, results.WithError(err) @@ -105,20 +113,23 @@ func Reconcile( RequeueAfter: certificates.ShouldRotateIn(time.Now(), transportCA.Cert.NotAfter, caRotation.RotateBefore), }) - // reconcile transport public certs secret: + // reconcile transport public certs secret + // TODO sabo have this return the cert, why doesnt this need the cert rotation param? + // this just updates the cert if err := transport.ReconcileTransportCertsPublicSecret(driver.K8sClient(), driver.Scheme(), es, transportCA); err != nil { return nil, results.WithError(err) } // reconcile transport certificates - result, err := transport.ReconcileTransportCertificatesSecrets( + transportResults := transport.ReconcileTransportCertificatesSecrets( driver.K8sClient(), driver.Scheme(), transportCA, es, certRotation, ) - if results.WithResult(result).WithError(err).HasError() { + + if results.WithResults(transportResults).HasError() { return nil, results } diff --git a/pkg/controller/elasticsearch/certificates/transport/reconcile.go b/pkg/controller/elasticsearch/certificates/transport/reconcile.go index 9e380918db..ed93390697 100644 --- a/pkg/controller/elasticsearch/certificates/transport/reconcile.go +++ b/pkg/controller/elasticsearch/certificates/transport/reconcile.go @@ -8,6 +8,9 @@ import ( "bytes" "reflect" "strings" + "time" + + "github.com/pkg/errors" esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/annotation" @@ -33,21 +36,21 @@ func ReconcileTransportCertificatesSecrets( ca *certificates.CA, es esv1.Elasticsearch, rotationParams certificates.RotationParams, -) (reconcile.Result, error) { +) *reconciler.Results { + results := &reconciler.Results{} var pods corev1.PodList matchLabels := label.NewLabelSelectorForElasticsearch(es) ns := client.InNamespace(es.Namespace) if err := c.List(&pods, matchLabels, ns); err != nil { - return reconcile.Result{}, err + return results.WithError(errors.WithStack(err)) } secret, err := ensureTransportCertificatesSecretExists(c, scheme, es) if err != nil { - return reconcile.Result{}, err + return results.WithError(err) } // defensive copy of the current secret so we can check whether we need to update later on currentTransportCertificatesSecret := secret.DeepCopy() - for _, pod := range pods.Items { if pod.Status.PodIP == "" { log.Info("Skipping pod because it has no IP yet", "namespace", pod.Namespace, "pod_name", pod.Name) @@ -57,8 +60,17 @@ func ReconcileTransportCertificatesSecrets( if err := ensureTransportCertificatesSecretContentsForPod( es, secret, pod, ca, rotationParams, ); err != nil { - return reconcile.Result{}, err + return results.WithError(err) + } + certCommonName := buildCertificateCommonName(pod, es.Name, es.Namespace) + cert := extractTransportCert(*secret, pod, certCommonName) + if cert == nil { + return results.WithError(errors.New("No certificate found for pod")) } + // handle cert expiry via requeue + results.WithResult(reconcile.Result{ + RequeueAfter: certificates.ShouldRotateIn(time.Now(), cert.NotAfter, rotationParams.RotateBefore), + }) } // remove certificates and keys for deleted pods @@ -95,14 +107,14 @@ func ReconcileTransportCertificatesSecrets( if !reflect.DeepEqual(secret, currentTransportCertificatesSecret) { if err := c.Update(secret); err != nil { - return reconcile.Result{}, err + return results.WithError(err) } for _, pod := range pods.Items { annotation.MarkPodAsUpdated(c, pod) } } - return reconcile.Result{}, nil + return results } // ensureTransportCertificatesSecretExists ensures the existence and Labels of the Secret that at a later point diff --git a/pkg/controller/kibana/certificates/reconcile.go b/pkg/controller/kibana/certificates/reconcile.go index 6d1192a3da..0f7dc88e5c 100644 --- a/pkg/controller/kibana/certificates/reconcile.go +++ b/pkg/controller/kibana/certificates/reconcile.go @@ -74,6 +74,15 @@ func Reconcile( if err != nil { return results.WithError(err) } + + primaryCert, err := certificates.GetPrimaryCertificate(httpCertificates.CertPem()) + if err != nil { + return results.WithError(err) + } + results.WithResult(reconcile.Result{ + RequeueAfter: certificates.ShouldRotateIn(time.Now(), primaryCert.NotAfter, certRotation.RotateBefore), + }) + // reconcile http public cert secret results.WithError(http.ReconcileHTTPCertsPublicSecret(d.K8sClient(), d.Scheme(), &kb, name.KBNamer, httpCertificates)) return &results From 96165ac2a154dc70ceed37c7de99887d8910a6aa Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Mon, 24 Feb 2020 21:54:19 -0600 Subject: [PATCH 4/4] Rm comments --- pkg/controller/elasticsearch/certificates/ca_reconcile.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controller/elasticsearch/certificates/ca_reconcile.go b/pkg/controller/elasticsearch/certificates/ca_reconcile.go index 159dc7fa6a..9d92d2f516 100644 --- a/pkg/controller/elasticsearch/certificates/ca_reconcile.go +++ b/pkg/controller/elasticsearch/certificates/ca_reconcile.go @@ -114,8 +114,6 @@ func Reconcile( }) // reconcile transport public certs secret - // TODO sabo have this return the cert, why doesnt this need the cert rotation param? - // this just updates the cert if err := transport.ReconcileTransportCertsPublicSecret(driver.K8sClient(), driver.Scheme(), es, transportCA); err != nil { return nil, results.WithError(err) }