Ci verify lockfiles signed

[Jontified]

Add script that verifies that lockfiles are only modified in signed commits and adds a github action workflow for this on PRs for Cargo.lock and package-lock.json. This is done in order to enforce a stricter security policy.

---

This change is 

[albin-mullvad]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Jontified)

---

ci/verify-lockfile-signature.sh line 5 at r2 (raw file):

shopt -s nullglob
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
LOCKFILES="$SCRIPT_DIR/../Cargo.lock $SCRIPT_DIR/../gui/package-lock.json"

I know this is sort of a WIP, but it would perhaps be good to move this list into a separate file so that it's easy to add additional lockfiles.

[Jontified]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Jontified)

ci/verify-lockfile-signature.sh line 5 at r2 (raw file):

shopt -s nullglob
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
LOCKFILES="$SCRIPT_DIR/../Cargo.lock $SCRIPT_DIR/../gui/package-lock.json"

I know this is sort of a WIP, but it would perhaps be good to move this list into a separate file so that it's easy to add additional lockfiles.

That sounds like a good idea, I'll add this in. Perhaps it might also be a good idea to make the naming a bit more generic in case the script will be used for other types of security critical files in the future?

[faern]

Reviewed 2 of 4 files at r4.
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Jontified)

---

ci/locked_down_files.txt line 3 at r4 (raw file):

Cargo.lock
gui/package-lock.json
ci/trusted_keys.pub

May I suggest that we keep each key in a separate file in a dedicated keys/ directory instead? That way it becomes easier to modify when a developer joins, leaves or changes key. Currently it's a blob that has to be fully re-generated to change anything.

---

.github/workflows/verify-locked-down-signatures.yml line 7 at r4 (raw file):

            - .github/workflows/verify-locked-down-signatures.yml
            - Cargo.lock
            - gui/package-lock.json

This list is now not in sync with locked_down_files.txt. Is there any way to automate that? Best would maybe be if github actions allowed loading a list from file, but that is probably not supported(?).

If the above is not possible, can we invert it? specify the file list here and then parse it out in the script where we need it? In order to need duplicates mostly.

---

.github/workflows/verify-locked-down-signatures.yml line 15 at r4 (raw file):

            - uses: actions/checkout@v3
              with:
                ref: ${{ github.event.pull_request.head.sha }}

Why do we need to explicitly give this sha? is this commit not the default one it checks out anyway?

[faern]

Nice! This looks very good overall. Some smaller stuff to look at maybe, but otherwise splendid.

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Jontified)

---

ci/verify-locked-down-signatures.sh line 16 at r4 (raw file):

LOCKED_DOWN_FILES=$(cat $SCRIPT_DIR/locked_down_files.txt)
for locked_file in $LOCKED_DOWN_FILES;
do

Very nitpicky, but according to our style guide, the do should be on the same line as the for. Just like how if and then are.

[Jontified]

.github/workflows/verify-locked-down-signatures.yml line 15 at r4 (raw file):

            - uses: actions/checkout@v3
              with:
                ref: ${{ github.event.pull_request.head.sha }}

Why do we need to explicitly give this sha? is this commit not the default one it checks out anyway?

It seems that cheackout@v3 will create a temporary merge branch, if we want to be on the latest commit we need to use that ref.

I have fixed all other comments. The deduplication of locked_down_files.txt became fairly complicated since parsing the yaml isn't straightforward. Please review the changes in the script that parse the yaml and tell me if you think there is a better way of doing it @faern.

[Jontified]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @faern)

---

ci/locked_down_files.txt line 3 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

May I suggest that we keep each key in a separate file in a dedicated keys/ directory instead? That way it becomes easier to modify when a developer joins, leaves or changes key. Currently it's a blob that has to be fully re-generated to change anything.

Done.

---

.github/workflows/verify-locked-down-signatures.yml line 7 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This list is now not in sync with locked_down_files.txt. Is there any way to automate that? Best would maybe be if github actions allowed loading a list from file, but that is probably not supported(?).

If the above is not possible, can we invert it? specify the file list here and then parse it out in the script where we need it? In order to need duplicates mostly.

Done.

---

ci/verify-locked-down-signatures.sh line 16 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Very nitpicky, but according to our style guide, the do should be on the same line as the for. Just like how if and then are.

Done.

[faern]

Reviewed 2 of 12 files at r5.
Reviewable status: 2 of 12 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad, @faern, and @Jontified)

---

ci/verify-locked-down-signatures.sh line 5 at r5 (raw file):

shopt -s nullglob
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
# In the CI environment we would like to import trusted public keys from a file, but not in our build environment

We usually try to stick to 100 column scripts. These comment lines are pretty long. Consider breaking them up. Maybe also introduce some space between lines? Currently everything from the shebang down to whitelisted_commit is a single block (no empty lines). In most other bash scripts we usually separate it something like

#!/usr/bin/env bash

set -eu

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# Constants

# Code

[Jontified]

Reviewed 2 of 12 files at r5.
Reviewable status: 2 of 12 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad, @faern, and @Jontified)

ci/verify-locked-down-signatures.sh line 5 at r5 (raw file):

shopt -s nullglob
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
# In the CI environment we would like to import trusted public keys from a file, but not in our build environment

We usually try to stick to 100 column scripts. These comment lines are pretty long. Consider breaking them up. Maybe also introduce some space between lines? Currently everything from the shebang down to whitelisted_commit is a single block (no empty lines). In most other bash scripts we usually separate it something like

#!/usr/bin/env bash

set -eu

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# Constants

# Code

I've looked over the code again and tried to trim down the rows as well as spaced it out a bit. Let me know what you think.

[faern]

Reviewed 1 of 12 files at r5.
Reviewable status: 3 of 12 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Jontified)

---

.github/workflows/verify-locked-down-signatures.yml line 25 at r7 (raw file):

                    git fetch --depth="$(( commits + 1 ))" origin ${{ github.head_ref }} master
                fi
                ci/verify-locked-down-signatures.sh --import-gpg-keys --whitelist origin/master

Maybe not important. But is there any way to get the name of the branch the PR is based off of? So we can do that instead of origin/master? That way, if you ever create a PR based off of some other branch, it will correctly check against that base commit. Only worth it if it's easily available through some variable or something. Not worth making a complicated solution for it.

[albin-mullvad]

Reviewable status: 3 of 12 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @faern)

---

.github/workflows/verify-locked-down-signatures.yml line 25 at r7 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Maybe not important. But is there any way to get the name of the branch the PR is based off of? So we can do that instead of origin/master? That way, if you ever create a PR based off of some other branch, it will correctly check against that base commit. Only worth it if it's easily available through some variable or something. Not worth making a complicated solution for it.

Sounds like a good idea! github.base_ref can be used for that, however note:

"This property is only available when the event that triggers a workflow run is either pull_request or pull_request_target"

https://docs.github.com/en/actions/learn-github-actions/contexts#github-context

[faern]

Reviewable status: 3 of 12 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad, @faern, and @Jontified)

---

ci/verify-locked-down-signatures.sh line 26 at r7 (raw file):

            shift
            ;;
        -*)

This branch not needed, as agreed upon in the coding guidelines PR :)

---

ci/verify-locked-down-signatures.sh line 43 at r7 (raw file):
Is --armor really needed? I have never used it when importing and I never hit any issues with armored key files. And the gpg man page seems to indicate that it's only for output:

   --armor
   -a     Create ASCII armored output.  The default is to create the binary OpenPGP format.

[Jontified]

Reviewable status: 3 of 12 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @faern)

---

ci/verify-locked-down-signatures.sh line 26 at r7 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This branch not needed, as agreed upon in the coding guidelines PR :)

Done.

---

ci/verify-locked-down-signatures.sh line 43 at r7 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is --armor really needed? I have never used it when importing and I never hit any issues with armored key files. And the gpg man page seems to indicate that it's only for output:

   --armor
   -a     Create ASCII armored output.  The default is to create the binary OpenPGP format.

Right you are, fixed.

[faern]

Reviewed 3 of 3 files at r8.
Reviewable status: 4 of 12 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Jontified)

---

ci/verify-locked-down-signatures.sh line 64 at r8 (raw file):

    for commit in $locked_file_commit_hashes;
    do

Style nitpick. for ...; do on one line

---

ci/verify-locked-down-signatures.sh line 65 at r8 (raw file):

    for commit in $locked_file_commit_hashes;
    do
        if ! $(git verify-commit $commit 2> /dev/null); then

Nitpick. I don't think you need a subshell here. Just if ! command should work

---

ci/verify-locked-down-signatures.sh line 66 at r8 (raw file):

    do
        if ! $(git verify-commit $commit 2> /dev/null); then
            echo Commit $commit which changed $locked_file is not signed.

Please enclose the string to echo with "". Not strictly needed. But we usually do that. And it makes syntax highlighting better.

[faern]

Reviewable status: 4 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Jontified)

---

ci/verify-locked-down-signatures.sh line 58 at r8 (raw file):

unsigned_commits_exist=0
for locked_file in $locked_down_files; do

For the path ci/keys this will be a directory. The current code here won't git rev-list each individual key file, but rather the entire keys directory. I think that's fine, because git will still list every commit any file in it was changed. But maybe name the variable locked_path instead to not say it's a file when it strictly isn't?

[faern]

Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

[faern]

Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 51 at r8 (raw file):

Previously, Jontified wrote…

Done.

This feels overkill to me. It adds an extra line of code, and it's not very likely to become a bug or issue if we don't readonly the variable.

[faern]

Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 5 at r8 (raw file):

Previously, Jontified wrote…

Good suggestion!

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
readonly SCRIPT_DIR

However above seems more idiomatic per:
https://www.shellcheck.net/wiki/SC2155

Let's not get this PR blocked on bash bikeshedding. That can be had in the coding guidelines repository. We need this CI job to be merged.

[Jontified]

Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 5 at r8 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Let's not get this PR blocked on bash bikeshedding. That can be had in the coding guidelines repository. We need this CI job to be merged.

Sounds good.

---

ci/verify-locked-down-signatures.sh line 51 at r8 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This feels overkill to me. It adds an extra line of code, and it's not very likely to become a bug or issue if we don't readonly the variable.

It might be a little bit overkill, but locked_down_paths is not super obviously read-only now that it's not longer SCREAMING_SNAKE_CASE. In the spirit of not bike-shedding further I suggest we just merge this as is.
Every other complaint is solved and this seems like it might be good.

[faern]

Reviewable status: 3 of 12 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

[faern]

Reviewed 8 of 12 files at r5.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

[faern]

Reviewable status: 10 of 12 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 51 at r8 (raw file):

Previously, Jontified wrote…

It might be a little bit overkill, but locked_down_paths is not super obviously read-only now that it's not longer SCREAMING_SNAKE_CASE. In the spirit of not bike-shedding further I suggest we just merge this as is.
Every other complaint is solved and this seems like this might be good.

On slack we agreed to not start littering with readonly for these uncertain cases until we have established a standard. So this should go away.

[Jontified]

Reviewable status: 10 of 12 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 51 at r8 (raw file):

Previously, faern (Linus Färnstrand) wrote…

On slack we agreed to not start littering with readonly for these uncertain cases until we have established a standard. So this should go away.

Done.

---

ci/verify-locked-down-signatures.sh line 64 at r8 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Style nitpick. for ...; do on one line

Fixed

---

ci/verify-lockfile-signature.sh line 5 at r2 (raw file):

Previously, Jontified wrote…

That sounds like a good idea, I'll add this in. Perhaps it might also be a good idea to make the naming a bit more generic in case the script will be used for other types of security critical files in the future?

This is done.

[faern]

Reviewed 1 of 1 files at r10, 1 of 1 files at r11.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @MrChocolatine)

---

.github/workflows/verify-locked-down-signatures.yml line 25 at r7 (raw file):

Previously, albin-mullvad wrote…

Sounds like a good idea! github.base_ref can be used for that, however note:

"This property is only available when the event that triggers a workflow run is either pull_request or pull_request_target"

https://docs.github.com/en/actions/learn-github-actions/contexts#github-context

This can be looked at later if hard to solve now.

[faern]

Reviewable status: 11 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, @Jontified, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 16 at r12 (raw file):

# The whitelisted commit can be set in order to allow github actions to only check changes
# since origin/master
whitelisted_commit="5d41b8a1d9745fbb3ff81ea6ea2eb8f202ca7ed0"

I suppose this commit hash has to be changed? Or have we really been signing cleanly since then? Sounds too good to be true :) But that can be fixed after merging also. Since our CI does not force us to sign, we have no incentive to sign. So I guess we need to merge this first.

[faern]

Reviewed 1 of 2 files at r13.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

[faern]

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

[Jontified]

Reviewable status: 10 of 12 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 16 at r12 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I suppose this commit hash has to be changed? Or have we really been signing cleanly since then? Sounds too good to be true :) But that can be fixed after merging also. Since our CI does not force us to sign, we have no incentive to sign. So I guess we need to merge this first.

Changed this hash to the latest master and signed all of the commits in this PR. We should not need to merge this later.

[faern]

Reviewed 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @MrChocolatine)

[faern]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @Jontified, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 72 at r14 (raw file):

done

exit $unsigned_commits_exist

A bit nitpicky. But I realize from the output of the CI that it's not apparent that this script even ran at all. It prints nothing on success. I feel like we should check unsigned_commits_exist here and print a success message before exiting the script if everything worked as expected. As a sort of canary making it easier to detect if the script did not run at all.

[faern]

Reviewable status: 11 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, @Jontified, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 16 at r15 (raw file):

# The whitelisted commit can be set in order to allow github actions to only check changes
# since origin/master
whitelisted_commit="618130520"

I think I signed everything between the last one and here. So should not need to bump. But does not matter.

[Jontified]

Reviewable status: 11 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad, @faern, and @MrChocolatine)

---

ci/verify-locked-down-signatures.sh line 72 at r14 (raw file):

Previously, faern (Linus Färnstrand) wrote…

A bit nitpicky. But I realize from the output of the CI that it's not apparent that this script even ran at all. It prints nothing on success. I feel like we should check unsigned_commits_exist here and print a success message before exiting the script if everything worked as expected. As a sort of canary making it easier to detect if the script did not run at all.

Added this.

[faern]

Reviewed 1 of 1 files at r16.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @MrChocolatine)

[faern]

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @MrChocolatine)