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

also whitelist ipv6 #1497

Merged
merged 1 commit into from
Mar 26, 2019
Merged

also whitelist ipv6 #1497

merged 1 commit into from
Mar 26, 2019

Conversation

mdonoughe
Copy link
Contributor

What this PR does / why we need it:

Whitelisting 0.0.0.0/0 means people who use whitelisting won't have problems when using acme over ipv4, but guarantees that people will have problems when using acme over ipv6 (see nginx rule below). I've added ::0/0 to the whitelist, which seems to solve the problem I've been having today.

geo $the_real_ip $deny_QevWrNHVjGFiQYHjfMrOsfzrjRZiYJTr {
        default 1;

        0.0.0.0/0 0;
        ::/0 0;
}

Without ::0/0 in this filter, IPv6 acme requests are blocked with error 403.

if ($deny_QevWrNHVjGFiQYHjfMrOsfzrjRZiYJTr) {
        return 403;
}

You can tell this is happening when the nginx logs show the pattern of returning 200 for a challenge requests coming from one of your cluster nodes, followed by a 403 response to a request with the Let's Encrypt user agent from an IPv6 address. On the log entries with a 200 response you will see the container address where the request is forwarded to, but on the 403 log entries you will not.

Which issue this PR fixes: none that I know of

Special notes for your reviewer:

Release note:

All IPv6 addresses are added to the nginx-ingress whitelist for HTTP challenges,
restoring HTTP challenge support for domains with AAAA records when using nginx-ingress.

otherwise it's blacklisted

Signed-off-by: Matthew Donoughe <mdonoughe@gmail.com>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 24, 2019
@jetstack-bot jetstack-bot added area/acme Indicates a PR directly modifies the ACME Issuer code area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code labels Mar 24, 2019
@jetstack-bot
Copy link
Contributor

Hi @mdonoughe. Thanks for your PR.

I'm waiting for a jetstack or cert-manager 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.

@jetstack-bot jetstack-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 24, 2019
@johannwagner
Copy link

We ran into the same bug and still thought, how to report it :D

@munnerz
Copy link
Member

munnerz commented Mar 26, 2019

Thanks! 😄

/lgtm
/approve
/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 26, 2019
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdonoughe, munnerz

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:

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2019
@jetstack-bot jetstack-bot merged commit 4f67c80 into cert-manager:master Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code area/acme Indicates a PR directly modifies the ACME Issuer code dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants