Replace 'configparser' with 'rust-ini' for quoting and stable section order

Author: elonen

Cargo.lock

@@ -42,12 +42,12 @@ path = "src/main.rs"
 [dependencies]
 anyhow = "1.0.68"
 async-recursion = "1.0.2"
-configparser = "3.0.2"
 docopt = "1.1.1"
 hyper = { version = "0.14.23", features = ["full"] }
 ldap3 = "0.11.1"
 lru_time_cache = "0.11.11"
 regex = "1.7.1"
+rust-ini = "0.18.0"
 sha2 = "0.10.6"
 tokio = { version = "1.24.2", features = ["full"] }
 tracing = "0.1.37"

Cargo.toml

@@ -23,18 +23,18 @@ The server is configured with an INI file, such as:
 
 ```ini
 [default]
-; Which HTTP header to read the %USERNAME% from
-username_http_header = X-Ldap-Authz-Username
+; Which HTTP header to read %USERNAME% from
+username_http_header = "X-Ldap-Authz-Username"
 
 ; Example LDAP server configuration. This is for Active Directory,
 ; and makes a recursive membership query to given group.
-ldap_server_url = ldap://dc1.example.test:389
+ldap_server_url = "ldap://dc1.example.test:389"
 ldap_conn_timeout = 10.0
-ldap_bind_dn = CN=service,CN=Users,DC=example,DC=test
-ldap_bind_password = password123
-ldap_search_base = DC=example,DC=test
-ldap_query = (&(objectCategory=Person)(sAMAccountName=%USERNAME%)(memberOf:1.2.840.113556.1.4.1941:=CN=%MY_CUSTOM_VAR%,CN=Users,DC=example,DC=test))
-ldap_attribs = displayName, givenName, sn, mail
+ldap_bind_dn = "CN=service,CN=Users,DC=example,DC=test"
+ldap_bind_password = "password123"
+ldap_search_base = "DC=example,DC=test"
+ldap_query = "(&(objectCategory=Person)(sAMAccountName=%USERNAME%)(memberOf:1.2.840.113556.1.4.1941:=CN=%MY_CUSTOM_VAR%,CN=Users,DC=example,DC=test))"
+ldap_attribs = "displayName, givenName, sn, mail"
 
 ; Cache size (these are defaults)
 cache_time = 30
@@ -47,31 +47,34 @@ sub_query_join = Main
 
 [users]
 ; Regular expression to match against the request URI
-http_path = /users$
+http_path = "/users$"
 ; Ldap query references variable MY_CUSTOM_VAR above. Set it for this query:
-query_vars = MY_CUSTOM_VAR=ACL_Users
+query_vars = "MY_CUSTOM_VAR = ACL_Users"
 ; Fetch additional attributes from LDAP my performing additional queries
 ; if this one succeeds. See below for their definitions.
-sub_queries = is_beta_tester, is_bug_reporter
+sub_queries = "is_beta_tester, is_bug_reporter"
 
 
 [admins]
-http_path = /admins$
-query_vars = MY_CUSTOM_VAR=ACL_Admins
+http_path = "/admins$"
+query_vars = "MY_CUSTOM_VAR = ACL_Admins"
 ; Fictional example: instruct backend app to show debug info for admins
-set_attribs_on_success = extraGroups=show_debug_info
+set_attribs_on_success = "extraGroups = show_debug_info"
+
 
 ; Internal sub-queries (not matched agains URI as http_path is not defined)
 ; These examples set additional attributes ("extraGroups") if the user is a
 ; member of specified groups.
 
 [is_beta_tester]
-query_vars = MY_CUSTOM_VAR=ACL_Beta_Testers
-set_attribs_on_success = extraGroups=beta_tester
+query_vars = "MY_CUSTOM_VAR = Role_Beta_Testers"
+set_attribs_on_success = "extraGroups = beta_tester"
 
 [is_bug_reporter]
-query_vars = MY_CUSTOM_VAR=ACL_Bug_Reporters
-set_attribs_on_success = extraGroups=bug_reporter, extraGroups=show_debug_info
+query_vars = "MY_CUSTOM_VAR = Role_Bug_Reporters"
+set_attribs_on_success = "extraGroups = bug_reporter, extraGroups = show_debug_info"
+; Circular references are pruned, so this nonsense won't crash - it's just useless:
+sub_queries = "is_bug_reporter, users"
 ```
 
 The `[default]` section contains defaults that can be overridden in other sections.
@@ -181,7 +184,7 @@ world-readable. The server itself doesn't need to be able to write
 to the configuration file.
 
 Usernames are quoted before being used in LDAP queries, so they (hopefully)
-can't be used to inject arbitrary LDAP queries. In any case, it's recommended
+can't be used to inject arbitrary LDAP queries. It's recommended
 to use a read-only LDAP bind user just in case.
 
 LDAPS is supported (even though the test scripts use plain ldap://), and is

README.md

@@ -1,16 +1,16 @@
 [default]
-; Which HTTP header to read the %USERNAME% from
-username_http_header = X-Ldap-Authz-Username
+; Which HTTP header to read %USERNAME% from
+username_http_header = "X-Ldap-Authz-Username"
 
 ; Example LDAP server configuration. This is for Active Directory,
 ; and makes a recursive membership query to given group.
-ldap_server_url = ldap://dc1.example.test:389
+ldap_server_url = "ldap://dc1.example.test:389"
 ldap_conn_timeout = 10.0
-ldap_bind_dn = CN=service,CN=Users,DC=example,DC=test
-ldap_bind_password = password123
-ldap_search_base = DC=example,DC=test
-ldap_query = (&(objectCategory=Person)(sAMAccountName=%USERNAME%)(memberOf:1.2.840.113556.1.4.1941:=CN=%MY_CUSTOM_VAR%,CN=Users,DC=example,DC=test))
-ldap_attribs = displayName, givenName, sn, mail
+ldap_bind_dn = "CN=service,CN=Users,DC=example,DC=test"
+ldap_bind_password = "password123"
+ldap_search_base = "DC=example,DC=test"
+ldap_query = "(&(objectCategory=Person)(sAMAccountName=%USERNAME%)(memberOf:1.2.840.113556.1.4.1941:=CN=%MY_CUSTOM_VAR%,CN=Users,DC=example,DC=test))"
+ldap_attribs = "displayName, givenName, sn, mail"
 
 ; Cache size (these are defaults)
 cache_time = 30
@@ -23,31 +23,31 @@ sub_query_join = Main
 
 [users]
 ; Regular expression to match against the request URI
-http_path = /users$
+http_path = "/users$"
 ; Ldap query references variable MY_CUSTOM_VAR above. Set it for this query:
-query_vars = MY_CUSTOM_VAR=ACL_Users
+query_vars = "MY_CUSTOM_VAR = ACL_Users"
 ; Fetch additional attributes from LDAP my performing additional queries
 ; if this one succeeds. See below for their definitions.
-sub_queries = is_beta_tester, is_bug_reporter
+sub_queries = "is_beta_tester, is_bug_reporter"
 
 
 [admins]
-http_path = /admins$
-query_vars = MY_CUSTOM_VAR=ACL_Admins
+http_path = "/admins$"
+query_vars = "MY_CUSTOM_VAR = ACL_Admins"
 ; Fictional example: instruct backend app to show debug info for admins
-set_attribs_on_success = extraGroups=show_debug_info
+set_attribs_on_success = "extraGroups = show_debug_info"
 
 
 ; Internal sub-queries (not matched agains URI as http_path is not defined)
 ; These examples set additional attributes ("extraGroups") if the user is a
 ; member of specified groups.
 
 [is_beta_tester]
-query_vars = MY_CUSTOM_VAR=Role_Beta_Testers
-set_attribs_on_success = extraGroups=beta_tester
+query_vars = "MY_CUSTOM_VAR = Role_Beta_Testers"
+set_attribs_on_success = "extraGroups = beta_tester"
 
 [is_bug_reporter]
-query_vars = MY_CUSTOM_VAR=Role_Bug_Reporters
-set_attribs_on_success = extraGroups=bug_reporter, extraGroups=show_debug_info
+query_vars = "MY_CUSTOM_VAR = Role_Bug_Reporters"
+set_attribs_on_success = "extraGroups = bug_reporter, extraGroups = show_debug_info"
 ; Circular references are pruned, so this nonsense won't crash - it's just useless:
-sub_queries = is_bug_reporter, users
+sub_queries = "is_bug_reporter, users"

example.ini

@@ -3,7 +3,7 @@ use std::collections::HashSet;
 
 use anyhow::bail;
 use anyhow::anyhow;
-use configparser::ini::Ini;
+use ini::Ini;
 use anyhow::Error;
 use anyhow::Result;
 use regex::Regex;
@@ -140,21 +140,19 @@ config_options! {
 
 /// Parse the configuration file
 /// 
-/// Returns a vector of ConfigSections, excluding the DEFAULT section, which
+/// Returns a vector of ConfigSections, excluding the [default] section, which
 /// is used to fill in missing values in the other sections.
 pub(crate) fn parse_config(config_file: &str) -> Result<Vec<ConfigSection>, Error>
 {
-    let mut config = Ini::new();
-    config.load(config_file).map_err(|e| anyhow!("Error loading config file: {}", e))?;
-    let map = config.get_map_ref();
+    let mut ini = Ini::load_from_file(config_file)?;
 
-    // Get the DEFAULT section
-    let mut defaults = match map.get("default") {
-            Some(d) => d,
-            None => bail!("No 'default' section in config file"),
-        }.clone();
-
-    // Fill in missing values from built-in defaults
+    // Collect defaults
+    let mut defaults = HashMap::new();
+    // ...from the [default] section
+    if let Some(default_sect) = ini.section(Some("default")) {
+        defaults.extend(default_sect.iter().map(|(k, v)| (k.to_string(), Some(v.to_string()))));
+    }
+    // ..from built-in defaults
     for (key, _, def) in CONFIG_OPTIONS.iter() {
         if !defaults.contains_key(*key) {
             if let Some(def) = def {
@@ -167,18 +165,22 @@ pub(crate) fn parse_config(config_file: &str) -> Result<Vec<ConfigSection>, Erro
     let mut res = Vec::new();
 
     // Walk through the sections
-    for section_name in config.sections() {
-        let mut sect_map = map.get(section_name.as_str()).unwrap().clone();
+    for (section_name, sect_props) in ini.iter_mut() {
+        let section_name = match section_name {
+            Some(name) => name,
+            None => { bail!("Options outside of a section are not allowed"); }
+        };
+
         if seen_sections.contains(&section_name) {
             bail!("Duplicate section [{}]", section_name);
         } else {
             seen_sections.insert(section_name.clone());
         }
 
         // Check that no unknown keys are set
-        let unknown_keys = sect_map.keys()
+        let unknown_keys = sect_props.iter()
+            .map(|(key, _)| key)
             .filter(|key| !CONFIG_OPTIONS.iter().any(|(k, _, _)| k == key))
-            .map(|key| key.clone())
             .collect::<Vec<_>>();
         if !unknown_keys.is_empty() {
             bail!("Unknown key(s) in section [{}]: {}", &section_name, unknown_keys.join(", "));
@@ -190,25 +192,26 @@ pub(crate) fn parse_config(config_file: &str) -> Result<Vec<ConfigSection>, Erro
 
         // Apply defaults
         for (key, value) in &defaults {
-            if sect_map.get(key).is_none() {
-                sect_map.insert(key.clone(), value.clone());
+            if sect_props.get(key).is_none() {
+                if let Some(value) = value {
+                    sect_props.insert(key.clone(), value.clone());
+                }
             }
         }
 
         // Check that all required keys are set
         let missing_keys = CONFIG_OPTIONS.iter()
-            .filter(|(key, _, _)| !sect_map.contains_key(*key))
+            .filter(|(key, _, _)| !sect_props.contains_key(*key))
             .map(|(key, _, _)| *key)
             .filter(|key| key != &"section")
             .collect::<Vec<_>>();
         if !missing_keys.is_empty() {
             bail!("Config option(s) not set in section [{}]: {}", section_name, missing_keys.join(", "));
         }
 
-        let get = |key: &str| sect_map.get(key)
+        let get = |key: &str| sect_props.get(key)
             .unwrap_or_else(|| panic!("BUG: missing key '{key}' after checking that it's not missing?!"))
-            .clone()
-            .expect("BUG? None value for an config value?");
+            .to_string();
 
         // Compile regex
         let http_path = get("http_path");
@@ -240,7 +243,7 @@ pub(crate) fn parse_config(config_file: &str) -> Result<Vec<ConfigSection>, Erro
 
         // Store result
         res.push(ConfigSection {
-            section: section_name.clone(),
+            section: section_name.to_string(),
             http_path: http_path_re,
             username_http_header: get("username_http_header"),