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

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

Closed
wants to merge 2 commits into from

Conversation

agenaille
Copy link

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
Copy link
Contributor

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
Copy link
Author

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 :)

@agenaille agenaille closed this Jul 1, 2016
@agenaille agenaille deleted the agenaille/issue/17 branch July 1, 2016 17:15
@dougwilson dougwilson self-assigned this Jul 2, 2016
dougwilson pushed a commit that referenced this pull request Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants