diff --git a/Cargo.lock b/Cargo.lock index 75156ed..ea55dcf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -170,7 +170,7 @@ checksum = "2439c63f3f6139d1b57529d16bc3b8bb855230c8efcc5d3a896c8bea7c3b1e84" [[package]] name = "spamassassin-milter" -version = "0.0.2" +version = "0.0.3" dependencies = [ "chrono", "clap", diff --git a/Cargo.toml b/Cargo.toml index 89afbd5..8c8e8f9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "spamassassin-milter" -version = "0.0.2" # remember to update html_root_url +version = "0.0.3" # remember to update html_root_url and VERSION description = "Milter for spam filtering with SpamAssassin" authors = ["David Bürgin "] edition = "2018" diff --git a/README.md b/README.md index 4714a4a..e3a7024 100644 --- a/README.md +++ b/README.md @@ -28,13 +28,13 @@ configuration and integration approaches. This application can be used as a replacement for [spamass-milt]; it has a reduced feature set, but it should be satisfactory for a personal mail server setup. SpamAssassin Milter has been used in such a setup together with Postfix, -SpamAssassin, and for delivery [Dovecot] with LMTP and the [Sieve] plugin. +SpamAssassin, and for delivery [Dovecot] with LMTP and the [Sieve plugin]. [Apache SpamAssassin]: https://spamassassin.apache.org [Postfix]: http://www.postfix.org [spamass-milt]: https://savannah.nongnu.org/projects/spamass-milt/ [Dovecot]: https://dovecot.org -[Sieve]: https://doc.dovecot.org/configuration_manual/sieve/ +[Sieve plugin]: https://doc.dovecot.org/configuration_manual/sieve/ ## Building @@ -119,6 +119,16 @@ and the server. The main SpamAssassin configuration file is `/etc/spamassassin/local.conf`. See `perldoc Mail::SpamAssassin::Conf` for detailed information. +The phrase ‘flagged as spam’, which appears throughout this documentation, +refers to whether SpamAssassin has marked a message as being spam by adding the +header `X-Spam-Flag: YES`. A message being spam or ham (not spam) is a binary +property. The classification threshold can be configured by setting +`required_score` as follows: + +``` +required_score 9.0 +``` + By default, SpamAssassin creates ‘reports’ for messages it recognises as spam. These reports replace the message body, that is, the message body is rewritten to present a report instead of the original message, and the original message is @@ -176,8 +186,8 @@ Finally, a pitfall of `spamc` deserves highlighting: `spamc` by default tries to resist failure to an extent that it will not indicate failure even if it cannot connect to SpamAssassin server at all (apart from warnings logged to syslog)! If it cannot connect to the server, it simply echoes what it received, and so masks -the error condition. This mechanism is labelled ‘safe fallback’ and should be -disabled once the system is set up and working well. Set the following flag: +the error condition. This behaviour is labelled ‘safe fallback’ and is perhaps +best disabled once the system is set up. Set the following flag: ``` --no-safe-fallback diff --git a/spamassassin-milter.8 b/spamassassin-milter.8 index 89f01f7..20bee1d 100644 --- a/spamassassin-milter.8 +++ b/spamassassin-milter.8 @@ -1,4 +1,4 @@ -.TH SPAMASSASSIN-MILTER 8 2020-02-09 +.TH SPAMASSASSIN-MILTER 8 2020-02-14 .SH NAME spamassassin-milter \- milter for spam filtering with SpamAssassin .SH SYNOPSIS diff --git a/src/callbacks.rs b/src/callbacks.rs index 66e188c..224aab7 100644 --- a/src/callbacks.rs +++ b/src/callbacks.rs @@ -39,7 +39,7 @@ fn handle_negotiate( } } - let req_protocol_opts = ProtocolOpts::SKIP; + let req_protocol_opts = ProtocolOpts::SKIP | ProtocolOpts::HEADER_LEADING_SPACE; assert!(actions.contains(req_actions), "required milter actions not supported"); assert!(protocol_opts.contains(req_protocol_opts), "required milter protocol options not supported"); @@ -130,6 +130,10 @@ fn handle_data(mut ctx: Context) -> milter::Result { let client_ip = conn.client_ip.to_string(); + // Note that when SpamAssassin reports are enabled (`report_safe 1`), the + // forged headers below are ‘leaked’ to users in the sense that they are + // included inside the email MIME attachment in the new message body. + client.send_envelope_sender()?; client.send_envelope_recipients()?; client.send_forged_received_header( diff --git a/src/client.rs b/src/client.rs index c568e3e..37c442a 100644 --- a/src/client.rs +++ b/src/client.rs @@ -23,6 +23,7 @@ pub struct Spamc { } impl Spamc { + // TODO Is the name enough? Do we need a path, eg when run as root? const SPAMC_PROGRAM: &'static str = "spamc"; pub fn new(spamc_args: &'static [String]) -> Self { @@ -35,7 +36,7 @@ impl Spamc { impl Process for Spamc { fn connect(&mut self) -> Result<()> { - // `Command::spawn` always succeeds when spamc can be invoked, even if + // `Command::spawn` always succeeds when `spamc` can be invoked, even if // logically the command is invalid, eg if it uses non-existing options. let child = Command::new(Spamc::SPAMC_PROGRAM) .args(self.spamc_args) @@ -50,17 +51,20 @@ impl Process for Spamc { fn writer(&mut self) -> &mut dyn Write { let spamc = self.spamc.as_mut().expect("spamc process not started"); + spamc.stdin.as_mut().unwrap() } fn finish(&mut self) -> Result> { let spamc = self.spamc.take().expect("spamc process not started"); + let output = spamc.wait_with_output()?; + // TODO put signal/exit code in message if output.status.success() { Ok(output.stdout) } else { - Err(Error::IoError(String::from("spamc returned error exit code"))) + Err(Error::Io(String::from("spamc returned error exit code"))) } } @@ -71,10 +75,12 @@ impl Process for Spamc { impl Drop for Spamc { fn drop(&mut self) { - // Kill spamc: a child process will continue to run even after its - // `Child` handle has gone out of scope. + // Kill (and wait on) `spamc`: a child process will continue to run even + // after its `Child` handle has gone out of scope. if let Some(spamc) = self.spamc.as_mut() { + // The results are no longer of interest at this point. let _ = spamc.kill(); + let _ = spamc.wait(); } } } @@ -83,7 +89,7 @@ pub struct Client { process: Box, sender: String, recipients: Vec, - headers: HeaderMap<'static>, // `'static` keys in canonical form + headers: HeaderMap, bytes: usize, } @@ -98,23 +104,23 @@ impl Client { } } - pub fn bytes_written(&self) -> usize { - self.bytes + pub fn add_recipient(&mut self, recipient: String) { + self.recipients.push(recipient); } - pub fn add_recipient(&mut self, rcpt: String) { - self.recipients.push(rcpt); + pub fn bytes_written(&self) -> usize { + self.bytes } pub fn connect(&mut self) -> Result<()> { self.process.connect() } - // Implementation note: the send operations can all fail with an IO error - // and so return a Result that is then simply unwrapped with `?` in the - // `callbacks` module. That is, in normal circumstances we don’t expect this - // to occur, only in abnormal circumstances such as when wrong flags where - // passed to spamc! + // Implementation note: The send operations all may fail with an I/O error, + // and so return a `Result<()>` that is then unceremoniously unwrapped with + // `?` in the callback functions. This is acceptable, because we don’t + // expect this to happen in normal circumstances, only when something is + // wrong with `spamc` configuration or operation. pub fn send_envelope_sender(&mut self) -> Result<()> { let buf = format!("X-Envelope-From: {}\r\n", self.sender); @@ -163,22 +169,17 @@ impl Client { } pub fn send_header(&mut self, name: &str, value: &str) -> Result<()> { - // Record selected SpamAssassin headers, but never pass any on. - if email::is_spam_assassin_header(name) { - if let Some(canonical_name) = email::SPAM_ASSASSIN_HEADERS.get(name) { - self.headers.insert_if_absent(canonical_name, email::ensure_crlf(value)); - } - - return Ok(()); - } - + // Remember that the value includes leading whitespace, as we’ve + // requested in the milter protocol options. This lets us keep + // whitespace exactly as is. let value = email::ensure_crlf(value); - let buf = format!("{}: {}\r\n", name, value); + let buf = format!("{}:{}\r\n", name, value); - if let Some(canonical_name) = email::REWRITE_HEADERS.get(name) - .or_else(|| email::REPORT_HEADERS.get(name)) + if email::is_spam_assassin_header(name) + || email::REWRITE_HEADERS.contains(name) + || email::REPORT_HEADERS.contains(name) { - self.headers.insert_if_absent(canonical_name, value); + self.headers.insert_if_absent(name, value); } self.bytes += self.process.writer().write(buf.as_bytes())?; @@ -207,6 +208,7 @@ impl Client { let output = match self.process.finish() { Ok(output) => output, Err(e) => { + // TODO revise error message (not failed to wait?) eprintln!("{}: failed to wait for spamc to exit: {}", id, e); return Ok(Status::Tempfail); } @@ -215,7 +217,7 @@ impl Client { let email = match Email::parse(&output) { Ok(email) => email, Err(e) => { - eprintln!("{}: {}", id, e); + eprintln!("{}: invalid response from spamc: {}", id, e); return Ok(Status::Tempfail); } }; @@ -243,7 +245,6 @@ impl Client { } } - verbose!(config, "{}: finished processing", id); Ok(Status::Accept) } } @@ -392,8 +393,8 @@ mod tests { let spamc = MockSpamc::new(); let mut client = Client::new(spamc, String::from("sender")); - client.send_header("name1", "value1").unwrap(); - client.send_header("name2", "value2\n\tcontinued").unwrap(); + client.send_header("name1", " value1").unwrap(); + client.send_header("name2", " value2\n\tcontinued").unwrap(); client.send_eoh().unwrap(); client.send_body_chunk(b"body").unwrap(); @@ -404,17 +405,6 @@ mod tests { ); } - #[test] - fn client_send_no_spam_assassin_headers() { - let spamc = MockSpamc::new(); - - let mut client = Client::new(spamc, String::from("sender")); - client.send_header("x-spam-flag", "YES").unwrap(); - client.send_header("x-spam-bogus", "x").unwrap(); - - assert_eq!(client.bytes_written(), 0); - } - #[test] fn client_process_invalid_response() { let spamc = MockSpamc::with_output(b"invalid message response".to_vec()); @@ -439,13 +429,16 @@ mod tests { let status = client.process("id", &actions, &config).unwrap(); assert_eq!(status, Status::Reject); + + let called = actions.called.borrow(); + assert_eq!(called.len(), 1); assert_eq!( - actions.called.borrow().first(), - Some(&Action::SetErrorReply( + called.first().unwrap(), + &Action::SetErrorReply( "550".into(), Some("5.7.1".into()), vec!["Spam message refused".into()] - )) + ) ); } @@ -458,8 +451,8 @@ mod tests { let config = Default::default(); let mut client = Client::new(spamc, String::from("sender")); - client.send_header("x-spam-level", "*").unwrap(); - client.send_header("x-spam-report", "...").unwrap(); + client.send_header("x-spam-level", " *").unwrap(); + client.send_header("x-spam-report", " ...").unwrap(); let status = client.process("id", &actions, &config).unwrap(); @@ -469,8 +462,8 @@ mod tests { assert_eq!(called.len(), 4); let mut expected = HashSet::new(); - expected.insert(Action::AddHeader("X-Spam-Flag".into(), "YES".into())); - expected.insert(Action::ReplaceHeader("X-Spam-Level".into(), 1, Some("*****".into()))); + expected.insert(Action::AddHeader("X-Spam-Flag".into(), " YES".into())); + expected.insert(Action::ReplaceHeader("X-Spam-Level".into(), 1, Some(" *****".into()))); expected.insert(Action::ReplaceHeader("X-Spam-Report".into(), 1, None)); expected.insert(Action::AppendBodyChunk(b"Report".to_vec())); diff --git a/src/collections.rs b/src/collections.rs index 966d900..c57a5c2 100644 --- a/src/collections.rs +++ b/src/collections.rs @@ -1,38 +1,43 @@ use std::mem; -/// An array map with ASCII-case-insensitive `&str` keys. +// Design note: Header handling requires ASCII-case-insensitive map keys. This +// complexity could have been pushed out to the map key type (a newtype +// implementing `Hash` and `Eq`) while using an ordinary `HashMap`. We’ve found +// it simpler to extract the complexity into these custom collections instead, +// and use plain strings in the application logic. + +/// A vector map with ASCII-case-insensitive `AsRef` keys. #[derive(Clone, Debug, Default)] -pub struct StrVecMap<'k, V> { - entries: Vec<(&'k str, V)>, +pub struct StrVecMap { + entries: Vec<(K, V)>, } -impl<'k, V> StrVecMap<'k, V> { +impl StrVecMap +where + K: AsRef, +{ pub fn new() -> Self { Self { entries: Default::default() } } - pub fn iter(&self) -> impl Iterator { - self.entries.iter() + pub fn iter(&self) -> impl Iterator { + self.entries.iter().map(|e| (&e.0, &e.1)) } - pub fn keys(&self) -> impl Iterator { + pub fn keys(&self) -> impl Iterator { self.iter().map(|e| e.0) } pub fn contains_key(&self, key: &str) -> bool { - self.iter().any(|e| e.0.eq_ignore_ascii_case(key)) + self.iter().any(|e| e.0.as_ref().eq_ignore_ascii_case(key)) } pub fn get(&self, key: &str) -> Option<&V> { - self.iter().find(|e| e.0.eq_ignore_ascii_case(key)).map(|e| &e.1) + self.iter().find(|e| e.0.as_ref().eq_ignore_ascii_case(key)).map(|e| e.1) } - pub fn get_key_value(&self, key: &str) -> Option<(&str, &V)> { - self.iter().find(|e| e.0.eq_ignore_ascii_case(key)).map(|e| (e.0, &e.1)) - } - - pub fn insert(&mut self, key: &'k str, value: V) -> Option { - match self.entries.iter_mut().find(|e| e.0.eq_ignore_ascii_case(key)) { + pub fn insert(&mut self, key: K, value: V) -> Option { + match self.entries.iter_mut().find(|e| e.0.as_ref().eq_ignore_ascii_case(key.as_ref())) { Some(e) => Some(mem::replace(&mut e.1, value)), None => { self.entries.push((key, value)); @@ -40,29 +45,39 @@ impl<'k, V> StrVecMap<'k, V> { } } } +} - pub fn insert_if_absent(&mut self, key: &'k str, value: V) -> Option { - if self.contains_key(key) { +// Ad-hoc implementation, only does `to_owned` on the lookup key if the key is +// not yet present. +impl StrVecMap { + pub fn insert_if_absent(&mut self, key: K, value: V) -> Option + where + K: AsRef, + { + if self.contains_key(key.as_ref()) { Some(value) } else { - self.entries.push((key, value)); + self.entries.push((key.as_ref().to_owned(), value)); None } } } -/// An array set containing ASCII-case-insensitive `&str` elements. +/// A vector set containing ASCII-case-insensitive `AsRef` elements. #[derive(Clone, Debug, Default)] -pub struct StrVecSet<'e> { - map: StrVecMap<'e, ()>, +pub struct StrVecSet { + map: StrVecMap, } -impl<'e> StrVecSet<'e> { +impl StrVecSet +where + E: AsRef, +{ pub fn new() -> Self { - Default::default() + Self { map: StrVecMap::new() } } - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> impl Iterator { self.map.keys() } @@ -70,11 +85,7 @@ impl<'e> StrVecSet<'e> { self.map.contains_key(key) } - pub fn get(&self, key: &str) -> Option<&str> { - self.map.get_key_value(key).map(|(k, _)| k) - } - - pub fn insert(&mut self, key: &'e str) -> bool { + pub fn insert(&mut self, key: E) -> bool { self.map.insert(key, ()).is_none() } } @@ -88,17 +99,17 @@ mod tests { let mut map = StrVecMap::new(); map.insert("KEY1", 1); - assert_eq!(map.contains_key("key1"), true); - assert_eq!(map.contains_key("key2"), false); + assert!(map.contains_key("key1")); + assert!(!map.contains_key("key2")); } #[test] - fn map_get_key_value() { + fn map_get() { let mut map = StrVecMap::new(); map.insert("KEY1", 1); - assert_eq!(map.get_key_value("key1"), Some(("KEY1", &1))); - assert_eq!(map.get_key_value("key2"), None); + assert_eq!(map.get("key1"), Some(&1)); + assert_eq!(map.get("key2"), None); } #[test] @@ -110,8 +121,8 @@ mod tests { assert_eq!(map.insert("key1", 3), Some(1)); let mut iter = map.iter(); - assert_eq!(iter.next(), Some(&("KEY1", 3))); - assert_eq!(iter.next(), Some(&("KEY2", 2))); + assert_eq!(iter.next(), Some((&"KEY1", &3))); + assert_eq!(iter.next(), Some((&"KEY2", &2))); assert_eq!(iter.next(), None); } @@ -124,31 +135,22 @@ mod tests { assert_eq!(map.insert_if_absent("key1", 3), Some(3)); let mut iter = map.iter(); - assert_eq!(iter.next(), Some(&("KEY1", 1))); - assert_eq!(iter.next(), Some(&("KEY2", 2))); + assert_eq!(iter.next(), Some((&String::from("KEY1"), &1))); + assert_eq!(iter.next(), Some((&String::from("KEY2"), &2))); assert_eq!(iter.next(), None); } - #[test] - fn set_get() { - let mut set = StrVecSet::new(); - set.insert("KEY1"); - - assert_eq!(set.get("key1"), Some("KEY1")); - assert_eq!(set.get("key2"), None); - } - #[test] fn set_insert() { let mut set = StrVecSet::new(); - assert_eq!(set.insert("KEY1"), true); - assert_eq!(set.insert("KEY2"), true); - assert_eq!(set.insert("key1"), false); + assert!(set.insert("KEY1")); + assert!(set.insert("KEY2")); + assert!(!set.insert("key1")); let mut iter = set.iter(); - assert_eq!(iter.next(), Some("KEY1")); - assert_eq!(iter.next(), Some("KEY2")); + assert_eq!(iter.next(), Some(&"KEY1")); + assert_eq!(iter.next(), Some(&"KEY2")); assert_eq!(iter.next(), None); } } diff --git a/src/config.rs b/src/config.rs index 268e68b..84c46f3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -92,7 +92,7 @@ impl Default for ConfigBuilder { trusted_networks: Default::default(), auth_untrusted: Default::default(), spamc_args: Default::default(), - max_message_size: 512_000, + max_message_size: 512_000, // same as in `spamc` dry_run: Default::default(), reject_spam: Default::default(), preserve_headers: Default::default(), diff --git a/src/email.rs b/src/email.rs index c0603d7..9fdd87c 100644 --- a/src/email.rs +++ b/src/email.rs @@ -80,14 +80,13 @@ fn parse_header_line(bytes: &[u8]) -> Result> { return Err(Error::ParseEmail); } - let ascii_whitespace: &[_] = &[' ', '\t']; - let value = (&value[1..]).trim_start_matches(ascii_whitespace); + let value = &value[1..]; Ok(Header { name, value }) } pub fn ensure_crlf(s: &str) -> String { - // For symmetry, ensure existing occurrences of b"\r\n" remain unchanged. + // For symmetry, ensure existing occurrences of "\r\n" remain unchanged. s.split('\n') .map(|line| match line.as_bytes().last() { Some(&last) if last == b'\r' => &line[..(line.len() - 1)], @@ -108,8 +107,9 @@ pub fn is_spam_assassin_header(name: &str) -> bool { name[..cmp::min(prefix.len(), name.len())].eq_ignore_ascii_case(prefix) } -pub type HeaderMap<'k> = StrVecMap<'k, String>; // values use CRLF line breaks -pub type HeaderSet<'e> = StrVecSet<'e>; +// Values use CRLF line breaks and include leading whitespace. +pub type HeaderMap = StrVecMap; +pub type HeaderSet<'e> = StrVecSet<&'e str>; // Selected subset of ‘X-Spam-’ headers for which we assume responsibility. pub static SPAM_ASSASSIN_HEADERS: Lazy> = Lazy::new(|| { @@ -137,12 +137,13 @@ pub static REPORT_HEADERS: Lazy> = Lazy::new(|| { h }); -/// A header rewriter going between incoming headers, and headers returned from -/// the SpamAssassin service. Operates only on the first occurrence of headers -/// with the same name. +/// A header rewriter that processes headers returned by SpamAssassin, and +/// computes and applies modifications by referring back to the original set of +/// headers. The rewriter operates only on the first occurrence of headers with +/// the same name. #[derive(Clone, Debug)] pub struct HeaderRewriter<'a> { - original: HeaderMap<'a>, + original: HeaderMap, processed: HeaderSet<'a>, spam_assassin_mods: Vec>, rewrite_mods: Vec>, @@ -151,7 +152,7 @@ pub struct HeaderRewriter<'a> { } impl<'a> HeaderRewriter<'a> { - pub fn new(original: HeaderMap<'a>, config: &'a Config) -> Self { + pub fn new(original: HeaderMap, config: &'a Config) -> Self { Self { original, processed: HeaderSet::new(), @@ -163,7 +164,8 @@ impl<'a> HeaderRewriter<'a> { } pub fn process_header(&mut self, name: &'a str, value: &'a str) { - // Assumes that the value is normalised to using CRLF line breaks. + // Assumes that the value is normalised to using CRLF line breaks, and + // includes leading whitespace. if is_spam_assassin_header(name) { if let Some(m) = self.convert_to_header_mod(name, value) { self.spam_assassin_mods.push(m); @@ -201,18 +203,27 @@ impl<'a> HeaderRewriter<'a> { self.spam_assassin_mods.iter().any(|m| match m { Add { name, value } | Replace { name, value } => { - name.eq_ignore_ascii_case("X-Spam-Flag") && value.eq_ignore_ascii_case("YES") + name.eq_ignore_ascii_case("X-Spam-Flag") && value.trim().eq_ignore_ascii_case("YES") } _ => false, }) } - pub fn rewrite_report_headers( + pub fn rewrite_spam_assassin_headers( &self, id: &str, actions: &impl ActionContext, ) -> milter::Result<()> { - execute_mods(id, self.report_mods.iter(), actions, self.config) + execute_mods(id, self.spam_assassin_mods.iter(), actions, self.config)?; + + // Delete certain incoming SpamAssassin headers not returned by + // SpamAssassin, to get rid of foreign `X-Spam-Flag` etc. headers. + let deletions = SPAM_ASSASSIN_HEADERS.iter() + .filter(|n| self.original.contains_key(n) && !self.processed.contains(n)) + .map(|name| HeaderMod::Delete { name }) + .collect::>(); + + execute_mods(id, deletions.iter(), actions, self.config) } pub fn rewrite_rewrite_headers( @@ -223,20 +234,12 @@ impl<'a> HeaderRewriter<'a> { execute_mods(id, self.rewrite_mods.iter(), actions, self.config) } - pub fn rewrite_spam_assassin_headers( + pub fn rewrite_report_headers( &self, id: &str, actions: &impl ActionContext, ) -> milter::Result<()> { - execute_mods(id, self.spam_assassin_mods.iter(), actions, self.config)?; - - // Delete incoming SpamAssassin headers not returned by SpamAssassin. - let deletions = SPAM_ASSASSIN_HEADERS.iter() - .filter(|n| self.original.contains_key(n) && !self.processed.contains(n)) - .map(|name| HeaderMod::Delete { name }) - .collect::>(); - - execute_mods(id, deletions.iter(), actions, self.config) + execute_mods(id, self.report_mods.iter(), actions, self.config) } } @@ -273,8 +276,8 @@ pub fn replace_body( }) } -// Header rewriting modification operation, operates only on the first of -// headers occurring multiple times (index = 1). +/// A header rewriting modification operation. These are intended to operate +/// only on the first instance of headers occurring multiple times. #[derive(Clone, Copy, Debug)] enum HeaderMod<'a> { Add { name: &'a str, value: &'a str }, @@ -286,8 +289,8 @@ impl HeaderMod<'_> { fn execute(&self, actions: &impl ActionContext) -> milter::Result<()> { use HeaderMod::*; - // Experiment shows that the milter library is smart enough to treat the - // name in case-insensitive manner, eg ‘Subject’ may replace ‘sUbject’. + // The milter library is smart enough to treat the name in a + // case-insensitive manner, eg ‘Subject’ may replace ‘sUbject’. match self { Add { name, value } => actions.add_header(name, &ensure_lf(value)), Replace { name, value } => actions.replace_header(name, 1, Some(&ensure_lf(value))), @@ -352,7 +355,7 @@ mod tests { assert_eq!(parse_header_line(b":empty name"), Err(Error::ParseEmail)); assert_eq!(parse_header_line(b"\t : whitespace name"), Err(Error::ParseEmail)); assert_eq!(parse_header_line(b"name:value"), Ok(Header { name: "name", value: "value" })); - assert_eq!(parse_header_line(b"name: value"), Ok(Header { name: "name", value: "value" })); + assert_eq!(parse_header_line(b"name: value"), Ok(Header { name: "name", value: " value" })); assert_eq!( parse_header_line(b"name:\r\n\tvalue"), Ok(Header { name: "name", value: "\r\n\tvalue" }) @@ -394,30 +397,30 @@ mod tests { #[test] fn header_rewriter_flags_spam() { - let config = Default::default(); let mut headers = HeaderMap::new(); - headers.insert("x-spam-flag", String::from("no")); + headers.insert(String::from("x-spam-flag"), String::from(" no")); + let config = Default::default(); let mut rewriter = HeaderRewriter::new(headers, &config); - rewriter.process_header("X-Spam-Flag", "YES"); + rewriter.process_header("X-Spam-Flag", " YES"); assert!(rewriter.is_flagged_spam()); } #[test] fn header_rewriter_processes_first_occurrence_only() { - let config = Default::default(); let headers = HeaderMap::new(); + let config = Default::default(); let mut rewriter = HeaderRewriter::new(headers, &config); - rewriter.process_header("X-Spam-Flag", "NO"); - rewriter.process_header("X-Spam-Flag", "YES"); + rewriter.process_header("X-Spam-Flag", " NO"); + rewriter.process_header("X-Spam-Flag", " YES"); let mut mods = rewriter.spam_assassin_mods.into_iter(); match mods.next().unwrap() { HeaderMod::Add { name, value } => { assert_eq!(name, "X-Spam-Flag"); - assert_eq!(value, "NO"); + assert_eq!(value, " NO"); } _ => panic!(), } @@ -426,20 +429,20 @@ mod tests { #[test] fn header_rewriter_replaces_different_values() { - let config = Default::default(); let mut headers = HeaderMap::new(); - headers.insert("x-spam-level", String::from("***")); - headers.insert("x-spam-report", String::from("original")); + headers.insert(String::from("x-spam-level"), String::from(" ***")); + headers.insert(String::from("x-spam-report"), String::from(" original")); + let config = Default::default(); let mut rewriter = HeaderRewriter::new(headers, &config); - rewriter.process_header("X-Spam-Level", "***"); - rewriter.process_header("X-Spam-Report", "new"); + rewriter.process_header("X-Spam-Level", " ***"); + rewriter.process_header("X-Spam-Report", " new"); let mut mods = rewriter.spam_assassin_mods.into_iter(); match mods.next().unwrap() { HeaderMod::Replace { name, value } => { assert_eq!(name, "X-Spam-Report"); - assert_eq!(value, "new"); + assert_eq!(value, " new"); } _ => panic!(), } diff --git a/src/error.rs b/src/error.rs index f13c842..efc0a83 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,8 +10,8 @@ pub type Result = result::Result; pub enum Error { ParseEmail, // For our purposes it is enough to record just the error message of I/O - // errors, no need to keep the full `io::Error` around. - IoError(String), + // errors, no need to keep the `io::Error` itself around. + Io(String), } impl Display for Error { @@ -19,26 +19,22 @@ impl Display for Error { use Error::*; match self { - ParseEmail => write!(f, "failed to parse email response"), - IoError(msg) => msg.fmt(f), + ParseEmail => write!(f, "failed to parse email"), + Io(msg) => msg.fmt(f), } } } -impl error::Error for Error { - fn source(&self) -> Option<&(dyn error::Error + 'static)> { - None - } -} +impl error::Error for Error {} impl From for Error { fn from(error: io::Error) -> Self { - Error::IoError(format!("{}", error)) // just record the error message + Error::Io(format!("{}", error)) // just record the error message } } impl From for milter::Error { - fn from(mine: Error) -> Self { - milter::Error::Custom(mine.into()) + fn from(error: Error) -> Self { + milter::Error::Custom(error.into()) } } diff --git a/src/lib.rs b/src/lib.rs index b0acb4f..1bd34d5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,6 @@ //! The SpamAssassin Milter application library. -#![doc(html_root_url = "https://docs.rs/spamassassin-milter/0.0.2")] +#![doc(html_root_url = "https://docs.rs/spamassassin-milter/0.0.3")] #![macro_use] macro_rules! verbose { @@ -31,9 +31,9 @@ use milter::Milter; pub const MILTER_NAME: &str = "SpamAssassin Milter"; /// The current version string of SpamAssassin Milter. -pub const VERSION: &str = "0.0.2"; +pub const VERSION: &str = "0.0.3"; -/// Starts SpamAssassin Milter at the given socket using the supplied +/// Starts SpamAssassin Milter listening on the given socket using the supplied /// configuration. /// /// This is a blocking call. diff --git a/src/main.rs b/src/main.rs index a52fa74..cfddcca 100644 --- a/src/main.rs +++ b/src/main.rs @@ -88,27 +88,28 @@ fn main() { fn build_config(matches: &ArgMatches<'_>) -> Result { let mut config = Config::builder(); - if let Some(s) = matches.value_of(ARG_MAX_MESSAGE_SIZE) { - match s.parse() { + if let Some(bytes) = matches.value_of(ARG_MAX_MESSAGE_SIZE) { + match bytes.parse() { Ok(bytes) => { config.set_max_message_size(bytes); } Err(_) => { - return Err(format!("invalid max message size \"{}\"", s)); + return Err(format!("invalid max message size \"{}\"", bytes)); } } } - if let Some(t) = matches.values_of(ARG_TRUSTED_NETWORKS) { + if let Some(nets) = matches.values_of(ARG_TRUSTED_NETWORKS) { config.set_has_trusted_networks(true); - for n in t.filter(|n| !n.trim().is_empty()) { - match n.parse().or_else(|_| n.parse::().map(From::from)) { + for net in nets.filter(|n| !n.trim().is_empty()) { + // Both `ipnet::IpNet` and `std::net::IpAddr` are supported. + match net.parse().or_else(|_| net.parse::().map(From::from)) { Ok(net) => { config.add_trusted_network(net); } Err(_) => { - return Err(format!("invalid trusted network address \"{}\"", n)); + return Err(format!("invalid trusted network address \"{}\"", net)); } } } @@ -133,8 +134,8 @@ fn build_config(matches: &ArgMatches<'_>) -> Result { config.set_verbose(true); } - if let Some(s) = matches.values_of(ARG_SPAMC_ARGS) { - config.set_spamc_args(s.map(|arg| arg.to_owned()).collect()); + if let Some(spamc_args) = matches.values_of(ARG_SPAMC_ARGS) { + config.set_spamc_args(spamc_args.map(|a| a.to_owned()).collect()); }; Ok(config.build()) diff --git a/tests/authenticated_sender.rs b/tests/authenticated_sender.rs index c7a510b..ff2478c 100644 --- a/tests/authenticated_sender.rs +++ b/tests/authenticated_sender.rs @@ -5,7 +5,7 @@ use spamassassin_milter::*; #[test] fn authenticated_sender() { - let config = Config::builder().build(); + let config = Default::default(); let miltertest = spawn_miltertest_runner(file!()); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 8af82e9..645f880 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -62,7 +62,7 @@ where match f(s) { // Again very basic handling of the spamd server protocol: add a forged - // protocol header terminated with "\r\n\r\n". + // protocol header terminated with "\r\n\r\n". (Currently not used.) Ok(ham) => format!( "{}\r\nContent-length: {}\r\nSpam: False ; 4.0 / 5.0\r\n\r\n{}", SPAMD_PROTOCOL_OK, diff --git a/tests/ham_flow.lua b/tests/ham_flow.lua index 3dec633..002d236 100644 --- a/tests/ham_flow.lua +++ b/tests/ham_flow.lua @@ -47,12 +47,18 @@ assert(mt.getreply(conn) == SMFIR_CONTINUE) local err = mt.header(conn, "Date", os.date("%a, %d %b %Y %H:%M:%S %Z")) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "X-Spam-Checker-Version", "BogusChecker 1.0.0") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "X-Spam-Report", "Bogus report") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) local err = mt.eoh(conn) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) -local err = mt.bodystring(conn, "Test message body") +local err = mt.bodystring(conn, "Hello, we would like to invite you to ...") assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) @@ -60,5 +66,9 @@ local err = mt.eom(conn) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_ACCEPT) +assert(mt.eom_check(conn, MT_HDRCHANGE, "X-Spam-Checker-Version", " MyChecker 1.0.0")) +assert(mt.eom_check(conn, MT_HDRADD, "X-Spam-Custom", " Custom-Value")) +assert(mt.eom_check(conn, MT_HDRDELETE, "X-Spam-Report")) + local err = mt.disconnect(conn) assert(err == nil, err) diff --git a/tests/ham_flow.rs b/tests/ham_flow.rs index e7c5d79..4525c6f 100644 --- a/tests/ham_flow.rs +++ b/tests/ham_flow.rs @@ -9,7 +9,27 @@ fn ham_flow() { builder.set_spamc_args(vec![format!("--port={}", SPAMD_PORT)]); let config = builder.build(); - let server = spawn_mock_spamd_server(SPAMD_PORT, Ok); + let server = spawn_mock_spamd_server(SPAMD_PORT, |ham| { + Ok(ham + .replacen( + "X-Spam-Checker-Version: BogusChecker 1.0.0\r\n", + "X-Spam-Checker-Version: MyChecker 1.0.0\r\n", + 1, + ) + .replacen( + "X-Spam-Report: Bogus report\r\n", + "", + 1, + ) + .replacen( + "\r\n\r\n", + "\r\n\ + X-Spam-Custom: Custom-Value\r\n\ + \r\n", + 1, + ) + ) + }); let miltertest = spawn_miltertest_runner(file!()); run("inet:3333@localhost", config).expect("milter execution failed"); diff --git a/tests/live.lua b/tests/live.lua index bcc0dff..7391c31 100644 --- a/tests/live.lua +++ b/tests/live.lua @@ -48,6 +48,8 @@ local err = mt.header(conn, "Date", os.date("%a, %d %b %Y %H:%M:%S %Z")) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) +-- TODO Add headers here to experiment + local err = mt.eoh(conn) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) diff --git a/tests/live.rs b/tests/live.rs index e7a8d84..441801d 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -10,8 +10,8 @@ use spamassassin_milter::*; #[ignore] // use option `--include-ignored` to run fn live() { // Without `spamc_args` set, `spamc` will try to connect to the default - // `spamd` port 783 (see also /etc/services). - let config = Config::builder().build(); + // `spamd` port 783 (see also `/etc/services`). + let config = Default::default(); let miltertest = spawn_miltertest_runner(file!()); diff --git a/tests/loopback_connection.rs b/tests/loopback_connection.rs index 70b0e4c..156f9ac 100644 --- a/tests/loopback_connection.rs +++ b/tests/loopback_connection.rs @@ -5,7 +5,7 @@ use spamassassin_milter::*; #[test] fn loopback_connection() { - let config = Config::builder().build(); + let config = Default::default(); let miltertest = spawn_miltertest_runner(file!()); diff --git a/tests/reject_spam.lua b/tests/reject_spam.lua new file mode 100644 index 0000000..e80ef22 --- /dev/null +++ b/tests/reject_spam.lua @@ -0,0 +1,69 @@ +-- A spam message is rejected with an SMTP error reply. + +conn = mt.connect("inet:3333@localhost") +assert(conn, "could not open connection") + +local err = mt.conninfo(conn, "client.gluet.ch", "123.123.123.123") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +local err = mt.helo(conn, "mail.gluet.ch") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +local err = mt.mailfrom(conn, "from@gluet.ch") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +local err = mt.rcptto(conn, "to@gluet.ch") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +SMFIC_DATA = string.byte("T") -- SMFIC_DATA not exported by miltertest +local err = mt.macro(conn, SMFIC_DATA, + "i", "1234567ABC", + "j", "localhost", + "_", "client.gluet.ch [123.123.123.123]", + "{tls_version}", "TLSv1.2", + "v", "Postfix 3.3.0") +assert(err == nil, err) + +local err = mt.data(conn) +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +local err = mt.header(conn, "From", "from@gluet.ch") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "To", "to@gluet.ch") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "Subject", "Test message") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "Message-ID", string.format("<%06d@gluet.ch>", math.random(999999))) +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "Date", os.date("%a, %d %b %Y %H:%M:%S %Z")) +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +local err = mt.eoh(conn) +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +local err = mt.bodystring(conn, "Test message body") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) + +-- A `miltertest` (or milter protocol?) pitfall. Even though we return the +-- `SMFIR_REJECT` status in the application code, because we use a custom error +-- reply, we must check for `SMFIR_REPLYCODE` instead. +local err = mt.eom(conn) +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_REPLYCODE) + +assert(mt.eom_check(conn, MT_SMTPREPLY, "550", "5.7.1", "Spam message refused")) + +local err = mt.disconnect(conn) +assert(err == nil, err) diff --git a/tests/reject_spam.rs b/tests/reject_spam.rs new file mode 100644 index 0000000..ac2ce18 --- /dev/null +++ b/tests/reject_spam.rs @@ -0,0 +1,24 @@ +mod common; + +pub use common::*; // `pub` only to silence unused code warnings +use spamassassin_milter::*; + +#[test] +fn reject_spam() { + let mut builder = Config::builder(); + builder.set_reject_spam(true); + builder.set_spamc_args(vec![format!("--port={}", SPAMD_PORT)]); + let config = builder.build(); + + let server = spawn_mock_spamd_server(SPAMD_PORT, |spam| { + Err(spam.replacen("\r\n\r\n", "\r\nX-Spam-Flag: YES\r\n\r\n", 1)) + }); + let miltertest = spawn_miltertest_runner(file!()); + + run("inet:3333@localhost", config).expect("milter execution failed"); + + let exit_code = miltertest.join().expect("panic in miltertest runner"); + assert!(exit_code.success(), "miltertest returned error exit code"); + + server.join().expect("panic in mock spamd server"); +} diff --git a/tests/spam_flow.lua b/tests/spam_flow.lua index 8f45399..5cb80ad 100644 --- a/tests/spam_flow.lua +++ b/tests/spam_flow.lua @@ -50,12 +50,15 @@ assert(mt.getreply(conn) == SMFIR_CONTINUE) local err = mt.header(conn, "X-Spam-Checker-Version", "BogusChecker 1.0.0") assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) +local err = mt.header(conn, "X-Spam-Report", "Bogus report") +assert(err == nil, err) +assert(mt.getreply(conn) == SMFIR_CONTINUE) local err = mt.eoh(conn) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) -local err = mt.bodystring(conn, "You have just won a billion dollars!!!!") +local err = mt.bodystring(conn, "HI!!! You have won a BILLION dollars!!!!") assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_CONTINUE) @@ -63,9 +66,13 @@ local err = mt.eom(conn) assert(err == nil, err) assert(mt.getreply(conn) == SMFIR_ACCEPT) -assert(mt.eom_check(conn, MT_HDRCHANGE, "X-Spam-Checker-Version", "MyChecker 1.0.0")) -assert(mt.eom_check(conn, MT_HDRADD, "X-Spam-Custom", "Custom-Value")) -assert(mt.eom_check(conn, MT_BODYCHANGE)) +assert(mt.eom_check(conn, MT_HDRCHANGE, "Subject", " [SPAM] Test message")) +assert(mt.eom_check(conn, MT_HDRCHANGE, "X-Spam-Checker-Version", " MyChecker 1.0.0")) +assert(mt.eom_check(conn, MT_HDRADD, "X-Spam-Flag", " YES")) +assert(mt.eom_check(conn, MT_HDRADD, "X-Spam-Custom", " Custom-Value")) +assert(mt.eom_check(conn, MT_HDRDELETE, "X-Spam-Report")) +assert(mt.eom_check(conn, MT_HDRADD, "Content-Type", " multipart/mixed; ...")) +assert(mt.eom_check(conn, MT_BODYCHANGE, "Spam detection software has identified ...")) local err = mt.disconnect(conn) assert(err == nil, err) diff --git a/tests/spam_flow.rs b/tests/spam_flow.rs index d1a1244..9da1f83 100644 --- a/tests/spam_flow.rs +++ b/tests/spam_flow.rs @@ -9,15 +9,36 @@ fn spam_flow() { builder.set_spamc_args(vec![format!("--port={}", SPAMD_PORT)]); let config = builder.build(); - let server = spawn_mock_spamd_server(SPAMD_PORT, |s| { - Err( - s.replacen("Subject: Test message\r\n", "Subject: [SPAM] Test message\r\n", 1) - .replacen("\r\n\r\n", "\r\n\ - X-Spam-Checker-Version: MyChecker 1.0.0\r\n\ - X-Spam-Flag: YES\r\n\ - X-Spam-Custom: Custom-Value\r\n\ - \r\n", 1) - ) + let server = spawn_mock_spamd_server(SPAMD_PORT, |spam| { + let mut spam = spam + .replacen("Subject: Test message\r\n", "Subject: [SPAM] Test message\r\n", 1) + .replacen( + "X-Spam-Checker-Version: BogusChecker 1.0.0\r\n", + "X-Spam-Checker-Version: MyChecker 1.0.0\r\n", + 1, + ) + .replacen( + "X-Spam-Report: Bogus report\r\n", + "", + 1, + ) + .replacen( + "\r\n\r\n", + "\r\n\ + X-Spam-Flag: YES\r\n\ + X-Spam-Custom: Custom-Value\r\n\ + Content-Type: multipart/mixed; ...\r\n\ + \r\n", + 1, + ); + + // Replace the message body with the report … + spam.replace_range( + (spam.find("\r\n\r\n").unwrap() + 4).., + "Spam detection software has identified ..." + ); + + Err(spam) }); let miltertest = spawn_miltertest_runner(file!()); diff --git a/tests/spamc_connection_error.rs b/tests/spamc_connection_error.rs index d3ec417..bc6ff28 100644 --- a/tests/spamc_connection_error.rs +++ b/tests/spamc_connection_error.rs @@ -6,7 +6,7 @@ use spamassassin_milter::*; #[test] fn spamc_connection_error() { let mut builder = Config::builder(); - // spamc always ‘works’ even if it cannot actually reach spamd! + // `spamc` always ‘works’ even if it cannot actually reach `spamd`! // `--no-safe-fallback` prevents this masking of connection errors. // TODO Also try with non-existing flag, behaviour is different? builder.set_spamc_args(vec![