Refactor new x/upgrade validation to allow checksum to be optional

[dwedul-figure]

Summary

When validating upgrade plans, make it possible for urls to not include a checksum query parameter.

Problem Definition

Currently, in cosmovisor, the checksums are optional. If provided, they're used for verification, but they're not required. The new x/upgrade module plan validation requires the urls to have a checksum query parameter though.

This makes it impossible to reuse the module's validation and download logic by cosmovisor. Related: #10464

Additionally, it'd be nice to allow users that don't use a checksum on one or more url, to be able to validate the other parts of their plans.

Proposal

Add three flags to the CLI software-upgrade command:

• --no-validate-info-checksum - if provided, a url in the info field will not be required to have a checksum.
• --no validate-binary-checksums - if provided, a url in the binaries map will not be required to have a checksum.
• --no-validate-checksums - Same as providing both --no-validate-info-checksum and --no validate-binary-checksums.

Then separate out the validation that requires urls to have a checksum to facilitate these new flags. Also ensure that the download functionality doesn't require urls to have a checksum.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[iramiller]

do we still need this?

[SpicyLemon]

I'm going to take myself off of this. A while back I had some work done toward it, but things have changed a bunch since then (e.g. gov v1), and I haven't been able to make any progress in a long time.

Since the submit-proposal is now legacy, and govv1 CLI doesn't allow a way to do special validation on a proposal, I'm not sure there's anything to do here now.

[julienrbrt]

Re-using x/upgrade logic in cosmovisor (#10464), made Cosmovisor require a binary checksum for upgrades (#14881).
Given that now checksums will be enforced for both (from Cosmovisor 1.5.0 #16131), I think we can close this.
What do you think?

[SpicyLemon]

Re-using x/upgrade logic in cosmovisor (#10464), made Cosmovisor require a binary checksum for upgrades (#14881). Given that now checksums will be enforced for both (from Cosmovisor 1.5.0 #16131), I think we can close this. What do you think?

This issue is very much still needed. Now, though, this issue is about making the checksums optional in cosmovisor again too.

The extra validation done using the software-upgrade command (or any tx command really) is all just sugar. It's an extra bonus you get for using that command. But there are plenty of other ways to submit an upgrade proposal that don't involve that command.

When this was written, that checksum was optional. The extra validation would require it, but that validation could be skipped. It was more of a suggestion. I.e. "You should use checksums on your urls or else jump through this hoop if you want to use this command." This issue was then to allow more granular skipping of parts of that validation so that you could, for example, have the validation check that all the URLs are valid and return expected things, but do so without requiring checksums on the URLs. But the checksums were always intended to be optional, not required.

Requiring URLs to have a checksum doesn't actually improve security (as claimed in the changelog). Requiring it only removes options. In fact, we've had two instances where security was improved by NOT having a checksum. In both cases, a security issue was identified in a version slated for upgrade. The new version had been created and an upgrade proposal had been submitted, voted on, and passed. Then issues were found when there wasn't enough time to get a new proposal passed before the upgrade height would be reached. By not having the checksum on the URL in the info field, we were able to update the binaries map returned by that URL to point to fixed versions of the software. So when the upgrade height was reached, the fixed versions were auto-downloaded and run instead of the vulnerable ones. If a checksum is required, that's no longer something that can be done. What we haven't had happen is someone hijacking the server/domain used for those URLs. I get that that's a concern, but it's not a concern for everyone.

Also, when we discussed requiring checksums in the cosmovisor/upgrade working group, we found that requiring checksums was extremely unlikely to cause anyone to enable auto-download that didn't already use auto-download. There were several use cases for not having them though. So requiring them would have lots of negative effects with little-to-no positive effects.

---

Additionally, the only place where there should be code that downloads anything is either in cosmovisor or the x/upgrade/client/cli/ directory. There should not be any code in x/upgrade/ for doing downloads that isn't in that client/cli/ directory. I know I'm partially responsible for that, but it's still something to address. Downloads aren't consistent and can't be trusted to be replayable; during normal blockchain operation, there shouldn't be anything that might rely on an external download like that. SDK users shouldn't even have the opportunity to accidentally trigger something like that. By putting it in the client/cli directory, it's clearer that nothing in x/upgrade/ will try to download anything related to a plan.

Really, since all that download and validation stuff is specific to cosmovisor, it's probably better to have the software-upgrade command just generate and submit the gov prop with only superficial validation of the content. Then have a validate-upgrade command in cosmovisor that checks that everything is downloadable and in the formats expected by cosmovisor. That way, there isn't cosmovisor-specific logic in the x/upgrade module. Cosmovisor can assume that the chain behaves the same as the x/upgrade module defines, but the x/upgrade module shouldn't assume that everyone is using cosmovisor.

[julienrbrt]

Thanks for the context. This makes sense and I'll look into that!
Let's block next Cosmovisor release until the above is resolved.