Skip to content

Commit

Permalink
Rack::ETag now supports SHA-1 and SHA-2 in addition to MD5
Browse files Browse the repository at this point in the history
  • Loading branch information
James Rosen committed Feb 6, 2010
1 parent 975d1e8 commit 5f2aa98
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 9 deletions.
49 changes: 45 additions & 4 deletions lib/rack/contrib/etag.rb
@@ -1,20 +1,61 @@
require 'digest/md5'

module Rack
# Automatically sets the ETag header on all String bodies
# if none is set.
#
# By default, uses an MD5 hash to generate the ETag.
#
# @param [#call] app the underlying Rack application. Required.
# @param [Symbol] digest the digest to use. Optional. Options are [:md5, :sha1, :sha256, :sha384, :sha512].
class ETag
def initialize(app)
def initialize(app, digest = :md5)
@app = app
load_digest(digest)
@digest_method = self.method("#{digest}_hash")
end

def call(env)
status, headers, body = @app.call(env)

if !headers.has_key?('ETag') && body.is_a?(String)
headers['ETag'] = %("#{Digest::MD5.hexdigest(body)}")
headers['ETag'] = %("#{@digest_method.call(body)}")
end

[status, headers, body]
end

private

def load_digest(digest)
case digest
when :md5
require 'digest/md5'
when :sha1
require 'digest/sha1'
when :sha256, :sha384, :sha512
require 'digest/sha2'
else
raise ArgumentError.new("Digest #{digest} is not supported.")
end
end

def md5_hash(body)
Digest::MD5.hexdigest(body)
end

def sha1_hash(body)
Digest::SHA1.hexdigest(body)
end

def sha256_hash(body)
Digest::SHA2.hexdigest(body)
end

def sha384_hash(body)
(Digest::SHA2.new(384) << body).to_s
end

def sha512_hash(body)
(Digest::SHA2.new(512) << body).to_s
end
end
end
36 changes: 31 additions & 5 deletions test/spec_rack_etag.rb
@@ -1,12 +1,38 @@
require 'test/spec'
require 'rack/mock'
require 'rack/contrib/etag'
require 'digest/md5'
require 'digest/sha1'
require 'digest/sha2'

context "Rack::ETag" do
specify "sets ETag if none is set" do
app = lambda { |env| [200, {'Content-Type' => 'text/plain'}, "Hello, World!"] }
response = Rack::ETag.new(app).call({})
response[1]['ETag'].should.equal "\"65a8e27d8879283831b664bd8b7f0ad4\""

body = 'Hello, World!'

context('if no ETag is set on a String body') do
before(:each) do
@app = lambda { |env| [200, {'Content-Type' => 'text/plain'}, body] }
end
specify 'sets ETag' do
response = Rack::ETag.new(@app).call({})
response[1]['ETag'].should.equal "\"#{Digest::MD5.hexdigest(body)}\""
end
specify 'uses SHA-1 if specified' do
response = Rack::ETag.new(@app, :sha1).call({})
response[1]['ETag'].should.equal "\"#{Digest::SHA1.hexdigest(body)}\""
end
specify 'uses SHA-256 if specified' do
response = Rack::ETag.new(@app, :sha256).call({})
response[1]['ETag'].should.equal "\"#{Digest::SHA2.hexdigest(body)}\""
end
specify 'uses SHA-384 if specified' do
response = Rack::ETag.new(@app, :sha384).call({})
response[1]['ETag'].should.equal "\"#{(Digest::SHA2.new(384) << body).to_s}\""
end
specify 'uses SHA-512 if specified' do
response = Rack::ETag.new(@app, :sha512).call({})
response[1]['ETag'].should.equal "\"#{(Digest::SHA2.new(512) << body).to_s}\""
end
end

specify "does not change ETag if it is already set" do
Expand All @@ -15,7 +41,7 @@
response[1]['ETag'].should.equal "\"abc\""
end

specify "does not set ETag if steaming body" do
specify "does not set ETag if streaming body" do
app = lambda { |env| [200, {'Content-Type' => 'text/plain'}, ["Hello", "World"]] }
response = Rack::ETag.new(app).call({})
response[1]['ETag'].should.equal nil
Expand Down

8 comments on commit 5f2aa98

@rtomayko
Copy link

Choose a reason for hiding this comment

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

I applied this in rack-contrib. I believe ETag was taken into rack core, though, so we might need to straighten that out.

@josh
Copy link

@josh josh commented on 5f2aa98 Feb 8, 2010

Choose a reason for hiding this comment

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

I'm -1 on using other hashing algorithms for Etags. There is no security issue.

ETag is so simple (3 really lines), I think it makes more sense to have a SHA1Etag middleware instead of burying the functionality under additional options.

@rtomayko
Copy link

Choose a reason for hiding this comment

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

Aren't there performance advantages to using SHA1/SHA2? I know they're much less likely to collide than etag.

I do like the idea of making this a separate middleware class, though. Or, the ETag middleware should just pick the best one and use it.

@rtomayko
Copy link

Choose a reason for hiding this comment

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

s/to collide than etag/to collide than md5/

@josh
Copy link

@josh josh commented on 5f2aa98 Feb 8, 2010

Choose a reason for hiding this comment

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

Right, we really care about speed here. If SHA1/SHA2 are faster than MD5 lets just make that the default.

@rtomayko
Copy link

Choose a reason for hiding this comment

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

I'll see what the actual difference is. Also, I was taking a shower and it occurred to me that collisions don't matter at all because it's just a validator, unless two different pieces of content were to hash to the same value for the same resource, which should be basically impossible even with md5.

@rtomayko
Copy link

Choose a reason for hiding this comment

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

Here's a performance benchmark of Ruby's crc32, md5, and sha1:

             user     system      total        real
base:    0.020000   0.000000   0.020000 (  0.018913)
crc32:   0.090000   0.010000   0.100000 (  0.096223)
md5:    0.350000   0.000000   0.350000 (  0.420743)
sha1:     0.390000   0.010000   0.400000 (  0.421582)

So it doesn't matter, unless we want to switch to crc32 I suppose.

I'm going to revert back to md5 for now. @jamesarosen: I think it's easy enough to bust out a separate middleware should people want to use SHA1.

@benschwarz
Copy link

Choose a reason for hiding this comment

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

It isn't totally applicable to rack middleware and dynamic content, but its worth reading how the hash is created for apache

Also, this change is useless - As Tomayko mentioned its simply pretty much impossible.

Please sign in to comment.