Opened 9 years ago

Closed 8 years ago

#3928 enhancement closed fixed (fixed)

Add support for SPF records

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: names Keywords:
Cc: Mark Branch: branches/spf-3928
branch-diff, diff-cov, branch-cov, buildbot
Author: shylent

Attachments (2)

Record_SPF.patch (7.7 KB) - added by Mark 8 years ago.
Record_SPF_2.patch (9.1 KB) - added by Mark 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by Mark

Cc: Mark added
Keywords: review added
Owner: Jean-Paul Calderone deleted

Add support for SPF records.

As per RFC 4408, SPF record format is similar to TXT record format and, as I understand it, it is not up to twisted.names to parse the actual policy declaration.

I've reused existing TXT record behaviour and added the necessary resolver methods and tests (hopefully).

Changed 8 years ago by Mark

Attachment: Record_SPF.patch added

comment:2 Changed 8 years ago by lvh

Keywords: review removed
Owner: set to Mark

Hello, and thanks for your patch!

  1. Style issue with kwarg notation in def lookupSenderPolicy(name, timeout = 10): should be timeout=10, but apparently this will be fixed in #2336 Soon(TM), plus it is consistent with the rest of the source file so not a showstopper
  2. I can not find anything about the ttl kwarg or attribute. This appears to be documented on SimpleRecord but Record_TXT (and by extension Record_SPF) does not use it. I think the easiest fix is: document on Record_TXT, point to that in Record_SPF, add attribute on the IRecord interface. The last thing should probably be a separate ticket.
  3. I'm not sure about the tests: twisted/names/test/ line 1227. I think the records are used wrong which makes the tests silently fail. Here is an example:
    >>> from twisted.names.dns import Record_TXT
    >>> Record_SPF = Record_TXT # handwavy argument
    >>> r = Record_SPF(['foo', 'bar'], 10)
    >>> r.ttl
    >>> # Note how the ttl is None, not 10.

Everything else looks okay :-)

Changed 8 years ago by Mark

Attachment: Record_SPF_2.patch added

comment:3 Changed 8 years ago by Mark

Keywords: review added

Added documentation for ttl instance attribute for Record_TXT and Record_SPF.

Fixed the mentioned tests for both Record_TXT and Record_SPF (wrong __init__ signature was used).

comment:4 Changed 8 years ago by lvh

Owner: Mark deleted

comment:5 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/spf-3928

(In [30240]) Branching to 'spf-3928'

comment:6 Changed 8 years ago by Jean-Paul Calderone

(In [30241]) Apply Record_SPF_2.patch

refs #3928

comment:7 Changed 8 years ago by Jean-Paul Calderone

Author: exarkunshylent
Keywords: review removed
Owner: set to Jean-Paul Calderone

A few minor points:

  1. Copyright dates should be updated on modified files
  2. There should be a news fragment explaining the new feature
  3. New tests should follow the current naming/docstring convention (so testSPF should be test_spf)

These are all trivial, so I made the changes myself. The only interesting comment I have, I think, is the IResolver change. In general I would shy away from changing a released interface. Adding methods to implementations is fine, but adding methods to interfaces can suddenly make previously valid, complete implementations suddenly incomplete. This may break things, so we should probably avoid it. On top of that, IResolver is an ugly interface: it's defined in the wrong place and the way it forces a new method for every record type is cruddy. We should replace it with something simpler that won't suffer from the same proliferation issue. I talked about this with glyph and he agreed, so I've reverted the change to the interface.

Aside from that, this looks good, and buildbot is happy, so I'll merge. Thanks!

comment:8 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [30253]) Merge spf-3928

Author: shylent Reviewer: lvh, exarkun Fixes: #3928

Add basic support for serving and requesting SPF records with Twisted Names.

comment:9 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.