-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow ed25519 node and validator keys #2647
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20190615 - 08:57:46 Test Results
|
Codecov Report
@@ Coverage Diff @@
## develop #2647 +/- ##
===========================================
+ Coverage 70.02% 70.04% +0.02%
===========================================
Files 704 704
Lines 55262 55250 -12
===========================================
+ Hits 38696 38699 +3
+ Misses 16566 16551 -15
Continue to review full report at Codecov.
|
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.
Looks pretty good. I haven't finished my review yet, but I want to pass these thoughts along to you now for your consideration.
s.add32(std::uint32_t(proposal.proposeSeq())); | ||
s.add32(proposal.closeTime().time_since_epoch().count()); | ||
s.add256(proposal.prevLedger()); | ||
s.add256(proposal.position()); |
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.
Not excited about the duplication of the serialization code here and in RCLCxPeerPos.cpp. Consider refactoring that code into ConsensusProposal.h. A cherry-pickable example is here: scottschurr@0c2f17f. I was initially worried that would lead to a levelization violation in ConsensusProposal.h, but it was already bringing in stuff from ripple/protocol
, so it looks like it will be okay.
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.
ca2e49a incorporates the changes from your commit, except I opted to replace RCLCxPeerPos::signingData()
with a non-member function similar to the existing proposalUniqueId instead of including more ripple/
in the consensus code.
|
||
//! Verify the signing hash of the proposal | ||
//! Verify the signing data of the proposal | ||
bool | ||
checkSign() const; | ||
|
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 noticed that hash_append()
, further down in this file, doesn't seem to be used any more. I don't know whether that's been true for a while or if it's due to changes in this branch. You might take a look around for other unused code. I wonder if there are some calls to getSigningHash()
that are no longer used? Thanks.
auto const m = Manifest::make_Manifest( | ||
boost::beast::detail::base64_decode(token->manifest)); | ||
if (! m) | ||
{ | ||
configInvalid_ = true; |
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.
The stuff inside these braces is not being hit by your unit tests. Consider adding a unit test for a config with a corrupt validator token.
} | ||
|
||
bool | ||
RCLCxPeerPos::checkSign() const | ||
{ | ||
return verifyDigest( | ||
publicKey(), signingHash(), signature(), false); | ||
return verify( |
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.
This method and the signingData()
method above are not being hit by your unit tests. Would it be possible to exercise this code with a unit test? Thanks.
You might take a peek in your codecov.io results (https://codecov.io/gh/ripple/rippled/compare/09050a860be745c88a54fbcbce1e317c72f2f51e...6c7a1270996b0dcae68bf2b8ae233c9f987c5917/diff) for additional unit testing ideas.
src/test/app/ValidatorKeys_test.cpp
Outdated
@@ -70,31 +70,29 @@ class ValidatorKeys_test : public beast::unit_test::suite | |||
"RFE9PSIsInZhbGlkYXRpb25fc2VjcmV0X2tleSI6IjkyRDhCNDBGMzYwMTc5MTkwMU\n", | |||
"MzQTUzMzI3NzBDMkUwMTA4MDI0NTZFOEM2QkI0NEQ0N0FFREQ0NzJGMDQ2RkYifQ==\n"}; | |||
|
|||
const std::vector<std::string> tokenBlobEd25519 = { |
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.
Consider renaming the other tokenManifest
s and tokenBlob
s in this test to include sepc256k
in their names. That might make future maintenance easier.
{ | ||
SerialIter sit{payload2, sizeof(payload2)}; | ||
auto stx = std::make_shared<ripple::STValidation>(sit, | ||
[](PublicKey const& pk) { | ||
return calcNodeID(pk); | ||
}, false); | ||
fail("An exception should have been thrown"); |
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.
Even though payload2 is now valid, it feels like it would be nice to have a different payload that causes an exception to be thrown.
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.
@scottschurr payload3 still causes the exception
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.
It might be worth adding another payload with a public key and a bad signature...
// Invalid signature
constexpr unsigned char payload4[] = {
0x73, 0x21, 0xed, 0xff, 0x03, 0x1c, 0xbe, 0x65, 0x22,
0x61, 0x9c, 0x5e, 0x13, 0x12, 0x00, 0x3b, 0x43, 0x00,
0x00, 0x00, 0xf7, 0x3f, 0x3f, 0x3f, 0x3f, 0x3f, 0x3f,
0x3f, 0x3f, 0x13, 0x13, 0x13, 0xaa, 0xaa, 0xaa};
try
{
SerialIter sit{payload4, sizeof(payload4)};
std::make_shared<ripple::STValidation>(
sit, [](PublicKey const& pk) { return calcNodeID(pk); }, true);
fail("An exception should have been thrown");
}
catch (std::exception const& e)
{
BEAST_EXPECT(
strcmp(e.what(), "Invalid signature in validation") == 0);
}
0x75, 0x73, 0x74, 0x53, 0x65, 0x74, 0x65, 0x61, 0x74, 0x65, 0x88, 0x00, 0xe6, 0x88, 0x00, 0xe6, | ||
0x73, 0x00, 0x72, 0x00, 0x8a, 0x00, 0x88, 0x00, 0xe6 | ||
}; | ||
|
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.
Consider condensing into something like
std::vector<std::vector<unsigned char>> const payloads {
// specifies a secp256k1 public key
{0x72, 0x00, 0x73, 0x21, 0x03, 0x76, 0x08, 0x7D, 0x78, 0x33,
0x5F, 0xE2, 0x08, 0xEE, 0xE1, 0x5D, 0x30, 0x96, 0x50, 0xFA,
0xAE, 0x60, 0xF4, 0xC5, 0xB7, 0xF3, 0x62, 0xAF, 0x97, 0x43,
0xF1, 0x72, 0x01, 0x9C, 0xBB, 0x20, 0xC8, 0x76, 0x31, 0x30,
0x37, 0x5f, 0x5f, 0x63, 0x6c, 0x61, 0x73, 0x73, 0x5f, 0x74,
0x79, 0x70, 0x65, 0x5f, 0x69, 0x6e, 0x66, 0x6f, 0x45, 0x00,
0xe6, 0x88, 0x54, 0x72, 0x75, 0x73, 0x74, 0x53, 0x65, 0x74,
0x65, 0x61, 0x74, 0x65, 0x88, 0x00, 0xe6, 0x88, 0x00, 0xe6,
0x73, 0x00, 0x72, 0x00, 0x8a, 0x00, 0x88, 0x00, 0xe6},
// specifies an Ed25519 public key
{0x72, 0x00, 0x73, 0x21, 0xed, 0x78, 0x00, 0xe6, 0x73, 0x00,
0x72, 0x00, 0x3c, 0x00, 0x00, 0x00, 0x88, 0x00, 0xe6, 0x73,
0x38, 0x00, 0x00, 0x8a, 0x00, 0x88, 0x4e, 0x31, 0x30, 0x5f,
0x5f, 0x63, 0x78, 0x78, 0x61, 0x62, 0x69, 0x76, 0x31, 0x30,
0x37, 0x5f, 0x5f, 0x63, 0x6c, 0x61, 0x73, 0x73, 0x5f, 0x74,
0x79, 0x70, 0x65, 0x5f, 0x69, 0x6e, 0x66, 0x6f, 0x45, 0x00,
0xe6, 0x88, 0x54, 0x72, 0x75, 0x73, 0x74, 0x53, 0x65, 0x74,
0x65, 0x61, 0x74, 0x65, 0x88, 0x00, 0xe6, 0x88, 0x00, 0xe6,
0x73, 0x00, 0x72, 0x00, 0x8a, 0x00, 0x88, 0x00, 0xe6},
// specifies an Ed25519 public key
{0x73, 0x21, 0xed, 0xff, 0x03, 0x1c, 0xbe, 0x65, 0x22,
0x61, 0x9c, 0x5e, 0x13, 0x12, 0x00, 0x3b, 0x43, 0x00,
0x00, 0x00, 0xf7, 0x3f, 0x3f, 0x3f, 0x3f, 0x3f, 0x3f,
0x3f, 0x3f, 0x13, 0x13, 0x13, 0x3a, 0x27, 0xff},
// Has no public key at all
{0x72, 0x00, 0x76, 0x31, 0x30, 0x37, 0x5f, 0x5f, 0x63, 0x6c, 0x61,
0x73, 0x73, 0x5f, 0x74, 0x79, 0x70, 0x65, 0x5f, 0x69, 0x6e, 0x66,
0x6f, 0x45, 0x00, 0xe6, 0x88, 0x54, 0x72, 0x75, 0x73, 0x74, 0x53,
0x65, 0x74, 0x65, 0x61, 0x74, 0x65, 0x88, 0x00, 0xe6, 0x88, 0x00,
0xe6, 0x73, 0x00, 0x72, 0x00, 0x8a, 0x00, 0x88, 0x00, 0xe6}
};
try
{
for (auto const& payload : payloads)
{
SerialIter sit{payload.data(), payload.size()};
std::make_shared<ripple::STValidation>(
sit,
[](PublicKey const& pk) {return calcNodeID(pk);},
false);
}
fail("An exception should have been thrown");
}
catch (std::exception const& e)
{
BEAST_EXPECT(
strcmp(e.what(), "Invalid public key in validation") == 0);
}
KeyType::ed25519 | ||
}}; | ||
|
||
for (auto const& keyType : keyTypes) |
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.
Consider eliminating keyTypes
and using for (auto const &keyType : {KeyType::secp256k1, KeyType::ed25519})
BEAST_EXPECT(k.manifest == tokenManifestEd25519); | ||
BEAST_EXPECT(!k.configInvalid()); | ||
} | ||
|
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.
Consider condensing into...
auto validateToken = [&](
KeyType keyType,
std::string tknManifest,
std::vector<std::string> tknBlob)
{
auto const m {Manifest::make_Manifest(
boost::beast::detail::base64_decode(tknManifest))};
BEAST_EXPECT(m);
Config c;
c.section(SECTION_VALIDATOR_TOKEN).append(tknBlob);
ValidatorKeys k{c, j};
auto const tknPublicKey{derivePublicKey(keyType, tokenSecretKey)};
BEAST_EXPECT(k.publicKey == tknPublicKey);
BEAST_EXPECT(k.secretKey == tokenSecretKey);
auto const tknNodeID{calcNodeID(m->masterKey)};
BEAST_EXPECT(k.nodeID == tknNodeID);
BEAST_EXPECT(k.manifest == tknManifest);
BEAST_EXPECT(!k.configInvalid());
};
// validator token
validateToken(KeyType::secp256k1, tokenManifest, tokenBlob);
// validator token with ed25519 key
validateToken(KeyType::ed25519, tokenManifestEd25519, tokenBlobEd25519);
Another option is to place the validations in a container of tuples (key type, manifest, blob) and use a range based loop.
eg.
std::vector<std::tuple<KeyType, std::string, std::vector<std::string>>>
const valData
{
{
KeyType::secp256k1,
// token manifest
"JAAAAAFxIe1FtwmimvGtH2iCcMJqC9gVFKilGfw1/vCxHXXLplc2GnMhAkE1agqXxBwD"
"wDbID6OMSYuM0FDAlpAgNk8SKFn7MO2fdkcwRQIhAOngu9sAKqXYouJ+l2V0W+sAOkVB"
"+ZRS6PShlJAfUsXfAiBsVJGesaadOJc/aAZokS1vymGmVrlHPKWX3Yywu6in8HASQKPu"
"gBD67kMaRFGvmpATHlGKJdvDFlWPYy5AqDedFv5TJa2w0i21eq3MYywLVJZnFOr7C0kw"
"2AiTzSCjIzditQ8=",
// token blob
{" "
"eyJ2YWxpZGF0aW9uX3NlY3JldF9rZXkiOiI5ZWQ0NWY4NjYyNDFjYzE4YTI3ND"
"diNT\n",
" \tQzODdjMDYyNTkwNzk3MmY0ZTcxOTAyMzFmYWE5Mzc0NTdmYTlkYWY2Iiwib"
"WFuaWZl "
" \n",
"\tc3QiOiJKQUFBQUFGeEllMUZ0d21pbXZHdEgyaUNjTUpxQzlnVkZLaWxHZncx"
"L3ZDeE"
"\n",
"\t "
"hYWExwbGMyR25NaEFrRTFhZ3FYeEJ3RHdEYklENk9NU1l1TTBGREFscEFnTms4"
"U0tG\t "
"\t\n",
"bjdNTzJmZGtjd1JRSWhBT25ndTlzQUtxWFlvdUorbDJWMFcrc0FPa1ZCK1pSUz"
"ZQU2\n",
"hsSkFmVXNYZkFpQnNWSkdlc2FhZE9KYy9hQVpva1MxdnltR21WcmxIUEtXWDNZ"
"eXd1\n",
"NmluOEhBU1FLUHVnQkQ2N2tNYVJGR3ZtcEFUSGxHS0pkdkRGbFdQWXk1QXFEZW"
"RGdj\n",
"VUSmEydzBpMjFlcTNNWXl3TFZKWm5GT3I3QzBrdzJBaVR6U0NqSXpkaXRROD0i"
"fQ==\n"}
}
};
...
for (auto const& v : valData)
{
auto const m {Manifest::make_Manifest(
boost::beast::detail::base64_decode(std::get<1>(v)))};
BEAST_EXPECT(m);
Config c;
c.section(SECTION_VALIDATOR_TOKEN).append(std::get<2>(v));
ValidatorKeys k{c, j};
auto const tknPublicKey{
derivePublicKey(std::get<0>(v), tokenSecretKey)};
BEAST_EXPECT(k.publicKey == tknPublicKey);
BEAST_EXPECT(k.secretKey == tokenSecretKey);
auto const tknNodeID{calcNodeID(m->masterKey)};
BEAST_EXPECT(k.nodeID == tknNodeID);
BEAST_EXPECT(k.manifest == std::get<1>(v));
BEAST_EXPECT(!k.configInvalid());
};
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.
Consider condensing all of the config checks into somehting like ...
auto configCheck = [&](std::vector<
std::pair<std::string, std::vector<std::string>>> sections)
{
Config c;
for (auto const& s : sections)
c.section(s.first).append(s.second);
ValidatorKeys k{c, j};
BEAST_EXPECT(k.configInvalid());
BEAST_EXPECT(k.publicKey.size() == 0);
BEAST_EXPECT(k.manifest.empty());
};
// invalid validator token
configCheck({{SECTION_VALIDATOR_TOKEN, {"badtoken"}}});
// Cannot specify both
configCheck({
{SECTION_VALIDATION_SEED, {seed}},
{SECTION_VALIDATOR_TOKEN, tokenBlob}});
// Token manifest and private key must match
configCheck({{SECTION_VALIDATOR_TOKEN, invalidTokenBlob}});
The most recent commits are not building for me in Nounity/Debug/Clang/MacOS. Adding |
bool | ||
RCLCxPeerPos::checkSign() const | ||
{ | ||
return verifyDigest( | ||
publicKey(), signingHash(), signature(), false); | ||
return verify( |
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.
It looks like, as a result of this change, the verifyDigest()
free function no longer needs to be public. It can be an implementation detail of the PublicKey.cpp
file. So I suggest removing the declaration of verifyDigest()
from PublicKey.h
and make the implementation of verifyDigest()
in PublicKey.cpp
static
.
You'll need to fix up SecretKey_test.cpp
if you choose to make this change.
proposal.prevLedger(), | ||
proposal.position()); | ||
|
||
auto sig = signDigest(valPublic_, valSecret_, signingHash); |
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.
As a consequence of this change the two SignDigest()
free functions in the code base are no longer used. Their declarations can be removed from SecretKey.h
and the implementation can be removed from SecretKey.cpp
.
If you choose to make this change you'll need to fix up SecretKey_test.cpp
.
bb67a9f
to
56a9a17
Compare
Rebased to resolve conflicts with rc3 |
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.
scratched
Looked at the wrong version of the code. Oops!
“VETO” caught my eye @nbougalis
I can’t quite make sense of your comment though.
It looks like the hashing is moved to ‘sign()’ function where it can be
optionally done? Ed25519 manages message hashing itself.
https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/SecretKey.cpp#L141-L148
|
Aaaaah, I saw the email notification, oops! |
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.
A few thoughts on unit tests.
{ | ||
BEAST_EXPECT(strcmp(e.what(), "Invalid public key in validation") == 0); | ||
// Invalid signature |
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.
This test exercises an invalid signature and hits this check: https://github.com/ripple/rippled/blob/56a9a170c29f9ec593e9f34b81c80d6a86ddd5cc/src/ripple/protocol/STValidation.h#L87
Would it be possible to make a similar test with a bad public key? Then you could hit this check, which is not currently exercised: https://github.com/ripple/rippled/blob/56a9a170c29f9ec593e9f34b81c80d6a86ddd5cc/src/ripple/protocol/STValidation.h#L80
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 think that's happening in the test above
https://github.com/ripple/rippled/pull/2647/files/56a9a170c29f9ec593e9f34b81c80d6a86ddd5cc#diff-78fe179d4e58a29ba79c1d7a7b6a033fR107
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.
Interesting. Wonder why the tests are not hitting here then: https://github.com/ripple/rippled/blob/56a9a170c29f9ec593e9f34b81c80d6a86ddd5cc/src/ripple/protocol/STValidation.h#L80. But we can't kit 100% coverage anyway, so it's probably not worth pushing hard on.
proposal().prevLedger(), | ||
proposal().position()); | ||
} | ||
|
||
bool | ||
RCLCxPeerPos::checkSign() const |
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 noticed that RCLCxPeerPos::checkSign()
doesn't appear to be exercised by the unit tests. Seems like it should be? But I'll admit that may be outside what you think is appropriate to include in this pull request. Your call.
"NJtYpVLHRwGcBJAC5ijq2BUDGbuLoDfqfdBQZl0DPu83pIs49lQsG+29yvl" | ||
"flAp8BV7wPOG+HX2Azwh3wqsmthuDeLCM3nVKXNfCw=="; | ||
|
||
testValidatorToken(KeyType::ed25519, tokenManifest, tokenBlob); |
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 like this test with a valid ed25519 key. I'm looking for a similar test in this file that uses an secp256k1 key but didn't spot one. If there isn't one it seems like that would be a good thing to add.
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.
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.
Thanks. Sorry I missed that one.
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 tests. As far as I can tell this looks good. 👍
"NJtYpVLHRwGcBJAC5ijq2BUDGbuLoDfqfdBQZl0DPu83pIs49lQsG+29yvl" | ||
"flAp8BV7wPOG+HX2Azwh3wqsmthuDeLCM3nVKXNfCw=="; | ||
|
||
testValidatorToken(KeyType::ed25519, tokenManifest, tokenBlob); |
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.
Thanks. Sorry I missed that one.
{ | ||
BEAST_EXPECT(strcmp(e.what(), "Invalid public key in validation") == 0); | ||
// Invalid signature |
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.
Interesting. Wonder why the tests are not hitting here then: https://github.com/ripple/rippled/blob/56a9a170c29f9ec593e9f34b81c80d6a86ddd5cc/src/ripple/protocol/STValidation.h#L80. But we can't kit 100% coverage anyway, so it's probably not worth pushing hard on.
RIPD-1424
a la proposalUniqueId
56a9a17
to
e014419
Compare
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.
Conditional approval.
I like this commit and would be happy to allow Ed25519 keys for validations and node identities, but deploying this is, at best, challenging.
We need to add support for such keys first, but not offer a way to configure such keys (absent users changing the code, of course, which is beyond our control) until the network, as a whole, understands such keys and how to verify signatures.
This might mean introducing version 1.3 of the protocol, but stick to talking 1.2. Once we're confident t that the servers understand this, we can then add another commit to allow using these keys.
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 am closing this, because it has bit-rotted. We will look at reimplementing this in the future. |
This change enables rippled to handle peer messages, proposals and validations signed by ed25519 keys.
You can configure rippled to use an ed25519 validator key by including a validator token with a manifest whose signing public key is that type.
There is not currently a way to configure rippled to use an ed25519 node public key.
RIPD-1424