Fix ACME when mTLS with client CN check is enabled #11062

bossm8 · 2024-03-04T13:13:57Z

What this PR does / why we need it:

Described in #10185
CertManager will fail to create a certificate If we enable mTLS on an ingress with client CN match enabled:

    cert-manager.io/cluster-issuer: my-issuer
    nginx.ingress.kubernetes.io/auth-tls-secret: ns/secret
    nginx.ingress.kubernetes.io/auth-tls-verify-client: "on"
    nginx.ingress.kubernetes.io/auth-tls-match-cn: "CN=some-cn"

If the annotation nginx.ingress.kubernetes.io/auth-tls-match-cn: "CN=some-cn" is removed it does not fail.
The issue is that the client CN match is implemented too early (which makes sense - fail fast). The PR fixes this case
by only checking the client certificate for its CN if the location is not .well-known/acme-challenge/....

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes #10185

How Has This Been Tested?

The following configuration was mounted into a local nginx container (docker run --rm -v $PWD/default.conf:/etc/nginx/conf.d/default.conf -p 8080:80 nginx:

server {
    listen       80;
    server_name  localhost;

    ssl_verify_client on;

    if ( $ssl_client_s_dn !~ some-cn ) { 
        return 403 "client certificate unauthorized"; 
    }

    location / {
        root   /usr/share/nginx/html;
        index  index.html index.htm;
    }
}

When running curl against this config we get 403:

curl http://localhost:8080/.well-known/acme-challenge/redacted
client certificate unauthorized

The same command against this config:

server {
    listen       80;
    server_name  localhost;

    ssl_verify_client on;

    location ~ ^/(?!(\.well-known/acme-challenge)) {
        if ( $ssl_client_s_dn !~ some-cn ) { 
            return 403 "client certificate unauthorized"; 
        }
    }

    location / {
        root   /usr/share/nginx/html;
        index  index.html index.htm;
    }
}

returns 404, which is expected since there is nothing on this url:

curl http://localhost:8080/.well-known/acme-challenge/redacted
<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>nginx/1.25.4</center>
</body>
</html>

While other urls still return 403:

curl http://localhost:8080/test                               
client certificate unauthorized

This test should be sufficient as acme works on http. Similar is shown in #10185 when removing the match-ch annotation.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.

…nabled

linux-foundation-easycla · 2024-03-04T13:14:02Z

✅login: bossm8 / (33b18c3)
✅login: bossm8 / (33b18c3, 17889e8)

The committers listed above are authorized under a signed CLA.

k8s-ci-robot · 2024-03-04T13:14:06Z

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2024-03-04T13:14:07Z

Welcome @bossm8!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-03-04T13:14:08Z

Hi @bossm8. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2024-03-04T13:14:15Z

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	`17889e8`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/65e5c918bfe8000008088ef2

cpanato

/approve

/hold for @Gacko

Gacko · 2024-03-04T14:48:08Z

I don't see a reason to hold (especially not for me) if this looks good to you.

/unhold

Gacko · 2024-03-04T14:53:56Z

I'm rather asking myself why there's mTLS on a non-HTTPS server. Do we support forwarding the Client Certificate DN in TLS offloading setups?

longwuyuan · 2024-03-04T15:54:05Z

the issue mentioned does not show kubectl describe of a ingress nor the curl command sending a client cert nor the logs of the controller

rikatz · 2024-03-08T20:29:47Z

/ok-to-test
/lgtm
/approve

Long story short, acme challenges usually runs against http (not https), as you are asking for a new certificate, + you don't validate the caller.

think on let's encrypt sending some challenge, and being blocked because you have enabled mTLS/clientcert on your side :)

Thanks for the contribution

k8s-ci-robot · 2024-03-08T20:30:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bossm8, cpanato, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cpanato,rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bossm8 · 2024-03-08T22:55:40Z

@rikatz @Gacko Abort!

I'm really sorry I just realized it now, this configuration will break. The location matches everything except acme-challenge, meaning all locations after will be skipped! I'm so sorry, I thought location is the way to go since ifs are discouraged by nginx.

* [mTLS] Fix acme verfication when mTLS and Client CN verification is enabled * revert mTLS location excluding acme-challenge since each location will match ultimately resulting in 404 for all request paths

Gacko · 2024-03-10T10:34:09Z

Once again, I'm asking myself what's the use case for mTLS on HTTP (not HTTPS). Can one please enlighten me? :)

bossm8 · 2024-03-10T10:50:04Z

Once again, I'm asking myself what's the use case for mTLS on HTTP (not HTTPS). Can one please enlighten me? :)

The current implementation of the nginx templating does not allow to get mTLS when certificate CN matching is enabled (please see #10185) because no certificate can be requested for the server via ACME's HTTP challenge. This is because the well-known ACME endpoint is handled via the same server block. ACME does however not send a client certificate (obviously) and will be rejected even before it can verify the challenge, so we will never get the Certificate to even enable mTLS.

bossm8 · 2024-03-10T10:54:20Z

(Could we thus reopen #10185?)

Gacko · 2024-03-10T13:58:38Z

For mTLS you would need a client certificate and you would normally hand that via HTTPS only. ACME on the other hand is handled via HTTP. So I'm asking myself why we are checking for a client certificate (HTTPS) in a HTTP block.

That's probably done because there might be use cases where TLS is terminated before NGINX (offloading).

rikatz · 2024-03-10T14:49:36Z

I think your proposal is to change this logic to only happen when the connection is a tls one right?

As we mix both listeners on the same server directive, we probably need some check like if ssl_protocols for the session is not false

https://nginx.org/en/docs/http/ngx_http_ssl_module.html#variables

Gacko · 2024-03-10T14:55:17Z

That depends on where the "$ssl_client_s_dn" is coming from. I'm thinking of scenarios where an upstream load balancer is terminating SSL and passing the client certificate DN as a header. In that case we would still use HTTP...

rikatz · 2024-03-10T15:12:44Z

Good point! I think we set some variables based on a trust from loadbalancers (eg x-forwarded-* and the certs) but IIRC some variables are set per nginx listener directly and are RO.

Anyway this needs to be think better.

bossm8 and others added 2 commits March 4, 2024 13:41

[mTLS] Fix acme verfication when mTLS and Client CN verification is e…

33b18c3

…nabled

Merge branch 'kubernetes:main' into main

17889e8

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 4, 2024

k8s-ci-robot requested review from cpanato and strongjz March 4, 2024 13:14

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2024

k8s-ci-robot added needs-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 4, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 4, 2024

cpanato approved these changes Mar 4, 2024

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 4, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2024

k8s-ci-robot assigned rikatz Mar 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2024

k8s-ci-robot merged commit 7d12628 into kubernetes:main Mar 8, 2024
23 checks passed

bossm8 mentioned this pull request Mar 8, 2024

Undo #11062 since it breaks the nginx config #11082

Merged

10 tasks

Gacko mentioned this pull request Mar 11, 2024

Chores: Pick patches from main. #11103

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ACME when mTLS with client CN check is enabled #11062

Fix ACME when mTLS with client CN check is enabled #11062

bossm8 commented Mar 4, 2024 •

edited

Loading

linux-foundation-easycla bot commented Mar 4, 2024 •

edited

Loading

k8s-ci-robot commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

netlify bot commented Mar 4, 2024 •

edited

Loading

cpanato left a comment

Gacko commented Mar 4, 2024

Gacko commented Mar 4, 2024

longwuyuan commented Mar 4, 2024

rikatz commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

bossm8 commented Mar 8, 2024

Gacko commented Mar 10, 2024

bossm8 commented Mar 10, 2024 •

edited

Loading

bossm8 commented Mar 10, 2024

Gacko commented Mar 10, 2024 •

edited

Loading

rikatz commented Mar 10, 2024

Gacko commented Mar 10, 2024

rikatz commented Mar 10, 2024

Fix ACME when mTLS with client CN check is enabled #11062

Fix ACME when mTLS with client CN check is enabled #11062

Conversation

bossm8 commented Mar 4, 2024 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Mar 4, 2024 • edited Loading

k8s-ci-robot commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

netlify bot commented Mar 4, 2024 • edited Loading

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

cpanato left a comment

Choose a reason for hiding this comment

Gacko commented Mar 4, 2024

Gacko commented Mar 4, 2024

longwuyuan commented Mar 4, 2024

rikatz commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

bossm8 commented Mar 8, 2024

Gacko commented Mar 10, 2024

bossm8 commented Mar 10, 2024 • edited Loading

bossm8 commented Mar 10, 2024

Gacko commented Mar 10, 2024 • edited Loading

rikatz commented Mar 10, 2024

Gacko commented Mar 10, 2024

rikatz commented Mar 10, 2024

bossm8 commented Mar 4, 2024 •

edited

Loading

linux-foundation-easycla bot commented Mar 4, 2024 •

edited

Loading

netlify bot commented Mar 4, 2024 •

edited

Loading

bossm8 commented Mar 10, 2024 •

edited

Loading

Gacko commented Mar 10, 2024 •

edited

Loading