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

Some validation of site-specific occurs after file is saved #6950

Open
rocodes opened this issue Sep 25, 2023 · 1 comment
Open

Some validation of site-specific occurs after file is saved #6950

rocodes opened this issue Sep 25, 2023 · 1 comment
Assignees

Comments

@rocodes
Copy link
Contributor

rocodes commented Sep 25, 2023

Description

Surfaced by @zenmonkeykstop in review of #6928 (comment).

We do 2 kinds of validation of the user input provided during sdconfig. One kind of very basic 'validation' happens during the sdconfig prompt, and ensures that (eg) the PGP key fingerprint is 40 hex characters, or the filename provided for a public key actually points to a file (see: the SiteConfig validators here), or that the SD test key isn't being used to provision an instance. Here, the user is informed in real time if they try to enter bad data into the config file.

The other kind of validation happens once the file has been written. We call validate-gpg-keys and validate_journalist_email right after we've saved the config file. This means that the config file could be written with bad data.

The reason I'm filing this and not just fixing it is because I think there are a couple ways to fix it - see below.

Steps to Reproduce

./securedrop-admin sdconfig , then enter eg a 40-char wrong fingerprint for a supplied pubkey file during configuration, and observe the validation error message. Then inspect the site-specific file

Expected Behavior

Invalid config file is not saved

Actual Behavior

Invalid config is written to file

Comments

Fixes:

  1. Validate before save. (Just switch the order of the calls in update_config). The downside of this is that if we don't save everything the admin has just typed, they may get frustrated when they run sdconfig again, and/or have a hard time spotting their mistake.
  2. Validate at the time of the prompt input. I think this way is better, but it introduces a little more logic.
    a. We'd change ValidateFingerprint and ValidateOptionalFingerprint to not just validate that the fingerprint is the correct number of characters, but also that it meets our other validation criteria (meaning it corresponds to the correct PGP pubkey file, and it passes sq-keyring-linter). Note: while writing this I realized I wasn't sure why we ask the user to type out the fingerprint, since we already ask for the pubkey file anyway. If it's as a double check, there may be a less burdensome way to do that, like just displaying the fingerprint for them. But that's another kettle of fish.)
    b. We'd move the journalist email address "validator" (really just checks that, if we're supplying an email address, we have a string and an @) into the section where we have the user input that email.

I have WIP for option 2, just didn't want to creep outside the scope of the reject SHA-1 keys PR into touching other logic. [Edit: but since the changes would apply to keys and email addresses, which is pretty similar to the scope of the reject SHA-1 keys changes, if people are good with approach 2 I can tack on some commits to my existing PR].

@rocodes
Copy link
Contributor Author

rocodes commented Sep 28, 2023

A followup comment (mostly for myself) for if we are updating the validation process:

Right now all keys are validated at the same time, and the first error that's encountered is what's reported to the user. (So if there are 2 bad keys only the first error msg is shown - then the user reruns sdconfig and sees the next error, etc).

This still leads me to favour option 2, which would check each key and fingerprint as they're input, and show an error during the prompt (the same as the way we display the errors about fingerprint length, not using the test key, etc)

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

No branches or pull requests

1 participant