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

We should not be using MD5 (or SHA1 if possible) in any new code/spec #1

Closed
pwolanin opened this issue Apr 23, 2015 · 4 comments
Closed

Comments

@pwolanin
Copy link
Contributor

This spec looks flawed since it's using MD5 as the body hash.

Any new spec should use SHA2 such as SHA256 or SHA512

AWS is probably using the content-md5 since it's the only standard http hash header, but there is not a good reason to mimic this, especially if you are not actually sending that HTTP header. IF you want to send the header, I'd also use a SHA2 hash of the body.

In addition, the algorithm spec is unclear - it suggests hash not HMAC. This should be HMAC-SHA-256 or HMAC-SHA-512

Finally, there is no nonce value specified in the spec.

@cpliakas cpliakas added this to the 2.0.0 milestone Apr 30, 2015
@cpliakas
Copy link
Contributor

Thanks for the feedback.

Re: the SHA2. I agree, @mimrock and @baliame provided similar feedback.

Re: the spec not being clear, I am all for improving this. It is unclear to me specifically where your feedback could be applied, so any details or a pull request would be appreciated.

Re: The nonce, given that a nonce isn't required o necessary for all implementations of this algorithm, it is expected to be in a custom header such as x-acquia-nonce. I don't see a good reason to enforce a nonce, but I think we should document an opinion and guide people towards understanding whee this might be useful.

This 1.0.0 version of the spec is actively being used both inside and outside of Acquia, so the changes to the hashing algorithm should be reflected in a 2.0.x version. I created a branch at https://github.com/acquia/http-hmac-spec/tree/2.0 where any pull requests or changes can be made against.

@pwolanin
Copy link
Contributor Author

I would say every implementation must have a nonce even if the back-end doesn't currently track it.

The document as-is doesn't not constitute a spec in my mind since it's way to vague about the specific algorithms to use.

If we want to have spec, it should be very specific so it has value in being able to re-use code.

Also - there is nothing here about a HMAC of the response body. I think that symmetry is important (especially if SSL is not 100% required). acquiasearch module, and the Cloud master API certainly do that. acquia agent used to, not sure where things stand now - nspi weakened the algorithm in some ways.

@cpliakas
Copy link
Contributor

There are a few meaty issues here, so I am going to close this thread in favor or separate ones. The topics I see are below:

  • Use a SHA2 hash function
  • Enforce a nonce
  • HTTP response definition

Regarding the vagueness of the spec, I think we need to specifics in order to move forward, please feel free to post separate issues as those specifics are identified.

@cpliakas
Copy link
Contributor

Closing as dup of #3, #4, and #5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants