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

Router support for mutual tls authentication #19891

Merged
merged 1 commit into from
Jul 12, 2018
Merged
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
8 changes: 8 additions & 0 deletions contrib/completions/bash/oc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions contrib/completions/zsh/oc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 60 additions & 0 deletions images/router/haproxy/conf/haproxy-config.template
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,9 @@ frontend fe_sni
{{- if isTrue (env "ROUTER_STRICT_SNI") }} strict-sni {{ end }}
{{- ""}} crt {{firstMatch ".+" .DefaultCertificate "/var/lib/haproxy/conf/default_pub_keys.pem"}}
{{- ""}} crt-list /var/lib/haproxy/conf/cert_config.map accept-proxy
{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{.}} {{ end }}
{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{.}} {{ end }}
{{- with (env "ROUTER_MUTUAL_TLS_AUTH") }} verify {{.}} {{ end }}
{{- if isTrue (env "ROUTER_ENABLE_HTTP2") }} alpn h2,http/1.1{{ end }}
mode http

Expand All @@ -235,6 +238,37 @@ frontend fe_sni
# before matching, or any requests containing uppercase characters will never match.
http-request set-header Host %[req.hdr(Host),lower]

{{ if ne (env "ROUTER_MUTUAL_TLS_AUTH" "none") "none" }}
{{- with (env "ROUTER_MUTUAL_TLS_AUTH_FILTER") }}
# If a mutual TLS auth subject filter environment variable is set, we deny
# requests if the DN field in the client certificate doesn't match that value.
# Please note that this match is a regular expression match.
# Example: For DN set to: /CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3,
# A. ROUTER_MUTUAL_TLS_AUTH_FILTER="header.test" OR
# ROUTER_MUTUAL_TLS_AUTH_FILTER="head" OR
# ROUTER_MUTUAL_TLS_AUTH_FILTER="^/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3$" /* exact match example */
# the filter would match the DN field (substring or exact match)
# and the request will be passed on to the backend.
# B. ROUTER_MUTUAL_TLS_AUTH_FILTER="legacy-web-client", the request
# will be rejected.
acl cert_cn_matches ssl_c_s_dn -m reg {{.}}
http-request deny unless cert_cn_matches
{{- end }}

# Add X-SSL* headers to pass client certificate information to the backend.
http-request set-header X-SSL %[ssl_fc]
http-request set-header X-SSL-Client-Verify %[ssl_c_verify]
http-request set-header X-SSL-Client-Serial %{+Q}[ssl_c_serial,hex]
http-request set-header X-SSL-Client-Version %{+Q}[ssl_c_version]
http-request set-header X-SSL-Client-SHA1 %{+Q}[ssl_c_sha1,hex]
http-request set-header X-SSL-Client-DN %{+Q}[ssl_c_s_dn]
http-request set-header X-SSL-Client-CN %{+Q}[ssl_c_s_dn(cn)]
http-request set-header X-SSL-Issuer %{+Q}[ssl_c_i_dn]
http-request set-header X-SSL-Client-NotBefore %{+Q}[ssl_c_notbefore]
http-request set-header X-SSL-Client-NotAfter %{+Q}[ssl_c_notafter]
http-request set-header X-SSL-Client-DER %{+Q}[ssl_c_der,base64]
{{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these headers required for edge routes which use plain http backend ? seems just re-encrypt route need them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion either way. But the headers do reflect information about the connection as it was handled on the edge by the router, so it is still relevant request info (irrespective of the type of backend/route) much like the [X-]Forwarded[-*] and Via headers.

# map to backend
# Search from most specific to general path (host case).
# Note: If no match, haproxy uses the default_backend, no other
Expand All @@ -261,6 +295,9 @@ backend be_no_sni
frontend fe_no_sni
# terminate ssl on edge
bind 127.0.0.1:{{env "ROUTER_SERVICE_NO_SNI_PORT" "10443"}} ssl no-sslv3 crt {{firstMatch ".+" .DefaultCertificate "/var/lib/haproxy/conf/default_pub_keys.pem"}} accept-proxy
{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{.}} {{ end }}
{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{.}} {{ end }}
{{- with (env "ROUTER_MUTUAL_TLS_AUTH") }} verify {{.}} {{ end }}
mode http

# Strip off Proxy headers to prevent HTTpoxy (https://httpoxy.org/)
Expand All @@ -270,6 +307,29 @@ frontend fe_no_sni
# before matching, or any requests containing uppercase characters will never match.
http-request set-header Host %[req.hdr(Host),lower]

{{ if ne (env "ROUTER_MUTUAL_TLS_AUTH" "none") "none" }}
{{- with (env "ROUTER_MUTUAL_TLS_AUTH_FILTER") }}
# If a mutual TLS auth subject filter environment variable is set, we deny
# requests if the DN field in the client certificate doesn't match that value.
# Please note that this match is a regular expression match.
# See the config section 'frontend fe_sni' for examples.
acl cert_cn_matches ssl_c_s_dn -m reg {{.}}
http-request deny unless cert_cn_matches
{{- end }}

# Add X-SSL* headers to pass client certificate information to the backend.
http-request set-header X-SSL %[ssl_fc]
http-request set-header X-SSL-Client-Verify %[ssl_c_verify]
http-request set-header X-SSL-Client-Serial %{+Q}[ssl_c_serial,hex]
http-request set-header X-SSL-Client-Version %{+Q}[ssl_c_version]
http-request set-header X-SSL-Client-SHA1 %{+Q}[ssl_c_sha1,hex]
http-request set-header X-SSL-Client-DN %{+Q}[ssl_c_s_dn]
http-request set-header X-SSL-Client-CN %{+Q}[ssl_c_s_dn(cn)]
http-request set-header X-SSL-Issuer %{+Q}[ssl_c_i_dn]
http-request set-header X-SSL-Client-NotBefore %{+Q}[ssl_c_notbefore]
http-request set-header X-SSL-Client-NotAfter %{+Q}[ssl_c_notafter]
http-request set-header X-SSL-Client-DER %{+Q}[ssl_c_der,base64]
{{- end }}

# map to backend
# Search from most specific to general path (host case).
Expand Down
111 changes: 109 additions & 2 deletions pkg/oc/admin/router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/client-go/dynamic"
"k8s.io/kubernetes/pkg/api/legacyscheme"
Expand Down Expand Up @@ -84,6 +85,11 @@ var (
privkeyName = "router.pem"
privkeyPath = secretsPath + "/" + privkeyName

defaultMutualTLSAuth = "none"
clientCertConfigDir = "/etc/pki/tls/client-certs"
clientCertConfigCA = "ca.pem"
clientCertConfigCRL = "crl.pem"

defaultCertificatePath = path.Join(defaultCertificateDir, "tls.crt")
)

Expand Down Expand Up @@ -237,6 +243,23 @@ type RouterConfig struct {
Threads int32

Local bool

// MutualTLSAuth controls access to the router using a mutually agreed
// upon TLS authentication mechanism (example client certificates).
// One of: required | optional | none - the default is none.
MutualTLSAuth string

// MutualTLSAuthCA contains the CA certificates that will be used
// to verify a client's certificate.
MutualTLSAuthCA string

// MutualTLSAuthCRL contains the certificate revocation list used to
// verify a client's certificate.
MutualTLSAuthCRL string

// MutualTLSAuthFilter contains the value to filter requests based on
// a client certificate subject field substring match.
MutualTLSAuthFilter string
Copy link
Contributor

Choose a reason for hiding this comment

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

I still do not think this should be a substring match.
In what case would you want to match only part of a cert without explicitly specifying which part at least ?
Unless there is a very specific reason to use a substring match we should only do an exact match.
Otherwise we risk opening up users to easy attacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent here is to restrict an already validated set of requests. It is important to note here that the connections are already filtered based on the CA signing the certificate (and/or any revoked certificates).

So the ROUTER_MUTUAL_TLS_AUTH_CA and ROUTER_MUTUAL_TLS_AUTH_CRL control the first pass of which connections (or certificates) are allowed and the substring match here is the second pass that is restricting that first block of connections to a smaller subset.

If we restrict that to specific match, then we can really only allow one single certifying authority per router. Multiple orgs/depts/units cannot be serviced by a single router instance.

This allows us flexibility to support that and if you really wanted a further locked down environment, you could just as well make the substring match more restrictive ala:
ROUTER_MUTUAL_TLS_AUTH_FILTER="/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3" which effectively becomes a specific match.

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this match also: /CN=blah/CN=blah/CN=more-blah/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would string substring match that. But that said, is that a valid DN/subject?
From the looks of it the / is a separator in the subject, correct?
So shouldn't that be escaped if someone forces that into an input field? Otherwise, there's potential of other issues, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simo5 also pertinent here is the fact that the ROUTER_MUTUAL_TLS_AUTH_FILTER is optional. Meaning if its not specified, we don't do any restrictive checks. This is a secondary check.

But if this is still a sticking point for you, we could potentially change the substring match to a regular expression ... so you could potentially do a ^<escaped-text>$ kind of expression for an exact match.
That said, it does make the user interface a bit more awkward as it requires the user to specify a regular expression. Not a back breaker though!
And it is worth noting that a regular expression without the positional anchor ... ala CN=foo would still match a /CN=blah/CN=foo/CN=blah/ string.

@knobunc / @ironcladlou any issues with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can have as many CN elements as you want in a DN.
Changing it to a regular expression would be definitely better and I would accept as a solution because then you can do exact matches if you want or anything.

An example with exact match and one with subfiltering should be provided as doc then.

}

const (
Expand Down Expand Up @@ -271,6 +294,8 @@ func NewCmdRouter(f kcmdutil.Factory, parentName, name string, out, errout io.Wr
StatsPort: defaultStatsPort,
HostNetwork: true,
HostPorts: true,

MutualTLSAuth: defaultMutualTLSAuth,
}

cmd := &cobra.Command{
Expand Down Expand Up @@ -325,15 +350,25 @@ func NewCmdRouter(f kcmdutil.Factory, parentName, name string, out, errout io.Wr
cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")
cmd.Flags().Int32Var(&cfg.Threads, "threads", cfg.Threads, "Specifies the number of threads for the haproxy router.")

cmd.Flags().StringVar(&cfg.MutualTLSAuth, "mutual-tls-auth", cfg.MutualTLSAuth, "Controls access to the router using mutually agreed upon TLS configuration (example client certificates). You can choose one of 'required', 'optional', or 'none'. The default is none.")
cmd.Flags().StringVar(&cfg.MutualTLSAuthCA, "mutual-tls-auth-ca", cfg.MutualTLSAuthCA, "Optional path to a file containing one or more CA certificates used for mutual TLS authentication. The CA certificate[s] are used by the router to verify a client's certificate.")
cmd.Flags().StringVar(&cfg.MutualTLSAuthCRL, "mutual-tls-auth-crl", cfg.MutualTLSAuthCRL, "Optional path to a file containing the certificate revocation list used for mutual TLS authentication. The certificate revocation list is used by the router to verify a client's certificate.")
cmd.Flags().StringVar(&cfg.MutualTLSAuthFilter, "mutual-tls-auth-filter", cfg.MutualTLSAuthFilter, "Optional regular expression to filter the client certificates. If the client certificate subject field does _not_ match this regular expression, requests will be rejected by the router.")

cfg.Action.BindForOutput(cmd.Flags())
cmd.Flags().String("output-version", "", "The preferred API versions of the output objects")

return cmd
}

// generateMutualTLSSecretName generates a mutual TLS auth secret name.
func generateMutualTLSSecretName(prefix string) string {
return fmt.Sprintf("%s-mutual-tls-auth", prefix)
}

// generateSecretsConfig generates any Secret and Volume objects, such
// as SSH private keys, that are necessary for the router container.
func generateSecretsConfig(cfg *RouterConfig, namespace string, defaultCert []byte, certName string) ([]*kapi.Secret, []kapi.Volume, []kapi.VolumeMount, error) {
func generateSecretsConfig(cfg *RouterConfig, namespace, certName string, defaultCert, mtlsAuthCA, mtlsAuthCRL []byte) ([]*kapi.Secret, []kapi.Volume, []kapi.VolumeMount, error) {
var secrets []*kapi.Secret
var volumes []kapi.Volume
var mounts []kapi.VolumeMount
Expand Down Expand Up @@ -440,6 +475,42 @@ func generateSecretsConfig(cfg *RouterConfig, namespace string, defaultCert []by
}
mounts = append(mounts, mount)

mtlsSecretData := map[string][]byte{}
if len(mtlsAuthCA) > 0 {
mtlsSecretData[clientCertConfigCA] = mtlsAuthCA
}
if len(mtlsAuthCRL) > 0 {
mtlsSecretData[clientCertConfigCRL] = mtlsAuthCRL
}

if len(mtlsSecretData) > 0 {
secretName := generateMutualTLSSecretName(cfg.Name)
secret := &kapi.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
},
Data: mtlsSecretData,
}
secrets = append(secrets, secret)

volume := kapi.Volume{
Name: "mutual-tls-config",
VolumeSource: kapi.VolumeSource{
Secret: &kapi.SecretVolumeSource{
SecretName: secretName,
},
},
}
volumes = append(volumes, volume)

mount := kapi.VolumeMount{
Name: volume.Name,
ReadOnly: true,
MountPath: clientCertConfigDir,
}
mounts = append(mounts, mount)
}

return secrets, volumes, mounts, nil
}

Expand Down Expand Up @@ -608,6 +679,14 @@ func RunCmdRouter(f kcmdutil.Factory, cmd *cobra.Command, out, errout io.Writer,
if err != nil {
return fmt.Errorf("error getting client: %v", err)
}

if len(cfg.MutualTLSAuthCA) > 0 || len(cfg.MutualTLSAuthCRL) > 0 {
secretName := generateMutualTLSSecretName(cfg.Name)
if _, err := kClient.Core().Secrets(namespace).Get(secretName, metav1.GetOptions{}); err == nil {
return fmt.Errorf("router could not be created: mutual tls secret %q already exists", secretName)
}
}

service, err := kClient.Core().Services(namespace).Get(name, metav1.GetOptions{})
if err != nil {
if !generate {
Expand Down Expand Up @@ -660,6 +739,20 @@ func RunCmdRouter(f kcmdutil.Factory, cmd *cobra.Command, out, errout io.Writer,
return fmt.Errorf("router could not be created; error reading default certificate file: %v", err)
}

mtlsAuthOptions := []string{"required", "optional", "none"}
allowedMutualTLSAuthOptions := sets.NewString(mtlsAuthOptions...)
if !allowedMutualTLSAuthOptions.Has(cfg.MutualTLSAuth) {
return fmt.Errorf("invalid mutual tls auth option %v, expected one of %v", cfg.MutualTLSAuth, mtlsAuthOptions)
}
mtlsAuthCA, err := fileutil.LoadData(cfg.MutualTLSAuthCA)
if err != nil {
return fmt.Errorf("reading ca certificates for mutual tls auth: %v", err)
}
mtlsAuthCRL, err := fileutil.LoadData(cfg.MutualTLSAuthCRL)
if err != nil {
return fmt.Errorf("reading certificate revocation list for mutual tls auth: %v", err)
}

if len(cfg.StatsPassword) == 0 {
cfg.StatsPassword = generateStatsPassword()
if !cfg.Action.ShouldPrint() {
Expand Down Expand Up @@ -719,6 +812,20 @@ func RunCmdRouter(f kcmdutil.Factory, cmd *cobra.Command, out, errout io.Writer,
env["ROUTER_METRICS_TLS_CERT_FILE"] = "/etc/pki/tls/metrics/tls.crt"
env["ROUTER_METRICS_TLS_KEY_FILE"] = "/etc/pki/tls/metrics/tls.key"
}
mtlsAuth := strings.TrimSpace(cfg.MutualTLSAuth)
if len(mtlsAuth) > 0 && mtlsAuth != defaultMutualTLSAuth {
env["ROUTER_MUTUAL_TLS_AUTH"] = cfg.MutualTLSAuth
if len(mtlsAuthCA) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it use the system store if the CA flag is not set? Otherwise it should be required when mTLS is enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The defaults section in the haproxy-config.template sets the default ca-base to the system store which gets used if one is not specified on the bind directive.

env["ROUTER_MUTUAL_TLS_AUTH_CA"] = path.Join(clientCertConfigDir, clientCertConfigCA)
}
if len(mtlsAuthCRL) > 0 {
env["ROUTER_MUTUAL_TLS_AUTH_CRL"] = path.Join(clientCertConfigDir, clientCertConfigCRL)
}
if len(cfg.MutualTLSAuthFilter) > 0 {
env["ROUTER_MUTUAL_TLS_AUTH_FILTER"] = strings.Replace(cfg.MutualTLSAuthFilter, " ", "\\ ", -1)
}
}

env.Add(secretEnv)
if len(defaultCert) > 0 {
if cfg.SecretsAsEnv {
Expand All @@ -729,7 +836,7 @@ func RunCmdRouter(f kcmdutil.Factory, cmd *cobra.Command, out, errout io.Writer,
}
env.Add(app.Environment{"DEFAULT_CERTIFICATE_DIR": defaultCertificateDir})
var certName = fmt.Sprintf("%s-certs", cfg.Name)
secrets, volumes, routerMounts, err := generateSecretsConfig(cfg, namespace, defaultCert, certName)
secrets, volumes, routerMounts, err := generateSecretsConfig(cfg, namespace, certName, defaultCert, mtlsAuthCA, mtlsAuthCRL)
if err != nil {
return fmt.Errorf("router could not be created: %v", err)
}
Expand Down