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

Add serialization of account secret key #19

Merged
merged 1 commit into from Nov 13, 2023
Merged

Add serialization of account secret key #19

merged 1 commit into from Nov 13, 2023

Conversation

davidyuk
Copy link
Member

@davidyuk davidyuk commented Aug 1, 2023

JS SDK misses a fancy way to serialize an account secret key. Currently, it uses a very long hex string representing 64 bytes. Would be good to define a more efficient and distinguishable way to encode account secret keys.

This PR is supported by the Æternity Crypto Foundation

@@ -191,6 +192,7 @@ type2enc(oracle_query) -> ?BASE64;
type2enc(oracle_query_id) -> ?BASE58;
type2enc(oracle_response) -> ?BASE64;
type2enc(account_pubkey) -> ?BASE58;
type2enc(account_secret_key) -> ?BASE64;
Copy link
Member Author

Choose a reason for hiding this comment

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

I propose to use BASE64 as it is a low-level thing don't be needed to handle by users in everyday life. Though I'm fine to use BASE58.

@@ -219,6 +221,7 @@ type2pfx(oracle_query) -> <<"ov">>;
type2pfx(oracle_query_id) -> <<"oq">>;
type2pfx(oracle_response) -> <<"or">>;
type2pfx(account_pubkey) -> <<"ak">>;
type2pfx(account_secret_key) -> <<"sk">>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Using "secret" instead of "private" to have less visual overlap with "public", also this is consistent with naming in tweetnacl https://www.npmjs.com/package/tweetnacl

Copy link
Member

@dincho dincho Aug 9, 2023

Choose a reason for hiding this comment

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

I'd argue about consistency here: account_prvkey

Copy link

Choose a reason for hiding this comment

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

I agree with Dincho

Copy link
Member Author

Choose a reason for hiding this comment

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

Would prvkey be consistent with naming in this repo or the node? Or you mean in style of pubkey? Maybe to name it seckey, scrtkey, or secrkey?
http://acronymsandslang.com/abbreviation-for/SECRET.html
https://www.allacronyms.com/secret/abbreviated

Would you rename prefix also to "pk"?

Here is one more example from original NaCl docs

unsigned char pk[crypto_sign_PUBLICKEYBYTES];
unsigned char sk[crypto_sign_SECRETKEYBYTES];

https://nacl.cr.yp.to/sign.html

Copy link

Choose a reason for hiding this comment

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

I don't have hard feelings here, I agree that consistency is good, but I see Denis' point too.

@@ -275,6 +279,7 @@ byte_size_for_type(oracle_query) -> not_applicable;
byte_size_for_type(oracle_query_id) -> 32;
byte_size_for_type(oracle_response) -> not_applicable;
byte_size_for_type(account_pubkey) -> 32;
byte_size_for_type(account_secret_key) -> 32;
Copy link
Member Author

Choose a reason for hiding this comment

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

tweetnacl operates with 64-byte secret keys though half of it is a public key, we can safely drop it to reduce key size. Not sure if would it affect performance or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

While the public key can always be derived from the seed, the precomputation saves a significant amount of CPU cycles when signing.

https://doc.libsodium.org/public-key_cryptography/public-key_signatures#extracting-the-seed-and-the-public-key-from-the-secret-key
https://github.com/aeternity/protocol/blob/master/consensus/README.md#keys

in the most cases in can be computed once by app and cached 🤷‍♀️ 32 bytes would be easier to read in QR codes (if used in invite links)

@hanssv
Copy link
Member

hanssv commented Aug 28, 2023

No opinion about the format - just a reminder from the past. It is NOT an omission that there is no way to serialize a secret/private key... You should never be tempted to do it, except for possibly testing, you should not serialize and store a private key.

@davidyuk
Copy link
Member Author

For a backend service (or an oracle) can serialized private key be stored along with other API tokens in production? I mean as a hot coin storage.

@dincho @loxs Let's agree on sk_ prefix, account_seckey.

@loxs
Copy link

loxs commented Aug 28, 2023

Probably what Hans has in mind is that such services need to use aex3 wallet files in such cases. As always, the real world is a bit different, even the node gained this ability just recently (not merged yet): aeternity/aeternity#4171

The current state is that we put base64 encoded private keys in the node config file... (of course)

My opinion is that we cannot escape from various people doing it and this PR is more good than bad (although it's a bit debatable).

@davidyuk
Copy link
Member Author

need to use aex3 wallet files

Where store the AEX-3 password then? (in case of a service, not a user) If you have a good place to store it then you can put the whole private key there. AEX-3 changes the situation only by splitting a secret key into two pieces, not sure how it can be useful.

@hanssv
Copy link
Member

hanssv commented Aug 28, 2023

Base64 are used for variable sized things base58 for fixed sized things...

@davidyuk
Copy link
Member Author

@hanssv I'm switched it to base58 🙌

@loxs loxs self-requested a review September 16, 2023 11:21
@dincho
Copy link
Member

dincho commented Nov 7, 2023

test trigger CI?!

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.

None yet

5 participants