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

RFC - Full Wallet Lifecycle #18

Merged
merged 21 commits into from Aug 21, 2019

Conversation

@yeastplume
Copy link
Member

commented Jul 23, 2019

Replaces #11 (for git reasons, conversation in previous issue is still valid)

Adds detailed description of potential security model and sequence diagrams demonstrating.

Link to rendered doc

Tracking issue: mimblewimble/grin-wallet#212

yeastplume added some commits Jun 26, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

yeastplume added some commits Jul 24, 2019

@yeastplume yeastplume changed the title [WIP] - RFC - Full Wallet Lifecycle RFC - Full Wallet Lifecycle Jul 29, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

In accordance with our RFC process, as of today this can be considered in Final Comment Period with a disposition of merge

@quentinlesceller
Copy link
Member

left a comment

Awesome 👍. Couple of minors comment for formatting and a few typos other than that all good 👍 .


Given that the Wallet's Owner API needs to deal with sensitive data such as passwords and seed phrases, the API will be enhanced with a new ECDH and Token-based security model, the primary goals of which are to:

* Ensure sensitive data such as passwords or seed phrases are always end-to-end encrypted between the client and Owner API server, regardless of what higher-level protocols are used during the exchange.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

Missing "the" in front of Owner API server?

* Minimize the potential for damage that can be done by a third party listening on the exchange between a wallet client and its corresponding server.
* Ensure that sensitive data such as passwords or seed phrases are not resident in server-side memory any longer than they absolutely need to be.

Note that this mode of operation is primarly intended for use over the JSON-RPC API, which supports many different architectural possiblities. Clients that link libraries directly and keep all sensitive data in the same process would see less benefit from this scheme, and an alternative model which doesn't encrypt any sensitive data is provided. Further, authors of existing wallets will need time to consider and/or implement the added complexity needed on the client-side to support ECDH and encryption. It's therefore proposed that the Owner API initially provide the new "SecureAPI" mode as an optional feature, with wallet authors strongly encouraged to make use of it. Support for the "InsecureAPI" model can be maintained indefinitely for directly-linked wallets, and for the JSON-RPC API until a cut-off release at some point in the future.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

Note that this mode of operation is primarly intended

Not English native but isn't it primarily?

Also

architectural possiblities

architectural possibilities.

Missing "s" at:

It's therefore proposed that the Owner API initially provide


![init_api_secure](../assets/0000/0000-dh-init.svg)

Once `init_api_secure` is called the API will assume that subsequent API requests will be encrypted with the shared secret, and that that all subsequent API calls (other than `open_wallet`) will be accompanied with a valid encrypted token derived during the call to the `open_wallet` function. This assumption will remain until the server process exits or a call to a corresponding `close_api_secure` function is called.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

Missing comma after "Once init_api_secure is called". Not sure about the need for a comma before the ", and that that all".


##### Legacy support

If `init_api_secure` is not called, the mode of operation for the JSON-RPC API will be assumed to work as currently, i.e. requests and responses are unencrypted, the wallet stores its full seed in-memory between requests and the providing of a Token with each request is not requred. However, the new lifecycle functions described in this RFC, which deal with highly sensitive data such as passwords and master keys, will not be available via the JSON-RPC API unless `init_api_secure` has been called. This setup should allow existing wallets to continue working as-is until a cutoff release for legacy mode is determined.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

providing of a Token with each request is not requred

providing of a token with each request is not required.


#### Opening a Wallet in SecureAPI Mode

Opening a wallet in SecureAPI mode consists of encrypting a request to `open_wallet` (which contains the wallet password) with the shared secret `s`. The request is decrypted in the JSON-RPC layer, and the password is used in the wallet backend to unlock the wallet master seed. The master seed is stored XORed against a randomly-generated token T, which is returned to the client in an encrypted response for inclusion in all further API calls. T is valid for the lifetime of the process, or until a corresponding call to `close_wallet`.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

The request is decrypted in the JSON-RPC layer, and the password

Unnecessary comma?

process, or until a corresponding call to close_wallet.

Same comment.


### Directly Linked Wallets

Wallets that link the wallet API directly will not be required to encrypt parameters, as there would be little benefit to doing so within a single process. However, for consistency they will be expected to store and supply a token to each API call. The modified workflow for a linked wallet is outlined below:

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

However, for consistency they...

However, for consistency, they...

![open-wallet-direct](../assets/0000/0000-open-wallet-direct.svg)
![api-call-direct](../assets/0000/0000-api-call-direct.svg)

'Legacy' support will not be provided for directly-linked wallets on release of the features described in this RFC. Is is expected wallet authors will need to update their code to store and supply the token with each request.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

Is is expected wallet authors

It is expected wallet authors


The functions as shown here are for illustrative purposes, and their signatures will change during implementation.

* `OwnerAPI::init_api_secure(pubkey: Secp256k1Point) -> Result<pubkey: Secp256K1Point, Error>

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

Missing ` at the end of the line.


Although this document doesn't attempt to outline implementation, a few notes to consider for the implementor:

* Currently, the code that deals with wallet initialization and seed management sits outside the wallet APIs, in the `impls` crate, (denoting they're implementation specific). The implementation should attempt to refactor traits from these hard implementations into a new interface, similar to the existing WalletBackend and NodeClient interfaces (WalletLifecycleManager, for instance). The implementation within `impls` will then become an implementation of that trait, and can be substituted by wallet authors with their own implementations.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

(WalletLifecycleManager, for instance)

Why comma here?

trait, and can

Same

- The 'name' parameter is included for future use as in `open_wallet` above.
* `OwnerAPI::open_wallet(name: Option<String>, password: String) -> Result<t:Token, libwallet::Error>`
- Opens a wallet and sets it as the 'active' wallet. All further API commands will be performed against this wallet.
- 'Opens' the wallet seed in memory, stored XORd against a new token. The token is to be returned to the client for use in all further API calls.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Aug 20, 2019

Member

XORd

XORed.

yeastplume added some commits Aug 21, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Thanks! Comments noted and doc updated.

yeastplume added some commits Aug 21, 2019

@yeastplume yeastplume merged commit 3fc8d75 into mimblewimble:master Aug 21, 2019

@lehnberg lehnberg referenced this pull request Aug 21, 2019
0 of 4 tasks complete
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.