From e8638ae594a7668805e2489198ceaabea8d05c6c Mon Sep 17 00:00:00 2001 From: Phil Cameron Date: Tue, 13 Jun 2017 15:04:24 -0400 Subject: [PATCH] Donot serve certificate content for Non-SSL routes By default, when a host does not resolve to a route in a HTTPS or tls sni request, the default cert is returned to the caller as part of the 503 response. This exposes the default cert and may pose security concerns. Haproxy strict-sni option to bind suppresses use of the default cert. This adds a new environment variable to the router deployment controller, ROUTER_STRICT_SNI, to control bind processing. When set to "true" or "TRUE", "strict-sni" is added to the bind. Default is "false". oc adm router --strict-sni sets ROUTER_STRICT_SNI="true" bug 1369865 https://bugzilla.redhat.com/show_bug.cgi?id=1369865 --- contrib/completions/bash/oadm | 2 ++ contrib/completions/bash/oc | 2 ++ contrib/completions/bash/openshift | 6 ++++++ contrib/completions/zsh/oadm | 2 ++ contrib/completions/zsh/oc | 2 ++ contrib/completions/zsh/openshift | 6 ++++++ images/router/haproxy/conf/haproxy-config.template | 5 ++++- pkg/cmd/admin/router/router.go | 7 +++++++ pkg/cmd/infra/router/template.go | 3 +++ pkg/router/template/plugin.go | 1 + test/cmd/router.sh | 2 ++ 11 files changed, 37 insertions(+), 1 deletion(-) diff --git a/contrib/completions/bash/oadm b/contrib/completions/bash/oadm index 8b8c4a88cd80..d287dd0fc5e1 100644 --- a/contrib/completions/bash/oadm +++ b/contrib/completions/bash/oadm @@ -4856,6 +4856,8 @@ _oadm_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") diff --git a/contrib/completions/bash/oc b/contrib/completions/bash/oc index e4618951a651..3b3e77173f36 100644 --- a/contrib/completions/bash/oc +++ b/contrib/completions/bash/oc @@ -4857,6 +4857,8 @@ _oc_adm_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") diff --git a/contrib/completions/bash/openshift b/contrib/completions/bash/openshift index d264e346e832..0036051a1056 100644 --- a/contrib/completions/bash/openshift +++ b/contrib/completions/bash/openshift @@ -4856,6 +4856,8 @@ _openshift_admin_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") @@ -10035,6 +10037,8 @@ _openshift_cli_adm_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") @@ -23380,6 +23384,8 @@ _openshift_infra_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--template=") local_nonpersistent_flags+=("--template=") flags+=("--token=") diff --git a/contrib/completions/zsh/oadm b/contrib/completions/zsh/oadm index df75e31684e8..ee26454c531a 100644 --- a/contrib/completions/zsh/oadm +++ b/contrib/completions/zsh/oadm @@ -5005,6 +5005,8 @@ _oadm_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") diff --git a/contrib/completions/zsh/oc b/contrib/completions/zsh/oc index 19d55959568a..ed6e3367d751 100644 --- a/contrib/completions/zsh/oc +++ b/contrib/completions/zsh/oc @@ -5006,6 +5006,8 @@ _oc_adm_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") diff --git a/contrib/completions/zsh/openshift b/contrib/completions/zsh/openshift index 8d01d1adb49e..f63530eb28f0 100644 --- a/contrib/completions/zsh/openshift +++ b/contrib/completions/zsh/openshift @@ -5005,6 +5005,8 @@ _openshift_admin_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") @@ -10184,6 +10186,8 @@ _openshift_cli_adm_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--subdomain=") local_nonpersistent_flags+=("--subdomain=") flags+=("--type=") @@ -23529,6 +23533,8 @@ _openshift_infra_router() local_nonpersistent_flags+=("--stats-port=") flags+=("--stats-user=") local_nonpersistent_flags+=("--stats-user=") + flags+=("--strict-sni") + local_nonpersistent_flags+=("--strict-sni") flags+=("--template=") local_nonpersistent_flags+=("--template=") flags+=("--token=") diff --git a/images/router/haproxy/conf/haproxy-config.template b/images/router/haproxy/conf/haproxy-config.template index eb997bd7259c..c339a86d766f 100644 --- a/images/router/haproxy/conf/haproxy-config.template +++ b/images/router/haproxy/conf/haproxy-config.template @@ -204,7 +204,10 @@ backend be_sni frontend fe_sni # terminate ssl on edge - bind 127.0.0.1:{{env "ROUTER_SERVICE_SNI_PORT" "10444"}} ssl no-sslv3 {{ if gt (len .DefaultCertificate) 0 }}crt {{.DefaultCertificate}}{{ else }}crt /var/lib/haproxy/conf/default_pub_keys.pem{{ end }} crt-list /var/lib/haproxy/conf/cert_config.map accept-proxy + bind 127.0.0.1:{{env "ROUTER_SERVICE_SNI_PORT" "10444"}} ssl no-sslv3 + {{- if matchPattern "true|TRUE" (env "ROUTER_STRICT_SNI" "") }} strict-sni {{ end }} + {{- if gt (len .DefaultCertificate) 0 }} crt {{.DefaultCertificate}}{{ else }} crt /var/lib/haproxy/conf/default_pub_keys.pem{{ end }} + {{- ""}} crt-list /var/lib/haproxy/conf/cert_config.map accept-proxy mode http # check re-encrypt backends first - from most specific to general path. diff --git a/pkg/cmd/admin/router/router.go b/pkg/cmd/admin/router/router.go index ce8b9e9755ae..76a52d0131bd 100644 --- a/pkg/cmd/admin/router/router.go +++ b/pkg/cmd/admin/router/router.go @@ -230,6 +230,9 @@ type RouterConfig struct { // Ciphers is the set of ciphers to use with bind // modern | intermediate | old | set of cihers Ciphers string + + // Strict SNI (do not use default cert) + StrictSNI bool } const ( @@ -310,6 +313,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io. cmd.Flags().BoolVar(&cfg.DisableNamespaceOwnershipCheck, "disable-namespace-ownership-check", cfg.DisableNamespaceOwnershipCheck, "Disables the namespace ownership check and allows different namespaces to claim either different paths to a route host or overlapping host names in case of a wildcard route. The default behavior (false) to restrict claims to the oldest namespace that has claimed either the host or the subdomain. Please be aware that if namespace ownership checks are disabled, routes in a different namespace can use this mechanism to 'steal' sub-paths for existing domains. This is only safe if route creation privileges are restricted, or if all the users can be trusted.") cmd.Flags().StringVar(&cfg.MaxConnections, "max-connections", cfg.MaxConnections, "Specifies the maximum number of concurrent connections. Not supported for F5.") cmd.Flags().StringVar(&cfg.Ciphers, "ciphers", cfg.Ciphers, "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list. Not supported for F5.") + cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.") cfg.Action.BindForOutput(cmd.Flags()) cmd.Flags().String("output-version", "", "The preferred API versions of the output objects") @@ -664,6 +668,9 @@ func RunCmdRouter(f *clientcmd.Factory, cmd *cobra.Command, out, errout io.Write if cfg.DisableNamespaceOwnershipCheck { env["ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK"] = "true" } + if cfg.StrictSNI { + env["ROUTER_STRICT_SNI"] = "true" + } if len(cfg.RouterCanonicalHostname) > 0 { if errs := validation.IsDNS1123Subdomain(cfg.RouterCanonicalHostname); len(errs) != 0 { return fmt.Errorf("invalid canonical hostname (RFC 1123): %s", cfg.RouterCanonicalHostname) diff --git a/pkg/cmd/infra/router/template.go b/pkg/cmd/infra/router/template.go index b0d91aeaa9d3..cb32fcb71fbd 100644 --- a/pkg/cmd/infra/router/template.go +++ b/pkg/cmd/infra/router/template.go @@ -72,6 +72,7 @@ type TemplateRouter struct { BindPortsAfterSync bool MaxConnections string Ciphers string + StrictSNI bool MetricsType string } @@ -102,6 +103,7 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) { flag.BoolVar(&o.BindPortsAfterSync, "bind-ports-after-sync", util.Env("ROUTER_BIND_PORTS_AFTER_SYNC", "") == "true", "Bind ports only after route state has been synchronized") flag.StringVar(&o.MaxConnections, "max-connections", util.Env("ROUTER_MAX_CONNECTIONS", ""), "Specifies the maximum number of concurrent connections.") flag.StringVar(&o.Ciphers, "ciphers", util.Env("ROUTER_CIPHERS", ""), "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list.") + flag.BoolVar(&o.StrictSNI, "strict-sni", util.Env("ROUTER_STRICT_SNI", "") == "true", "Use strict-sni bind processing (do not use default cert).") flag.StringVar(&o.MetricsType, "metrics-type", util.Env("ROUTER_METRICS_TYPE", ""), "Specifies the type of metrics to gather. Supports 'haproxy'.") } @@ -302,6 +304,7 @@ func (o *TemplateRouterOptions) Run() error { AllowWildcardRoutes: o.RouterSelection.AllowWildcardRoutes, MaxConnections: o.MaxConnections, Ciphers: o.Ciphers, + StrictSNI: o.StrictSNI, } oc, kc, err := o.Config.Clients() diff --git a/pkg/router/template/plugin.go b/pkg/router/template/plugin.go index c9661e03a1f4..9ff25803b0d6 100644 --- a/pkg/router/template/plugin.go +++ b/pkg/router/template/plugin.go @@ -55,6 +55,7 @@ type TemplatePluginConfig struct { BindPortsAfterSync bool MaxConnections string Ciphers string + StrictSNI bool } // routerInterface controls the interaction of the plugin with the underlying router implementation diff --git a/test/cmd/router.sh b/test/cmd/router.sh index e37ec4642418..649afbea32c4 100755 --- a/test/cmd/router.sh +++ b/test/cmd/router.sh @@ -44,6 +44,8 @@ os::cmd::expect_failure_and_text 'oadm router --dry-run --host-network=false --h os::cmd::expect_success_and_text 'oadm router --dry-run --host-network=false --host-ports=false --max-connections=14583 -o yaml' '14583' # ciphers os::cmd::expect_success_and_text 'oadm router --dry-run --host-network=false --host-ports=false --ciphers=modern -o yaml' 'modern' +# strict-sni +os::cmd::expect_success_and_text 'oadm router --dry-run --host-network=false --host-ports=false --strict-sni -o yaml' 'ROUTER_STRICT_SNI' # mount tls crt as secret os::cmd::expect_success_and_not_text 'oadm router --dry-run --host-network=false --host-ports=false -o yaml' 'value: /etc/pki/tls/private/tls.crt'