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

Being able to choose the digest algorithm #98

Merged
merged 3 commits into from
Feb 25, 2016
Merged

Conversation

fwininger
Copy link
Collaborator

Hi, could you consider that changes to be able to do HMAC-SHA256 ?
Thanks !

@kjg
Copy link
Collaborator

kjg commented Feb 10, 2016

Can you help explain the motivation behind wanting to use a different digest algorithm?

@fwininger
Copy link
Collaborator Author

I believe that force the use of SHA-1 is a functional limitation.
Give the opportunity to choose keeps the Gem's implementation in the future with more modern algorithms.

The last version of AWS api authentication use SHA-256 :
http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html

According to the Google Chromium Team :
"The SHA-1 cryptographic hash algorithm has been known to be considerably weaker than it was designed to be since at least 2005 — 9 years ago. Collision attacks against SHA-1 are too affordable for us to consider it safe for the public web PKI. We can only expect that attacks will get cheaper."
https://googleonlinesecurity.blogspot.co.uk/2014/09/gradually-sunsetting-sha-1.html

@kjg
Copy link
Collaborator

kjg commented Feb 10, 2016

I'm open to accepting something like this if we can do it in a way that make it obvious what is happening on the server and client side. What I don't want to happen is to have someone set their server up to use sha-256 and then forget to add that bit to their client (or vise versa).

Maybe there can be an additional header that specifies what hashing algorithm was used to sign the request.

@fwininger
Copy link
Collaborator Author

If you merge the pull request with Rubocop, I work to add a header like AWS, ie : AWS4-HMAC-SHA256.
http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html

@fwininger
Copy link
Collaborator Author

I have add a optional HMAC_METHOD header, to ensure that Client and Server use the same method by default.

@fwininger
Copy link
Collaborator Author

Up ?

@kjg
Copy link
Collaborator

kjg commented Feb 19, 2016

Thanks so much for working on this. We should either follow Amazon's lead there and have the "key" in the Authorization header be something like APIAuth-HMAC-SHA256

or if we're going to use a separate header, it needs to be prfixed with an X- since HMAC-METHOD isn't a standard HTTP header.

Another option is to do like OAuth 1a and have key value pairs within the content section of the Authoizaiton header and have a signature_method="SHA256" or something along those lines

@mgomes @awendt any opinions on which method above we should use to specify the signature method here?

@mgomes
Copy link
Owner

mgomes commented Feb 19, 2016

@kjg I like following Amazon's lead and avoiding a second header. What's your preference?

@kjg
Copy link
Collaborator

kjg commented Feb 19, 2016

I agree. I like the way amazon does it.

@fwininger
Copy link
Collaborator Author

Hi guys. I'm aggre to follow the new AWS V4 standard with a single Authorisation Header.

Authorization: APIAuth-HMAC-SHA256 Credential=ID, SignedHeaders=Content-MD5;Date, Signature=fe5f80f77d5fa3beca038a248ff027d0445342fe2855ddc963176630326f1024

I can implement that, as a v2 branch, because I think we will have several breaks with v1 branch.

@kjg
Copy link
Collaborator

kjg commented Feb 20, 2016

We don't necessarily need to follow everything they did. I like just
calling the 256 version APIAuth-HMAC-SHA256 and we can have an
explicit APIAuth-HMAC-SHA1
and then default to sha1 when it's just APIAuth. And the just the signature
part after that. I don't think we need to do the other key, value things.
Not just yet anyway.

On February 20, 2016 at 3:29:27 AM, florian wininger (
[email protected]) wrote:

Hi guys. I'm aggre to follow the new AWS V4 standard with a single
Authorisation Header.

Authorization: APIAuth-HMAC-SHA256
Credential=ID, SignedHeaders=Content-MD5;Date,
Signature=fe5f80f77d5fa3beca038a248ff027d0445342fe2855ddc963176630326f1024

I can implement that, as a v2 branch, because I think we will have several
breaks with v1 branch.


Reply to this email directly or view it on GitHub
#98 (comment).

@fwininger
Copy link
Collaborator Author

Ok, so with your suggestion for the first step, we use :
Autorization: APIAuth-HMAC-SHA256 access id:signature
in place of :
Autorization: APIAuth access id:signature
sounds good ?

@kjg
Copy link
Collaborator

kjg commented Feb 20, 2016

Sounds good to me! Thanks!

On February 20, 2016 at 7:50:16 AM, florian wininger (
[email protected]) wrote:

Ok, so with your suggestion for the first step, we use :
Autorization: APIAuth-HMAC-SHA256 access id:signature
in place of :
Autorization: APIAuth access id:signature
sounds good ?


Reply to this email directly or view it on GitHub
#98 (comment).

@@ -67,7 +69,7 @@ def generate_secret_key

private

AUTH_HEADER_PATTERN = /APIAuth ([^:]+):(.+)$/
AUTH_HEADER_PATTERN = /APIAuth(-HMAC-(SHA[0-9]+))? ([^:]+):(.+)$/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if it's worth it, but you could make the outer (-HMAC) part non-capturing since we're not using it for anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should we enforce only matching on SHA digests? I'd be okay with that I think but others are supported by OpenSSL::Digest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@fwininger fwininger force-pushed the master branch 2 times, most recently from f13f5f7 to 00dc7ed Compare February 22, 2016 21:32
@kjg
Copy link
Collaborator

kjg commented Feb 22, 2016

Thanks so much for all this!

I think we're there on the code. Can you please just update the README to reflect that authentic? Will detect the digest used from the Auth header. Do you think we should allow for it to still be overridden in authentic? I'm not sure if that's needed anymore, but I'm indifferent. If you think it should still be overridable in authentic? then just make it clear that it'll override what was detected in the header, and can make debugging harder or something

@fwininger fwininger force-pushed the master branch 2 times, most recently from 52983aa to e2abb84 Compare February 25, 2016 13:39
@fwininger
Copy link
Collaborator Author

Hi,

I prefer to keep the digest option in authentic to prevent downgrade attacks (like Poodle for SSLv3 vs TLS). In our case we want to support only SHA256 or newer.

I have an ApiAuth::InvalidRequestDigest error to raise a exception if the Digest in the client and the server is not the same.

@kjg
Copy link
Collaborator

kjg commented Feb 25, 2016

Ah, that makes sense. Thanks so much for this. It looks great! Merging this now.

I'm going to remove the code that allows for the canonical string without the http method, and I'll get a v2.0.0 out.

kjg added a commit that referenced this pull request Feb 25, 2016
Being able to choose the digest algorithm
@kjg kjg merged commit 4e23010 into mgomes:master Feb 25, 2016
@fwininger
Copy link
Collaborator Author

Thanks. Let me know if you need some code review or some help for others features.

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

Successfully merging this pull request may close these issues.

None yet

3 participants