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

feature: allow overriding tls config based on client hello #3303

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
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
18 changes: 18 additions & 0 deletions dataclients/kubernetes/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const (
skipperLoadBalancerAnnotationKey = "zalando.org/skipper-loadbalancer"
skipperBackendProtocolAnnotationKey = "zalando.org/skipper-backend-protocol"
pathModeAnnotationKey = "zalando.org/skipper-ingress-path-mode"
tlsClientAuthAnnotationKey = "zalando.org/skipper-client-auth"
ingressOriginName = "ingress"
tlsSecretType = "kubernetes.io/tls"
tlsSecretDataCrt = "tls.crt"
Expand All @@ -38,6 +39,7 @@ type ingressContext struct {
extraRoutes []*eskip.Route
backendWeights map[string]float64
pathMode PathMode
tlsClientAuth tls.ClientAuthType
redirect *redirectInfo
hostRoutes map[string][]*eskip.Route
defaultFilters defaultFilters
Expand All @@ -51,6 +53,7 @@ type ingress struct {
allowedExternalNames []*regexp.Regexp
kubernetesEastWestDomain string
pathMode PathMode
clientAuth tls.ClientAuthType
httpsRedirectCode int
kubernetesEnableEastWest bool
provideHTTPSRedirect bool
Expand Down Expand Up @@ -291,6 +294,21 @@ func pathMode(m *definitions.Metadata, globalDefault PathMode, logger *logger) P
return pathMode
}

// parse tls client auth type from annotation or fallback to global default
func tlsClientAuth(m *definitions.Metadata, globalDefault tls.ClientAuthType, logger *logger) tls.ClientAuthType {
clientAuth := globalDefault

if clientAuthString, ok := m.Annotations[tlsClientAuthAnnotationKey]; ok {
if c, err := ParseTLSClientAuth(clientAuthString); err != nil {
logger.Errorf("Failed to get tls client auth: %v", err)
} else {
logger.Debugf("Set tls client auth to %s", c)
clientAuth = c
}
}
return clientAuth
}

func (ing *ingress) addCatchAllRoutes(host string, r *eskip.Route, redirect *redirectInfo) []*eskip.Route {
catchAll := &eskip.Route{
Id: routeID("", "catchall", host, "", ""),
Expand Down
8 changes: 7 additions & 1 deletion dataclients/kubernetes/ingressv1.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,12 @@ func (ing *ingress) addSpecIngressTLSV1(ic *ingressContext, ingtls *definitions.
ic.logger.Errorf("Failed to find secret %s in namespace %s", secretID.Name, secretID.Namespace)
return
}
addTLSCertToRegistry(ic.certificateRegistry, ic.logger, hostlist, secret)

// Set tls config for all hosts defined in the ingress
tlsConfig := &certregistry.Config{
ClientAuth: ic.tlsClientAuth,
}
addTLSConfigToRegistry(ic.certificateRegistry, ic.logger, hostlist, tlsConfig, secret)
}

// converts the default backend if any
Expand Down Expand Up @@ -435,6 +440,7 @@ func (ing *ingress) ingressV1Route(
extraRoutes: extraRoutes(i.Metadata),
backendWeights: backendWeights(i.Metadata, logger),
pathMode: pathMode(i.Metadata, ing.pathMode, logger),
tlsClientAuth: tlsClientAuth(i.Metadata, ing.clientAuth, logger),
redirect: redirect,
hostRoutes: hostRoutes,
defaultFilters: df,
Expand Down
30 changes: 25 additions & 5 deletions dataclients/kubernetes/kube.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package kubernetes

import (
"crypto/tls"
"fmt"
"net"
"net/http"
Expand Down Expand Up @@ -383,6 +384,25 @@ func ParsePathMode(s string) (PathMode, error) {
}
}

// ParseTLSClientAuth parses the string representations of different
// client auth types.
func ParseTLSClientAuth(s string) (tls.ClientAuthType, error) {
switch s {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
switch s {
switch strings.ToLower(s) {

just an idea to support casing as well

case "noclientcert":
return tls.NoClientCert, nil
case "requestclientcert":
return tls.RequestClientCert, nil
case "requireanyclientcert":
return tls.RequireAnyClientCert, nil
case "verifyclientcertifgiven":
return tls.VerifyClientCertIfGiven, nil
case "requireandverifyclientcert":
return tls.RequireAndVerifyClientCert, nil
default:
return 0, fmt.Errorf("invalid client auth type string: %s", s)
}
}

func mapRoutes(routes []*eskip.Route) (map[string]*eskip.Route, []*eskip.Route) {
var uniqueRoutes []*eskip.Route
routesById := make(map[string]*eskip.Route)
Expand Down Expand Up @@ -603,18 +623,18 @@ func compareStringList(a, b []string) []string {
return c
}

// addTLSCertToRegistry adds a TLS certificate to the certificate registry per host using the provided
// Kubernetes TLS secret
func addTLSCertToRegistry(cr *certregistry.CertRegistry, logger *logger, hosts []string, secret *secret) {
// addTLSConfigToRegistry adds a TLS Config to the registry per host using the provided config and secret.
func addTLSConfigToRegistry(cr *certregistry.CertRegistry, logger *logger, hosts []string, config *certregistry.Config, secret *secret) {
cert, err := generateTLSCertFromSecret(secret)
if err != nil {
logger.Errorf("Failed to generate TLS certificate from secret: %v", err)
return
}
for _, host := range hosts {
err := cr.ConfigureCertificate(host, cert)
config.Certificate = *cert
err := cr.SetTLSConfig(host, config)
if err != nil {
logger.Errorf("Failed to configure certificate: %v", err)
logger.Errorf("Failed to configure TLS config: %v", err)
}
}
}
4 changes: 3 additions & 1 deletion dataclients/kubernetes/routegroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,9 @@ func (r *routeGroups) addRouteGroupTLS(ctx *routeGroupContext, tls *definitions.
ctx.logger.Errorf("Failed to find secret %s in namespace %s", secretID.Name, secretID.Namespace)
return
}
addTLSCertToRegistry(ctx.certificateRegistry, ctx.logger, hostlist, secret)

config := &certregistry.Config{}
addTLSConfigToRegistry(ctx.certificateRegistry, ctx.logger, hostlist, config, secret)

}

Expand Down
107 changes: 80 additions & 27 deletions secrets/certregistry/certregistry.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package certregistry

import (
"bytes"
"crypto/tls"
"crypto/x509"
"fmt"
Expand All @@ -9,62 +10,114 @@ import (
log "github.com/sirupsen/logrus"
)

// CertRegistry object holds TLS certificates to be used to terminate TLS connections
// Config holds a certificate registry TLS configuration.
type Config struct {
ClientAuth tls.ClientAuthType
Certificate tls.Certificate
}

// CertRegistry object holds TLS Config to be used to terminate TLS connections
// ensuring synchronized access to them.
type CertRegistry struct {
mu sync.Mutex
lookup map[string]*tls.Certificate
lookup map[string]*tlsConfigWrapper

// defaultTLSConfig is TLS config to be used as a base config for all host configs.
defaultConfig *tls.Config
}

// tlsConfigWrapper holds the tls.Config and a hash of a host configuration.
type tlsConfigWrapper struct {
config *tls.Config
hash []byte
}

// NewCertRegistry initializes the certificate registry.
func NewCertRegistry() *CertRegistry {
l := make(map[string]*tls.Certificate)
l := make(map[string]*tlsConfigWrapper)

return &CertRegistry{
lookup: l,
lookup: l,
defaultConfig: &tls.Config{},
}
}

// Configures certificate for the host if no configuration exists or
// if certificate is valid (`NotBefore` field) after previously configured certificate.
func (r *CertRegistry) ConfigureCertificate(host string, cert *tls.Certificate) error {
if cert == nil {
return fmt.Errorf("cannot configure nil certificate")
// Configures TLS for the host if no configuration exists or
// if config certificate is valid (`NotBefore` field) after previously configured certificate.
func (r *CertRegistry) SetTLSConfig(host string, config *Config) error {
if config == nil {
return fmt.Errorf("cannot configure nil tls config")
}
// loading parsed leaf certificate to certificate
leaf, err := x509.ParseCertificate(cert.Certificate[0])
leaf, err := x509.ParseCertificate(config.Certificate.Certificate[0])
if err != nil {
return fmt.Errorf("failed parsing leaf certificate: %w", err)
}
cert.Leaf = leaf
config.Certificate.Leaf = leaf

// Get tls.config and hash from the config
tlsConfig, configHash := r.configToTLSConfig(config)

r.mu.Lock()
defer r.mu.Unlock()

// Check if the config is already set
curr, found := r.lookup[host]
if found {
if cert.Leaf.NotBefore.After(curr.Leaf.NotBefore) {
log.Infof("updating certificate in registry - %s", host)
r.lookup[host] = cert
return nil
} else {
if found && bytes.Equal(curr.hash, configHash) {
return nil
}

if found && !bytes.Equal(curr.hash, configHash) {
if !config.Certificate.Leaf.NotBefore.After(curr.config.Certificates[0].Leaf.NotBefore) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

problem: this causes config to be skipped when separate ingresses have different annotations. We'd need to set the annotation for all hosts regardless of the ingress.

return nil
}
} else {
log.Infof("adding certificate to registry - %s", host)
r.lookup[host] = cert
return nil
}

log.Infof("setting tls config in registry - %s", host)
wrapper := &tlsConfigWrapper{
config: tlsConfig,
hash: configHash,
}
r.lookup[host] = wrapper

return nil
}

// GetCertFromHello reads the SNI from a TLS client and returns the appropriate certificate.
// If no certificate is found for the host it will return nil.
func (r *CertRegistry) GetCertFromHello(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
// GetConfigFromHello reads the SNI from a TLS client and returns the appropriate config.
func (r *CertRegistry) GetConfigFromHello(hello *tls.ClientHelloInfo) (*tls.Config, error) {
r.mu.Lock()
cert, found := r.lookup[hello.ServerName]
entry, found := r.lookup[hello.ServerName]
r.mu.Unlock()
if found {
return cert, nil
return entry.config, nil
}
return nil, nil
return entry.config, nil
}

// configToTLSConfig converts a Config to a tls.Config and returns the hash of the config.
func (r *CertRegistry) configToTLSConfig(config *Config) (*tls.Config, []byte) {
if config == nil {
return nil, nil
}

var hash []byte

tlsConfig := r.defaultConfig.Clone()

// Add client auth settings
tlsConfig.ClientAuth = config.ClientAuth
hash = append(hash, byte(config.ClientAuth>>8), byte(config.ClientAuth))

// Add certificate
tlsConfig.Certificates = append(tlsConfig.Certificates, config.Certificate)
for _, certData := range config.Certificate.Certificate {
hash = append(hash, certData...)
}

return tlsConfig, hash
}

// SetDefaultTLSConfig sets the default TLS config which should be used as a base for all host specific configs.
func (r *CertRegistry) SetDefaultTLSConfig(config *tls.Config) {
r.defaultConfig = config
}
10 changes: 5 additions & 5 deletions secrets/certregistry/certregistry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,11 @@ func TestCertRegistry(t *testing.T) {
t.Run("sync new certificate", func(t *testing.T) {
cr := NewCertRegistry()
cr.ConfigureCertificate(validHostname, validCert)
cert, found := cr.lookup[validHostname]
entry, found := cr.lookup[validHostname]
if !found {
t.Error("failed to read certificate")
}
if cert.Leaf == nil {
if entry.Certificate.Leaf == nil {
t.Error("synced cert should have a parsed leaf")
}
})
Expand All @@ -178,10 +178,10 @@ func TestCertRegistry(t *testing.T) {

cr := NewCertRegistry()
cr.ConfigureCertificate(validHostname, validCert)
cert1 := cr.lookup[validHostname]
entry1 := cr.lookup[validHostname]
cr.ConfigureCertificate(validHostname, newValidCert)
cert2 := cr.lookup[validHostname]
if equalCert(cert1, cert2) {
entry2 := cr.lookup[validHostname]
if equalCert(entry1.Certificate, entry2.Certificate) {
t.Error("host cert was not updated")
}

Expand Down
3 changes: 2 additions & 1 deletion skipper.go
Original file line number Diff line number Diff line change
Expand Up @@ -1210,7 +1210,8 @@ func (o *Options) tlsConfig(cr *certregistry.CertRegistry) (*tls.Config, error)
}

if cr != nil {
config.GetCertificate = cr.GetCertFromHello
cr.SetDefaultTLSConfig(config)
config.GetConfigForClient = cr.GetConfigFromHello
Copy link
Contributor Author

Choose a reason for hiding this comment

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

problem: the tls.Config from GetConfigForClient will override the previously used GetCertificate. So the two are mutually exclusive, from the docs:

Server configurations must set one of Certificates, GetCertificate or GetConfigForClient.

Copy link
Member

Choose a reason for hiding this comment

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

Check if the new one is non nil and if so only use that one, else fallback to current behavior.
Does it make sense?

Copy link
Contributor Author

@rickhlx rickhlx Nov 11, 2024

Choose a reason for hiding this comment

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

Makes sense.

I was also thinking we could also use GetConfigForClient only. This would allow us to use custom certs and custom TLS config. Config could be set to a "default" based on global skipper flags allowing it to be overridden. This approach shouldn't change the current behavior since it will effectively be the same functionality. Wdyt?

}

if o.CertPathTLS == "" && o.KeyPathTLS == "" {
Expand Down