dmarc: fix handling if multiple records #73
Conversation
mimi89999
force-pushed
the
multiple_dmarc
branch
from
744ac2d to
552330f
Compare
dmarc/lookup.go
Outdated
| return Parse(dmarcRecords[0]) | ||
| } | ||
|
|
||
| func IsDmarcRecord(txt string) bool { |
There was a problem hiding this comment.
According to section 6.6.3, checking for the v= prefix should be enough.
There was a problem hiding this comment.
But it states: that identifies the current version of DMARC
There is also:
v: Version (plain-text; REQUIRED). Identifies the record retrieved
as a DMARC record. It MUST have the value of "DMARC1". The value
of this tag MUST match precisely; if it does not or it is absent,
the entire retrieved record MUST be ignored. It MUST be the first
tag in the list.
I believe that the current version is part of the statement as if there was a conjunction.
There was a problem hiding this comment.
Also, matching only the v= would match SPF and DKIM records. That wouldn't make much sense.
There was a problem hiding this comment.
that identifies the current version of DMARC
Hm, you're right. I'm not a fan of duplicating the parsing logic though. I'd suggest either matching on the v=DMARC1 prefix, or parsing all records for key-values.
Also, matching only the v= would match SPF and DKIM records. That wouldn't make much sense.
DMARC records are stored in a separate, special "_dmarc" subdomain, so that shouldn't be the case.
There was a problem hiding this comment.
Hm, you're right. I'm not a fan of duplicating the parsing logic though. I'd suggest either matching on the
v=DMARC1prefix, or parsing all records for key-values.
That would also match things like v=DMARC1.0 or v=DMARC10 that shouldn't be matched. We also can't simple match v=DMARC1; as leading and trailing spaces in the tag value should be ignored. I believe that the current logic is quite robust and makes sense.
There was a problem hiding this comment.
@emersion I'm not quite sure how I should change the pull request. Could you please propose something more specific?
There was a problem hiding this comment.
The current logic duplicates parsing, and I'd like to avoid this. parseParams() can be called instead.
There was a problem hiding this comment.
I removed the duplication now.
|
The DKIM side of this was fixed in b8ad33f, it seems like we forgot DMARC. |
I'm not sure if that many comment are needed. I'm also not sure if we should return a different error when there are multiple DMARC records.