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 RSA signature recovery #5573

Merged
merged 8 commits into from Dec 8, 2020
Merged

Conversation

@misterzed88
Copy link
Contributor

@misterzed88 misterzed88 commented Nov 18, 2020

First round of suggested changes for adding RSA signature recovery, as discussed in issue #5495.

Loading
src/cryptography/hazmat/backends/openssl/rsa.py Outdated Show resolved Hide resolved
Loading
docs/hazmat/primitives/asymmetric/rsa.rst Outdated Show resolved Hide resolved
Loading
@alex
Copy link
Member

@alex alex commented Nov 28, 2020

Loading

@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Nov 30, 2020

Thanks for reviewing and good comments!

The current proposal does not implement the full signature data recovery feature (including DigestInfo), as mentioned in #5495. This was a small misunderstanding on my part - I now realize that you OKed that but only for the separate recovery case.

I will update the proposal with that feature, and the other things you mentioned.

Loading

@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Nov 30, 2020

I think this should have a slightly less generic name. My opening suggestion would be recover_digest_from_signature.

Yes, recover may sound a bit too ambiguous. What about recover_signature? Or, alternatively, recover_data_from_signature?

Technically, the function recovers any signature data (which may or may not be a digest), especially after I have made the planned changes where all types of data can be recovered, even without a DigestInfo block (as described in #5495).

Loading

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented Dec 1, 2020

recover_data_from_signature seems fine.

Loading

@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Dec 2, 2020

Updated the suggested recovery functionality with an option to return all of the recovered signature data, by allowing None as hash algorithm parameter.

Loading

docs/hazmat/primitives/asymmetric/rsa.rst Show resolved Hide resolved
Loading
docs/hazmat/primitives/asymmetric/rsa.rst Show resolved Hide resolved
Loading
docs/hazmat/primitives/asymmetric/rsa.rst Show resolved Hide resolved
Loading
docs/hazmat/primitives/asymmetric/rsa.rst Show resolved Hide resolved
Loading
@alex alex added this to the Thirty Third Release milestone Dec 7, 2020
alex
alex approved these changes Dec 8, 2020
@alex alex merged commit 6693d55 into pyca:master Dec 8, 2020
53 checks passed
Loading
@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Dec 8, 2020

Thanks again for reviewing and good comments regarding how to improve the documentation. I planned on doing the suggested changes this week but just noticed that you already marked them as resolved. Should I still do the changes or have you moved on without them?

Loading

@alex
Copy link
Member

@alex alex commented Dec 8, 2020

Loading

@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Dec 8, 2020

Ok, I see, thanks.

Are you sure Paul fixed the last-minute documentation changes that you proposed? (I couldn't see them included in commit 6693d55, but maybe I missed something).

Loading

@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Dec 10, 2020

The last document changes were added separately in #5614.

Loading

@misterzed88 misterzed88 deleted the sign-recover branch Dec 10, 2020
@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented Dec 10, 2020

@misterzed88 yeah, we did the changes...but didn't merge the right commit because I didn't actually commit, I just pushed a rebase. These were all docs so less critical of course, but generally we do prefer to merge them as one when the diff isn't too large. Sorry about the confusion.

Loading

@misterzed88
Copy link
Contributor Author

@misterzed88 misterzed88 commented Dec 10, 2020

Good to be reminded once in a while that we are all humans, to keep our humility. :)

Thanks for accepting the PR and for fixing the last issues in the docs.

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants