Add support for Ed25519 seeds encoded using ripple-lib: #2734

nbougalis · 2018-10-22T17:31:44Z

When Ed25519 support was added to ripple-lib, a way to specify whether a seed should be used to derive a "classic" secp256k1 keypair or a "new" Ed25519 keypair, and the requirements were that:

previously seeds would, correctly, generate a secp256k1 keypair.
users would not have to know about whether the seed was used to generate a secp256k1 or an Ed25519 keypair.

The solution was to encode the type of key within the seed and a custom encoding was designed. The encoding utilizes a 3-byte prefix for Ed25519 keypairs which results in such keypairs beginning with the string "sEd". The encoding is non-standard and such that other sofware will treat such keys as invalid.

This commit adds support to rippled for automatically detecting and properly handling such seeds.

nbougalis · 2018-10-22T17:34:33Z

src/ripple/rpc/handlers/WalletPropose.cpp


    Json::Value obj (Json::objectValue);

-    auto const seed1751 = seedAs1751 (*seed);
    auto const seedHex = strHex (*seed);
    auto const seedBase58 = toBase58 (*seed);

    obj[jss::master_seed] = seedBase58;


IMPORTANT THING TO POINT OUT:

While we parse the ripple-lib encoded Ed25519 key correctly, we return the "canonical" seed encoding. Should that be changed? And, more generally, should we just accept that that an encoded seed represents the key type and the seed used to generate the wallet?

It seems we should return an error upon detecting a type mismatch especially since we already reject upon an invalid keyType.

We do. Look at line 102 in this same file.

sublimator · 2018-10-22T17:38:54Z

@intelliot You mentioned some thought out bip for this at some time iirc, for use with destination tags?

nbougalis · 2018-10-22T17:44:07Z

That’s coming sometime this week. A new seed format could also be done at that time. But the current must be supported anyways.

sublimator · 2018-10-22T17:56:22Z

Haha, my bad :/

sublimator · 2018-10-22T17:59:01Z

Maybe u cld migrate ppl to new format using js tools? Rather than have to keep supporting?

seelabs · 2018-10-22T18:12:12Z

src/ripple/crypto/impl/KeyType.cpp and src/ripple/rpc/handlers/ValidationSeed.cpp need to be removed from the CMakeLists.txt file or nonunity build will fail.

src/ripple/crypto/KeyType.h

seelabs · 2018-10-22T18:53:28Z

src/ripple/rpc/handlers/WalletPropose.cpp

+    // ripple-lib encodes seed used to generate an Ed25519 wallet in a
+    // non-standard way. While we never encode seeds that way, we try
+    // to detect such keys to avoid user confusion.
+    if(seed = RPC::parseRippleLibSeed(params))


Suggested change

if(seed = RPC::parseRippleLibSeed(params))

if ((seed = RPC::parseRippleLibSeed(params)))

Assignments in if statements that don't declare variables always make me look twice. I think the convention is to add redundent parens to signal "yes, I really meant = and not ==."

Hmm, yeah. It’s sketchy even with the double parens. I’ll rewrite.

seelabs · 2018-10-22T18:55:09Z

src/ripple/rpc/impl/Handler.cpp

@@ -111,7 +111,6 @@ Handler const handlerArray[] {
    {   "tx_history",           byRef (&doTxHistory),           Role::USER,  NO_CONDITION     },
    {   "unl_list",             byRef (&doUnlList),             Role::ADMIN,   NO_CONDITION     },
    {   "validation_create",    byRef (&doValidationCreate),    Role::ADMIN,   NO_CONDITION     },
-    {   "validation_seed",      byRef (&doValidationSeed),      Role::ADMIN,   NO_CONDITION     },


I'd probably split the changes in this commit into two commits: 1) Handle ripple-lib encoded seeds, 2) Remove "validation_seed" command.

seelabs · 2018-10-22T18:56:04Z

src/ripple/rpc/handlers/WalletPropose.cpp

    auto const seedHex = strHex (*seed);
    auto const seedBase58 = toBase58 (*seed);

    obj[jss::master_seed] = seedBase58;
    obj[jss::master_seed_hex] = seedHex;
-    obj[jss::master_key] = seed1751;


We'll need to document that we're removing this field.

I'll just re-add it. It's pointless to remove it at this point.

seelabs · 2018-10-22T18:58:32Z

src/ripple/rpc/impl/RPCHelpers.cpp

+        return boost::none;
+    };
+
+    if (params.isMember(jss::passphrase) && !params.isMember(jss::seed))


Do we want to do an entropy check on the passphrase like we do for non-ripple-lib encoded passphrases?
Edit: also see comment above about using passphrases; I'm not sure we should be checking this at all.

Don't we need to check jss::secret?
Edit: keypairForSignature needs to check this, WalletPropose does not. Consider having the caller find the correct field the secret is contained in and just pass in either the std::string to decode or Json::Value to the parseRippleLibSeed function. i.e pretty much what the decode lambda does now.

Ugh... 😡

src/ripple/rpc/impl/RPCHelpers.cpp

seelabs · 2018-10-22T19:02:27Z

src/ripple/rpc/impl/RPCHelpers.cpp

+    // ripple-lib encodes seed used to generate an Ed25519 wallet in a
+    // non-standard way. While we never encode seeds that way, we try
+    // to detect such keys to avoid user confusion.
+    if(seed = RPC::parseRippleLibSeed(params))


Suggested change

if(seed = RPC::parseRippleLibSeed(params))

if ((seed = RPC::parseRippleLibSeed(params)))

Add redundant params arround assignment in if when not declaring a new variable.

seelabs · 2018-10-22T19:56:46Z

src/ripple/rpc/impl/RPCHelpers.cpp

+        if (!j.isString())
+            return boost::none;
+
+        auto const result = decodeBase58Token(j.asString(), TokenType::None);


A passphrase isn't going to be base58 encoded. Shouldn't this use a different decode for passphrases?
Edit: We derive the seed from a passphrase in two ways: 1) we hash it; 2) we use rfc1715 to get a 128 bit seed. I don't see how either of those can be used to add additional encoding to specify a ripple lib seed. So I don't think we should use a passphrase here at all.

Several codepaths use “passphrase” as the catch-all (e.g. wallet_propose) and we make an effort to determine what this thing is.

I tried very carefully to keep the semantics sane and in line with previous expectations, but I’ll double-check.

You're right, I'm looking at ParseGenericSeed (https://github.com/seelabs/rippled/blob/develop/src/ripple/protocol/impl/Seed.cpp#L95-L130), and GetSeedFromRPC (https://github.com/seelabs/rippled/blob/develop/src/ripple/rpc/impl/RPCHelpers.cpp#L542-L579) the logic is:

If one of the following are set: passphrase, seed, seed hex, try the following:

If "seed", parse as base58

If "passphrase":
a) check if hex, otherwise
b) check if base 58, otherwise
c) use RFC1751, otherwise
d) hash the passphrase

if "seed_hex", set hex exact

Yeah, the logic is, let's be generous and say convoluted. I went through and reverified the logic and I'm convinced the changes do the right thing.

The only "potential" problem is someone that used a passphrase that happens to also be a valid "ripple-lib encoded Ed25519" string. But the chances of that are, essentially, astronomical.

yxxyun · 2018-10-23T00:51:20Z

will this change affect Ed25519 seeds generated by other tool？
https://github.com/rubblelabs/ripple/blob/master/crypto/ed25519.go#L35

ripplelabs-jenkins · 2018-10-23T05:12:42Z

Jenkins Build Summary

Built from this commit

Built at 20181023 - 19:23:46

Test Results

Build Type	Log	Result	Status
rpm	logfile	1141 cases, 0 failed, t: n/a	PASS ✅
msvc.Debug	logfile	1138 cases, 0 failed, t: 621s	PASS ✅
msvc.Debug, NINJA_BUILD=true	logfile	1138 cases, 0 failed, t: 641s	PASS ✅
gcc.Release -Dassert=ON, MANUAL_TESTS=true	logfile	1275 cases, 0 failed, t: 6m8s	PASS ✅
docs, TARGET=docs	logfile	1 cases, 0 failed, t: 0m1s	PASS ✅
gcc.Debug -Dcoverage=ON, TARGET=coverage_report, SKIP_TESTS=true	logfile	1141 cases, 0 failed, t: 17m6s	PASS ✅
clang.Debug	logfile	1141 cases, 0 failed, t: 3m11s	PASS ✅
msvc.Debug -Dunity=OFF	logfile	1138 cases, 0 failed, t: 1277s	PASS ✅
gcc.Debug	logfile	1141 cases, 0 failed, t: 3m28s	PASS ✅
clang.Debug -Dunity=OFF	logfile	1141 cases, 0 failed, t: 9m7s	PASS ✅
msvc.Release	logfile	1138 cases, 0 failed, t: 410s	PASS ✅
clang.Release -Dassert=ON	logfile	1141 cases, 0 failed, t: 5m44s	PASS ✅
gcc.Debug -Dunity=OFF	logfile	1141 cases, 0 failed, t: 11m44s	PASS ✅
gcc.Release -Dassert=ON	logfile	1141 cases, 0 failed, t: 6m5s	PASS ✅
gcc.Debug -Dstatic=OFF	logfile	1141 cases, 0 failed, t: 3m28s	PASS ✅
gcc.Debug -Dstatic=OFF -DBUILD_SHARED_LIBS=ON	logfile	1141 cases, 0 failed, t: 3m26s	PASS ✅
gcc.Debug, NINJA_BUILD=true	logfile	1141 cases, 0 failed, t: 3m3s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=address, PARALLEL_TESTS=false, DEBUGGER=false	logfile	1141 cases, 0 failed, t: 9m28s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=undefined, PARALLEL_TESTS=false	logfile	1141 cases, 0 failed, t: 11m40s	PASS ✅

nbougalis · 2018-10-23T06:19:50Z

@yxxyun: this issue is because ripple-lib (or, more specifically, ripple-keypairs) uses a "non-standard" base58 encoding of seeds used to generate Ed25519 wallets.

The code you link to does not appear to handle base58 encoded keys; it deals with the raw value of the seed, after it has been decoded from its string form. I don't know if that code handles string-encoded seeds at all, thought I assume it does. If it does, it may be useful to at least add detection of these "non-standard" seeds. Perhaps @donovanhide can chime in on this.

The 'validation_seed' RPC command was used to change the validation key used by a validator at runtime. Its implementation was commented out with commit fa796a2 which has been included in the codebase since the 0.30.0 release and there are no plans to reintroduce the functionality at this point. Validator operators should migrate to using validator manifests instead.

When Ed25519 support was added to ripple-lib, a way to specify whether a seed should be used to derive a "classic" secp256k1 keypair or a "new" Ed25519 keypair was needed, and the requirements were that: 1. previously seeds would, correctly, generate a secp256k1 keypair. 2. users would not have to know about whether the seed was used to generate a secp256k1 or an Ed25519 keypair. To address these requirements, the decision was made to encode the type of key within the seed and a custom encoding was designed. The encoding uses a token type of 1 and prefixes the actual seed with a 2 byte header, selected to ensure that all such keypairs will, when encoded, begin with the string "sEd". This custom encoding is non-standard and was not previously documented; as a result, it is not widely supported and other sofware may treat such keys as invalid. This can make it difficult for users that have stored such a seed to use wallets or other tooling that is not based on ripple-lib. This commit adds support to rippled for automatically detecting and properly handling such seeds.

MarkusTeufelberger · 2018-10-23T09:57:13Z

Where is the documentation for validator manifests?

seelabs

This code is getting messy (not your fault, it was messy already when you added your modifications). I'm going to try to refactor some of this code with the following plan:

Find the field the secret lives in from a list of candidate fields
Decode that field accoring to field specific rules (i.e. hex for key_hex, longer list for passphrase)
If the field can decode as base58, handle both ripple-lib and non-ripple lib cases
Try the other decoders
I'll try to code that up. We still may want to do that in another PR even if it looks good.

src/ripple/crypto/KeyType.h

seelabs · 2018-10-23T14:08:10Z

src/ripple/crypto/impl/KeyType.cpp

@@ -1,39 +0,0 @@
-//------------------------------------------------------------------------------


This file needs to be removed from CMakeLists.txt or the non unity build will fail.

Gah, I thought I did this.

seelabs · 2018-10-23T14:21:34Z

src/ripple/rpc/impl/RPCHelpers.cpp

+
+    // ripple-lib encodes seed used to generate an Ed25519 wallet in a
+    // non-standard way. While we never encode seeds that way, we try
+    // to detect such keys to avoid user confusion.
    {


We already know what field the secret is in. Lines 677-683 can be simplified to:

if (secretType != jss::seed_hex.c_str()){ seed = RPC::parseRippleLibSeed(params[secretType]);

seelabs · 2018-10-23T14:39:08Z

src/test/rpc/KeyGeneration_test.cpp

+                Json::Value error;
+
+                params[jss::key_type] = "secp256k1";
+                params[jss::secret] = seed;


This should be jss::seed, not jss::secret. A jss::secret is not allowed to set the key type, and this test is testing for a key type mismatch.

Co-Authored-By: nbougalis <[email protected]>

seelabs · 2018-10-23T19:03:18Z

Here's my take on a refactor: seelabs@034e7b3 Here's the most relevant part: https://github.com/seelabs/rippled/blob/json_seed_refactor/src/ripple/rpc/impl/RPCHelpers.cpp#L546-L729
A couple notes:

Note both keypairForSignature and getSeedFromRPC call the same helper function.
This decodes base 58 many fewer times than the pre-refactored code (there were at least 5 redundant decodes to check for AccuontID ect)
I think the code is clearer

Also please note I'm not proposing that for the code base. I just wanted to give an idea of what I had in mind (that doesn't even pass the JSONRPC test - most likely because the error messages changed, though I haven't dug into it. It does pass the WalletPropose tests).

seelabs

👍

HowardHinnant · 2018-10-24T16:22:52Z

src/ripple/rpc/handlers/WalletPropose.cpp

-        params.isMember (jss::seed_hex))
+    // ripple-lib encodes seed used to generate an Ed25519 wallet in a
+    // non-standard way. While we never encode seeds that way, we try
+    // to detect such keys to avoid user confusion.
    {


I found myself reading this over and over to see the purpose of the limited scope. I finally convinced myself that it has no purpose, and was just left over from the removed if.

I wanted group the ripple-lib specific workaround together in one block.

sublimator · 2018-10-31T07:34:46Z

src/ripple/rpc/impl/RPCHelpers.cpp

+boost::optional<Seed>
+parseRippleLibSeed(Json::Value const& value)
+{
+    // ripple-lib encodes seed used to generate an Ed25519 wallet in a


"used to"
Seems like it still does?

MarkusTeufelberger · 2018-10-31T08:18:31Z

I disagree with the concept by the way for what it's worth: If a library does something specific/weird that only makes sense within this library, it shouldn't be integrated in the server software but fixed within that library. Otherwise every library can just invent their custom encoding scheme.

sublimator · 2018-10-31T10:02:19Z

It made sense at the time :) Ripple still published a client with steps to back up your secret as a nice single base58 string. It was supposed to be implemented in rippled but it was never gotten around to... till now I guess :)

nbougalis · 2018-10-31T15:48:53Z

I disagree with the concept by the way for what it's worth: If a library does something specific/weird that only makes sense within this library, it shouldn't be integrated in the server software but fixed within that library. Otherwise every library can just invent their custom encoding scheme.

While I agree that we shouldn't support every encoding ever thought up, the simple fact is that ripple-lib is used by several wallets already. It generates canonical seeds for secp256k1 keys but non-canonical seeds for Ed25519 keys. This is very surprising behavior to users. We can't give them something that looks like a wallet seed, but does not behave like one. It certainly was surprising to me.

The alternative to this commit is to detect such seeds, when entered, and display a warning message so users understand what is happening.

MarkusTeufelberger · 2018-10-31T16:03:14Z

Since a canonical format can be generated from non-canonical seeds apparently, this should be done by ripple-lib before communicating with rippled (this function probably shouldn't be available to end users anyways without big fat warnings, as it sends private key material to a trusted server, something that end users shouldn't do).

MarkusTeufelberger · 2018-10-31T16:16:54Z

It is just making it more difficult for someone to reimplement rippled (especially since I don't find a good description of this custom encoding), but it might make it easier for ripple-lib users that send seeds to a rippled server.

nbougalis · 2018-10-31T16:42:09Z

The problem isn’t that ripple-lib communicates this seed to rippled; it doesn’t, because it has the ability to sign.

The problem is that it communicates this encoded seed to the user so that they can have a backup. And that encoding will only work with ripple-lib.

It’s a big surprise and at least a little bit scary (happy Halloween, by the way 🎃) if a seed that I was told to backup to be able to recover my wallet or import it to another one ends up not working when entered into another wallet.

MarkusTeufelberger · 2018-11-02T15:43:53Z

Just FYI: The "Ledger" hardware wallet (LedgerHQ/ledger-wallet-ripple#5) has a similar or even worse flaw/issue and is probably FAR more widespread than ed25519 keys on ripple-lib (unless they are now the default way to create wallets? Note to self: Get stats on distribution of 25519 vs 256k accounts).

nbougalis · 2018-11-02T23:50:37Z

I know that Ledger uses an incompatible derivation mechanism. However unfortunate, I understand their rationale. At least they don’t display the key formatted to look like a seed.

But Ripple is the primary author of ripple-lib and it is the reference JS library, not a third party wallet. And that, to me, is the primary distinction and why I believe it important to at least have rudimentary support for this new encoding in rippled.

Mind you, that it’s my position (speaking only for myself and not ex cathedra) that rippled should not have any signing functionality or wallet generating functionality built in at all. That should be delegated to a stand-alone tool. And I would not be opposed to such a tool having support for Ledger’s key format.

MarkusTeufelberger · 2018-11-03T00:01:48Z

I agree with you on the front of not being able to create keys, I'm kinda unsure about the signing part (signing alone is easy, serializing/encoding the data to be signed is a different matter). If wallet functionality is off limits for rippled however and the FinCen restrictions have run out a few months ago, I'd really love to see Ripple either sponsor wallet creation or take up building wallet software again.

That's just a side note though, I just wanted to voice my opinion on adding more seed/key formats to rippled and I guess I was heard. If you are already considering such things, adding BIP39 support to rippled might be really helpful to a lot more people too.

lokera666

Fine

nbougalis commented Oct 22, 2018

View reviewed changes

HowardHinnant self-assigned this Oct 22, 2018

HowardHinnant self-requested a review October 22, 2018 18:35

seelabs reviewed Oct 22, 2018

View reviewed changes

nbougalis added 2 commits October 22, 2018 23:31

seelabs self-assigned this Oct 23, 2018

seelabs self-requested a review October 23, 2018 13:53

seelabs reviewed Oct 23, 2018

View reviewed changes

seelabs and others added 2 commits October 23, 2018 09:37

Update src/ripple/crypto/KeyType.h

e0728eb

Co-Authored-By: nbougalis <[email protected]>

[FOLD] Address review comments

89d171a

seelabs approved these changes Oct 23, 2018

View reviewed changes

HowardHinnant reviewed Oct 24, 2018

View reviewed changes

HowardHinnant approved these changes Oct 24, 2018

View reviewed changes

sublimator reviewed Oct 31, 2018

View reviewed changes

nbougalis mentioned this pull request Nov 2, 2018

Remove validation_seed method #2748

Closed

This was referenced Nov 4, 2018

Proposed 1.2.0-b6 #2755

Merged

Triaging of existing TODOs/FIXMEs #2615

Open

nbougalis closed this in #2755 Nov 6, 2018

XRPLF deleted a comment from nbougalis Jan 28, 2020

yxxyun mentioned this pull request Apr 11, 2021

[Feature] Add Ed25519 key pair support XRPL-Labs/Xaman-Issue-Tracker#328

Closed

lokera666 reviewed Jun 5, 2022

View reviewed changes

nbougalis deleted the sed branch October 16, 2023 06:02

	if(seed = RPC::parseRippleLibSeed(params))
	if ((seed = RPC::parseRippleLibSeed(params)))

		@@ -1,39 +0,0 @@
		//------------------------------------------------------------------------------

Add support for Ed25519 seeds encoded using ripple-lib: #2734

Add support for Ed25519 seeds encoded using ripple-lib: #2734

Conversation

nbougalis commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

sublimator commented Oct 22, 2018 via email

nbougalis commented Oct 22, 2018

sublimator commented Oct 22, 2018 via email

sublimator commented Oct 22, 2018 via email

seelabs commented Oct 22, 2018 • 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

seelabs Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

seelabs Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxxyun commented Oct 23, 2018

ripplelabs-jenkins commented Oct 23, 2018 • edited Loading

Jenkins Build Summary

Test Results

nbougalis commented Oct 23, 2018 • edited Loading

MarkusTeufelberger commented Oct 23, 2018

seelabs left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Oct 23, 2018

seelabs left a 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

MarkusTeufelberger commented Oct 31, 2018

sublimator commented Oct 31, 2018 via email

nbougalis commented Oct 31, 2018 • edited Loading

MarkusTeufelberger commented Oct 31, 2018

MarkusTeufelberger commented Oct 31, 2018

nbougalis commented Oct 31, 2018 • edited Loading

MarkusTeufelberger commented Nov 2, 2018

nbougalis commented Nov 2, 2018 • edited Loading

MarkusTeufelberger commented Nov 3, 2018

lokera666 left a comment

Choose a reason for hiding this comment

nbougalis Oct 23, 2018 •

edited

Loading

seelabs commented Oct 22, 2018 •

edited

Loading

seelabs Oct 22, 2018 •

edited

Loading

seelabs Oct 22, 2018 •

edited

Loading

seelabs Oct 22, 2018 •

edited

Loading

seelabs Oct 23, 2018 •

edited

Loading

ripplelabs-jenkins commented Oct 23, 2018 •

edited

Loading

nbougalis commented Oct 23, 2018 •

edited

Loading

nbougalis commented Oct 31, 2018 •

edited

Loading

nbougalis commented Oct 31, 2018 •

edited

Loading

nbougalis commented Nov 2, 2018 •

edited

Loading