[trac772] Address review comments

src/bin/xfrout/tests/xfrout_test.py.in

@@ -158,11 +158,18 @@ class TestXfroutSession(unittest.TestCase):
         # This should be dropped completely, therefore returning None
         self.xfrsess._remote = ('192.0.2.1', 12345)
         rcode, msg = self.xfrsess._parse_query_message(self.mdata)
-        self.assertTrue(rcode is None)
-        # This should be rejected, therefore NOTAUTH
+        self.assertEqual(None, rcode)
+        # This should be refused, therefore NOTAUTH
         self.xfrsess._remote = ('192.0.2.2', 12345)
         rcode, msg = self.xfrsess._parse_query_message(self.mdata)
         self.assertEqual(rcode.to_text(), "REFUSED")
+        # If the TSIG check fails, it should not check ACL
+        # (If it checked ACL as well, it would just drop the request)
+        self.xfrsess._remote = ('192.0.2.1', 12345)
+        self.xfrsess._tsig_key_ring = TSIGKeyRing()
+        rcode, msg = self.xfrsess._parse_query_message(request_data)
+        self.assertEqual(rcode.to_text(), "NOTAUTH")
+        self.assertTrue(self.xfrsess._tsig_ctx is not None)
 
     def test_get_query_zone_name(self):
         msg = self.getmsg()
@@ -222,20 +229,6 @@ class TestXfroutSession(unittest.TestCase):
         self.assertEqual(msg.get_rcode(), rcode)
         self.assertTrue(msg.get_header_flag(Message.HEADERFLAG_AA))
 
-    def test_reply_query_with_format_error(self):
-        msg = self.getmsg()
-        self.xfrsess._reply_query_with_format_error(msg, self.sock)
-        get_msg = self.sock.read_msg()
-        self.assertEqual(get_msg.get_rcode().to_text(), "FORMERR")
-
-        # tsig signed message
-        msg = self.getmsg()
-        self.xfrsess._tsig_ctx = self.create_mock_tsig_ctx(TSIGError.NOERROR)
-        self.xfrsess._reply_query_with_format_error(msg, self.sock)
-        get_msg = self.sock.read_msg()
-        self.assertEqual(get_msg.get_rcode().to_text(), "FORMERR")
-        self.assertTrue(self.message_has_tsig(get_msg))
-
     def test_create_rrset_from_db_record(self):
         rrset = self.xfrsess._create_rrset_from_db_record(self.soa_record)
         self.assertEqual(rrset.get_name().to_text(), "example.com.")
@@ -614,13 +607,13 @@ class TestUnixSockServer(unittest.TestCase):
         self.assertEqual(self.unix.tsig_key_ring.size(), 0)
 
         # Load the ACL
-        self.unix.update_config_data({'ACL': [{'from': '127.0.0.1',
+        self.unix.update_config_data({'query_acl': [{'from': '127.0.0.1',
                                                'action': 'ACCEPT'}]})
         self.check_loaded_ACL()
         # Pass a wrong data there and check it does not replace the old one
         self.assertRaises(isc.acl.acl.LoaderError,
                           self.unix.update_config_data,
-                          {'ACL': ['Something bad']})
+                          {'query_acl': ['Something bad']})
         self.check_loaded_ACL()
 
     def test_get_db_file(self):

src/bin/xfrout/xfrout.py.in

@@ -144,21 +144,22 @@ class XfroutSession():
             # TSIG related checks
             rcode = self._check_request_tsig(msg, mdata)
 
-            # ACL checks
-            acl_result = self._acl.execute(
-                isc.acl.dns.RequestContext(self._remote))
-            if acl_result == isc.acl.acl.DROP:
-                logger.info(XFROUT_QUERY_DROPPED,
-                            self._get_query_zone_name(msg),
-                            self._get_query_zone_class(msg),
-                            self._remote[0], self._remote[1])
-                return None, None
-            elif acl_result == isc.acl.acl.REJECT:
-                logger.info(XFROUT_QUERY_REJECTED,
-                            self._get_query_zone_name(msg),
-                            self._get_query_zone_class(msg),
-                            self._remote[0], self._remote[1])
-                return Rcode.REFUSED(), msg
+            if rcode == Rcode.NOERROR():
+                # ACL checks
+                acl_result = self._acl.execute(
+                    isc.acl.dns.RequestContext(self._remote))
+                if acl_result == DROP:
+                    logger.info(XFROUT_QUERY_DROPPED,
+                                self._get_query_zone_name(msg),
+                                self._get_query_zone_class(msg),
+                                self._remote[0], self._remote[1])
+                    return None, None
+                elif acl_result == REJECT:
+                    logger.info(XFROUT_QUERY_REJECTED,
+                                self._get_query_zone_name(msg),
+                                self._get_query_zone_class(msg),
+                                self._remote[0], self._remote[1])
+                    return Rcode.REFUSED(), msg
 
         except Exception as err:
             logger.error(XFROUT_PARSE_QUERY_ERROR, err)
@@ -202,18 +203,11 @@ class XfroutSession():
 
 
     def _reply_query_with_error_rcode(self, msg, sock_fd, rcode_):
-        msg.make_response()
-        msg.set_rcode(rcode_)
-        self._send_message(sock_fd, msg, self._tsig_ctx)
-
-
-    def _reply_query_with_format_error(self, msg, sock_fd):
-        '''query message format isn't legal.'''
         if not msg:
             return # query message is invalid. send nothing back.
 
         msg.make_response()
-        msg.set_rcode(Rcode.FORMERR())
+        msg.set_rcode(rcode_)
         self._send_message(sock_fd, msg, self._tsig_ctx)
 
     def _zone_has_soa(self, zone):
@@ -268,7 +262,8 @@ class XfroutSession():
         elif rcode_ == Rcode.NOTAUTH() or rcode_ == Rcode.REFUSED():
             return self._reply_query_with_error_rcode(msg, sock_fd, rcode_)
         elif rcode_ != Rcode.NOERROR():
-            return self._reply_query_with_format_error(msg, sock_fd)
+            return self._reply_query_with_error_rcode(msg, sock_fd,
+                                                      Rcode.FORMERR())
 
         zone_name = self._get_query_zone_name(msg)
         zone_class_str = self._get_query_zone_class(msg)
@@ -553,9 +548,9 @@ class UnixSockServer(socketserver_mixin.NoPollMixIn, ThreadingUnixStreamServer):
 
     def update_config_data(self, new_config):
         '''Apply the new config setting of xfrout module. '''
-        if 'ACL' in new_config:
-            self._acl = REQUEST_LOADER.load(new_config['ACL'])
         logger.info(XFROUT_NEW_CONFIG)
+        if 'query_acl' in new_config:
+            self._acl = REQUEST_LOADER.load(new_config['query_acl'])
         self._lock.acquire()
         self._max_transfers_out = new_config.get('transfers_out')
         self.set_tsig_key_ring(new_config.get('tsig_key_ring'))
@@ -645,7 +640,9 @@ class XfroutServer:
             try:
                 self._unix_socket_server.update_config_data(self._config_data)
             except Exception as e:
-                answer = create_answer(1, "Bad configuration: " + str(e))
+                answer = create_answer(1,
+                                       "Failed to handle new configuration: " +
+                                       str(e))
 
         return answer
 

src/bin/xfrout/xfrout.spec.pre.in

@@ -51,13 +51,13 @@
          }
        },
        {
-         "item_name": "ACL",
+         "item_name": "query_acl",
          "item_type": "list",
          "item_optional": false,
          "item_default": [],
          "list_item_spec":
          {
-             "item_name": "ACL_element",
+             "item_name": "acl_element",
              "item_type": "any",
              "item_optional": true
          }

src/bin/xfrout/xfrout_messages.mes

@@ -95,13 +95,16 @@ in the log message, but at this point no specific information other
 than that could be given. This points to incomplete exception handling
 in the code.
 
-% XFROUT_QUERY_DROPPED request to transfer %1/%2 to %3:%4 dropped
+% XFROUT_QUERY_DROPPED request to transfer %1/%2 to [%3]:%4 dropped
 The xfrout process silently dropped a request to transfer zone to given host.
-This is required by the ACLs.
+This is required by the ACLs. The %1 and %2 represent the zone name and class,
+the %3 and %4 the IP address and port of the peer requesting the transfer.
 
-% XFROUT_QUERY_REJECTED request to transfer %1/%2 to %3:%4 rejected
+% XFROUT_QUERY_REJECTED request to transfer %1/%2 to [%3]:%4 rejected
 The xfrout process rejected (by REFUSED rcode) a request to transfer zone to
-given host. This is because of ACLs.
+given host. This is because of ACLs. The %1 and %2 represent the zone name and
+class, the %3 and %4 the IP address and port of the peer requesting the
+transfer.
 
 % XFROUT_RECEIVE_FILE_DESCRIPTOR_ERROR error receiving the file descriptor for an XFR connection
 There was an error receiving the file descriptor for the transfer