Dangerous failure mode regarding passphrase field within config

[Alrighttt]

I've come across a scenario where a typo in a field name within the JSON config passed to KDF at startup can lead to a user losing funds.

Consider the following config:

{
    "wallet_name": "debugging",
    "wallet_password": "password",
    "passphraase": "neutral neutral neutral neutral neutral neutral neutral neutral neutral neutral neutral noise",
    "enable_hd": true,
[...]
}

Notice the typo in the passphrase field name.

The intended behavior for generating a new seed is: provide a wallet_name and wallet_password, and omit the passphrase field entirely.

I did the following while running tests, and I believe it's a plausible way a user could lose funds.

1. Started KDF with a config similar to the one above.
2. Enabled coins
3. Funded the address returned in the enable_utxo response.
4. Performed some test swaps.
5. Shut down KDF after test swaps executed as expected.
6. Deleted the entire DB directory, believing I had my mnemonic backed up via the saved JSON config.
7. Started KDF expecting to have funds only to realize KDF had actually generated a new seed, saved it to disk, and I had deleted it thinking I had just transacted using my seed within the JSON.

A quick and obvious fix to this problem is to use #[serde(deny_unknown_fields)] while deserializing the config.

Additionally, I believe generating a new seed should be more intentional. Could we add a startup argument for this and remove the current method? eg, ./kdf generate_seed where the binary generates the seed, returns it or saves it and exits without actually starting KDF.

[Alrighttt]

I suggest we replace the conf field of MmCtx with a strong typed struct instead of using json::Value. This will provide compile-time validation.

For example:

fn default_true() -> bool { true }

#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
pub struct MmConfig {
    #[serde(default)]
    pub gui: String,
    pub rpcport: u16,
    pub https: bool,
    pub rpcuser: String,
    pub rpcpassword: String,
    #[serde(default)]
    pub allow_weak_password: bool,
    #[serde(default = "default_true")]
    pub enable_hd: bool,
    #[serde(default)]
    pub passphrase: Option<Bip39Seed>,
    pub netid: u16,
[...]
}

If we want to be precise, we could strongly type each field and constrain what data each type can hold via their Deserialize implementation and their ::new() method.

For example:

#[derive(Debug)]
pub struct NetId(u16); // inner field is intentionally private

impl NetId {
    /// Creates a new NetId if it is within the valid range.
    pub fn new(value: u16) -> Result<Self, String> {
       // arbitrarily constrain what data this type can store
       // eg, value != BANNED_NETID
       // eg, enforce a valid range
        if value <= 5 || value >= 1000 {
            Err(format!("netid {} is out of range (must be between 3 and 777)", value))
        } else {
            Ok(NetId(value))
        }
    }

    /// expose the inner value via a method
    pub fn get(&self) -> u16 {
        self.0
    }
}

impl<'de> Deserialize<'de> for NetId {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        let raw = u16::deserialize(deserializer)?;
        return Ok(NetId::new(raw)?); // assume this Into conversion works for the example
    }
}

This is preferable because we could have data guarantees at a glance. This is particularly useful pattern untrusted p2p data and user input.