Dietpi banner upgrades

[timjolson]

Enhancing dietpi-banner.

• Changed custom entry to accept multiple commands
• Added network usage by namespace
• Added disk usage by pattern matching
• Added systemd and fail2ban status
• Standardized printing
• Organized color resources
• Added G_TRUNCATE_MID

[timjolson]

Outstanding items I know of:

1. no IPv6 detection for the network traffic, only ipv4
2. dynamic disk space usage is independent of the pre-existing disk space entries (what do we do with the existing options?)
3. wrapping has not been tested or dealt with
4. IP address and network information (retrieval and parsing) use different methods than are pre-existing in the script - need to work through what might be more robust / efficient and consolidate to helper functions
5. no handling of permissions checking is in place yet
6. there sometimes is an issue with the terminal not being properly cleared when running dietpi-banner 1, probably applies to dietpi-banner 0, as well. The issue occurs often when using VSCode and has not been verified in other terminals.

[timjolson]

I'm not sure if things that (potentially) need root need to be run this way. There are other instances at line 591, 358, and indirectly on lines 530-565. Does G_CHECK_ROOT_USER handle this? Is there a better way to detect root access and manage non-root errors?

[MichaIng]

Yes, sudo -n is the best I could find if the script is called as non-root user. One needs to take care that sudo is returning the error code, rather than the called command, else the interpretation of the error can be wrong. Print_Cert_Status previously used does an echo as last command, hence any error was assured to be a failing sudo. Get_Fail2Ban_Status_Raw however returns an error as well if iptables and nft are both not installed (expected default on DietPi which uses a blackhole route by default as blocking action. I guess it should not return an error code, but just an empty string, which can then be interpreted as "no status", while any error is taken as "sudo failed"?

[timjolson]

I re-worked to use fail2ban-client banned and cleaned this up quite a bit.

[MichaIng]

A great extension! But there is some cleanup/polishing needed to reduce the overhead (in code an UI), and make it work for expected use cases.

[MichaIng]

Here are (almost?) all 3+4 bit ANSI color codes stored in the array + duplicate indexed entries, which are then stored back to the named entries, which means the named indexes can be wrong, if users store different colors in the config file. And actually, the script makes still only use of the same 4 + 2 (green, yellow) colors, rendering all the named indexes redundant.

I do understand that this was intended to be used by users in their custom commands? But if someone really needs to make use of like 50 colors, then them can also lookup the raw 8 bit + 24 bit color codes, and have the choice between all 256 + everything the terminal supports: https://en.wikipedia.org/wiki/ANSI_escape_code#8-bit

Defining this large array here looks like there are two contradicting ideas mixed:

1. The original idea was to have the colors as indexed array, to not associate them with particular colors, so it does not cause confusing if users change them. Changing the colors has however never been added to the GUI yet.
2. Using an associative array with (color-)named indexes, so the script is easier to read. This however conflicts with the first idea, as now aCOLOR[RED] can be green.

To solve this cleanly, I suggest the following: Use an associative array, but not with color names, instead using names which indicate their use in the script. What comes to my mind in 1 a.m. (just an example to clarify the general idea 😉):

• aCOLOR[ACCENT] for the green lines and bullet points
• aCOLOR[STRONG] for the bold white
• aCOLOR[WEAK] for the dark grey
• aCOLOR[ALERT] for the bright red currently used for all highlighted notifications, while this PR changes it to be used almost only for errors and immediate required action
• aCOLOR[HIGHLIGHT] for the yellow color introduced here for notifications which were previously bright red
• aCOLOR[ALT] for the blue introduced here as ... not really a specific meaning/purpose but just as alternative color to give some contrast in certain outputs? I guess this could be replaced with the accent or highlight color as well, to not have too many different colors in the output overall. But I did not actually review the banner with all its features yet.
• Remove every other index/value that is not explicitly used in this script.

Due to the dropped /boot/dietpi/.dietpi-banner_custom, we need to do a migration in dietpi-update patches anyway, hence we can migrate this color array as well.

[timjolson]

I was also thinking of these issues, and even started on a script for users to configure their own colors and formats, which would be useful for devices with abnormal/monochrome/etc displays.

A simple change that gets us to the better indexing scheme is renaming the indexes here and then using those new index names throughout. Then the saving/loading should be adjusted for the use-case names rather than index 0...6

On a larger scale, I had started to separate out all the color/formatting to a utility script that the end-user could also take advantage of, especially for the custom commands. This is why I started down the path of including formats and such.

Does it make sense to try to keep .dietpi-banner_custom?

[MichaIng]

useful for devices with abnormal/monochrome/etc displays.

Right, cases where the darker gray is hardly readable (too dark) are not even so uncommon, which was the reason I turned those colors into config file variables some time ago, but never found the time to finish this theming topic properly: #2651
There is another one for whiptail dialog theming: #7516

separate out all the color/formatting to a utility script

But do you really think this is such a benefit, to use aCOLOR[SOME_COLOR_NAME] rather than looking up from the full set of possible color codes e.g. on the Wikipedia page I linked? What I thought about was even using moreless the scripts on the Wikipedia page to print all possible 3/4/8 bit color codes, as text + applied color to the console, to users can see live how it looks on their terminal, and can copy and paste the code to apply. But a huge array seems unnecessary to me, and cannot really contain all colors anyway, with meaningful array key names. Hence my suggestion to focus on the colors used in this script. We can add a link to the config file to support looking up color codes for now, and add a menu to change them in TUI later.

Does it make sense to try to keep .dietpi-banner_custom?

IMO it is not needed, as it is superseded by the support for multiple commands added here. As said, we can migrate existing custom scripts, practically by just copying the call of the existing custom script into the new custom command array. That would be also the new way if uses need to run a more complex script that does not fit into a single inputbox line.

[timjolson]

I re-worked the colors for semantic indexing and updated save/load. patching on updates is beyond me.

[MichaIng]

2. dynamic disk space usage is independent of the pre-existing disk space entries (what do we do with the existing options?)

I suggest we merge "Disk usage (RootFS)" and "Disk usage (userdata)" into the new single option. Otherwise it is confusing to have 3 different options all showing some disk usage, and hence also 3 code sections to keep in sync. Maybe / and /mnt/dietpi_userdata (if a mount point) can be dedicated fixed quick-select options in a parent menu. Or, maybe simpler, all findmnt -no SOURCE --real mount points could be part of that menu. So when adding a disk, there is a menu showing all mount points to select + custom, and custom shows the inputbox? The fixed mount points can still be treated like a glob when printing the banner, of course.

[timjolson]

So a dynamic checkbox array that has findmnt -no SOURCE --real options listed, plus a checkbox to then enter custom matching? And the first array item is always /mnt/dietpi_userdata.

[MichaIng]

Right, that would be my suggestion. Little correction: findmnt -nro TARGET --real, to list them by mount point rather than block device, and without the tree view.

The hardcoded /mnt/dietpi_userdata wouldn't even be needed then, as its true mount point would show up in the list already, if it is a dedicated mount. And / shows up at the top, which is good as well.

[MichaIng]

The DietPi default block action is actually a blackhole route, neither iptables nor nft. Hence this would currently not work or show false info, if iptables is installed for other reasons. Not sure how to solve this properly, parsing the Fail2Ban jail/actions config is difficult, due to the unlimited different ways about how/where this can be set/overridden.

... actually fail2ban-client banned is the right command to use here, as it shows the number of blocks regardless which method was used for each of them. Not trivial to parse the number of banned IPs from that output, but we can figure something out using IPv4 + IPv6 match pattern wrapped into single quotes.

[timjolson]

I updated to use fail2ban-client banned, which also cleaned up the sudo complications, since it is only needed for a single command in this context.

[MichaIng]

Yes, sudo -n is the best I could find if the script is called as non-root user. One needs to take care that sudo is returning the error code, rather than the called command, else the interpretation of the error can be wrong. Print_Cert_Status previously used does an echo as last command, hence any error was assured to be a failing sudo. Get_Fail2Ban_Status_Raw however returns an error as well if iptables and nft are both not installed (expected default on DietPi which uses a blackhole route by default as blocking action. I guess it should not return an error code, but just an empty string, which can then be interpreted as "no status", while any error is taken as "sudo failed"?

[MichaIng]

ip should generally not require root permissions. EDIT: Or does the namespace change something about this?

[timjolson]

I think the specifying of a netns required root, that's where the prefix stuff comes from. It may need more testing to see what is needed.

[MichaIng]

Okay, if that is the case, let's use sudo only if a namespace was actually selected, i.e. apply it as part of ns_cmd_prefix for non-root calls of the script.

[timjolson]

I combined the sudo and ns prefixes, since they were only ever used together anyway.

[MichaIng]

This API should be world-readably by default. Else, instead of using the (expensive) sudo call by default, do so if the file is really not readable. But I think it always is, unless in containers, where however root does not help either.

EDIT: Same as in the other cases, maybe the namespace changes something. If so, sudo should only be used if an actual namespace is used, to avoid the overhead otherwise.

[timjolson]

So

if file is readable
     read rx_bytes
else
     read rx_bytes with sudo

[MichaIng]

Right. Since the namespace selection happens in two places, maybe it makes sense to move generating the ns_cmd_prefix array into a dedicated function, which leaves it empty if run as root or no namespace selected, else adds sudo -n ip netns exec to the array? Or the call via sudo -n, applying the STDERR redirect for the raw command and error capture for sudo itself is done in a dedicated function, which returns the command output directly. That sudo wrapper function could then be conditionally called only if sudo is needed, i.e. not root and namespace. Not sure which way makes it most compact, but the latter would basically keep the previous way with Print_Cert_Status being called conditionally with or without sudo wrapper.

[MichaIng]

G_GET_WAN_IP does a simple curl call, hence no sudo needed. Of does the namespace change something about that?

EDIT: If the namespace requires root permissions, it should be better handled in G_GET_WAN_IP itself, rather than here, and only if an actual namespace is passed.

[timjolson]

I believe any of the netns stuff requires root access.

[MichaIng]

Okay in this particular case, a G_CHECK_ROOT_USER "$@" || return 0 call in the dietpi-globals G_GET_WAN_IP function would probably make most sense (if a namespace was passed). It re-exec itself with sudo -n if needed. That way, this works as well outside of dietpi-banner. ... thought it currently prints an additional info line about that, which is not wanted in the banner. So we'd need to add a -q/--quiet flag to suppress this or so.

Hmm, actually it might still add output the wrong way. Probably easier to keep the sudo wrapper in dietpi-banner for now, to have full and consistent control of how STDERR and STDOUT are handled for showing them nicely inline in the banner.

For script-internal (non-dietpi-globals) functions G_CHECK_ROOT_USER is not a good option either, since it works only within functions name G_*, it always uses bash as shell (adding unnecessary overhead/slowdown compared to dash bourne shell). It really is meant/designed for interactive G_* function calls from login console, while dietpi-* script which require root re-exec themselves with sudo as a whole.

[timjolson]

I made a dedicated BANNER_GET_WAN_IP so the handling is separate from the global version. It needs more testing and eyes on it, though. IPv6 is not yet included, and the sudo may still have problems.

[MichaIng]

Is there a particular reason for this restructure? The downside is that the actual error messages reading the cert or from openssl are now masked, while they were previously transparently shown in the banner, replacing the cert status text. The separation of the sudo call block into an own function, and _Raw sub function is fine, but the exit code handling adds quite some more complexity, without a benefit that I can see.

The sudo wrapper could be actually a dedicated function, if needed multiple times, taking (now) _Raw function as input, returning either the NOPASSWD sudo required error, or whichever output/error the raw function echos?

[timjolson]

I don't remember any particular reason I rewrote this. I like the idea of a sudo-wrapper, but am not sure I could put one together. How would it be different than G_SUDO?

[MichaIng]

G_SUDO is meant for G_* functions, and hence G_CHECK_ROOT_USER "$@" works only for whole scripts or G_* functions. It is not designed for/suitable for script-internal functions, and it is especially not designed for this particular case where we want the wrapped function's STDOUT + STDERR + sudo error all inline in the banner output line. And we want to use the lightweight dash where possible, while G_SUDO always calls bash as subshell, to be able to load dietpi-globals (which is another major overhead).

Thinking about the overhead of loading dietpi-globals again within a subshell: Since a delay in the banner can be quite annoying, maybe it is better to not use G_GET_WAN_IP for calls with namespace at all. The function is small, especially since it wouldn't require options handling in the banner, so we can inline the sudo -n ip netns exec curl call in this case. This also avoids the implicit change you are currently doing, where G_GET_WAN_IP merges STDERR into STDOUT, which is currently not the case. This is something which should happen in the banner only, nowhere else, as the banner is a special case where inlineing error messages has a visual benefit compared to throwing errors as dedicated alien lines without indentation, messing with the banner format.