Add source-checksum option #619

tsimonq2 · Jun 30, 2016

This adds an optional source-checksum tag to ensure download integrity by checking the checksum.

Features

It supports the following source-types:
- source-type: tar
- source-type: zip
It supports the following checksums:
- md5
- sha1
- sha224
- sha256
- sha384
- sha512
You can use it in the following ways:
- source-checksum: HASH - raw hash
- source-checksum: checksum.txt - local file
- source-checksum: http(s)://example.com/checksum - remote URL

Failsafes

It has the following failsafes to ensure the code always works as intended:

Special exception for a checksum that doesn't match
Raises an exception for a checksum that is invalid, or incompatible with the checksums supported

Testing coverage

I have the following testing in place:

Unit tests:
- Testing of each checksum in raw form with a created file (tar/zip) that has a static checksum
- Creation of a checksum file and checking the checksum with that local file
- Pulling http://tsimonq2.net/misc/snapcraft-checksum-tar and http://tsimonq2.net/misc/snapcraft-checksum-zip respectively and checking the checksum of the static file with those
- Checking for an invalid checksum
Integration tests:
- Testing of each checksum in raw form with a created file (tar/zip) that has a static checksum in the snapcraft.yaml files for simple-tar and simple-zip

Discrepancies between this specification and the source

If you find any difference between the code in this PR and the specification above, please comment, as that wasn't intended.

snappy-m-o · Jun 30, 2016

Can one of the admins verify this patch?

snappy-m-o · Jun 30, 2016

Can one of the admins verify this patch?

sergiusens · Jun 30, 2016

sergiusens reviewed Jun 30, 2016

View changes

why this change?

sergiusens · Jun 30, 2016

sergiusens reviewed Jun 30, 2016

View changes

this should be done after the download

which is https://github.com/snapcore/snapcraft/pull/619/files#diff-e6205a80af76b4faec1d9241dc3b3b7cR119

sergiusens · Jun 30, 2016

sergiusens reviewed Jun 30, 2016

View changes

this looks like duplicate code, please consider adding it to FileBase (which also needs its init updated)

sergiusens · Jun 30, 2016

Please merge with master or rebase

sergiusens · Jun 30, 2016

you also need to update https://github.com/snapcore/snapcraft/pull/619/files#diff-e6205a80af76b4faec1d9241dc3b3b7cR455

tsimonq2 · Jun 30, 2016

@sergiusens Thanks for taking the time to look at this. I updated my code with your suggestions. Let me know what you think.

@elopio Can you please take a look?

sergiusens · Jul 1, 2016

From a quick look at the test failures it seems you still need to update all constructors for the sources used in the unit tests.

tsimonq2 · Jul 5, 2016

So right now, the Travis build has passed. My goal now is to do some more local testing and to get an integration test ready. I'll change the description accordingly.

snappy-m-o · Jul 6, 2016

Can one of the admins verify this patch?

snappy-m-o · Jul 6, 2016

Can one of the admins verify this patch?

snappy-m-o · Jul 7, 2016

Can one of the admins verify this patch?

snappy-m-o · Jul 7, 2016

Can one of the admins verify this patch?

snappy-m-o · Jul 8, 2016

Can one of the admins verify this patch?

snappy-m-o · Jul 8, 2016

Can one of the admins verify this patch?

tsimonq2 · Jul 8, 2016

I believe that all the intended testing and code is in place for this PR. Could this be evaluated please?

tsimonq2 · Jul 8, 2016

I also added sort of a specification to refer to in my initial comment.

tsimonq2 · Jul 12, 2016

Bump

sergiusens · Jul 12, 2016

sergiusens reviewed Jul 12, 2016

View changes

this should use a fakeserver, check snapcraft/tests/fake_servers.py

@sergiusens I'm a little confused as to the usage of this, where can I find an example of a test case that uses it?

sergiusens · Jul 12, 2016

sergiusens reviewed Jul 12, 2016

View changes

why the abreviation?

I didn't have any specific reason, I'll change it

sergiusens · Jul 12, 2016

Hi, looks good

One general comment is to use checksum instead of chksum

sergiusens · Jul 12, 2016

sergiusens reviewed Jul 12, 2016

View changes

what are you testing here, or in other words, what are your assertions to know it all worked as expected?

maybe I am confused by check_checksum_determine_format and a better method name is needed here.

@sergiusens this function uses the various supported checksum formats and checks the checksum with them. This ensures that the checksum can be successfully checked with the raw checksums.

I know this works because if it doesn't, the built-in errors will catch it. That's why I'm not doing a try/fail here.

The only reason why I had to split it into separate functions was because the static tests were complaining about check_checksum being too complex. This is now corrected to be in one function.

sergiusens · Jul 12, 2016

sergiusens reviewed Jul 12, 2016

View changes

ditto on assertions

sergiusens · Jul 12, 2016

sergiusens reviewed Jul 12, 2016

View changes

if this is only used by check_checksum_determine_format then prefix the method name with _

sergiusens · Jul 12, 2016

sergiusens reviewed Jul 12, 2016

View changes

This could be a dictionary instead

_HASH_FUNCTIONS = {
    32: hashlib.md5,
    40: hashlib.sha1,
    ...
    ...
}

try:
     checksumer = _HASH_FUNCTIONS[len(source_checksum)]()
except KeyError:
    raise IncompatibleOptionsError("Invalid checksum format")

sergiusens · Jul 12, 2016

Another general comment,

you should name the method the sources classes verify_checksum; verify_checksum should call _get_checksum and return the checksum doing the http, file or checksum dance you do.

With that returned value, if you use the dict, just do the check there and finally check if they are a match or raise the exception.

This IMHO would read much simpler.

tsimonq2 · Jul 21, 2016

@sergiusens I made some changes that addresses your comments, let me know if you have more.

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

use single quotes here

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

this should be an error, shouldn't it?

@sergiusens do you think it's always the case that the checksum file has a filename? If so, I'll correct it.

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

these two lines certainly fit in one line, right?

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

please use a different name than zip, zip is a python keyword and it might get confusing.

@sergiusens This is applicable to the original source as well and should be dealt with in a separate PR, I'm just using what has already been given to me for variable names.

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

quotes

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

should probably be an error.

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

can we stick to using format and single quotes? (no +)

I'm not sure what you mean here, @sergiusens

Should be something like

raise ChecksumDoesNotMatch(
   'the checksum {!r} doesn't match the file'.format(source_checksum)

I can't use single quotes because doesn't has a quote.

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

single quotes please

sergiusens · Jul 30, 2016

sergiusens reviewed Jul 30, 2016

View changes

these statements fit in the same line

Pushing a fix to a bunch of lines that can be fixed by this, thanks for pointing this out.

sergiusens · Aug 2, 2016

OK, one final checkup, in the bug sabdfl mentions we should support sha-3-384; how do we differentiate sha-2 and sha-3 with the autodetection?

kyrofa · Aug 12, 2016

@tsimonq2 did you see @sergiusens's question?

sergiusens · Aug 24, 2016

Closing due to inactivity

Merge branch 'master' into downloaded-files-checksum-bug-1585913

Some checks were not successful
1 failing and 1 pending checks

continuous-integration/travis-ci/pr — The Travis CI build failed

Details

coverage/coveralls — Coverage pending from Coveralls.io

Details

78f7fb3

Merge branch 'master' into downloaded-files-checksum-bug-1585913

Some checks were not successful
1 failing and 1 successful checks

coverage/coveralls — Coverage decreased (-0.2%) to 96.034%

Details

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

32ee9c3

tsimonq2 changed the title from Add source-checksum option for tar and zip sources (bug 1585913) to Add source-checksum option Jul 8, 2016

Merge branch 'master' into downloaded-files-checksum-bug-1585913

All checks have passed
2 successful checks

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

coverage/coveralls — Coverage increased (+0.02%) to 96.349%

Details

3a5a154

sergiusens closed this Aug 24, 2016

pachulo referenced this pull request Dec 15, 2016
Merged
sources: add optional "source-checksum" property #980

@@ -68,6 +71,89 @@ def test_pull_tarball_must_download_to_sourcedir(self, mock_prov):
                  with open(os.path.join(dest_dir, tar_file_name), 'r') as tar_file:
                      self.assertEqual('Test fake compressed file', tar_file.read())
+        +    def test_checksum(self):

@@ -114,6 +200,89 @@ def test_extract_and_keep_zipfile(self, mock_zip):
                  with open(zip_download, 'r') as zip_file:
                      self.assertEqual('Test fake compressed file', zip_file.read())
+        +    def test_checksum_of_zip(self):

+        +    elif len(source_checksum) == 128:
+        +        chksum = hashlib.sha512()
+        +    else:
+        +        raise IncompatibleOptionsError("Invalid checksum format")

+        +        data = response.read()
+        +        source_checksum = data.decode('utf-8')
+        +        if (" " in source_checksum):
+        +            source_checksum = source_checksum.split(" ", 1)[0]

+        +        if (" " in source_checksum):
+        +            source_checksum = source_checksum.split(" ", 1)[0]
+        +        else:
+        +            print('No file name detected in the checksum file, perhaps an '

snapcore/snapcraft

Join GitHub today

Add source-checksum option #619

Conversation

tsimonq2 commented Jun 30, 2016 • edited Edited 1 time tsimonq2 Jul 5, 2016

Features

Failsafes

Testing coverage

Discrepancies between this specification and the source

tsimonq2 added some commits Jun 29, 2016

Some checks were not successful

snappy-m-o commented Jun 30, 2016

snappy-m-o commented Jun 30, 2016

sergiusens reviewed Jun 30, 2016 View changes

sergiusens reviewed Jun 30, 2016 View changes

sergiusens reviewed Jun 30, 2016 View changes

sergiusens commented Jun 30, 2016

Some checks were not successful

sergiusens commented Jun 30, 2016

tsimonq2 added some commits Jun 30, 2016

Some checks were not successful

Some checks were not successful

tsimonq2 commented Jun 30, 2016

sergiusens commented Jul 1, 2016

tsimonq2 added some commits Jul 1, 2016

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

tsimonq2 commented Jul 5, 2016

tsimonq2 added some commits Jul 5, 2016

Some checks were not successful

All checks have failed

All checks have failed

Some checks were not successful

All checks have failed

snappy-m-o commented Jul 6, 2016

snappy-m-o commented Jul 6, 2016

Some checks were not successful

snappy-m-o commented Jul 7, 2016

snappy-m-o commented Jul 7, 2016

tsimonq2 added some commits Jul 7, 2016

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

All checks have failed

snappy-m-o commented Jul 8, 2016

tsimonq2 added some commits Jul 8, 2016

All checks have failed

All checks have failed

All checks have failed

All checks have failed

snappy-m-o commented Jul 8, 2016

tsimonq2 added some commits Jul 8, 2016

All checks have failed

Some checks were not successful

Some checks were not successful

All checks have passed

tsimonq2 commented Jul 8, 2016

tsimonq2 changed the title from Add source-checksum option for tar and zip sources (bug 1585913) to Add source-checksum option Jul 8, 2016

tsimonq2 commented Jul 8, 2016

All checks have passed

tsimonq2 commented Jul 12, 2016

sergiusens reviewed Jul 12, 2016 View changes

sergiusens reviewed Jul 12, 2016 View changes

sergiusens commented Jul 12, 2016

sergiusens reviewed Jul 12, 2016 View changes

tsimonq2 Jul 15, 2016 • edited Edited 1 time tsimonq2 Jul 15, 2016

sergiusens reviewed Jul 12, 2016 View changes

sergiusens reviewed Jul 12, 2016 View changes

sergiusens reviewed Jul 12, 2016 View changes

sergiusens Jul 12, 2016 • edited Edited 1 time sergiusens Jul 12, 2016

sergiusens commented Jul 12, 2016

tsimonq2 added some commits Jul 13, 2016

All checks have passed

All checks have passed

All checks have passed

tsimonq2 commented Jun 30, 2016 •

Edited 1 time

tsimonq2
Jul 5, 2016

sergiusens reviewed Jun 30, 2016

View changes

sergiusens reviewed Jun 30, 2016

View changes

sergiusens reviewed Jun 30, 2016

View changes

sergiusens reviewed Jul 12, 2016

View changes

sergiusens reviewed Jul 12, 2016

View changes

sergiusens reviewed Jul 12, 2016

View changes

tsimonq2 Jul 15, 2016 •

Edited 1 time

tsimonq2
Jul 15, 2016

sergiusens reviewed Jul 12, 2016

View changes

sergiusens reviewed Jul 12, 2016

View changes

sergiusens reviewed Jul 12, 2016

View changes

sergiusens Jul 12, 2016 •

Edited 1 time

sergiusens
Jul 12, 2016

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes

sergiusens reviewed Jul 30, 2016

View changes