luci-app-acme: rename keylength to key_type

[stokito]

The acme.sh package was changed recently:

• key_size option were renamed to key_type
• the webroot option is now should be a fixed path /var/run/acme/challenge to simplify configuration as part of acme: use the hotplug system packages#17721

@tohojo @hgl Please explain how to specify that I do really want to use a webroot but with the new /var/run/acme/challenge folder. The entire webroot option was deprecated so how the acme can distinguish webroot with a custom path like /var/www/acme/ and the new folder?
For now I made the /var/run/acme/challenge as a default value. So in the script you can just check if it's not the default value or empty and only then write a deprecation notice.

The current validation method is standalone, maybe we should change it to a webroot?

@jow- Please give an advice. We have a DNS API and for example for the DuckDNS:

config cert 'duckdns_wildcard'
  option validation_method 'dns'
  option dns 'dns_duckdns'
  list credentials 'DuckDNS_Token="6b52e9da-5229-4304-b846-187355f9q91e"'
  list domains 'example.duckdns.org'
  list domains '*.example.duckdns.org'

The problem is that the credentials is list of properties but I wish to turn it into a plain form. So when selecting a provider it's specific fields will be shown.
E.g. for the DuckDNS just show a field "Token" but on the form save it will be added to the list credentials 'DuckDNS_Token. How to make this saving?

For each provider we need a separate fields but it should be enough to just support a few main (Gandi, Cloudflare) and some standard like acmedns and nsupdate.

CC @jannispinter @zhanhb

[hgl]

webroot should not be customizable. All httpds should just use the standardized path.

Listing all credentials for all dns providers is impossible, since acme tries to be client agnostic. Users are expected to specify values pertain to the client they use. In other words, the Token example you use might only work for acme-acmesh, but not acme-uacme (if it’s created in the future).

[stokito]

Ok, I added a DNS API specific fields. At first step only some popular but later I'll add all of them. I just want to make a cleanup in the acme.sh itself because now it's a mess.

So a user now see DuckDNS Token field which on saving will be stored to the credentials.

The credentials field will be anyway visible and this may be confusing. I think that this is negligible a trade off.

[hgl]

@stokito First off, I want to thank you for taking the time writing this. Listing the DNS values must be a nontrivial work.

However, as I mentioned previously, the values like dns_cf are acme.sh specific. If this package absolutely has to explicitly list them, they should at least be nested under acme.sh.

I personally think we shouldn't explicitly list them. Users are expected to check OpenWrt documentation on acme (which should be updated) and use the dns value for the client they choose (e.g, acme-acmesh, acme-uacme, etc). For acme-acmesh it's dns_cf, for acme-uacme, it might be something else, so this LuCI package should just offer a text field. The same goes for credentials, which are essentially options for dns.

[stokito]

The acme-uacme is not even created yet (right?). But also I honestly don't see any reason why to use it: it's written in C (so needs to be compiled for all routers and waste CI time), depends on gnutls and much bigger than acme.sh. The acme.sh is also bloated but still is smaller than the uacme.

Currently uacme dns-01 supports only nsupdate, CF, gandi, deSEC.io. If in future the uacme will be supported we anyway need to change the UI to at least change translation keys and I'll do that.

Instead of uacme maybe in future the only one alternative ACME client that can be supported is LE certbot. But it's depends on Python and heavy. Maybe for x86 it may make sense.

[hgl]

acme-uacme only serves as an example. There are other potential clients like lego, which also has comprehensive support for dns-01.

I think my point is, it's best if this package is client agnostic. Asking you and other maintainers to keep this package up-to-date whenever a new client is added is unsustainable, and supporting all DNS values for all clients could also bloat this package.

[stokito]

The Lego is bloated too so I don't feel that this will be an issue in nearest years. We can solve problems for users today and once a new problems occur we can solve them latter. But maybe this won't happen if solve a root of a problem of many DSP APIs:

0. The acme.sh and lego (maybe even dnscontrol) may synchronize variable names.
1. Even today users may use relatively simple solution ACME DNS. There are few problems but this is a working solution.
2. The DNS providers may implement some basic standard API for ACME.

If none of them solved we can add a variable transform into OpenWrt acme.

Today most users will use DuckDNS or some popular DNS provider like Gandi and use the acme.sh so this already covers 99% of users. The credetials edit is still visible for users so even if we won't update in future users still can add any credential var that they need.

I really don't see problems here

[hgl]

I'm not sure I understand the solutions you mentioned:

0. Do you mean DNS values should be standardized and each client should convert from them?
1. You want to ask DNS-01 users to host their own name server? How does that "solve the root problem"?
2. I don't understand what you mean by this.

We can solve problems for users today and once a new problems occur we can solve them latter.

We are in circles. The current solution forces maintainers into a corner (supporting all DNS values for all clients). I don't understand why you would dismiss it as a non-issue. The acme package previously tried to update all available httpds after a certificate renews, and we migrated away from that. We should not fall into the same trap here.

[stokito]

Do you mean DNS values should be standardized and each client should convert from them?

Yes but if they don't then we can convert here.

You want to ask DNS-01 users to host their own name server? How does that "solve the root problem"?

It works like: you configure a CNAME for your subdomain to the acme-dns.io subdomain and the TXT record is updated there. This works only for subdomains but covers many cases.

The DNS providers may implement some basic standard API for ACME.

https://community.letsencrypt.org/t/dns-api-standard-or-specification/189887

The current solution forces maintainers into a corner (supporting all DNS values for all clients)

There is no corner. A user can add any variable directly to the credentials input. If the acme.sh has a new provider then a user still can edit a UCI config manually.
I think that eventually all DNS providers will have the same API.
Even now adding a new dns provider is a few simple lines change.

[hgl]

A user can add any variable directly to the credentials input.

Not just credentials, also this:

o.value("dns_duckdns", "DuckDNS.org");

This works for acme.sh but not lego for example.

Yes but if they don't then we can convert here.

There is no way to standardize DNS provider names, since they are endless.

---

I think I've made my point clear and would stop from further reiterating. I'll leave it to @tohojo and @jow- to make the call.

[tohojo]

The whole discussion around multiple backends is a bit hypothetical given that we don't have more than one currently. I don't think we should be limiting ourselves because future clients may not support the same feature set; if this is the case, we can just add a check to the UI and remove/grey out the providers that are not supported by the currently installed backend.

It a second client shows up that supports the same backend, but uses a different variable name to do so, that backend will just have to translate appropriately. Yes, this way we're making the acme.sh variable names the de facto "acme framework" names for those providers, but I don't think that's such a big deal: the names are a bit arbitrary anyway, and they're tied to the (often proprietary) provider anyway, so it's not like we have much say in the matter anyway. The best we can do is maybe bikeshed the names a bit, and sidestepping that by just going with the names that already exist is fine by me...

However, I don't think we should be endorsing any particular provider by setting a default value. Users will need to have an account with a provider before configuring this anyway, so let's just list the providers alphabetically, and they can pick whichever one works for them.

[tohojo]

Not just credentials, also this:

o.value("dns_duckdns", "DuckDNS.org");

This works for acme.sh but not lego for example.

Why not? Because lego doesn't support that provider at all? Then we can just prune the list according to which backend is installed, as mentioned above...

(Side note, I don't think lego is going to be usable on openwrt at all since golang only has limited architecture support, sadly)

[stokito]

Users will need to have an account with a provider before configuring this anyway, so let's just list the providers alphabetically, and they can pick whichever one works for them.

I agree and I even implemented some JS code to pre-selct the dns_duckdns if the domain ends with .duckdns.org and similarly for freedns. But it works with a bug so I didn't included it into the PR.

The main reason why I kept the duckdns by default is that if no default specified then anyway a first item will be selected e.g. 1984.is. So anyway we have a problem here so I decided to keep the DuckDNS by default which I believe should be the most popular option in context of routers.

Generally speaking this can be solved in a nice way.
We can lookup NS record of the domain and determine a provider by it e.g. ns1.gandi.net definetely belongs to gandi. Or a domain may add some TXT like "DNSAPI=gandi" and this can help to autodiscovery.
But this is non necessary complication.

@tohojo JFYI the Lego has a PR for the DuckDNS and also Golang can be compiled to MIPS (I used a Golang program on TPLINK wrn1043nd)

[hgl]

The whole discussion around multiple backends is a bit hypothetical

I'm writing acme-lego as we speak, I don't have an ETA, but it's not very far away.

Why not? Because lego doesn't support that provider at all?

Because this value, as far as acme-common is concerned, is passed directly to the client. It's dns_cf for acme.sh's --dns flag, and cloudflare for lego's, for example. So either acme-lego or this package has to convert dns_cf to cloudflare.

Yes, this way we're making the acme.sh variable names the de facto "acme framework" names for those providers

I want to add that acme-common doesn't care what's the de facto acme framework, it just passes down the specified value to clients.

if this is the case, we can just add a check to the UI and remove/grey out the providers that are not supported by the currently installed backend.

That's another issue. This means this package also needs to have a list of what providers are supported by each client.

The whole name conversion and provider list could be avoided, if this package simply offers a text field like how acme-common handles it.

[tohojo]

Because this value, as far as acme-common is concerned, is passed directly to the client. It's dns_cf for acme.sh's --dns flag, and cloudflare for lego's, for example. So either acme-lego or this package has to convert dns_cf to cloudflare.

Right, but that's kinda fundamental when the clients can't agree, no? Either acme.sh has to convert from "cloudflare" to "dns_cf", or lego has to do the reverse.

The whole name conversion and provider list could be avoided, if this package simply offers a text field like how acme-common handles it.

Right, but that's just bailing out and leaving the user to deal with the incompatibility. If acme-common is supposed to be an abstraction layer, we need to deal with this sort of incompatibility somehow. The goal of having a common format for /etc/config/acme is that the same config file should keep working across client implementations. If we tell users "figure out what your client supports and put the client-specific values into the config file", then what's the point of having the common framework?

[hgl]

The goal of having a common format for /etc/config/acme is that the same config file should keep working across client implementations

I'm not sure this was considered when developing acme-common. Even the same config can mean different things for different clients. For example, days mean how many days have passed in acme.sh, but how many remains in lego. The calias and dalias options aren't guaranteed to work the same way across different clients either.

Precisely define what each config means and have different clients do the conversion is not really feasible, even if it is, users aren't supposed to switch clients frequently, is it really worth every client maintainer's effort to support this?

IMHO acme-common exists mostly to address code reuse.

[hgl]

Another difference: days is only consulted when issuing in acme.sh, but for lego, it's only consulted when renewing. There is no way to make lego behave the same as acme.sh. There can be many subtle differences like this.

The point is, have an universal config that works across clients is a really hard promise to keep.

[stokito]

A small off topic:

On the UI we need to show a link to acme.sh Wiki with instructions for the DNS provider.
There are two pages dnsapi and dnsapi2.

I just added anchors for each DNS provider with the same name as it's code e.g. https://github.com/acmesh-official/acme.sh/wiki/dnsapi#dns_aws
And for providers on the dnsapi2 page I added links e.g. https://github.com/acmesh-official/acme.sh/wiki/dnsapi#dns_porkbun

So now we need to add some JS handler that will change the link to a wiki when the DNS provider dropdown is changed

[stokito]

Ok, so I made the dynamic URL to DSN API Wiki with instructions.
Also added more fields for other providers.
Please merge

[tohojo]

I'm not sure this was considered when developing acme-common. Even the same config can mean different things for different clients. For example, days mean how many days have passed in acme.sh, but how many remains in lego. The calias and dalias options aren't guaranteed to work the same way across different clients either.

Precisely define what each config means and have different clients do the conversion is not really feasible, even if it is, users aren't supposed to switch clients frequently, is it really worth every client maintainer's effort to support this?

Yes, I definitely think we should strive for compatibility. It may not be achievable for all things, but it's fine to have some things that are only supported by one client, when that is not avoidable. Then we can just disable those config options when an incompatible client is installed (or consider not supporting such options at all if they're not really needed). Having different semantics for the same config option we should definitely avoid, though! In that case they should have different names (and only work for the client that supports them).

It's not really clear to me why supporting a lot of different clients is useful, though. I can maybe see the value of supporting uacme assuming it can be made smaller than acme.sh (since acme.sh pulls in the openssl binaries), but what's the benefit of lego, exactly? :)

[stokito]

@tohojo I fixed your comments, please check

[stokito]

@tohojo It turns out that we can allow only to list files without reading them. So I restored the certificates tab because I believe it will be useful.

[hgl]

it's fine to have some things that are only supported by one client, when that is not avoidable.

For a new client author, finding out what options are unsupportable is the easy part. It doesn't create compatibility issues since the client can just ignore the options.

Having different semantics for the same config option we should definitely avoid, though!

The hard part is that author has to be aware it's different before she can avoid. Unless we provide a clear document or the author is very familiar with acme.sh, it can be really hard to discover days might not mean what she thought it meant for example. The extra effort involves understanding how the options work in acme.sh and how they differ from the new client.

Another big issue is that how credentials should be supported. Currently, this package uses acme.sh specific values like CF_Zone_ID, which isn't supported by lego. It either has to translate all these environ variables (not an easy task, since in acme-common we just directly eval export them) or just ask users to use different credential values, breaking the compatibility promise we want to keep. What do you think we should do in this regard? (If we should standardize credentials too, then maybe they should just be regular options in acme-common).

This package, as it is, means it will be very difficult for me to contribute the lego client.

what's the benefit of lego, exactly?

It has both comprehensive DNS support and no dependencies on openssl. acme.sh and uacme only has either one of the benefits. It shouldn't matter unless we want to endorse acme.sh and discourage other new clients.

It's not really clear to me why supporting a lot of different clients is useful, though.

I'm not sure this is a good strategy. The original goal was that writing a new client should be easy, since it can leverage a lot of existing code from acme-common, but now it becomes that writing a new client is discouraged, because we thought "supporting a lot of different clients isn't useful" and it requires deep understanding of acme.sh first.

Even if good acme clients are few now (which I'm not sure is true), the acme spec can still innovate and warrants new client to pop up.

[hgl]

To provide a more concrete example. Let’s say the lego package wants to support days, what it should do? Since converting remaining days to days passed requires parsing the notAfter value from the certificate, and the lego cli doesn’t offer that, the package has to introduce another option like days_remain. So now the user has to modify the config any way in order for it to work the same way after switching from acme.sh to lego. And what should this LuCI package do? Detecting what client is used and hide show options as needed? Plus the DNS provision list it has to support, this quickly gets out of hand.

I want to propose that we standardize option names, not their precise meanings. This means we shouldn’t standardize option values, unless it’s a well defined one like key_type (and doesn’t have many values, so it’s manageable for clients to convert from).

This makes writing both new client packages and this package much simpler. The only downside is that users are required to modify config when they switch clients. I think it should be obvious it’s a good trade? And as I mentioned, that downside can’t be eliminated anyway.

[stokito]

@tohojo @systemcrash I fixed many small issues and re-committed with more clear changes.
I feel that the PR is ready or at least good enough to be merged.
There are already a lot of changes and if anything else remain we can make a separate PR.
Please note that current users have a broken luci-app-acme that doesn't correspond for a new version of acmesh.

[tohojo]

Right, so after reviewing this discussion again: if the expectation is that the supported fields and/or there semantics is going to vary between different clients, that means that we're going to have to make adjustments in the UI depending on which client is installed in any case. In which case I don't see any obstacles to merging this, including the DNS provider list. @hgl WDYT?

[hgl]

that means that we're going to have to make adjustments in the UI depending on which client is installed in any case

Not if we only standardize option names and not values, but I guess making the UI offer values does make it more user friendly. I can get behind it since you gave the green light.

And maybe we only need to consider this once multi-client support is really needed, but just to put it out there: we might need to consider what should happen if the user uninstalls the current client and installs a new one. For new options, maybe we can just display an empty value. For options with the same name but different values (e.g., DNS provider), the current value is invalid, maybe the field should be marked with error or something?

[tohojo]

Not if we only standardize option names and not values, but I guess making the UI offer values does make it more user friendly. I can get behind it since you gave the green light.

Cool. Merging, then :)

And maybe we only need to consider this once multi-client support is really needed, but just to put it out there: we might need to consider what should happen if the user uninstalls the current client and installs a new one. For new options, maybe we can just display an empty value. For options with the same name but different values (e.g., DNS provider), the current value is invalid, maybe the field should be marked with error or something?

Yeah, hard to discuss all that in the abstract, let's evaluate once we actually have another client implementation to look at...

[stokito]

thank you all!

[stokito]

FYI: I sent a PR to acmesh where proposed to get automatically the list of DNS provides and their options acmesh-official/acme.sh#4738

[LokeYourC3PH]

So, I had to change this manually in my config still in /etc/confic/acme from keylength to key_type. When can we expect this to get fixed & pushed in the luci-app-acme LuCI GUI?

[stokito]

@LokeYourC3PH it's already merged and available. About a month ago I also added a script to migrate the old options to new.

FYI: You can remove the key_type and by default it will be ec256 which is shorter and supported by all modern servers.

[LokeYourC3PH]

@LokeYourC3PH it's already merged and available. About a month ago I also added a script to migrate the old options to new.

FYI: You can remove the key_type and by default it will be ec256 which is shorter and supported by all modern servers.

Well whatever it is, on my Linksys E7350 (ramips/mt7621) running OpenWRT 23.05.3 r23809-234f1a2efa, the latest luci-app-acme (Version: git-23.074.38708-acf40dc) still creates those values in the config when it shouldn't. The option for "Key Size" which writes key_length

should write the key_type instead, and only have RSA2048 or EC256 as an option available, yet it doesn't, hence why I was curious when that will be changed.

[stokito]

Maybe the change wasn't backported yet to the 23.05 (i.e. May of 2023, the PR was merged later).
This is a minor thing and the old keysize option is still supported.

[LokeYourC3PH]

Maybe the change wasn't backported yet to the 23.05 (i.e. May of 2023, the PR was merged later).
This is a minor thing and the old keysize option is still supported.

It's as a matter of fact completely deprecated and doesn't work as it seems:

daemon.warn acme: Option "keylength" is deprecated, please use key_type (e.g., ec256, rsa2048) instead.

[tohojo]

Ryan ***@***.***> writes:

> Maybe the change wasn't backported yet to the 23.05 (i.e. May of 2023, the PR was merged later). > This is a minor thing and the old keysize option is still supported. It's as a matter of fact completely deprecated and doesn't work as it seems: `daemon.warn acme: Option "keylength" is deprecated, please use key_type (e.g., ec256, rsa2048) instead.`

The code that emits this warning goes on to convert the old value, so things should work just fine apart from that message in the logs...