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

Support for DNS challenge #1022

Closed
wants to merge 1 commit into from
Closed

Support for DNS challenge #1022

wants to merge 1 commit into from

Conversation

daweedm
Copy link
Contributor

@daweedm daweedm commented Apr 6, 2023

Hello @buchdag
I have added the support for DNS challenges, as it's supported by acme.sh and with minor changes to the acme-companion code base.

Could you review this pull request ?

@daweedm
Copy link
Contributor Author

daweedm commented Apr 6, 2023

Forgot to mention that this PR is not specific to any provider, as the idea is to "forward" required environment variables by each provider and which are defined in the proxied container, to the letsencrypt_service. Then, these variables are simply read by acme.sh. I have updated the README explaining how to set up DNS challenge for a provider, with an example.

Copy link

@mustanggb mustanggb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you doing this:

if [[ -z "$variable" ]]; then
  <do nothing>
else
  <do something>
fi

Instead of this:

if [[ -n "$variable" ]]; then
  <do something>
fi

I imagine some HTTP challenge stuff should not happen for a DNS challenge.
Perhaps this is a good place to start:

add_location_configuration "$domain" || reload_nginx

@ne0YT
Copy link

ne0YT commented Jun 16, 2023

I would love to see this happening in the official image.. I want to integrate it in some "offline" setups. where devices have internet and are random/not managed, but the services are not publicly available.

@zorbathut
Copy link

For what it's worth, I am now using this locally and it seems to be working. I do wish there was a better way to handle all the variables; I end up with the same block of variables copypasted multiple places in order to handle various subdomains. On the other hand, this is relevant only for people running multiple subdomains, and I'm not sure how common that is.

@zorbathut
Copy link

Alright, never mind, update :V

It works fine on single domains but I'm having trouble getting it working on multidomains; it does the first domain with DNS, but then seems to fall back on HTTP for the second domain.

Somewhat-anonymized log:

[Wed Jun 28 01:35:42 UTC 2023] Multi domain='DNS:www.[mydomain].com,DNS:[mydomain].com'
[Wed Jun 28 01:35:42 UTC 2023] Getting domain auth token for each domain
[Wed Jun 28 01:35:43 UTC 2023] Getting webroot for domain='www.[mydomain].com'
[Wed Jun 28 01:35:44 UTC 2023] Getting webroot for domain='[mydomain].com'
[Wed Jun 28 01:35:44 UTC 2023] www.[mydomain].com is already verified, skip dns-01.
[Wed Jun 28 01:35:44 UTC 2023] Verifying: [mydomain].com
[Wed Jun 28 01:35:46 UTC 2023] Pending
[Wed Jun 28 01:35:48 UTC 2023] Pending
[Wed Jun 28 01:35:51 UTC 2023] Pending
[Wed Jun 28 01:35:53 UTC 2023] Pending
[Wed Jun 28 01:35:55 UTC 2023] [mydomain].com:Verify error:1.2.3.4: Fetching http://[mydomain].com/.well-known/acme-challenge/[challenge-string]: Timeout during connect (likely firewall problem)
[Wed Jun 28 01:35:55 UTC 2023] Please check log file for more details: /dev/null

Not sure what I'm doing wrong.

@daweedm
Copy link
Contributor Author

daweedm commented Jun 28, 2023

For what it's worth, I am now using this locally and it seems to be working. I do wish there was a better way to handle all the variables; I end up with the same block of variables copypasted multiple places in order to handle various subdomains. On the other hand, this is relevant only for people running multiple subdomains, and I'm not sure how common that is.

If you are using docker-compose, you can store your common environment variables in a file, e.g. mydnsprovider.env, and reference it in multiple containers like this :

version: '3'
services:
  first:
    image: nginx
    env_file:
      - path/to/mydnsprovider.env
    environment:
      - VIRTUAL_HOST=first.com
      - LETSENCRYPT_HOST=first.com

and

version: '3'
services:
  second:
    image: nginx
    env_file:
      - path/to/mydnsprovider.env
    environment:
      - VIRTUAL_HOST=second.com
      - LETSENCRYPT_HOST=second.com

https://docs.docker.com/compose/environment-variables/set-environment-variables/#use-the-env_file-attribute

@daweedm
Copy link
Contributor Author

daweedm commented Jun 28, 2023

Alright, never mind, update :V

It works fine on single domains but I'm having trouble getting it working on multidomains; it does the first domain with DNS, but then seems to fall back on HTTP for the second domain.

Somewhat-anonymized log:

[Wed Jun 28 01:35:42 UTC 2023] Multi domain='DNS:www.[mydomain].com,DNS:[mydomain].com'
[Wed Jun 28 01:35:42 UTC 2023] Getting domain auth token for each domain
[Wed Jun 28 01:35:43 UTC 2023] Getting webroot for domain='www.[mydomain].com'
[Wed Jun 28 01:35:44 UTC 2023] Getting webroot for domain='[mydomain].com'
[Wed Jun 28 01:35:44 UTC 2023] www.[mydomain].com is already verified, skip dns-01.
[Wed Jun 28 01:35:44 UTC 2023] Verifying: [mydomain].com
[Wed Jun 28 01:35:46 UTC 2023] Pending
[Wed Jun 28 01:35:48 UTC 2023] Pending
[Wed Jun 28 01:35:51 UTC 2023] Pending
[Wed Jun 28 01:35:53 UTC 2023] Pending
[Wed Jun 28 01:35:55 UTC 2023] [mydomain].com:Verify error:1.2.3.4: Fetching http://[mydomain].com/.well-known/acme-challenge/[challenge-string]: Timeout during connect (likely firewall problem)
[Wed Jun 28 01:35:55 UTC 2023] Please check log file for more details: /dev/null

Not sure what I'm doing wrong.

Passing multiple domains is done the same way by using multiple -d flags, either using web or dns challenge according to https://github.com/acmesh-official/acme.sh#9-use-dns-manual-mode so it should work as is since I didn't modified the behaviour related to the -d flag in this PR. How does your config looks like ?

@yeliex
Copy link

yeliex commented Aug 30, 2023

@mustanggb any progress please?

@mustanggb
Copy link

Nothing to do with me.

  1. @daweedm hasn't made the requested changes.
  2. I don't have commit access.

@cistes44
Copy link

Hello @buchdag, any plan on this? If none, please let me know, I could take over the commit, try to fix things @mustanggb pointed out.

@daweedm
Copy link
Contributor Author

daweedm commented Oct 24, 2023

@mustanggb @yeliex @cistes44 sorry guys, I hadn't a lot of spare time last weeks. I'll try to take care of it if I can find some time soon. Meanwhile, feel free to pursue the work, there is not much left. @mustanggb what exactly do you need me to do for you to get commit access?

@24367dfa
Copy link

@mustanggb @yeliex @cistes44 sorry guys, I hadn't a lot of spare time last weeks. I'll try to take care of it if I can find some time soon. Meanwhile, feel free to pursue the work, there is not much left. @mustanggb what exactly do you need me to do for you to get commit access?

I think he you should grant him access to your fork: https://github.com/daweedm/acme-companion/ Otherwise he yould be unable to build on your work for this PR.

Copy link

@sokai sokai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @buchdag, may I ask you please to merge this (= the main feature: DNS challenge with single domain) in an official release (and solve the multi-domain thing later)?

Review:

  • feature is/seems very needed
  • single domain seems to work
  • multi-domain not working (can be done later)

#ty! @daweedm

Thanks a lot in advance + KR

@cxhamilton
Copy link

+1 for this feature for the same reason as other comments left in this thread

@buchdag
Copy link
Member

buchdag commented Jul 15, 2024

This PR will be completed in #1137.

@buchdag buchdag closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants