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 <[email protected]>

Use the %s placeholder instead of URLs. Signed-off-by: Sergey Ponomarev <[email protected]>

The introduction is confusing because the acme.sh use by default ZeroSSL. Signed-off-by: Sergey Ponomarev <[email protected]>

Signed-off-by: Sergey Ponomarev <[email protected]>

By default it's ZeroSSL. Signed-off-by: Sergey Ponomarev <[email protected]>

Also change days placeholder to 60 which is a default for acme.sh Signed-off-by: Sergey Ponomarev <[email protected]>

The article is "Get a free HTTPS certificate from LetsEncrypt for OpenWrt with ACME.sh" Signed-off-by: Sergey Ponomarev <[email protected]>

A user must specify the validation_method first. Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

…S mode is used Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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 <[email protected]>

We must simplify for a user to understand what happened. Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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 <[email protected]>

Also use L.resolveDefault() to not fail if the ACL is not present. Signed-off-by: Sergey Ponomarev <[email protected]>

Make the wildcard allowed only the beginning. Add lowercase requirement. Signed-off-by: Sergey Ponomarev <[email protected]>

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

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

Signed-off-by: Sergey Ponomarev <[email protected]>

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

Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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 <[email protected]>

luci-app-acme: Move link out of translations

28ec2b4

Use the %s placeholder instead of URLs. Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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

b0d2426

Signed-off-by: Sergey Ponomarev <[email protected]>

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

abc07d5

By default it's ZeroSSL. Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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 <[email protected]>

luci-app-acme: put validation_method above domains

76ca13f

A user must specify the validation_method first. Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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

30f8b1b

…S mode is used Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

94f551e

Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

luci-app-acme: domains validate

ef59e3c

Make the wildcard allowed only the beginning. Add lowercase requirement. Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

3ff1564

Signed-off-by: Sergey Ponomarev <[email protected]>

luci-app-acme: remove the ZeroSSL mention

6b4530d

Signed-off-by: Sergey Ponomarev <[email protected]>

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

c5a7289

Signed-off-by: Sergey Ponomarev <[email protected]>

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

faefaf1

Signed-off-by: Sergey Ponomarev <[email protected]>

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 <[email protected]>

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