StackExchange / dnscontrol

Infrastructure as code for DNS!
https://dnscontrol.org/
MIT License
3.14k stars 399 forks source link

Deprecate "get-certs" (Let's Encrypt) support #1400

Open tlimoncelli opened 2 years ago

tlimoncelli commented 2 years ago

We are considering removing this feature in the future.

Why:

Post your thoughts in the comments.

DECISION: get-certs/ACME support is frozen and will be removed without notice between now and July 2025.

IzumiSenaSora commented 2 years ago

Its really a good feature!... We can manage all domain related things like (manage dns + certificate) in one place!... Please for now don't remove it!.... Unless it breaks!

juliusrickert commented 2 years ago

I didn't even know this feature existed. What were the motivations behind including this functionality into DNSControl?

I think this should not be the concern of DNSControl. For me, DNSControl is an application that is intended to be primarily run in CI. Thus, I don't see how issuing certificates would be beneficial. This is a concern of a load balancer or any other public-facing server.

There are other projects that are better at fulfilling this need, e.g., cert-manager.

On that note: I'm also not quite sure whether I like the deep integration with Cloudflare's redirects and workers. But I can see how that's related to DNSControl's goal. Both are redirects/routes in a way.

tlimoncelli commented 2 years ago

Please for now don't remove it!.... Unless it breaks!

Removing it when it breaks sounds like a terrible idea. People would be stranded. We'd rather give people advanced warning so they can remove it on their schedule. (Which is why we're starting the discussion now.)

tlimoncelli commented 2 years ago

I didn't even know this feature existed. What were the motivations behind including this functionality into DNSControl?

The motivation was: Let's Encrypt DNS01 requires the ability to talk to your DNS Provider, and dnscontrol already does that, it seems like a good match.

IzumiSenaSora commented 2 years ago

Please for now don't remove it!.... Unless it breaks!

Removing it when it breaks sounds like a terrible idea. People would be stranded. We'd rather give people advanced warning so they can remove it on their schedule. (Which is why we're starting the discussion now.)

Oh Sorry i'm not giving any idea!... i'm just saying if no one step forward to maintain it!... Then maybe removing it is only option! BTW I'm bad in English

IzumiSenaSora commented 2 years ago

I didn't even know this feature existed. What were the motivations behind including this functionality into DNSControl?

I think this should not be the concern of DNSControl. For me, DNSControl is an application that is intended to be primarily run in CI. Thus, I don't see how issuing certificates would be beneficial. This is a concern of a load balancer or any other public-facing server.

There are other projects that are better at fulfilling this need, e.g., cert-manager.

On that note: I'm also not quite sure whether I like the deep integration with Cloudflare's redirects and workers. But I can see how that's related to DNSControl's goal. Both are redirects/routes in a way.

It's useful! At least in Private ORG's Cause self sign cert need extra step to Add root cert manually in every browser!... But if we just generate lets encrypt cert which every browser/phone supports already!... So accessing private websites via VPN to work remotely this feature needed!... We can generate lets encrypt cert via dns verification instead of certbot like function which need http verification maybe 🤔 but work websites which is private so let's encrypt can't access those site to verify!... So dns verification to generate let's encrypt cert is useful

juliusrickert commented 2 years ago

Cause self sign cert need extra step to Add root cert manually in every browser!

That's not what I was trying to suggest, sorry for causing confusion here. Please do not modify trust stores and use services like Let's Encrypt! I'm fully encouraging using Let's Encrypt and other CAs supporting ACME!

We can generate lets encrypt cert via dns verification instead of certbot like function which need http verification maybe

Certbot supports DNS01 domain verification too. I think DNS01 is supported by all major ACME clients.

So there's really no benefit in using DNSControl.

The other tools are more widely used for this purpose too, so finding help and integrations is a lot easier with them.

tlimoncelli commented 2 years ago

I've decided to deprecate this feature.

I haven't set a specific date but it will most likely be early 2023.

I'd like to thank @captncraig for the original implementation. It's high quality code and one of the first DNS-01 implementations.

smehrens commented 1 year ago

certbot uses python plugins for the dns request.

I think -without having python knowledge- it would be easy to write a plugin for dnscontrol.

I took a look at https://github.com/siilike/certbot-dns-standalone and it seems no magic.

In this case certbot would do all the certificate stuff and dnscontrol would publish and remove the dns records for verification.

just my two cents.

tlimoncelli commented 1 year ago

certbot uses python plugins for the dns request.

I haven't counted lately but last I checked, certbot supported more providers than DNSControl. What would the benefit be of having certbot call dnscontrol?

smehrens commented 1 year ago

It makes it easier to configure certbot, if you already have configured DNSControl.
Less configfiles with dns provider api keys. Just convience.

I think "DNSControl will achieve world domination" smile and will soon support more provider then certbot. grin

tlimoncelli commented 1 year ago

I hate to be a bummer but... I'm not interested in world domination. I'm interested in doing a few specific tasks well. A tight-coupling between certbot and dnscontrol sounds like a support nightmare.

cafferata commented 7 months ago

This feature is frozen and will be removed in early 2023. docs.dnscontrol.org/commands/get-certs

@tlimoncelli, should we phase out this feature?

tlimoncelli commented 7 months ago

Update: get-certs/ACME support is frozen and will be removed without notice between now and July 2025.

jauderho commented 1 month ago

gopkg.in/square/go-jose.v2 is being flagged by Dependabot as having some security issues. I took a look earlier today and it appears that this is tied to the v2 version of lego that is used by dnscontrol (current is v4).

Something to consider for removal earlier than next year. Given the push for moving to ppreview/ppush, that might be a logical point to remove lego support completely at that time too.

tlimoncelli commented 1 month ago

@jauderho link to the Dependabot report?

jauderho commented 1 month ago

@tlimoncelli Here's a screenshot

Screenshot 2024-10-17 at 11 41 32 AM

This is the link to the GitHub advisory: https://github.com/advisories/GHSA-c5q2-7r4c-mv6g

jauderho@localhost:~/src/dnscontrol$ go mod why -m gopkg.in/square/go-jose.v2 
# gopkg.in/square/go-jose.v2
github.com/StackExchange/dnscontrol/v4/pkg/acme
github.com/go-acme/lego/certificate
github.com/go-acme/lego/acme/api
github.com/go-acme/lego/acme/api/internal/secure
gopkg.in/square/go-jose.v2

I did try to build using a v3 version of lego and that does seem to build without complaint. Using v4 throws an error but I have not had a chance to look as to why yet.

Since Go does not have an easy way to search and replace module paths, I quickly threw a script together to point to the new path. ~/scripts/sh/updateGoModules.sh github.com/go-acme/lego github.com/go-acme/lego/v3

Here's the script:

#!/bin/bash

# Check if exactly two arguments are provided
if [ "$#" -ne 2 ]; then
    echo "Usage: $0 <source_path> <replacement_path>"
    exit 1
fi

SOURCE_PATH=$1
REPLACEMENT_PATH=$2

# Recursively find all .go files and replace the source with the replacement
# find . -type f -name "*.go" -exec sed -i '' "s|$SOURCE_PATH|$REPLACEMENT_PATH|g" {} +

# For Linux (uncomment the line below and comment the macOS line above if necessary)
find . -type f -name "*.go" -exec sed -i "s|$SOURCE_PATH|$REPLACEMENT_PATH|g" {} +

echo "Replaced '$SOURCE_PATH' with '$REPLACEMENT_PATH' in all .go files."
tlimoncelli commented 1 month ago

Thanks for the detailed research!

We have to retain the feature until June 2024 because we (Stack Overflow) have an internal system that depends on it. That system will be sunset in June 2024. Our other option is to build a replacement cert renewal system then throw it away in June 2024. As you may guess, we'd prefer to the option that requires no effort. :-)

I think it is worth it to test v3 and try getting v4 to work.

jauderho commented 1 month ago

It looks like the v3 version of lego still relies on a bad version of go-jose.

This is the error that I get when I try using lego v4.

# github.com/StackExchange/dnscontrol/v4/pkg/acme
pkg/acme/acme.go:144:54: not enough arguments in call to client.Certificate.Renew
    have (certificate.Resource, bool, bool)
    want (certificate.Resource, bool, bool, string)