search

jshttp / etag

Create simple HTTP ETags
MIT License
256 stars 32 forks source link

Issue 17 - Use of md5 is not FIPS compliant. Provide ability to custo… #18

Closed agenaille closed 8 years ago

agenaille commented 8 years ago

The minimum supported hash algorithm that is FIPS compliant is sha1. Realizing there are performance concerns with using sha1, i've provided a way to customize the hashing algorithm per node environment. This way, the change is transparent and does not affect any users of etag, and any team needing sha can either add code to set the NODE_ETAG_HASH_ALGORITHM algorithm, or just set it on the machines running their application.

This change does not affect the current unit tests, as they will just use md5 by default. Also, I felt it was not necessary to add any SHA tests, as it will just result in my adding the same tests and changing the hashed values. The algorithm customization will not affect functionality of the etag module.

dougwilson commented 8 years ago

I have a good answer for you on the issue, but need to actually get to a computer to write it up. As for this change, there are a few issues:

  1. You added no tests
  2. You added no documentation
  3. There is no error handling for when that environment variable contains an invalid value.
  4. Do not ever bump people's versions, as it is pretty rude to dictate what version this would be published as. It is a new feature, so your suggested bump does not even comply with semver.

I suggest closing this PR, especially from what I will type up later which will solve your issue without any changes to this or express.

agenaille commented 8 years ago

Thanks @dougwilson for the pointers. I've never contributed to any modules, so I do appreciate the feedback. I definitely was not trying to be rude, and I'm sorry. I'll be very interested to hear your suggested solution to this :)