Conversation
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.
Member
Author
|
New builds posted with the above fix - fixes |
Member
Author
|
Added a Docker build for linux amd64. Now live in production on nozomu and everything seems to be working. |
1lann
approved these changes
Feb 2, 2024
Member
1lann
left a comment
There was a problem hiding this comment.
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?)
Member
Author
|
Sounds good, will do. Thanks! |
Lustyn
approved these changes
Feb 4, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
tls.dns.cloudflareprovider 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.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.
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.CleanUpOwnLocksfunction. I haven't set anything up for this, but have imported the module to pass the no-op logger:casket/casket/casketmain/run.go
Lines 89 to 92 in dd12a8a
RevokeCertnow 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 pass0, meaning "unspecified".casket/caskettls/tls.go
Lines 78 to 84 in dd12a8a
casket/caskethttp/httpserver/server.go
Lines 105 to 115 in d529a84
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