Loosen certificate checks (#1413)

The checks we had were a bit too constrained, and don't follow the golang
standard library code. So, replace our replacement functions with the
originals.

This level of checking (e.g. mistyping a path) is not really necessary
when people are using our (e.g. cert-manager) manifests.

Signed-off-by: Todd Short <[email protected]>

Author: tmshort

internal/httputil/certutil.go

@@ -1,10 +1,7 @@
 package httputil
 
 import (
-	"bytes"
 	"crypto/x509"
-	"encoding/base64"
-	"encoding/pem"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -13,192 +10,6 @@ import (
 	"github.com/go-logr/logr"
 )
 
-var pemStart = []byte("\n-----BEGIN ")
-var pemEnd = []byte("\n-----END ")
-var pemEndOfLine = []byte("-----")
-var colon = []byte(":")
-
-// getLine results the first \r\n or \n delineated line from the given byte
-// array. The line does not include trailing whitespace or the trailing new
-// line bytes. The remainder of the byte array (also not including the new line
-// bytes) is also returned and this will always be smaller than the original
-// argument.
-func getLine(data []byte) ([]byte, []byte) {
-	i := bytes.IndexByte(data, '\n')
-	var j int
-	if i < 0 {
-		i = len(data)
-		j = i
-	} else {
-		j = i + 1
-		if i > 0 && data[i-1] == '\r' {
-			i--
-		}
-	}
-	return bytes.TrimRight(data[0:i], " \t"), data[j:]
-}
-
-// removeSpacesAndTabs returns a copy of its input with all spaces and tabs
-// removed, if there were any. Otherwise, the input is returned unchanged.
-//
-// The base64 decoder already skips newline characters, so we don't need to
-// filter them out here.
-func removeSpacesAndTabs(data []byte) []byte {
-	if !bytes.ContainsAny(data, " \t") {
-		// Fast path; most base64 data within PEM contains newlines, but
-		// no spaces nor tabs. Skip the extra alloc and work.
-		return data
-	}
-	result := make([]byte, len(data))
-	n := 0
-
-	for _, b := range data {
-		if b == ' ' || b == '\t' {
-			continue
-		}
-		result[n] = b
-		n++
-	}
-
-	return result[0:n]
-}
-
-// This version of pem.Decode() is a bit less flexible, it will not skip over bad PEM
-// It is basically the guts of pem.Decode() inside the outer for loop, with error
-// returns rather than continues
-func pemDecode(data []byte) (*pem.Block, []byte) {
-	// pemStart begins with a newline. However, at the very beginning of
-	// the byte array, we'll accept the start string without it.
-	rest := data
-	if bytes.HasPrefix(rest, pemStart[1:]) {
-		rest = rest[len(pemStart)-1:]
-	} else if _, after, ok := bytes.Cut(rest, pemStart); ok {
-		rest = after
-	} else {
-		return nil, data
-	}
-
-	var typeLine []byte
-	typeLine, rest = getLine(rest)
-	if !bytes.HasSuffix(typeLine, pemEndOfLine) {
-		return nil, data
-	}
-	typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)]
-
-	p := &pem.Block{
-		Headers: make(map[string]string),
-		Type:    string(typeLine),
-	}
-
-	for {
-		// This loop terminates because getLine's second result is
-		// always smaller than its argument.
-		if len(rest) == 0 {
-			return nil, data
-		}
-		line, next := getLine(rest)
-
-		key, val, ok := bytes.Cut(line, colon)
-		if !ok {
-			break
-		}
-
-		key = bytes.TrimSpace(key)
-		val = bytes.TrimSpace(val)
-		p.Headers[string(key)] = string(val)
-		rest = next
-	}
-
-	var endIndex, endTrailerIndex int
-
-	// If there were no headers, the END line might occur
-	// immediately, without a leading newline.
-	if len(p.Headers) == 0 && bytes.HasPrefix(rest, pemEnd[1:]) {
-		endIndex = 0
-		endTrailerIndex = len(pemEnd) - 1
-	} else {
-		endIndex = bytes.Index(rest, pemEnd)
-		endTrailerIndex = endIndex + len(pemEnd)
-	}
-
-	if endIndex < 0 {
-		return nil, data
-	}
-
-	// After the "-----" of the ending line, there should be the same type
-	// and then a final five dashes.
-	endTrailer := rest[endTrailerIndex:]
-	endTrailerLen := len(typeLine) + len(pemEndOfLine)
-	if len(endTrailer) < endTrailerLen {
-		return nil, data
-	}
-
-	restOfEndLine := endTrailer[endTrailerLen:]
-	endTrailer = endTrailer[:endTrailerLen]
-	if !bytes.HasPrefix(endTrailer, typeLine) ||
-		!bytes.HasSuffix(endTrailer, pemEndOfLine) {
-		return nil, data
-	}
-
-	// The line must end with only whitespace.
-	if s, _ := getLine(restOfEndLine); len(s) != 0 {
-		return nil, data
-	}
-
-	base64Data := removeSpacesAndTabs(rest[:endIndex])
-	p.Bytes = make([]byte, base64.StdEncoding.DecodedLen(len(base64Data)))
-	n, err := base64.StdEncoding.Decode(p.Bytes, base64Data)
-	if err != nil {
-		return nil, data
-	}
-	p.Bytes = p.Bytes[:n]
-
-	// the -1 is because we might have only matched pemEnd without the
-	// leading newline if the PEM block was empty.
-	_, rest = getLine(rest[endIndex+len(pemEnd)-1:])
-	return p, rest
-}
-
-// This version of (*x509.CertPool).AppendCertsFromPEM() will error out if parsing fails
-func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte, firstExpiration *time.Time) error {
-	n := 1
-	for len(pemCerts) > 0 {
-		var block *pem.Block
-		block, pemCerts = pemDecode(pemCerts)
-		if block == nil {
-			return fmt.Errorf("unable to PEM decode cert %d", n)
-		}
-		// ignore non-certificates (e.g. keys)
-		if block.Type != "CERTIFICATE" {
-			continue
-		}
-		if len(block.Headers) != 0 {
-			// This is a cert, but we're ignoring it, so bump the counter
-			n++
-			continue
-		}
-
-		cert, err := x509.ParseCertificate(block.Bytes)
-		if err != nil {
-			return fmt.Errorf("unable to parse cert %d: %w", n, err)
-		}
-		if firstExpiration.IsZero() || firstExpiration.After(cert.NotAfter) {
-			*firstExpiration = cert.NotAfter
-		}
-		now := time.Now()
-		if now.Before(cert.NotBefore) {
-			return fmt.Errorf("not yet valid cert %d: %q", n, cert.NotBefore.Format(time.RFC3339))
-		} else if now.After(cert.NotAfter) {
-			return fmt.Errorf("expired cert %d: %q", n, cert.NotAfter.Format(time.RFC3339))
-		}
-		// no return values - panics or always succeeds
-		s.AddCert(cert)
-		n++
-	}
-
-	return nil
-}
-
 func NewCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) {
 	caCertPool, err := x509.SystemCertPool()
 	if err != nil {
@@ -231,11 +42,10 @@ func NewCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) {
 		if err != nil {
 			return nil, fmt.Errorf("error reading cert file %q: %w", file, err)
 		}
-		err = appendCertsFromPEM(caCertPool, data, &firstExpiration)
-		if err != nil {
-			return nil, fmt.Errorf("error adding cert file %q: %w", file, err)
+		// The return indicates if any certs were added
+		if caCertPool.AppendCertsFromPEM(data) {
+			count++
 		}
-		count++
 	}
 
 	// Found no certs!

internal/httputil/certutil_test.go

@@ -11,26 +11,15 @@ import (
 )
 
 // The "good" test consists of 3 Amazon Root CAs, along with a "PRIVATE KEY" in one of the files
-// The "bad" test consists of 2 Amazon Root CAs, the second of which is garbage, and the test fails
-// The "ugly" test consists of a single file:
-// - Amazon_Root_CA_1
-// - garbage PEM
-// - Amazon_Root_CA_3
-// The error is _not_ detected because the golang standard library PEM decoder skips right over the garbage
-// This demonstrates the danger of putting multiple certificates into a single file
+// The "empty" test includes a single file with no PEM contents
 func TestNewCertPool(t *testing.T) {
 	caDirs := []struct {
 		dir string
 		msg string
 	}{
 		{"../../testdata/certs/", `no certificates found in "../../testdata/certs/"`},
 		{"../../testdata/certs/good", ""},
-		{"../../testdata/certs/bad", `error adding cert file "../../testdata/certs/bad/Amazon_Root_CA_2.pem": unable to PEM decode cert 1`},
-		{"../../testdata/certs/ugly", `error adding cert file "../../testdata/certs/ugly/Amazon_Root_CA.pem": unable to PEM decode cert 2`},
-		{"../../testdata/certs/ugly2", `error adding cert file "../../testdata/certs/ugly2/Amazon_Root_CA_1.pem": unable to PEM decode cert 1`},
-		{"../../testdata/certs/ugly3", `error adding cert file "../../testdata/certs/ugly3/not_a_cert.pem": unable to PEM decode cert 1`},
-		{"../../testdata/certs/empty", `error adding cert file "../../testdata/certs/empty/empty.pem": unable to parse cert 1: x509: malformed certificate`},
-		{"../../testdata/certs/expired", `error adding cert file "../../testdata/certs/expired/expired.pem": expired cert 1: "2024-01-02T15:00:00Z"`},
+		{"../../testdata/certs/empty", `no certificates found in "../../testdata/certs/empty"`},
 	}
 
 	log, _ := logr.FromContext(context.Background())