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

Sign manifest with ephemeral and master keys (RIPD-1083) #1865

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@wilsonianb
Copy link

commented Oct 5, 2016

No description provided.

@codecov-io

This comment has been minimized.

Copy link

commented Oct 5, 2016

Current coverage is 64.44% (diff: 69.56%)

Merging #1865 into develop will increase coverage by 0.01%

@@            develop      #1865   diff @@
==========================================
  Files           683        683          
  Lines         48973      48987    +14   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          31554      31569    +15   
+ Misses        17419      17418     -1   
  Partials          0          0          

Powered by Codecov. Last update aa11eff...b5f7ddc

@scottschurr
Copy link
Contributor

left a comment

I'd like to take another run at this review after doc/manifest-tool-guide.md is updated to match. Then I can exercise the process. Fair?

SF_Blob const sfMemoType = make::one<SF_Blob::type>(&sfMemoType, STI_VL, 12, "MemoType");
SF_Blob const sfMemoData = make::one<SF_Blob::type>(&sfMemoData, STI_VL, 13, "MemoData");
SF_Blob const sfMemoFormat = make::one<SF_Blob::type>(&sfMemoFormat, STI_VL, 14, "MemoFormat");
SF_Blob const sfMasterSignature = make::one<SF_Blob::type>(&sfMasterSignature, STI_VL, 15, "MasterSignature", SField::sMD_Default, SField::notSigning);

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 6, 2016

Contributor

If I recall correctly, encodings 1 - 15 take fewer bytes than encodings in the range 16 and up. I suggest using the code 18 (just above sfProof) instead of 15, since I don't think sfMasterSignature will be a commonly used field. Unfortunately that would entail also changing the python...

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 10, 2016

Author

Changed the code to 18

Create a new signed manifest with the given sequence
number, validator public key, and master secret key.
number, validation public key, and master public key.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 6, 2016

Contributor

I'm trying to follow the directions in doc/manifest-tool-guide.md and I'm getting lost. There are a lot of keys flying by here. I'd like to suggest that, as far as humanly possible, we always use exactly the same name for a give type of key. Here are the names I think we can't escape because they are in the config or are otherwise human facing:

  • The validator_key.
  • The master_secret.
  • The validation_public_key. Also called the "ephemeral public key".
  • The validation_seed. Also called the "ephemeral secret key".

If we can limit ourselves to those 6 names and always use the same spelling (in both C++ and Python, especially in the UI visible text, and please don't replace _ with - or a space) I think we'll end up way ahead.

What I'd like to suggest is that you make a pass through doc/manifest-tool-guide.md and update it to match the new process. After you've done that I'll attempt to follow the process using the updated doc. (By the way, there's an odd sentence fragment on line 67 of the doc: "sequence numbers are be explained below". You may want to fix that as well.)

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 6, 2016

Author

I totally overlooked doc/manifest-tool-guide.md. Sorry about that.
I currently have the manifest tool using validation_private_key instead of validation_seed primarily because I wasn't having success deriving the ecdsa keypair from the seed in python.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 6, 2016

Contributor

No problem. The only reason I knew the manifest-tool-guide was there was from my previous puzzlement and fumbling with manifests. And whichever key you need to use is fine with me. We just need to be able to keep the names straight enough that a user can follow the instructions once every 9 months to 3 years when they need to change the ephemeral key. Thanks for your understanding.

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 10, 2016

Contributor

I took a stab at cleaning up Sign.py. I replaced private key with sk, for secret key, and public key with pk. I also always prefixed these keys with either validator for the ephemeral keys or master for the master keys. I also added python docs to two of the functions. We should start adding those docs when we change code.

This is for your consideration, and of course should be reviewed (I only checked the unit tests still ran):

Edit: Use this commit instead (I accidentally left a comment in the last commit that I didn't mean to check in): seelabs@5a2c14f

This comment has been minimized.

Copy link
@wilsonianb

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 10, 2016

Author

I've updated manifest-tool-guide.md and cherry-picked @seelabs 's renaming commit. I further renamed validator to validation since rippled validation_create returns validation_public_key and validation_private_key

"\t794ZpMdkC+8l5n3R/CHP6SAwhYDOaqub0Cs2NjjewBnp1mf\n"
"\t 23rhAzdcjRuWzm0IT12eduZ0DwcF5Ng8rAelaYP1iT93ScE\t \t";
" JAAAAAFxIe1gWiRDgypRVdw3juSr1rintFBx8FpS7kk5ZEgA\n"
" \hg1wuHMhAuUTyasIhvj2KPfNRbmmIBnqNUzidgkKb244eP79 \n"

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 7, 2016

Member

\h? Something is wrong here.

This comment has been minimized.

Copy link
@wilsonianb
PublicKey const& pk,
bool mustBeFullyCanonical)
verify (STObject const& st, HashPrefix const& prefix,
PublicKey const& pk, bool mustBeFullyCanonical,

This comment has been minimized.

Copy link
@nbougalis

nbougalis Oct 7, 2016

Member

So I noticed that mustBeFullyCanonical is unused. We should just remove it. You can cherry-pick 2480301 if you want. Or I can do a separate pull request, after this is merged. Up to you.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 10, 2016

Author

Cherry-picked

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 10, 2016

The steps for creating a manifest are currently:

  1. bin/manifest create to get master keys
  2. rippled validation_create to get ephemeral signing keys
  3. bin/manifest sign <seq> <validation_public_key> <validation_private_key> <master_secret> to get manifest

Since manifest signing now requires the ephemeral validation private key, I'm thinking we can simplify this and ditch step 2. bin/manifest sign <seq> <master_secret> could generate the ephemeral signing keys and then output both [validation_seed] and [validation_manifest] (the two fields that need to be added to the config).

The problem is I don't think we currently have our ecdsa keypair generation implemented in Python (and I'm not comfortable writing it), and rippled currently assumes [validation_seed] is for a secp256k1 key.
https://github.com/ripple/rippled/blob/develop/src/ripple/core/impl/Config.cpp#L364-L365

A possible solution is to have the manifest tool generate ed25519 ephemeral keys. Then rippled can check the ephemeral public key's key type in [validation_manifest] to determine if [validation_seed] is for ed25519 keys.

Thoughts?

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 10, 2016

Hrm, STValidation::isValid() calls verifyDigest which only supports secp256k1 signatures. So more changes in rippled would be required to support ed25519 ephemeral validation keys.
https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/PublicKey.cpp#L220-L221

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

I have a couple of hair-brained ideas; I'm not sure they fully qualify as thoughts.

  1. When I was generating manifests (about a year ago) I found the process felt hacked together and disorganized. Some parts came from rippled. Other parts came from this python script. I had to build myself a detailed check list and follow my list closely to get the process right.
  2. Anything we can do to simplify the process (without compromising security) seems like a good idea. This is an activity that our customers will have to perform either when initially setting up a server or under times of churn (when changing the ephemeral key). In particular if we could have a single tool that does the whole job then the process would be simpler. However, I believe there's also a drive to keep extraneous helper facilities out of rippled. I suspect that's how the python manifest tool came into existence in the first place.
  3. It sounds like Python doesn't (right now) support some of the crypto that we'd like to use in the manifest tool.
  4. This leads me to wonder if the manifest tool would be a good internal consumer of the C++ rippled support library that @ximinez is putting together. I suspect the library provides the right crypto interfaces. And this way the library could, at least in part, be designed against an internal tool.

I don't know the current state of the library. I also can't say positively that it makes huge sense to re-write the manifest tool in C++. But I thought it made sense to at least broach the subject.

@wilsonianb wilsonianb force-pushed the wilsonianb:manifest-ephemeral-sign branch from b73ff7e to 90483f3 Oct 10, 2016

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 10, 2016

I like the idea of replacing the Python manifest tool with one using ripple-libpp, @scottschurr
I'm thinking we'll keep our hacky manifest generation process for now but open a ticket to use ripple-libpp for manifest key generation and signing.

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

Link to documentation preview?

SField.sfSigningPubKey,
prepend_length_byte(validation_pk)])

def sign_manifest(manifest, validation_sk, master_sk, master_pk):

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 10, 2016

Contributor

Can we either check that the master public key pairs with the master secret key, or derive the public key from the secret key and not pass the public key?

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 10, 2016

Author

Those are good ideas, however I'm not having success getting either to work.
I think this is the missing functionality that we'd need in Python:
https://github.com/ripple/rippled/blob/develop/src/ripple/crypto/impl/GenerateDeterministicKey.cpp#L119

@seelabs

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

The code looks good to me. I do have some general comments:

  1. I also like the idea of using rippled as a lib.

  2. This needs to be tested with Scott S's testing scripts to test updating ephemeral keys.

  3. I don't see any transition from the old manifests to this new manifests. It appears once this is activated, all validators that used ephemeral keys become untrusted. Can we have a transition period where: 1) Both new and old manifests are accepted; 2) Give anyone running a validator warning that they must update their ephemeral keys to the new scheme; 3) Transition to the new scheme after either an amendment or date (probably amendment) and no longer accept the old manifests.

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

@seelabs I don't know that amendments is the right way to do this, because the gossip protocol is not strictly part of the consensus protocol. There's nothing in the ledger having to do with manifests.

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 11, 2016

I think our data collection can give us an idea if manifests are being used outside the altnet.
If they are, we could encourage validator operators to upgrade promptly and restart with a manifest in the new format using the same ephemeral keys. The only ones who would have an issue are rippleds who upgrade before any manifest-using validators they trust.

@scottschurr
Copy link
Contributor

left a comment

This feels like its getting better. Nice work with manifest-tool-guide.md.

Do we believe manifests are being used nowhere other than the AltNet? If that's true that would make the transition easier. But, still, we should probably have an official transition plan written up since people other than you, Brandon, will have to implement the transition.

verify <sequence> <validator-public> <signature> <master-public>
Verify hex-encoded manifest signature with master public key.
number, validation public key, and master public key.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

The sentence following the list of arguments doesn't look right. Shouldn't the "master public key" be "master secret"? We could either fix the list in the sentence or change the sentence to something like "Create a new signed manifest based on the passed arguments." I'm not convinced that listing the arguments twice adds value.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 11, 2016

Author

-> Create a new signed manifest with the given sequence number and keys.

JAAAAAFxIe3t8rIb4Ba8JHI97CbwpxmTq0LhN/7ZAbsNaSwrbHaypHMhAzTuu07YGOvVvB3+
aS0jhP+q0TVgTjGJKhx+yTY1Da3ddkYwRAIgDkmIt3dPNsfeCH3ApMZgpwqG4JwtIlKEymqK
S7v+VqkCIFQXg20ZMpXXT86vmLdlmPspgeUN1scWsuFoPYUUJywycBJAl93+/bZbfZ4quTeM
5y80/OSIcVoWPcHajwrAl68eiAW4MVFeJXvShXNfnT+XsxMjDh0VpOkhvyp971i1MgjBAA==
```

Copy this to the config for this validator. Don't forget to update the comment

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

You didn't introduce this, but I suggest removing the commas from the number on line 112. If they use commas in the sign sequence argument they will get an invalid literal error from Python.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 11, 2016

Author

commas removed

Parameters
----------
master_pk : string
validator's master public key (_not_ BASE58 encoded)

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

I like the PyDoc, but no good deed should go unpunished...

The _not_ is probably useful, but it would be even better to note what the encoding actually is. Perhaps this is true: "(binary encoded, not BASE58)"? Whatever is the correct statement for that clause could probably replace the other places in this file that specify "(not BASE58 encoded)". I found 5 of 'em.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 11, 2016

Author

-> (binary, _not_ BASE58 encoded)

@@ -59,13 +52,17 @@ def test_make_manifest(self):
'\xef*\x97\x16n<\xa6\xf2\xe4\xfb\xfc\xcd\x80P[\xf1s\x06verify')

def test_sign_manifest(self):
ssk, spk = Sign.make_ecdsa_keypair(self.urandom)

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

I tried to figure out how to run test_Sign.py and came up empty. If I invoke the test as "python test_Sign.py" I get:

Traceback (most recent call last):
  File "test_Sign.py", line 3, in <module>
    from ripple.util import Sign
ImportError: No module named ripple.util

So I tried running ValidatorManifestTest.py to see if that would help. I got a long error message ending in:

...
  File "ValidatorManifestTest.py", line 251, in startup
    self.to_run, stdout=fout, stderr=subprocess.STDOUT)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child
    raise child_exception
TypeError: execv() arg 2 must contain only strings

Perhaps we need to document how to run the tests? Or did I miss something blindingly obvious?

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 11, 2016

Contributor

There's a README.md in the python directory. The incantation is python -m unittest discover. Point taken though about making that more obvious.

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

Thanks for the pointer @seelabs. I tried it and all of the python unit tests passed. There's not a lot of output from passing tests (probably a design feature) so I'll have to take your word that running python -m unittest discover exercises the test_Sign.py test.

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 11, 2016

Contributor

Good point. Maybe hack the test so it fails and make sure it fails? (Edit: I did that myself and it does report a failure if you do so)

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Oct 11, 2016

Author

I can also attest to test_Sign.py tests failing when running python -m unittest discover

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

Great suggestion @seelabs. I hacked test_sign_manifest and it did indeed fail. Comforting.

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 11, 2016

Contributor

For future reference, the -v flag will output a more verbose running of the tests. So python -m unittest discover -v will show you what tests are being run. To run an individual test: python -m unittest ripple.util.test_Sign

This comment has been minimized.

Copy link
@seelabs

seelabs Oct 11, 2016

Contributor

@wilsonianb Can you add a note in the python README.md documenting how to run an individual test and documenting the verbose flag with discover?

This comment has been minimized.

Copy link
@wilsonianb
JAAAAAFxIe3t8rIb4Ba8JHI97CbwpxmTq0LhN/7ZAbsNaSwrbHaypHMhAzTuu07YGOvVvB3+
aS0jhP+q0TVgTjGJKhx+yTY1Da3ddkYwRAIgDkmIt3dPNsfeCH3ApMZgpwqG4JwtIlKEymqK
S7v+VqkCIFQXg20ZMpXXT86vmLdlmPspgeUN1scWsuFoPYUUJywycBJAl93+/bZbfZ4quTeM
5y80/OSIcVoWPcHajwrAl68eiAW4MVFeJXvShXNfnT+XsxMjDh0VpOkhvyp971i1MgjBAA==

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

Nicely done with manifest-tool-guide.md. I was able to successfully follow the instructions from start to finish.

bool mustBeFullyCanonical);
verify (STObject const& st, HashPrefix const& prefix,
PublicKey const& pk,
SF_Blob const& sigField = sfSignature);

This comment has been minimized.

Copy link
@scottschurr

scottschurr Oct 11, 2016

Contributor

Thanks for getting rid of the mustBeFullyCanonical bool. Those flags always give me the creeps, since I'm never quite sure when they should be true or false.

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2016

So, in terms of a testing and release plan, Let's see if this holds water:

  1. We get the AltNet and Scott D's validators running your software, but without manifests. I'm under the vague impression that will require restarting the AltNet and giving all of the validators new keys? Or is there a less drastic approach to taking the AltNet off of validator manifests?
  2. For testing we add manifests to the AltNet validators. We probably don't have to repeat all of the testing I did. Simply putting manifests on the validators one-at-a-time and watching them re-sync should be sufficient. And we should probably successfully roll the ephemeral key on one AltNet validator.
  3. Once the AltNet testing is done, we release the new code onto the part of the network we control -- all of our client handlers and validators.
  4. Then we give folks running rippled servers a heads up that we're going the switch our validators over to running with manifests. We give them a reasonable amount of time to switch to the appropriate software version.
  5. One at a time, we switch all of our validators over to running manifests.

Does that seem reasonable?

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 11, 2016

1-3 sound good. I think we can just upgrade altnet validators one at a time and give each a new manifest with the same ephemeral keys.
I think 4-5 will be easiest if we wait for everyone to use the dynamic UNL #1842

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2016

Okay, I think I'm good. The code and documentation looks reasonable to me. There's a plan for testing and release onto the network. 👍

@seelabs

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2016

👍 I'm good with this too, though we should make sure Vinnie is OK with the upgrade plan before we mark this passed.

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2016

Yup. Both Vinnie and Warren.

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 17, 2016

Nik's requested changes have been addressed, and Vinnie has approved the plan.

@wilsonianb wilsonianb added the Passed label Oct 17, 2016

@nbougalis

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

👍

@wilsonianb wilsonianb force-pushed the wilsonianb:manifest-ephemeral-sign branch from ac542be to b5f7ddc Oct 17, 2016

@ximinez ximinez referenced this pull request Oct 17, 2016

Merged

Proposed 0.40.0-b8 #1874

@ximinez

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

Merged to develop as b55edfa and 027b289.

@ximinez ximinez closed this Oct 18, 2016

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 20, 2016

I just realized the transition to the new manifests could be a lot simpler if we keep the master key signature in sfSignature and add the new ephemeral key signature to the new field (probably with a different name than sfMasterSignature). I think this would make the new manifests backwards compatible since older versions of rippled would only verify the manifest key signature in sfSignature and wouldn't care about the added signature field (I could be wrong there).
We're probably fine as is, but this is an option if we realize we're not.

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Naively, backward compatibility seems like a good thing since it would require less coordination across the network to deploy the feature, since we can't use an amendment. And very old versions of rippled will become amendment blocked by some other feature, so they will eventually be forced to upgrade. Does that make sense?

@wilsonianb

This comment has been minimized.

Copy link
Author

commented Oct 20, 2016

That makes sense.
Unfortunately, switching the signature fields might not actually make the new manifests backward compatible. I just tried putting a new manifest (with the master key signature in sfSignature) in the config of a rippled without the new functionality (pre-0.40.0-b8), and it failed to load the manifest.

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Trying it is often illuminating...

@wilsonianb wilsonianb deleted the wilsonianb:manifest-ephemeral-sign branch Nov 29, 2016

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.