From af2296580f61dd631297e279252487c9fb0e4381 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Fri, 3 May 2024 21:27:38 -0300 Subject: [PATCH] ensure https redirect happens before root redirect app-root config key configures the root path redirect in haproxy frontend. https redirect however is configured in the backend. Because of that haproxy is redirecting from the root path to the application path in plain http, before redirecting to https. This is not a good approach because it makes security scanners infer that the application does not have a secure proxy. This update adds a https redirect before the application redirect, in the case the root path of the host renders its ssl-redirect to true. --- pkg/converters/ingress/annotations/global.go | 1 + pkg/haproxy/config.go | 21 +++++++++++++++++++ pkg/haproxy/instance_test.go | 6 ++++++ pkg/haproxy/types/host.go | 6 ++++++ pkg/haproxy/types/types.go | 2 ++ rootfs/etc/templates/haproxy/haproxy.tmpl | 13 +++++++++++- tests/framework/framework.go | 1 + tests/framework/options/objects.go | 6 ++---- tests/integration/integration_test.go | 22 ++++++++++++++++++-- 9 files changed, 71 insertions(+), 7 deletions(-) diff --git a/pkg/converters/ingress/annotations/global.go b/pkg/converters/ingress/annotations/global.go index 033b8b3f6..93506cb90 100644 --- a/pkg/converters/ingress/annotations/global.go +++ b/pkg/converters/ingress/annotations/global.go @@ -314,6 +314,7 @@ func (c *updater) buildGlobalSSL(d *globalData) { ssl.ModeAsync = d.mapper.Get(ingtypes.GlobalSSLModeAsync).Bool() ssl.Options = d.mapper.Get(ingtypes.GlobalSSLOptions).Value ssl.RedirectCode = d.mapper.Get(ingtypes.GlobalSSLRedirectCode).Int() + ssl.SSLRedirect = d.mapper.Get(ingtypes.BackSSLRedirect).Bool() } func (c *updater) buildGlobalHTTPStoHTTP(d *globalData) { diff --git a/pkg/haproxy/config.go b/pkg/haproxy/config.go index c661a9271..7fd21c0de 100644 --- a/pkg/haproxy/config.go +++ b/pkg/haproxy/config.go @@ -180,6 +180,7 @@ func (c *config) WriteFrontendMaps() error { // RedirFromRootMap: mapBuilder.AddMap(mapsDir + "/_front_redir_fromroot.map"), RedirFromMap: mapBuilder.AddMap(mapsDir + "/_front_redir_from.map"), + RedirRootSSLMap: mapBuilder.AddMap(mapsDir + "/_front_redir_root_ssl.map"), RedirToMap: mapBuilder.AddMap(mapsDir + "/_front_redir_to.map"), SSLPassthroughMap: mapBuilder.AddMap(mapsDir + "/_front_sslpassthrough.map"), VarNamespaceMap: mapBuilder.AddMap(mapsDir + "/_front_namespace.map"), @@ -274,6 +275,26 @@ func (c *config) WriteFrontendMaps() error { // TODO wildcard/alias/alias-regex hostname can overlap // a configured domain which doesn't have rootRedirect if host.RootRedirect != "" { + // looking for root path configuration - if ssl redirect is enabled, + // we need to redirect to https before redirect the path. + redirectssl := func() bool { + redir := c.global.SSL.SSLRedirect + for _, path := range host.FindPath("/") { + if backend := c.backends.Items()[path.Backend.ID]; backend != nil { + if bpath := backend.FindBackendPath(path.Link); bpath != nil { + redir = bpath.SSLRedirect + if !path.Link.ComposeMatch() { + // give precedence for root path without method, header or cookie matching + return redir + } + } + } + } + return redir + } + if redirectssl() { + fmaps.RedirRootSSLMap.AddHostnameMapping(host.Hostname, "") + } fmaps.RedirFromRootMap.AddHostnameMapping(host.Hostname, host.RootRedirect) } // diff --git a/pkg/haproxy/instance_test.go b/pkg/haproxy/instance_test.go index 866f61405..7a4d57ea6 100644 --- a/pkg/haproxy/instance_test.go +++ b/pkg/haproxy/instance_test.go @@ -4042,6 +4042,8 @@ func TestInstanceRootRedirect(t *testing.T) { c := setup(t) defer c.teardown() + c.config.global.SSL.SSLRedirect = true + var h *hatypes.Host var b = c.config.Backends().AcquireBackend("d1", "app", "8080") h = c.config.Hosts().AcquireHost("*.d1.local") @@ -4081,6 +4083,7 @@ frontend _front_http mode http bind :80 <> + http-request redirect scheme https if { path / } { var(req.host) -i -m str -f /etc/haproxy/maps/_front_redir_root_ssl__exact.map } http-request set-var(req.rootredir) var(req.host),map_str(/etc/haproxy/maps/_front_redir_fromroot__exact.map) http-request set-var(req.rootredir) var(req.host),map_reg(/etc/haproxy/maps/_front_redir_fromroot__regex.map) if !{ var(req.rootredir) -m found } http-request redirect location %[var(req.rootredir)] if { path / } { var(req.rootredir) -m found } @@ -4116,6 +4119,9 @@ d2.local#/app1 d2_app_8080 `) c.checkMap("_front_redir_fromroot__exact.map", ` d2.local /app1 +`) + c.checkMap("_front_redir_root_ssl__exact.map", ` +d2.local `) c.checkMap("_front_redir_fromroot__regex.map", ` ^[^.]+\.d1\.local$ /app diff --git a/pkg/haproxy/types/host.go b/pkg/haproxy/types/host.go index a416b1c86..38fb440e7 100644 --- a/pkg/haproxy/types/host.go +++ b/pkg/haproxy/types/host.go @@ -405,6 +405,12 @@ func (l *PathLink) Equals(other *PathLink) bool { return l.hash == other.hash } +// ComposeMatch returns true if the pathLink has composing match, +// by adding method, header or cookie match. +func (l *PathLink) ComposeMatch() bool { + return len(l.headers) > 0 +} + // WithHostname ... func (l *PathLink) WithHostname(hostname string) *PathLink { l.hostname = hostname diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index df565b167..60d6f3efa 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -151,6 +151,7 @@ type SSLConfig struct { ModeAsync bool Options string RedirectCode int + SSLRedirect bool } // DHParamConfig ... @@ -406,6 +407,7 @@ type FrontendMaps struct { HTTPSSNIMap *HostsMap // RedirFromRootMap *HostsMap + RedirRootSSLMap *HostsMap RedirFromMap *HostsMap RedirToMap *HostsMap SSLPassthroughMap *HostsMap diff --git a/rootfs/etc/templates/haproxy/haproxy.tmpl b/rootfs/etc/templates/haproxy/haproxy.tmpl index 865bee60e..782cc7aaf 100644 --- a/rootfs/etc/templates/haproxy/haproxy.tmpl +++ b/rootfs/etc/templates/haproxy/haproxy.tmpl @@ -1156,8 +1156,19 @@ frontend {{ $proxy__front_http }} http-request set-var(req.host) hdr(host),field(1,:),lower http-request set-var(req.base) var(req.host),concat(\#,req.path) -{{- /*------------------------------------*/}} {{- $acmeexclusive := and $global.Acme.Enabled (not $global.Acme.Shared) }} + +{{- /*------------------------------------*/}} +{{- if $fmaps.RedirRootSSLMap.HasHost }} +{{- range $match := $fmaps.RedirRootSSLMap.MatchFiles }} + http-request redirect scheme https + {{- if $global.SSL.RedirectCode }} code {{ $global.SSL.RedirectCode }}{{ end }} + {{- "" }} if{{ if $acmeexclusive }} !acme-challenge{{ end }} + {{- "" }} { path / } { var(req.host) -i -m {{ $match.Method }} -f {{ $match.Filename }} } +{{- end }} +{{- end }} + +{{- /*------------------------------------*/}} {{- if $fmaps.RedirFromRootMap.HasHost }} {{- range $match := $fmaps.RedirFromRootMap.MatchFiles }} http-request set-var(req.rootredir) var(req.host) diff --git a/tests/framework/framework.go b/tests/framework/framework.go index b65e35ebc..068f7bf32 100644 --- a/tests/framework/framework.go +++ b/tests/framework/framework.go @@ -166,6 +166,7 @@ func (f *framework) StartController(ctx context.Context, t *testing.T) { global.Data = map[string]string{ "http-port": "18080", "https-port": "18443", + "stats-port": "11936", "max-connections": "20", } err = f.cli.Create(ctx, &global) diff --git a/tests/framework/options/objects.go b/tests/framework/options/objects.go index 6da01280e..dc1203095 100644 --- a/tests/framework/options/objects.go +++ b/tests/framework/options/objects.go @@ -8,15 +8,13 @@ import ( type Object func(o *objectOpt) -func AddConfigKeyAnnotations(ann map[string]string) Object { +func AddConfigKeyAnnotation(key, value string) Object { annprefix := "haproxy-ingress.github.io/" return func(o *objectOpt) { if o.Ann == nil { o.Ann = make(map[string]string) } - for k, v := range ann { - o.Ann[annprefix+k] = v - } + o.Ann[annprefix+key] = value } } diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index db25376f7..23238ffc1 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -50,7 +50,7 @@ func TestIntegrationIngress(t *testing.T) { svc := f.CreateService(ctx, t, httpServerPort) _, hostname := f.CreateIngress(ctx, t, svc, options.DefaultHostTLS(), - options.AddConfigKeyAnnotations(map[string]string{ingtypes.BackSSLRedirect: "false"}), + options.AddConfigKeyAnnotation(ingtypes.BackSSLRedirect, "false"), ) res := f.Request(ctx, t, http.MethodGet, hostname, "/", options.ExpectResponseCode(http.StatusOK)) assert.True(t, res.EchoResponse) @@ -61,7 +61,7 @@ func TestIntegrationIngress(t *testing.T) { svc := f.CreateService(ctx, t, httpServerPort) _, hostname := f.CreateIngress(ctx, t, svc, options.DefaultHostTLS(), - options.AddConfigKeyAnnotations(map[string]string{ingtypes.BackSSLRedirect: "true"}), + options.AddConfigKeyAnnotation(ingtypes.BackSSLRedirect, "true"), ) res := f.Request(ctx, t, http.MethodGet, hostname, "/", options.ExpectResponseCode(http.StatusFound)) assert.False(t, res.EchoResponse) @@ -103,6 +103,24 @@ func TestIntegrationIngress(t *testing.T) { assert.Equal(t, reqHeaders, res.ReqHeaders) }) + t.Run("should redirect to https before app-root", func(t *testing.T) { + t.Parallel() + svc := f.CreateService(ctx, t, httpServerPort) + _, hostname := f.CreateIngress(ctx, t, svc, + options.DefaultHostTLS(), + options.AddConfigKeyAnnotation(ingtypes.BackSSLRedirect, "true"), + options.AddConfigKeyAnnotation(ingtypes.HostAppRoot, "/app"), + ) + + res := f.Request(ctx, t, http.MethodGet, hostname, "/", options.ExpectResponseCode(http.StatusFound)) + assert.False(t, res.EchoResponse) + assert.Equal(t, fmt.Sprintf("https://%s/", hostname), res.HTTPResponse.Header.Get("location")) + + res = f.Request(ctx, t, http.MethodGet, hostname, "/", options.HTTPSRequest(true)) + assert.False(t, res.EchoResponse) + assert.Equal(t, "/app", res.HTTPResponse.Header.Get("location")) + }) + t.Run("should take leader", func(t *testing.T) { t.Parallel() assert.EventuallyWithT(t, func(collect *assert.CollectT) {