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

Isolate crypto stuff and avoid dealing with leaking SecretKey #365

Merged
merged 4 commits into from
Jul 7, 2020

Conversation

tomusdrw
Copy link
Owner

@tomusdrw tomusdrw commented Jul 6, 2020

Follow up on #358 (review)

This PR re-organizes the code using crypto stuff into signing.rs module and hides secp256k1 details behind the Sign trait.

The purpose of this is to avoid handling SecretKey and needing to zeroize or protect it at any point in time, rather this responsibility is passed to the users of the library.
Secondly this should make it easier to switch to a different (for instance no-std compatible) version of secp256k1 (like libsecp256k1) in the future since secp256k1 is not part of the public API any more.

This might be a little bit more cumbersome to use, we can possibly provide examples if that becomes an issue.

CC @That3Percent @cheme :)

Copy link
Contributor

@That3Percent That3Percent left a comment

Choose a reason for hiding this comment

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

Overall I like this approach. I think the use of lifetimes here effectively side-steps the entire issue of requiring rust-web3 to manage zeroization.

@tomusdrw
Copy link
Owner Author

tomusdrw commented Jul 7, 2020

Thanks for reviewing this PR, merging then.

@tomusdrw tomusdrw merged commit cd483f2 into master Jul 7, 2020
@tomusdrw tomusdrw deleted the td-signing branch July 7, 2020 19:26
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.

2 participants