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

Accept SHA-256 without complaint in logs #3546

Merged
merged 4 commits into from Jul 12, 2018

Conversation

3 participants
@daniel-beck
Copy link
Member

daniel-beck commented Jul 11, 2018

We may need to scale down the requirement for SHA-512 checksums a bit here due to performance considerations in our infra. TBD.

Followup to #3356. Motivated by jenkins-infra/update-center2#198.

Proposed changelog entries

  • Don't log warnings when SHA-256 checksums are provided but SHA-512 are not for plugin downloads.

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.)
  • [n/a] 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

Desired reviewers

@dwnusbaum

@daniel-beck daniel-beck requested review from dwnusbaum and Wadeck Jul 11, 2018

@Wadeck

Wadeck approved these changes Jul 11, 2018

Copy link
Contributor

Wadeck left a comment

🐝

LGTM except the non-deleted comment line

nit: perhaps rename one of the two methods verifyChecksums, like the private one to verifySingleChecksum to avoid having two methods with same name but completely different signature + logic.

@@ -1888,46 +1888,40 @@ private static void throwVerificationFailure(String expected, String actual, Fil
switch (result512) {
case PASS:
// this has passed but no real reason not to check the others
break;
return;

This comment has been minimized.

@Wadeck

Wadeck Jul 11, 2018

Contributor

I think the comment just above is no longer correct

daniel-beck added some commits Jul 11, 2018

@daniel-beck daniel-beck removed the on-hold label Jul 11, 2018

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

Would it make sense to have a configuration option in Update Center Advanced Settings to enforce sha512-only?

@daniel-beck

This comment has been minimized.

Copy link
Member Author

daniel-beck commented Jul 12, 2018

Would it make sense to have a configuration option in Update Center Advanced Settings to enforce sha512-only?

I don't think that's needed yet, especially since that'd break our public update sites.

@daniel-beck daniel-beck merged commit 319ce84 into jenkinsci:master Jul 12, 2018

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.