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

Switch to mainline certmagic (closes #19) #29

Merged
merged 6 commits into from
Feb 9, 2024
Merged

Conversation

Lemmmy
Copy link
Member

@Lemmmy Lemmmy commented Jan 28, 2024

This PR removes the need for tmpim/certmagic and switches back to using mainline certmagic. The majority of the changes involved in switching to certmagic are trivial, but there are major breaking changes in the DNS providers, since certmagic now uses libdns instead of lego. Those changes are documented in tmpim/dnsproviders#1, which will need to be merged before this PR (this has now been done).

The following other changes were made:

  • Configuration parsing now supports nested blocks, backported from the dispenser code in Caddy v2. This has not been extensively tested.
  • Casket was updated to build with Go 1.19. If possible, I'd like to catch back up with 1.21, but a lot of our dependencies (e.g. quic-go) had major breaking changes, even in just this version bump.
  • The tls.dns.cloudflare provider now only supports scoped zone tokens. I figured this was worth mentioning here as it's the biggest breaking change in dnsproviders, and most people (to my knowledge) use it if they're not using the HTTP-01 challenge or self-signed certificates + a proxy like Cloudflare.
  • Telemetry events for certificate-related actions were removed.

There are also a few things I didn't do in this PR, which may need addressing before being merged. I'm not enough of an expert in Go to know what to do here.

  • certmagic now uses contexts in every function. Casket doesn't use these anywhere, so I had nothing to pass in here. I've passed context.TODO() to everything. I'd imagine using contexts properly would require some large architectural changes to Casket, which is outside the scope of this PR. I hope this doesn't decrease stability in any way? I haven't yet tested bulk certificate renewal.
  • certmagic expects zap loggers to be passed around, in our case only to the CleanUpOwnLocks function. I haven't set anything up for this, but have imported the module to pass the no-op logger:
    casket.OnProcessExit = append(casket.OnProcessExit, func() {
    // TODO: Redirect to our own logger instead of zap.NewNop()
    certmagic.CleanUpOwnLocks(context.TODO(), zap.NewNop())
    })
  • RevokeCert now expects reasons to be passed. I think this function is only called via user-intervention, so perhaps a CLI change is necessary here? For now I just pass 0, meaning "unspecified".

    casket/caskettls/tls.go

    Lines 78 to 84 in dd12a8a

    // Revoke revokes the certificate for host via the ACME protocol.
    // It assumes the certificate was obtained from certmagic.CA.
    func Revoke(domainName string) error {
    // TODO: Bubble down certificate revocation reasons per RFC 5280. Is this function only ever called by human
    // interaction?
    return certmagic.NewDefault().RevokeCert(context.TODO(), domainName, 0, true)
    }
  • Changes in quic-go have removed the wrapping Server argument here. I haven't tested QUIC after this change, other than running the go tests, so I'm not sure if this replacement is correct:
    // enable QUIC if desired (requires HTTP/2)
    if HTTP2 && QUIC {
    // As of https://github.com/quic-go/quic-go/pull/3397 this longer directly accepts a `Server` field
    // TODO: Verify this correct
    s.quicServer = &http3.Server{
    Addr: s.Server.Addr,
    Handler: s.Server.Handler,
    TLSConfig: s.Server.TLSConfig,
    MaxHeaderBytes: s.Server.MaxHeaderBytes,
    }
    }

casket-certmagic-1.4.0-pre4-linux_amd64
casket-certmagic-1.4.0-pre4-linux_arm64
casket-certmagic-1.4.0-pre4-linux_armv6
casket-certmagic-1.4.0-pre4-linux_armv7
casket-certmagic-1.4.0-pre4-windows_amd64

Docker image for Linux amd64:

curl -sL https://lemmmy.pw/casket-tmp-builds/casket-1.4.0-pre4-linux_amd64-docker.tar | docker load

Still a lot to do. Everywhere that `context`s are used have currently
been passed `context.TODO()`. A logger should be passed to certmagic's
lock cleanup function. Most importantly, though, it doesn't seem to be
possible to map certmagic's LibDNS providers to Lego's. In the next
commit
I will try to see how much work it is to switch to using LibDNS instead,
 which will work natively with Certmagic. The plugin registration will
 need to change, and constructors may need to be written for each
 provider. Best-case, there should be minimal changes to the
 configuration and documentation.
Updates go to 1.19. It's not the latest, but was the most compatible
version I could bump up to while requiring the least changes. quic-go
had to be updated a few versions (again, not the latest) and required
minor changes.

This PR updates all tests to work for the certmagic changes, and they
are now all passing, except for the plugin count test.

Next up is to change all of the DNS providers in tmpim/dnsproviders to
use libdns. Eventually each provider should just become minimal glue
that takes `credentials ...string`, and all the environment variables
lego supported, and returns the configured libdns provider.

A temporary Cloudflare provider has been added in
`caskettls/dnsproviders.go` to show what that would look like. The
Cloudflare provider update already has a breaking change; legacy auth
tokens are no longer supported.
The config parser now supports nesting, and all of the DNS providers
support configuration via blocks.
@Lemmmy
Copy link
Member Author

Lemmmy commented Jan 28, 2024

New builds posted with the above fix - fixes config returned for certificate is not nil and points to different cache for certificates generated from a site config that doesn't have a tls block.

@Lemmmy Lemmmy marked this pull request as ready for review February 1, 2024 04:04
@Lemmmy
Copy link
Member Author

Lemmmy commented Feb 1, 2024

Added a Docker build for linux amd64. Now live in production on nozomu and everything seems to be working.

Copy link
Member

@1lann 1lann left a comment

Choose a reason for hiding this comment

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

looks reasonable enough to me, if it works it works. I'd release a beta version first for people to try out on a new minor version (1.4.0-beta.1 or something?)

@Lemmmy
Copy link
Member Author

Lemmmy commented Feb 2, 2024

Sounds good, will do. Thanks!

@Lemmmy Lemmmy merged commit 768979f into master Feb 9, 2024
1 check failed
@Lemmmy Lemmmy deleted the update-certmagic branch February 9, 2024 10:34
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.

3 participants