Boolean Blindness - Authentication uses bool flags allowing invalid states

[nachog00]

Boolean Blindness - Authentication uses bool flags allowing invalid states

Labels: configs, tech-debt, type-safety

Problem

Authentication configuration suffers from Boolean Blindness - using boolean flags combined with optional fields, creating unclear semantics and allowing invalid states.

Current Problematic Code

pub struct IndexerConfig {
    // Boolean + optional fields = confusion
    pub validator_cookie_auth: bool,
    pub validator_cookie_path: Option<String>,  // What if true + None?
    
    pub enable_cookie_auth: bool,
    pub cookie_dir: Option<PathBuf>,  // Another true + None possibility
    
    pub grpc_tls: bool,
    pub tls_cert_path: Option<String>,  // And another!
    pub tls_key_path: Option<String>,
}

// Runtime validation tries to catch invalid states
impl IndexerConfig {
    pub fn check_config(&self) -> Result<(), IndexerError> {
        // Cookie auth enabled but no path? Error!
        if self.validator_cookie_auth {
            if self.validator_cookie_path.is_none() {
                return Err(IndexerError::ConfigError(
                    "Cookie auth enabled but no path provided".to_string(),
                ));
            }
        }
        
        // TLS enabled but missing cert? Error!
        if self.grpc_tls {
            if self.tls_cert_path.is_none() || self.tls_key_path.is_none() {
                return Err(IndexerError::ConfigError(
                    "TLS enabled but cert/key missing".to_string(),
                ));
            }
        }
        // More validation...
    }
}

The confusion in practice:

// What does this mean?
let config = IndexerConfig {
    validator_cookie_auth: true,
    validator_cookie_path: None,  // Oops! Invalid but compiles
    // ...
};

// Which is the "right" way to disable?
// Option 1:
validator_cookie_auth: false,
validator_cookie_path: Some("/path/to/cookie"),  // Path ignored?

// Option 2:
validator_cookie_auth: false,
validator_cookie_path: None,  // Is None because disabled or missing?

// The intent is unclear!

Impact

• Invalid States Compile: Can create nonsensical configurations
• Unclear Intent: Multiple ways to express the same thing
• Runtime Errors: Problems only discovered when config is validated
• Documentation Burden: Must explain the relationship between fields
• Maintenance Hazard: Easy to forget to check both fields

Solution

Use enums that make the valid states explicit and invalid states impossible:

// Authentication is either enabled (with required data) or disabled
#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "lowercase")]
pub enum ZebradAuth {
    Disabled,
    Cookie { path: PathBuf },  // Path is required when using cookies!
}

#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "lowercase")]
pub enum ZcashdAuth {
    Disabled,
    Password { 
        username: String,  // Both required!
        password: String,
    },
}

// TLS configuration - clear states
#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "lowercase")]
pub enum TlsConfig {
    Disabled,
    Enabled {
        cert_path: PathBuf,  // Both required!
        key_path: PathBuf,
    },
}

Usage becomes self-documenting:

// Crystal clear intent
let config = BackendConfig {
    auth: ZebradAuth::Cookie { 
        path: PathBuf::from(".cookie") 
    },
    // or
    auth: ZebradAuth::Disabled,  // No ambiguity!
};

// TOML is also clearer
auth = "disabled"
# or
auth = { cookie = { path = ".cookie" }}

// TLS config
tls = "disabled"  
# or
tls = { enabled = { cert_path = "cert.pem", key_path = "key.pem" }}

Benefits:

• Impossible to have cookie auth = true without a path
• Single way to express each state
• No runtime validation needed for auth structure
• Self-documenting - the type explains the requirements
• Exhaustive matching ensures all cases are handled

[idky137]

One thing we need to keep in mind is zainods cookie Auth setup, this should be the default for th fetch service, with fixes in place to make sure we also work with zcashd.

[nachog00]

One great benefit of having the BackendConfig enum with variants for the different setups, is we know at compile time wether it's a zainod, a zcashd, etc. And we can have each auth be tailored to the specific case.

[idky137]

Again these details are read from a text file at runtime so we will always need the runtime checks to happen.

[nachog00]

you are right. When i say "we know at compile time wether it's a a zainod, zcashd, etc", i mean that those variants are meant for each of the different BackendConfig variants, so for each of them we can assume a bunch of stuff statically. Of course what variant is picked, has to be distinguished at runtime.

[ala-mode]

This may well be completed with #571 being merged. There's also the follow up #623. @nachog00 or @zancas please take a look at this and the other issues I've moved to 'in review' in Zaino, The Zallet Release (view)

[zancas]

@ala-mode does authentication use bool flags? If not then can't we mark this as complete?