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

Implement Checksum validation #59

Merged

Conversation

Shaderbug
Copy link
Contributor

@Shaderbug Shaderbug commented Apr 24, 2024

Hi everyone,

this is my implementation of checksum checking, I tried to make my contributions match the existing style in this codebase. Looking forward to your feedback :)

Overall design

While Godot unfortunately doesn't sign their releases, they publish JSON files containing SHA-512 checksums of their official builds in the godotengine/godot-builds repository.

This PR introduces a IGodotChecksumClient interface, which is intended to be used to work with and validate checksums of archives, and implements it with a GodotChecksumClient that accesses the aforementioned builds repository to obtain checksums and validate them.

This client is then used in the GodotRepository after a file has been downloaded to verify its checksum against the published one.

As Godot doesn't publish checksums for builds up to 3.2.2-beta1, an additional command-line parameter called --unsafe-skip-checksum-verification has been added to the godot install command, that allows skipping the checksum verification.

For JSON parsing, System.Text.Json is used, and for Checksum calculation System.Security.Cryptography, introducing no new dependencies.

As the unit tests require real JSON data to test the implementation against, two files from the godot-builds repository are added to the repository and included into the test assembly as embedded resource.

Another thing of note is that two unit tests create temporary files for testing the checksum calculation and mismatch handling, but they should be collision free due to one using a standard library function for creating a temporary file, the other appending a GUID to the path it creates.

Additional Changes

  • Moved GodotRepositoryTest into a folder matching its namespace
  • Tests for the SemanticVersion class were added, and it was extended with a method that formats semantic versions the way Godot prints them (i.e. omitting the patch if it's zero).
  • Added a fix, as older macOS releases don't use the .universal suffix, but .64 instead

Closes #58

@Shaderbug Shaderbug force-pushed the checksum-validation branch 2 times, most recently from 408698b to dba19db Compare April 24, 2024 22:11
@Shaderbug Shaderbug marked this pull request as draft April 24, 2024 22:17
@Shaderbug
Copy link
Contributor Author

Converted to draft PR, as I've just noticed that Godot doesn't seem to provide checksums for builds up to 3.2.2-beta1, which will need to be handled by informing the user about the missing checksum and asking how to proceed.

I'll extend the PR for that, and if anybody wants, intermediate feedback is greatly appreciated :)

@definitelyokay
Copy link
Member

definitelyokay commented Apr 24, 2024

Hey, thank you! This looks like it took quite a bit of effort, so I applaud you for getting it done! Overall, code looks very well structured. I might suggest renaming the GodotChecksumRepository to GodotChecksumClient since I usually think of repositories like the GodotRepository as owning and leveraging clients (one layer beneath), but that's a pretty small nit.

Would you go into more detail in your PR description about the steps this takes to ensure checksums are valid and how it does it, as well as how the tests or test data work (specifically, what the GodotEnv.Tests/src/features/godot/domain/data/godot-4.3-dev5.json file is for) ? I simply need a high level explanation that I can reference later whenever I have to work on GodotEnv next, and I want to make sure I don't get too behind.

@Shaderbug
Copy link
Contributor Author

Thanks for the feedback, I'll address it in the coming days, and seeing the failed CI runs, I'll also test it more thoroughly.

@ghost
Copy link

ghost commented Apr 26, 2024

Nice addition! Thanks for working on this @Shaderbug.

@Shaderbug
Copy link
Contributor Author

Hi everyone,

I need some input from you on how we want to proceed with this PR when there's no published checksum for an archive, or a checksum mismatch.

My first plan would be:

  • If there's a checksum mismatch, or no published checksum file for a given version can be found, it's an unrecoverable error and we abort installation without option to override
    • Seems like the safest default, and if for some reason there are use cases that require it, an override can be implemented and shipped quickly in an update.
  • If there is a published checksum file, but it doesn't contain a checksum for a given downloaded file
    • Skip verification if a --unsafe-allow-files-without-published-checksum flag was passed
      • This would allow usage in non-interactive sessions
    • Otherwise print a big warning for the user and ask interactively if they still want to go ahead with the installation
    • Generally, this is only the case for old Godot versions, so it's unlikely users will encounter this.

@definitelyokay
Copy link
Member

If I understand correctly, I think prefer if users just retry the command with a flag in either of those scenarios, as opposed to interactive input prompts. It's simpler. Maybe a shorter flag name, like --allow-unverified?

@ghost
Copy link

ghost commented Apr 29, 2024

+1 to using a flag for both scenarios over interactive prompts.

I like the shorter flag name. Here are a a few suggestions to consider if you'd prefer the flag to include "checksum" in the name.
--allow-unsafe-checksum
--allow-invalid-checksum
--allow-unverified-checksum

@Shaderbug Shaderbug marked this pull request as ready for review May 1, 2024 15:56
@Shaderbug
Copy link
Contributor Author

I've updated the description as requested. If further questions arise during the review, I'm happy to amend it :)

@definitelyokay definitelyokay merged commit 205bffd into chickensoft-games:main May 2, 2024
7 checks passed
@definitelyokay
Copy link
Member

I've released version 2.1.0 with your changes! Thank you so much for your hard work 🥳

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

Successfully merging this pull request may close these issues.

Add checksum verification for Godot downloads
2 participants