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 4 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: 8 additions & 0 deletions dataclients/kubernetes/ingressv1.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package kubernetes

import (
"crypto/tls"
"errors"
"fmt"
"regexp"
Expand Down Expand Up @@ -318,6 +319,12 @@ func (ing *ingress) addSpecIngressTLSV1(ic *ingressContext, ingtls *definitions.
return
}
addTLSCertToRegistry(ic.certificateRegistry, ic.logger, hostlist, secret)

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

// converts the default backend if any
Expand Down Expand Up @@ -435,6 +442,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
29 changes: 29 additions & 0 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 @@ -618,3 +638,12 @@ func addTLSCertToRegistry(cr *certregistry.CertRegistry, logger *logger, hosts [
}
}
}

func addTLSConfigToRegistry(cr *certregistry.CertRegistry, logger *logger, hosts []string, tlsConfig *tls.Config) {
for _, host := range hosts {
err := cr.ConfigureTLSConfig(host, tlsConfig)
if err != nil {
logger.Errorf("Failed to configure TLS config: %v", err)
}
}
}
55 changes: 48 additions & 7 deletions secrets/certregistry/certregistry.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,22 @@ import (
log "github.com/sirupsen/logrus"
)

// CertRegistryEntry holds a certificate and tls configuration.
type CertRegistryEntry struct {
Certificate *tls.Certificate
Config *tls.Config
}

// CertRegistry object holds TLS certificates 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]*CertRegistryEntry
}

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

return &CertRegistry{
lookup: l,
Expand All @@ -43,16 +49,40 @@ func (r *CertRegistry) ConfigureCertificate(host string, cert *tls.Certificate)

curr, found := r.lookup[host]
if found {
if cert.Leaf.NotBefore.After(curr.Leaf.NotBefore) {
if cert.Leaf.NotBefore.After(curr.Certificate.Leaf.NotBefore) {
log.Infof("updating certificate in registry - %s", host)
r.lookup[host] = cert
r.lookup[host].Certificate = cert
return nil
} else {
return nil
}
} else {
log.Infof("adding certificate to registry - %s", host)
r.lookup[host] = cert
r.lookup[host].Certificate = cert
return nil
}
}

// ConfigureTLSConfig configures a tls config for the host.
func (r *CertRegistry) ConfigureTLSConfig(host string, config *tls.Config) error {
if config == nil {
return fmt.Errorf("cannot configure nil tls config")
}

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

curr, found := r.lookup[host]

if found && curr.Config != nil {
log.Infof("updating tls config for host - %s", host)
r.lookup[host].Config = config
return nil
} else {
log.Infof("adding tls config for host - %s", host)
r.lookup[host] = &CertRegistryEntry{
Config: config,
}
return nil
}
}
Expand All @@ -61,10 +91,21 @@ func (r *CertRegistry) ConfigureCertificate(host string, cert *tls.Certificate)
// If no certificate is found for the host it will return nil.
func (r *CertRegistry) GetCertFromHello(hello *tls.ClientHelloInfo) (*tls.Certificate, 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.Certificate, nil
}
return nil, nil
}

// 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()
entry, found := r.lookup[hello.ServerName]
r.mu.Unlock()
if found {
return entry.Config, nil
}
return entry.Config, nil
}
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
1 change: 1 addition & 0 deletions skipper.go
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,7 @@ func (o *Options) tlsConfig(cr *certregistry.CertRegistry) (*tls.Config, error)

if cr != nil {
config.GetCertificate = cr.GetCertFromHello
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
Loading