[master] Merge branch 'trac1351'

doc/guide/bind10-guide.xml

@@ -758,7 +758,7 @@ as a dependency earlier -->
           If the configure fails, it may be due to missing or old
           dependencies.
         </para>
-        
+
         <note>
           <para>For notes on configuring and building DHCPv6 with MySQL see <xref linkend="dhcp6-install">.</xref></para>
         </note>
@@ -1841,10 +1841,8 @@ config set /Boss/components/b10-zonemgr/kind dispensable
         <para>
           The key ring lives in the configuration in "tsig_keys/keys". Most of
           the system uses the keys from there &mdash; ACLs, authoritative server to
-          sign responses to signed queries, and <command>b10-xfrout</command>
-          to sign transfers. The <command>b10-xfrin</command> uses its own
-          configuration for keys, but that will be fixed in Trac ticket
-          <ulink url="http://bind10.isc.org/ticket/1351">#1351</ulink>.
+          sign responses to signed queries, and <command>b10-xfrin</command>
+          and <command>b10-xfrout</command> to sign transfers.
         </para>
 
         <para>
@@ -2722,6 +2720,15 @@ TODO
     </section>
 
     <section>
+        <title>TSIG</title>
+        If you want to use TSIG for incoming transfers, a system wide TSIG
+        key ring must be configured (see <xref linkend="tsig-key-ring"/>).
+        To specify a key to use, set tsig_key value to the name of the key
+        to use from the key ring.
+&gt; <userinput>config set Xfrin/zones[0]/tsig_key "<option>example.key</option>"</userinput>
+    </section>
+
+    <section>
       <title>Enabling IXFR</title>
       <para>
         As noted above, <command>b10-xfrin</command> uses AXFR for

src/bin/xfrin/b10-xfrin.xml

@@ -112,20 +112,18 @@ in separate zonemgr process.
       <varname>master_addr</varname> (the zone master to transfer from),
       <varname>master_port</varname> (defaults to 53),
       <varname>use_ixfr</varname> (defaults to false), and
-      <varname>tsig_key</varname> (optional TSIG key to use).
-      The <varname>tsig_key</varname> is specified using a full string
-      colon-delimited name:key:algorithm representation (e.g.
-      <quote>foo.example.org:EvABsfU2h7uofnmqaRCrhHunGsd=:hmac-sha1</quote>).
+      <varname>tsig_key</varname> (optional TSIG key name to use).
+      The <varname>tsig_key</varname> is specified using a name that
+      corresponds to one of the TSIG keys configured in the global
+      TSIG key ring (<quote>/tsig_keys/keys</quote>).
     </para>
 <!-- TODO: document this better -->
-<!-- TODO: the tsig_key format may change -->
 
     <para>
       (The site-wide <varname>master_addr</varname> and
       <varname>master_port</varname> configurations are deprecated;
       use the <varname>zones</varname> list configuration instead.)
     </para>
-<!-- NOTE: also tsig_key but not mentioning since so short lived. -->
 
 <!-- TODO: formating -->
     <para>

src/bin/xfrin/tests/xfrin_test.py

@@ -26,6 +26,7 @@ from xfrin import *
 import xfrin
 from isc.xfrin.diff import Diff
 import isc.log
+from isc.server_common.tsig_keyring import init_keyring, get_keyring
 # If we use any python library that is basically a wrapper for
 # a library we use as well (like sqlite3 in our datasources),
 # we must make sure we import ours first; If we have special
@@ -139,6 +140,16 @@ class MockCC(MockModuleCCSession):
         if identifier == "zones/use_ixfr":
             return False
 
+    def add_remote_config_by_name(self, name, callback):
+        pass
+
+    def get_remote_config_value(self, module, identifier):
+        if module == 'tsig_keys' and identifier == 'keys':
+            return (['example.com.key.:EvAAsfU2h7uofnmqaTCrhHunGsc='], True)
+        else:
+            raise Exception('MockCC requested for unknown config value ' +
+                            + module + "/" + identifier)
+
     def remove_remote_config(self, module_name):
         pass
 
@@ -229,6 +240,7 @@ class MockXfrin(Xfrin):
     def _cc_setup(self):
         self._tsig_key = None
         self._module_cc = MockCC()
+        init_keyring(self._module_cc)
         pass
 
     def _get_db_file(self):
@@ -2427,9 +2439,10 @@ class TestXfrin(unittest.TestCase):
             self.assertEqual(str(zone_info.master_addr), zone_config['master_addr'])
             self.assertEqual(zone_info.master_port, zone_config['master_port'])
             if 'tsig_key' in zone_config:
-                self.assertEqual(zone_info.tsig_key.to_text(), TSIGKey(zone_config['tsig_key']).to_text())
+                self.assertEqual(zone_info.tsig_key_name.to_text(),
+                                 Name(zone_config['tsig_key']).to_text())
             else:
-                self.assertIsNone(zone_info.tsig_key)
+                self.assertIsNone(zone_info.tsig_key_name)
             if 'use_ixfr' in zone_config and\
                zone_config.get('use_ixfr'):
                 self.assertTrue(zone_info.use_ixfr)
@@ -2562,7 +2575,7 @@ class TestXfrin(unittest.TestCase):
                   { 'name': 'test2.example.',
                     'master_addr': '192.0.2.9',
                     'master_port': 53,
-                    'tsig_key': 'badkey'
+                    'tsig_key': 'badkey..'
                   }
                 ]}
         self.assertEqual(self.xfr.config_handler(zones)['result'][0], 1)
@@ -2581,13 +2594,14 @@ class TestXfrin(unittest.TestCase):
         self.assertEqual(self.xfr.config_handler(config)['result'][0], 0)
         self._check_zones_config(config)
 
-    def common_ixfr_setup(self, xfr_mode, use_ixfr):
+    def common_ixfr_setup(self, xfr_mode, use_ixfr, tsig_key_str = None):
         # This helper method explicitly sets up a zone configuration with
         # use_ixfr, and invokes either retransfer or refresh.
         # Shared by some of the following test cases.
         config = {'zones': [
                 {'name': 'example.com.',
                  'master_addr': '192.0.2.1',
+                 'tsig_key': tsig_key_str,
                  'use_ixfr': use_ixfr}]}
         self.assertEqual(self.xfr.config_handler(config)['result'][0], 0)
         self.assertEqual(self.xfr.command_handler(xfr_mode,
@@ -2603,6 +2617,34 @@ class TestXfrin(unittest.TestCase):
         self.common_ixfr_setup('refresh', True)
         self.assertEqual(RRType.IXFR(), self.xfr.xfrin_started_request_type)
 
+    def test_command_handler_retransfer_with_tsig(self):
+        self.common_ixfr_setup('retransfer', False, 'example.com.key')
+        self.assertEqual(RRType.AXFR(), self.xfr.xfrin_started_request_type)
+
+    def test_command_handler_retransfer_with_tsig_bad_key(self):
+        # bad keys should not reach xfrin, but should they somehow,
+        # they are ignored (and result in 'key not found' + error log).
+        self.assertRaises(XfrinZoneInfoException, self.common_ixfr_setup,
+                          'retransfer', False, 'bad.key')
+
+    def test_command_handler_retransfer_with_tsig_unknown_key(self):
+        self.assertRaises(XfrinZoneInfoException, self.common_ixfr_setup,
+                          'retransfer', False, 'no.such.key')
+
+    def test_command_handler_refresh_with_tsig(self):
+        self.common_ixfr_setup('refresh', False, 'example.com.key')
+        self.assertEqual(RRType.AXFR(), self.xfr.xfrin_started_request_type)
+
+    def test_command_handler_refresh_with_tsig_bad_key(self):
+        # bad keys should not reach xfrin, but should they somehow,
+        # they are ignored (and result in 'key not found' + error log).
+        self.assertRaises(XfrinZoneInfoException, self.common_ixfr_setup,
+                          'refresh', False, 'bad.key')
+
+    def test_command_handler_refresh_with_tsig_unknown_key(self):
+        self.assertRaises(XfrinZoneInfoException, self.common_ixfr_setup,
+                          'refresh', False, 'no.such.key')
+
     def test_command_handler_retransfer_ixfr_disabled(self):
         # Similar to the previous case, but explicitly disabled.  AXFR should
         # be used.

src/bin/xfrin/xfrin.py.in

@@ -34,6 +34,7 @@ from isc.datasrc import DataSourceClient, ZoneFinder
 import isc.net.parse
 from isc.xfrin.diff import Diff
 from isc.server_common.auth_command import auth_loadzone_command
+from isc.server_common.tsig_keyring import init_keyring, get_keyring
 from isc.log_messages.xfrin_messages import *
 
 isc.log.init("b10-xfrin", buffer=True)
@@ -69,7 +70,10 @@ AUTH_SPECFILE_LOCATION = AUTH_SPECFILE_PATH + "/auth.spec"
 
 AUTH_MODULE_NAME = 'Auth'
 XFROUT_MODULE_NAME = 'Xfrout'
+
+# Remote module and identifiers (according to their spec files)
 ZONE_MANAGER_MODULE_NAME = 'Zonemgr'
+
 REFRESH_FROM_ZONEMGR = 'refresh_from_zonemgr'
 
 # Constants for debug levels.
@@ -1179,7 +1183,7 @@ class ZoneInfo:
 
         self.set_master_port(config_data.get('master_port'))
         self.set_zone_class(config_data.get('class'))
-        self.set_tsig_key(config_data.get('tsig_key'))
+        self.set_tsig_key_name(config_data.get('tsig_key'))
         self.set_use_ixfr(config_data.get('use_ixfr'))
 
     def set_name(self, name_str):
@@ -1240,20 +1244,32 @@ class ZoneInfo:
                 errmsg = "invalid zone class: " + zone_class_str
                 raise XfrinZoneInfoException(errmsg)
 
-    def set_tsig_key(self, tsig_key_str):
-        """Set the tsig_key for this zone, given a TSIG key string
-           representation. If tsig_key_str is None, no TSIG key will
-           be set. Raises XfrinZoneInfoException if tsig_key_str cannot
-           be parsed."""
+    def set_tsig_key_name(self, tsig_key_str):
+        """Set the name of the tsig_key for this zone. If tsig_key_str
+           is None, no TSIG key will be used. This name is used to
+           find the TSIG key to use for transfers in the global TSIG
+           key ring.
+           Raises XfrinZoneInfoException if tsig_key_str is not a valid
+           (dns) name."""
         if tsig_key_str is None:
-            self.tsig_key = None
+            self.tsig_key_name = None
         else:
+            # can throw a number of exceptions but it is just one
+            # call, so Exception should be OK here
             try:
-                self.tsig_key = TSIGKey(tsig_key_str)
-            except InvalidParameter as ipe:
-                logger.error(XFRIN_BAD_TSIG_KEY_STRING, tsig_key_str)
-                errmsg = "bad TSIG key string: " + tsig_key_str
-                raise XfrinZoneInfoException(errmsg)
+                self.tsig_key_name = Name(tsig_key_str)
+            except Exception as exc:
+                raise XfrinZoneInfoException("Bad TSIG key name: " + str(exc))
+
+    def get_tsig_key(self):
+        if self.tsig_key_name is None:
+            return None
+        result, key = get_keyring().find(self.tsig_key_name)
+        if result != isc.dns.TSIGKeyRing.SUCCESS:
+            raise XfrinZoneInfoException("TSIG key not found in keyring: " +
+                                         self.tsig_key_name.to_text())
+        else:
+            return key
 
     def set_use_ixfr(self, use_ixfr):
         """Set use_ixfr. If set to True, it will use
@@ -1308,6 +1324,7 @@ class Xfrin:
         self.config_handler(config_data)
         self._module_cc.add_remote_config(AUTH_SPECFILE_LOCATION,
                                           self._auth_config_handler)
+        init_keyring(self._module_cc)
 
     def _cc_check_command(self):
         '''This is a straightforward wrapper for cc.check_command,
@@ -1468,7 +1485,7 @@ class Xfrin:
                                                rrclass,
                                                self._get_db_file(),
                                                master_addr,
-                                               zone_info.tsig_key, request_type,
+                                               zone_info.get_tsig_key(), request_type,
                                                True)
                         answer = create_answer(ret[0], ret[1])
                     else:
@@ -1491,7 +1508,7 @@ class Xfrin:
                 tsig_key = None
                 request_type = RRType.AXFR()
                 if zone_info:
-                    tsig_key = zone_info.tsig_key
+                    tsig_key = zone_info.get_tsig_key()
                     if zone_info.use_ixfr:
                         request_type = RRType.IXFR()
                 db_file = args.get('db_file') or self._get_db_file()

src/bin/xfrin/xfrin_messages.mes

@@ -43,7 +43,7 @@ The master port as read from the configuration is not a valid port number.
 
 % XFRIN_BAD_TSIG_KEY_STRING bad TSIG key string: %1
 The TSIG key string as read from the configuration does not represent
-a valid TSIG key.
+a valid TSIG key. The key is ignored.
 
 % XFRIN_BAD_ZONE_CLASS Invalid zone class: %1
 The zone class as read from the configuration is not a valid DNS class.
@@ -160,6 +160,13 @@ run time: Time (in seconds) the complete axfr took
 
 bytes/second: Transfer speed
 
+% XFRIN_TSIG_KEY_NOT_FOUND TSIG key not found in key ring: %1
+An attempt to start a transfer with TSIG was made, but the configured TSIG
+key name was not found in the TSIG key ring (configuration option
+tsig_keys/keys). The transfer is aborted. The key name that could not be
+found is shown in the log message. Check the configuration and the
+TSIG key ring.
+
 % XFRIN_UNKNOWN_ERROR unknown error: %1
 An uncaught exception was raised while running the xfrin daemon. The
 exception message is printed in the log message.

src/lib/dns/python/tests/tsigkey_python_test.py

@@ -170,6 +170,12 @@ class TSIGKeyRingTest(unittest.TestCase):
         self.assertEqual(TSIGKey.HMACSHA256_NAME, key.get_algorithm_name())
         self.assertEqual(self.secret, key.get_secret())
 
+        (code, key) = self.keyring.find(self.key_name)
+        self.assertEqual(TSIGKeyRing.SUCCESS, code)
+        self.assertEqual(self.key_name, key.get_key_name())
+        self.assertEqual(TSIGKey.HMACSHA256_NAME, key.get_algorithm_name())
+        self.assertEqual(self.secret, key.get_secret())
+
         (code, key) = self.keyring.find(Name('different-key.example'),
                                         self.sha256_name)
         self.assertEqual(TSIGKeyRing.NOTFOUND, code)

src/lib/dns/python/tsigkey_python.cc

@@ -287,7 +287,9 @@ PyMethodDef TSIGKeyRing_methods[] = {
       METH_VARARGS,
       "Remove a TSIGKey for the given name from the TSIGKeyRing." },
     { "find", reinterpret_cast<PyCFunction>(TSIGKeyRing_find), METH_VARARGS,
-      "Find a TSIGKey for the given name in the TSIGKeyRing. "
+      "Find a TSIGKey for the given name in the TSIGKeyRing. Optional "
+      "second argument is an algorithm, in which case it only returns "
+      "a key if both match.\n"
       "It returns a tuple of (result_code, key)." },
     { NULL, NULL, 0, NULL }
 };
@@ -362,13 +364,16 @@ TSIGKeyRing_remove(const s_TSIGKeyRing* self, PyObject* args) {
 PyObject*
 TSIGKeyRing_find(const s_TSIGKeyRing* self, PyObject* args) {
     PyObject* key_name;
-    PyObject* algorithm_name;
+    PyObject* algorithm_name = NULL;
 
-    if (PyArg_ParseTuple(args, "O!O!", &name_type, &key_name,
+    if (PyArg_ParseTuple(args, "O!|O!", &name_type, &key_name,
                          &name_type, &algorithm_name)) {
-        const TSIGKeyRing::FindResult result =
-            self->cppobj->find(PyName_ToName(key_name),
-                               PyName_ToName(algorithm_name));
+        // Can't init TSIGKeyRing::FindResult without actual result,
+        // so use ternary operator
+        TSIGKeyRing::FindResult result = (algorithm_name == NULL) ?
+                    self->cppobj->find(PyName_ToName(key_name)) :
+                    self->cppobj->find(PyName_ToName(key_name),
+                                       PyName_ToName(algorithm_name));
         if (result.key != NULL) {
             s_TSIGKey* key = PyObject_New(s_TSIGKey, &tsigkey_type);
             if (key == NULL) {

src/lib/dns/tests/tsigkey_unittest.cc

@@ -251,6 +251,15 @@ TEST_F(TSIGKeyRingTest, find) {
     const TSIGKeyRing::FindResult result3 = keyring.find(key_name, md5_name);
     EXPECT_EQ(TSIGKeyRing::NOTFOUND, result3.code);
     EXPECT_EQ(static_cast<const TSIGKey*>(NULL), result3.key);
+
+    // But with just the name it should work
+    const TSIGKeyRing::FindResult result4(keyring.find(key_name));
+    EXPECT_EQ(TSIGKeyRing::SUCCESS, result1.code);
+    EXPECT_EQ(key_name, result1.key->getKeyName());
+    EXPECT_EQ(TSIGKey::HMACSHA256_NAME(), result1.key->getAlgorithmName());
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, secret, secret_len,
+                        result1.key->getSecret(),
+                        result1.key->getSecretLength());
 }
 
 TEST_F(TSIGKeyRingTest, findFromSome) {

src/lib/dns/tsigkey.cc

@@ -51,7 +51,7 @@ namespace {
         if (name == TSIGKey::HMACSHA512_NAME()) {
             return (isc::cryptolink::SHA512);
         }
- 
+
         return (isc::cryptolink::UNKNOWN_HASH);
     }
 }
@@ -270,6 +270,16 @@ TSIGKeyRing::remove(const Name& key_name) {
 }
 
 TSIGKeyRing::FindResult
+TSIGKeyRing::find(const Name& key_name) const {
+    TSIGKeyRingImpl::TSIGKeyMap::const_iterator found =
+        impl_->keys.find(key_name);
+    if (found == impl_->keys.end()) {
+        return (FindResult(NOTFOUND, NULL));
+    }
+    return (FindResult(SUCCESS, &((*found).second)));
+}
+
+TSIGKeyRing::FindResult
 TSIGKeyRing::find(const Name& key_name, const Name& algorithm_name) const {
     TSIGKeyRingImpl::TSIGKeyMap::const_iterator found =
         impl_->keys.find(key_name);

src/lib/dns/tsigkey.h

@@ -327,6 +327,27 @@ public:
     /// Find a \c TSIGKey for the given name in the \c TSIGKeyRing.
     ///
     /// It searches the internal storage for a \c TSIGKey whose name is
+    /// \c key_name.
+    /// It returns the result in the form of a \c FindResult
+    /// object as follows:
+    /// - \c code: \c SUCCESS if a key is found; otherwise \c NOTFOUND.
+    /// - \c key: A pointer to the found \c TSIGKey object if one is found;
+    /// otherwise \c NULL.
+    ///
+    /// The pointer returned in the \c FindResult object is only valid until
+    /// the corresponding key is removed from the key ring.
+    /// The caller must ensure that the key is held in the key ring while
+    /// it needs to refer to it, or it must make a local copy of the key.
+    ///
+    /// This method never throws an exception.
+    ///
+    /// \param key_name The name of the key to be found.
+    /// \return A \c FindResult object enclosing the search result (see above).
+    FindResult find(const Name& key_name) const;
+
+    /// Find a \c TSIGKey for the given name in the \c TSIGKeyRing.
+    ///
+    /// It searches the internal storage for a \c TSIGKey whose name is
     /// \c key_name and that uses the hash algorithm identified by
     /// \c algorithm_name.
     /// It returns the result in the form of a \c FindResult
@@ -346,6 +367,7 @@ public:
     /// \param algorithm_name The name of the algorithm of the found key.
     /// \return A \c FindResult object enclosing the search result (see above).
     FindResult find(const Name& key_name, const Name& algorithm_name) const;
+
 private:
     struct TSIGKeyRingImpl;
     TSIGKeyRingImpl* impl_;

tests/lettuce/features/xfrin_bind10.feature

@@ -79,7 +79,7 @@ Feature: Xfrin
     When I send bind10 the following commands:
     """
     config add tsig_keys/keys "example.key.:c2VjcmV0"
-    config set Xfrin/zones[0]/tsig_key  "example.key.:c2VjcmV0"
+    config set Xfrin/zones[0]/tsig_key  "example.key."
     config commit
     """