search

sdgathman / pyspf

Other
49 stars 26 forks source link

pyspf allows invalid ALPHA/DIGIT in alphanum #12

Open kitterma opened 5 years ago

kitterma commented 5 years ago

I believe that the example below should error out due to the '|' in the included domain before attempting a DNS lookup:

$ pyspf "v=spf1 include:a.b.com|c.d.com|e.f.com ~all" 1.1.1.1 test@kitterman.com test.kitterman.com result: ('permerror', 550, 'SPF Permanent Error: No valid SPF record for included domain: a.b.com|c.d.com|e.f.com: include:a.b.com|c.d.com|e.f.com') ~all

Since neither ALPHA nor DIGIT have "|", I think it's invalid per the ABNF.

sdgathman commented 5 years ago

That at least needs a test case for now. But how will the test suite framework check how many lookups were done? I think the framework needs to provide that information to the tests. For now, checking for a different Permerror explanation might be sufficient.

sdgathman commented 5 years ago

RFC 7208 says:

   Note: This document and its predecessors make no provisions for
   defining correct handling of a syntactically invalid <domain-spec>
   (which might be the result of macro expansion), per [RFC1035].

So the result will be undefined, like the invalid-domain-long test case.

The goal seems to be to prevent useless DNS lookups.

sdgathman commented 5 years ago

RFC 7208 7.1/2 says:

   domain-spec      = macro-string domain-end
   domain-end       = ( "." toplabel [ "." ] ) / macro-expand
   macro-string     = *( macro-expand / macro-literal )
   macro-expand     = ( "%{" macro-letter transformers *delimiter "}" )
                      / "%%" / "%_" / "%-"
   macro-literal    = %x21-24 / %x26-7E
                      ; visible characters except "%"

We even have a test case with '/' in the domain-spec. So, I'm afraid the '|' is valid per SPF syntax. BUT, since invalid hostname per RFC 1035 is undefined, maybe we could avoid the lookup without breaking anything. This issue is on hold while RFC lawyers weigh in.

sdgathman commented 4 years ago

@kitterma Since the result is undefined, current behavior is correct. If you have a reliable and efficient test for a valid domain-spec per RFC 1035 that we could do after macro expansion, then if might be a win to avoid the DNS lookup. Since the result is undefined per RFC 7208 - we can make the result of an invalid domain-spec PermErr since we think the policy owner needs to fix their policy.