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

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

@alex
Copy link
Member

alex commented Nov 28, 2020 via email

@misterzed88
Copy link
Contributor Author

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.

@misterzed88
Copy link
Contributor Author

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).

@reaperhulk
Copy link
Member

recover_data_from_signature seems fine.

@misterzed88
Copy link
Contributor Author

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

@alex alex added this to the Thirty Third Release milestone Dec 7, 2020
@alex alex merged commit 6693d55 into pyca:master Dec 8, 2020
@misterzed88
Copy link
Contributor Author

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?

@alex
Copy link
Member

alex commented Dec 8, 2020 via email

@misterzed88
Copy link
Contributor Author

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).

@misterzed88
Copy link
Contributor Author

The last document changes were added separately in #5614.

@misterzed88 misterzed88 deleted the sign-recover branch December 10, 2020 02:48
@reaperhulk
Copy link
Member

@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.

@misterzed88
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

3 participants