Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cert rotate parameter #2541

Merged
merged 6 commits into from
Feb 25, 2020
Merged

Use cert rotate parameter #2541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2fca99
Correct ES cert rotation
anyasabo Feb 10, 2020
25deaf9
Increase CA issuing time skew allowance
anyasabo Feb 10, 2020
f170bb5
Merge branch 'master' into certrotate
anyasabo Feb 13, 2020
1b9c4da
Merge branch 'master' into certrotate
anyasabo Feb 24, 2020
67b7108
Requeue for rest of results
anyasabo Feb 24, 2020
96165ac
Rm comments
anyasabo Feb 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,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 a client to set the operator uuid config map
clientset, err := kubernetes.NewForConfig(cfg)
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/apmserver/apmserver_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 14 additions & 4 deletions pkg/controller/apmserver/certificates/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -48,15 +49,15 @@ func Reconcile(
as,
labels,
certificates.HTTPCAType,
rotation,
caRotation,
)
if err != nil {
return results.WithError(err)
}

// 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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also make that call for the other certs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow your comment here @sebgl. Do you mean we should requeue within the validity period of the individual certificates and not just before the CA expires?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I mean also requeueing after we reconcile HTTP certificates below. Since we have caRotation and certRotation that can be different, it would make sense to make sure we requeue before any of these are reached? Maybe I'm missing something here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not requeue on the minimum of the two?

Copy link
Contributor

@sebgl sebgl Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! That's why I thought we would call results.WithResult () twice (one for CA, one for cert 10 lines below), and then we let the results aggregation do its job of picking the most appropriate requeue?

Copy link
Collaborator

@pebrc pebrc Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM (I always forget that we already pick the shortest requeue in the aggregation)

})

// discover and maybe reconcile for the http certificates to use
Expand All @@ -68,11 +69,20 @@ func Reconcile(
as.Spec.HTTP.TLS,
labels,
services,
rotation, // todo correct rotation
certRotation,
)
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
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/common/certificates/ca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/common/certificates/expiration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
16 changes: 15 additions & 1 deletion pkg/controller/common/certificates/pem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
19 changes: 14 additions & 5 deletions pkg/controller/elasticsearch/certificates/ca_reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -77,12 +77,20 @@ func Reconcile(
es.Spec.HTTP.TLS,
labels,
services,
caRotation,
certRotation,
)
if err != nil {
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)
Expand All @@ -105,20 +113,21 @@ func Reconcile(
RequeueAfter: certificates.ShouldRotateIn(time.Now(), transportCA.Cert.NotAfter, caRotation.RotateBefore),
})

// reconcile transport public certs secret:
// reconcile transport public certs secret
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
}

Expand Down
26 changes: 19 additions & 7 deletions pkg/controller/elasticsearch/certificates/transport/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 14 additions & 4 deletions pkg/controller/kibana/certificates/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -48,15 +49,15 @@ func Reconcile(
&kb,
labels,
certificates.HTTPCAType,
rotation,
caRotation,
)
if err != nil {
return results.WithError(err)
}

// 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
Expand All @@ -68,11 +69,20 @@ func Reconcile(
kb.Spec.HTTP.TLS,
labels,
services,
rotation, // todo correct rotation
certRotation,
)
if err != nil {
return results.WithError(err)
}

primaryCert, err := certificates.GetPrimaryCertificate(httpCertificates.CertPem())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit unfortunate that we have to parse the PEM again that we just parsed and or encoded in the reconcile function. But I guess changing this would be a larger refactoring of our certificate generation logic, so happy to track this in a follow up task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I wasn't very happy about it, but the needs of both of them are discrete enough (one needs to construct the whole chain, the other just needs the expiration date of one cert) that I couldn't think of a better way to fix it without a larger scale change. One option might be for ReconcileHTTPCertificates() to return a Results, which could include errors and a resync time.

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
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/kibana/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down