From 452bede2e15efee8de67987c0ccdd9bc1b03e71d Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Wed, 23 Aug 2023 05:58:40 -0700 Subject: [PATCH] dkim: switch to our own mailparsing implementation --- Cargo.lock | 2 +- crates/dkim/Cargo.toml | 2 +- crates/dkim/examples/sign_bench.rs | 2 +- crates/dkim/src/hash.rs | 66 ++++++++++++------------------ crates/dkim/src/lib.rs | 32 +++++++-------- crates/dkim/src/parsed_email.rs | 49 ++++++++++------------ crates/dkim/src/roundtrip_test.rs | 4 +- crates/dkim/src/sign.rs | 6 +-- crates/mailparsing/src/lib.rs | 2 +- crates/message/src/dkim.rs | 3 +- 10 files changed, 74 insertions(+), 94 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bc6ca0ea..10599284 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2307,7 +2307,7 @@ dependencies = [ "futures", "indexmap 1.9.3", "k9", - "mailparse", + "mailparsing", "memchr", "nom 7.1.3", "once_cell", diff --git a/crates/dkim/Cargo.toml b/crates/dkim/Cargo.toml index 5333cf6e..87ae8f13 100644 --- a/crates/dkim/Cargo.toml +++ b/crates/dkim/Cargo.toml @@ -20,7 +20,7 @@ chrono = { version = "0.4.26", default-features = false, features = ["clock", "s ed25519-dalek = {workspace=true, features=["pkcs8"]} futures = "0.3.28" indexmap = "1.9.3" -mailparse = "0.14" +mailparsing = { path="../mailparsing" } memchr = "2.5" nom = "7.1.0" once_cell = "1.17" diff --git a/crates/dkim/examples/sign_bench.rs b/crates/dkim/examples/sign_bench.rs index 8b84f4e2..0fe4863d 100644 --- a/crates/dkim/examples/sign_bench.rs +++ b/crates/dkim/examples/sign_bench.rs @@ -53,7 +53,7 @@ ipsum dolor sit a. fn main() { let email_text = email_text(); - let email = ParsedEmail::parse_bytes(email_text.as_bytes()).unwrap(); + let email = ParsedEmail::parse(email_text).unwrap(); for canon in [Type::Simple, Type::Relaxed] { let private_key = diff --git a/crates/dkim/src/hash.rs b/crates/dkim/src/hash.rs index a3756883..11d13d90 100644 --- a/crates/dkim/src/hash.rs +++ b/crates/dkim/src/hash.rs @@ -103,7 +103,7 @@ pub(crate) fn compute_body_hash<'a>( hash_algo: HashAlgo, email: &'a ParsedEmail<'a>, ) -> Result { - let body = email.get_body_bytes(); + let body = email.get_body(); let limit = length.unwrap_or(usize::MAX); let mut hasher = LimitHasher { @@ -112,7 +112,7 @@ pub(crate) fn compute_body_hash<'a>( hashed: 0, }; - canonicalization_type.canon_body(body, &mut hasher); + canonicalization_type.canon_body(body.as_bytes(), &mut hasher); Ok(hasher.finalize()) } @@ -158,11 +158,7 @@ impl HeaderList { /// Apply `apply` to each header in the provided email that /// matches the headers, follow the order set out in Section 5.4.2 - fn apply<'a, F: FnMut(std::borrow::Cow<'a, str>, &'a [u8])>( - &self, - email: &'a ParsedEmail, - apply: F, - ) { + fn apply<'a, F: FnMut(&'a str, &'a [u8])>(&self, email: &'a ParsedEmail, apply: F) { match self { Self::MaybeMultiple(list) => Self::apply_multiple(list, email, apply), Self::Unique(list) => Self::apply_unique(list, email, apply), @@ -172,7 +168,7 @@ impl HeaderList { /// Perform the apply when we know that the list of header names /// are unique. /// We can avoid allocating any additional state for this case. - fn apply_unique<'a, F: FnMut(std::borrow::Cow<'a, str>, &'a [u8])>( + fn apply_unique<'a, F: FnMut(&'a str, &'a [u8])>( header_list: &[String], email: &'a ParsedEmail, mut apply: F, @@ -181,8 +177,8 @@ impl HeaderList { 'outer: for name in header_list { for header in email_headers.iter().rev() { - if header.get_key_ref().eq_ignore_ascii_case(&name) { - apply(header.get_key_ref(), header.get_value_raw()); + if header.get_name().eq_ignore_ascii_case(&name) { + apply(header.get_name(), header.get_raw_value().as_bytes()); continue 'outer; } } @@ -198,7 +194,7 @@ impl HeaderList { /// To facilitate this, we need to maintain state for each header name /// in the list to ensure that we select the appropriate header in the /// appropriate order. - fn apply_multiple<'a, F: FnMut(std::borrow::Cow<'a, str>, &'a [u8])>( + fn apply_multiple<'a, F: FnMut(&'a str, &'a [u8])>( header_list: &[String], email: &'a ParsedEmail, mut apply: F, @@ -218,8 +214,8 @@ impl HeaderList { .rev() .skip(num_headers - index) { - if header.get_key_ref().eq_ignore_ascii_case(&name) { - apply(header.get_key_ref(), header.get_value_raw()); + if header.get_name().eq_ignore_ascii_case(&name) { + apply(header.get_name(), header.get_raw_value().as_bytes()); last_index.insert(name, header_index); continue 'outer; } @@ -286,7 +282,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(email).unwrap(); let canonicalization_type = canonicalization::Type::Simple; let length = None; @@ -311,7 +307,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(email).unwrap(); let canonicalization_type = canonicalization::Type::Relaxed; let length = None; @@ -336,7 +332,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(email).unwrap(); let canonicalization_type = canonicalization::Type::Relaxed; let length = Some(3); @@ -354,7 +350,7 @@ Hello Alice #[test] fn test_compute_body_hash_empty_simple() { - let email = ParsedEmail::parse_bytes(b"Subject: nothing\r\n\r\n").unwrap(); + let email = ParsedEmail::parse("Subject: nothing\r\n\r\n").unwrap(); let canonicalization_type = canonicalization::Type::Simple; let length = None; @@ -372,7 +368,7 @@ Hello Alice #[test] fn test_compute_body_hash_empty_relaxed() { - let email = ParsedEmail::parse_bytes(b"Subject: nothing\r\n\r\n").unwrap(); + let email = ParsedEmail::parse("Subject: nothing\r\n\r\n").unwrap(); let canonicalization_type = canonicalization::Type::Relaxed; let length = None; @@ -397,7 +393,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(email).unwrap(); let canonicalization_type = canonicalization::Type::Simple; let hash_algo = HashAlgo::RsaSha1; @@ -442,7 +438,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(email).unwrap(); let canonicalization_type = canonicalization::Type::Relaxed; let hash_algo = HashAlgo::RsaSha1; @@ -480,18 +476,14 @@ Hello Alice #[test] fn test_get_body() { - let email = ParsedEmail::parse_bytes("Subject: A\r\n\r\nContent\n.hi\n.hello..".as_bytes()) - .unwrap(); - assert_eq!( - String::from_utf8_lossy(email.get_body_bytes()), - "Content\n.hi\n.hello..".to_owned() - ); + let email = ParsedEmail::parse("Subject: A\r\n\r\nContent\n.hi\n.hello..").unwrap(); + assert_eq!(email.get_body(), "Content\n.hi\n.hello.."); } fn select_headers<'a>( header_list: &HeaderList, email: &'a ParsedEmail, - ) -> Vec<(std::borrow::Cow<'a, str>, &'a [u8])> { + ) -> Vec<(&'a str, &'a [u8])> { let mut result = vec![]; header_list.apply(email, |key, value| { result.push((key, value)); @@ -507,10 +499,9 @@ Hello Alice "to".to_string(), ]); - let email1 = ParsedEmail::parse_bytes( - b"from: biz\r\nfoo: bar\r\nfrom: baz\r\nsubject: boring\r\n\r\ntest", - ) - .unwrap(); + let email1 = + ParsedEmail::parse("from: biz\r\nfoo: bar\r\nfrom: baz\r\nsubject: boring\r\n\r\ntest") + .unwrap(); let result1 = select_headers(&header_list, &email1); assert_eq!( @@ -522,8 +513,7 @@ Hello Alice ); let email2 = - ParsedEmail::parse_bytes(b"From: biz\r\nFoo: bar\r\nSubject: Boring\r\n\r\ntest") - .unwrap(); + ParsedEmail::parse("From: biz\r\nFoo: bar\r\nSubject: Boring\r\n\r\ntest").unwrap(); let result2 = select_headers(&header_list, &email2); assert_eq!( @@ -544,10 +534,9 @@ Hello Alice "from".to_string(), ]); - let email1 = ParsedEmail::parse_bytes( - b"from: biz\r\nfoo: bar\r\nfrom: baz\r\nsubject: boring\r\n\r\ntest", - ) - .unwrap(); + let email1 = + ParsedEmail::parse("from: biz\r\nfoo: bar\r\nfrom: baz\r\nsubject: boring\r\n\r\ntest") + .unwrap(); let result1 = select_headers(&header_list, &email1); assert_eq!( @@ -560,8 +549,7 @@ Hello Alice ); let email2 = - ParsedEmail::parse_bytes(b"From: biz\r\nFoo: bar\r\nSubject: Boring\r\n\r\ntest") - .unwrap(); + ParsedEmail::parse("From: biz\r\nFoo: bar\r\nSubject: Boring\r\n\r\ntest").unwrap(); let result2 = select_headers(&header_list, &email2); assert_eq!( diff --git a/crates/dkim/src/lib.rs b/crates/dkim/src/lib.rs index 57fb0f47..53422afe 100644 --- a/crates/dkim/src/lib.rs +++ b/crates/dkim/src/lib.rs @@ -11,8 +11,6 @@ use sha1::Sha1; use sha2::Sha256; use trust_dns_resolver::TokioAsyncResolver; -use mailparse::MailHeaderMap; - #[macro_use] extern crate quick_error; @@ -231,8 +229,8 @@ pub async fn verify_email_with_resolver<'a>( ) -> Result { let mut last_error = None; - for h in email.get_headers().get_all_headers(HEADER) { - let value = String::from_utf8_lossy(h.get_value_raw()); + for h in email.get_headers().iter_named(HEADER) { + let value = h.get_raw_value(); tracing::debug!("checking signature {:?}", value); let dkim_header = match DKIMHeader::parse(&value) { @@ -429,20 +427,19 @@ We lost the game. Are you hungry yet? Joe."# .replace('\n', "\r\n"); - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); - let h = email + let email = ParsedEmail::parse(raw_email).unwrap(); + let raw_header_dkim = email .get_headers() - .get_all_headers(HEADER) - .first() + .iter_named(HEADER) + .next() .unwrap() - .get_value_raw(); - let raw_header_dkim = String::from_utf8_lossy(h); + .get_raw_value(); let resolver = MockResolver::new(); let dkim_verify_result = verify_email_header( &resolver, - &DKIMHeader::parse(&raw_header_dkim).unwrap(), + &DKIMHeader::parse(raw_header_dkim).unwrap(), &email, ) .await; @@ -479,20 +476,19 @@ We lost the game. Are you hungry yet? Joe. "# .replace('\n', "\r\n"); - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); - let h = email + let email = ParsedEmail::parse(raw_email).unwrap(); + let raw_header_rsa = email .get_headers() - .get_all_headers(HEADER) - .first() + .iter_named(HEADER) + .next() .unwrap() - .get_value_raw(); - let raw_header_rsa = String::from_utf8_lossy(h); + .get_raw_value(); let resolver = MockResolver::new(); let dkim_verify_result = verify_email_header( &resolver, - &DKIMHeader::parse(&raw_header_rsa).unwrap(), + &DKIMHeader::parse(raw_header_rsa).unwrap(), &email, ) .await; diff --git a/crates/dkim/src/parsed_email.rs b/crates/dkim/src/parsed_email.rs index df401b75..e931f8ed 100644 --- a/crates/dkim/src/parsed_email.rs +++ b/crates/dkim/src/parsed_email.rs @@ -1,51 +1,46 @@ -use mailparse::MailHeader; -use memchr::memmem::Finder; -use once_cell::sync::Lazy; - -static CRLFCRLF: Lazy = Lazy::new(|| memchr::memmem::Finder::new("\r\n\r\n")); +use mailparsing::{ + Header, HeaderConformance, HeaderMap, HeaderParseResult, MimePart, SharedString, +}; pub enum ParsedEmail<'a> { - FullyParsed(mailparse::ParsedMail<'a>), + FullyParsed(MimePart<'a>), HeaderOnlyParse { - headers: Vec>, - body_bytes: &'a [u8], + parsed: HeaderParseResult<'a>, + bytes: SharedString<'a>, }, } -impl<'a> From> for ParsedEmail<'a> { - fn from(mail: mailparse::ParsedMail<'a>) -> Self { +impl<'a> From> for ParsedEmail<'a> { + fn from(mail: MimePart<'a>) -> Self { Self::FullyParsed(mail) } } impl<'a> ParsedEmail<'a> { - pub fn parse_bytes(bytes: &'a [u8]) -> Option { - if CRLFCRLF.find(bytes).is_none() { + pub fn parse>>(bytes: S) -> Option { + let bytes: SharedString = bytes.into(); + let parsed = Header::parse_headers(bytes.clone()).ok()?; + if parsed + .overall_conformance + .contains(HeaderConformance::NON_CANONICAL_LINE_ENDINGS) + { // Canonical line endings are required, but are missing return None; } - - let (headers, offset) = mailparse::parse_headers(bytes).ok()?; - Some(Self::HeaderOnlyParse { - headers, - body_bytes: &bytes[offset..], - }) + Some(Self::HeaderOnlyParse { parsed, bytes }) } - pub fn get_body_bytes(&self) -> &[u8] { + pub fn get_body(&'a self) -> SharedString<'a> { match self { - Self::FullyParsed(email) => CRLFCRLF - .find(email.raw_bytes) - .map(|idx| &email.raw_bytes[idx + 4..]) - .unwrap_or(b""), - Self::HeaderOnlyParse { body_bytes, .. } => body_bytes, + Self::FullyParsed(email) => email.raw_body(), + Self::HeaderOnlyParse { bytes, parsed } => bytes.slice(parsed.body_offset..bytes.len()), } } - pub fn get_headers(&self) -> &[MailHeader<'a>] { + pub fn get_headers(&'a self) -> &HeaderMap<'a> { match self { - Self::FullyParsed(email) => &email.headers, - Self::HeaderOnlyParse { headers, .. } => headers, + Self::FullyParsed(email) => &email.headers(), + Self::HeaderOnlyParse { parsed, .. } => &parsed.headers, } } } diff --git a/crates/dkim/src/roundtrip_test.rs b/crates/dkim/src/roundtrip_test.rs index 7eccfa97..47347124 100644 --- a/crates/dkim/src/roundtrip_test.rs +++ b/crates/dkim/src/roundtrip_test.rs @@ -20,7 +20,7 @@ fn dkim_record() -> String { } fn sign(domain: &str, raw_email: &str) -> String { - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(raw_email).unwrap(); let private_key = DkimPrivateKey::rsa_key_file("./test/keys/2022.private").unwrap(); let time = chrono::Utc.with_ymd_and_hms(2021, 1, 1, 0, 0, 1).unwrap(); @@ -40,7 +40,7 @@ fn sign(domain: &str, raw_email: &str) -> String { } async fn verify(resolver: &dyn dns::Lookup, from_domain: &str, raw_email: &str) -> DKIMResult { - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(raw_email).unwrap(); verify_email_with_resolver(from_domain, &email, resolver) .await diff --git a/crates/dkim/src/sign.rs b/crates/dkim/src/sign.rs index 43974b25..c91b2881 100644 --- a/crates/dkim/src/sign.rs +++ b/crates/dkim/src/sign.rs @@ -297,7 +297,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(raw_email).unwrap(); let private_key = DkimPrivateKey::rsa_key_file("./test/keys/2022.private").unwrap(); let time = chrono::Utc.with_ymd_and_hms(2021, 1, 1, 0, 0, 1).unwrap(); @@ -337,7 +337,7 @@ From: Sven Sauleau Hello Alice "# .replace("\n", "\r\n"); - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(raw_email).unwrap(); let data = std::fs::read("./test/keys/2022.private").unwrap(); let pkey = openssl::rsa::Rsa::private_key_from_pem(&data).unwrap(); @@ -384,7 +384,7 @@ We lost the game. Are you hungry yet? Joe."# .replace('\n', "\r\n"); - let email = ParsedEmail::parse_bytes(raw_email.as_bytes()).unwrap(); + let email = ParsedEmail::parse(raw_email).unwrap(); let file_content = fs::read("./test/keys/ed.private").unwrap(); let file_decoded = general_purpose::STANDARD.decode(file_content).unwrap(); diff --git a/crates/mailparsing/src/lib.rs b/crates/mailparsing/src/lib.rs index c167760c..f8ad81c8 100644 --- a/crates/mailparsing/src/lib.rs +++ b/crates/mailparsing/src/lib.rs @@ -9,7 +9,7 @@ mod strings; pub use error::MailParsingError; pub type Result = std::result::Result; -pub use header::Header; +pub use header::{Header, HeaderConformance, HeaderParseResult}; pub use headermap::HeaderMap; pub use mimepart::MimePart; pub use rfc5322_parser::*; diff --git a/crates/message/src/dkim.rs b/crates/message/src/dkim.rs index d982c06b..08722e32 100644 --- a/crates/message/src/dkim.rs +++ b/crates/message/src/dkim.rs @@ -186,7 +186,8 @@ pub struct CFSigner { impl CFSigner { fn sign(&self, message: &[u8]) -> anyhow::Result { - let mail = kumo_dkim::ParsedEmail::parse_bytes(&message) + let message_str = std::str::from_utf8(message).context("message is not ASCII or UTF-8")?; + let mail = kumo_dkim::ParsedEmail::parse(message_str) .ok_or_else(|| anyhow::anyhow!("failed to parse message to pass to dkim signer"))?; let dkim_header = self.signer.sign(&mail)?;