feat: implement DNS resolution bypass config flag

permits to ignore DNS resolution and will permit to fix #15 some refactoring with this commit to make it easier to implement this feature
postfinance · Jan 3, 2022 · 1652c90 · 1652c90
1 parent 0bb1705
commit 1652c90
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 93 deletions.
diff --git a/go.mod b/go.mod
@@ -4,8 +4,7 @@ go 1.17
 
 require (
 	github.com/foxcpp/go-mockdns v0.0.0-20210729171921-fb145fc6f897
-	github.com/go-logr/logr v1.2.2
-	github.com/go-logr/zapr v1.2.2
+	github.com/go-logr/zapr v1.2.0
 	github.com/postfinance/flash v0.2.0
 	github.com/stretchr/testify v1.7.0
 	github.com/thanhpk/randstr v1.0.4
@@ -31,6 +30,7 @@ require (
 	github.com/evanphx/json-patch v4.12.0+incompatible // indirect
 	github.com/form3tech-oss/jwt-go v3.2.3+incompatible // indirect
 	github.com/fsnotify/fsnotify v1.5.1 // indirect
+	github.com/go-logr/logr v1.2.0 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.2 // indirect

diff --git a/go.sum b/go.sum
@@ -188,12 +188,10 @@ github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V
 github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG1KdI/P7A=
 github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas=
 github.com/go-logr/logr v0.2.0/go.mod h1:z6/tIYblkpsD+a4lm/fGIIU9mZ+XfAiaFtq7xTgseGU=
+github.com/go-logr/logr v1.2.0 h1:QK40JKJyMdUDz+h+xvCsru/bJhvG0UxvePV0ufL/AcE=
 github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
-github.com/go-logr/logr v1.2.2 h1:ahHml/yUpnlb96Rp8HCvtYVPY8ZYpxq3g7UYchIYwbs=
-github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
+github.com/go-logr/zapr v1.2.0 h1:n4JnPI1T3Qq1SFEi/F8rwLrZERp2bso19PJZDB9dayk=
 github.com/go-logr/zapr v1.2.0/go.mod h1:Qa4Bsj2Vb+FAVeAKsLD8RLQ+YRJB8YDmOAKxaBQf7Ro=
-github.com/go-logr/zapr v1.2.2 h1:5YNlIL6oZLydaV4dOFjL8YpgXF/tPeTbnpatnu3cq6o=
-github.com/go-logr/zapr v1.2.2/go.mod h1:eIauM6P8qSvTw5o2ez6UEAfGjQKrxQTl5EoK+Qa2oG4=
 github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg=
 github.com/go-openapi/jsonpointer v0.19.5/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg=
 github.com/go-openapi/jsonreference v0.19.3/go.mod h1:rjx6GuL8TTa9VaixXglHmQmIL98+wF9xc8zWvFonSJ8=

diff --git a/internal/cmd/cmd.go b/internal/cmd/cmd.go
@@ -35,13 +35,14 @@ var (
 
 // Config stores all parameters needed to configure a controller-manager
 type Config struct {
-	logLevel    int
-	metricsAddr string
-	probeAddr   string
-	RegexStr    string
-	MaxSec      int
-	K8sConfig   *rest.Config
-	DNSResolver controller.HostResolver
+	logLevel            int
+	metricsAddr         string
+	probeAddr           string
+	RegexStr            string
+	MaxSec              int
+	K8sConfig           *rest.Config
+	DNSResolver         controller.HostResolver
+	BypassDNSResolution bool
 }
 
 // Run will start the controller with the default settings
@@ -113,6 +114,7 @@ func CreateControllerManager(config *Config) (mgr ctrl.Manager, code int) {
 		ProviderRegexp:       providerRegexp.MatchString,
 		MaxExpirationSeconds: int32(config.MaxSec),
 		Resolver:             config.DNSResolver,
+		BypassDNSResolution:  config.BypassDNSResolution,
 	}
 
 	if err = csrController.SetupWithManager(mgr); err != nil {
@@ -132,11 +134,12 @@ func prepareCmdlineConfig() *Config {
 	fs := flag.NewFlagSet("kubelet-csr-approver", flag.ExitOnError)
 
 	var (
-		logLevel    = fs.Int("level", 0, "level ranges from -5 (Fatal) to 10 (Verbose)")
-		metricsAddr = fs.String("metrics-bind-address", ":8080", "address the metric endpoint binds to.")
-		probeAddr   = fs.String("health-probe-bind-address", ":8081", "address the probe endpoint binds to.")
-		regexStr    = fs.String("provider-regex", "", "provider-specified regex to validate CSR SAN names against")
-		maxSec      = fs.Int("max-expiration-sec", 367*24*3600, "maximum seconds a CSR can request a cerficate for. defaults to 367 days")
+		logLevel            = fs.Int("level", 0, "level ranges from -5 (Fatal) to 10 (Verbose)")
+		metricsAddr         = fs.String("metrics-bind-address", ":8080", "address the metric endpoint binds to.")
+		probeAddr           = fs.String("health-probe-bind-address", ":8081", "address the probe endpoint binds to.")
+		regexStr            = fs.String("provider-regex", "", "provider-specified regex to validate CSR SAN names against")
+		maxSec              = fs.Int("max-expiration-sec", 367*24*3600, "maximum seconds a CSR can request a cerficate for. defaults to 367 days")
+		bypassDNSResolution = fs.Bool("bypass-dns-resolution", false, "set this parameter to true to bypass DNS resolution checks")
 	)
 
 	err := ff.Parse(fs, os.Args[1:], ff.WithEnvVarNoPrefix())
@@ -147,11 +150,12 @@ func prepareCmdlineConfig() *Config {
 	}
 
 	config := Config{
-		logLevel:    *logLevel,
-		metricsAddr: *metricsAddr,
-		probeAddr:   *probeAddr,
-		RegexStr:    *regexStr,
-		MaxSec:      *maxSec,
+		logLevel:            *logLevel,
+		metricsAddr:         *metricsAddr,
+		probeAddr:           *probeAddr,
+		RegexStr:            *regexStr,
+		BypassDNSResolution: *bypassDNSResolution,
+		MaxSec:              *maxSec,
 	}
 
 	config.DNSResolver = net.DefaultResolver

diff --git a/internal/controller/csr_controller.go b/internal/controller/csr_controller.go
@@ -22,8 +22,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/go-logr/logr"
-
 	certificatesv1 "k8s.io/api/certificates/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -48,6 +46,7 @@ type CertificateSigningRequestReconciler struct {
 	Scheme               *runtime.Scheme
 	ProviderRegexp       func(string) bool
 	MaxExpirationSeconds int32
+	BypassDNSResolution  bool
 	Resolver             HostResolver
 }
 
@@ -56,6 +55,7 @@ type CertificateSigningRequestReconciler struct {
 //+kubebuilder:rbac:groups=certificates.k8s.io,resources=signers,resourceNames="kubernetes.io/kubelet-serving",verbs=approve
 
 // Reconcile will perform a series of checks before deciding whether the CSR should be approved or denied
+//nolint: gocyclo // cyclomatic complexity is high (over 15), but this improves readibility for the programmer, therefore we ignore the linting error
 func (r *CertificateSigningRequestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, returnErr error) {
 	l := log.FromContext(ctx)
 
@@ -71,18 +71,31 @@ func (r *CertificateSigningRequestReconciler) Reconcile(ctx context.Context, req
 		return
 	}
 
-	if passed := baselineCsrChecks(l, &csr); !passed {
+	// baseline CSR checks - triage to ignore CSR we should process
+	if csr.Spec.SignerName != certificatesv1.KubeletServingSignerName {
+		l.V(4).Info("Ignoring non-kubelet-serving CSR.")
+		return
+	}
+
+	if approved, denied := GetCertApprovalCondition(&csr.Status); approved || denied {
+		l.V(3).Info("The CSR is already approved|denied. Ignoring", "approved", approved, "denied", denied)
+		return
+	}
+
+	if len(csr.Status.Certificate) > 0 {
+		l.V(3).Info("The CSR is already signed. No need to do anything else.")
 		return
 	}
 
+	// actual CSR and x509 CR checks
 	x509cr, err := ParseCSR(csr.Spec.Request)
 	if err != nil {
 		l.Error(err, fmt.Sprintf("unable to parse csr %q", csr.Name))
 		return
 	}
 
-	if csr.Spec.ExpirationSeconds != nil && *csr.Spec.ExpirationSeconds > r.MaxExpirationSeconds {
-		reason := "CSR spec.expirationSeconds is longer than the maximum allowed expiration second"
+	if len(x509cr.DNSNames)+len(x509cr.IPAddresses) == 0 {
+		reason := "The x509 Cert Request SAN contains neither an IP address nor a DNS name"
 		l.V(0).Info("Denying kubelet-serving CSR. Reason:" + reason)
 
 		appendCondition(&csr, false, reason)
@@ -97,12 +110,17 @@ func (r *CertificateSigningRequestReconciler) Reconcile(ctx context.Context, req
 			"commonName", x509cr.Subject.CommonName, "specUsername", csr.Spec.Username)
 
 		appendCondition(&csr, false, reason)
-	} else if valid, reason, err := r.RegexIPChecks(ctx, &csr, x509cr); !valid {
+	} else if valid, reason, err := r.DNSCheck(ctx, &csr, x509cr); !valid {
 		if err != nil {
 			l.V(0).Error(err, reason)
 			return res, err // returning a non-nil error to make this request be processed again in the reconcile function
 		}
-		l.V(0).Info("Denying kubelet-serving CSR. Regex/IP checks failed. Reason:" + reason)
+		l.V(0).Info("Denying kubelet-serving CSR. DNS checks failed. Reason:" + reason)
+
+		appendCondition(&csr, false, reason)
+	} else if csr.Spec.ExpirationSeconds != nil && *csr.Spec.ExpirationSeconds > r.MaxExpirationSeconds {
+		reason := "CSR spec.expirationSeconds is longer than the maximum allowed expiration second"
+		l.V(0).Info("Denying kubelet-serving CSR. Reason:" + reason)
 
 		appendCondition(&csr, false, reason)
 	} else if valid, reason := ProviderChecks(&csr, x509cr); !valid {
@@ -127,27 +145,6 @@ func (r *CertificateSigningRequestReconciler) Reconcile(ctx context.Context, req
 	return res, nil
 }
 
-func baselineCsrChecks(l logr.Logger, csr *certificatesv1.CertificateSigningRequest) (passed bool) {
-	passed = false
-
-	if csr.Spec.SignerName != certificatesv1.KubeletServingSignerName {
-		l.V(4).Info("Ignoring non-kubelet-serving CSR.")
-		return
-	}
-
-	if approved, denied := GetCertApprovalCondition(&csr.Status); approved || denied {
-		l.V(3).Info("The CSR is already approved|denied. Ignoring", "approved", approved, "denied", denied)
-		return
-	}
-
-	if len(csr.Status.Certificate) > 0 {
-		l.V(3).Info("The CSR is already signed. No need to do anything else.")
-		return
-	}
-
-	return true
-}
-
 func appendCondition(csr *certificatesv1.CertificateSigningRequest, approved bool, reason string) {
 	if approved {
 		csr.Status.Conditions = append(csr.Status.Conditions, certificatesv1.CertificateSigningRequestCondition{

diff --git a/internal/controller/regex_ip_checks.go b/internal/controller/regex_ip_checks.go
@@ -12,69 +12,74 @@ import (
 	certificatesv1 "k8s.io/api/certificates/v1"
 )
 
-// RegexIPChecks is a function checking that the DNS Name and the IP address comply with the provider-specific regex/ranges
-func (r *CertificateSigningRequestReconciler) RegexIPChecks(ctx context.Context, csr *certificatesv1.CertificateSigningRequest, x509cr *x509.CertificateRequest) (valid bool, reason string, err error) {
+// DNSCheck is a function checking that the DNS name:
+// complies with the provider-specific regex
+// is resolvable (this check can be opted out with a parameter)
+func (r *CertificateSigningRequestReconciler) DNSCheck(ctx context.Context, csr *certificatesv1.CertificateSigningRequest, x509cr *x509.CertificateRequest) (valid bool, reason string, err error) {
 	if valid = (len(x509cr.DNSNames) <= 1); !valid {
 		reason = "The x509 Cert Request contains more than 1 DNS name"
 		return
 	}
 
-	if valid = (len(x509cr.DNSNames)+len(x509cr.IPAddresses) != 0); !valid {
-		reason = "The x509 Cert Request SAN doesn't contain IP address nor DNS name"
-		return
+	// no DNS name to check, the DNS check is approved
+	if len(x509cr.DNSNames) == 0 {
+		valid = true
+		return valid, reason, nil
 	}
 
-	if len(x509cr.DNSNames) == 1 {
-		sanDNSName := x509cr.DNSNames[0]
+	sanDNSName := x509cr.DNSNames[0]
+	hostname := strings.TrimPrefix(csr.Spec.Username, "system:node:")
 
-		if valid = r.ProviderRegexp(sanDNSName); !valid {
-			reason = "The DNS name in the x509 CSR is not allowed by the Cloud provider regex"
-			return
-		}
+	if valid = strings.HasPrefix(sanDNSName, hostname); !valid {
+		reason = "The SAN DNS Name in the x509 CSR is not prefixed by the node name (hostname)"
+		return
+	}
 
-		hostname := strings.TrimPrefix(csr.Spec.Username, "system:node:")
-		if valid = strings.HasPrefix(sanDNSName, hostname); !valid {
-			reason = "The SAN DNS Name in the x509 CSR is not prefixed by the node name (hostname)"
-			return
-		}
+	if valid = r.ProviderRegexp(sanDNSName); !valid {
+		reason = "The SAN DNS name in the x509 CR is not allowed by the Cloud provider regex"
+		return
+	}
 
-		dnsCtx, dnsCtxCancel := context.WithDeadline(ctx, time.Now().Add(time.Second)) // 1 second timeout for the dns request
-		defer dnsCtxCancel()
+	// bypassing DNS reslution - DNS check is approved
+	if r.BypassDNSResolution {
+		valid = true
+		return valid, reason, nil
+	}
 
-		var resolvedAddrs []string
-		resolvedAddrs, err = r.Resolver.LookupHost(dnsCtx, sanDNSName)
+	dnsCtx, dnsCtxCancel := context.WithDeadline(ctx, time.Now().Add(time.Second)) // 1 second timeout for the dns request
+	defer dnsCtxCancel()
 
-		if err != nil || len(resolvedAddrs) == 0 {
-			return false, "The SAN DNS Name could not be resolved, denying the CSR", nil
-		}
+	var resolvedAddrs []string
+	resolvedAddrs, err = r.Resolver.LookupHost(dnsCtx, sanDNSName)
 
-		var setBuilder netaddr.IPSetBuilder
+	if err != nil || len(resolvedAddrs) == 0 {
+		return false, "The SAN DNS Name could not be resolved, denying the CSR", nil
+	}
 
-		for _, a := range resolvedAddrs {
-			ipa, err := netaddr.ParseIP(a)
-			if err != nil {
-				return false, fmt.Sprintf("Error while parsing resolved IP address %s, denying the CSR", ipa), nil
-			}
+	var setBuilder netaddr.IPSetBuilder
 
-			setBuilder.Add(ipa)
+	for _, a := range resolvedAddrs {
+		ipa, err := netaddr.ParseIP(a)
+		if err != nil {
+			return false, fmt.Sprintf("Error while parsing resolved IP address %s, denying the CSR", ipa), nil
 		}
 
-		ipSet, _ := setBuilder.IPSet()
+		setBuilder.Add(ipa)
+	}
 
-		sanIPAddrs := x509cr.IPAddresses
-		for _, ip := range sanIPAddrs {
-			ipa, ok := netaddr.FromStdIP(ip)
-			if !ok {
-				return false, fmt.Sprintf("Error while x509 CR IP address %s, denying the CSR", ip), nil
-			}
+	resolvedIPSet, _ := setBuilder.IPSet()
 
-			if !ipSet.Contains(ipa) {
-				return false, fmt.Sprintf("One of the SAN IP addresses, %s, is not contained in the set of resolved IP addresses, denying the CSR.", ipa), nil
-			}
+	sanIPAddrs := x509cr.IPAddresses
+	for _, ip := range sanIPAddrs {
+		ipa, ok := netaddr.FromStdIP(ip)
+		if !ok {
+			return false, fmt.Sprintf("Error while parsing x509 CR IP address %s, denying the CSR", ip), nil
 		}
-	}
 
-	valid = true
+		if !resolvedIPSet.Contains(ipa) {
+			return false, fmt.Sprintf("One of the SAN IP addresses, %s, is not contained in the set of resolved IP addresses, denying the CSR.", ipa), nil
+		}
+	}
 
 	return valid, reason, nil
 }