Opened 7 years ago

Closed 6 years ago

#3928 enhancement closed fixed (fixed)

Add support for SPF records

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

Attachments (2)

Record_SPF.patch (7.7 KB) - added by shylent 6 years ago.
Record_SPF_2.patch (9.1 KB) - added by shylent 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by shylent

  • Cc shylent added
  • Keywords review added
  • Owner exarkun 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 6 years ago by shylent

comment:2 Changed 6 years ago by lvh

  • Keywords review removed
  • Owner set to shylent

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 6 years ago by shylent

comment:3 Changed 6 years ago by shylent

  • 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 6 years ago by lvh

  • Owner shylent deleted

comment:5 Changed 6 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/spf-3928

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

comment:6 Changed 6 years ago by exarkun

(In [30241]) Apply Record_SPF_2.patch

refs #3928

comment:7 Changed 6 years ago by exarkun

  • Author changed from exarkun to shylent
  • Keywords review removed
  • Owner set to exarkun

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 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 6 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.