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

Replace manifest with validator token (RIPD-1386) #1975

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
4 participants
@wilsonianb
Copy link

commented Jan 19, 2017

Replaces [validation_manifest] config section with [validator_token].
[validation_manifest] was not documented in rippled-example.cfg or the dev portal, and there is no evidence of it being used externally by validator operators.
Removes admin-only validation_manifest field from server_info

The changes only consist of the last two commits:

  • Add SecretKey comparison operator (RIPD-1382)
  • Replace manifest with validator token (RIPD-1386)

I've based this on the the dynamic UNL branch (#1842) to minimize conflicts. Let me know if there's a better way to submit this.

Reviewers: @nbougalis @scottschurr

@wilsonianb wilsonianb requested review from nbougalis and scottschurr Jan 19, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Jan 19, 2017

Codecov Report

Merging #1975 into develop will increase coverage by <.01%.

@@             Coverage Diff             @@
##           develop    #1975      +/-   ##
===========================================
+ Coverage    67.31%   67.31%   +<.01%     
===========================================
  Files          686      690       +4     
  Lines        49320    49729     +409     
===========================================
+ Hits         33201    33477     +276     
- Misses       16119    16252     +133
Impacted Files Coverage Δ
src/ripple/rpc/impl/Handler.cpp 97.67% <ø> (ø)
src/ripple/app/ledger/LedgerConsensus.h 100% <ø> (ø)
src/ripple/core/JobTypes.h 98.3% <ø> (-0.03%)
src/ripple/app/main/Application.h 100% <ø> (ø)
src/ripple/net/InfoSub.h 75% <ø> (ø)
src/ripple/app/ledger/impl/LedgerConsensusImp.h 50% <ø> (ø)
src/ripple/core/ConfigSections.h 100% <ø> (ø)
src/ripple/overlay/Overlay.h 61.11% <ø> (ø)
src/ripple/net/impl/RPCCall.cpp 36.16% <ø> (+0.83%)
src/ripple/core/Job.h 100% <ø> (ø)
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8a5f5b...4ae8fa6. Read the comment docs.

std::memcmp(lhs.data(),
rhs.data(), rhs.size()) == 0;
}

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 20, 2017

Contributor

In alignment with the principle of least surprise, it's often a good idea to supply != if a type supplies ==. The implementation could be as simple as...

inline
bool
operator!= (SecretKey const& lhs,
    SecretKey const& rhs)
{
    return ! (lhs == rhs);
}
@@ -153,7 +145,7 @@ class SecretKey_test : public beast::unit_test::suite
"pnen77YEeUd4fFKG7iycBWcwKpTaeFRkW2WFostaATy1DSupwXe");
BEAST_EXPECT(sk2);

BEAST_EXPECT(equal (sk1, *sk2));
BEAST_EXPECT(sk1 == *sk2);

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 20, 2017

Contributor

Nicely done with the testing.

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2017

I have a meta question before I review the second commit. Somehow I got the impression this pull request was attempting to move away from the term "manifest". But I'm still seeing "manifest" all over the second commit. If we end up with something that we call a "manifest" internally but we give it another name in the config file, well, I'm not very excited about that prospect. I think that would make the code harder to maintain and to talk about. The new name in the config file would need to be dramatically better than "manifest", and the cost of changing the the internal names away from "manifest" would need to be very high.

Can you give me a summary of your plans and intentions? Thanks.

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Jan 21, 2017

My goal is to replace "manifest" with "token" as far the user is concerned. The current implementation has the token as an encoded json object containing "validation_private_key" and "manifest" fields. We can't simply replace Manifest in the code with the token, because the token also contains the private key which should not be forwarded to peers. So manifests still exist as a distinct concept from tokens but are obscured from the user.

Maybe the PR/commit message should be changed to "Add validator token".

I also realize the Python tool and its manifest-tool-guide both still have manifest in the names. I anticipate both of those being removed in favor of the ripple-libpp based tool. What do you think about them being removed now?

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2017

@wilsonianb and I talked off line. Here's my takeaway.

The old [validation_manifest] required a second field in the config file: the [validation_seed]. The contents of the [validation_manifest] actually consists of three fields mashed together: two PublicKeys and an integer sequence. The [validation_manifest] and [validation_seed] data always needed to be generated at the same time and kept in sync. If you changed one in the config file you needed to also change the other.

The (new) [validation_token] field in the config file replaces both the [validation_manifest] and the [validation_seed] fields. The [validation _token] contains all of the information in the old [validation_manifest] plus the information in the [validation_seed] mashed together in one string. This way a user only needs to deal with a single large field (instead of dealing with a large field plus a smaller field).

When rippled parses the [validation_token] it separates the parts. So, internally, rippled still deals with a Manifest and a validationSeed as separate entities with different purposes. In particular, the Manifest still has a reason to stay in the code base since it is streamed to peers.

# validation_public_key: n9LtZ9haqYMbzJ92cDd3pu3Lko6uEznrXuYea3ehuhVcwDHF5coX
# sequence number: 1
```

A manifest is a signed message used to inform other servers of this validator's

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 23, 2017

Contributor

Some of the descriptions in this file that could be significantly improved given the changes you're making. However I'm not going to dig into this file in hopes that this all goes away when the ripple-libpp based tool is released in concert with these changes.

# validation without having to store the validator keys on the network
# connected server. The field should contain a JSON object with two fields:
# "validation_private_key" and "manifest". The first contains a hex-encoded
# private key used for signing validations. The second contains a

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 23, 2017

Contributor

I think this paragraph contains way too much information. The presence of the validation_private_key and manifest is completely invisible to someone editing the config. All they should see is a base64 encoded blob. Tell them that the field is a blob that is generated by external tools. Otherwise you raise too many questions.

config().section (SECTION_VALIDATORS).values (),
config().section (SECTION_VALIDATOR_LIST_KEYS).values (),
config().section (SECTION_VALIDATION_MANIFEST).lines()))
config().VALIDATION_MANIFEST))

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 23, 2017

Contributor

Should there be a VALIDATION_MANIFEST section of the config? Somehow I don't think the config should be responsible for ripping apart the [validator_token].

for (auto const& line : section (SECTION_VALIDATOR_TOKEN).lines())
tokenStr += beast::rfc2616::trim(line);

tokenStr = beast::detail::base64_decode(tokenStr);

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 23, 2017

Contributor

Um, this feels odd to me. We have a JSON formatted object encoded in base64. Partly it is unlike any of the rest of the config. This is the only place where the config decodes base64. This is also the only place that the config parses JSON.

I don't have a problem with the config handling mysterious encoded text; the manifest was doing that before. But it feels weird to me that the config is doing the decoding.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Jan 23, 2017

Author

I'm having the config do this since it is currently where VALIDATION_PRIV and VALIDATION_PUB are stored, and that information is now in the token.
I'll see if the key pair could be stored somewhere else.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 23, 2017

Contributor

I think I'd probably feel okay with the config returning a reference to a const ValidatorToken object that did the parsing and kept the data. It just feels funny for the config to be so intimate with the details of a non-human-readable field. You could also try to convince me that what the config is doing now is okay, if it doesn't feel odd to you.

@wilsonianb wilsonianb force-pushed the wilsonianb:rename-manifests branch from ebc9fb4 to b2ccffd Jan 24, 2017

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Jan 24, 2017

Rebased on dynamic-unl branch, and added a fold commit that moves validator key and token parsing out of Config (good) while adding a couple functions to LedgerConsensus (maybe not so good?)

@wilsonianb wilsonianb force-pushed the wilsonianb:rename-manifests branch from b2ccffd to 03803d1 Jan 25, 2017

@scottschurr
Copy link
Contributor

left a comment

This looks like an improvement to me. I left some notes about const and naming and the like. But I don't believe I have any structural concerns at the moment. Nice work.

PublicKey validationPub;
SecretKey validationPriv;
std::string manifest;
if (config().exists (SECTION_VALIDATOR_TOKEN))

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Should there be a check that returns an error if both SECTION_VALIDATOR_TOKEN and SECTION_VALIDATION_SEED are present in the config?

Oh, never mind. That's handled in Config.cpp where it should be.

"Invalid entry in validator configuration.";
return false;
PublicKey validationPub;
SecretKey validationPriv;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Should we name this variable validationSec or validationSecret?. It seems like we're migrating toward calling the non-public key a "secret" key instead of a "private" key.

"Invalid seed specified in [" SECTION_VALIDATION_SEED "]");
validationPriv = generateSecretKey (KeyType::secp256k1, *seed);
validationPub = derivePublicKey (KeyType::secp256k1, validationPriv);
}

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

I feel better about this code being here in the Application than I did with it in Config. How does it feel to you?

{
return mLedgerConsensus->getValidationPublicKey ();
}
void setValidationKeys (SecretKey validationPriv,

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Again, maybe this parameter should be renamed from validationPriv to validationSec or validationSecret. Or, since the method name already tells us were setting "ValidationKeys", possibly just secret, and public...?

std::string manifest;
SecretKey validationPrivate;

ValidatorToken(std::string m, SecretKey valPriv);

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

We don't expect the ValidatorToken constructor to be used by anything other than make_ValidatorToken, right? So I think the constructor should probably be private.

Do you want a copy constructor? If not you should probably declare it = delete.

Can you see if the manifest and validationPrivate members (er, should that be validationSecret?) could be const?

{
try
{
std::string tokenStr;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Do you know the maximum expected size of tokenStr? If not you could find it by traversing tokenBlob. Once you know the size you can reserve it and avoid many trips to the heap. Perhaps something like:

    tokenStr.reserve (
        std::accumulate (tokenBlob.cbegin(), tokenBlob.cend(), std::size_t(0),
            [] (std::size_t init, std::string const& s)
            {
                return init + s.size();
            }));
public:
/** Returns validation public key */
PublicKey
getValidationPublicKey () override;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Method should be const. Does this want to return a PublicKey const& (avoid a copy)?

@@ -342,11 +331,10 @@ class Subscribe_test : public beast::unit_test::suite
using namespace jtx;

// Public key must be derived from the private key
const std::string valPrivateKey =
"paEdUCVVCNnv4aYBepid9Xh3NaAr9xWRw2vh351piFJrxQwvExd";
const std::string seed = "snpTg5uPtiRG2hE8HHCAF4NzdorKT";

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Nit: usually in this code base the const comes second. That's dissimilar from most code bases I've worked in before.

"paQmjZ37pKKPMrgadBLsuf9ab7Y7EUNzh27LQrZqoexpAs31nJi");

// Format token string to test trim()
std::vector<std::string> const tokenBlob = {

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

This is a single large string in a std::vector<std::string>. Is that what you want? If you want 8 strings in the vector you need to enclose each string in {} and put commas between.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Jan 26, 2017

Author

It looks like just commas is sufficient. I get 8 for tokenBlob.size().

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 26, 2017

Contributor

Interesting. Glad that worked. Thanks for checking.

@@ -177,6 +177,9 @@ class NetworkOPs

// FIXME(NIKB): Remove the need for this function
virtual void setLastCloseTime (NetClock::time_point t) = 0;
virtual PublicKey getValidationPublicKey () = 0;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 25, 2017

Contributor

Should be const.

@scottschurr
Copy link
Contributor

left a comment

It looks like there are a couple of private -> secret conversions that I missed. I tagged those. Other than that things look good.

std::string manifest;
if (config().exists (SECTION_VALIDATOR_TOKEN))
{
if (auto const token = ValidatorToken::make_ValidatorToken (
config().section (SECTION_VALIDATOR_TOKEN).lines ()))
{
validationPriv = token->validationPrivate;
validationPub = derivePublicKey (KeyType::secp256k1, validationPriv);
valSecret = token->validationPrivate;

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 27, 2017

Contributor

Thanks for all of the renames private -> secret. It looks like I may have missed a few. How would you feel about renaming the validatorToken::validationPrivate field?

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Jan 27, 2017

Author

Do you think the "validation_private_key" json field in the token should also be renamed?
https://github.com/wilsonianb/rippled/blob/rename-manifests/src/ripple/app/misc/impl/Manifest.cpp#L176
One of the reasons I had been using "private" is that the key uses the TOKEN_NODE_PRIVATE base58 encoding.
https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/tokens.h#L33

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 27, 2017

Contributor

Legacy names I'm not so confident about, particularly if they may have leaked into our API. Mostly I'm reacting to how our newer code seems to prefer "secret" over "private". Where it's possible I'd like to continue that trend toward "secret" so we don't end up with an ugly mix. Just my take on it.

@@ -133,7 +133,12 @@ struct ValidatorToken
std::string manifest;
SecretKey validationPrivate;

ValidatorToken(std::string m, SecretKey valPriv);
private:
ValidatorToken(std::string const& m, SecretKey const& valPriv);

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 27, 2017

Contributor

The SecretKey member should maybe be names validationSecret? And maybe rename the constructor argument to valSecret?

@@ -143,7 +143,7 @@ Blob Manifest::getMasterSignature () const
return st.getFieldVL (sfMasterSignature);
}

ValidatorToken::ValidatorToken(std::string m, SecretKey valPriv)
ValidatorToken::ValidatorToken(std::string const& m, SecretKey const& valPriv)

This comment has been minimized.

Copy link
@scottschurr

scottschurr Jan 27, 2017

Contributor

Maybe rename the argument to valSecret?

@wilsonianb wilsonianb force-pushed the wilsonianb:rename-manifests branch from b6d4eda to 6199e03 Jan 30, 2017

@@ -757,30 +739,14 @@ LedgerMaster::checkAccept (uint256 const& hash, std::uint32_t seq)
}

/**
* Determines how many validations are needed to fully-validated a ledger
* Determines how many validations are needed to fully-validate a ledger

This comment has been minimized.

Copy link
@nbougalis

nbougalis Feb 1, 2017

Member

Micro-nit: remove the "-" between "fully" and "validate"

This comment has been minimized.

auto constexpr DEFAULT_REFRESH_INTERVAL = std::chrono::minutes{5};

ValidatorSite::ValidatorSite (
Application& app,

This comment has been minimized.

Copy link
@nbougalis

nbougalis Feb 1, 2017

Member

We only use app to get the IO service and once to call app_.validators(). We can cut out the dependency on the app object if we change this constructor to accept the I/O service to be used and a ValidatorList&.

The call in the Application constructor can be changed to this:

, validatorSites_ (std::make_unique<ValidatorSite> (
    get_io_service (), *validators_, logs_->journal("ValidatorSite")))

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 1, 2017

Author

Removed Application from ValidatorSite
f0940f1

wilsonianb added some commits Jul 11, 2016

wilsonianb
Dynamize trusted validator list and quorum (RIPD-1220):
Instead of specifying a static list of trusted validators in the config
or validators file, the configuration can now include trusted validator
list publisher keys.

The trusted validator list and quorum are now reset each consensus
round using the latest validator lists and the list of recent
validations seen. The minimum validation quorum is now only
configurable via the command line.
wilsonianb
Fetch validator lists from remote sites:
Validator lists from configured remote sites are fetched at a regular
interval. Fetched lists are expected to be in JSON format and contain the
following fields:

* "manifest": Base64-encoded serialization of a manifest containing the
  validator publisher's master and signing public keys.

* "blob": Base64-encoded JSON string containing a "sequence",
  "expiration" and "validators" field. "expiration" contains the Ripple
   timestamp (seconds since January 1st, 2000 (00:00 UTC)) for when the
  list expires. "validators" contains an array of objects with a
  "validation_public_key" field.

* "signature": Hex-encoded signature of the blob using the publisher's
  signing key.

* "version": 1

* "refreshInterval" (optional)
@wilsonianb

This comment has been minimized.

Copy link
Author

commented Feb 1, 2017

Squashed, rebased on the dynamic-unl branch, and renamed the last commit.
Once this is approved, I'm going to close it and add the commits to the dynamic unl pr (#1842)

@scottschurr
Copy link
Contributor

left a comment

👍 Looks good to me.

master public key to its config. Only add keys received from trusted sources.
The first value is the master public key. Any other rippled trusting the
validator needs to add the master public key to its config. Only add keys
received from trusted sources.

The second value is the corresponding master secret key. **DO NOT INSTALL THIS

This comment has been minimized.

Copy link
@nbougalis

nbougalis Feb 1, 2017

Member

We should consider changing the output of the manifest tool to include this warning, and format it in a way that makes it less likely for someone to copy-paste the whole thing in the config file accidentally.

For example:

### MASTER SECRET KEY
### PLEASE SAFEGUARD THE MASTER SECRET KEY AND DO NOT
### INSTALL IT IN THE CONFIG FILE.
###
### [master_secret]
### paamfhAn5m1NM4UUu5mmvVaHQy8Fb65bkpbaNrvKwX3YMKdjzi2

### MASTER PUBLIC KEY
[validator_keys]
nHDwNP6jbJgVKj5zSEjMn8SJhTnPV54Fx5sSiNpwqLbb42nmh3Ln

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 1, 2017

Author

I'd change it if it didn't affect the python tests.
I'm hoping to have the ripple-libpp based tool ready soon, which will remove the need to copy and paste these keys anywhere.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Feb 1, 2017

Contributor

My hope as well. The ripple-libpp based tool is much cleaner.

@wilsonianb wilsonianb added the Passed label Feb 1, 2017

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Feb 1, 2017

Moving commits to #1842. Thanks

@wilsonianb wilsonianb closed this Feb 1, 2017

@wilsonianb wilsonianb deleted the wilsonianb:rename-manifests branch Feb 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.