Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maddy fails to validate SPF record with long lookup chain #487

Closed
moritzheiber opened this issue May 15, 2022 · 11 comments
Closed

maddy fails to validate SPF record with long lookup chain #487

moritzheiber opened this issue May 15, 2022 · 11 comments
Assignees
Labels
bug Something isn't working. ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele release-blocker Critical problem that should not go as a "known issue" into any release
Milestone

Comments

@moritzheiber
Copy link

Describe the bug

When receiving email with a standard-compliant SPF header maddy fails to validate the SPF record because it considers it to have too many lookups. It should fail to deliver the message though since the SPF header is considered valid.

Steps to reproduce

The SPF record is considered valid when using popular tools to verify it. It's a rather long chain, but it's compliant as far as I can tell. And yes, it is lacking a valid DKIM signature (and therefore doesn't pass the DMARC check).

Google's servers are receiving email from the same domain without any issues.

Log files

2022-05-15T09:33:06.307Z smtp: incoming message	{"msg_id":"f0359554","sender":"[email protected]","src_host":"mail2.km-it.de","src_ip":"130.180.63.115:37838"}
2022-05-15T09:33:06.344Z smtp: RCPT ok	{"msg_id":"f0359554","rcpt":"<redacted>"}
2022-05-15T09:33:06.419Z smtp/pipeline: no check action	{"check":"check.dkim","msg_id":"f0359554","reason":"No DKIM signatures","smtp_code":550,"smtp_enchcode":"5.7.20","smtp_msg":"No DKIM signatures"}
2022-05-15T09:33:07.652Z smtp: DATA error	{"check":"check.spf","msg_id":"f0359554","reason":"lookup limit reached","smtp_code":550,"smtp_enchcode":"5.7.23","smtp_msg":"SPF authentication failed with a permanent error"}

Configuration file

$(hostname) = mail.<redacted>
$(primary_domain) = <redacted>
$(local_domains) = $(primary_domain)

log stderr_ts

tls file <redacted>

hostname $(hostname)

table.chain local_rewrites {
    optional_step regexp "(.+)\+(.+)@(.+)" "$1@$3"
    optional_step static {
        entry postmaster postmaster@$(primary_domain)
    }
    optional_step file /etc/maddy/aliases
}

msgpipeline local_routing {
    destination postmaster $(local_domains) {
        modify {
            replace_rcpt &local_rewrites
        }

        deliver_to &local_mailboxes
    }

    default_destination {
        reject 550 5.1.1 "User doesn't exist"
    }
}

smtp tcp://[::]:25 {
    limits {
        # Up to 20 msgs/sec across max. 10 SMTP connections.
        all rate 20 1s
        all concurrency 10
    }

    dmarc yes
    check {
        require_mx_record
        dkim
        spf
        rspamd {
            api_path http://rspamd:11332
            io_error_action ignore
            error_resp_action ignore
            add_header_action quarantine
            rewrite_subj_action quarantine
            flags pass_all
        }
    }

    source $(local_domains) {
        reject 501 5.1.8 "Use Submission for outgoing SMTP"
    }
    default_source {
        destination postmaster $(local_domains) {
            deliver_to &local_routing
        }
        default_destination {
            reject 550 5.1.1 "User doesn't exist"
        }
    }
}

submission tcp://[::]:587 {
    auth dovecot_sasl tcp://dovecot:5520

    limits {
      all rate 50 1s
    }

    source $(local_domains) {
        check {
            authorize_sender {
                prepare_email &local_rewrites
                user_to_email identity
            }
        }

        destination postmaster $(local_domains) {
            deliver_to &local_routing
        }
        default_destination {
            modify {
                dkim {
                    domains $(primary_domain) $(local_domains)
                    selector ed25519
                    key_path dkim-keys/{domain}-{selector}.key
                }
                dkim {
                    domains $(primary_domain) $(local_domains)
                    selector rsa
                    key_path dkim-keys/{domain}-{selector}.key
                }
            }
            deliver_to &remote_queue
        }
    }
    default_source {
        reject 501 5.1.8 "Non-local sender domain"
    }
}

target.remote outbound_delivery {
    limits {
        # Up to 20 msgs/sec across max. 10 SMTP connections
        # for each recipient domain.
        destination rate 20 1s
        destination concurrency 10
    }
    mx_auth {
        dane
        mtasts {
            cache fs
            fs_dir mtasts_cache/
        }
        local_policy {
            min_tls_level encrypted
            min_mx_level none
        }
    }
}

target.queue remote_queue {
    target &outbound_delivery

    autogenerated_msg_domain $(primary_domain)
    bounce {
        destination postmaster $(local_domains) {
            deliver_to &local_routing
        }
        default_destination {
            reject 550 5.0.0 "Refusing to send DSNs to non-local addresses"
        }
    }
}

target.lmtp local_mailboxes {
    targets tcp://dovecot:5500
}

Environment information

  • maddy version: 0.5.4
@moritzheiber moritzheiber added the bug Something isn't working. label May 15, 2022
@moritzheiber moritzheiber changed the title Bug report maddy fails to validate SPF record with long lookup chain May 15, 2022
@foxcpp
Copy link
Owner

foxcpp commented Jun 18, 2022

Related to #485 (maddy not ignoring permerror status as most services do).

@foxcpp foxcpp self-assigned this Jun 18, 2022
@foxcpp
Copy link
Owner

foxcpp commented Jun 18, 2022

From RFC 7208:

SPF implementations MUST limit the total number of those terms to 10 during SPF evaluation, to avoid unreasonable load on the DNS. If this limit is exceeded, the implementation MUST return "permerror".

Seems like I am a month late - currently set SPF record for that domain cannot cause more than 9 lookups.

@foxcpp
Copy link
Owner

foxcpp commented Jun 18, 2022

Closing as non-actionable.

@foxcpp foxcpp closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2022
@moritzheiber
Copy link
Author

@foxcpp Appreciate the reply.. have you checked on whether maddy actually handles that record? Might be that the implementation doesn’t fully conform to the expectation of the RFC..? I’ve seen this error with other domains (and meanwhile have set permerror to “ignore”)

@foxcpp foxcpp reopened this Jun 18, 2022
@foxcpp
Copy link
Owner

foxcpp commented Jun 18, 2022

Woops, I double-checked what was I doing and apparently I passed parameters in the wrong format to the SPF library.

Here is the list of DNS queries maddy does while checking 130.180.63.115 against mediterana.de SPF record. SPF library used by maddy counts A+AAAA queries as one and actually does 14 queries before stopping.

TXT mediterana.de
A/AAAA mediterana.de
MX mediterana.de
A/AAAA mx01.oevermann.de.   <-- I forgot that these queries would need to be done while counting them manually
A/AAAA mx03.oevermann.de.
TXT _spf.on.services
TXT servers.mcsv.net
TXT shops.shopify.com
TXT spf.sendinblue.com
TXT spf.crsend.com
TXT mail2.km-it.de
*stopped, limit hit*

@moritzheiber
Copy link
Author

Ah, that’s a tricky one, good catch! Mixed IPv4/6 is a nightmare 🙈

@foxcpp
Copy link
Owner

foxcpp commented Jun 18, 2022

It gets even more interesting.

Apparently, RFC 4408 text means that the limit is in terms of SPF "terms", not DNS lookups.
RFC 7208 uses a more clear wording for it:

Some mechanisms and modifiers (collectively, "terms") cause DNS
queries at the time of evaluation, and some do not. The following
terms cause DNS queries: the "include", "a", "mx", "ptr", and
"exists" mechanisms, and the "redirect" modifier. SPF
implementations MUST limit the total number of those terms to 10
during SPF evaluation, to avoid unreasonable load on the DNS.

In this case, the problematic record has only 8 out of 10 allowed terms.

@albertito, is this intentional that blitiri.com.ar/go/spf counts DNS lookups and not terms?

@foxcpp foxcpp added this to the 0.6 milestone Jun 18, 2022
@foxcpp foxcpp added the release-blocker Critical problem that should not go as a "known issue" into any release label Jun 18, 2022
@albertito
Copy link

albertito commented Jun 18, 2022

Sorry about this, the problem is indeed caused by a bug in the SPF library, in the way that some terms are counted for the purposes of DNS lookup limits.

Coincidentally, this problem was reported yesterday and fixed today in commit 48ee700, which is in the next branch for now.

So far I know of two domains affected, microsoft.com (from yesterday's bug report) and mediterana.de (reported in here).

While looking at it, I've identified two other minor bugs related to how the DNS lookup limit was counted. I think those are more rare to hit in practice, but in any case, they've also been fixed in the next branch.

With the fixes, both domains now work fine.

Given it seems to be affecting now more than one person, and two domains, I plan to cut a release tomorrow, once I've done a bit more real-life testing just in case.

I hope this helps, please let me know if there are any issues, questions or concerns; and sorry again for the hassle!

@moritzheiber
Copy link
Author

@albertito Fantastic response time, thank you for your hard work!

@albertito
Copy link

FYI I've just cut v1.4.0 of the spf package which includes a fix for this problem.

@foxcpp
Copy link
Owner

foxcpp commented Jun 19, 2022

Good. Thanks, @albertito! I will bump version used in maddy and probably tag 0.6 finally.

foxcpp added a commit that referenced this issue Jun 23, 2022
@foxcpp foxcpp added the ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele label Jun 23, 2022
@foxcpp foxcpp closed this as completed Jun 23, 2022
shift pushed a commit to shift/maddy that referenced this issue Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele release-blocker Critical problem that should not go as a "known issue" into any release
Projects
None yet
Development

No branches or pull requests

3 participants