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

pecameron · 2017-06-13T19:27:22Z

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

pecameron · 2017-06-13T19:28:38Z

@rajatchopra @knobunc First part, haproxy-template.conf change. PTAL

pecameron · 2017-06-14T12:59:52Z

@knobunc @rajatchopra @JacobTanenbaum Added command option PTAL

knobunc

In general it looks great. Thanks.

knobunc · 2017-06-14T13:01:04Z

images/router/haproxy/conf/haproxy-config.template

@@ -204,7 +204,7 @@ 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


It looks right to me, but I'd like to see some whitespace so I can follow what we are doing a little more clearly:

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

knobunc · 2017-06-14T13:12:52Z

@openshift/networking PTAL

pecameron · 2017-06-14T15:19:58Z

@knobunc PTAL

knobunc · 2017-06-14T18:42:47Z

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'


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

I think you should check the value

pravisankar · 2017-06-14T18:52:50Z

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 }}


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

rajatchopra

LGTM

rajatchopra · 2017-06-14T18:53:36Z

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 }}


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

knobunc · 2017-06-14T18:56:22Z

[test]

pravisankar · 2017-06-14T18:58:13Z

@pecameron Can you split this change into 2 commits. One commit for the actual change and another commit for the auto generated stuff that way it will be cleaner.
I added one other comment, otherwise change looks good.

knobunc · 2017-06-14T19:08:21Z

@pravisankar is that safe? Won't one commit fail tests because the autogen stuff isn't current? Thus it will break bisect? Or are you asking that we do it for the PR only and then squash before it is merged?

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

JacobTanenbaum

why are we setting this on the cli and with an environment variable?

JacobTanenbaum · 2017-06-14T20:01:20Z

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'


I think you should check the value

pravisankar · 2017-06-14T20:04:47Z

On Wed, Jun 14, 2017 at 12:08 PM, Ben Bennett ***@***.***> wrote: @pravisankar <https://github.com/pravisankar> is that safe? Won't one commit fail tests because the autogen stuff isn't current? Thus it will break bisect? Or are you asking that we do it for the PR only and then squash before it is merged?

No, it won't break. Test/Merge is performed on the entire changes rebased on master. This was suggested on my PR long time back and I have seen others following this convention (just grep for 'generated' in 'git log' output).

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABM0hi5NHy090OC9koQanBbPfbTgjJj_ks5sEC-rgaJpZM4N49tj> .

knobunc

LGTM

knobunc · 2017-06-14T20:34:55Z

[merge][severity: blocker]

openshift-bot · 2017-06-14T21:06:21Z

Evaluated for origin test up to e8638ae

openshift-bot · 2017-06-14T22:51:58Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2240/) (Base Commit: 0be6aee)

pecameron · 2017-06-15T13:51:07Z

Yes it does put strict-sni on the bind line (I verified this)

…

On 06/14/2017 02:53 PM, Ravi Sankar Penta wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In images/router/haproxy/conf/haproxy-config.template <#14621 (comment)>: > @@ -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 }} Will this end up putting 'strict-sni' on the same line as 'bind 127.0.0.1'? (Assuming {{-' only removes leading spaces) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14621 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANUgeru4Av6pcb7GkeCejd7DVYmB5mhqks5sECwMgaJpZM4N49tj>.

openshift-bot · 2017-06-16T08:24:07Z

Evaluated for origin merge up to e8638ae

openshift-bot · 2017-06-16T08:24:17Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1017/) (Base Commit: 6d961d6) (PR Branch Commit: e8638ae) (Extended Tests: blocker) (Image: devenv-rhel7_6366)

Openshift 3.6 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" origin PR 14621 openshift/origin#14621 bug 1369865 https://bugzilla.redhat.com/show_bug.cgi?id=1369865

knobunc requested review from rajatchopra and JacobTanenbaum June 14, 2017 12:43

pecameron force-pushed the br1369865 branch from 5af74fa to 12e05ef Compare June 14, 2017 12:57

knobunc requested changes Jun 14, 2017

View reviewed changes

knobunc changed the title ~~Donot serve certificate content for Non-SSL routes~~ Do not serve certificate content for Non-SSL routes Jun 14, 2017

pecameron force-pushed the br1369865 branch from 12e05ef to 14bc364 Compare June 14, 2017 15:19

knobunc reviewed Jun 14, 2017

View reviewed changes

pravisankar reviewed Jun 14, 2017

View reviewed changes

rajatchopra approved these changes Jun 14, 2017

View reviewed changes

pecameron force-pushed the br1369865 branch from 14bc364 to e8638ae Compare June 14, 2017 20:02

JacobTanenbaum reviewed Jun 14, 2017

View reviewed changes

knobunc approved these changes Jun 14, 2017

View reviewed changes

openshift-bot merged commit a68db68 into openshift:master Jun 16, 2017

pecameron mentioned this pull request Jun 16, 2017

Do not serve certificate content for Non-SSL routes openshift/openshift-docs#4595

Merged

pecameron deleted the br1369865 branch July 27, 2017 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pecameron commented Jun 13, 2017 •

edited

Loading

pecameron commented Jun 13, 2017

pecameron commented Jun 14, 2017

knobunc left a comment

knobunc Jun 14, 2017 •

edited

Loading

knobunc commented Jun 14, 2017

pecameron commented Jun 14, 2017

knobunc Jun 14, 2017

JacobTanenbaum Jun 14, 2017

pravisankar Jun 14, 2017

rajatchopra left a comment

rajatchopra Jun 14, 2017

knobunc commented Jun 14, 2017

pravisankar commented Jun 14, 2017

knobunc commented Jun 14, 2017

JacobTanenbaum left a comment •

edited

Loading

JacobTanenbaum Jun 14, 2017

pravisankar commented Jun 14, 2017 via email

knobunc left a comment

knobunc commented Jun 14, 2017 •

edited

Loading

openshift-bot commented Jun 14, 2017

openshift-bot commented Jun 14, 2017

pecameron commented Jun 15, 2017 via email

openshift-bot commented Jun 16, 2017

openshift-bot commented Jun 16, 2017 •

edited

Loading

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

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

Conversation

pecameron commented Jun 13, 2017 • edited Loading

pecameron commented Jun 13, 2017

pecameron commented Jun 14, 2017

knobunc left a comment

Choose a reason for hiding this comment

knobunc Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

knobunc commented Jun 14, 2017

pecameron commented Jun 14, 2017

knobunc Jun 14, 2017

Choose a reason for hiding this comment

JacobTanenbaum Jun 14, 2017

Choose a reason for hiding this comment

pravisankar Jun 14, 2017

Choose a reason for hiding this comment

rajatchopra left a comment

Choose a reason for hiding this comment

rajatchopra Jun 14, 2017

Choose a reason for hiding this comment

knobunc commented Jun 14, 2017

pravisankar commented Jun 14, 2017

knobunc commented Jun 14, 2017

JacobTanenbaum left a comment • edited Loading

Choose a reason for hiding this comment

JacobTanenbaum Jun 14, 2017

Choose a reason for hiding this comment

pravisankar commented Jun 14, 2017 via email

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Jun 14, 2017 • edited Loading

openshift-bot commented Jun 14, 2017

openshift-bot commented Jun 14, 2017

pecameron commented Jun 15, 2017 via email

openshift-bot commented Jun 16, 2017

openshift-bot commented Jun 16, 2017 • edited Loading

pecameron commented Jun 13, 2017 •

edited

Loading

knobunc Jun 14, 2017 •

edited

Loading

JacobTanenbaum left a comment •

edited

Loading

knobunc commented Jun 14, 2017 •

edited

Loading

openshift-bot commented Jun 16, 2017 •

edited

Loading