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

determine post-upgrade failure-mode for a SHA-1-signed submission key #7030

Closed
cfm opened this issue Oct 26, 2023 · 3 comments · Fixed by #7035
Closed

determine post-upgrade failure-mode for a SHA-1-signed submission key #7030

cfm opened this issue Oct 26, 2023 · 3 comments · Fixed by #7035
Assignees

Comments

@cfm
Copy link
Member

cfm commented Oct 26, 2023

Description

After #6948 (for #6399), redwood will refuse to encrypt to a submission key with a SHA-1 signature.

After #6928, securedrop-admin sdconfig will reject a submission key with a SHA-1 signature. This check guarantees that new and reconfigured instances will comply with #6948.

What will happen to an instance with a SHA-1-signed signature after upgrading to v2.7.0?

Possible approaches

Option Documentation changes Code changes Implication
Fail open, but log optional Admin must monitor logs and/or OSSEC alerts.
Fail open, but document Admin must monitor release notes or check documentation.
Fail closed optional ✓[1] Admin can contact us for help.

Notes:

  1. @legoktm observes that, without a code change to handle this case, Apache will come back up after reboot even if the postinst script fails under unattended-upgrades.
@legoktm
Copy link
Member

legoktm commented Oct 26, 2023

For context, we expect this to be a pretty rare issue given that if people followed our documentation, they should not have generated a SHA-1 submission key.

The submission key is currently stored in the GPG keyring, so we export it to disk for Sequoia to read (and for SecureDrop to serve) in the postinst:

export_journalist_public_key() {
    # config.py is root-writable, so it's safe to run it as root to extract the fingerprint
    journalist_pub="/var/lib/securedrop/journalist.pub"
    # If the journalist.pub file doesn't exist
    if ! test -f $journalist_pub; then
        # And we have a config.py (during initial install it won't exist yet)
        if test -f /var/www/securedrop/config.py; then
            # n.b. based on sdconfig.py, this should work with very old config.py files.
            fingerprint=$(cd /var/www/securedrop; python3 -c "import config; print(config.JOURNALIST_KEY)")
            # Set up journalist.pub as root/www-data 640 before writing to it.
            touch $journalist_pub
            chown root:www-data $journalist_pub
            chmod 640 $journalist_pub
            # Export the GPG public key
            # shellcheck disable=SC2024
            sudo -u www-data gpg2 --homedir=/var/lib/securedrop/keys --export --armor "$fingerprint" > $journalist_pub
            # Verify integrity of what we just exported
            sudo -u www-data /var/www/securedrop/scripts/validate-pgp-key "$journalist_pub" "$fingerprint"
        fi
    fi

}

The final step is to validate the exported key file passes validate-pgp-key, which is a wrapper for redwood.is_valid_public_key(). However, if validation fails for whatever reason (e.g. GPG export fails, it's a SHA-1 key, etc.), then we abort, which interrupts the postinst, so, e.g. database upgrades won't be applied. apache will be down, though it should be restarted when the nightly instance reboot happens.

With that in mind, I think having the validation fail at package install time is bad, because it requires even more manual work to get an instance in a working state since you need to manually apply the updates after doing a key rotation.

One of the proposals in standup from @zenmonkeykstop was to just have the JI exit if the submission key isn't valid, which will alert journalists and then their admin that something is wrong. I think this is reasonable and probably a better UX, and will let us remove the validate step in the postinst IMO.

@zenmonkeykstop
Copy link
Contributor

Works for me (:)) - if we can just use is_valid_public_key() in app initialization then that sounds simple enough.

@rocodes
Copy link
Contributor

rocodes commented Oct 26, 2023

Yeah, from trying this on my staging instance I was momentarily confused by the postinst validation/failure, and as you mentioned it has a bunch of unintended side-effects. I think it's better to have it fail in the JI. In this case we don't have to worry about feature parity with SDW.

legoktm added a commit that referenced this issue Oct 26, 2023
During the Sequoia migration, we need to export the journalist public
key from the GPG keyring into a file on disk. We also needed to validate
the key was usable by Sequoia (e.g. no SHA-1 binding signatures).

Previously the plan was to validate it during the postinst and error out
if it wasn't valid, but if validation fails for whatever reason, then we
abort, which interrupts the postinst, so, e.g. database upgrades won't
be applied. In retrospect having the validation fail at package install
time is bad, because it requires even more manual work to get an
instance in a working state since you need to manually apply the updates
after doing a key rotation.

Now we validate the journalist key during startup of the Journalist
Interface, printing and logging an error if it doesn't validate and then
exiting. This should bring attention to journalists and therefore the
admin that the instance needs manual attention. We will also include
information about this change in the pre-release and release
announcements.

Fixes #7030.
@cfm cfm moved this to In Progress in SecureDrop dev cycle Oct 26, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in SecureDrop dev cycle Oct 27, 2023
zenmonkeykstop pushed a commit that referenced this issue Oct 27, 2023
During the Sequoia migration, we need to export the journalist public
key from the GPG keyring into a file on disk. We also needed to validate
the key was usable by Sequoia (e.g. no SHA-1 binding signatures).

Previously the plan was to validate it during the postinst and error out
if it wasn't valid, but if validation fails for whatever reason, then we
abort, which interrupts the postinst, so, e.g. database upgrades won't
be applied. In retrospect having the validation fail at package install
time is bad, because it requires even more manual work to get an
instance in a working state since you need to manually apply the updates
after doing a key rotation.

Now we validate the journalist key during startup of the Journalist
Interface, printing and logging an error if it doesn't validate and then
exiting. This should bring attention to journalists and therefore the
admin that the instance needs manual attention. We will also include
information about this change in the pre-release and release
announcements.

Fixes #7030.

(cherry picked from commit 7fd01d9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants