luci-app-nut: add/update SSL configuration for full support - #8926
luci-app-nut: add/update SSL configuration for full support#8926danielfdickinson wants to merge 1 commit into
Conversation
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. Main functional finding: fs.read() is not newline-trimmed, so the ssl_support_type comparisons in both admin views will not match a shell-written /usr/share/nut/ssl_backend and the SSL sections would never render — see the inline comment on nut_monitor_admin.js. Also flagged the tuple() datatypes rejecting the quoted form the help text prescribes, the unchanged luci-app-nut-read uci ACL vs. the commit message's security claim, and the missing upsd-side consumers for several new options. The .po/.pot regeneration looks clean and CI is still pending on this head, so nothing to report there.
Commit checks
- 75d1349 "luci-app-nut: add/update SSL configuration for full support" — the message covers the SSL rework, the
ssl_backendswitch and the ldd removal, but not the newdebug_minoption added tonut_server.jsandnut_monitor.js. It also states the SSL settings are no longer "readable by a non-privileged user", which the ACL change does not actually accomplish (see the inline comment onacl.d/luci-app-nut.json).
Generated by Claude Code
75d1349 to
6c73922
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-push (75d1349 → 6c73922). All of the previous round's findings are addressed: fs.trimmed() replaces the untrimmed fs.read() + unreachable .catch() in both admin views, the tuple() datatypes that rejected the quoted syntax are gone, the certifcte / has as space / as and empty pair typos and the unbalanced NO (... label are fixed, the monitor-side certfile is now labelled "Client", the NSS certpath optional flag is consistent across both admin views, debug_min is dropped so the diff matches the subject, and the commit message no longer claims the ACL restricts read access. The .pot/.po regeneration is consistent with the new sources — no stale Unable to read ssl_backend / Minimum debug level / ldd strings remain. CI is green on this head, so nothing to report there.
Three remaining items inline, all minor: a leftover empty load() override in nut_monitor.js, a question about placeholder vs default for the NSS certpath, and a comment-style nit on the new /* Update certpath ... */ lines.
Generated by Claude Code
6c73922 to
3a6958f
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-push (6c73922 → 3a6958f). The delta is three files / six lines and it addresses all three items from the last round: the empty load() override in nut_monitor.js is gone, the /* Update certpath ... */ changelog-style comments are now // statements of fact matching the file's existing style, and the NSS certpath placeholder became default on both admin views. The commit message matches the diff. CI is green on this head (build x86_64, eslint, CodeQL, FormalityCheck all passing), so nothing to report there.
One item inline, and it is a correction of my own previous advice: o.default combined with o.optional = true does not persist the value either — form.js explicitly removes the option when the form value equals the default. So the placeholder → default swap changes only the rendering, not what ends up in UCI.
Generated by Claude Code
3a6958f to
e37d312
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the delta since 3a6958f — the branch was rebased onto 98417a6 and the single commit force-pushed to e37d312. Outside the .po/.pot regeneration the change is seven lines across the two admin views.
The previous round's item is resolved, and resolved correctly: rather than trying to make default/placeholder persist /etc/nut/cert_db, the NSS certpath field is now left genuinely empty with the rationale spelled out in an inline comment ("the default database has no certificates, and must be filled to be useful") and a help string telling the user how to populate it. That sidesteps the parse()-removes-value-equal-to-default behaviour entirely, so there is nothing left to flag there.
Two things I checked specifically because of the rebase:
- The rebase base
98417a6added+USE_GLIBC:lddtoLUCI_DEPENDS, which this PR's own removal of thelddprobe makes dead. The rebase handled it —98417a6..e37d312drops that token from the Makefile, and nolddreference survives anywhere inapplications/luci-app-nut/(Makefile, ACL, or views). - The
.potregeneration is consistent with the sources: the newAn empty database was created…msgid is present with correctnut_monitor_admin.js:52/nut_server_admin.js:50references, the shiftednut_monitor.jsline numbers now match the file after theload()removal from the previous round, and all 44.pofiles carry the same reference updates with no translations misattached.
Commit message is unchanged from the last round and still matches the diff. GitHub reports no check runs on this head yet, so nothing to report from CI.
Two nits inline, both cosmetic and both in strings/comments introduced by this delta.
Generated by Claude Code
e37d312 to
2218fe0
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the delta since e37d312 (force-pushed to 2218fe0d). Outside the .po/.pot regeneration the change is three lines across the two admin views — both items from the last round.
- The comment-parity nit is fixed correctly:
nut_server_admin.js:41now carries the same sentence-terminating period as its twin innut_monitor_admin.js:43, and the two blocks are byte-identical again. - The grammar nit is not fixed correctly — the comma was inserted one word to the left of where it belongs, producing "To use, it SSH in", which is worse than the original. Inline comment with the suggestion; it needs applying in both views.
The .pot/.po regeneration is consistent with the (new) source string: the msgid is rewrapped in the template and in all 43 .po files with the same nut_monitor_admin.js:52 / nut_server_admin.js:50 references, no msgstr was left attached to the old msgid, and no copy of the previous wording survives anywhere under applications/luci-app-nut/.
Commit message is unchanged from the last round and still matches the diff. CI is green on this head (build x86_64, eslint, CodeQL, all three FormalityCheck jobs), so nothing to report there.
Generated by Claude Code
2218fe0 to
7b479dc
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
Re-reviewed the delta since 2218fe0d (force-pushed to 7b479dc). Outside the .po/.pot regeneration the change is one line, duplicated in the two admin views, and it is the grammar nit from the last round — now fixed correctly: "To use, it SSH in" → "To use it, SSH in". Applied identically in nut_monitor_admin.js:52 and [`nut_server_admin.js:50`](https://github.com/openwrt/luci/blob/7b479dcbea9d183fa3a3843ed348f0760809919d/applications/luci-app-nut/htdocs/luci-static/resources/view/nut/nut_server_admin.js#L50),`` and the surrounding NSS certpath blocks in the two views remain byte-identical.
The .pot/.po regeneration is consistent with the new source string: the msgid is rewrapped in the template and in all 42 .po files, every file carries exactly the same three removed / two added msgid lines and no other change (no msgstr line was touched anywhere in the delta), the nut_monitor_admin.js:52 / nut_server_admin.js:50 references are unchanged, and no copy of the old wording survives anywhere under applications/luci-app-nut/.
Commit message is unchanged from the last round and still matches the diff. CI is green on this head (build x86_64, eslint, CodeQL, all three FormalityCheck jobs).
Generated by Claude Code
|
Now that openwrt/packages#29893 has landed, this PR is relevant / ready for review. |
|
Oh and I see there is a conflict to resolve. |
7b479dc to
9b33f4c
Compare
9b33f4c to
30e45b6
Compare
30e45b6 to
b09b978
Compare
|
@BKPepe Found another thing to you may (or may not) want to update in your bot - LUCI_EXTRA_DEPENDS needs to be of the form |
b09b978 to
ae8be46
Compare
Both the configuration for NSS-based SSL and the configuration for OpenSSL-based SSL were incomplete, so add missing options, and enhance existing settings. Additionally, the method of checking for being compiled with SSL changes with openwrt/packages#29893. We now have a file /usr/share/nut/ssl_backend which contains either 'nss', 'openssl', or 'none'. We use this to show the applicable configuration options in the UI, and remove the runtime ldd check on the upsd and upsmon binaries. That same pull request adds server-side CERTPATH and so on handling in https://github.com/danielfdickinson/packages/blob/c0ec3741ce64b8613b8324575f907342303563fc/net/nut/files/nut-server-config.sh.functions#L94-L107 Finally, SSL settings are security-sensitive so move them to the 'admin' NUT screens to reduce visibility, and to prepare for a future commit to the NUT package which will separate the UCI configs for admin and non-privileged users. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
ae8be46 to
8196c15
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
Re-reviewed the delta since 7b479dc (rebased from 98417a6 onto 77dad3f and force-pushed to 8196c15). The commit's file list is byte-for-byte the same as the last round except for two hunks, and the commit message is unchanged.
Makefile — EXTRA_DEPENDS:=nut (>= 2.8.5-5) → LUCI_EXTRA_DEPENDS:=nut (>=2.8.5-r5). Both halves of this check out:
- The variable rename is a real fix, not cosmetic.
EXTRA_DEPENDSis only ever emitted from inside thePackage/$(PKG_NAME)define, vialuci.mk:166— a bareEXTRA_DEPENDS:=set beforeinclude ../../luci.mkis just a global make variable and never reaches the package metadata, so the constraint was inert before this.luci-app-adguardhomeis the only other in-tree user of the working form and it matches (LUCI_EXTRA_DEPENDS:=adguardhome (>=0.107.73-r3));luci-app-radicale3still carries the inertEXTRA_DEPENDSspelling. - The
-r5version format is right.include/package-defaults.mkbuilds the package version asVERSION:=$(PKG_VERSION)-r$(PKG_RELEASE), andnet/nut/Makefilein packages master is nowPKG_VERSION:=2.8.5/PKG_RELEASE:=5, so2.8.5-r5is exactly what the constraint has to compare against. The previous2.8.5-5would not have matched.
Now that openwrt/packages#29893 has landed I also re-ran the backend-consumer check that was open from the first round — every UCI option the two admin views write has a consumer in the merged package: CERTFILE, CERTPATH, CERTIDENT, CERTHOST, CERTVERIFY, FORCESSL are all handled in nut-monitor-config.sh.functions, and CERTFILE, CERTPATH, CERTIDENT, CERTREQUEST, DISABLE_WEAK_SSL in nut-server-config.sh.functions, each gated on the same ssl_backend values (openssl / nss) the views gate their sections on. /usr/share/nut/ssl_backend is installed by the package Makefile and is readable through the luci-app-nut-admin ACL. Nothing frontend-only left.
po/sv/nut.po — the rebase pulled in a Weblate refresh of the Swedish catalogue, so this file now carries both the upstream retranslations and this commit's regeneration. I checked the merge did not damage anything:
- The
.potis exactly consistent with the head sources: all 137 source references resolve to an existing file and an in-range line, and for all 149 entries the referenced line actually contains the msgid. - All 44
.pofiles carry precisely the.pot's 149 msgids — no orphaned entries, no missing ones, in any language. - The only
msgstrvalues dropped in the Swedish file are the ones whose msgids this commit removes or rewords (Sökväg till CA-certifikat,Certifikatfil (SSL),Sökväg med CA-certifikat…,Kräv SSL och kontrollera…,Det gick inte att köra ldd: %s). No live translation was lost and no fuzzy marker was introduced. lddsurvives only as#~-commented obsolete entries in a handful of catalogues, which is normal msgmerge output; nolddreference remains in the Makefile, ACL, or views.
GitHub reports no check runs on this head yet, so nothing to report from CI. mergeable_state is clean, so the conflict you spotted is resolved.
Generated by Claude Code
It was caught by @hnyman in #8904 (comment), it needs to be discussed first, though. |
Thanks for pointing me to that discussion. In this case, though, it's actually about the format of EXTRA_DEPENDS (might apply beyond LuCI); It would be nice if the bot caught a wrong EXTRA_DEPENDS format (I originally had |
Pull request details
Description
Both the configuration for NSS-based SSL and the configuration for OpenSSL-based SSL were incomplete, so add missing options, and enhance existing settings.
Additionally, the method of checking for being compiled with SSL changes with openwrt/packages#29893. We now have a file /usr/share/nut/ssl_backend which contains either 'nss', 'openssl', or 'none'. We use this to show the applicable configuration options in the UI, and remove the runtime ldd check on the upsd and upsmon binaries.
Finally, SSL settings are security-sensitive so move them to the 'admin' NUT screens instead of being readable by a non-privileged user.
Maintainer
@danielfdickinson @systemcrash
Tested on
OpenWrt version: OpenWrt SNAPSHOT r35715-94a21b3fe9
LuCI version: LuCI Master 26.220.05649~98417a6
Web browser(s): Firefox 140.13.0esr (64-bit)
Checklist
Depends on: nut: enable SSL via NSS by default packages#29893