jcmoraisjr / haproxy-ingress

HAProxy Ingress
https://haproxy-ingress.github.io
Apache License 2.0
1.02k stars 272 forks source link

Change order of use_backend haproxy.tmpl #1109

Closed jessequinn closed 3 weeks ago

jessequinn commented 2 months ago

What are you trying to do

We require that the SNIBACKEND come before the HOSTBACKEND on the frontend configuration.

What HAProxy Ingress should do or how it should behave differently

frontend _front_https
    mode http
    bind :443 accept-proxy ssl alpn h2,http/1.1 crt-list /etc/haproxy/maps/_front_bind_crt.list ca-ignore-err all crt-ignore-err all
    log-format %ci:%cp\ method=%HM\ uri=%HU\ rcvms=%TR\ serverms=%Tr\ activems=%Ta\ bytes=%B\ status=%ST
    http-request set-var(req.path) path
    http-request set-var(req.host) hdr(host),field(1,:),lower
    http-request set-var(req.base) var(req.host),concat(\#,req.path)
    http-request set-var(req.hostbackend) var(req.base),map_reg(/etc/haproxy/maps/_front_https_host__regex.map)
    http-request set-header X-Forwarded-Proto https
    http-request del-header X-SSL-Client-CN
    http-request del-header X-SSL-Client-DN
    http-request del-header X-SSL-Client-SHA1
    http-request del-header X-SSL-Client-SHA2
    http-request del-header X-SSL-Client-Cert
    acl tls-has-crt ssl_c_used
    acl tls-need-crt ssl_fc_sni -i -m str -f /etc/haproxy/maps/_front_tls_needcrt__exact.list
    acl tls-host-need-crt var(req.host) -i -m str -f /etc/haproxy/maps/_front_tls_needcrt__exact.list
    acl tls-has-invalid-crt ssl_c_verify gt 0
    acl tls-check-crt ssl_fc_sni -i -m str -f /etc/haproxy/maps/_front_tls_auth__exact.list
    http-request set-var(req.snibase) ssl_fc_sni,lower,concat(\#,req.path)
    http-request set-var(req.snibackend) var(req.snibase),map_dir(/etc/haproxy/maps/_front_https_sni__prefix.map)
    http-request set-var(req.snibackend) var(req.base),map_dir(/etc/haproxy/maps/_front_https_sni__prefix.map) if !{ var(req.snibackend) -m found } !tls-has-crt !tls-host-need-crt
    http-request set-var(req.tls_nocrt_redir) str(_internal) if !tls-has-crt tls-need-crt
    http-request set-var(req.tls_invalidcrt_redir) str(_internal) if tls-has-invalid-crt tls-check-crt
    http-request use-service lua.send-421 if tls-has-crt { ssl_fc_has_sni } !{ ssl_fc_sni,strcmp(req.host) eq 0 }
    http-request use-service lua.send-496 if { var(req.tls_nocrt_redir) -m str _internal }
    http-request use-service lua.send-421 if !tls-has-crt tls-host-need-crt
    http-request use-service lua.send-495 if { var(req.tls_invalidcrt_redir) -m str _internal }
    use_backend %[var(req.hostbackend)] if { var(req.hostbackend) -m found }
    use_backend %[var(req.snibackend)] if { var(req.snibackend) -m found } <-- move this up above the hostbackend
    default_backend _error404

This would require a change in the haproxy.tmpl

https://github.com/jcmoraisjr/haproxy-ingress/blob/v0.14.6/rootfs/etc/templates/haproxy/haproxy.tmpl

Original code:

{{- /*------------------------------------*/}}
    use_backend %[var(req.hostbackend)]
        {{- "" }} if { var(req.hostbackend) -m found }
{{- if $hasTLSAuth }}
    use_backend %[var(req.snibackend)]
        {{- "" }} if { var(req.snibackend) -m found }
{{- end }}
{{- template "defaultbackend" map $hosts $defaultbackend }}

{{- end }}{{/* has $fmaps */}}
{{- end }}{{/* define "frontends" */}}

{{- /*------------------------------------*/}}

New code:

{{- /*------------------------------------*/}}
{{- if $hasTLSAuth }}
    use_backend %[var(req.snibackend)]
        {{- "" }} if { var(req.snibackend) -m found }
{{- end }}
    use_backend %[var(req.hostbackend)]
        {{- "" }} if { var(req.hostbackend) -m found }
{{- template "defaultbackend" map $hosts $defaultbackend }}

{{- end }}{{/* has $fmaps */}}
{{- end }}{{/* define "frontends" */}}

{{- /*------------------------------------*/}}

Would this be feasible @jcmoraisjr or do you suggest another way to attack our problem?

jcmoraisjr commented 2 months ago

Hi, can you describe a bit more about your need? How your ingresses resources are configured, how the request looks like.

The hostname in the SNI extension is not supposed to be used to route requests, but instead to choose a proper server certificate for the TLS handshake. Note also that it's a common practice for a http client (e.g. browsers), over H2 connections, to reuse the same TCP connection for requests with distinct hostnames, leading the request with distinct values between SNI and Host header, being the Host header the correct one to be used. The proposed configuration change might lead to wrong routing and maybe to a security issue depending on the ingress configurations.

GuilhermeAbraham commented 2 months ago

To give you more insight, the context for this issue is:

  1. The ingress is in front of a service that creates a communication/proxy between two other external services
  2. One of the external services will communicate to <random>-something.domain.com and authenticate using a session
  3. The other external service will communicate to something.domain.com and authenticate using mTLS

Since <random>-something.domain.com is not acceptable as a host, we matching for *.domain.com, we were under the assumption that the host that is more specific, something.domain.com, would have priority in the match over the host that uses regex/glob. And this would be true if both are on the maps/_front_https_host__* because HaProxy default behavior is to prioritize matches in this order: exact,prefix,begin,regex (can be overwritten on the path-type-order parameter).

But since one of the endpoints uses mTLS, it's mapping is being stored on the SNI mapping instead of the Host mapping.

Jesse suggestion is just the more simple way we could go to solve this, any solution from changing the order, making the order configurable or even allowing us to specify the regex and match type for the host (instead of only basing it on the host) would work for us.

Let me know if this gives you enough information or if you need any further details. Thanks in advance for your attention.

jcmoraisjr commented 2 months ago

Hi thanks for the clarification. The detailed description of your use case led me to identify a missing piece in the frontend configuration that I think it should be there. Can you folks share the ingress resources you are using? Better if using fake, but semantically compatible with the real data. Those resources along with the provided descriptions should be enough for me to create an e2e test covering the expectations. Maybe we can fix this behavior for you in the code.

In the mean time I think you can safely overwrite the template file and change the order of the use_backend evaluation. The SNI validation I was worried about happens on another place of the template, so changing the order should be safe, except for some non expected behavior changed on other routing you might have in your cluster.

GuilhermeAbraham commented 1 month ago

Hey, sorry for the delayed response. Bellow the details you requested.

First let me just define the 2 external services:

  1. customer frontend (CF)
  2. destination resource (DR)

Ingress objects: customer *.domain.com destination something.domain.com

Ingress Yaml:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    ingress.kubernetes.io/auth-tls-secret: project/cfssl-chained-cas
    ingress.kubernetes.io/auth-tls-verify-client: "on"
    ingress.kubernetes.io/hsts: "true"
    kubernetes.io/ingress.class: haproxy-destination
  name: destination
  namespace: project
spec:
  rules:
  - host: something.domain.com
    http:
      paths:
      - backend:
          service:
            name: proxy-service
            port:
              number: 7171
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - something.domain.com
    secretName: tls-secret
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    ingress.kubernetes.io/config-backend: |
      acl allowed_subdomains req.hdr(host) -m reg -i '.+(-|\.)something\.domain\.com$'
      http-request reject if !allowed_subdomains
      acl is_websocket hdr(Upgrade) -i WebSocket
      http-request set-var(req.domain_hash) req.hdr(host),lower,regsub('.something\.domain\.com$','')
      http-request set-path /v1/project/destination/%[var(req.domain_hash)]/http/443%[path] if !is_websocket
      http-request set-path /v1/project/destination/%[var(req.domain_hash)]/ws/443%[path] if is_websocket
    ingress.kubernetes.io/cors-allow-credentials: "true"
    ingress.kubernetes.io/cors-allow-headers: Content-Type, Accept, Origin, User-Agent,
      DNT, Cache-Control, X-Mx-ReqToken, Keep-Alive, X-Requested-With, If-Modified-Since,
      X-XSRF-Token, X-Request-Id
    ingress.kubernetes.io/cors-allow-origin: https://www.domain.com,https://domain.com
    ingress.kubernetes.io/cors-enable: "true"
    kubernetes.io/ingress.class: haproxy-customer
  name: customer
  namespace: project
spec:
  rules:
  - host: '*.domain.com'
    http:
      paths:
      - backend:
          service:
            name: proxy-service
            port:
              number: 7171
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - '*.domain.com'
    secretName: tls-secret

Please let me know if you need anything else.

GuilhermeAbraham commented 3 weeks ago

@jessequinn there is a PR for this issue, can we consider trying it out on our problem? Will need your help to do it.

@jcmoraisjr thanks for the fast response and resolution!

jcmoraisjr commented 3 weeks ago

@GuilhermeAbraham you're welcome, thanks folks for sharing the problem.

I've just pushed v0.14 branch with some cherry picks from master, so you can build your testing image from there. Please let us know if it fixes the problem without any side effect. I'm missing a few more tests with this change before closing the issue, any help on that would be really appreciated.

jcmoraisjr commented 3 weeks ago

Fix merged to master and v0.14 branch. Closing, but feel free to update this same issue if the problem continues after updating.