Bug 4981 - [review] urirhssub only supports A records and bitmasks, docs suggest it should support more
Summary: [review] urirhssub only supports A records and bitmasks, docs suggest it shou...
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: 3.1.3
Hardware: Other other
: P5 normal
Target Milestone: 3.1.5
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-12 16:19 UTC by Nick Leverton
Modified: 2006-08-22 22:54 UTC (History)
0 users


Attachment Type Modified Status Actions Submitter/CLA Status
Test mail text/plain None Nick Leverton [HasCLA]
rulefile for testing rhssub with A records text/plain None Nick Leverton [HasCLA]
rulefile for testing rhssub with TXT records text/plain None Nick Leverton [HasCLA]
test results from the A record case. text/plain None Nick Leverton [HasCLA]
Patch to remove regexp from POD text, remove regexp case in code, and cause lint error if subtest is not a dotted quad or a 32 bit int number patch None Sidney Markowitz [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Leverton 2006-07-12 16:19:04 UTC 
Documentation says:        
        
urirhssub NAME_OF_RULE rhsbl_zone lookuptype subtest        
        
 Specify a RHSBL-style domain lookup with a sub-test.  "NAME_OF_RULE" is the        
 name of the rule to be used, "rhsbl_zone" is the zone to look up domain names        
 in, and "lookuptype" is the type of lookup (TXT or A).        
        
 "subtest" is the sub-test to run against the returned data.  The sub-test may        
 either be an IPv4 dotted address for RHSBLs that return multiple A records, a        
 non-negative decimal number to specify a bitmask for RHSBLs that return a        
 single A record containing a bitmask of results, or (if none of the preceding        
 options seem to fit) a regular expression.        
        
 Example:        
        
    urirhssub   URIBL_RHSBL_4    rhsbl.example.org.   A    127.0.0.4        
    urirhssub   URIBL_RHSBL_8    rhsbl.example.org.   A    8        
        
However for an A record, only the bitmask form actually works.  The       
dotted-quad and regexp forms give a warning and fail to match.  The attached   
test email and test cf-file 'A' should show this.   Note you need to remove   
the standard 25_uribl.cf rulefile because it also uses the multi.surbl.org   
zone.  The source of the warning is the following line in Plugin/URIDNSBL.pm 
but I am not sure if that is the whole problem: 
   
    uridnsbl_subs_bits |= $_ for keys %{$uridnsbl_subs};   
      
For a TXT record, the results are confusing.  I wouldn't expect the mask form   
or dotted-quad form to work, but the regexp form doesn't work either.  The 
test email and the test cf-file 'TXT' will show them.  DNS trace shows a TXT 
record is queried as expected, it's just not matched.
Comment 1 Nick Leverton 2006-07-12 16:21:17 UTC 
Created attachment 3576 [details]
Test mail
Comment 2 Nick Leverton 2006-07-12 16:22:06 UTC 
Created attachment 3577 [details]
rulefile for testing rhssub with A records
Comment 3 Nick Leverton 2006-07-12 16:22:29 UTC 
Created attachment 3578 [details]
rulefile for testing rhssub with TXT records
Comment 4 Nick Leverton 2006-07-12 16:24:55 UTC 
Created attachment 3579 [details]
test results from the A record case.

25_uribl.cf deliberately made unreadable, as it uses the same zones as the test
case.
Comment 5 Nick Leverton 2006-07-12 16:30:09 UTC 
Please separate my three tests per file into six separate tests - the docs  
say the zones should be identical and in those files they aren't.  For me, the  
warnings and the failure to match still happen when I run each test  
separately.
Comment 6 Sidney Markowitz 2006-08-09 04:36:29 UTC 
The patch for bug 4842 makes the warnings go away.

Does it fix the rest of this bug too?
Comment 7 Sidney Markowitz 2006-08-09 08:44:21 UTC 
Looking over the code in URIDNSBL.pm I can see how the regexp test here will not
work.

urirhssub TEST_A_RE multi.surbl.org. A /\./

parses the regular expression field into a string with value '/\./' and then
does the match using
 if ($rdatastr =~ m/${subtest}/)

Am I misunderstanding the perl or is that not correctly dealing with the
delimiters in the subtest field?

I looked at how we do it in the main rule file, with various tests and then
calling Mail::Conf::Parser->add_test and I'm hesitant to jump in with changes to
URIDNSBL.pm.

Can someone who is familiar with the code in Conf.pm and Parse.pm jump in and
look at how it should be done here? The relevant code is in URIDNSBL.pm, where
the urirhssub rule is parsed in set_config and where the match is done in
complete_dnsbl_lookup.
Comment 8 Justin Mason 2006-08-09 09:27:15 UTC 
I'd prefer to just remove the regexp test ability entirely, if possible.  if it
doesn't work, that's hardly a regression ;), and supporting regexp-testing
requires hairy code.
Comment 9 Sidney Markowitz 2006-08-09 10:17:41 UTC 
I've tracked down what doesn't work with these tests once the patch for bug 4842
is in place:

1) The regexp can't work for either A or TXT subrules

2) The TXT record with this example returns a string
  "multi.surbl.org permanent test point"
but the rule parser in URIDNSBL.pm has no provision for specifying an arbitrary
string with spaces in it.

I think this comes down to looking at what the RBLs want to put in the records,
especially the TXT records, and decide just what functionality we want to
provide for matching them.

The code may be complex, but isn't it already in place for rule parsing?
Comment 10 Nick Leverton 2006-08-11 15:40:05 UTC 
As the reporter, I can certainly live without regexp tests. 
 
I fell over this problem when finding out why a third party ruleset with a 
dotted quad on the RHS didn't work, so it seems other people are using dotted 
quads.
Comment 11 Sidney Markowitz 2006-08-20 22:29:15 UTC 
Created attachment 3659 [details]
Patch to remove regexp from POD text, remove regexp case in code, and cause lint error if subtest is not a dotted quad or a  32 bit int number

Suggested patch to code and POD against 3.1 branch that removes regexp as an
option and enforces in lint that subtest is either a number IP address (in
dotted quad syntax) or else is a decimal integer to be used as a bit mask.

Please check my regexp for correctness and especially for perl style.

review for 3.1. If it's ok I'll also check it in to trunk.
Comment 12 Justin Mason 2006-08-21 23:13:24 UTC 
+1
Comment 13 Daryl C. W. O'Shea 2006-08-23 04:33:43 UTC 
+1
Comment 14 Sidney Markowitz 2006-08-23 05:54:17 UTC 
Committed to 3.1 revision 433916.

Committed to trunk revision 433917.