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

Add support for Coursier checksum verification #134

Merged
merged 1 commit into from
May 1, 2019

Conversation

mandrean
Copy link
Contributor

@mandrean mandrean commented Apr 29, 2019

This PR adds support for Coursier's --checksum flag.

While it's not SHA pinning or verified with the actual SHA of the downloaded artifact, it's still better than nothing which is the case today.

Also, if one uses HTTPS Maven repositories, in-transit tampering should be reasonably secure.

@jin
Copy link
Member

jin commented Apr 29, 2019

Why don't we make this compulsory with --checksum SHA-256,SHA-1,MD5 for every call?

@mandrean
Copy link
Contributor Author

mandrean commented Apr 29, 2019

I guess I assumed Coursier defaults to None (and it's nicer to set None than "None" 😄 ) but either way SHA-1 is the default on most Maven repos. Done!

@jin
Copy link
Member

jin commented Apr 29, 2019

Sorry, I wasn't clear -- we can make every call issued with --checksums SHA-256,SHA-1,MD5. Then the API for maven_install will be a boolean attribute (error_on_missing_checksum?) to disable erroring out if there are no checksums by specifying --checksums SHA-256,SHA-1,MD5,None instead.

Specifying --checksums SHA-256,SHA-1,MD5 allows falling through the types, so at least we make checksum verification compulsory for artifacts that do have checksums. We also want to keep the option open for artifacts that don't have checksums without skipping other artifacts, like in that spring_boot example.

SHA-1 has also been cracked, so it may make sense to prioritize SHA-256 over the other checksums if it exists.

@mandrean
Copy link
Contributor Author

@jin Better? Also, do you want it on or off by default?

@jin
Copy link
Member

jin commented Apr 30, 2019

Hmm looks like there are issues with sha256 on the springboot example. Let's drop sha256 from the argument lists for now? Looks like sha1 is still the defacto standard for Maven deps.

@mandrean
Copy link
Contributor Author

Strange... Fails:

$ coursier fetch com.fasterxml:oss-parent:24 -r https://jcenter.bintray.com --checksum SHA-256 --cache v1

Works:

$ coursier fetch com.fasterxml:oss-parent:24 -r https://jcenter.bintray.com --checksum SHA-256,None --cache v1

Fails:

$ coursier fetch org.hamcrest:hamcrest-library:1.3 org.springframework.boot:spring-boot-autoconfigure:2.1.3.RELEASE org.springframework.boot:spring-boot-test-autoconfigure:2.1.3.RELEASE org.springframework.boot:spring-boot-test:2.1.3.RELEASE org.springframework.boot:spring-boot:2.1.3.RELEASE org.springframework.boot:spring-boot-starter-web:2.1.3.RELEASE org.springframework:spring-beans:5.1.5.RELEASE org.springframework:spring-context:5.1.5.RELEASE org.springframework:spring-test:5.1.5.RELEASE org.springframework:spring-web:5.1.5.RELEASE --artifact-type jar,aar,bundle,eclipse-plugin,orbit,test-jar,src --quiet --no-default --json-output-file dep-tree.json --checksum SHA-256,SHA-1,MD5,None --repository https://jcenter.bintray.com --cache v1

The fall-through doesn't seem perfectly reliable...

@mandrean
Copy link
Contributor Author

@jin Green builds now

Copy link
Member

@jin jin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jin jin merged commit 1b81f67 into bazelbuild:master May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants