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

Support checksum (sha256) verification in dist fields (for downloaded data) #5940

Open
emanuelb opened this issue Dec 5, 2016 · 22 comments
Open
Labels
Milestone

Comments

@emanuelb
Copy link

emanuelb commented Dec 5, 2016

While it will not provide the security offered by signing verification:
#4022

it easier to implement and does help against malformed target resource (server/mirror/CDN hacked / MITM on connection / etc...) when assuming composer.json content authentic.

suggestion:
add sha256 field that will verify the downloaded data.
for example in:
https://www.phpmyadmin.net/packages.json
all the dist fields except dev-master can contain sha256 field that contain checksum as they point to static files.

@alcohol
Copy link
Member

alcohol commented Dec 5, 2016

This is not really feasible. The majority of downloads are automatically fetched from github's dist API. Users have no way of knowing up front what exactly the hash will be, nor does Packagist actually download any of the dist files, so it cannot add this metadata either.

@emanuelb
Copy link
Author

emanuelb commented Dec 5, 2016

I see, it's feasible only in some cases (such as phpmyadmin.net/packages.json) but not for most downloads triggered by composer (as it's download dynamic files [can have different content X minutes later])
so it's not a high priority, but still good to have as there is no timeframe for signature validation support (which effect both static & dynamic files)
The installation instruction at PMA are:

composer create-project phpmyadmin/phpmyadmin --repository-url=https://www.phpmyadmin.net/packages.json --no-dev

The files.phpmyadmin.net domain (which the static files are served from) point to CDN (thus it's better to validate the downloads) as they can be malformed (for example: as happened long time ago in hacked mirror of sourceforge)

@stof
Copy link
Contributor

stof commented Dec 5, 2016

The checksum feature is already supported for dist archives (but it uses sha1, not sha256). Simply add the checksum key inside dist alongside the url and type.

@emanuelb
Copy link
Author

emanuelb commented Dec 5, 2016

Thanks, it's better then no verification :)
where is the mention of checksum in documentation? (didn't found it)

@stof
Copy link
Contributor

stof commented Dec 5, 2016

AFAICT, it is not documented. The doc for the repository schema is a bit incomplete currently.

@emanuelb
Copy link
Author

emanuelb commented Dec 5, 2016

Well then there is 2 issues remain:

  1. update the documentation regarding the checksum field.
  2. support stronger hash functions (such as sha256) as sha1 usage is deprecated (better to use stronger hash if possible):
    https://blog.qualys.com/ssllabs/2014/09/09/sha1-deprecation-what-you-need-to-know

@rgpublic
Copy link

Huh. Does the checksum feature work for anyone?
I have in my packages.json sth. like:

            "dist": {
                    "url": "https:\/\/blablabla.com\/bla.tar.gz",
                    "type": "tar",
                    "checksum": "idontcare"
                }

...and the package happily installs. Why? Shouldnt the given checksum ("idontcare") be compared to the sha1 of the downloaded tar.gz?

@alcohol
Copy link
Member

alcohol commented Jul 24, 2018

$checksum = $package->getDistSha1Checksum();
$cacheKey = $this->getCacheKey($package, $processedUrl);
// download if we don't have it in cache or the cache is invalidated
if (!$this->cache || ($checksum && $checksum !== $this->cache->sha1($cacheKey)) || !$this->cache->copyTo($cacheKey, $fileName)) {
if (!$this->outputProgress) {
$this->io->writeError('Downloading', false);
}
// try to download 3 times then fail hard
$retries = 3;
while ($retries--) {
try {
$rfs->copy($hostname, $processedUrl, $fileName, $this->outputProgress, $package->getTransportOptions());
break;
} catch (TransportException $e) {
// if we got an http response with a proper code, then requesting again will probably not help, abort
if ((0 !== $e->getCode() && !in_array($e->getCode(), array(500, 502, 503, 504))) || !$retries) {
throw $e;
}
$this->io->writeError('');
$this->io->writeError(' Download failed, retrying...', true, IOInterface::VERBOSE);
usleep(500000);
}
}
if (!$this->outputProgress) {
$this->io->writeError(' (<comment>100%</comment>)', false);
}
if ($this->cache) {
$this->lastCacheWrites[$package->getName()] = $cacheKey;
$this->cache->copyFrom($cacheKey, $fileName);
}
} else {
if (!$this->outputProgress) {
$this->io->writeError('Loading from cache', false);
}
}
if (!file_exists($fileName)) {
throw new \UnexpectedValueException($url.' could not be saved to '.$fileName.', make sure the'
.' directory is writable and you have internet connectivity');
}
if ($checksum && hash_file('sha1', $fileName) !== $checksum) {
throw new \UnexpectedValueException('The checksum verification of the file failed (downloaded from '.$url.')');
}

Are you sure your "package.json" is actually read? Did you run your composer install or update command in -vvv mode?

@rgpublic
Copy link

@alcohol Thanks for your answer. Yes, it is read. I checked with -vvv. It seems as if it doesnt have any effect what I write as value.

@alcohol
Copy link
Member

alcohol commented Jul 24, 2018

Can you share your verbose output with us?

@rgpublic
Copy link

@stof
Copy link
Contributor

stof commented Jul 24, 2018

is https://packages.test.local/download.php/8.x/autocron.tar.gz the one for which you have this idontcare checksum ? Because that's the only downloaded package, and it does not match what you showed in your report (trying to obfuscate names in bug reports makes things much harder, if it is not applied consistently in the discussion).

@rgpublic
Copy link

Yes, this is the package I try to (re-)download. I delete it from the folder (modules/autocron). Run the command. It downloads and installs again. I wonder why. I've added the "idontcare" checksum to all packages in packages.json.

@rgpublic
Copy link

        "pw6\/autocron": {
            "25.0.0": {
                "name": "pw6\/autocron",
                "version": "25.0.0",
                "type": "drupal-custom-module",
                "dist": {
                    "url": "https:\/\/packages.test.local\/download.php\/8.x\/autocron.tar.gz",
                    "type": "tar",
                    "checksum": "idontcare"
                }
            }
        },

@rgpublic
Copy link

Ah, okay. Solved it now. Thanks @alcohol for bringing me on the right track. I've unpacked the PHAR archive of composer to debug everything and followed the files. I finally found out that the key is not called "checksum" but "shasum". Hooray. Progress. +1 for better docs, though ;-)

@alcohol
Copy link
Member

alcohol commented Jul 25, 2018

Ah, yeah, it is actually documented also:

"shasum": {

But since it is most often used only internally, I kind of forgot about it.

@staabm
Copy link
Contributor

staabm commented Jul 25, 2018

Should composer install/update warn on invalid schema?

Wasting time because of invalid files and missing to call composer validate is a pity

@alcohol
Copy link
Member

alcohol commented Jul 25, 2018

I think composer diagnose validates the schema? Not certain.

Never mind, it is validate yeah, and probably most people do not run that often.

@talbergs
Copy link

Nix community really needs this sha256 to be there in composer.lock.

@stof
Copy link
Contributor

stof commented Jul 10, 2023

Well, as long as Github and Gitlab don't ensure that their commit zip archives have a stable checksum, there is nothing composer can do about it (as packagist.org references their archives and Github and Gitlab are representing the vast majority of packages registered on packagist.org, probably over 95%).

@talbergs
Copy link

talbergs commented Aug 3, 2023

Can't composer pull in the zip into tmp and take the sha from it? @stof
If so, I am happy to prepare PR for this feature.

@imme-emosol
Copy link
Contributor

As far as i can tell, the suggested resolve for this issue is written in #2540 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants