From e48bcd3b57ca4e353c35172ddc2334ecb237c349 Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Thu, 2 May 2024 11:45:07 -0700 Subject: [PATCH] return 550 for relay_to=false, log_oob=true case When `log_arf` or `log_oob` are set to true with `relay_to=false`, we now return a 550 error response for messages that are not ARF or OOB reports. Previously, we would return a 250 response and silently drop the message in this case, which gave the false impression that it was accepted for relaying. Expand integration test to explicitly assert that the right things are allowed/denied/relayed/parsed. --- crates/integration-tests/source.lua | 17 ++++++- crates/integration-tests/src/main.rs | 71 +++++++++++++++++++++++----- crates/kumod/src/smtp_server.rs | 27 ++++++++--- docs/changelog/main.md | 4 ++ 4 files changed, 101 insertions(+), 18 deletions(-) diff --git a/crates/integration-tests/source.lua b/crates/integration-tests/source.lua index d053fc9f..859e31c4 100644 --- a/crates/integration-tests/source.lua +++ b/crates/integration-tests/source.lua @@ -4,13 +4,20 @@ local kumo = require 'kumo' local TEST_DIR = os.getenv 'KUMOD_TEST_DIR' local SINK_PORT = tonumber(os.getenv 'KUMOD_SMTP_SINK_PORT') local WEBHOOK_PORT = os.getenv 'KUMOD_WEBHOOK_PORT' +local LISTENER_MAP = os.getenv 'KUMOD_LISTENER_DOMAIN_MAP' kumo.on('init', function() kumo.configure_accounting_db_path(TEST_DIR .. '/accounting.db') + local relay_hosts = { '0.0.0.0/0' } + local RELAY_HOSTS = os.getenv 'KUMOD_RELAY_HOSTS' + if RELAY_HOSTS then + relay_hosts = kumo.json_parse(RELAY_HOSTS) + end + kumo.start_esmtp_listener { listen = '127.0.0.1:0', - relay_hosts = { '0.0.0.0/0' }, + relay_hosts = relay_hosts, } kumo.start_http_listener { @@ -79,6 +86,14 @@ if WEBHOOK_PORT then end kumo.on('get_listener_domain', function(domain, listener, conn_meta) + if LISTENER_MAP then + local map = kumo.json_parse(LISTENER_MAP) + local params = map[domain] + if params then + return kumo.make_listener_domain(params) + end + end + return kumo.make_listener_domain { relay_to = true, log_oob = true, diff --git a/crates/integration-tests/src/main.rs b/crates/integration-tests/src/main.rs index ec8a1bf7..f982601f 100644 --- a/crates/integration-tests/src/main.rs +++ b/crates/integration-tests/src/main.rs @@ -19,6 +19,7 @@ mod test { use kumo_log_types::RecordType::{Bounce, Delivery, Reception, TransientFailure}; use mailparsing::DecodedBody; use rfc5321::*; + use serde_json::json; use std::collections::BTreeMap; use std::time::Duration; @@ -577,11 +578,28 @@ Some( Ok(()) } + fn json_string(v: serde_json::Value) -> String { + serde_json::to_string(&v).unwrap() + } + #[tokio::test] async fn log_oob_arf() -> anyhow::Result<()> { - let mut daemon = DaemonWithMaildir::start() - .await - .context("DaemonWithMaildir::start")?; + let mut daemon = DaemonWithMaildir::start_with_env(vec![ + ( + "KUMOD_LISTENER_DOMAIN_MAP", + &json_string(json!({ + "example.com": { + "relay_to": false, + "log_oob": true, + "log_arf": true, + } + + })), + ), + ("KUMOD_RELAY_HOSTS", &json_string(json!([]))), + ]) + .await + .context("DaemonWithMaildir::start")?; eprintln!("sending message"); let mut client = daemon.smtp_client().await.context("make smtp_client")?; @@ -600,6 +618,40 @@ Some( anyhow::ensure!(response.code == 250); } + { + // and verify that relaying for !report is not allowed + let body = generate_message_text(1024, 78); + let response = MailGenParams { + body: Some(&body), + ..Default::default() + } + .send(&mut client) + .await + .unwrap_err(); + k9::snapshot!( + response, + r#" +Rejected( + Response { + code: 550, + enhanced_code: Some( + EnhancedStatusCode { + class: 5, + subject: 7, + detail: 1, + }, + ), + content: "relaying not permitted", + command: Some( + ".\r +", + ), + }, +) +"# + ); + } + daemon .wait_for_maildir_count(2, Duration::from_secs(10)) .await; @@ -613,15 +665,12 @@ Some( " DeliverySummary { source_counts: { - Reception: 1, - Delivery: 2, + Reception: 2, OOB: 1, Feedback: 1, + Rejection: 1, }, - sink_counts: { - Reception: 2, - Delivery: 2, - }, + sink_counts: {}, } " ); @@ -629,8 +678,8 @@ DeliverySummary { daemon.source.accounting_stats()?, " AccountingStats { - received: 1, - delivered: 2, + received: 2, + delivered: 0, } " ); diff --git a/crates/kumod/src/smtp_server.rs b/crates/kumod/src/smtp_server.rs index 00347047..d7b25eb4 100644 --- a/crates/kumod/src/smtp_server.rs +++ b/crates/kumod/src/smtp_server.rs @@ -1456,6 +1456,8 @@ impl SmtpServer { // get to work on logging and injecting into the queues let mut messages = vec![]; + let mut was_arf_or_oob = false; + for message in accepted_messages { if self.params.trace_headers.supplemental_header { let mut object = json!({ @@ -1511,6 +1513,13 @@ impl SmtpServer { message.save().await?; } } + + if relay_disposition.log_arf && matches!(message.parse_rfc5965(), Ok(Some(_))) { + was_arf_or_oob = true; + } else if relay_disposition.log_oob && matches!(message.parse_rfc3464(), Ok(Some(_))) { + was_arf_or_oob = true; + } + log_disposition(LogDisposition { kind: RecordType::Reception, msg: message.clone(), @@ -1539,14 +1548,20 @@ impl SmtpServer { } } - if !messages.is_empty() { - for (queue_name, msg) in messages { - QueueManager::insert(&queue_name, msg).await?; - } + let relayed_any = !messages.is_empty(); + + for (queue_name, msg) in messages { + QueueManager::insert(&queue_name, msg).await?; + } + + if !relayed_any && !was_arf_or_oob { + self.write_response(550, "5.7.1 relaying not permitted") + .await?; + } else { + let ids = ids.join(" "); + self.write_response(250, format!("OK ids={ids}")).await?; } - let ids = ids.join(" "); - self.write_response(250, format!("OK ids={ids}")).await?; Ok(()) } } diff --git a/docs/changelog/main.md b/docs/changelog/main.md index 476e7f73..e3eb4b8b 100644 --- a/docs/changelog/main.md +++ b/docs/changelog/main.md @@ -103,3 +103,7 @@ and sending to domain(s) with an MTA-STS policy. * If a TSA rule suspends a ready queue in response to a 421-before-MAIL-FROM, the contents of the corresponding ready queue could get stuck +* When `log_arf` or `log_oob` are set to true with `relay_to=false`, we now return + a 550 error response for messages that are not ARF or OOB reports. + Previously, we would return a 250 response and silently drop the message in this case, + which gave the false impression that it was accepted for relaying.