Check SPF/DKIM/DMARC record on dns like MX records #3083

Riscue · Dec 7, 2023

What type of PR?

Enhancement

What does this PR do?

Checks TXT records for SPF, DKIM, DMARC same as MX record checker

mergify · Dec 7, 2023

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors-mailu · Dec 7, 2023

try

Build succeeded:

CI-Done

ghostwheel42

Hi. Thanks for your contribution.
I was thinking about doing the checks asynchronously (by having a "spinner" image and requesting the check via src="" returning a good or bad image), but this is a good start.
I didn't run in yet, but I think the regex should be changed.

core/admin/mailu/models.py

Riscue · Dec 7, 2023

Hi. Thanks for your contribution. I was thinking about doing the checks asynchronously (by having a "spinner" image and requesting the check via src="" returning a good or bad image), but this is a good start. I didn't run in yet, but I think the regex should be changed.

If we do this asynchronously, I will explicitly check all dns entries as well as auto-configuration entries.

And check methods could return states like exist-correct, exist-unknown, not exist? Any thought?

ghostwheel42 · Dec 22, 2023

Sorry for the delay. At first I thought the simplest way to implement this would be a route to check dns entries like .../check/dkim returning a single location header pointing to an image (good or bad).
The web page would have a spinning circle as lowsrc and the check as src.
Another way would be to do it via JS/jquery and a jsonp response. This could return more precise error messages and the values of the actual records.

Alex

mergify · Feb 22, 2024

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors-mailu · Feb 29, 2024

try

Build succeeded:

CI-Done

Pull request has been modified.

…oading

Riscue · Oct 8, 2024

Sorry for the delay. At first I thought the simplest way to implement this would be a route to check dns entries like .../check/dkim returning a single location header pointing to an image (good or bad). The web page would have a spinning circle as lowsrc and the check as src. Another way would be to do it via JS/jquery and a jsonp response. This could return more precise error messages and the values of the actual records.

Alex

Done via async image loading. Please check the routes for images and css changes

Riscue · Oct 8, 2024

bors try

bors-mailu · Oct 8, 2024

🔒 Permission denied

Existing reviewers: click here to make Riscue a reviewer

Riscue · Oct 10, 2024

@ghostwheel42 Hey alex bors stuck, could you please force for retry and review new commits

ghostwheel42 · Oct 10, 2024

bors try

bors-mailu · Oct 10, 2024

try

Build failed:

CI-Done

Riscue · Oct 16, 2024

@ghostwheel42 Hey alex fixed a bug, please retry

ghostwheel42 · Oct 16, 2024

bors try

bors-mailu · Oct 16, 2024

try

Build succeeded:

CI-Done

Riscue · Oct 21, 2024

Hey Alex @ghostwheel42 it is done actually, what should i do next?

core/admin/mailu/models.py

ghostwheel42

Hi @Riscue and sorry for being quiet for so long.

I think this is a good start at checking if the DNS config is like the one proposed.

Of course it is not a 100% check as some mx tools do and it will probably yield false-positives and false negatives, but the people using their own records should know what they are doing (for example when using "include:" in the spf records).

I would move the routes below the 'details' path, so instead of '/domain/<domain_name>/check/mx' I would use '/domain/details/<domain_name>/check/mx'.
This will allow to strip the '../../domain/' and '../../alternative/' from the templates.

@nextgens Do you think we need to do some kind of caching or rate limiting on the check routes to avoid abuse by doing lots of dns queries?

I think after the changes are done we could start testing this in the master branch.

ghostwheel42 · Jan 5, 2025

core/admin/mailu/models.py

+        """ checks if DMARC record for domain points to mailu host """
+        try:
+            return any(
+                rset.to_text().startswith('"v=DMARC1;')


Suggested change

rset.to_text().startswith('"v=DMARC1;')

rset.to_text().strip('"').split(';')[0].strip() == 'v=DMARC1'

This is more robust - also use this for the report check, as there do not need to be any other tags defined.

ghostwheel42 · Jan 5, 2025

core/admin/mailu/models.py

+        app_config_domain = app.config['DOMAIN']
+        try:
+            return any(
+                rset.to_text().startswith('"v=DMARC1;')


Suggested change

rset.to_text().startswith('"v=DMARC1;')

rset.to_text().strip('"').split(';')[0].strip() == 'v=DMARC1'

ghostwheel42 · Jan 5, 2025

core/admin/mailu/models.py

+        """ checks if DMARC record for domain points to mailu host """
+        try:
+            return any(
+                rset.to_text().startswith('"v=DMARC1;')


Suggested change

rset.to_text().startswith('"v=DMARC1;')

rset.to_text().strip('"').split(';')[0].strip() == 'v=DMARC1'

ghostwheel42 · Jan 5, 2025

core/admin/mailu/models.py

+        app_config_domain = app.config['DOMAIN']
+        try:
+            return any(
+                rset.to_text().startswith('"v=DMARC1;')


Suggested change

rset.to_text().startswith('"v=DMARC1;')

rset.to_text().strip('"').split(';')[0].strip() == 'v=DMARC1'

nextgens

@ghostwheel42 I'm not worried about it. DNS has a built-in cache mechanism with TTL, this will go no further than the local resolver.

What needs to be checked is how timeouts are handled. Browsers only allow so many parallel connections... if we block awaiting for a DNS reply for several items on the same page, we may exceed the limit. It may be better to timeout earlier and return some "unknown" png faster... refreshing the page (which could also be done automatically) would then display the right results

nextgens · Jan 6, 2025

core/admin/mailu/models.py

+        try:
+            hostnames = app.config['HOSTNAMES'].replace(',', '|')
+            return any(
+                re.search(f'^"v=spf1 mx a:({hostnames}) [?+-~]all"$', rset.to_text())


[?+-~] without escapes and without marking the string as a regexp pattern? - will break for sure

nextgens · Jan 6, 2025

core/admin/mailu/models.py

+        try:
+            hostnames = app.config['HOSTNAMES'].replace(',', '|')
+            return any(
+                re.search(f'^"v=spf1 mx a:({hostnames}) [?+-~]all"$', rset.to_text())


nextgens · Jan 6, 2025

core/admin/mailu/models.py

            return False


+    def check_spf(self):


Let's avoid code duplication please, make those methods static or something

bors-mailu bot added a commit that referenced this pull request Dec 7, 2023

Try #3083:

8f47189

ghostwheel42 previously requested changes Dec 7, 2023

View reviewed changes

core/admin/mailu/models.py Outdated Show resolved Hide resolved

core/admin/mailu/models.py Show resolved Hide resolved

ghostwheel42 added type/enhancement Enhances existing functionality priority/p2 Minor bug / Could have python Pull requests that update Python code labels Dec 22, 2023

bors-mailu bot added a commit that referenced this pull request Feb 29, 2024

Try #3083:

f72d5f9

Check SPF/DKIM/DMARC records on dns like MX records via async image l…

9cc2ca2

…oading

Riscue force-pushed the master branch from 181b560 to 9cc2ca2 Compare October 8, 2024 17:05

Changelog

040867d

bors-mailu bot added a commit that referenced this pull request Oct 10, 2024

Try #3083:

feb0ba5

Fix bors build

c977abd

bors-mailu bot added a commit that referenced this pull request Oct 16, 2024

Try #3083:

303473c

ErwanMAS reviewed Dec 30, 2024

View reviewed changes

core/admin/mailu/models.py Outdated Show resolved Hide resolved

Riscue added 2 commits January 4, 2025 16:11

Merge branch 'Mailu:master' into master

91a3ff5

Fix DMARC Report TXT

7b69b84

ghostwheel42 reviewed Jan 5, 2025

View reviewed changes

nextgens requested changes Jan 6, 2025

View reviewed changes

	rset.to_text().startswith('"v=DMARC1;')
	rset.to_text().strip('"').split(';')[0].strip() == 'v=DMARC1'

Search code, repositories, users, issues, pull requests...

Uh oh!

Check SPF/DKIM/DMARC record on dns like MX records #3083

Are you sure you want to change the base?

Check SPF/DKIM/DMARC record on dns like MX records #3083

Uh oh!

Conversation

Riscue commented Dec 7, 2023

What type of PR?

What does this PR do?

Uh oh!

mergify bot commented Dec 7, 2023

Uh oh!

bors-mailu bot commented Dec 7, 2023

try

Uh oh!

ghostwheel42 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Riscue commented Dec 7, 2023

Uh oh!

ghostwheel42 commented Dec 22, 2023

Uh oh!

mergify bot commented Feb 22, 2024

Uh oh!

bors-mailu bot commented Feb 29, 2024

try

Uh oh!

Riscue commented Oct 8, 2024

Uh oh!

Riscue commented Oct 8, 2024

Uh oh!

bors-mailu bot commented Oct 8, 2024

Uh oh!

Riscue commented Oct 10, 2024

Uh oh!

ghostwheel42 commented Oct 10, 2024

Uh oh!

bors-mailu bot commented Oct 10, 2024

try

Uh oh!

Riscue commented Oct 16, 2024

Uh oh!

ghostwheel42 commented Oct 16, 2024

Uh oh!

bors-mailu bot commented Oct 16, 2024

try

Uh oh!

Riscue commented Oct 21, 2024

Uh oh!

Uh oh!

ghostwheel42 left a comment

Choose a reason for hiding this comment

Uh oh!

ghostwheel42 Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwheel42 Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwheel42 Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwheel42 Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

nextgens left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nextgens Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

nextgens Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

nextgens Jan 6, 2025

Choose a reason for hiding this comment

nextgens left a comment •

edited

Loading