Some questions about the codebase

[33KK]

I've been skimming through the codebase while configuring this to figure out what were the default config values, and noticed that there's a custom toml parser implementation, is there any reason to do that instead of defining some structs with derived serde::Deserialize and parse the config using the toml crate?

Same for the JMAP parser, I'm not sure why not just use serde and something like serde::Value::pointer + serde::Value for the JMAP result references.

These parsers make it much harder to understand the codebase, though in the case of JMAP I can somewhat understand it, if there's a performance benefit to doing it this way.

Another thing are the OAuth tokens, why not just use PASETO? It's generally better to not make custom crypto. Token revocation is a bit complicated though, but if you plan to do individual token revocation, that likely would involve storing information about the tokens in the database, at which point you might aswell just use a randomly generated string stored in the database.

EDIT: right, also the current situation with the config is a bit weird, there often aren't any clear defaults when I tried removing the column mappings thing, I assumed that it would just default to the values in the example, but that wasn't the case apparently. All of these config options would be much easier to keep track of if they were a struct instead of using toml,get("path.to.property") everywhere and unwrapping it with the default value.

[mdecimus]

I've been skimming through the codebase while configuring this to figure out what were the default config values, and noticed that there's a custom toml parser implementation, is there any reason to do that instead of defining some structs with derived serde::Deserialize and parse the config using the toml crate?

The custom TOML parser builds a key/value hashmap which allows to easily parse rules and other configuration settings. All TOML parsers I checked did not have this option.

These parsers make it much harder to understand the codebase, though in the case of JMAP I can somewhat understand it, if there's a performance benefit to doing it this way.

In the case of JMAP, it was for performance and because serde could not gracefully recover from errors within a nested JMAP structure. Version 0.2 of Stalwart used serde and a custom parser was implemented for these reasons.

Another thing are the OAuth tokens, why not just use PASETO? It's generally better to not make custom crypto. Token revocation is a bit complicated though, but if you plan to do individual token revocation, that likely would involve storing information about the tokens in the database, at which point you might aswell just use a randomly generated string stored in the database.

A simple OAuth token was needed and PASETO is a bit too bloated for what Stalwart needs No custom crypto is being done though, tokens use AES256_GCM_SIV along with BLAKE3.

[33KK]

The custom TOML parser builds a key/value hashmap which allows to easily parse rules and other configuration settings. All TOML parsers I checked did not have this option.

Is that necessary though? I think it should be pretty easy to parse rules with enums using something like this, maybe even merge actual rule resolution into this enum

#[serde(untagged)]
#[serde(rename_all = "kebab-case" )]
enum Rule {
  IfEq { if: String, eq: String },
  IfStartsWith { if: String, starts_with: String },
  AnyOf { any_of: Vec<Rule> },
  ...
}

Or worst-case using toml::Table which is basically a key-value map https://docs.rs/toml/0.7.6/toml/type.Table.html or a combination of structs with toml::Table as a property

In the case of JMAP, it was for performance and because serde could not gracefully recover from errors within a nested JMAP structure. Version 0.2 of Stalwart used serde and a custom parser was implemented for these reasons.

What kind of errors exactly? Not exactly sure why it should even try to recover from an error

A simple OAuth token was needed and PASETO is a bit too bloated for what Stalwart needs No custom crypto is being done though, tokens use AES256_GCM_SIV along with BLAKE3.

I've looked at the implementation and I think the best way to go would be to just store the session tokens in the database. There's already a queries being made to fetch the user and password hashes, this would easily allow for revocation and doesn't need any additional crypto on top of database requests which also should be slightly more efficient and have less potential for issues. Since you're basically already making a request to the database thus making the session stateful anyway, this is basically only increasing attack surface and complexity

[33KK]

Here's a working example from the docs:

use serde::{Serialize, Deserialize};

#[derive(Debug, Serialize, Deserialize)]
struct Config {
    chunking: Vec<Rule<Option<bool>>>,
}

#[derive(Debug, Serialize, Deserialize)]
#[serde(untagged)]
#[serde(rename_all = "kebab-case" )]
enum Rule<T> {
    #[serde(rename_all = "kebab-case" )]
    IfEq { r#if: String, eq: toml::Value, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    IfNe { r#if: String, ne: toml::Value, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    IfStartsWith { r#if: String, starts_with: String, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    IfEndsWith { r#if: String, ends_with: String, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    AnyOf { any_of: Vec<Rule<T>>, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    AllOf { all_of: Vec<Rule<T>>, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    NoneOf { none_of: Vec<Rule<T>>, then: Option<T> },
    #[serde(rename_all = "kebab-case" )]
    Else { r#else: Option<T> },
    Value(T),
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let value: Config = toml::from_str(r#"
chunking = [
	{ any-of = [
		{ if = "rcpt-domain", eq = "example.org" },
		{ if = "remote-ip", eq = "192.168.0.0/24" },
		{ all-of = [
			{ if = "rcpt", starts-with = "no-reply@" },
			{ if = "sender", ends-with = "@domain.org" },
			{ none-of = [
				{ if = "priority", eq = 1 },
				{ if = "priority", ne = -2 },
			] }
		] }
	], then = false },
	{ else = true }
]
    "#)?;
    println!("{:#?}", value);
    
    Ok(())
}

[33KK]

Deserializing using an enum of Comparator should also be possible using visit_map: https://serde.rs/deserialize-struct.html

[33KK]

Deserializing using an enum of Comparator should also be possible using visit_map: https://serde.rs/deserialize-struct.html

TIL the hard way after several hours and several hundred open tabs: serde untagged enum error messages are literally useless and they refused to do anything about it, "just implement a visitor yourself", even though the data types used for buffering data in untagged enums are unstable internal APIs ( serde-rs/serde#2376 serde-rs/serde#1544 ), not merging this literally feels insane to me considering just how much better that would have been, the example in the PR just looks amazing.

I posted an entire essay on the issue serde-rs/serde#1544 (comment) , hoping for the best, expecting the usual.

Anyway, this does work, about as well as it could, but has awful error messages due to those serde_derive limitations, and I don't think it's possible to fix that:

use std::{fmt, marker::PhantomData};

use serde::{
    de::{self, MapAccess, Visitor},
    Deserialize, Deserializer,
};

#[macro_export]
macro_rules! sub_enum {
    ($sub_enum_name:ident : $super_enum_name:ty {
        $($variant:ident),* $(,)?
    }) => {
        #[derive(Debug, Deserialize)]
        #[serde(rename_all = "kebab-case")]
        pub enum $sub_enum_name {
            $($variant,)*
        }

        impl std::convert::TryFrom<$super_enum_name> for $sub_enum_name {
            type Error = String;
            fn try_from(val: $super_enum_name) -> Result<Self, Self::Error> {
                Ok(match val {
                    $(<$super_enum_name>::$variant => Self::$variant,)*
                    _ => return Err(format!(concat!("Unknown variant {:#?} of ", stringify!($sub_enum_name)), val))
                })
            }
        }
    }
}

#[derive(Debug, Deserialize)]
pub struct Config {
    chunking: ValueOrRules<bool>,
}

#[derive(Debug, Deserialize)]
#[serde(untagged)]
pub enum ValueOrRules<T> {
    Rule(Vec<Rule<T>>),
    Value(T),
}

#[derive(Debug)]
pub enum Rule<T> {
    If {
        variable: String,
        comparator: Comparator,
        cmp_value: toml::Value,
        value: Option<T>,
    },
    Logic {
        expr: Expr,
        conditions: Vec<Rule<T>>,
        value: Option<T>,
    },
    Else(T),
}

#[derive(Debug, Deserialize)]
#[serde(field_identifier, rename_all = "kebab-case")]
enum Field {
    Eq,
    Ne,
    StartsWith,
    NotStartsWith,
    EndsWith,
    NotEndsWith,
    InList,
    NotInList,
    Matches,
    NotMatches,

    AllOf,
    AnyOf,
    NoneOf,

    If,
    Else,
    Then,

    Unknown,
}

sub_enum!(Comparator : Field {
    Eq, Ne,
    StartsWith, NotStartsWith,
    EndsWith, NotEndsWith,
    InList, NotInList,
    Matches, NotMatches,
});

sub_enum!(Expr : Field {
    AllOf, AnyOf, NoneOf,
});

impl<'de, T: Deserialize<'de>> Deserialize<'de> for Rule<T> {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        struct RuleVisitor<T>(PhantomData<T>);

        impl<'de, T: Deserialize<'de>> Visitor<'de> for RuleVisitor<T> {
            type Value = Rule<T>;

            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
                formatter.write_str("enum Rule")
            }

            fn visit_map<V>(self, mut map: V) -> Result<Rule<T>, V::Error>
            where
                V: MapAccess<'de>,
            {
                let key = map.next_key()?;

                match key {
                    Some(Field::If) => {
                        let variable: String = map.next_value()?;
                        let comparator: Comparator = map
                            .next_key::<Field>()?
                            .unwrap_or(Field::Unknown)
                            .try_into()
                            .map_err(de::Error::custom)?;
                        let cmp_value: toml::Value = map.next_value()?;
                        let value: Option<T> = if let Some(Field::Then) = map.next_key()? {
                            map.next_value()?
                        } else {
                            None
                        };

                        Ok(Rule::If {
                            variable,
                            comparator,
                            cmp_value,
                            value,
                        })
                    }
                    Some(Field::AnyOf) | Some(Field::AllOf) | Some(Field::NoneOf) => {
                        let expr: Expr = key.unwrap().try_into().map_err(de::Error::custom)?;
                        let conditions = map.next_value()?;
                        let value: Option<T> = if let Some(Field::Then) = map.next_key()? {
                            map.next_value()?
                        } else {
                            None
                        };

                        Ok(Rule::Logic {
                            expr,
                            conditions,
                            value,
                        })
                    }
                    Some(Field::Else) => Ok(Rule::Else(map.next_value()?)),
                    _ => Err(de::Error::missing_field("xd")),
                }
            }
        }

        deserializer.deserialize_map(RuleVisitor::<T>(PhantomData))
    }
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let value: Config = toml::from_str(
        r#"
chunking = [ { any-of = [ { if = "rcpt-domain", xd = "example.org" },
                          { if = "remote-ip", eq = "192.168.0.0/24" },
                          { all-of = [
                              { if = "rcpt", starts-with = "no-reply@" },
                              { if = "sender", ends-with = "@domain.org" },
                              { none-of = [
                                  { if = "priority", eq = 1 },
                                  { if = "priority", ne = -2 },
                              ]}
                          ]}
                        ], then = false },
              { else = true } ]
    "#,
    )?;

    println!("{:#?}", value);

    Ok(())
}

I can now see why one would want to implement a parser from scratch 💀 , though I still don't like the super dynamic nature of the current solution, I think it's much cleaner to have all the repetitive mapping code derived.