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

acme-challenge blocked with 403 when auth-tls-match-cn is being used #10185

Closed
mbettsteller opened this issue Jul 6, 2023 · 7 comments · Fixed by #11062
Closed

acme-challenge blocked with 403 when auth-tls-match-cn is being used #10185

mbettsteller opened this issue Jul 6, 2023 · 7 comments · Fixed by #11062
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@mbettsteller
Copy link

mbettsteller commented Jul 6, 2023

What happened:

When using cert-manager to get an SSL cert in an ingress that also verifies the client with a client ssl cert and also uses auth-tls-match-cn the acme-challenge is blocked with a 403.

This does NOT happen when you do not use auth-tls-match-cn!

cert-manager(acme challenge ingress) + mTLS client auth = OK
cert-manager(acme challenge ingress) + mTLS client auth + auth-tls-match-cn = NOK

What you expected to happen:

The location /.well-known/acme-challenge/redacted/ should not be blocked as 403, so the challenge could be answered even when the auth-tls-match-cn is set in the ingress.

What do you think went wrong?

When looking at the configuration that is being produced I see:

		if ( $ssl_client_s_dn !~ CN=(foo|bar|baz|) ) {
			return 403 "client certificate unauthorized";
		}

pretty early in the server section.

More detail:

server {
		server_name test.domain.tld ;
		
		listen 80  ;
		listen [::]:80  ;
		listen 443  ssl http2 ;
		listen [::]:443  ssl http2 ;
		
		set $proxy_upstream_name "-";
		
		if ( $ssl_client_s_dn !~ CN=(foo|bar|baz|) ) {
			return 403 "client certificate unauthorized";
		}
		
		ssl_certificate_by_lua_block {
			certificate.call()
		}
		
		# PEM sha: redacted
		ssl_client_certificate                  /etc/ingress-controller/ssl/ca-something-ca.pem;
		ssl_verify_client                       on;
		ssl_verify_depth                        2;
		
		# Custom code snippet configured for host test.domain.tld
		ssl_stapling on;
		ssl_stapling_verify on;
		
		location /.well-known/acme-challenge/redacted/ {
			
			set $namespace      "test";
			set $ingress_name   "cm-acme-http-solver-xxxxx";
			set $service_name   "cm-acme-http-solver-yyyyy";
			set $service_port   "8089";
			set $location_path  "/.well-known/acme-challenge/redacted";
			set $global_rate_limit_exceeding n;
			
			rewrite_by_lua_block {
				lua_ingress.rewrite({
					force_ssl_redirect = false,
					ssl_redirect = true,
					force_no_ssl_redirect = true,
					preserve_trailing_slash = false,
					use_port_in_redirects = false,
					global_throttle = { namespace = "", limit = 0, window_size = 0, key = { }, ignored_cidrs = { } },
				})
				balancer.rewrite()
				plugins.run()
			}
			
			# be careful with `access_by_lua_block` and `satisfy any` directives as satisfy any
			# will always succeed when there's `access_by_lua_block` that does not have any lua code doing `ngx.exit(ngx.DECLINED)`
			# other authentication method such as basic auth or external auth useless - all requests will be allowed.
			#access_by_lua_block {
			#}
			
			header_filter_by_lua_block {
				lua_ingress.header()
				plugins.run()
			}
			
			body_filter_by_lua_block {
				plugins.run()
			}
			
			log_by_lua_block {
				balancer.log()
				
				monitor.call()
				
				plugins.run()
			}
			
			port_in_redirect off;
			
			set $balancer_ewma_score -1;
			set $proxy_upstream_name "redacted";
			set $proxy_host          $proxy_upstream_name;
			set $pass_access_scheme  $scheme;
			
			set $pass_server_port    $server_port;
			
			set $best_http_host      $http_host;
			set $pass_port           $pass_server_port;
			
			set $proxy_alternative_upstream_name "";
			
			allow 0.0.0.0/0;
			allow ::/0;
			deny all;
			
			client_max_body_size                    1m;
			
			proxy_set_header Host                   $best_http_host;
			
			# Pass the extracted client certificate to the backend
			
			proxy_set_header ssl-client-verify      $ssl_client_verify;
			proxy_set_header ssl-client-subject-dn  $ssl_client_s_dn;
			proxy_set_header ssl-client-issuer-dn   $ssl_client_i_dn;
			
			# Allow websocket connections
			proxy_set_header                        Upgrade           $http_upgrade;
			
			proxy_set_header                        Connection        $connection_upgrade;
			
			proxy_set_header X-Request-ID           $req_id;
			proxy_set_header X-Real-IP              $remote_addr;
			
			proxy_set_header X-Forwarded-For        $remote_addr;
			
			proxy_set_header X-Forwarded-Host       $best_http_host;
			proxy_set_header X-Forwarded-Port       $pass_port;
			proxy_set_header X-Forwarded-Proto      $pass_access_scheme;
			proxy_set_header X-Forwarded-Scheme     $pass_access_scheme;
			
			proxy_set_header X-Scheme               $pass_access_scheme;
			
			# Pass the original X-Forwarded-For
			proxy_set_header X-Original-Forwarded-For $http_x_forwarded_for;
			
			# mitigate HTTPoxy Vulnerability
			# https://www.nginx.com/blog/mitigating-the-httpoxy-vulnerability-with-nginx/
			proxy_set_header Proxy                  "";
			
			# Custom headers to proxied server
			
			proxy_connect_timeout                   5s;
			proxy_send_timeout                      60s;
			proxy_read_timeout                      60s;
			
			proxy_buffering                         off;
			proxy_buffer_size                       4k;
			proxy_buffers                           4 4k;
			
			proxy_max_temp_file_size                1024m;
			
			proxy_request_buffering                 on;
			proxy_http_version                      1.1;
			
			proxy_cookie_domain                     off;
			proxy_cookie_path                       off;
			
			# In case of errors try the next upstream server before returning an error
			proxy_next_upstream                     error timeout;
			proxy_next_upstream_timeout             0;
			proxy_next_upstream_tries               3;
			
			proxy_pass http://upstream_balancer;
			
			proxy_redirect                          off;
			
		}

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

NGINX Ingress controller
Release: v1.5.1
Build: d003aae
Repository: https://github.com/kubernetes/ingress-nginx
nginx version: nginx/1.21.6


Kubernetes version (use kubectl version):

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.10", GitCommit:"e770bdbb87cccdc2daa790ecd69f40cf4df3cc9d", GitTreeState:"clean", BuildDate:"2023-05-17T14:12:20Z", GoVersion:"go1.19.9", Compiler:"gc", Platform:"linux/amd64"}
Kustomize Version: v4.5.7
Server Version: version.Info{Major:"1", Minor:"25+", GitVersion:"v1.25.10-eks-c12679a", GitCommit:"bbfa7e393476eb418f98a8c785721a006ba830cd", GitTreeState:"clean", BuildDate:"2023-05-22T20:31:17Z", GoVersion:"go1.19.9", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Cloud provider or hardware configuration: AWS (EKS)

  • OS (e.g. from /etc/os-release): AMI 1.25.9-20230607

  • Kernel (e.g. uname -a): 5.10.179-168.710.amzn2.x86_64

  • Install tools: helm (version.BuildInfo{Version:"v3.12.0", GitCommit:"c9f554d75773799f72ceef38c51210f1842a1dea", GitTreeState:"clean", GoVersion:"go1.20.3"})

  • How was the ingress-nginx-controller installed:

    • ingress-nginx redacted 35 2022-12-21 15:22:34.248122542 +0100 CET deployed ingress-nginx-4.4.0 1.5.1
    • cert-manager redacted 4 2023-05-23 07:20:31.081590225 +0200 CEST deployed cert-manager-v1.12.0 v1.12.0
USER-SUPPLIED VALUES:
controller:
  admissionWebhooks:
    enabled: false
    service:
      servicePort: 9443
    timeoutSeconds: 120
  affinity:
    podAntiAffinity:
      preferredDuringSchedulingIgnoredDuringExecution:
      - podAffinityTerm:
          labelSelector:
            matchExpressions:
            - key: app
              operator: In
              values:
              - ingress-nginx
          topologyKey: kubernetes.io/hostname
        weight: 100
  autoscaling:
    enabled: false
    maxReplicas: 3
    minReplicas: 2
    targetCPUUtilizationPercentage: 75
    targetMemoryUtilizationPercentage: 80
  config:
    annotation-value-word-blocklist: load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,}
    enable-ocsp: "true"
    keep-alive-requests: "10000"
    limit-conn-status-code: "429"
    limit-req-status-code: "429"
    log-format-escape-json: "true"
    log-format-stream: '{"timestamp": "$time_iso8601", "proxyUpstreamName": "$proxy_upstream_name",
      "upstreamAddr": "$upstream_addr", "httpRequest":{ "status": $status, "remoteIp":
      "$remote_addr"} }'
    log-format-upstream: '{"timestamp": "$time_iso8601", "requestID": "$req_id", "proxyUpstreamName":
      "$proxy_upstream_name", "proxyAlternativeUpstreamName": "$proxy_alternative_upstream_name",
      "upstreamStatus": "$upstream_status", "upstreamAddr": "$upstream_addr", "httpRequest":{
      "requestMethod": "$request_method", "requestUrl": "$host$request_uri", "status":
      $status, "requestSize": "$request_length", "responseSize": "$upstream_response_length",
      "userAgent": "$http_user_agent", "remoteIp": "$remote_addr", "referer": "$http_referer",
      "latency": "$upstream_response_time s", "protocol":"$server_protocol"}}'
    max-worker-connections: "10000"
    ssl-session-cache: "true"
    ssl-session-cache-size: 50m
    ssl-session-tickets: "true"
    ssl-session-timeout: 5m
    upstream-keepalive-requests: "10000"
    use-forwarded-headers: "false"
    worker-cpu-affinity: auto 1111
    worker-processes: "4"
  extraArgs:
    enable-ssl-chain-completion: "true"
  livenessProbe:
    failureThreshold: 60
    initialDelaySeconds: 10
    periodSeconds: 10
    successThreshold: 1
    timeoutSeconds: 1
  metrics:
    enabled: true
    port: 10254
    service:
      annotations:
        prometheus.io/port: "10254"
        prometheus.io/scrape: "true"
  minAvailable: 2
  minReadySeconds: 30
  readinessProbe:
    failureThreshold: 1
    initialDelaySeconds: 10
    periodSeconds: 10
    successThreshold: 1
    timeoutSeconds: 1
  replicaCount: 3
  resources:
    limits: {}
    requests:
      cpu: 100m
      ephemeral-storage: 5Gi
      memory: 512Mi
  service:
    annotations:
      service.beta.kubernetes.io/aws-load-balancer-backend-protocol: tcp
      service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: "true"
      service.beta.kubernetes.io/aws-load-balancer-eip-allocations: redacted
      service.beta.kubernetes.io/aws-load-balancer-type: nlb
    externalTrafficPolicy: Local
  terminationGracePeriodSeconds: 600
  topologySpreadConstraints:
  - labelSelector:
      matchLabels:
        app.kubernetes.io/component: controller
        app.kubernetes.io/instance: ingress-nginx
    maxSkew: 1
    topologyKey: topology.kubernetes.io/zone
    whenUnsatisfiable: DoNotSchedule
defaultBackend:
  enabled: true
  resources:
    requests:
      cpu: 10m
      memory: 20Mi
rbac:
  create: true
  scope: false

How to reproduce this issue:

Create an application with an ingress that will get an SSL cert from any ACME provider (lets encrypt staging) and additionally activate client ssl cert authentication with CN checking.

Anything else we need to know:

https://github.com/kubernetes/ingress-nginx/blob/helm-chart-4.4.0/rootfs/etc/nginx/template/nginx.tmpl#L961 is producing the CN checking line in the nginx config file. This needs to be smartened to not kill acme challenge pod locations.

As a test take out the CN checking (you will get your cert!).

Caveat: When you need to retest, you might need to use a different URL because once you succeeded, cert-manager will take the cert from the local cache!

@mbettsteller mbettsteller added the kind/bug Categorizes issue or PR as related to a bug. label Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Jul 6, 2023
@strongjz
Copy link
Member

strongjz commented Jul 6, 2023

/assign @rikatz
/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Jul 6, 2023
@rikatz
Copy link
Contributor

rikatz commented Jul 16, 2023

Ok, so taking a look here, the issue is ssl_verify_client is a server directive, and we cannot disable it per location: https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_verify_client

An alternative you have is to set it as optional and deal with it on your backend.

When cert auth is enabled, the var https://nginx.org/en/docs/http/ngx_http_ssl_module.html#var_ssl_client_verify is passed to the backend (see https://github.com/kubernetes/ingress-nginx/blob/main/rootfs/etc/nginx/template/nginx.tmpl#L1180) so you can turn it optional, but on your backends deny the authentication.

We can set on the template a validation enforcement or a new type of validation called on-without-restrictions that tells us it is "optional", but should enforce denial if the ssl_client_verify is an error except on auth locations, but this would take a bit of time for me to implement right now.

So, my suggestion right now is to turn it optional and validate on backend, if this is possible

@mbettsteller
Copy link
Author

Hi,
thank you very much for looking into this.

We can set on the template a validation enforcement or a new type of validation called on-without-restrictions that tells us it is "optional", but should enforce denial if the ssl_client_verify is an error except on auth locations, but this would take a bit of time for me to implement right now.

So, my suggestion right now is to turn it optional and validate on backend, if this is possible

I will check back with our backend DEVs and see if that can be easily done. But it is vacation time, so might take a while :-). Will post later how we decided to go on.

Thanks!

@github-actions
Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 19, 2023
@bossm8
Copy link
Contributor

bossm8 commented Feb 22, 2024

We got the same problem, is there any information if this will be fixed? Maybe by reordering in the nginx template?

When having a nginx.ingress.kubernetes.io/auth-tls-match-cn annotation configured ingress-nginx responds with 403 client certificate unauthorized. When the annotation is commented out it works as @mbettsteller already pointed out.

@bossm8
Copy link
Contributor

bossm8 commented Mar 2, 2024

@strongjz Would a change like this be accepted? It would be really great if this issue could be fixed.
(tested and verified locally, as the problem is not the ssl_verify_client directive itself but the reject if the client CN does not match when the certificate issuer requests /.well-known/acme-challenge without a certificate obviously)

        {{ if not ( empty $server.CertificateAuth.MatchCN ) }}
        {{ if gt (len $server.CertificateAuth.MatchCN) 0 }}
        location ~ ^/(?!(\.well-known/acme-challenge)) {
            if ( $ssl_client_s_dn !~ {{ $server.CertificateAuth.MatchCN }} ) {
                return 403 "client certificate unauthorized";
            }
        }
        {{ end }}
        {{ end }}

{{ if not ( empty $server.CertificateAuth.MatchCN ) }}
{{ if gt (len $server.CertificateAuth.MatchCN) 0 }}
if ( $ssl_client_s_dn !~ {{ $server.CertificateAuth.MatchCN }} ) {
return 403 "client certificate unauthorized";
}
{{ end }}
{{ end }}

@lkt82
Copy link

lkt82 commented Aug 28, 2024

Hi

The fix for this issue was reverted in 11082

Is it possible to reopen as the issue is still present in the current release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging a pull request may close this issue.

6 participants