crypto: Rederive pubkey for encode/decode #6989

joyqvq · 2022-12-22T04:25:27Z

What

This changes the serialization of a SuiKeyPair from base64 encoded flag || pubkey || privkey to flag || privkey.

Why

This is to never trust the pubkey bytes inputs when but to always derive it directly from privkey when bootstrapping from read/write from files.

Notes for breaking change

If you have existing keys in sui.keystore, add new keys will throw error because the old serialization cannot be parsed anymore.

target/debug/sui keytool generate ed25519
Invalid Keypair file InvalidInput "~/.sui/sui_config/sui.keystore"

How to Fix

If you are ok with wiping the keys, rm ~/.sui/sui_config/sui.keystore and then sui client new-address should work again and new keys are formatted with only private key.

If you would like to keep the existing keys, you will need to convert them manually. Here is a manual python script:

import base64
def convert(s): 
    bytes = base64.b64decode(s)
    print(f"{len(bytes)} orginal: {s}")
    if len(bytes) == 65 and bytes[0] == 0:
        print(f"ed25519 key, converted: {base64.b64encode(bytes[:33])}")
    elif len(bytes) == 66 and bytes[0] == 1:
        print(f"secpk1 key, converted: {base64.b64encode(bytes[:33])}")
    elif len(bytes) == 66 and bytes[0] == 2:
        print(f"secpr1 key, converted: {base64.b64encode(bytes[:33])}")
    elif len(bytes) == 128:
        print(f"bls key, converted: {base64.b64encode(bytes[:32])}")
    elif len(bytes) == 64:
        print(f"no flag ed25519, converted: {base64.b64encode(bytes[:32])}")
convert("$INSERT_KEY_HERE")

To check if the conversion works, all keys should be of length 33 regardless of the the signature scheme flag.

cat ~/.sui/sui_config/sui.keystore
[
  "ATWhXWl/zlrENWnc2AIbjChwAB5FGMsBp3RBcKUCUe90",
  "AOhFYQNpqLgJyYmfg975nwQmQ8YCPXCnTw/l8ZjIBEqy",
  "ArYljv5d6NQWahm8/oDF7xa+E5f6XpyMdDxTfPuxhLzD"
]

Test

target/debug/sui client new-address ed25519
Created new keypair for address with scheme ED25519: [0x25142b365037715dae989375ac2b74b2517edd62]
Secret Recovery Phrase : [original rent brick leader middle target cheap rather choose elephant system abstract despair math pudding music garment young syrup foam core try good market]

target/debug/sui client new-address secp256k1
Created new keypair for address with scheme Secp256k1: [0x049045049bbcd071609e0caf88b0542ed791aa3e]
Secret Recovery Phrase : [enroll field short hurry unique morning glad can bridge immune sorry blood badge collect gesture math gallery rifle lens surge nasty hurt couch delay]

target/debug/sui client new-address secp256r1 
Created new keypair for address with scheme Secp256r1: [0xa76b0f7b5e19b86c768486b1ffb757b9952f8d61]
Secret Recovery Phrase : [witness stuff sorry try chuckle cliff moon share purchase person runway online usual exit lunar same limit act walk spread abandon cradle basic coin]

target/debug/sui keytool list
                Sui Address                 |              Public Key (Base64)              | Scheme
----------------------------------------------------------------------------------------------------
 0x049045049bbcd071609e0caf88b0542ed791aa3e | AQKxLzhpin3HwLzcjscgPGzrcdRvCX6vZT0xTH5X6PDKRA== | secp256k1
 0x25142b365037715dae989375ac2b74b2517edd62 | AOBT/yUKPQuCNh8eiLaGyyIoUTa3HbWpxwYxM/1dNdoV  | ed25519
 0xa76b0f7b5e19b86c768486b1ffb757b9952f8d61 | AgNy9UEXhU/K1aro4vY/QxssR6pfw9K6NmNLsUDi0VydSg== | secp256r1

vercel · 2022-12-22T04:25:30Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
explorer	🔄 Building (Inspect)			Jan 5, 2023 at 3:43AM (UTC)

2 Ignored Deployments

Name	Status	Preview	Comments	Updated
explorer-storybook	⬜️ Ignored (Inspect)			Jan 5, 2023 at 3:43AM (UTC)
wallet-adapter	⬜️ Ignored (Inspect)			Jan 5, 2023 at 3:43AM (UTC)

kchalkias · 2022-12-22T07:23:17Z

crates/sui-types/src/crypto.rs

@@ -136,61 +136,56 @@ impl FromStr for SuiKeyPair {
 }

 impl EncodeDecodeBase64 for SuiKeyPair {
+    /// Encode a SuiKeyPair as `flag || pubkey || privkey` in Base64. The pubkey bytesare derived directly from privkey.


bytes _ are

Also, do we want for the base64 to have flag || pubkey || privkey or flag || privkey and then rederive the pub key as we do in the rest of serializers?

I understand base64 is mostly for external UX, so I'm happy with both decisions, but we should document it properly as we received some feedback re different serializers or behavior in the past.

IMO its more consistent to keep it flag || pubkey || privkey since the struct itself is called a keypair. As long as in our system, we read a keypair and ignores the pubkey part but to only read the privkey bytes to derive the keypair, i think its safe and less confusing.

+1 lemme document this more explicitly in comments

It depends on why we need such a serializer (cause encoding is usually used for (de)serialization). Assuming each receiver of the encoded format wants to deserialize the key pair into internal structs, then we should promote secure deserialization by rederiving the pub part for better hygiene, especially sensitive object types.

Re the argument "since the struct is called keypair": there are many types across programming languages that their serialized bytes are compressed for a reason (security or bandwidth). I'd only buy the use case of compatibility with other (ie JS) libs to allow for quick conversion without rederivation, but still, IMO it's better to provide a tool to rederive Vs encoding it.

kchalkias

Is this work affected by the recent changes in fastcrypto + conflicts re serialization api changes?

joyqvq · 2023-01-02T07:00:11Z

Is this work affected by the recent changes in fastcrypto + conflicts re serialization api changes?

rebased.

crates/sui-core/tests/staged/sui.yaml

benr-ml · 2023-01-03T15:28:49Z

crates/sui-types/src/crypto.rs

            }
        }
        Base64::encode(&bytes[..])
    }

+    /// Decode a SuiKeyPair from Base64 encoded `flag || privkey || pubkey` in Base64. Use the privkey bytes only to derive the pubkey and the full keypair instead of using the pubkey bytes.
    fn decode_base64(value: &str) -> Result<Self, eyre::Report> {
        let bytes = Base64::decode(value).map_err(|e| eyre::eyre!("{}", e.to_string()))?;
        match bytes.first() {
            Some(x) => {


wouldn't something like
match bytes.first() { Some(Ed25519SuiSignature::SCHEME.flag()) => ....

work here?

simplified this part a bit

joyqvq · 2023-01-04T09:06:42Z

crates/sui-config/tests/snapshots/snapshot_tests__network_config_snapshot_matches.snap

    worker-key-pair:
-      value: AB8qeQGoQuTTYjvGHOHBcX0udo4P1y34NBr1ZhW5FvA4fsz863qJR38mPjuvloaZBE4vbibFPgrwQXUa+OGTTNM=
+      value: AH7M/Ot6iUd/Jj47r5aGmQROL24mxT4K8EF1Gvjhk0zT


this is used for swarm test only

This reverts commit 28c4823.

joyqvq force-pushed the rederivekp1 branch from a7646c4 to d656b08 Compare December 22, 2022 06:45

joyqvq marked this pull request as ready for review December 22, 2022 07:18

joyqvq requested a review from huitseeker as a code owner December 22, 2022 07:18

joyqvq requested a review from kchalkias December 22, 2022 07:18

kchalkias reviewed Dec 22, 2022

View reviewed changes

joyqvq force-pushed the rederivekp1 branch from d656b08 to a553fc9 Compare December 27, 2022 10:37

joyqvq requested a review from kchalkias December 27, 2022 10:37

joyqvq added the breaking change label Dec 28, 2022

joyqvq force-pushed the rederivekp1 branch from a553fc9 to 92d0e05 Compare December 28, 2022 05:25

joyqvq requested a review from benr-ml December 28, 2022 05:28

joyqvq force-pushed the rederivekp1 branch 2 times, most recently from 1b7906b to 8c4a678 Compare December 28, 2022 09:26

kchalkias reviewed Dec 30, 2022

View reviewed changes

joyqvq force-pushed the rederivekp1 branch from 8c4a678 to 3602f16 Compare January 2, 2023 06:59

joyqvq force-pushed the rederivekp1 branch from 3602f16 to 2cdf9c0 Compare January 3, 2023 14:41

benr-ml reviewed Jan 3, 2023

View reviewed changes

joyqvq force-pushed the rederivekp1 branch 2 times, most recently from 24664c4 to a694f43 Compare January 4, 2023 06:59

joyqvq mentioned this pull request Jan 4, 2023

crypto: Add serde for BLSAggregateSignature MystenLabs/fastcrypto#340

Closed

joyqvq force-pushed the rederivekp1 branch from a694f43 to b13983f Compare January 4, 2023 09:04

joyqvq commented Jan 4, 2023

View reviewed changes

benr-ml approved these changes Jan 4, 2023

View reviewed changes

crypto: Rederive pubkey for encode/decode

05ca6ec

joyqvq force-pushed the rederivekp1 branch from b13983f to 05ca6ec Compare January 5, 2023 03:42

joyqvq enabled auto-merge (squash) January 5, 2023 03:50

joyqvq merged commit 28c4823 into main Jan 5, 2023

joyqvq deleted the rederivekp1 branch January 5, 2023 04:02

mwtian added a commit that referenced this pull request Jan 5, 2023

Revert "crypto: Rederive pubkey for encode/decode (#6989)"

9b6f36c

This reverts commit 28c4823.

lxfind added a commit that referenced this pull request Jan 5, 2023

Revert "crypto: Rederive pubkey for encode/decode (#6989)"

ae7df41

This reverts commit 28c4823.

FrankC01 mentioned this pull request Jan 12, 2023

[maintenance] Tracking Sui defects and changes FrankC01/pysui#51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: Rederive pubkey for encode/decode #6989

crypto: Rederive pubkey for encode/decode #6989

joyqvq commented Dec 22, 2022 •

edited

Loading

vercel bot commented Dec 22, 2022 •

edited

Loading

kchalkias Dec 22, 2022

kchalkias Dec 22, 2022

kchalkias Dec 22, 2022

joyqvq Dec 23, 2022

kchalkias Dec 27, 2022

joyqvq Dec 28, 2022

kchalkias left a comment

joyqvq commented Jan 2, 2023

benr-ml Jan 3, 2023

joyqvq Jan 4, 2023

joyqvq Jan 4, 2023

crypto: Rederive pubkey for encode/decode #6989

crypto: Rederive pubkey for encode/decode #6989

Conversation

joyqvq commented Dec 22, 2022 • edited Loading

What

Why

Notes for breaking change

How to Fix

Test

vercel bot commented Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchalkias left a comment

Choose a reason for hiding this comment

joyqvq commented Jan 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyqvq commented Dec 22, 2022 •

edited

Loading

vercel bot commented Dec 22, 2022 •

edited

Loading