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

sdk: enforce that Keypair::from_bytes bytes match secret-derived Pubkey #32926

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

t-nelson
Copy link
Contributor

Problem

ed25519_dalek v1 Keypair construction footgun

Summary of Changes

fail solana_sdk::signer::keypair::Keypair::from_bytes if the specified Pubkey does not match the one derived from the specified SecretKey

this will backstop our wrapper types from the overly permissive underlying api

joncinque
joncinque previously approved these changes Aug 21, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great! (Once CI passes)

brooksprumo
brooksprumo previously approved these changes Aug 21, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@t-nelson
Copy link
Contributor Author

lmao... we have illegal keypair bytes in a test 🙃

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #32926 (a10a187) into master (910b0f5) will decrease coverage by 0.1%.
Report is 6 commits behind head on master.
The diff coverage is 90.0%.

@@            Coverage Diff            @@
##           master   #32926     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         785      785             
  Lines      212391   212397      +6     
=========================================
- Hits       174204   174174     -30     
- Misses      38187    38223     +36     

@t-nelson
Copy link
Contributor Author

lmao... we have illegal keypair bytes in a test 🙃

looks like this key material has existed since we used the ring crate for ed25519. i'm guessing we botched converting the key from pkcs8 format when porting over to dalek 🤷

@t-nelson t-nelson merged commit 9d83bb2 into solana-labs:master Aug 22, 2023
@t-nelson t-nelson deleted the rka branch August 22, 2023 04:40
@t-nelson
Copy link
Contributor Author

i'm guessing we botched converting the key from pkcs8 format when porting over to dalek 🤷

confirmed. this patch should've been something like...

@@ -1,5 +1,4 @@
-        48, 83, 2, 1, 1, 48, 5, 6, 3, 43, 101, 112, 4, 34, 4, 32, 255, 101, 36, 24, 124, 23,
-        167, 21, 132, 204, 155, 5, 185, 58, 121, 75, 156, 227, 116, 193, 215, 38, 142, 22, 8,
-        14, 229, 239, 119, 93, 5, 218, 161, 35, 3, 33, 0, 36, 100, 158, 252, 33, 161, 97, 185,
-        62, 89, 99, 195, 250, 249, 187, 189, 171, 118, 241, 90, 248, 14, 68, 219, 231, 62, 157,
-        5, 142, 27, 210, 117,
+        255, 101, 36, 24, 124, 23, 167, 21, 132, 204, 155, 5, 185, 58, 121, 75, 156, 227, 116,
+        193, 215, 38, 142, 22, 8, 14, 229, 239, 119, 93, 5, 218, 36, 100, 158, 252, 33, 161, 97,
+        185, 62, 89, 99, 195, 250, 249, 187, 189, 171, 118, 241, 90, 248, 14, 68, 219, 231, 62,
+        157, 5, 142, 27, 210, 117,

original bytes break down as...

        # some pkcs8 bullshit
        48, 83, 2, 1, 1, 48, 5, 6, 3, 43, 101, 112, 4, 34, 4, 32,
        # raw private key
        255, 101, 36, 24, 124, 23, 167, 21, 132, 204, 155, 5, 185, 58, 121, 75, 156, 227, 116, 193, 215, 38, 142, 22, 8, 14, 229, 239, 119, 93, 5, 218,
        # more pkcs8 bullshit
        161, 35, 3, 33, 0,
        # raw public key
        36, 100, 158, 252, 33, 161, 97, 185, 62, 89, 99, 195, 250, 249, 187, 189, 171, 118, 241, 90, 248, 14, 68, 219, 231, 62, 157, 5, 142, 27, 210, 117,

will correct in followup

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.

3 participants