Improvements to scripts under initrd

[Thrilleratplay]

During the course of debugging #872, correcting shellcheck errors and warnings, a number of issues presented that made verifying the changes difficult. Examples of this were errors that existed in the master branch (although these may have been unique to using qemu-coreboot-fwwhiptail), needing to unquote variables used as whiptail parameters then adding shellcheck exceptions, as many scripts do not have .sh extensions there stances where it is difficult to determine if a function or script is being invoked, and not owning a Librem/Nitro Key to be able to test and of those scripts. Given how fragile the existing initrd script is, I believe a different approach is needed.

The original plan was to correct shellcheck errors and warning then progress to cleaning up and tightening the scripts. At this time, it seems that these should be done together; focusing on improving, cleaning up, documenting, normalizing individual functionality to minimize the amount of testing and reduce the potential for introducing bugs.

A few suggestions:

• enable CONFIG_BASH_IS_ASH - Done in feat(busybox): set CONFIG_BASH_IS_ASH #909 and busybox: CONFIG_BASH_IS_ASH is incompatible with CONFIG_BASH_IS_NONE #945
• Give all script files .sh exetension. Be sure to add .sh extension to references in the script.
• Replace sheband with #!/bin/bash
• Use function to generate/normalize y/n menus
• use environmental variables via /etc/profile instead of /tmp/config (is there a reason?)
• Pass shellcheck without any no ignore statements
• No global variables. Either are defined or documented as an environmental variable.
• Functions much return value or return true.
• Remove .ash_history, replace examples in ash_history with alias or bash scripts
• Ensure all user facing bash scripts have usage menus.
• (?) Abstract out logic into functions.sh or other function files. Leaving scripts found in /initrd/bin to be user facing end points to deal with usage menus, basic input validation and proper calling of the appropriate file.
• Add unit testing. I have used shellspec and would recommend it.
• (?) Break each function into individual files. Sourcing would be more complicated but will allow tracking function usage and explicitly define a function's source.
• (?) Include documentation annotations such as [shdoc(]https://github.com/reconquest/shdoc)

Test cases/issues to correct:

• [] If a fully encrypted drive or other drive that cannot be mounted is used in a configured Heads device, it will result in an error loop printing mount: mounting /dev/sda1 on /boot failed: invalid argument infinitely. Reported in Error loop if Heads is configured and hard drive is missing or replaced #911 .

[tlaurion]

(?) Break each function into individual files. Sourcing would be more complicated but will allow tracking function usage and explicitly define a function's source.

@Thrilleratplay I agree with all your planning but precedent point!

Inversely, I believe Heads is currently filled with functions duplicates, that since common, should all be under /etc/functions.sh where . /etc/functions.sh should normally be included in all shell scripts.

[tlaurion]

use environmental variables via /etc/profile instead of /tmp/config (is there a reason?)
@Thrilleratplay
look into combine_configs which is used to aplly /etc/config.user injected in rom if user wants to customize board filled /etc/config at compile time throughOptions->Change Configuration settings in gui-init which calls config-gui.sh and permits to save config.user in rom.

So in short, init merges configs and then exports environement variables by . /tmp/config consequently of that added functionality. Changing this would require rethinking of the config.user overlay, which I kinda like and would be useful for #555 and recently proposed #890 to facilitate tinkering

cbfs files are measured under PCR7 prior of the merging by done by merge_configs later on.

[Thrilleratplay]

@tlaurion I think my main issue has been trying to determine from the source where this was coming from. Within the past few days realized it is concatenated using the build config and a few other items. Its name suggested it was used to change and update parameters in various scripts. It is was called /etc/heads_config.sh or /etc/heads_profile.sh, it would have clearer how it was used and that it is immutable. There are other environmental variables would be useful in other places and through to place them in one location. As these are immutable, it may be better to leave them in a file.

Seeing that making the shellcheck related updates look like they will cause a number of possible issues despite being supposedly benign, I am shelving the PR to focus on IFD/GBE blob generation. This was to document the things I wanted to tackle when returning the intrd scripts but also put them out there for others to critique or add onto. These are vague because there are a lot of ideas I am seeing looking through the scripts, but the devil is always in the details on how it will come together. It is difficult to point to one clear example, I'll try explaining mounting boot/.

I noticed this issue when looking into #447, kexec-select-boot takes a parameter of the boot directory. This is where environmental variables would be useful. What if there was an environmental variable $BOOT_DIR, if set, it would have path to the boot directory, it is was not set, boot is not mounted. If all of the logic is moved to one file /etc/functions.sh, the file would be difficult to navigate and isolate issues. So, how about /etc/heads_functions/mount_boot.sh (it probably should be under user/shared/lib/ or /bin/ but that is trivial detail). Any function that needs to access $BOOT_DIR would import /etc/heads_functions/mount_boot.sh which contains the functions mount_boot_dir_rw and is_boot_mounted.

# mock code

is_boot_mounted() {
 return [-z $BOOT_DIR ]
}


mount_boot_dir_rw() {
    if is_boot_mounted; then
      return $BOOT_DIR;
    elif mount -o rw /boot; then
       BOOT_DIR= "/boot"
      
       #if boot is not a separate partition, set the boot directory to prevent scanning of entire partition
       if [ -d "/boot/boot/" ]; then
         BOOT_DIR= "/boot/boot"
        fi      
       export BOOT_DIR

       return $BOOT_DIR
   fi
   
  unset BOOT_DIR
  #SOMETHING WENT WRONG!!! ABANDON SHIP!
   return "SOME ERROR"
}

Instead of passing in /boot to individual functions, the function that requires this information could include source /etc/heads_functions/mount_boot.sh and have a few lines to mount and verify:

some_function(() {
  error=mount_boot_dir_rw
  if is_boot_mounted; then
    do_something $BOOT_DIR;
  else
    echo $error
  fi
}

Obviously better error handling can be implemented but the idea is that how the boot directory can be changed (different partitions, user parameters, different drives, etc) without modifying any of the dependent functions it if it adheres to the just setting an known and documented environmental variable.

Breaking down other functionality like HOTP, where the function would contain seal, unseal, check, etc would be more complicated as they would share many TOPT functionality. This break down should also make removing dead code easier, unit testing easier and theoretically cut down on nuance bugs where someone didn't listen to Egon and decided to cross the streams.

[danielp96]

Any news on this?
I was trying to improve navigation between menus but ran into the problem of things being mixed between functions and scripts (and also mixing user interface and functionality).

[tlaurion]

@danielp96 A lot of side improvements have happened, but I think I know what you mean.
mount-usb, mount_usb? There are still a bit of duplicates under gui-init and /etc/functions, and it would definitely be nice that all scripts under heads have proper sh filetypes.

The main issue here, and linked @Thrilleratplay (sorry if I misinterpret), is that current version of busybox offers poor compatibility with bash. It improved under latest version, which I experimented a bit trying to modify wyng-extract.sh script to realize even with latest version, CONFIG_BASH_IS_ASH is misleading. So one trying to move things forward here are better testing code as if it was DASH then BASH, which shellcheck cannot know. But knowing that busybox is more compliant with DASH then BASH explains why @Thrilleratplay had some issues with 7740f53

I have to admit I didn't revisited this issue for a while, outside of the recent work that was done to move more into /etc/functions and reduce deduplication of functions, but that work is not over yet.

Basically, prior of going forward in this path, it would be advisable to update busybox to latest version and restart where 7740f53 was left, but trying to have scripts more posix compliant then bash compliant. Otherwise, things will break.

We are getting a bit closer with #1188. So I guess it would make sense to test this PR first @danielp96 and then move formward into testing local changes with QEMU instead of bricking machine (and external flashing to get a machine working).

[Thrilleratplay]

@danielp96, that is a good summary and like @tlaurion, I have not taken a look a look at this in a while. I was distracted by reproducible builds for Heads, other projects and life in general (sorry @tlaurion, I didn't mean to just disappear and do plan on continuing the reproducible build environment when I can devote the necessary time to it).

When I last took a look, the various scripts were written in different styles, function inputs were not always obvious and passed in different formats, and there were redundancies that could have been streamlined. What I had envisioned was a core script framework that could be extended, minified and unit tested. I bit off more than I could chew where just linting with shellcheck caused potential problems if merged. I was thinking about how to approach this in a safer manner, but then was distracted.

I think a script framework for Heads would make development easier with fewer bugs but the it will be a balancing act of how/where to restructure while not breaking anything existing or adding too much redundancy while migrating.

[danielp96]

Together with @JonathonHall-Purism and Tray we are willing to work on this.

So as starting would be updating busybox and making sure everything is compatible with it, if I understood correctly?

Then I was thinking about adding the .sh extension to all the scripts (to avoid the confusion of script vs function) and separating the menu navigation and functions into separate scripts, that way the menus are more flexible and the functions are free to be sorced whenever they're needed without risk of running something else.

After that, solving the other tasks on the list.

Any thoughts/corrections/recommendations?

[Thrilleratplay]

• The CONFIG_ASH_IS_BASH is simply a symlink and offers no additional compatibility. If I remember correctly, it was only for providing a way to run shellcheck. For sanity sake, it may make sense to remove it.
• Adding .sh to shell scripts was challenging because it was not obvious due to how some were called.

If following a similar path I had originally laid out, I am not sure what to recommend. There are a lot of script quirks that exist and it is difficult to know what is using used by different users. As I kept running into "I don't know who is using this and if I change it will it break the platform for them", a better first step may be:

1. Audit and catalog all existing features and functions. Contact different key stake holders to plan refactoring, removing code rot and testing.

[tlaurion]

Well, the last iteration on better splitting things up if I recall well happened under 1f27dea

So as starting would be updating busybox and making sure everything is compatible with it, if I understood correctly?

I will try to be clearer from last personal experiments:

1. busybox provides ASH, and tries to approximate BASH, but is not feature complete. Consequently, it is advised to code with ASH in mind more then bash, and have all scripts use the ASH interpreter then bash and avoid bashisms https://github.com/tasket/wyng-backup/pull/104/files#diff-e874df9a730390d6d1f5991b7249cdeac695104b2f152d6319c5bc3e334de965R1.

• As an example <<< is not supported under ASH.
• Lowercasing is not supported under ash
• etc

2. The general advice is to abuse sed/cut/awk and focus on portability vs speed and try to avoid bashisms.

---

• There are still duplicates which cause confusion: mount-boot(script), mount_boot(function), mount-usb, mount_usb
• gui-init is not importing /etc/gui_functions for itself and duplicates functions it uses. (usb-scan, config-gui, flash-gui, gpg-gui and luks_functions are importing and using /etc/gui_functions)

The challenge here is to go in as many little steps needed

• rename all scripts to have .sh extension, make all scripts import *functions.sh is a good first step
• change interpreter from sh(bash) to ash so shellcheck reports for dash, not bash
  • think adding "CONFIG_ASH_IS_BASH" under busybox config was an error and the proper way would be to have proper ASH interpreter declared in scripts. We might want to revert that config change in the same commit
• lint code so that all scripts have same indentation
• Do code rewrite from there. I think @Thrilleratplay advice on checking up with stake holders is a good advice, but I'm not really sure of what is actually unused as of right now. /bin/wget-measure? uefi-init(linuxboot)?
• .ash_history: I agree this should probably be empty as of now. Most of the things there (seal-totp from console would seal a bad value if not using generic-init (who uses generic-init when gui-init can output on bmc if not using fbwhiptail?) gpg functions are now properly being dealt with by gui-nit....

As for all other recommendations from OP, I agree with all of them.

Comments:

Adding .sh to shell scripts was challenging because it was not obvious due to how some were called.
@Thrilleratplay have more details here outside of mount-boot mount_boot duplicates?

[tlaurion]

Sorry I didn't pick up on the ASH (posix) approach (#862) vs BASH compatibility problems (#872) where #862 was the desired approach.

(Banged my head like I said previously on bash-isms that people will think supported by having scripts declaring bash vs ash, and where ash is now supported from shellcheck. Having posix compliant scripts is the case under Heads right now and should be the way to go.)

Closed #872 and reopened #862 while updating the later with #862 (comment)