Rethink Default configuration

[mversic]

Description

At the moment when deserializing config files into config structures if the field is missing the field will be taken from the Default implementation of the structure. E.g. for cli::Configuration if private_key is missing in the config.json file and is not defined as a IROHA_PUBLIC_KEY env variable, it's be taken from the cli::Configuration::Default. Needless to say, this a big potential security issue. In this case a more prudent course of action would be to fail starting Iroha.

We can also say this happens because we implement Default for structures which don't have reasonable defaults

Cause

This happens because configuration structures have serde(default) attached to them

Solution

1. Remove serde(default). This is a hazzard waiting to happen. However, if the annotation is removed, deserialization from a file would fail even if env variable defines the value. This suggests that deserializing from file and from environment have to be combined into a single step - they cannot be independent steps.
2. Maybe it would be possible to use serde(default = "path") to force it to try to deserialize from environment if deserialization from file fails. It should be investigated which kinds of fallbacks serde offers
3. Consider removing Default implementation from configurations which don't have a sensible default such as cli::Configuration(because there is no meaningful default private_key) and rather rely on serde(default = "path") (or implement custom config(default = "path") attached on a field, not a whole container. This way default values will be defined only for fields which have sensible defaults.

[mversic]

@appetrosyan suggests:

Have you considered transparent wrappers with unique impl Default? Adding the delineation at the type level as well as in documentation?

[mversic]

I would say, let's remove implementation of Default for cli::Configuration, client::Configuration, genesis::Configuration and sumeragi::Configuration (because there isn't a sensible default, at least for the private key) and use #[serde(default)] attached to fields which have a default. It's no big deal to force users, devs, deops to define a few keys when starting Iroha

[mversic]

For structures which don't have a sensible default because of a minor number of fields (as is the case of client::Configuration it may be cumbersome to construct them if all you need to do is set one field and use the defaults for all others. I'm not sure anyone would be constructing client::Configuration by hand instead of deserializing it from a file, but if that happens to be the case I would suggest a builder is added for such structures. E.g:

#[derive(Debug, Clone, Deserialize, Serialize, Configurable)]
#[serde(rename_all = "UPPERCASE")]
#[config(env_prefix = "IROHA_")]
pub struct Configuration {
    /// Public key of the user account.
    #[config(serde_as_str)]
    pub public_key: PublicKey,
    /// Private key of the user account.
    pub private_key: PrivateKey,
    /// User account id.
    pub account_id: AccountId,
    /// Basic Authentication credentials
    pub basic_auth: Option<BasicAuth>,
    /// Torii URL.
    pub torii_api_url: SmallStr,
    /// Status URL.
    pub torii_telemetry_url: SmallStr,
    /// Proposed transaction TTL in milliseconds.
    pub transaction_time_to_live_ms: u64,
    /// Transaction status wait timeout in milliseconds.
    pub transaction_status_timeout_ms: u64,
    /// Limits to which transactions must adhere to
    pub transaction_limits: TransactionLimits,
    /// If `true` add nonce, which make different hashes for transactions which occur repeatedly and simultaneously
    pub add_transaction_nonce: bool,
}


pub struct ConfigurationBuilder {
    public_key: Option<PublicKey>,
    private_key: Option<PrivateKey>,
    account_id: Option<AccountId>,
    basic_auth: Option<BasicAuth>,
    torii_api_url: SmallStr,
    torii_telemetry_url: SmallStr,
    transaction_time_to_live_ms: u64,
    transaction_status_timeout_ms: u64,
    transaction_limits: TransactionLimits,
    add_transaction_nonce: bool,
}

impl ConfigurationBuilder {
    fn new() -> Self {
        Self {
            public_key: None,
            private_key: None,
            account_id: None,
            basic_auth: None,
            torii_api_url: small::SmallStr::from_str(uri::DEFAULT_API_URL),
            torii_telemetry_url: small::SmallStr::from_str(DEFAULT_TORII_TELEMETRY_URL),
            transaction_time_to_live_ms: DEFAULT_TRANSACTION_TIME_TO_LIVE_MS,
            transaction_status_timeout_ms: DEFAULT_TRANSACTION_STATUS_TIMEOUT_MS,
            transaction_limits: TransactionLimits {
                max_instruction_number: transaction::DEFAULT_MAX_INSTRUCTION_NUMBER,
                max_wasm_size_bytes: transaction::DEFAULT_MAX_WASM_SIZE_BYTES,
            },
            add_transaction_nonce: DEFAULT_ADD_TRANSACTION_NONCE,
        }
    }

    fn build(self) -> Result<Configuration, Error> {
        if self.public_key.is_none() || self.private_key.is_none() || self.account_id().is_none() {
            return Err(Error::ConfigError);
        }

        Ok(Configuration {
            public_key: self.public_key.unwrap(),
            private_key: self.private_key.unwrap(),
            account_id: self.account_id.unwrap(),
            basic_auth: self.basic_auth,
            torii_api_url: self.torii_api_url,
            torii_telemetry_url: self.torii_telemetry_url,
            transaction_time_to_live_ms: self.transaction_time_to_live_ms,
            transaction_status_timeout_ms: self.transaction_status_timeout_ms,
            transaction_limits: self.transaction_limits,
            add_transaction_nonce: self.add_transaction_nonce,
        })
    }
}

fn main() {
    let client_config = ConfigurationBuilder::new()
        .with_public_key("pub_key")
        .with_private_key("priv_key")
        .with_account_id("acc_id")
        .build();
}

I would say this way you get all the benefits and a good user experience without having a Default implementation

[mversic]

sdk devs would like us to get rid of the defaults:

@Mingela right, this is exactly what I referred to, the service proxies an error message from Iroha: no domain 'wonderland', but if you open genesis.json the domain is defined there, so I conclude the genesis is not being used and the default one is used (inside Iroha itself)

[appetrosyan]

Extending this idea. Instead of deserialising the configuration directly, we could have a proxy object which has all of its fields optional, e.g. ConfigurationProxy.

It then implements the Override trait,

pub trait Override {
    fn override(self, other: Self) -> Self {
         // For each field e.g.
         if let Some(field_0) = other.field_0 {
              self.field_0 = Some(field_0)
         }
    }
}

We deserialise both from the config.json, and ENV, run file.override(env) and then call build() on the proxy.

[Erigara]

I like idea about proxy object, i think it would work nicely with configuration views introduced in #2435.
Loading configuration parameters from configuration view (received from bootstrap peer) would be just another call to override.

[ilchu]

Closing it as discussion is finished and implementation will be tracked by the #2585 epic.