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

The dns_wait is supported by the acme but wasn't yet configurable on UI. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Use the %s placeholder instead of URLs. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

The introduction is confusing because the acme.sh use by default ZeroSSL. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

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

By default it's ZeroSSL. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Also change days placeholder to 60 which is a default for acme.sh 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>

A user must specify the validation_method first. 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. 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>

We must simplify for a user to understand what happened. 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>

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

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

Also use L.resolveDefault() to not fail if the ACL is not present. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

Make the wildcard allowed only the beginning. Add lowercase requirement. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

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

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

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

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

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

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);

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

dannil · 2024-06-07T20:32:21Z

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

+			let ddnsDomainsList = [];
+			for (let ddnsDomain of ddnsDomains) {
+				ddnsDomainsList.push(ddnsDomain.domains[0]);
+			}


If you want to clean it up, I'm fine with either tho.

let ddnsDomainsList = ddnsDomains.map(d => d.domains[0]);

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...

tohojo · 2024-06-10T09:51:32Z

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

@@ -665,6 +657,45 @@ function _collectDdnsDomains() {
 	return ddnsDomains;
 }

+function _importDdns(ddnsDomains) {
+	alert(_('After import check the added domain certificate configurations.'));


alert() is not a good way to send messages to the uses. Pretty sure luci has some way of adding messages, please use that instead.

Please remove this alert()

tohojo · 2024-06-10T09:52:13Z

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

+			for (let d of ddnsDomain.domains) {
+				let dupDomainSection = certSectionDomains.get(d);
+				if (dupDomainSection) {
+					alert(_('The domain %s in DDNS %s already was configured in %s. Please check it after the import.').format(d, sectionId, dupDomainSection));


If it's already configured we should just skip it instead of bugging the user...

Please remove this alert()

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

Normally the message should never occur. It would be better to let a user to know for any duplucate. Or we can just override and a user will check the Changes to apply to see what changed 10 июн. 2024 г. 12:52:35 Toke Høiland-Jørgensen ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js[#7147 (comment)]: > + } + } + } + console.log(certSections); + console.log(certSectionDomains); + for (let ddnsDomain of ddnsDomains) { + let sectionId = ddnsDomain.sectionId; + // ensure unique sectionId + if (certSectionNames.has(sectionId)) { + sectionId += '_' + new Date().getTime(); + } + if (ddnsDomain.domains) { + for (let d of ddnsDomain.domains) { + let dupDomainSection = certSectionDomains.get(d); + if (dupDomainSection) { + alert(_('The domain %s in DDNS %s already was configured in %s. Please check it after the import.').format(d, sectionId, dupDomainSection)); If it's already configured we should just skip it instead of bugging the user... — Reply to this email directly, view it on GitHub[#7147 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AADFODUDNZGP6H2A67FIIVTZGVZOFAVCNFSM6AAAAABIUMU2QOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBXGMZDSMBRGI]. You are receiving this because you authored the thread. [Отслеживающее изображение][https://github.com/notifications/beacon/AADFODTJ2OJU4DRCOBO65VDZGVZOFA5CNFSM6AAAAABIUMU2QOWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT5TNE7I.gif]

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. 10 июн. 2024 г. 12:50:48 Toke Høiland-Jørgensen ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js[#7147 (comment)]: > @@ -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... — Reply to this email directly, view it on GitHub[#7147 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AADFODVK34LZBZEHGG3JWADZGVZHRAVCNFSM6AAAAABIUMU2QOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBXGMZDEMRXGM]. You are receiving this because you authored the thread. [Отслеживающее изображение][https://github.com/notifications/beacon/AADFODV36ZNNE3NNETB4BXTZGVZHRA5CNFSM6AAAAABIUMU2QOWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT5TMX2C.gif]

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

Yes, the alert() is anoying, still it's much easier to implement it. Ideally a user should never see it 10 июн. 2024 г. 12:51:54 Toke Høiland-Jørgensen ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In applications/luci-app-acme/htdocs/luci-static/resources/view/acme/acme.js[#7147 (comment)]: > @@ -665,6 +657,45 @@ function _collectDdnsDomains() { return ddnsDomains; } +function _importDdns(ddnsDomains) { + alert(_('After import check the added domain certificate configurations.')); *alert()* is not a good way to send messages to the uses. Pretty sure luci has some way of adding messages, please use that instead. — Reply to this email directly, view it on GitHub[#7147 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AADFODQD22UNPSCPBG4JEKLZGVZLTAVCNFSM6AAAAABIUMU2QOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBXGMZDMNBWGU]. You are receiving this because you authored the thread. [Отслеживающее изображение][https://github.com/notifications/beacon/AADFODQON3AGDYWZM73JAE3ZGVZLTA5CNFSM6AAAAABIUMU2QOWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT5TNAAC.gif]

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.

stokito added 12 commits June 1, 2024 15:39

luci-app-acme: dns_wait option

4007313

The dns_wait is supported by the acme but wasn't yet configurable on UI. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: Move link out of translations

28ec2b4

Use the %s placeholder instead of URLs. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: introduction: remove LetsEncrypt mention

96495b6

The introduction is confusing because the acme.sh use by default ZeroSSL. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: introduction: clarify that ZeroSSL is by default

b0d2426

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

luci-app-acme: acme_server: remove Let's Encrypt from title

abc07d5

By default it's ZeroSSL. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

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

6249c07

Also change days placeholder to 60 which is a default for acme.sh Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

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

7706c6d

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: put validation_method above domains

76ca13f

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

luci-app-acme: Validate domains

99aea87

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. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

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

30f8b1b

…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.

1e8a228

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

8e86389

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>

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 added 2 commits June 2, 2024 18:23

luci-app-acme: Add Log reader

8ed4768

We must simplify for a user to understand what happened. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: Set default validation_method to standalone

30e8b84

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>

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 added 4 commits June 3, 2024 13:19

luci-app-acme: LetsEncrypt is default

86458bb

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

luci-app-acme: code style: Use <br />

94f551e

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

luci-app-acme: ACL for /proc/sys/kernel/hostname

dc3c47d

Also use L.resolveDefault() to not fail if the ACL is not present. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

luci-app-acme: domains validate

ef59e3c

Make the wildcard allowed only the beginning. Add lowercase requirement. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>

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

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

35f4d1e

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

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 added 4 commits June 4, 2024 16:43

luci-app-acme: fix typo

c11d803

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

luci-app-acme: fix _isFqdn() to not allow raw IPv4

3ff1564

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

luci-app-acme: remove the ZeroSSL mention

6b4530d

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

luci-app-acme: DDNS import: check for duplicates

c5a7289

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

luci-app-acme: DDNS import: also create a wildcard domain

faefaf1

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

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

luci-app-acme: wildcards * require Validation method: DNS

7b3206d

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

dannil reviewed Jun 7, 2024

View reviewed changes

tohojo reviewed Jun 10, 2024

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stokito commented Jun 1, 2024

tohojo left a comment

stokito commented Jun 3, 2024

stokito commented Jun 7, 2024 via email

dannil Jun 7, 2024

tohojo commented Jun 10, 2024

tohojo Jun 10, 2024

tohojo Jun 10, 2024

systemcrash Jun 30, 2024

tohojo Jun 10, 2024

systemcrash Jun 30, 2024

stokito commented Jun 10, 2024 via email

stokito commented Jun 10, 2024 via email

stokito commented Jun 10, 2024 via email

tohojo commented Jun 10, 2024 via email

tohojo commented Jun 10, 2024 via email

systemcrash commented Jun 10, 2024 via email

systemcrash commented Jul 22, 2024

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

Conversation

stokito commented Jun 1, 2024

tohojo left a comment

Choose a reason for hiding this comment

stokito commented Jun 3, 2024

stokito commented Jun 7, 2024 via email

dannil Jun 7, 2024

Choose a reason for hiding this comment

tohojo commented Jun 10, 2024

tohojo Jun 10, 2024

Choose a reason for hiding this comment

tohojo Jun 10, 2024

Choose a reason for hiding this comment

systemcrash Jun 30, 2024

Choose a reason for hiding this comment

tohojo Jun 10, 2024

Choose a reason for hiding this comment

systemcrash Jun 30, 2024

Choose a reason for hiding this comment

stokito commented Jun 10, 2024 via email

stokito commented Jun 10, 2024 via email

stokito commented Jun 10, 2024 via email

tohojo commented Jun 10, 2024 via email

tohojo commented Jun 10, 2024 via email

systemcrash commented Jun 10, 2024 via email

systemcrash commented Jul 22, 2024