improvement: set acceptAnonymousUploads=1 for initial kotsadm-tls cert (

#4344)

* improvement: set acceptAnonymousUploads=1 for initial kotsadm-tls cert

SC: https://app.shortcut.com/replicated/story/94724

* Changes as per PR comments

kurl_proxy/cmd/main.go

@@ -8,9 +8,8 @@ import (
 	"encoding/pem"
 	"fmt"
 	"html/template"
-	"io/ioutil"
+	"io"
 	"log"
-	"math/rand"
 	"net"
 	"net/http"
 	"net/http/httputil"
@@ -44,6 +43,8 @@ var (
 	}
 )
 
+const DEFAULT_KOTSADM_CERT_CN = "kotsadm.default.svc.cluster.local"
+
 type cert struct {
 	tlsCert                tls.Certificate
 	fingerprint            string
@@ -53,8 +54,6 @@ type cert struct {
 func main() {
 	log.Printf("Commit %s\n", os.Getenv("COMMIT"))
 
-	rand.Seed(time.Now().UnixNano())
-
 	upstreamOrigin := os.Getenv("UPSTREAM_ORIGIN")
 	dexUpstreamOrigin := os.Getenv("DEX_UPSTREAM_ORIGIN")
 	tlsSecretName := os.Getenv("TLS_SECRET_NAME")
@@ -86,16 +85,19 @@ func main() {
 	if err != nil {
 		log.Panic(err)
 	}
+	// TODO: Assert namespace not empty, else we get secrets from all namespaces
 	secrets := clientset.CoreV1().Secrets(namespace)
 
 	_, err = secrets.Get(context.Background(), tlsSecretName, metav1.GetOptions{})
 	if err != nil {
-		log.Print("creating tls secret")
+		log.Print("creating default tls secret")
 
 		err = generateDefaultCertSecret(secrets, namespace)
 		if err != nil {
 			log.Printf("Could not regenerate default certificate: %v", err)
 		}
+		// TODO: Why are we exiting here? It leads to a pod restart
+		// Leaving as-is for now to avoid unwanted regressions
 		return
 	}
 
@@ -332,7 +334,7 @@ func getHttpsServer(upstream, dexUpstream *url.URL, tlsSecretName string, secret
 			c.AbortWithStatus(http.StatusInternalServerError)
 			return
 		}
-		log.Printf("hostname=%v", hostString)
+		log.Printf("Skipping TLS cert upload. Generating self-signed cert for %q", hostString)
 
 		secret, err := secrets.Get(c.Request.Context(), tlsSecretName, metav1.GetOptions{})
 		if err != nil {
@@ -485,7 +487,7 @@ func getUploadedCerts(c *gin.Context) ([]byte, []byte, error) {
 		return nil, nil, errors.Wrapf(err, "open cert file")
 	}
 	defer certFile.Close()
-	certData, err := ioutil.ReadAll(certFile)
+	certData, err := io.ReadAll(certFile)
 	if err != nil {
 		return nil, nil, errors.Wrapf(err, "read cert file")
 	}
@@ -499,7 +501,7 @@ func getUploadedCerts(c *gin.Context) ([]byte, []byte, error) {
 		return nil, nil, errors.Wrapf(err, "open key file")
 	}
 	defer keyFile.Close()
-	keyData, err := ioutil.ReadAll(keyFile)
+	keyData, err := io.ReadAll(keyFile)
 	if err != nil {
 		return nil, nil, errors.Wrapf(err, "read key file")
 	}
@@ -577,17 +579,17 @@ func regenerateCertWithHostname(certData []byte, hostname string) ([]byte, []byt
 	// will be empty. If already rotated by ekco then Subject will be like
 	// "kotsadm.default.svc.cluster.local@1604697213" and Issuer like
 	// "kotsadm.default.svc.cluster.local-ca@1604697213"
-	if cert.Issuer.CommonName != "" && !strings.HasPrefix(cert.Issuer.CommonName, "kotsadm.default.svc.cluster.local") {
-		return nil, nil, errors.New("cert issuer common name is not kotsadm.default.svc.cluster.local")
+	if cert.Issuer.CommonName != "" && !strings.HasPrefix(cert.Issuer.CommonName, DEFAULT_KOTSADM_CERT_CN) {
+		return nil, nil, errors.Errorf("cert issuer common name is not %s", DEFAULT_KOTSADM_CERT_CN)
 	}
-	if !strings.HasPrefix(cert.Subject.CommonName, "kotsadm.default.svc.cluster.local") {
-		return nil, nil, errors.New("cert common name is not kotsadm.default.svc.cluster.local")
+	if !strings.HasPrefix(cert.Subject.CommonName, DEFAULT_KOTSADM_CERT_CN) {
+		return nil, nil, errors.Errorf("cert common name is not %s", DEFAULT_KOTSADM_CERT_CN)
 	}
 
 	dnsNames := cleanStringSlice(append(cert.DNSNames, hostname))
 
 	// Generate a new self-signed cert
-	certData, keyData, err := certutil.GenerateSelfSignedCertKey("kotsadm.default.svc.cluster.local", cert.IPAddresses, dnsNames)
+	certData, keyData, err := certutil.GenerateSelfSignedCertKey(DEFAULT_KOTSADM_CERT_CN, cert.IPAddresses, dnsNames)
 	if err != nil {
 		return nil, nil, errors.Wrapf(err, "generate self-signed cert")
 	}
@@ -621,23 +623,15 @@ func generateDefaultCertSecret(secrets corev1.SecretInterface, namespace string)
 			Name:      "kotsadm-tls",
 			Namespace: namespace,
 			Annotations: map[string]string{
-				"acceptAnonymousUploads": "0",
+				"acceptAnonymousUploads": "1",
 			},
 		},
 		Type:       "kubernetes.io/tls",
 		Data:       make(map[string][]byte),
 		StringData: make(map[string]string),
 	}
 
-	altNames := []string{
-		"kotsadm",
-		"kotsadm.default",
-		"kotsadm.default.svc",
-		"kotsadm.default.svc.cluster",
-		"kotsadm.default.svc.cluster.local",
-	}
-
-	hostname := "kotsadm.default.svc.cluster.local"
+	hostname, altNames := generateCertHostnames(namespace)
 
 	// Generate a new self-signed cert
 	certData, keyData, err := certutil.GenerateSelfSignedCertKey(hostname, nil, altNames)
@@ -656,3 +650,30 @@ func generateDefaultCertSecret(secrets corev1.SecretInterface, namespace string)
 
 	return nil
 }
+
+func generateCertHostnames(namespace string) (string, []string) {
+	if namespace == "" {
+		namespace = "default"
+	}
+
+	// Since ecko relies on the hostname being kotsadm.default.svc.cluster.local
+	// we should leave this as-is. Lets also leave the old hostname in the altNames
+	altNames := []string{
+		"kotsadm",
+		"kotsadm.default",
+		"kotsadm.default.svc",
+		"kotsadm.default.svc.cluster",
+		DEFAULT_KOTSADM_CERT_CN,
+	}
+
+	// If the kurl-proxy is deployed in another namespace, add the DNS alt names
+	// to allow verifying a TLS connection to "kotsadm.mynamespace.svc.cluster.local"
+	if namespace != "default" {
+		altNames = append(altNames, fmt.Sprintf("kotsadm.%s", namespace))
+		altNames = append(altNames, fmt.Sprintf("kotsadm.%s.svc", namespace))
+		altNames = append(altNames, fmt.Sprintf("kotsadm.%s.svc.cluster", namespace))
+		altNames = append(altNames, fmt.Sprintf("kotsadm.%s.svc.cluster.local", namespace))
+	}
+
+	return DEFAULT_KOTSADM_CERT_CN, altNames
+}

kurl_proxy/cmd/main_test.go

@@ -1,13 +1,21 @@
 package main
 
 import (
+	"context"
+	"crypto/rsa"
+	"crypto/x509"
+	"encoding/pem"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"os"
 	"path/filepath"
+	"reflect"
 	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
 )
 
 const (
@@ -216,3 +224,99 @@ func Test_httpServerCSPHeaders(t *testing.T) {
 	}
 
 }
+
+func Test_generateDefaultCertSecret(t *testing.T) {
+	client := fake.NewSimpleClientset()
+	ns := "default"
+	secrets := client.CoreV1().Secrets(ns)
+	err := generateDefaultCertSecret(secrets, ns)
+	if err != nil {
+		t.Errorf("generateDefaultCertSecret() error = %v", err)
+	}
+
+	secret, err := secrets.Get(context.Background(), "kotsadm-tls", metav1.GetOptions{})
+	if err != nil {
+		t.Errorf("failed to get secret: %v", err)
+	}
+
+	// Check client cert
+	if secret.Data["tls.crt"] == nil {
+		t.Errorf("expected tls.crt to be set")
+	}
+
+	block, _ := pem.Decode(secret.Data["tls.crt"])
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Errorf("failed to parse tls.crt: %v", err)
+	}
+
+	// Check client key
+	if secret.Data["tls.key"] == nil {
+		t.Errorf("expected tls.key to be set")
+	}
+	block, _ = pem.Decode(secret.Data["tls.key"])
+	key, err := x509.ParsePKCS1PrivateKey(block.Bytes)
+	if err != nil {
+		t.Errorf("failed to parse tls.key: %v", err)
+	}
+
+	if !key.Public().(*rsa.PublicKey).Equal(cert.PublicKey) {
+		t.Errorf("expected public key to be equal")
+	}
+
+	if secret.StringData["hostname"] != "kotsadm.default.svc.cluster.local" {
+		t.Errorf("expected hostname to be set")
+	}
+
+	if secret.Annotations["acceptAnonymousUploads"] != "1" {
+		t.Errorf("expected acceptAnonymousUploads to be set to '1'")
+	}
+}
+
+func Test_generateCertHostnames(t *testing.T) {
+	tests := []struct {
+		name  string
+		namespace string
+		hostname  string
+		altNames []string
+	}{
+		{
+			name:  "with no namespace",
+			hostname:  "kotsadm.default.svc.cluster.local",
+			altNames : []string{
+				"kotsadm",
+				"kotsadm.default",
+				"kotsadm.default.svc",
+				"kotsadm.default.svc.cluster",
+				"kotsadm.default.svc.cluster.local",
+			},
+		},
+		{
+			name:  "with some other namespace",
+			namespace: "somecluster",
+			hostname:  "kotsadm.default.svc.cluster.local",
+			altNames : []string{
+				"kotsadm",
+				"kotsadm.default",
+				"kotsadm.default.svc",
+				"kotsadm.default.svc.cluster",
+				"kotsadm.default.svc.cluster.local",
+				"kotsadm.somecluster",
+				"kotsadm.somecluster.svc",
+				"kotsadm.somecluster.svc.cluster",
+				"kotsadm.somecluster.svc.cluster.local",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hostname, altNames := generateCertHostnames(tt.namespace)
+			if hostname != tt.hostname {
+				t.Errorf("generateCertHostnames() hostname = %v, want %v", hostname, tt.hostname)
+			}
+			if !reflect.DeepEqual(altNames, tt.altNames) {
+				t.Errorf("generateCertHostnames() altNames = %v, want %v", altNames, tt.altNames)
+			}
+		})
+	}
+}

kurl_proxy/go.mod

@@ -17,6 +17,7 @@ require (
 	github.com/chenzhuoyu/base64x v0.0.0-20221115062448-fe3a3abad311 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/emicklei/go-restful/v3 v3.11.0 // indirect
+	github.com/evanphx/json-patch v4.12.0+incompatible // indirect
 	github.com/gabriel-vasile/mimetype v1.4.2 // indirect
 	github.com/gin-contrib/sse v0.1.0 // indirect
 	github.com/go-logr/logr v1.3.0 // indirect

kurl_proxy/go.sum

@@ -10,6 +10,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/emicklei/go-restful/v3 v3.11.0 h1:rAQeMHw1c7zTmncogyy8VvRZwtkmkZ4FxERmMY4rD+g=
 github.com/emicklei/go-restful/v3 v3.11.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
+github.com/evanphx/json-patch v4.12.0+incompatible h1:4onqiflcdA9EOZ4RxV643DvftH5pOlLGNtQ5lPWQu84=
+github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
 github.com/gabriel-vasile/mimetype v1.4.2 h1:w5qFW6JKBz9Y393Y4q372O9A7cUSequkh1Q7OhCmWKU=
 github.com/gabriel-vasile/mimetype v1.4.2/go.mod h1:zApsH/mKG4w07erKIaJPFiX0Tsq9BFQgN3qGY5GnNgA=
 github.com/gin-contrib/sse v0.1.0 h1:Y/yl/+YNO8GZSjAhjMsSuLt29uWRFHdHYUb5lYOV9qE=