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.

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

Introduce min-tls and ciphers-suite application options for webhook server #556

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions cmd/trust-manager/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package app

import (
"crypto/tls"
"errors"
"fmt"
"net/http"
Expand All @@ -28,6 +29,7 @@ import (
"k8s.io/client-go/kubernetes"
clientv1 "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/tools/record"
cliflag "k8s.io/component-base/cli/flag"
"k8s.io/klog/v2"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/cache"
Expand Down Expand Up @@ -89,6 +91,25 @@ func NewCommand() *cobra.Command {
Port: opts.Webhook.Port,
Host: opts.Webhook.Host,
CertDir: opts.Webhook.CertDir,
TLSOpts: []func(*tls.Config){
func(c *tls.Config) {
// Get Minimum TLS version from CLI arguments
c.MinVersion, err = cliflag.TLSVersion(opts.MinTLSVersion)
if err != nil {
log.Error(err, "error parsing minimum TLS version")
}

// Note that TLS 1.3 ciphersuites are not configurable.
if c.MinVersion == tls.VersionTLS13 {
return
}

c.CipherSuites, err = cliflag.TLSCipherSuites(opts.CipherSuite)
if err != nil {
log.Error(err, "error parsing cipher suites")
}
},
},
}),
Metrics: server.Options{
BindAddress: fmt.Sprintf("0.0.0.0:%d", opts.MetricsPort),
Expand Down
259 changes: 259 additions & 0 deletions cmd/trust-manager/app/ciphers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,259 @@
/*
Copyright 2025 The cert-manager Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package app

import (
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rand"
"crypto/rsa"
"crypto/tls"
"crypto/x509"
"crypto/x509/pkix"
"encoding/pem"
"fmt"
"log"
"math/big"
"net"
"net/http"
"os"
"path"
"strings"
"testing"
"time"

"github.com/spf13/cobra"
cliflag "k8s.io/component-base/cli/flag"
"sigs.k8s.io/controller-runtime/pkg/webhook"

"github.com/cert-manager/trust-manager/cmd/trust-manager/app/options"
)

func TestCiphersSuite(t *testing.T) {
tmpDir := t.TempDir()
err := setupWebHookServer(tmpDir)
if err != nil {
t.Fatal(fmt.Errorf("unable to setup webhook server %s\n", err))
}

t.Run("TLS 1.2 client connect to TLS 1.2 server, ciphers suite supported", func(t *testing.T) {
client := http.Client{
Transport: &http.Transport{
ForceAttemptHTTP2: false,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true, //nolint: gosec
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS12,
CipherSuites: []uint16{
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
tls.TLS_RSA_WITH_AES_128_GCM_SHA256,
},
},
},
}

resp, err := client.Get(fmt.Sprintf("https://%s", net.JoinHostPort("localhost", "6443"))) //nolint: noctx
if err != nil {
t.Fatalf("error to connect to webhook server, %s\n", err)
}
resp.Body.Close()
Comment on lines +69 to +73
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: If we call Fatalf the Body will never be closed - as far as I know at least!

Suggested change
resp, err := client.Get(fmt.Sprintf("https://%s", net.JoinHostPort("localhost", "6443"))) //nolint: noctx
if err != nil {
t.Fatalf("error to connect to webhook server, %s\n", err)
}
resp.Body.Close()
resp, err := client.Get(fmt.Sprintf("https://%s", net.JoinHostPort("localhost", "6443"))) //nolint: noctx
defer resp.Body.Close()
if err != nil {
t.Fatalf("error to connect to webhook server, %s\n", err)
}

})

t.Run("TLS 1.2 client connect to TLS 1.2 server, ciphers suite unsupported", func(t *testing.T) {
client := http.Client{
Transport: &http.Transport{
ForceAttemptHTTP2: false,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true, //nolint: gosec
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS12,
CipherSuites: []uint16{
tls.TLS_RSA_WITH_AES_256_CBC_SHA,
},
},
},
}

resp, err := client.Get(fmt.Sprintf("https://%s", net.JoinHostPort("localhost", "6443"))) //nolint: noctx
if err != nil {
return
}
defer resp.Body.Close()
Comment on lines +92 to +95
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: the defer will never get called if we return here; the order should be different. Does that sound right to you?

Also since this doesn't call t.Error this test won't ever fail.

Suggested change
if err != nil {
return
}
defer resp.Body.Close()
defer resp.Body.Close()
if err == nil {
t.Error("expected an error from talking to a server with an unsupported cipher suite but got none")
return
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,
Yes, looks good! Thank you!

})
}

func setupWebHookServer(tmpDir string) error {
err := genPrivAndCert(tmpDir)
if err != nil {
return fmt.Errorf("unable to generate certificate and privkey %s\n", err)
}

args := []string{
"--readiness-probe-port=9443",
"--readiness-probe-path=/readyz",
"--leader-election-lease-duration=15s",
"--leader-election-renew-deadline=10s",
"--metrics-port=9402",
"--trust-namespace=cert-manager",
"--secret-targets-enabled=false",
"--filter-expired-certificates=false",
"--webhook-host=0.0.0.0",
"--webhook-port=6443",
Comment on lines +114 to +115
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Do we need to listen on 0.0.0.0 here? Could we listen on 127.0.0.1 to make it a little safer?

"--tls-min-version=VersionTLS12",
"--tls-cipher-suites=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_128_GCM_SHA256",
}

opts := options.New()

cmd := &cobra.Command{
RunE: func(cmd *cobra.Command, args []string) error {
srv := webhook.NewServer(webhook.Options{
CertDir: tmpDir,
Port: opts.Port,

TLSOpts: []func(*tls.Config){
func(cfg *tls.Config) {
cfg.MinVersion, err = cliflag.TLSVersion(opts.MinTLSVersion)
if err != nil {
cmd.PrintErrf("error parsing minimum TLS version, given %s\n", opts.MinTLSVersion)
}

cfg.CipherSuites, err = cliflag.TLSCipherSuites(opts.CipherSuite)
if err != nil {
cmd.PrintErrf("error parsing cipher suites, given %s\n", opts.CipherSuite)
}
},
},
})

if err != nil {
cmd.PrintErrf("error creating webhook server %s\n", err)
return err
}

go func() {
if err := srv.Start(cmd.Context()); err != nil {
cmd.PrintErrf("error starting webhook server %s\n", err)
}
}()

return nil
},
}
cmd.SetArgs(args)
opts.Prepare(cmd)
err = cmd.Execute()
if err != nil {
cmd.PrintErrf("error: %v\n", err)
return err
}
return nil
}

// This implementation was taken as is from
// https://go.dev/src/crypto/tls/generate_cert.go source file
func genPrivAndCert(dir string) error {
var priv any
var err error
priv, err = rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
return fmt.Errorf("Failed to generate private key: %v", err)
}

keyUsage := x509.KeyUsageDigitalSignature
if _, isRSA := priv.(*rsa.PrivateKey); isRSA {
keyUsage |= x509.KeyUsageKeyEncipherment
}

var notBefore = time.Now()
notAfter := notBefore.Add(365 * 24 * time.Hour)

serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128)
serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
if err != nil {
return fmt.Errorf("Failed to generate serial number: %v", err)
}

template := x509.Certificate{
SerialNumber: serialNumber,
Subject: pkix.Name{
Organization: []string{"Acme Co"},
},
NotBefore: notBefore,
NotAfter: notAfter,

KeyUsage: keyUsage,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
BasicConstraintsValid: true,
}

hosts := strings.Split("localhost, 127.0.0.1", ",")
for _, h := range hosts {
if ip := net.ParseIP(h); ip != nil {
template.IPAddresses = append(template.IPAddresses, ip)
} else {
template.DNSNames = append(template.DNSNames, h)
}
}

derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, publicKey(priv), priv)
if err != nil {
return fmt.Errorf("Failed to create certificate: %v", err)
}

certOut, err := os.Create(path.Join(dir, "tls.crt"))
if err != nil {
return fmt.Errorf("Failed to open cert.pem for writing: %v", err)
}
if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}); err != nil {
return fmt.Errorf("Failed to write data to tls.crt: %v", err)
}
if err := certOut.Close(); err != nil {
return fmt.Errorf("Error closing tls.crt: %v", err)
}
log.Print("wrote tls.crt\n")

keyOut, err := os.OpenFile(path.Join(dir, "tls.key"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
if err != nil {
return fmt.Errorf("Failed to open tls.key for writing: %v", err)
}
privBytes, err := x509.MarshalPKCS8PrivateKey(priv)
if err != nil {
return fmt.Errorf("Unable to marshal private key: %v", err)
}
if err := pem.Encode(keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: privBytes}); err != nil {
return fmt.Errorf("Failed to write data to tls.key: %v", err)
}
if err := keyOut.Close(); err != nil {
return fmt.Errorf("Error closing tls.key: %v", err)
}
log.Print("wrote tls.key\n")
return nil
}

func publicKey(priv any) any {
switch k := priv.(type) {
case *rsa.PrivateKey:
return &k.PublicKey
case *ecdsa.PrivateKey:
return &k.PublicKey
case ed25519.PrivateKey:
return k.Public().(ed25519.PublicKey)
default:
return nil
}
}
37 changes: 37 additions & 0 deletions cmd/trust-manager/app/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ limitations under the License.
package options

import (
"crypto/tls"
"errors"
"fmt"
"log/slog"
"os"
"strings"
"time"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -80,6 +82,24 @@ type Options struct {
log logOptions

LeaderElectionConfig LeaderElectionConfig

// Leader election lease duration
LeaseDuration time.Duration

// Leader election lease renew duration
RenewDeadline time.Duration

// minTLSVersion is the minimum TLS version supported.
// Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants).
// If not specified, the default for the Go version will be used and may change over time.
MinTLSVersion string

// cipherSuites is the list of allowed cipher suites for the webhook server.
// Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants).
// If not specified, the default for the Go version will be used and may change over time.
CipherSuite []string

TLSConfig tls.Config
}

type logOptions struct {
Expand Down Expand Up @@ -173,6 +193,7 @@ func (o *Options) addFlags(cmd *cobra.Command) {
o.addBundleFlags(nfs.FlagSet("Bundle"))
o.addLoggingFlags(nfs.FlagSet("Logging"))
o.addWebhookFlags(nfs.FlagSet("Webhook"))
o.addTLSConfigFlags(nfs.FlagSet("TLSConfig"))
o.kubeConfigFlags = genericclioptions.NewConfigFlags(true)
o.kubeConfigFlags.AddFlags(nfs.FlagSet("Kubernetes"))

Expand Down Expand Up @@ -261,3 +282,19 @@ func (o *Options) addWebhookFlags(fs *pflag.FlagSet) {
"Certificate and private key must be named 'tls.crt' and 'tls.key' "+
"respectively.")
}

func (o *Options) addTLSConfigFlags(fs *pflag.FlagSet) {
tlsPossibleVersions := cliflag.TLSPossibleVersions()
fs.StringVar(&o.MinTLSVersion,
"tls-min-version", "",
"Minimum TLS version supported. "+
"If omitted, the default Go minimum version will be used. "+
"Possible values: "+strings.Join(tlsPossibleVersions, ","))

tlsCipherPossibleValues := cliflag.TLSCipherPossibleValues()
fs.StringSliceVar(&o.CipherSuite,
"tls-cipher-suites", nil,
"Comma-separated list of cipher suites for the server. "+
"If omitted, the default Go cipher suites will be used. "+
"Possible values: "+strings.Join(tlsCipherPossibleValues, ","))
}
14 changes: 14 additions & 0 deletions deploy/charts/trust-manager/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,20 @@ topologySpreadConstraints:
> ```

Whether to filter expired certificates from the trust bundle.
#### **app.minTLSVersion** ~ `string`
> Default value:
> ```yaml
> ""
> ```

Minimum TLS version supported. If omitted, the default Go minimum version will be used.
#### **app.ciphersSuite** ~ `string`
> Default value:
> ```yaml
> ""
> ```

Comma-separated list of cipher suites for the server. If omitted, the default Go cipher suites will be used.
#### **app.logFormat** ~ `string`
> Default value:
> ```yaml
Expand Down
2 changes: 2 additions & 0 deletions deploy/charts/trust-manager/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ spec:
initialDelaySeconds: 3
periodSeconds: 7
args:
- "--tls-min-version={{.Values.app.minTLSVersion}}"
- "--tls-cipher-suites={{.Values.app.ciphersSuite}}"
- "--log-format={{.Values.app.logFormat}}"
- "--log-level={{.Values.app.logLevel}}"
- "--metrics-port={{.Values.app.metrics.port}}"
Expand Down
Loading