luci-app-acme: improve UI for inexperienced users #7147

stokito · 2024-06-01T19:37:49Z

Maintainer: @tohojo

A cleanup of translations.
Autodetect of domain when adding
Import domains from DDNS
Logging tab

tohojo

I generally applaud the effort to make things more user friendly, but have some comments, see below.

Also a general one: You're adding new reads to the file system but not updating the acl config accordingly. So will things actually work correctly? How did you test all the permutations?

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

tohojo · 2024-06-03T08:29:50Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

+		o.value('standalone', _('Standalone'));
+		o.value('webroot', _('Webroot'));
+		o.value('dns', _('DNS'));
+		o.default = 'webroot';


Why this change? I personally consider the validation method an "advanced" setting, it should be possible for a user to just specify the domain and have everything work without caring about the validation method, no?

A user must at least check the validation method if the DNS is used. Only for the DNS we allow wildcards.

Sure, but the domain is still the primary input, and the validation method is a detail for special cases (I don't think wildcard certificates are terribly common).

I still don't think we should swap these; please drop this commit.

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

stokito · 2024-06-03T11:29:49Z

Please squash before merging, I made many commits to just simplify the review.

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js

stokito · 2024-06-07T19:54:09Z

Chrome console: _isFqdn('0a.net') true _isFqdn('0a') false _isFqdn('192.168.1.1') false _isFqdn('[::1]') false The wildcards are out of context here. The main goal of the function is to determine if the browser URL can be used as a domain e.g. `example.com` - ok, just `router` - not ok.

…

On 07/06/2024 22:46, Paul Donald wrote: function _isFqdn(domain) { + // Is not an IP i.e. starts from alphanumeric and has least one dot + return /[a-z0-9-]\..*$/.test(domain) && !/[0-9-]\..*$/.test(domain);

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js

tohojo · 2024-06-10T09:49:39Z

Chrome console:
_isFqdn('0a.net')
true _isFqdn('0a')

Right, but:
_isFqdn("a0.net")
false

Maybe something like:

function _isIP(str) {
  return /^[0-9\.]+$/.test(str) || /[\[\]:]/.test(str);
}

function _isFqdn(str) {
  return !_isIP(str) && /\./.test(str);
}

tohojo · 2024-06-10T09:50:27Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js

@@ -648,7 +648,7 @@ function _collectDdnsDomains() {
 		if (credentials.length > 0) {
 			ddnsDomains.push({
 				sectionId: ddnsService['.name'],
-				domains: [ddnsService['domain']],
+				domains: [ddnsService['domain'], '*.' + ddnsService['domain']],


Why? I don't think we should be encouraging people to randomly issue wildcard certs...

This import functionality is intended for non experienced users.
The DDNS is most likely used for personal and home usage so wildcard is not that risky. People may want to use subdomains for separate home services like HomeAssistant.my.ddns.test or NextCloud.my.ddns.test. The wildcard will significantly simplify things. If someone is paranoid they are free to edit manually the generated config.

That's a big assumption; those services may not even be running on the router. I don't think we should be doing the less secure thing by default. Add a note about it, sure, but don't just add the *.

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js

stokito · 2024-06-10T09:57:43Z

Normally the message should never occur. It would be better to let a user to know for any duplicate. Or we can just override and a user will check the Changes to apply to see what changed

stokito · 2024-06-10T10:03:13Z

Nothing wrong with wildcards per se. Having them by default may simplify next configurations steps for new subdomains e.g. adding of nextcloud.example.duckdns.org. An advanced user can remove the wildcard if not needed.

stokito · 2024-06-10T10:06:38Z

Yes, the alert() is annoying, still it's much easier to implement it. Ideally a user should never see it

tohojo · 2024-06-10T10:19:52Z

Sergey Ponomarev ***@***.***> writes:

Nothing wrong with wildcards per se. Having them by default may simplify next configurations steps for new subdomains e.g. adding of nextcloud.example.duckdns.org.

I disagree. A wildcard certificate assigns a lot more trust to the device than a single domain, so the principle of least privilege dictates that these should only be used when absolutely necessary.

An advanced user can remove the wildcard if not needed.

No, an advanced user can add it if needed :)

tohojo · 2024-06-10T10:21:27Z

Sergey Ponomarev ***@***.***> writes:

Yes, the alert() is anoying, still it's much easier to implement it. Ideally a user should never see it

No, don't offload pain on the user to make things easier to implement. "Should never see it" is not a good argument, it will definitely happen more than expected.

systemcrash · 2024-06-10T11:57:45Z

Do not use alert. It's is extremely difficult to manage with keyboard only.

systemcrash · 2024-07-22T22:35:56Z

Please remove alert and sync i18n.

systemcrash · 2024-11-23T00:14:04Z

ping @stokito

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

The article is "Get a free HTTPS certificate from LetsEncrypt for OpenWrt with ACME.sh" Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

A user must specify the validation_method first. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

In the 585df1d I changed the validation_method to webroot by mistake because I thought this is a default in the acme.sh. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

We can't just use the datatype = "list(hostname)" because a domain may have a wildcard. So check the domain by a simple regexp. Check that DNS mode is used for wildcard. Make the wildcard allowed only the beginning. Add lowercase requirement. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

…S mode is used Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Check if the hostname is FQDN (e.g. has least one dot). Check if the domain in the browser is not an IP and FQDN. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Many users already have a DDNS configured e.g. DuckDNS.org or Cloudflare. We can import the configurations to simplify configurations and avoid mistakes. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

stokito · 2025-07-08T21:04:04Z

Please re-review the PR. Now it doesn't contains changes in stings to make it easier to review and merge. I'll send another small PR for them.

tohojo · 2025-08-12T13:05:39Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

+			)
+		);
+		o.depends('acme_server', '');
+		o.depends('acme_server', 'letsencrypt');


This won't work - the acmesh script has this logic:

if [ "$acme_server" ]; then set -- "$@" --server "$acme_server" # default to letsencrypt because the upstream default may change elif [ "$staging" = 1 ]; then set -- "$@" --server letsencrypt_test else set -- "$@" --server letsencrypt fi

So if acme_server is set to letsencrypt, the staging field won't actually do anything.

tohojo · 2025-08-12T13:07:33Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

+		o.value('standalone', _('Standalone'));
+		o.value('webroot', _('Webroot'));
+		o.value('dns', _('DNS'));
+		o.default = 'webroot';


I still don't think we should swap these; please drop this commit.

tohojo · 2025-08-12T13:13:18Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

@@ -16,11 +16,13 @@ return view.extend({
 				}
 				return certs;
 			}),
+			L.resolveDefault(fs.stat('/usr/lib/acme/client/dnsapi'), null),


You're not adding this to the ACL file

tohojo · 2025-08-12T13:18:21Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

+function _isFqdn(domain) {
+	// Is not an IP i.e. starts from alphanumeric and has least one dot
+	return /[a-z0-9-]\..*$/.test(domain) && !/[0-9-]\..*$/.test(domain);
+}


This test doesn't work:

_isFqdn("a1.com") false

tohojo · 2025-08-12T13:19:44Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme.js

@@ -18,6 +19,7 @@ return view.extend({
 			}),
 			L.resolveDefault(fs.stat('/usr/lib/acme/client/dnsapi'), null),
 			L.resolveDefault(fs.lines('/proc/sys/kernel/hostname'), ''),
+			L.resolveDefault(uci.load('ddns')),


...nor this

tohojo · 2025-08-12T13:28:45Z

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js

@@ -648,7 +648,7 @@ function _collectDdnsDomains() {
 		if (credentials.length > 0) {
 			ddnsDomains.push({
 				sectionId: ddnsService['.name'],
-				domains: [ddnsService['domain']],
+				domains: [ddnsService['domain'], '*.' + ddnsService['domain']],


That's a big assumption; those services may not even be running on the router. I don't think we should be doing the less secure thing by default. Add a note about it, sure, but don't just add the *.

stokito force-pushed the luci-app-acme branch from 72be42e to ac3e058 Compare June 1, 2024 19:39

stokito changed the title ~~Luci app acme~~ luci-app-acme: improve UI for inexperienced users Jun 1, 2024

stokito force-pushed the luci-app-acme branch from ac3e058 to 30e8b84 Compare June 2, 2024 16:04

tohojo requested changes Jun 3, 2024

View reviewed changes

stokito requested a review from tohojo June 3, 2024 11:20

tohojo reviewed Jun 4, 2024

View reviewed changes

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js Outdated Show resolved Hide resolved

stokito requested review from tohojo, dannil and systemcrash June 7, 2024 08:04

systemcrash requested changes Jun 7, 2024

View reviewed changes

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js Outdated Show resolved Hide resolved

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js Show resolved Hide resolved

dannil reviewed Jun 7, 2024

View reviewed changes

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js Outdated Show resolved Hide resolved

tohojo reviewed Jun 10, 2024

View reviewed changes

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js Outdated Show resolved Hide resolved

tohojo reviewed Jun 10, 2024

View reviewed changes

applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js Outdated Show resolved Hide resolved

systemcrash marked this pull request as draft June 24, 2024 12:21

stokito added 4 commits July 8, 2025 21:30

luci-app-acme: Add dns_wait option

9ea217d

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: introduction: add link to OpenWrt Wiki

6686006

The article is "Get a free HTTPS certificate from LetsEncrypt for OpenWrt with ACME.sh" Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: option acme_server: add "See more" link

baed0d4

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: option staging: show the flag only for letsencrypt

5a059ed

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

stokito added 8 commits July 8, 2025 21:31

luci-app-acme: option days: default to 60 as in the acme.sh

9f2727a

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: put validation_method above domains

2e26ae2

A user must specify the validation_method first. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: Set default validation_method to standalone

19fe099

In the 585df1d I changed the validation_method to webroot by mistake because I thought this is a default in the acme.sh. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: show button "Install package acme-acmesh-dnsapi" if DN…

bf23011

…S mode is used Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: Guess the system domain and pre-fill it to domains.

6300b91

Check if the hostname is FQDN (e.g. has least one dot). Check if the domain in the browser is not an IP and FQDN. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: Import domains from DDNS

5e96229

Many users already have a DDNS configured e.g. DuckDNS.org or Cloudflare. We can import the configurations to simplify configurations and avoid mistakes. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: Add Log reader

ff3f321

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

stokito force-pushed the luci-app-acme branch from 7b3206d to ff3f321 Compare July 8, 2025 21:00

stokito marked this pull request as ready for review July 8, 2025 21:01

tohojo requested changes Aug 12, 2025

View reviewed changes

luci-app-acme: improve UI for inexperienced users #7147

Are you sure you want to change the base?

luci-app-acme: improve UI for inexperienced users #7147

Uh oh!

Conversation

stokito commented Jun 1, 2024

Uh oh!

tohojo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stokito commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stokito commented Jun 7, 2024 via email

Uh oh!

Uh oh!

tohojo commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stokito commented Jun 10, 2024 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stokito commented Jun 10, 2024 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stokito commented Jun 10, 2024 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tohojo commented Jun 10, 2024 via email

Uh oh!

tohojo commented Jun 10, 2024 via email

Uh oh!

systemcrash commented Jun 10, 2024 via email

Uh oh!

systemcrash commented Jul 22, 2024

Uh oh!

systemcrash commented Nov 23, 2024

Uh oh!

stokito commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stokito commented Jun 10, 2024 via email •

edited

Loading

stokito commented Jun 10, 2024 via email •

edited

Loading

stokito commented Jun 10, 2024 via email •

edited

Loading