[1357] Commit only after verifying the last TSIG

We can't commit during the stream of changes, they might be not signed
and that'd be a security risk (if someone stole the TCP connection and
pushed a change that'd put bogus data inside before getting to the last
message which would be rejected then).

Fixed the check: it now happens on the last message, not on each message
except the last. Returning true there does not mean to continue, but
that the current RR has been handled fully.

src/bin/xfrin/tests/xfrin_test.py

@@ -571,10 +571,18 @@ class TestXfrinIXFRAdd(TestXfrinState):
         self.conn._diff.add_data(self.ns_rrset) # put some dummy change
         self.conn._tsig_ctx = MockTSIGContext(TSIG_KEY)
         self.conn._tsig_ctx.last_has_signature = lambda: False
+        # First, push a starting SOA inside. This should be OK, nothing checked
+        # yet.
+        self.state.handle_rr(self.conn, self.begin_soa)
+        end_soa_rdata = Rdata(RRType.SOA(), TEST_RRCLASS,
+                              'm. r. 1234 0 0 0 0')
+        end_soa_rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.SOA(),
+                                RRTTL(3600))
+        end_soa_rrset.add_rdata(end_soa_rdata)
         # This would try to finish up. But the TSIG pretends not everything is
         # signed, rejecting it.
         self.assertRaises(xfrin.XfrinProtocolError, self.state.handle_rr,
-                          self.conn, self.begin_soa)
+                          self.conn, end_soa_rrset)
 
     def test_handle_out_of_sync(self):
         # getting SOA with an inconsistent serial.  This is an error.
@@ -1577,16 +1585,18 @@ class TestIXFRResponse(TestXfrinConnection):
         self.conn._handle_xfrin_responses()
         self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
         self.assertEqual([], self.conn._datasrc_client.diffs)
+        # Everything is committed as one bunch, currently we commit at the very
+        # end.
         check_diffs(self.assertEqual,
                     [[('delete', begin_soa_rrset),
                       ('delete', self._create_a('192.0.2.1')),
                       ('add', self._create_soa('1231')),
-                      ('add', self._create_a('192.0.2.2'))],
-                     [('delete', self._create_soa('1231')),
+                      ('add', self._create_a('192.0.2.2')),
+                      ('delete', self._create_soa('1231')),
                       ('delete', self._create_a('192.0.2.3')),
                       ('add', self._create_soa('1232')),
-                      ('add', self._create_a('192.0.2.4'))],
-                     [('delete', self._create_soa('1232')),
+                      ('add', self._create_a('192.0.2.4')),
+                      ('delete', self._create_soa('1232')),
                       ('delete', self._create_a('192.0.2.5')),
                       ('add', soa_rrset),
                       ('add', self._create_a('192.0.2.6'))]],

src/bin/xfrin/xfrin.py.in

@@ -420,9 +420,9 @@ class XfrinIXFRAdd(XfrinState):
             conn.get_transfer_stats().ixfr_changeset_count += 1
             soa_serial = get_soa_serial(rr.get_rdata()[0])
             if soa_serial == conn._end_serial:
-                # FIXME: It's insecure to do a full commit here, due
-                # to the fact that the message we're working on might not
-                # be signed right now.
+                # The final part is there. Check all was signed
+                # and commit it to the database.
+                conn._check_response_tsig_last()
                 conn._diff.commit()
                 self.set_xfrstate(conn, XfrinIXFREnd())
                 return True
@@ -432,8 +432,10 @@ class XfrinIXFRAdd(XfrinState):
                                          str(conn._current_serial) +
                                          ', got ' + str(soa_serial))
             else:
-                conn._check_response_tsig_last()
-                conn._diff.commit()
+                # Apply a change to the database. But don't commit it yet,
+                # we can't know if the message is/will be properly signed.
+                # A complete commit will happen after the last bit.
+                conn._diff.apply()
                 self.set_xfrstate(conn, XfrinIXFRDeleteSOA())
                 return False
         conn._diff.add_data(rr)