go-acme / lego

Let's Encrypt/ACME client and library written in Go
https://go-acme.github.io/lego/
MIT License
7.84k stars 1.01k forks source link

Creating Present/Cleanup methods providing just fqdn, value as arguments ? #720

Open mdbraber opened 5 years ago

mdbraber commented 5 years ago

Hi,

I'm writing acme-proxy to relay dns challenges using the httpreq provider and integrating with lego DNS providers. It's easy to write a "RAW" method that calls the Present() and Cleanup() methods using domain,token and keyAuth arguments.

I'd also like to write a version that integrates the "default" method that calls a method using just the fqdn and value. But there isn't a constructor that I can call for lego DNS providers with just those two values (and logically it's impossible to reverse value to a keyAuth as it's sha256.

I'm wondering if anyone has a good suggestion to leverage the existing DNS providers. A complete "solution" would of course make a separate constructor with the fqdn and value parameter that is called by the 'outer' constructor that uses the domain, token and keyAuth parameters. That might solve the issue but probably this is considered overkill. OTOH having a constructor that can be called with just fqdn and value for DNS challenges might be useful for other uses cases too.

I'm happy to rewrite the DNS providers if this would be interesting, but first would like to see if this would be the best way to go or other solutions might be better (or just not having a "default" mode for acme-proxy might be just best)

/cc @ldez

mdbraber commented 5 years ago

@ldez any thoughts on this? I'm trying to figure out if there's a way to use lego if I only have the FQDN and Value (TXT record) parameters?

ldez commented 5 years ago

I think this is a edge-case and that there are some design issues. For example, the Present / Cleanup methods are defined by the Provider interface.

I think that the RAW mode allow to meet acme-proxy needs.

mdbraber commented 5 years ago

@ldez thanks for your reply! The RAW mode is indeed great for outgoing requests to (in this case) acmeproxy using httpreq that are passed on to the lego Present / CleanUp methods.

My question is how to make use of lego as a backend when for handling incoming requests which are issued through acmeproxy. As an example: when I want to use acmeproxy with acme.sh the following takes place:

  1. solve a challenge with acme.sh using a new httpreq provider
  2. send fqdn, value to acmeproxy
  3. acmeproxy receives fqdn, value
  4. acmeproxy creates TXT record based on fqdn, value using lego as backend

Step 1-3 work great, step 4 is not possible yet with lego because it only accepts unsolved challenges with domain, keyauth. But all the relevant logic for creating TXT records is there.

As lego is great at providing interfaces to all possible DNS providers, it's the best (and only choice as far as I can see) that can solve ACME challenges using Go. The problem is that presented challenges are already solved (and can't be unsolved) and so there's no way to use the DNS providers from lego, even though all the relevant logic is there.

As I see (and I'm sure I might be mising something), the logic for most (all?) DNS providers is exactly like is described in the wiki entry Writing a Challenge Solver:

func (d *DNSProviderBestDNS) Present(domain, token, keyAuth string) error {
    fqdn, value := dns01.GetRecord(domain, keyAuth)
    // make API request to set a TXT record on fqdn with value and ttl
    return nil
}

As far as I can see all the DNS providers use the same logic:

  1. get fqdn, value using dns01.GetRecord(domain, keyAuth).
  2. make API request to set a TXT record on fqdn with value and ttl

It could be easy to make the //make API request to set a TXT record on fqdn with value and ttl-logic a separate constructor (e.g. PresentSolved(fqdn, value), that possibly could be called directly, as well as from Present(domain, token, keyAuth). That would make lego much more widely usable as backend for other implementations (without the need to rewrite all the DNS logic of setting DNS challenge TXT records for each provider already included).

I understand that it's definitely a design decision. I'm wondering if you'd be willing to consider this because it would make lego more valuable as package to be integrated in other projects.

If not I'd be at least curious if you would have any suggestions on what would be a good (clean) approach to solve this using Go.

mdbraber commented 5 years ago

@ldez practical what I'd suggest would be the following:

Update the interfaces in challenge/provider.go to look like this:

type Provider interface {
    Present(domain, token, keyAuth string) error
    CleanUp(domain, token, keyAuth string) error
}

type DNSProvider interface {
    Provider
    CreateRecord(fqdn, value string) error
    RemoveRecord(fqdn, value string) error
}

As DNSProvider also implements Provider it should be usable wherever a Provider inteface is required.

Next to creating this interface it would be necessary to update func NewDNSChallengeProvider(name string) (challenge.Provider, error) in providers/dns/dns_providers.go to return a challenge.DNSProvider instead.

This would mean that the providers in providers/dns need to be updated to implement a CreateRecord and RemoveRecord method. This would make the already existing functionality in lego much wider applicable, without giving up on flexibility.

I'd be happy to submit a PR for that if you'd agree this would be a good way to offer this functionality.

ldez commented 5 years ago

Those interfaces must not be changed because they are used in a lot of projects.

mdbraber commented 5 years ago

@ldez the Provider interface would stay the same. it would only add an extra interface DNSProvider with additional functionality (or it could be called ProviderSolved if it would conflict with DNSProvider). Or am I missing that this would screw up many other things? I'm trying to see if this can be done with minimal impact and understand that it would need some good designing. But it would be unfortunate if it would need a fork of lego to replicate already existing functionality of creating/removing DNS records for ACME validation.

ldez commented 5 years ago

The new interface will be introduce breaking changes on exported methods. And I'm not agree with your design proposal.

I don't see any way to do what you want without break the public contract of the lego API.

could you explain why you use acme.sh instead of Lego, Traefik or Caddy?

mdbraber commented 5 years ago

@ldez first, thanks for taking the time to give feedback - I'm trying to figure this out, but have much less experience in using good design patterns than most of you, so happy to understand more of how this would break exported methods. I've tried understanding from reading https://golang.org/ref/spec#Interface_types how this works, but can't see how a new interface breaks exported methods.

Obviously I don't want to break the public contract of the lego API, which is why introducing a new interface would offer an extra option (e.g. a ProviderSolved interface), rather than replacing an existing one.

The design goal for acmeproxy is to be universally useable, independent of the client. Personally I'm using acme.sh because it's part of the (FreeBSD) pfSense router I'm using. For all other parts of my network I'm using Lego and Caddy. Of course I could try and change this, but there would be many more circumstances that might want to make use of fqdn, value (just as why httpreq offers both a default and RAW mode.

mdbraber commented 5 years ago

Could this be a workable idea without breaking anything?

type Provider interface {
    Present(domain, token, keyAuth string) error
    CleanUp(domain, token, keyAuth string) error
}

type ProviderSolved interface {
    Provider
    CreateRecord(fqdn, value string) error
    RemoveRecord(fqdn, value string) error
}

We can then try to cast a Provider interface into a ProviderSolved interface where needed, like such:

p, err := dns.NewDNSChallengeProviderByName(config.Provider)
if err != nil {
    panic(err)
}

provider, ok := p.(ProviderSolved)
if ok {
    // this provider implements ProviderSolved
} else {
    // this is a regular Provider
}

/EDIT: we could even do without creating the ProviderSolved interface separately and just check in place like such:

if provider, ok := p.(interface{CreateRecord(),RemoveRecord()}); ok { 
  // this provider offers CreateRecord and RemoveRecord methods
} 

This would mean that we could rewrite DNS providers that want to offer an export CreateRecord / RemoveRecord functionality without changing much else.

ldez commented 5 years ago

I need time to think about the best solution.

mdbraber commented 5 years ago

Going through the different options it would be enough if there would be an exported method that accepts fqdn, value. Thanks for considering this @ldez

mdbraber commented 5 years ago

@ldez I recently worked on this a bit more - any thoughts in the meantime? I've submitted https://github.com/go-acme/lego/pull/883 as an example of how I've solved this.

mholt commented 4 years ago

This is a low-friction change which has really profound consequences (in a good way). I'd use a feature like this to dynamically update DNS A/AAAA records if my machine changes IP addresses, before obtaining a certificate.

mdbraber commented 4 years ago

I need time to think about the best solution.

@ldez any ideas if / how we can get this on the road?

mdbraber commented 4 years ago

Hi! Just trying to check in to see if this could still be considered or if there could be any alterantive? The change above would be useful and seem rather low-fricting like @mholt mentions. Would you consider it @ldez?

mholt commented 4 years ago

(To clarify, as an update to my previous comment, while I do still think this could be a low-friction change, I have since found workarounds for my use case and don't need to use it myself, anyway.)

mdbraber commented 4 years ago

@mholt did you find a way to use lego with fqdn,value as input or did you find a different workaround altogether?

mholt commented 3 years ago

@mdbraber Ha, sorry, I lost track of this issue... anyway, I have since written an alternate ACME library with a different API for DNS providers (and the DNS challenge altogether).

Blackclaws commented 4 months ago

@ldez So this has been stale for a couple of years now. Is there any clue on how we might move forward with generic record creation or is this simply out of scope for lego and acmeproxy will have to fall back to using RAW mode always?

mholt commented 4 months ago

@Blackclaws If arbitrary/generic DNS record manipulation is what you need, then libdns might be what you're looking for.