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 SHA-256/512 in update site metadata #3356

Merged
merged 2 commits into from Jun 29, 2018

Conversation

5 participants
@daniel-beck
Member

daniel-beck commented Mar 17, 2018

If the update site metadata provides SHA-512 or SHA-256 checksums, check those.

This is intended as a security hardening. SHA-1 isn't a good choice anymore.

Companion PR in infra: jenkins-infra/update-center2#191. As Jenkins will continue to support update sites that only provide SHA-1 (for now, and with a warning), they could be merged independently, but probably should not be.

Proposed changelog entries

  • RFE: Check SHA-512 or SHA-256 checksums of update site metadata and core and plugin downloads if the update site provides them.

Submitter checklist

  • [n/a] JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs
@Wadeck

Wadeck approved these changes Mar 20, 2018

🐝 nothing seems weird.

Build failed but no errors are shown...

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 10, 2018

@daniel-beck is it still on hold?

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Apr 10, 2018

@oleg-nenashev This is on-hold because it depends on the corresponding PR to Jenkins project infra. Timeline:

  1. merge update-center2 PR
  2. Run this PR build and confirm it works as expected against a live UC
  3. Merge this.

This is ready to be reviewed etc. -- in fact, I invite feedback before I make infra changes.

@daniel-beck daniel-beck removed the on-hold label Apr 25, 2018

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Apr 25, 2018

Corresponding infra change is merged.

@jenkinsci/code-reviewers Ping -- would like to land this this week.

@daniel-beck daniel-beck added the on-hold label Apr 26, 2018

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Apr 26, 2018

@rtyler objected to the massive increase in size of update-center.json, so I've followed up in jenkins-infra/update-center2#196 and need to adapt this PR.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Apr 26, 2018

Rebased to current master.

I'm not convinced this needs adapting, will try the upcoming PR build.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented May 29, 2018

Un-onholding this. @jenkinsci/code-reviewers assemble!

@daniel-beck daniel-beck requested review from dwnusbaum and Wadeck May 29, 2018

@Wadeck

Wadeck approved these changes May 29, 2018

🐝

@dwnusbaum

What is the reason for the partial SHA-256 support if the update center only has SHA-1 and SHA-512 after jenkins-infra/update-center2#196? Is it so that if we eventually add SHA-256 back to the update center we don't have to modify as much code on the Jenkins side? Other than that question the changes look good to me.

VerificationResult result512 = verifyChecksums(entry.getSha512(), job.getComputedSHA512(), false);
switch (result512) {
case PASS:
// this has passed but no real reason not to check the others

This comment has been minimized.

@dwnusbaum

dwnusbaum May 29, 2018

Member

Is the idea behind falling through here that if there is a SHA-512 collision that isn't also a SHA-256 or SHA-1 collision it will be caught by checking the other signatures?

This comment has been minimized.

@daniel-beck

daniel-beck May 30, 2018

Member

Basically, yes. Seems extremely unlikely that this is a likely case, but as the comment says, no real reason not to check the others, if they are defined.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented May 30, 2018

What is the reason for the partial SHA-256 support if the update center only has SHA-1 and SHA-512 after jenkins-infra/update-center2#196?

It's essentially free to support this way, and SHA-256 could be a desirable mode for some custom update sites.

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented May 30, 2018

It's essentially free to support this way, and SHA-256 could be a desirable mode for some custom update sites.

In that case, should we add support for SHA-256 to JSONSignatureValidator for consistency?

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented May 31, 2018

In that case, should we add support for SHA-256 to JSONSignatureValidator for consistency?

That's probably a good idea. Let's see whether I can find the time to do this within this PR.

@dwnusbaum

Looks good to me. I am fine with SHA-256 support being added to JSONSignatureValidator as a separate change.

@oleg-nenashev

Looks good

// Irrelevant as the Java spec says SHA-1 must exist. Still, if this fails
// the DownloadJob will just have computedSha1 = null and that is expected
// to be handled by caller
sha256 = MessageDigest.getInstance("SHA-256");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 8, 2018

Member

is it considered as weak now? Logging message would say so if there is no SHA-512

try {
// Java spec says SHA-1 and SHA-256 exist, and SHA-512 might not, so one try/catch block should be fine
sha1 = MessageDigest.getInstance("SHA-1");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 8, 2018

Member

I suggest to create a follow-up ticket to disable weak algorithms (e.g. via Security settings or system property)

@@ -1107,12 +1108,15 @@ public void postValidate(DownloadJob job, File src) throws IOException {
*/
public File download(DownloadJob job, URL src) throws IOException {
MessageDigest sha1 = null;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 8, 2018

Member

having one digest field and ENUM for type would simplify the logic below

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jun 9, 2018

I forgot to adapt the tool installer metadata as well, so on-holding this while Jenkins would show messages like this:

INFO: JSON data source 'downloadable 'hudson.tasks.Maven.MavenInstaller'' does not provide a SHA-512 content checksum or signature. Looking for SHA-1.

@daniel-beck daniel-beck referenced this pull request Jun 12, 2018

Merged

Add update-center2 #729

6 of 6 tasks complete

@daniel-beck daniel-beck removed the on-hold label Jun 26, 2018

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jun 26, 2018

Un-onholding this, after releasing update-center2-2.0 and using it in jenkins-infra/crawler#77 tools metadata include the additional checksums.

Haven't attempted this PR build yet, but I expect this to resolve the problem from the previous comment.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jun 26, 2018

Jun 26, 2018 4:18:59 PM hudson.model.UpdateSite updateData
INFO: Obtained the latest update center data file for UpdateSource default
Jun 26, 2018 4:19:00 PM hudson.model.DownloadService$Downloadable load
INFO: Obtained the updated data file for hudson.tasks.Maven.MavenInstaller
Jun 26, 2018 4:19:00 PM hudson.model.DownloadService$Downloadable load
INFO: Obtained the updated data file for hudson.tasks.Ant.AntInstaller
Jun 26, 2018 4:19:01 PM hudson.model.DownloadService$Downloadable load
INFO: Obtained the updated data file for hudson.plugins.gradle.GradleInstaller
Jun 26, 2018 4:19:02 PM hudson.model.DownloadService$Downloadable load
INFO: Obtained the updated data file for hudson.tools.JDKInstaller

😃

@daniel-beck daniel-beck merged commit a2b24b3 into jenkinsci:master Jun 29, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@daniel-beck daniel-beck referenced this pull request Jul 11, 2018

Merged

Accept SHA-256 without complaint in logs #3546

1 of 1 task complete
@varyvol

This comment has been minimized.

Contributor

varyvol commented Aug 23, 2018

@daniel-beck please note the behavior for the 'verifyChecksums' method has changed, since no error was thrown previously if no SHA-1 was provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment