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

Store XORed wallet seed in-mem #200

Merged
merged 21 commits into from Aug 6, 2019

Conversation

@yeastplume
Copy link
Member

commented Jul 29, 2019

Experimental, trying to figure out the most desirable way to store an XORed wallet seed and pass around the token to reconstruct it. My first inclination is to just ensure all calls to wallet.keychain() take an optional token so as ensure it's impossible to miss it anywhere in the code (as opposed to requiring an explicit call to an open(token) close(token) type function anywhere the wallet is called.

Edit: ready for merging, these are the features here.

  • Nothing should be changed from a user perspective, this is all rewiring underneath.
  • Require an Option<SecretKey> representing the mask to be passed along everywhere the keychain is needed. I think this is better than inserting open/close statements in the code (a bit more prone to bugs in future development).
  • There's a flag on open_wallet that determines whether the key gets masked. If this is false, it returns None and all wallet API calls can be made with None as the mask. If true, it returns the mask that needs to be provided.
  • A second implementation of the OwnerRpc API is provided which requires a mask to be passed in (this can be null but still needs to be named and provided). The idea is the older non-masked version of the API will still be present and usable, and the tokenized API will either be run in parallel or swapped out for the new one when the planned start_secure_api is called (will save details of that for the next PR.
  • Some tests now use a secure token, others don't (to exercise both options.)

It appears a lot of things have changed here, but the majority are just propagating the mask throughout and test updates. I'll go through and point out the interesting bits.

yeastplume added 2 commits Jul 29, 2019
@DavidBurkett

This comment has been minimized.

Copy link

commented Jul 29, 2019

I like the idea of not requiring pre-/post-conditions (open/close) on the wallet. Why optional though? It would always be required.
Also, token and keychain_mask both refer to the same thing, correct?

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Token and keychain_mask are the same, still working on naming.

Optional because a) I want to keep it as an option on the trait, rather than force other keychain implementations into using this and b) it still needs to be optional for backwards compatibility reasons with older functions, and right now it feels like it will be less hacky to have the XOR feature optional on the keychain rather than providing fake in-process tokens for the foreign API and/or directly linked wallets. Will see how it turns out and the Option can always be dropped in later versions.

yeastplume added 2 commits Jul 30, 2019
yeastplume added 6 commits Jul 30, 2019
yeastplume added 9 commits Aug 2, 2019

@yeastplume yeastplume changed the title [WIP] Store XORed wallet seed in-mem Store XORed wallet seed in-mem Aug 2, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Should be ready for reviewing, I've updated the top comment and will point out a few things in the code

@@ -67,6 +68,8 @@ where
pub doctest_mode: bool,
/// foreign check middleware
middleware: Option<ForeignCheckMiddleware>,
/// Stored keychain mask (in case the stored wallet seed is tokenized)
keychain_mask: Option<SecretKey>,

This comment has been minimized.

Copy link
@yeastplume

yeastplume Aug 2, 2019

Author Member

This is now just stored by a Foreign API instance, for the case where you want to run a foreign and owner API in the same process but only reference a single instance of the wallet.

@@ -99,6 +103,8 @@ where
data_file_dir: String,
/// Keychain
pub keychain: Option<K>,
/// Check value for XORed keychain seed
pub master_checksum: Box<Option<Blake2bResult>>,

This comment has been minimized.

Copy link
@yeastplume

yeastplume Aug 2, 2019

Author Member

This is here because an incorrectly masked key is indistinguishable from the right key. The idea here is to keep a hash of the seed so we can verify whether the stored_seed^mask is correct and return an appropriate error if so. Otherwise it'll just look to the user like their outputs have disappeared when they provide the wrong mask.

@@ -131,7 +132,13 @@ where
Ok(())
}

fn open_wallet(&mut self, _name: Option<&str>, password: ZeroingString) -> Result<(), Error> {
fn open_wallet(

This comment has been minimized.

Copy link
@yeastplume

yeastplume Aug 2, 2019

Author Member

Modified to return the mask if create_mask is true.

@chisa0a
chisa0a approved these changes Aug 4, 2019
Copy link

left a comment

LGTM. As long as the XOR key is only used against one plaintext (the master seed), no issues. Looks like a new mask is generated for a new seed right? E.g. a wallet with a stored mask that generates a new seed won't use the same mask against the new seed?

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

@chisa0a Thanks for the review, yes the XOR key is ephemeral, not stored anywhere and re-generated whenever open-wallet is called.

yeastplume added 2 commits Aug 6, 2019

@yeastplume yeastplume merged commit f8c316a into mimblewimble:master Aug 6, 2019

9 checks passed

mimblewimble.grin-wallet Build #20190806.2 succeeded
Details
mimblewimble.grin-wallet (linux config/libwallet/api) linux config/libwallet/api succeeded
Details
mimblewimble.grin-wallet (linux controller/all) linux controller/all succeeded
Details
mimblewimble.grin-wallet (linux impls) linux impls succeeded
Details
mimblewimble.grin-wallet (linux release) linux release succeeded
Details
mimblewimble.grin-wallet (macos release) macos release succeeded
Details
mimblewimble.grin-wallet (macos test) macos test succeeded
Details
mimblewimble.grin-wallet (windows release) windows release succeeded
Details
mimblewimble.grin-wallet (windows test) windows test succeeded
Details

@yeastplume yeastplume deleted the yeastplume:token_xor branch Aug 13, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

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