-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix private decoding bug #61
Conversation
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.
I tested these changes, and unfortunately decoding encrypted keys is still broken :(
EDIT: well I guess you didn't port every change from the other pr haha
libsignify/Cargo.toml
Outdated
rand_core = { version = "0.5", default-features = false } | ||
bcrypt-pbkdf = { version = "0.10", default-features = false } | ||
base64ct = { version = "1.6", default-features = false, features = ["alloc"] } | ||
ed25519-dalek = { version = "2", default-features = false, features = ["alloc", "fast", "rand_core"] } |
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.
good call! GHSA-w5vr-6qhr-36cc
I don't quite know how to reproduce your issue yet, so no deep dive has been attempted. Is therea way you could create an encrypted key that reproduces the issue which can be uploaded here for testing?
@sug0 Apologies if that wasn't clear. I had posted on a specific thread in your PR as a scope attempt. |
@BlackHoleFox of course. clone https://github.com/sug0/git-signify and checkout the branch this util signs arbitrary git objects. let's say we want to sign
even after you input the right password, you'll never be able to decrypt the private key. if you switch back to |
40ee419
to
03fc2e0
Compare
Fixes a bug where the decode implementation of
PrivateKey
would attempt to validate the full key matched itself even when it was encrypted. This is obviously wrong so don't do that. Regression test included that fails when the fix is removed.Depends on #60Bug originally reported in #59 (comment)