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

docs: Update ZeroSSL issuer for v2.8 #414

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kekalainen
Copy link

Addresses the changes in caddyserver/caddy#6229 to the best of my understanding.

Particularly, marks the API key for the ZeroSSL issuer as required.

Documentation for the fields previously denoted to be inherited by ... should still be provided, but I am not familiar with those.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Oh wow, thanks. Not sure how I forgot to update this!


```caddy-d
... zerossl [<api_key>] {
... zerossl <api_key> {
...
Copy link
Author

Choose a reason for hiding this comment

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

The omitted fields here should be documented.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, here they are from the Godoc:

//	... zerossl <api_key> {
//		    validity_days <days>
//		    alt_http_port <port>
//		    dns <provider_name> ...
//		    propagation_delay <duration>
//		    propagation_timeout <duration>
//		    resolvers <list...>
//		    dns_ttl <duration>
//	}

Copy link
Author

Choose a reason for hiding this comment

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

Added, save for describing validity_days.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Maybe we should refer the users to the ZeroSSL API docs anyway. But for that field it's basically either 365 or 90.

@kekalainen kekalainen marked this pull request as ready for review September 6, 2024 12:31
@@ -359,22 +359,31 @@ Obtains certificates using the ACME protocol. Note that `acme` is a default issu

- **any_common_name** <span id="any_common_name"/> is a list of one or more common names; Caddy will choose the first chain that has an issuer that matches with at least one of the specified common names.

<aside class="tip">

The `acme` issuer module will implicitly use [ZeroSSL's ACME endpoint](https://zerossl.com/documentation/acme/) (and generate EAB credentials) if you specify the the [`email` global option](/docs/caddyfile/options#email).
Copy link
Member

Choose a reason for hiding this comment

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

So, this is only true if it's unspecified in the config. If you specify an issuer in the config, that overrides the defaults. Since this is docs describing how to override the defaults, maybe it's not the place to describe the default behavior which might be confusing.

Copy link
Author

Choose a reason for hiding this comment

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

I see. Removing the paragraph altogether would muddy the difference between the acme and zerossl issuer modules, though. Perhaps prefixing it with "When explicitly unconfigured" would do?

Copy link
Member

Choose a reason for hiding this comment

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

What if instead, we add a sentence to the zerossl section that clarifies: "The ZeroSSL API is distinct from its ACME endpoint." or something like that.

Copy link
Author

Choose a reason for hiding this comment

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

While that'd cover the distinction more directly, the acme section still claims the module is "using Let's Encrypt" by default. For consistency either all or none of the possible implicit/default configuration should be mentioned IMO.

Especially since the release notes claim that when the email global is configured,

you don't have to make any changes and you'll still get Let's Encrypt and ZeroSSL automatically as defaults.

which, turns out, is arguably not true if acme is explicitly configured. That's not apparent (to me anyway) when said configuration is not providing a directory_url as an argument nor field value. (For context, I use the acme issuer directive to override the DNS resolvers used for challenges (since those aren't globally configurable AFAIK) and had the false intuition it'd leave the dirs intact.)

Copy link
Member

Choose a reason for hiding this comment

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

the acme section still claims the module is "using Let's Encrypt" by default. For consistency either all or none of the possible implicit/default configuration should be mentioned IMO.

Fair point, maybe that section needs to be tweaked to say "(using Let's Encrypt, and if an email is provided, ZeroSSL too)" or similar.

which, turns out, is arguably not true if acme is explicitly configured. That's not apparent (to me anyway) when said configuration is not providing a directory_url as an argument nor field value.

Perhaps verbiage clarifying that specifying any issuers wipes out implicit defaults.

Copy link
Member

Choose a reason for hiding this comment

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

What's the status on this at this point?

Copy link
Member

Choose a reason for hiding this comment

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

I think we're still waiting for a few tweaks to be made; but if not, I might try to wrap this up myself at some point. Although, I don't think I have push rights to the branch.

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