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

Do not treat unknown checksum types as MD5 #932

Closed
jberezanski opened this issue Aug 26, 2016 · 9 comments
Closed

Do not treat unknown checksum types as MD5 #932

jberezanski opened this issue Aug 26, 2016 · 9 comments
Assignees
Milestone

Comments

@jberezanski
Copy link

jberezanski commented Aug 26, 2016

Currently, when a package passes a checksum type unknown to Chocolatey, Chocolatey treats it as MD5. This is suboptimal, as it leads to misleading error messages about checksum verification failures, such as here.

Upon encountering an unknown checksum type, Chocolatey should stop the installation with a clear error: "Checksum type '$checksumType' is unsupported. Please upgrade your Chocolatey client."

sha256/sha512 were added in 0.9.9.9 - #113

@ferventcoder
Copy link
Member

And how do you propose we fix that older version?

@ferventcoder
Copy link
Member

More information - We can fix this going forward, but the message is already there in the older versions, where we would want the to be. But we can't put it in the older versions so they would need to upgrade to take advantage of that. A slight chicken and egg problem if you get where I'm going with this.

@ferventcoder
Copy link
Member

In retrospect, it was likely a bad idea not to bubble that up as a warning. And possibly to do that at all.

@ferventcoder
Copy link
Member

ferventcoder commented Aug 26, 2016

I guess it used to be worse -

if ($checksumType -ne 'sha1') { $checksumType = 'md5'}

This is representative of what those older clients are doing, not even a warning or a debug message.

@ferventcoder
Copy link
Member

In all fairness, at least the error message mentions the wrong type -

throw "Checksum for `'$file'` did not meet `'$checksum`' for checksum type `'$checksumType`'."

@ferventcoder
Copy link
Member

ferventcoder commented Aug 27, 2016

It's still wrong though, and we should handle it. As a workaround, I think we'll suggest folks take a dependency on at least the version of Chocolatey that is necessary to support the package. Although that may not fix the current install chain (until choco reloads during the current install chain when it is brought in as a dependency).

@jberezanski
Copy link
Author

Yes, there is nothing we can do about the older versions, we can only prevent this from occurring in the future (in case a new hash algorithm gains popularity).

I like the dependency on a minimum Chocolatey version. Perhaps it could be made an official moderation requirement and a check for it could even be implemented in the validator?

The issue of choco upgrade not affecting the current install brings back memories: chocolatey-archive/chocolatey#460
I'm not sure how the autoupgrade process of Chocolatey works currently (which files are replaced when). However, checksum support is implemented in Get-CheckSumValid.ps1 and checksum.exe, so reloading of choco.exe should not be required for the new algorithms to light up - assuming (1) the helpers and tools are upgraded in place and (2) choco.exe resets its PowerShell host state between packages.

@ferventcoder
Copy link
Member

@jberezanski with upgrade, all module files are reimported, but the rest of Chocolatey (exe parts) still runs under the old process during upgrade.

This can lead to an undesired situation. For instance - if I called an install for something that needed 0.10.0, which has the new checksum requirement and it brought the module files along with it, the install will fail for something that takes a dependency but doesn't have checksums in it. Plus you can't pass --allow-empty-checksums to the older version. Even if you could, choco.exe 0.9.10.3 wouldn't be able to pass the proper arguments/environment variables through to the new module code.

The above situation is about impossible to expect it to ever pass, but I can imagine there may be a situation where one thing is required to be passed from the exe to the PowerShell modules that could fail spectacularly.

@jberezanski
Copy link
Author

Agreed.
I've created a new issue so that we do not wander off-topic here too much ;-)

@ferventcoder ferventcoder added this to the 0.10.1 milestone Sep 10, 2016
@ferventcoder ferventcoder self-assigned this Sep 10, 2016
ferventcoder added a commit that referenced this issue Sep 12, 2016
Let the checksum stay as whatever unrecognized version it is and throw
an error when it is not a recognized type. Mention that the value may
be supported in a newer version of Choolatey.
ferventcoder added a commit that referenced this issue Sep 12, 2016
* stable: (24 commits)
  (GH-839) Switch to apply package parameters to dependent packages
  (maint) formatting methods / parameters in calls
  (GH-958) If SSLv3 in Posh v2 Fails, Use Original
  (GH-746) Use HTTPS if available when HTTP url
  (GH-957) Skip Get-WebFileName When FTP
  (GH-948) Ensure passwords / keys are not logged
  (GH-952) Get-ChocolateyWebFile enhancements
  (doc) update generated docs
  (maint) formatting
  (docs) move GenerateDocs.ps1 / update
  (GH-932) Do not set unknown checksum to 'md5'
  (GH-719) Reset ServerCertificateValidationCallback
  (GH-305) add MSP/MSU installer types
  (GH-305) update exitcodes to long
  (GH-954) Pending fails when lib does not exist
  (GH-950) Install-ChocolateyPackage - UseOriginalLocation
  (maint) formatting
  (GH-922) Automatically determine checksum type
  (maint) fixes for shimgen
  (GH-948) Do not log sensitive arguments
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants