chocolatey / chocolatey/choco

CryptoHashProvider and FilesService snapshotting behaviour needs to be redesigned

Open
#3,133 0 comments 1 reaction 0 assignees View on GitHub
0 - Triaging Enhancement
Dominant language
C#
Stars
11.5k
Forks
960
PR merge metrics
No merged PRs in 30d

Description

### Checklist

- [X] I have verified this is the correct repository for opening this issue.
- [X] I have verified no other issues exist related to my problem.
- [X] I have verified this is not an issue for a specific package.
- [X] I have verified this issue is not security related.

### What You Are Seeing?

The current API structure of CryptoHashProvider / IHashProvider makes it very easy to create accidental side effects. Calling `SetHashAlgorithm` takes effect for _the entire Chocolatey process_ and there is no real way to check which algorithm is currently being used.

We may also want to look into setting the IHashProvider up in the SimpleInjectorContainer as a _transient_ service rather than a single-instance service to help mitigate potential accidents here.

Additionally, in our FilesService where we take file snapshots that include a file hash. This API also needs some improvement to make it a little less opaque and a little more configurable. Currently it takes no parameters as to what kind of file hashes to generate, so it is fully reliant on the _current_ settings of the IHashProvider.

By default, the FilesService generates MD5 file hashes. If something in the Choco process before calling into the FilesService receives an IHashProvider from dependency injection and calls `SetHashAlgorithm`, the generated file snapshots will be using _that_ setting for file hashes.

This causes issues in our NugetService methods `RemoveInstallationFiles` and `RemoveInstallationFilesUnsafe` which are using the file hashes to check which files it should delete. If a file snapshot has been serialised previously with non-MD5 hashes, these methods will be checking hashes generated from completely different hash algorithms and fail to delete files they should.

### What is Expected?

For `CryptoHashProvider / IHashProvider`:

- A method should be added to get what the current hash algorithm in use is
- We should add a way to "scope" the use of a specific hash provider if we need to change it, rather than setting it for the entire process, which can have odd side effects. This could potentially be implemented via a helper class that implements IDisposable and resets the hash provider back to what it was before when it leaves the scope.

For `FilesService` / `IFilesService`:

- Methods for snapshotting files should have overloads which take a `CryptoHashProviderType` to use during snapshotting
- Generated snapshots should store both the hash _and_ the hash algorithm used to generate the hash going forward
- A helper method should be added to "guess" the probable hash algorithm used to generate a given hash, so that existing file snapshots that don't record the checksum type can still be understood and we can compare them against appropriately generated hashes. Unsure exactly where this method should be placed, but probably here or in CryptoHashProvider.
- We should probably add a helper method here that is specifically for comparing a previously generated file snapshot's hash to the current hash of a given file, using a guessed hash algorithm if necessary.

For `NugetService`'s `RemoveInstallationFiles*` methods:

- These should be refactored, they're almost identical and almost all their code is duplicated.
- They should delegate the actual checking of the hash to the FilesService helper method mentioned above that should be added.

Additionally, we should reevaluate why we're setting all hashes to MD5 in `ConfigurationBuilder.SetHashProvider` when FIPS is not enabled. There doesn't seem to be any reason for it that I was able to find, and it doesn't seem sensible to do so. Given this is (currently) a process-wide setting, it seems unwise to use MD5 here.

### How Did You Get This To Happen?

This actually was discovered due to a subtle difference in the v5.x and 6.x branches of CLE, the reproduction there was:

1. Install an application outside of choco
2. Use `choco sync` to generate package files for it
3. Uninstall the package
4. Realise all the package files are still in the `lib` directory despite the package being "uninstalled"

A workaround can be applied there, but we should design this API area much better, there's too much potential for accidents the way this is currently setup.

### System Details

N/A

### Installed Packages

```bash
N/A
```

### Output Log

```bash
N/A
```

### Additional Context

_No response_

Contributor guide

Open the contributing guide

Research direction

Start by reading CryptoHashProvider/IHashProvider and tracing their registration in SimpleInjectorContainer, then inspect FilesService/IFilesService, NugetService's RemoveInstallationFiles methods, and ConfigurationBuilder.SetHashProvider. Map how algorithms flow into snapshots and deletion checks before proposing the API design. Done should cover scoped or inspectable hash providers, algorithm-aware snapshots and comparisons, reduced NugetService duplication, and updated configuration behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, cryptography
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.