Merge pull request #689 from jcmoraisjr/jm-log-acme

Improve certificate validation of acme signer
jcmoraisjr · Nov 5, 2020 · 8ccfff7 · 8ccfff7
2 parents c07fbf3 + 238c7f0
commit 8ccfff7
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 25 deletions.
diff --git a/pkg/acme/signer.go b/pkg/acme/signer.go
@@ -17,7 +17,6 @@ limitations under the License.
 package acme
 
 import (
-	"crypto/rsa"
 	"crypto/x509"
 	"fmt"
 	"reflect"
@@ -53,14 +52,13 @@ type Cache interface {
 
 // SignerResolver ...
 type SignerResolver interface {
-	GetTLSSecretContent(secretName string) *TLSSecret
+	GetTLSSecretContent(secretName string) (*TLSSecret, error)
 	SetTLSSecretContent(secretName string, pemCrt, pemKey []byte) error
 }
 
 // TLSSecret ...
 type TLSSecret struct {
 	Crt *x509.Certificate
-	Key *rsa.PrivateKey
 }
 
 type signer struct {
@@ -123,14 +121,14 @@ func (s *signer) Notify(item interface{}) error {
 
 func (s *signer) verify(secretName string, domains []string) (verifyErr error) {
 	duedate := time.Now().Add(s.expiring)
-	tls := s.cache.GetTLSSecretContent(secretName)
+	tls, errSecret := s.cache.GetTLSSecretContent(secretName)
 	strdomains := strings.Join(domains, ",")
-	if tls == nil || tls.Crt.NotAfter.Before(duedate) || !match(domains, tls.Crt) {
+	if errSecret != nil || tls.Crt.NotAfter.Before(duedate) || !match(domains, tls.Crt) {
 		var collector func(domains string, success bool)
 		var reason string
-		if tls == nil {
+		if errSecret != nil {
 			collector = s.metrics.IncCertSigningMissing
-			reason = "certificate does not exist"
+			reason = fmt.Sprintf("certificate does not exist (%v)", errSecret)
 		} else if tls.Crt.NotAfter.Before(duedate) {
 			collector = s.metrics.IncCertSigningExpiring
 			reason = fmt.Sprintf("certificate expires in %s", tls.Crt.NotAfter.String())
@@ -174,4 +172,4 @@ func match(domains []string, crt *x509.Certificate) bool {
 		}
 	}
 	return true
-}
+}
diff --git a/pkg/acme/signer_test.go b/pkg/acme/signer_test.go
@@ -20,6 +20,7 @@ import (
 	"crypto"
 	"crypto/x509"
 	"encoding/base64"
+	"fmt"
 	"testing"
 	"time"
 
@@ -68,7 +69,7 @@ INFO acme: new certificate issued: id=1 secret=s1 domain(s)=d3.local`,
 			expiresIn: 10 * 24 * time.Hour,
 			cert: dumbcrt,
 			logging: `
-INFO acme: authorizing: id=1 secret=s2 domain(s)=d1.local endpoint=https://acme-v2.local reason='certificate does not exist'
+INFO acme: authorizing: id=1 secret=s2 domain(s)=d1.local endpoint=https://acme-v2.local reason='certificate does not exist (secret not found: s2)'
 INFO acme: new certificate issued: id=1 secret=s2 domain(s)=d1.local`,
 		},
 		{
@@ -151,12 +152,12 @@ func (c *cache) GetToken(domain, uri string) string {
 	return ""
 }
 
-func (c *cache) GetTLSSecretContent(secretName string) *TLSSecret {
+func (c *cache) GetTLSSecretContent(secretName string) (*TLSSecret, error) {
 	tls, found := c.tlsSecret[secretName]
 	if found {
-		return tls
+		return tls, nil
 	}
-	return nil
+	return nil, fmt.Errorf("secret not found: %s", secretName)
 }
 
 func (c *cache) SetTLSSecretContent(secretName string, pemCrt, pemKey []byte) error {

diff --git a/pkg/controller/cache.go b/pkg/controller/cache.go
@@ -444,30 +444,26 @@ func (c *k8scache) GetKey() (crypto.Signer, error) {
 }
 
 // Implements acme.SignerResolver
-func (c *k8scache) GetTLSSecretContent(secretName string) *acme.TLSSecret {
+func (c *k8scache) GetTLSSecretContent(secretName string) (*acme.TLSSecret, error) {
 	secret, err := c.GetSecret(secretName)
 	if err != nil {
-		return nil
+		return nil, err
 	}
 	pemCrt, foundCrt := secret.Data[api.TLSCertKey]
-	pemKey, foundKey := secret.Data[api.TLSPrivateKeyKey]
-	if !foundCrt || !foundKey {
-		return nil
+	if !foundCrt {
+		return nil, fmt.Errorf("secret %s does not have %s key", secretName, api.TLSCertKey)
 	}
 	derCrt, _ := pem.Decode(pemCrt)
-	derKey, _ := pem.Decode(pemKey)
-	if derCrt == nil || derKey == nil {
-		return nil
+	if derCrt == nil {
+		return nil, fmt.Errorf("error decoding crt of secret %s: cannot find a proper pem block", secretName)
 	}
 	crt, errCrt := x509.ParseCertificate(derCrt.Bytes)
-	key, errKey := x509.ParsePKCS1PrivateKey(derKey.Bytes)
-	if errCrt != nil || errKey != nil {
-		return nil
+	if errCrt != nil {
+		return nil, fmt.Errorf("error parsing crt of secret %s: %w", secretName, errCrt)
 	}
 	return &acme.TLSSecret{
 		Crt: crt,
-		Key: key,
-	}
+	}, nil
 }
 
 // Implements acme.SignerResolver