Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please refine the various shell scripts for clarity, robustness, and portability #202

Open
cbiedl opened this issue May 24, 2020 · 1 comment

Comments

@cbiedl
Copy link

cbiedl commented May 24, 2020

Looking into various scripts, I found a few things I'd like to improve:

These scripts are written for bash. Since I want to include clevis in initrds with size constraints, this creates a problem.

The jose invocations are hard to understand since they use the short form.

There are several different coding styles around, for example sometimes using jose -j"$VAR" and sometimes jose -j- <<< "$VAR". Likewise for loading the output of external programs. And some constructions look sloppy. They might be safe but it eases code review if you use additional safeguards.

Suggestions:

  • In jose invocations, always use the long parameter form. Only exception: -UUU but see jose fmt: Please implement shotcuts for operations on deep hashes. jose#80 for a suggestion about this.

  • Have a little style guide and re-write the scripts accordingly. For me, the rules are:

  • (At least) scripts running in the early userland must be executable by busybox' ash

  • Run with "set -e" and "set -u". The latter gives a bad time initially but since the scripts are run in a sensitive situation, it's worth it.

  • Use $(...) instead of backticks.

  • Always put quotes around expansion unless you really know what you're doing.

  • In strings, use double quotes only when expansion is necessary, single quotes else.

  • And over all: Be consistent in how you do things.

This is some work, I know. I created two prototypes, see the next issues, so you can get am impression how the results will look. I can to do this for the existing scripts as well. But only if you signalize you're fine with the idea and are willing include the result.

To be honest, I failed to replace the read -d . idiom used in various unlockers. I was wondering however whether that feature - reading and printing until the first dot in the input stream - could perhaps be implemented as a new jose subcommand, something like jose jwe hdr. This is latchset/jose#81.

sergio-correia added a commit to sergio-correia/clevis that referenced this issue May 26, 2020
As suggested in latchset#202, let's try to define some sort of style guide
to try to improve portability and consistency across the scripts.

The suggestions include:

- getting rid of bashisms, at least for scripts running in early
  userland
- running with "set -e" and "set -u"
- using $() instead of backticks
- using long form of `jose' commands, for better readability

This commit converts clevis-luks-common-function and its user
clevis-luks-list.

As of now, shellcheck complains of the following:
"SC2039: In POSIX sh, 'local' is undefined."

This can be muted with `shellcheck -eSC2039'
sergio-correia added a commit to sergio-correia/clevis that referenced this issue May 26, 2020
As suggested in latchset#202, let's try to define some sort of style guide
to try to improve portability and consistency across the scripts.

The suggestions include:

- getting rid of bashisms, at least for scripts running in early
  userland
- running with "set -e" and "set -u"
- using $() instead of backticks
- using long form of `jose' commands, for better readability

This commit converts clevis-luks-common-function and its user
clevis-luks-list.

As of now, shellcheck complains of the following:
"SC2039: In POSIX sh, 'local' is undefined."

This can be muted with `shellcheck -eSC2039'
@sergio-correia
Copy link
Collaborator

Looking into various scripts, I found a few things I'd like to improve:

These scripts are written for bash. Since I want to include clevis in initrds with size constraints, this creates a problem.

The jose invocations are hard to understand since they use the short form.

There are several different coding styles around, for example sometimes using jose -j"$VAR" and sometimes jose -j- <<< "$VAR". Likewise for loading the output of external programs. And some constructions look sloppy. They might be safe but it eases code review if you use additional safeguards.

Suggestions:

* In jose invocations, always use the long parameter form. Only exception: `-UUU` but see [latchset/jose#80](https://github.com/latchset/jose/issues/80) for a suggestion about this.

* Have a little style guide and re-write the scripts accordingly. For me, the rules are:

* (At least) scripts running in the early userland must be executable by busybox' ash

* Run with "set -e" and "set -u". The latter gives a bad time initially but since the scripts are run in a sensitive situation, it's worth it.

* Use `$(...)` instead of backticks.

* _Always_ put quotes around expansion unless you really know what you're doing.

* In strings, use double quotes only when expansion is necessary, single quotes else.

* And over all: Be consistent in how you do things.

This is some work, I know. I created two prototypes, see the next issues, so you can get am impression how the results will look. I can to do this for the existing scripts as well. But only if you signalize you're fine with the idea and are willing include the result.

I like the ideas here, and the end result of improving portability/readability/consistency is also very interesting. So, feel free to submit changes to existing scripts to incorporate these suggestions.

I have just submitted #205 changing clevis-luks-common-functions and its user. I'd appreciate if you could take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants