-
Notifications
You must be signed in to change notification settings - Fork 31
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 TryFrom from &[u8] and Vec<u8> to PrivateKeyDer #40
Conversation
I'm using something that requires std. Need to adjust 😮💨 |
Your pull request seems much better than mine, but I am not an expert on the subject. Why couldn't the tests be moved with the other tests if they need std? |
I think the issue isn't in the tests, it's the way I'm trying to build a pattern to match for the There isn't a nice way to make a compile-time const that combines two const slice fragments without defining a macro (as far as I can tell). I'll have to rework the approach slightly. |
6e19a65
to
5bd8a95
Compare
I gave up on pulling out the VERSION_ONE and VERSION_TWO slices and inlined their values into the three hints. It's a little less nice but I think making a macro would be overkill. |
5bd8a95
to
d54d883
Compare
FWIW I ran the fuzzer "a while" (~30m) on this branch, and the original commit from #39 without any findings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
In terms of commit history, I would prefer not to keep @Alvenix' original commit separate from the other commits that change the code/tests, and instead squash them together (and use Co-authored-by or whatever the tag is to clarify origination) -- keeping the fuzzing commit separate is fine, of course.
d54d883
to
19558d2
Compare
That sounds good to me. I've pushed an update that folds the revisions and the test updates into the original commit, maintaining the authorship from #39. I left the fuzzing commits separate. |
This commits adds two new `TryFrom` implementations to `PrivateKeyDer`. These implementations support heuristically constructing an appropriate `PrivateKeyDer` based on DER encoded input that may be a PKCS8, PKCS1, or SEC1 input.
19558d2
to
4352292
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
This branch is an alternative to #39 that takes the base commit and reworks it slightly to:
Correct the boundary for the short form length check.(I think we've reasoned out this was a no-op change)Along the way I also:
cargo fuzz
to initialize a fuzzing crate, and a target for fuzzing this new API surface.I thought that it would be easier to open this PR rather than deliver a large volume of review feedback. Many thanks to @Alvenix for the original implementation.