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

Do not serve certificate content for Non-SSL routes #14621

Merged
merged 1 commit into from
Jun 16, 2017
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
2 changes: 2 additions & 0 deletions contrib/completions/bash/oadm
Original file line number Diff line number Diff line change
Expand Up @@ -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=")
Expand Down
2 changes: 2 additions & 0 deletions contrib/completions/bash/oc
Original file line number Diff line number Diff line change
Expand Up @@ -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=")
Expand Down
6 changes: 6 additions & 0 deletions contrib/completions/bash/openshift
Original file line number Diff line number Diff line change
Expand Up @@ -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=")
Expand Down Expand Up @@ -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=")
Expand Down Expand Up @@ -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=")
Expand Down
2 changes: 2 additions & 0 deletions contrib/completions/zsh/oadm
Original file line number Diff line number Diff line change
Expand Up @@ -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=")
Expand Down
2 changes: 2 additions & 0 deletions contrib/completions/zsh/oc
Original file line number Diff line number Diff line change
Expand Up @@ -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=")
Expand Down
6 changes: 6 additions & 0 deletions contrib/completions/zsh/openshift
Original file line number Diff line number Diff line change
Expand Up @@ -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=")
Expand Down Expand Up @@ -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=")
Expand Down Expand Up @@ -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=")
Expand Down
5 changes: 4 additions & 1 deletion images/router/haproxy/conf/haproxy-config.template
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}

Choose a reason for hiding this comment

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

Will this end up putting 'strict-sni' on the same line as 'bind 127.0.0.1'? (Assuming {{-' only removes leading spaces)

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this collapse on the previous line? not sure how the white-space collapse works.
All else looks good.

{{- 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.
Expand Down
7 changes: 7 additions & 0 deletions pkg/cmd/admin/router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/infra/router/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type TemplateRouter struct {
BindPortsAfterSync bool
MaxConnections string
Ciphers string
StrictSNI bool
MetricsType string
}

Expand Down Expand Up @@ -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'.")
}

Expand Down Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions pkg/router/template/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions test/cmd/router.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check sufficient? You always set the variable, but to true or false as appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should check the value


# 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'
Expand Down