Reject weak keys (SHA-1 signature) in admin gpg-validate script

[rocodes]

Status

Ready for review:

• Pending discussion question here about using sq-keyring-linter on ossec, journo key or just submission key
• Still need to update some of our tests to create valid test keys :)
• Should we do any kind of more formal review of sq-keyring-linter before including it on admin workstations? (discussed: it's in default repos, not happening now and Tails is considering including it stock ,so we aren't doing review for now)

Description of Changes

Fixes #6796

Changes proposed in this pull request:

• Per original issue, add sq-keyring-linter package to admin bootstrap deps and use it to perform additional validation on pubkeys.
• To be discussed: currently, this validates against all pubkeys in use (submission key, journo key if applicable, ossec key). We could change it to only check the Submission key, or we could leave it as is.

Note: In cases where there is more than one Admin workstation at an organization, any users on a non-updated admin workstation (who have not imported the updated pubkey) will see this validation error. I have tried to address this case in the console error message that the user sees.

Testing

How should the reviewer test this PR?

• visual review makes sense and console error messages in securedrop-admin are clear
• admin-tests are passing (not yet - wip :) )
• "Fresh install" scenario aka no venv present: check out this branch in Tails admin workstation and run ./securedrop-admin --force setup -- the sq-keyring-linter package should be installed
• "Existing admin stick" scenario (aka venv present, previous run of ./securedrop-admin setup completed): User should be informed that a dependency is missing, then prompted for their passphrase and the sq-keyring-linter package will be downloaded
• the validate-gpg.sh script correctly handles various cases: a good GPG pubkey successfully validates, return code 0; a GPG pubkey with mismatched fingerprint does not successfully validate, return code 1; and a GPG pubkey with correct fingerprint but weak key (SHA-1 signed) does not validate, return code 2.
• validate-gpg.sh gracefully handles case where sq-keyring-linter package is missing (prints a warning but continues with validation).

Here is a sample bad key for ease of testing, or you can generate one with gpg --full-generate-key (choose RSA, 1024 bit):

-----BEGIN PGP PUBLIC KEY BLOCK-----

mI0EZOPgWwEEALRtUxob9g8IYRBVGZfsIM79e7OuqCUNnnpcNkNZ4tiKyd8yjjfD
2t4OZ5WAv7VuseDThwGnDoCUJ3ZZXtKTtJITtYvtHsQox3BZoz5wSWRSJDO8npKU
Zv0j7Dy8uqv0n69J402+3Fq9mELyekH9/j29UqLdUTzRQgH+ZkXAH27DABEBAAG0
O0JBREtFWSAoU2hvdWxkIGZhaWwgc3Eta2V5cmluZy1saW50ZXIpIDxCQURLRVlA
Tk9SRVBMWS5ORVQ+iM4EEwEKADgWIQRA8cF7fngm2rQLFK53hrAA5tCnbgUCZOPg
WwIbAwULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAKCRB3hrAA5tCnboiIA/oDjkPm
fhg9/2XdylpdB0WhY55vNLweSyQW6tWEgoL4IMl2HJx38WiMsNbqgWfsw/cLHrRW
eQfPJiB4n8jbucSFmutO9nlKT+yN/G1eVrSikz0nZ4OlLtW5Jy2T70LVzo3Lb6fE
Ve9zkVp+AaOxgsxlLj8aEX8E9tdD7EdmMKSJIbiNBGTj4FsBBACt+L8aGNutQfqK
iqqtwncUGBWdXNZgy+2SCmNF6QGwj0m8AlgBjERfbeBcYo3Mw2PIPM1r5UlXFiMy
blF32L7kZGxy5ETYaADHilGoJHCubtpBH4hDRsmt9OKydFSQvE01+CHLmAfiXBzx
KK48B2nVTseLIPhdxOW15GGd9QwLNwARAQABiLYEGAEKACAWIQRA8cF7fngm2rQL
FK53hrAA5tCnbgUCZOPgWwIbDAAKCRB3hrAA5tCnbqQ1A/9JSda8nzav4lgBw8co
dbB0s9AdvGymtlTWLUFdfHRaNYwHInUtXIagDhgJNLaa75xd/WNLvvjPcV3SoaOC
hGVDM/BkMb87VxjeYgHzKdN5MxMrPvITS5Y0EGB0ITvG1MTqHWalhY99pyRqeCRA
2BWLtQSMMPWQxML48db4EtfTQw==
=rpbe
-----END PGP PUBLIC KEY BLOCK-----

key details:

$ gpg --fingerprint 40F1C17B7E7826DAB40B14AE7786B000E6D0A76E
pub   rsa1024 2023-08-21 [SC]
      40F1 C17B 7E78 26DA B40B  14AE 7786 B000 E6D0 A76E
uid           [ultimate] BADKEY (Should fail sq-keyring-linter) <[email protected]>
sub   rsa1024 2023-08-21 [E]

Deployment

TK

Checklist

If you made changes to the server application code:

• Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

• Linting and tests (make -C admin test) pass in the admin development container not yet, tk

If you made changes to the system configuration:

• Configuration tests pass

If you added or removed a file deployed with the application:

• I have updated AppArmor rules to include the change

If you made non-trivial code changes:

• I have written a test plan and validated it for this PR

Choose one of the following:

• I have opened a PR in the docs repo for these changes, or will do so later
• I would appreciate help with the documentation
• These changes do not require documentation

If you added or updated a production code dependency:

Production code dependencies are defined in:

• admin/requirements.in
• admin/requirements-ansible.in
• securedrop/requirements/python3/requirements.in

If you changed another requirements.in file that applies only to development
or testing environments, then no diff review is required, and you can skip
(remove) this section.

Choose one of the following:

• I have performed a diff review and pasted the contents to the packaging wiki
• I would like someone else to do the diff review

[rocodes]

Per #6796 (comment), will keep the behaviour that checks all keys against sq linter, not just the Submission Key. This will likely mean some additional support tickets as users have to update their keys and push them to the server (and to other Admin workstations) - we may even want to create a docs page and link to it in the console error message.

[legoktm]

Just skimming right now, will do a more complete review shortly. I missed that there was another copy of the test_journalist_key.pub in the admin/ hierarchy, I fixed that key in 4b5ef49 to not use SHA-1 signatures. I would suggest we copy the fixed key into admin/ (so they're in sync) and then commit your generated bad key as the failure example.

(I'm not asking you to rewrite it, but I don't understand why our Python code is shelling out to a shell script, this logic seems much easier to do in Python IMO...)

[legoktm]

Should we do any kind of more formal review of sq-keyring-linter before including it on admin workstations?

I don't personally see the need since we're getting it from Debian/Tails. One note is that the packaged sequoia stuff is using nettle as the crypto library (instead of openssl, which we're using for redwood) but we're not doing any crypto stuff here, just examining the keys, so the difference doesn't matter.

[zenmonkeykstop]

• visual review makes sense and console error messages in securedrop-admin are clear
• admin-tests are passing
• check out this branch in Tails admin workstation and run ./securedrop-admin setup -- the sq-keyring-linter package should be installed FAILED when admin/.venv3 already exists
• the validate-gpg.sh script correctly handles various cases:
  • a good GPG pubkey successfully validates, return code 0;
  • a GPG pubkey with mismatched fingerprint does not successfully validate, return code 1 (passes except when key is truncated by 1 char;
  • and a GPG pubkey with correct fingerprint but weak key (SHA-1 signed) does not validate, return code 2.

Also tested via ./securedrop-admin --force sdconfig - the validation happens and an error is displayed for bad keys, but the site-specific file is still created, so an admin could miss the error and still install.

[zenmonkeykstop]

This will not be run on systems where there's an existing virtualenv. So the sq-keyring-linter package would not be installed by most GUI updater runs.

[zenmonkeykstop]

This returns positive for cases where the supplied fingerprint is truncated by 1 char.

[rocodes]

Thank you for your comments @zenmonkeykstop - I think I've addressed them in 6875fcb. I've also stepped through ./securedrop-admin setup and config on a Tails machine with and without the apt dependencies already installed, to make sure all cases are covered (sorry about that oversight).

Re: the truncated fingerprint: Using sdconfig blocks the user from submitting anything but a 40-char hex fingerprint. But even so, using just the validate-gpg.sh script, I could not produce a false positive with a too-short or too-long fingerprint. If you can still do so do you mind sharing an example pubkey and fingerprint that produce this result?

[rocodes]

(Just mentioning out loud something that was said in standup)
A change that was proposed in this PR is to check if admin dependencies are stale every time envsetup is run (so every time the admin workstation is run), to ensure apt dependencies are up to date.

However, it was mentioned that this is a pretty flaky part of the workstation and might lead to frustration (the "check network status and try again" issue), which is a good point.

In that case, I'd propose to revert that change to the PR (so we'd be back to only checking apt dependencies if there's no venv present). The script already handles cases where the sq-keyring-linter package is not installed by doing the rest of the gpg validation, just skipping the key linting.

(And then optionally if we wanted to, to handle the 'existing venv' case, we could print a message to console, eg advising them to run ./securedrop-admin setup again but not requiring it).

@zenmonkeykstop is that a better approach? I admit that network flakiness wasn't top of mind when I wrote this pr.

[rocodes]

@zenmonkeykstop if the admin workstation has all the right dependencies, is_missing_dependency won't prompt for sudo or do any network calls.

If a package name changes (eg sq-keyring-linter gets replaced by sq), this still won't try and install sq-keyring-linter, because the apt-cache policy results wont be Installed: (none), they'll be N: unable to locate package $funnyname.

If you're not a fan, let me know and we can at least move the check into the branch of envsetup where no existing venv is detected.

[rocodes]

(dependencies refactored from line 120)

[zenmonkeykstop]

LGTM, thanks for the changes!