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

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

Merged
merged 3 commits into from
Jan 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
387fba0
improvement: set acceptAnonymousUploads=1 for initial kotsadm-tls cert
banjoh Jan 11, 2024
ca52a28
Merge branch 'main' into em/sc-94724/set-acceptAnonymousUploads-to-1
banjoh Jan 11, 2024
1eba4b9
Changes as per PR comments
banjoh Jan 12, 2024
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
67 changes: 44 additions & 23 deletions kurl_proxy/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ import (
"encoding/pem"
"fmt"
"html/template"
"io/ioutil"
"io"
"log"
"math/rand"
"net"
"net/http"
"net/http/httputil"
Expand Down Expand Up @@ -44,6 +43,8 @@ var (
}
)

const DEFAULT_KOTSADM_CERT_CN = "kotsadm.default.svc.cluster.local"

type cert struct {
tlsCert tls.Certificate
fingerprint string
Expand All @@ -53,8 +54,6 @@ type cert struct {
func main() {
log.Printf("Commit %s\n", os.Getenv("COMMIT"))

rand.Seed(time.Now().UnixNano())
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why remove this?

Accidental clean up as I was removing deprecated code. I meant to put it back.

We however do not need it. It was deprecated in go1.20. The runtime automatically generates a seed for us. Also, I do not see anywhere where we use random numbers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we are safe to remove this unless there are objections.

I reviewed the code and do not see any reason for keeping it


upstreamOrigin := os.Getenv("UPSTREAM_ORIGIN")
dexUpstreamOrigin := os.Getenv("DEX_UPSTREAM_ORIGIN")
tlsSecretName := os.Getenv("TLS_SECRET_NAME")
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure. on the surface, it seems like we dont need a restart for anything

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave that behaviour for now, but leave the TODO there so as not to introduce some unwanted behaviour change.

// Leaving as-is for now to avoid unwanted regressions
return
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
104 changes: 104 additions & 0 deletions kurl_proxy/cmd/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (
Expand Down Expand Up @@ -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)
}
})
}
}
1 change: 1 addition & 0 deletions kurl_proxy/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions kurl_proxy/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
Loading