Patch #25240

Use SHA256 for attachment digest computation

Added by Jens Krämer about 1 month ago. Updated 21 days ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Attachments
Target version:3.4.0

Description

As discussed in #25215 we should change the digest method used for attachment checksums to something stronger. The attached patches

  • widen the attachments.digest column to 64 chars
  • change the digest used for new attachments to SHA256
  • provide a rake task for upgrading the digest value of existing attachments

I also changed the label of the 'MD5' column header in the files list to 'Checksum'.

0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch Magnifier (2.62 KB) Jens Krämer, 2017-03-02 04:27

0001-changes-the-digest-used-for-attachments-to-SHA256.patch Magnifier (5.49 KB) Jens Krämer, 2017-03-02 04:27

0003-change-MD5-table-header-to-Checksum.patch Magnifier (1.28 KB) Jens Krämer, 2017-03-02 04:27

0002-adds-a-rake-task-to-convert-the-digests-of-existing-.patch Magnifier - updated version of patch 2 that doesn't error on missing files (2.66 KB) Jens Krämer, 2017-03-02 07:16

add-algorithm-name.png (65.3 KB) Go MAEDA, 2017-03-02 13:45

0003-change-MD5-table-header-to-Checksum.patch Magnifier - updated version of patch 3 including the display of digest type for each file (2.43 KB) Jens Krämer, 2017-03-12 00:56


Related issues

Blocks Redmine - Patch #25215: re-use existing identical disk files for new attachments Closed

Associated revisions

Revision 16454
Added by Jean-Philippe Lang 21 days ago

Changes the digest used for attachments to SHA256 (#25240).

Patch by Jens Kraemer.

Revision 16455
Added by Jean-Philippe Lang 21 days ago

Adds a rake task to update attachments digests to SHA256 (#25240).

Patch by Jens Krämer.

Revision 16456
Added by Jean-Philippe Lang 21 days ago

Change MD5 table header to Checksum (#25240).

Patch by Jens Krämer.

Revision 16457
Added by Jean-Philippe Lang 21 days ago

Adds :field_digest string to locales (#25240).

History

#1 Updated by Go MAEDA about 1 month ago

  • Blocks Patch #25215: re-use existing identical disk files for new attachments added

#2 Updated by Go MAEDA about 1 month ago

Thanks for the patch.
But I encountered the following error while running "rake redmine:attachments:update_digest_to_sha256" on my test environment.

rake aborted!
Errno::ENOENT: No such file or directory @ rb_sysopen - /Users/maeda/redmines/redmine-trunk/files/2006/07/060719210727_error281.txt

I think that Attachment#update_digest_to_sha256! should simply ignore the record if the corresponding file for the record is not exists.

#3 Updated by Jens Krämer about 1 month ago

Yes. Here's the updated patch no. 2 with the readable? check added.

#4 Updated by Go MAEDA about 1 month ago

Jens Krämer wrote:

Yes. Here's the updated patch no. 2 with the readable? check added.

Thanks, now it works fine for me.

I have a suggestion. What about adding "MD5: " or "SHA256: " before hash values in app/views/files/index.html.erb? In the current implementation, it is a little bit difficult for users to know what hash algorithm is used to calculate the checksum.

diff --git a/app/views/files/index.html.erb b/app/views/files/index.html.erb
index 05fe37a..f111744 100644
--- a/app/views/files/index.html.erb
+++ b/app/views/files/index.html.erb
@@ -31,7 +31,7 @@
     <td class="created_on"><%= format_time(file.created_on) %></td>
     <td class="filesize"><%= number_to_human_size(file.filesize) %></td>
     <td class="downloads"><%= file.downloads %></td>
-    <td class="digest"><%= file.digest %></td>
+    <td class="digest"><%= file.digest.size < 64 ? "MD5" : "SHA256" %>: <%= file.digest %></td>
     <td class="buttons">
     <%= link_to(image_tag('delete.png'), attachment_path(file),
                                          :data => {:confirm => l(:text_are_you_sure)}, :method => :delete) if delete_allowed %>

#5 Updated by Jens Krämer about 1 month ago

Yes, that makes sense. The check for length of the digest to find out which it is isn't particularly nice (I know I did the same in my query for finding the attachments to upgrade) but I'm still not sure adding the hashing algorithm as a field to the Attachment model is any better. Maybe we could have a method named Attachment#digest_type to at least clean up the view?

#6 Updated by Go MAEDA about 1 month ago

Jens Krämer wrote:

Maybe we could have a method named Attachment#digest_type to at least clean up the view?

Yes, absolutely agree.

#7 Updated by Jean-Philippe Lang about 1 month ago

  • Target version set to 3.4.0

#8 Updated by Jens Krämer about 1 month ago

here's the updated patch 3 showing the digest used for each file in the files list

#9 Updated by Jean-Philippe Lang 21 days ago

  • Status changed from New to Closed
  • Assignee set to Jean-Philippe Lang

Patches are committed, thanks Jens. I've made a few changes to the patches and changed the fixture used in the test to a binary file (possible failure due to \r\n EOLs).

Also available in: Atom PDF