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

OAuth 1.0a signature methods: RSA-SHA256, RSA-SHA512 and HMAC-SHA512 #723

Merged
merged 16 commits into from Jun 3, 2020

Conversation

hoylen
Copy link
Contributor

@hoylen hoylen commented Mar 26, 2020

Implemented new OAuth 1.0a signature methods that work exactly like RSA-SHA1, but replaces the deprecated SHA-1 algorithm with the more stronger SHA-256 or SHA-512 algorithm. For completeness, also implemented a HMAC-based signature method that also replaces SHA-1 with SHA-512.

This pull request addresses #722.

Overview of the changes

Implementation

The code in oauthlib/oauth1/rfc5849/signatures.py was reorganised so all of the HMAC-based signing functions calls a common _sign_hmac function with the hash function to use as a parameter. Similarly, all of the RSA-based signing functions calls a common _sign_rsa function. This is better than having lots of duplicated code (with only has one small difference between them) which would be harder to understand and maintain.

This has resulted in some functions becoming deprecated. The old code implemented signing with a pair of functions. For example, sign_rsa_sha1_with_client and sign_rsa_sha1. The first calls the second. Now with a common _sign_rsa function, the first directly calls the new common function - so the second function has been deprecated.

Tests

The code in the unit test, tests/oauth1/rfc5849/test_signatures.py, has been substantially rewritten to be more clear and complete. It now tests the signature verification functions (it didn't before). It also achieves complete code coverage for signatures.py, with the exception of several functions which are now considered deprecated.

Documentation

There are some updates to the feature matrix document and the installation instructions document. The installation needed to be corrected to ensure RSA support is properly installed, if RSA-based signature methods are needed.

Some other files were updated so that the documentation built without errors. However, most of the changed are limited to oauthlib/oauth1/rfc5849/signatures.py, files that imports and uses it, and the unit tests for it.

Copy link
Collaborator

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

The implementation itself is fine but I do have some minor issues and questions I need answered before I merge this.

I'm no expert on OAuth 1 so this PR will also require a review from someone who does.

docs/feature_matrix.rst Outdated Show resolved Hide resolved
oauthlib/oauth1/__init__.py Outdated Show resolved Hide resolved
oauthlib/oauth1/rfc5849/endpoints/base.py Show resolved Hide resolved
oauthlib/oauth1/rfc5849/signature.py Show resolved Hide resolved
oauthlib/oauth1/rfc5849/signature.py Show resolved Hide resolved
oauthlib/oauth1/rfc5849/signature.py Show resolved Hide resolved
oauthlib/oauth1/rfc5849/signature.py Show resolved Hide resolved
oauthlib/oauth1/rfc5849/signature.py Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@hoylen hoylen requested a review from thedrow April 18, 2020 00:47
@hoylen hoylen marked this pull request as draft May 3, 2020 12:02
@hoylen hoylen marked this pull request as ready for review May 4, 2020 00:42
@hoylen
Copy link
Contributor Author

hoylen commented May 4, 2020

@thedrow I have addressed the review comments you made on 2020-03-29, but this PR still claims there are "Changes requested" (in big bold red text). This seems to be a bug/feature in GitHub, and you'll need to do something to move the PR forward. Thanks.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you fix the minor merge conflict?

@hoylen hoylen marked this pull request as draft May 9, 2020 10:31
@hoylen hoylen marked this pull request as ready for review May 9, 2020 10:32
@hoylen
Copy link
Contributor Author

hoylen commented May 9, 2020

@auvipy Merge conflict fixed. Thanks.

The dependency in master on pyjwt had been bumped up from 1.6.0 to 1.7.1. Change merged into PR.

@hoylen
Copy link
Contributor Author

hoylen commented May 18, 2020

@auvipy I have addressed the review comments you made on 2020-04-09, but this PR still claims there are "Changes requested" (in big bold red text). This seems to be a bug/feature in GitHub, and you'll need to do something to move the PR forward. Thanks.

docs/feature_matrix.rst Outdated Show resolved Hide resolved
Co-authored-by: Omer Katz <omer.drow@gmail.com>
@hoylen hoylen requested a review from thedrow May 29, 2020 07:30
@thedrow
Copy link
Collaborator

thedrow commented Jun 2, 2020

I'm going to wait for another day to see if someone else would like to review this PR.
If not, I'll merge it.

@hoylen
Copy link
Contributor Author

hoylen commented Jun 2, 2020

Thanks Omer. Much appreciated.

@auvipy auvipy merged commit bda81b3 into oauthlib:master Jun 3, 2020
@JonathanHuot JonathanHuot added this to the 3.1.1 milestone Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants