ietf
[Top] [All Lists]

Re: A note about draft-ietf-spfbis-4408bis

2013-05-05 22:52:07

In message <1962766.G247B9R6HU@scott-latitude-e6320>, Scott Kitterman writes:
On Monday, May 06, 2013 10:10:40 AM Mark Andrews wrote:
In message 
<42523d2d-85c6-4e6d-b2a7-6791a0e5d4a8(_at_)email(_dot_)android(_dot_)com>, 
Scott
Kitterman writes:
Mark Andrews <marka(_at_)isc(_dot_)org> wrote:
In message 
<6(_dot_)2(_dot_)5(_dot_)6(_dot_)2(_dot_)20130505082013(_dot_)0adbbe40(_at_)elandnews(_dot_)com>,
S
Moonesamy write

s:
Hi Mark,

At 15:57 04-05-2013, Mark Andrews wrote:
The publisher can choose to interoperate with everyone by publishing
both.

The client side can choose to interoperate with everyone by looking
for both.

Both side can choose their level of interoperability.  There is no
bug.

Thanks for the feedback.

Based on the quoted text I would write the text as:
   (i)  you must have X and Y where X and Y are identical.
   
   (ii) I ask you for both X and Y (see [1] for example).

Item (i) is a combination of the previous items (a) and (c).  Item
(ii) is the last part of previous item (d).

That was not the intent.  Having choice here is very important here.
In fact it is essential to reach the end goal of Y only when starting
with X only.

There is nothing wrong with failing to catch every possible forgery
possible if both sides are using SPF.  Unfortunately the SPF WG
seem to think that unless the RFC does catch every possible forgery
that it is broken.  The SPF WG appears to not want to allow operators
to have the choice.  This is the case "pefect" being the enemy of
"good enough".  We need "good enough" here not "perfect".

Mark

If we publish a 4408bis that suggests the normal way to publish an SPF
record is in type SPF, then it'll get about 98% less effective based on
the data we've collected. What you are suggesting is more like 'ignore
the deployed base and start over'.  That's not wgat the WG was chartered
to do.

No one said "ignore deployed base".  Firstly normal != only.

Secondly one could quite easly add "fixup SPF" functionality to
nameservers/zone signers by having the master server/signers add
type SPF records if they are not present when there are v=spf1 TXT
records.  This would also require fixing some DNSSEC records but
it is doable.

Name servers/signers fixup DNSSEC records all the time.  Adding
another type of record to fixup is a relatively trivial change.

For unsigned zones one could do this on slave servers as well.

You have already mentions you have a script that does it.  A script
needs someone to install it and run it so it is not comparable,
other than a proof of concept that it can be done, to getting
nameservers to do the fixup.  This get done installed and run
automatically.  Installation happens as part of OS upgrades / new
server installs.  It gets run as it is part of the default behaviour.

None of this happened in 8 years.  There's no basis for the next 8 years 
being 
any different.

Sorry that mantra doesn't cut it.

Add words to say "Namesservers SHOULD add SPF records if they
discover v=spf1 TXT records without SPF records." and it gets dead
easy to add code like this below which does it for dnssec-signzone.

The nameserver is a little more complicated but not much more.  Add
some code to UPDATE processing.  Add some code when a master zone
gets loaded.

diff --git a/bin/dnssec/dnssec-signzone.c b/bin/dnssec/dnssec-signzone.c
index 5be654d..cfb3f8c 100644
--- a/bin/dnssec/dnssec-signzone.c
+++ b/bin/dnssec/dnssec-signzone.c
@@ -1677,6 +1677,67 @@ remove_sigs(dns_dbnode_t *node, dns_rdatatype_t which) {
        dns_rdatasetiter_destroy(&rdsiter);
 }
 
+static isc_result_t
+spfify(dns_db_t *db, dns_db_t *version, dns_dbnode_t *node) {
+       dns_rdata_t rdata = DNS_RDATA_INIT;
+       dns_rdataset_t rdataset;
+       isc_boolean_t have_txt = ISC_FALSE;
+       isc_result_t result;
+
+       dns_rdataset_init(&rdataset);
+
+       result = dns_db_findrdataset(db, node, NULL, dns_rdatatype_spf,
+                                    0, 0, &rdataset, NULL);
+       if (result == ISC_R_SUCCESS) {
+               dns_rdataset_disassociate(&rdataset);
+               return (ISC_R_SUCCESS);
+       }
+
+       result = dns_db_findrdataset(db, node, NULL, dns_rdatatype_txt,
+                                    0, 0, &rdataset, NULL);
+       if (result != ISC_R_SUCCESS)
+               return (ISC_R_SUCCESS);
+
+       result = dns_rdataset_first(&rdataset);
+       while (result == ISC_R_SUCCESS) {
+               dns_rdataset_current(&rdataset, &rdata);
+               have_txt = dns_rdata_isspf(&rdata);
+               if (have_txt)
+                       break;
+               dns_rdata_reset(&rdata);
+               result = dns_rdataset_next(&rdataset);
+       }
+
+       if (have_txt) {
+               dns_rdata_t spf = DNS_RDATA_INIT;
+               dns_rdatalist_t rdatalist;
+               dns_rdataset_t spfset;
+
+               dns_rdataset_init(&spfset);
+
+               dns_rdata_clone(&rdata, &spf);
+               spf.type = dns_rdatatype_spf;
+       
+               rdatalist.rdclass = spf.rdclass;
+               rdatalist.type = spf.type;
+               rdatalist.covers = 0;
+               rdatalist.ttl = rdataset.ttl;
+               ISC_LIST_INIT(rdatalist.rdata);
+               ISC_LIST_APPEND(rdatalist.rdata, &spf, link);
+               result = dns_rdatalist_tordataset(&rdatalist, &spfset);
+               if (result == ISC_R_SUCCESS) {
+                       result = dns_db_addrdataset(db, node, version, 0,
+                                                   &spfset, 0, NULL);
+                       dns_rdataset_disassociate(&spfset);
+               }
+       } else
+               result = ISC_R_SUCCESS;
+
+       dns_rdataset_disassociate(&rdataset);
+
+       return (result);
+}
+
 /*%
  * Generate NSEC records for the zone and remove NSEC3/NSEC3PARAM records.
  */
@@ -1798,6 +1859,10 @@ nsecify(void) {
                        fatal("iterating through the database failed: %s",
                              isc_result_totext(result));
                dns_dbiterator_pause(dbiter);
+
+               result = spfify(gdb, gversion, node);
+               check_result(result, "spfify()");
+
                result = dns_nsec_build(gdb, gversion, node, nextname,
                                        zone_soa_min_ttl);
                check_result(result, "dns_nsec_build()");
@@ -2353,6 +2418,10 @@ nsec3ify(unsigned int hashalg, unsigned int iterations,
                 * We need to pause here to release the lock on the database.
                 */
                dns_dbiterator_pause(dbiter);
+
+               result = spfify(gdb, gversion, node);
+               check_result(result, "spfify()");
+
                addnsec3(name, node, salt, salt_length, iterations,
                         hashlist, zone_soa_min_ttl);
                dns_db_detachnode(gdb, &node);
diff --git a/lib/dns/include/dns/rdata.h b/lib/dns/include/dns/rdata.h
index 89ecaf8..d15b589 100644
--- a/lib/dns/include/dns/rdata.h
+++ b/lib/dns/include/dns/rdata.h
@@ -769,6 +769,9 @@ dns_rdata_makedelete(dns_rdata_t *rdata);
 const char *
 dns_rdata_updateop(dns_rdata_t *rdata, dns_section_t section);
 
+isc_boolean_t
+dns_rdata_isspf(const dns_rdata_t *rdata);
+
 ISC_LANG_ENDDECLS
 
 #endif /* DNS_RDATA_H */
diff --git a/lib/dns/rdata.c b/lib/dns/rdata.c
index 2bed961..bbc2a4a 100644
--- a/lib/dns/rdata.c
+++ b/lib/dns/rdata.c
@@ -2174,3 +2174,31 @@ dns_rdata_updateop(dns_rdata_t *rdata, dns_section_t 
section) {
        }
        return ("invalid");
 }
+
+isc_boolean_t
+dns_rdata_isspf(const dns_rdata_t *rdata) {
+        char buf[1024];
+        const unsigned char *data = rdata->data;
+        unsigned int rdl = rdata->length, i = 0, tl, len;
+
+        while (rdl > 0U) {
+                len = tl = *data;
+                ++data;
+                --rdl;
+                INSIST(tl <= rdl);
+                if (len > sizeof(buf) - i - 1)
+                        len = sizeof(buf) - i - 1;
+                memcpy(buf + i, data, len);
+                i += len;
+                data += tl;
+                rdl -= tl;
+        }
+
+        if (i < 6U)
+                return(ISC_FALSE);
+
+        buf[i] = 0;
+        if (strncmp(buf, "v=spf1", 6) == 0 && (buf[6] == 0 || buf[6] == ' '))
+                return (ISC_TRUE);
+        return (ISC_FALSE);
+}

Additionally, I'm personally against publishing documents that require
special external knowledge (if 4408bis prefers SPF over TXT deployers
will have know to ignore that part of the RFC if they actually want the
protocol to be useful. To promote interoperability there has to be a MUST
publish and a MUST check format in common.  Given the lack of type SPF
deployment, it's crazy to suggest that it should be the required type.

What external knowledge.  4408 already effectly says that you need
to publish SPF records.  TXT records are described as "for backwards
i compatibilty".  It says you SHOULD publish both.

You are worrying about it not been "perfect" when it was in fact
what was in 4088 was "good enough".

What was in 4408 turned out to be pointless and complicating.  There are also 
a few pour souls out there who published type SPF records only and are 
probably wondering why it's not working very well.

 
Personally, I'm quite surprised that doubling the DNS queries associated with 
SPF for the foreseeable future is a "meh" issue to DNS people.  

You just don't get it.  If you ask nameserver developers to HELP
you they will.

Getting code to modify zone content without a RFC saying to do it
is a uphill battle.  With a RFC it becomes a trivial exercise, "We
need to make our nameserver compliant with RFC XXXX".

If you're going to query for type SPF, then you have to do an asynchronous 
query of some kind due to broken DNS servers that return either nothing 
(meaning you end up waiting for a timeout) or SERVFAIL for unknown types.

And TXT is almost as equally bad as SPF.

You 
still have to do type TXT queries and performance will suffer badly if you do 
them serially.  Additionally, we'd need to special case SERVFAIL for type SPF 
so that in appropriate temperrors weren't returned.  It adds a significant 
amount of complexity if you want to implement both types without poor 
performance (Mail::SPF does the queries sequentially and will fail on a bad 
DNS server, it's not really the model one would want).

You need to add failure code for TXT as well.

I get the theory of type SPF, but dual types adds a lot of complexity for 
minuscule benefit.  A single type is much simpler and more reliable.  
Unfortunately, if it's only going to be one, TXT is the only one that makes 
any sense operationally.

Scott K 
-- 
Mark Andrews, ISC
1 Seymour St., Dundas Valley, NSW 2117, Australia
PHONE: +61 2 9871 4742                 INTERNET: marka(_at_)isc(_dot_)org