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

Add support for HMAC-SHA256 (builds on PR#388) #498

Merged
merged 8 commits into from
Nov 14, 2017
Merged

Add support for HMAC-SHA256 (builds on PR#388) #498

merged 8 commits into from
Nov 14, 2017

Conversation

ViktorHaag
Copy link
Contributor

This pull incorporates changes from PR #388 as it appears orphaned.

I cleaned up the comments for the HMAC-SHA256 signing method, and added tests for the SHA1 and SHA256 signing methods (I verified the signature generation code against node's oauth-1.0a and crypto modules, to be satisfied that the test signature values were valid).

@ViktorHaag ViktorHaag changed the title Add support for HMAC-SHA256 Add support for HMAC-SHA256 (builds on PR#388) Oct 27, 2017
This was referenced Oct 27, 2017
The "HMAC-SHA256" signature method uses the HMAC-SHA256 signature
algorithm as defined in `RFC4634`_::

digest = HMAC-SHA256 (key, text)
Copy link
Collaborator

@lepture lepture Oct 27, 2017

Choose a reason for hiding this comment

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

There is no such thing in RFC5849.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Note that my comment didn't claim it was in RFC5849, but defined in RFC4634 (as one of the suite of accepted Secure Hash Algorithms beyond SHA-1).

I took 5849's provision of three signature methods as not exclusive. In section 3.4, it says "OAuth does not mandate a particular signature method" (my emphasis), and so it seems useful, with little harm, to add support for SHA2-family signing methods.

If the maintainers would prefer to have clients add support for more modern signing methods via extension, rather than provided in the library, I can understand that decision. I would have thought having the support in-library would at least help clients ensure consistency, but it's clearly not my decision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ViktorHaag See my comment. There is a register function to extend signature methods. The folder is named as rfc5849, things beyond this scope should not be included.

cc @thedrow @ib-lundgren

Copy link
Contributor Author

@ViktorHaag ViktorHaag Oct 27, 2017

Choose a reason for hiding this comment

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

Yes, I understand your point. I had assumed from the comments in #388, that the blocking factor here was a lack of testing and documentation, and not the idea in the first place. It was for that reason that I decided to help by "finishing" the original PR. If the maintainer group would prefer not to include it, that's fine: I'm glad there is a way to extend the package usefully.

@lepture
Copy link
Collaborator

lepture commented Oct 27, 2017

Actually there is a way to register your own signature method.
https://github.com/idan/oauthlib/blob/master/oauthlib/oauth1/rfc5849/__init__.py#L52

This HMAC-SHA256 is not a part of the specification in RFC. This patch should not be included in this repo.

@lepture lepture closed this Oct 27, 2017
@lepture lepture reopened this Oct 27, 2017
@thedrow
Copy link
Collaborator

thedrow commented Nov 14, 2017

I have given this some thought.
I don't see the harm in providing this implementation to our users.
It's a pretty common usecase.

@thedrow thedrow merged commit cfb82fe into oauthlib:master Nov 14, 2017
@thedrow
Copy link
Collaborator

thedrow commented Nov 14, 2017

Thank you @ViktorHaag @mick88
Feel free to add yourselves to the AUTHORS file.

@lepture If you strongly disagree let's chat about this over email.
We can always revert before making a new release.

@skion skion mentioned this pull request Mar 9, 2018
@skion skion added this to the 2.1.0 milestone Mar 17, 2018
@skion skion modified the milestones: 2.1.0, 3.0.0 Jul 21, 2018
@JonathanHuot JonathanHuot mentioned this pull request Dec 3, 2018
23 tasks
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.

4 participants