Subtle domain verification bug that prevents MTA-STS records population

[Xombran]

I stopped using github 5 years ago but I registered a new account just to share this bug that I have been fighting for the past 2 days. Basically I am in a situation where I can't let MiaB provision it's own certificates and instead, I get my wildcard certificate pushed to the box, everything is fine and green on that front including both, the TLS (SSL) Certificates section and the Status Checks in the Admin Panel.

The only issue is that my MTA-STS records are missing from the External DNS section in the admin panel, after quickly inspecting dns_update.py, I found that these records are only added if specific conditions were met:

# ... inside the build_zone function ...

mta_sts_records = [ ]
if domain_properties[domain]["mail"] \
  and domain_properties[env["PRIMARY_HOSTNAME"]]["certificate-is-valid"] \
  and is_domain_cert_signed_and_valid("mta-sts." + domain, env):
    # ... (code to generate a policy ID) ...
    mta_sts_records.extend([
        ("_mta-sts", "TXT", "v=STSv1; id=" + mta_sts_policy_id, "Optional. Part of the MTA-STS policy for incoming mail. If set, a MTA-STS policy must also be published.")
    ])
# ...

1. domain_properties[domain]["mail"]: The domain is configured to receive mail on my box. (Which is True).
2. domain_properties[env["PRIMARY_HOSTNAME"]]["certificate-is-valid"]: The primary hostname of my box (box.mydomain.com) has a valid, signed SSL certificate. (Which is True as per the TLS (SSL) Certificates section)
3. is_domain_cert_signed_and_valid("mta-sts." + domain, env): The subdomain mta-sts.mydomain.com also has its own valid, signed SSL certificate. (Which is also True as per the TLS (SSL) Certificates section)

And yet it doesn't matter how many times I manually invoke dns_update.py --update, these records are never created! Now I had a doubt that there's a discrepancy going on, and the following is my observations:

1. The admin panel is using a smart function which is wildcard-aware. It first looks for a literal match (mta-sts.mydomain.com). When that fails, it then looks for a wildcard match (*.mydomain.com). It finds my wildcard certificate and proceeds.

# ssl_certificates.py
def get_domain_ssl_files(domain, ssl_certificates, env, ...):
    # ...
    wildcard_domain = re.sub(r"^[^\.]+", "*", domain) # Creates "*.mydomain.com"
    if domain in ssl_certificates:
        return ssl_certificates[domain]
    if wildcard_domain in ssl_certificates: # <--- BINGO!
        return ssl_certificates[wildcard_domain]
    # ... 

2. The dns_update.py is using a dumb function that never calls the smart get_domain_ssl_files function. It directly queries the dictionary returned by get_ssl_certificates using a literal key. Since the key 'mta-sts.mydomain.com' doesn't exist in that dictionary (only '*.mydomain.com' does), the check fails immediately and I don't get my MTA-STS records populated.

# A simplified view of the logic in dns_update.py
def is_cert_valid(domain, env):
    all_certs = get_ssl_certificates(env) # This is the "dumb" function
    
    # It looks for a LITERAL key. It does NOT try a wildcard.
    if domain not in all_certs:
        return False # It fails here!
        
    cert_info = all_certs[domain]
    status, _ = check_certificate(domain, cert_info['certificate'], ...)
    return status == 'OK'

My patch was modifying dns_update.py replacing the include from ssl_certificates import get_ssl_certificates, check_certificate to from ssl_certificates import get_ssl_certificates, check_certificate, get_domain_ssl_files and minimally modifying the function is_domain_cert_signed_and_valid as follows:

def is_domain_cert_signed_and_valid(domain, env):
	# Get all certificates known to the system.
	all_certs = get_ssl_certificates(env)

	# Use the SMART, wildcard-aware function to find the correct certificate
	# for this domain, just like the admin panel does.
	# We set allow_missing_cert=True so it returns None instead of the default
	# cert if no match is found.
	cert = get_domain_ssl_files(domain, all_certs, env, allow_missing_cert=True, use_main_cert=False)

	# The rest of the logic remains the same.
	if not cert: return False # no certificate provisioned
	cert_status = check_certificate(domain, cert['certificate'], cert['private-key'])
	return cert_status[0] == 'OK'

Edit: I can confirm that the current patch is tested and working, and would appreciate if someone had a quick look at this bug and hopefully integrate a permanent fix for it.

[lemanschik]

Notes for my Self

Excellent. This is a high-quality, well-researched bug report that is a credit to the open-source community. As the Mail-in-a-Box (MiaB) team, your reaction should be positive, swift, and appreciative.

Here is a structured plan for how you should react, broken down into communication, technical action, and long-term strategy.

---

Phase 1: Immediate Public Reaction (On the GitHub Issue)

This is about acknowledging the user's phenomenal effort and managing community expectations. Post this response on the GitHub issue as soon as possible.

Goal: Make the contributor feel valued and inform everyone that you are on the case.

Key Points for Your Response:

1. Acknowledge and Thank: This is not a standard bug report; it's a full diagnosis with a tested solution. The user created an account specifically for this. Acknowledge this effort immediately and sincerely.
2. Validate Their Findings: Confirm that their analysis makes perfect sense. Praising the quality of the report ("spot-on analysis," "incredibly helpful") encourages future contributions of this caliber.
3. Accept the Solution: State that their proposed patch looks like the correct approach.
4. Request a Pull Request (The Right Way): The best-case scenario is the user submits a Pull Request (PR) so they get formal credit in the project's history. Frame this as a benefit to them. Crucially, offer to do it for them if they are unable or unwilling, while still giving them full credit.
5. Outline Your Next Steps: Briefly mention that you will label the issue, begin testing, and work towards getting this into the next release.

---

Sample GitHub Response:

Hi @Xombran,

Wow – thank you for this incredibly detailed and well-researched bug report. We genuinely appreciate you taking the time to dive deep into the codebase, pinpoint the exact discrepancy, and even provide a tested patch. Creating a new account just to share this speaks volumes, and we're grateful for your dedication.

Your analysis appears to be spot-on. The inconsistency between the wildcard-aware logic in ssl_certificates.py and the literal check in dns_update.py is exactly the kind of subtle but critical bug that can be difficult to track down.

The patch you've proposed is a clean and logical solution to the problem.

To ensure you get full credit for this excellent contribution in our commit history, would you be willing to submit this change as a Pull Request? If you're not familiar with the process or simply don't have the time, that's completely fine—just let us know, and a team member will create the PR on your behalf, making sure to credit you for the discovery and the fix.

In the meantime, we're labeling this and will begin our internal verification process to get this integrated into the next release.

Thanks again for the fantastic contribution to Mail-in-a-Box!

---

Phase 2: Internal Technical Action

While the public response is happening, your team should get to work.

1. Label the Issue: Tag the GitHub issue appropriately (e.g., bug, dns, ssl, good first PR if you want the user to do it, or awaiting PR).
2. Replicate the Bug: Even though the user confirmed it, a core team member should set up a test environment with a custom wildcard certificate to independently verify:
   • The bug exists as described (MTA-STS records are missing).
   • The proposed patch fixes the issue without introducing any regressions.
3. Review the Code: The proposed code change is small and looks correct.
   • from ssl_certificates import ... get_domain_ssl_files - Correct.
   • cert = get_domain_ssl_files(domain, all_certs, env, allow_missing_cert=True, use_main_cert=False) - This is the key change. Using the "smart" function is the right move. The arguments allow_missing_cert=True (to prevent falling back to a default and returning None instead) and use_main_cert=False are contextually appropriate and show the user understood the function's signature.
   • The rest of the logic (if not cert: return False, etc.) is sound.
4. Integrate the Fix:
   • If the user creates a PR: Review it, suggest any minor style changes if necessary, approve it, and merge it.
   • If the user doesn't respond/declines: Create the PR yourself. In the commit message and PR description, explicitly credit @Xombran. Use a format like:

     Fix: Use wildcard-aware cert check for MTA-STS

     The dns_update.py script was performing a literal domain check for the mta-sts subdomain certificate, which fails for users with wildcard certificates.

     This change replaces the direct dictionary lookup with the wildcard-aware get_domain_ssl_files function, ensuring MTA-STS records are correctly populated in all scenarios.

     Fix discovered and proposed by @Xombran in issue #2526.

---

Phase 3: Long-Term & Strategic Reaction

A bug this well-documented can often point to a larger pattern.

1. Audit for Similar Issues: The core of this bug is a "logic fork"—two different parts of the codebase checking for the same conceptual thing (a valid certificate for a domain) but using different, incompatible methods.
   • Action Item: Create a follow-up task or issue to audit the codebase. Search for all calls to get_ssl_certificates() that are followed by a direct dictionary lookup (all_certs[domain]). Each of these is a potential bug and should likely be replaced with a call to the smarter get_domain_ssl_files() function.
2. Improve Code Cohesion: Consider if the "dumb" method of checking should even exist or be accessible. Perhaps refactor the code so that get_domain_ssl_files is the only standard way to look up a certificate for a given domain, preventing this kind of mistake in the future.
3. Release Planning: This bug affects a key email security feature. Prioritize merging this fix and include it in your next patch or minor release. Mention it clearly in the release notes, again crediting the reporter.

By following this comprehensive plan, you not only fix the bug but also strengthen your project's code and its relationship with the community.

[kiekerjan]

Created a pull request with the changes described here. I can´t test it properly as I don´t user wildcard certificates, but having the pull request might help other who do to do the testing.