Fix #212 - Restrict mntner creation to override password (#219)

Thanks to @bitcynth for suggesting this change.

docs/users/database-changes.rst

@@ -79,7 +79,11 @@ Notifications to maintainers or the address in the ``notify`` attribute are
 
 If an invalid override password is used, or if no override password was
 configured, the invalid use is logged, and authentication and notification
-proceeds as usual, as if no override password was provided.
+proceeds as usual, **as if no override password was provided.**
+
+.. note::
+    New `mntner` objects can only be created using the override password.
+
 
 Working with auth hash masking
 ------------------------------
@@ -107,6 +111,7 @@ Any other scenario, like submitting a mix of dummy and real hashes, or
 submitting dummy hashes along with multiple ``password`` attributes in
 the message, is considered an error.
 
+
 Referential integrity
 ---------------------
 IRRd enforces referential integrity between objects. This means it is not
@@ -123,6 +128,7 @@ fail, as A still exists.)
 In the same way, it's possible to create multiple objects that depend on each
 other in the same submission to IRRd.
 
+
 Authentication checks
 ---------------------
 When changing an object, authentication must pass for one of the
@@ -138,6 +144,7 @@ When creating a new `mntner`, a submission must pass authorisation for
 one of the auth methods of the new mntner. Other objects can be submitted
 that depend on the new `mntner` in the same submission.
 
+
 Object templates
 ----------------
 
@@ -183,13 +190,15 @@ This template shows:
 * The `admin-c` and `tech-c` attributes reference a `role` or `person`.
   This means they may refer to either object class.
 
+
 Notifications
 -------------
 IRRd will always reply to a submission with a report on the requested
 changes. Depending on the request and its result, additional notifications
 may be sent. The overview below details all notifications that may be
 sent.
 
+
 Authentication and notification overview
 ----------------------------------------
 

irrd/integration_tests/run.py

@@ -52,9 +52,7 @@
     SAMPLE_INET6NUM,
     SAMPLE_INETNUM,
     SAMPLE_KEY_CERT,
-    SAMPLE_MNTNER_CLEAN,
     SAMPLE_PEERING_SET,
-    SAMPLE_PERSON,
     SAMPLE_PERSON.replace('PERSON-TEST', 'DUMY2-TEST'),
     SAMPLE_ROLE,
     SAMPLE_ROUTE,
@@ -93,7 +91,7 @@ def test_irrd_integration(self, tmpdir):
         self._start_irrds()
 
         # Attempt to load a mntner with valid auth, but broken references.
-        self._submit_update(self.config_path1, SAMPLE_MNTNER + '\n\npassword: md5-password')
+        self._submit_update(self.config_path1, SAMPLE_MNTNER + '\n\noverride: override-password')
         messages = self._retrieve_mails()
         assert len(messages) == 1
         mail_text = self._extract_message_body(messages[0])
@@ -110,7 +108,7 @@ def test_irrd_integration(self, tmpdir):
         # Load a regular valid mntner and person into the DB, and verify
         # the contents of the result.
         self._submit_update(self.config_path1,
-                            SAMPLE_MNTNER_CLEAN + '\n\n' + SAMPLE_PERSON + '\n\npassword: md5-password')
+                            SAMPLE_MNTNER_CLEAN + '\n\n' + SAMPLE_PERSON + '\n\noverride: override-password')
         messages = self._retrieve_mails()
         assert len(messages) == 1
         mail_text = self._extract_message_body(messages[0])
@@ -299,8 +297,9 @@ def test_irrd_integration(self, tmpdir):
         assert 'No entries found for the selected source(s)' in person_text
         assert 'PERSON-TEST' not in person_text
 
-        # Load samples of all known objects.
-        self._submit_update(self.config_path1, LARGE_UPDATE + '\n\npassword: md5-password')
+        # Load the mntner and person again, using the override password
+        self._submit_update(self.config_path1,
+                            SAMPLE_MNTNER_CLEAN + '\n\n' + SAMPLE_PERSON + '\n\noverride: override-password')
         messages = self._retrieve_mails()
         assert len(messages) == 1
         mail_text = self._extract_message_body(messages[0])
@@ -309,6 +308,17 @@ def test_irrd_integration(self, tmpdir):
         assert messages[0]['To'] == 'Sasha <sasha@example.com>'
         assert '\nCreate succeeded: [mntner] TEST-MNT\n' in mail_text
         assert '\nCreate succeeded: [person] PERSON-TEST\n' in mail_text
+        assert 'email footer' in mail_text
+        assert 'Generated by IRRd version ' in mail_text
+
+        # Load samples of all known objects, using the mntner password
+        self._submit_update(self.config_path1, LARGE_UPDATE + '\n\npassword: md5-password')
+        messages = self._retrieve_mails()
+        assert len(messages) == 3
+        mail_text = self._extract_message_body(messages[0])
+        assert messages[0]['Subject'] == 'SUCCESS: my subject'
+        assert messages[0]['From'] == 'from@example.com'
+        assert messages[0]['To'] == 'Sasha <sasha@example.com>'
         assert '\nINFO: AS number as065537 was reformatted as AS65537\n' in mail_text
         assert '\nCreate succeeded: [filter-set] FLTR-SETTEST\n' in mail_text
         assert '\nINFO: Address range 192.0.2.0 - 192.0.02.255 was reformatted as 192.0.2.0 - 192.0.2.255\n' in mail_text
@@ -466,7 +476,7 @@ def _start_irrds(self):
 
                 'auth': {
                     'gnupg_keyring': None,
-                    'override_password': None,
+                    'override_password': '$1$J6KycItM$MbPaBU6iFSGFV299Rk7Di0',
                 },
 
                 'email': {

irrd/rpsl/tests/test_rpsl_objects.py

@@ -321,8 +321,8 @@ def test_parse_invalid_partial_dummy_hash(self):
     @pytest.mark.usefixtures('tmp_gpg_dir')  # noqa: F811
     def test_verify(self, tmp_gpg_dir):
         rpsl_text = object_sample_mapping[RPSLMntner().rpsl_object_class]
-        # Unknown hashes should simply be ignored.
-        obj = rpsl_object_from_text(rpsl_text + 'auth: UNKNOWN_HASH foo')
+        # Unknown hashes and invalid hashes should simply be ignored.
+        obj = rpsl_object_from_text(rpsl_text + 'auth: UNKNOWN_HASH foo\nauth: MD5-PW 💩')
 
         assert obj.verify_auth(['crypt-password'])
         assert obj.verify_auth(['md5-password'])

irrd/updates/tests/test_handler.py

@@ -11,7 +11,7 @@
 
 
 @pytest.fixture()
-def prepare_mocks(monkeypatch):
+def prepare_mocks(monkeypatch, config_override):
     monkeypatch.setenv('IRRD_SOURCES_TEST_AUTHORITATIVE', '1')
     mock_dh = Mock()
     monkeypatch.setattr('irrd.updates.handler.DatabaseHandler', lambda enable_preload_update=True: mock_dh)
@@ -21,14 +21,15 @@ def prepare_mocks(monkeypatch):
     monkeypatch.setattr('irrd.updates.validators.RPSLDatabaseQuery', lambda: mock_dq)
     mock_email = Mock()
     monkeypatch.setattr('irrd.utils.email.send_email', mock_email)
+    config_override({'auth': {'override_password': '$1$J6KycItM$MbPaBU6iFSGFV299Rk7Di0'}})
     yield mock_dq, mock_dh, mock_email
 
 
 class TestChangeSubmissionHandler:
     # NOTE: the scope of this test also includes ChangeRequest, ReferenceValidator and AuthValidator -
     # this is more of an update handler integration test.
 
-    def test_parse_valid_new_objects(self, prepare_mocks):
+    def test_parse_valid_new_objects_with_override(self, prepare_mocks):
         mock_dq, mock_dh, mock_email = prepare_mocks
         mock_dh.execute_query = lambda query: []
 
@@ -51,7 +52,7 @@ def test_parse_valid_new_objects(self, prepare_mocks):
         changed:        2016-10-05T10:41:15Z
         source:         TEST
         
-        password: md5-password
+        override: override-password
 
         inetnum:        80.16.151.184 - 80.016.151.191
         netname:        NETECONOMY-MG41731
@@ -75,9 +76,6 @@ def test_parse_valid_new_objects(self, prepare_mocks):
             ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pk', ('TEST-MNT',), {}],
             ['sources', (['TEST'],), {}], ['object_classes', (['inetnum'],), {}],
             ['rpsl_pk', ('80.16.151.184 - 80.16.151.191',), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}]
         ]
 
         assert mock_dh.mock_calls[0][0] == 'upsert_rpsl_object'
@@ -132,7 +130,7 @@ def test_parse_valid_new_objects(self, prepare_mocks):
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         """)
 
-    def test_parse_valid_new_objects_pgp_key(self, prepare_mocks):
+    def test_parse_valid_new_person_existing_mntner_pgp_key(self, prepare_mocks):
         mock_dq, mock_dh, mock_email = prepare_mocks
 
         person_text = textwrap.dedent("""
@@ -433,7 +431,7 @@ def test_parse_invalid_cascading_failure(self, prepare_mocks):
         changed:        2016-10-05T10:41:15Z
         source:         TEST
 
-        password: md5-password
+        override: override-password
 
         inetnum:        80.16.151.184 - 80.016.151.191
         netname:        NETECONOMY-MG41731
@@ -466,15 +464,9 @@ def test_parse_invalid_cascading_failure(self, prepare_mocks):
             ['sources', (['TEST'],), {}], ['object_classes', (['inetnum'],), {}],
             ['rpsl_pk', ('80.16.151.184 - 80.16.151.191',), {}],
             ['sources', (['TEST'],), {}], ['object_classes', (['person'],), {}], ['rpsl_pk', ('PERSON-TEST',), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'OTHER-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['role', 'person'],), {}],
-            ['rpsl_pk', ('PERSON-TEST',), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['role', 'person'],), {}],
-            ['rpsl_pk', ('PERSON-TEST',), {}],
+            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pk', ('OTHER-MNT',), {}],
+            ['sources', (['TEST'],), {}], ['object_classes', (['role', 'person'],), {}], ['rpsl_pk', ('PERSON-TEST',), {}],
+            ['sources', (['TEST'],), {}], ['object_classes', (['role', 'person'],), {}], ['rpsl_pk', ('PERSON-TEST',), {}],
             ['sources', (['TEST'],), {}], ['object_classes', (['role', 'person'],), {}], ['rpsl_pk', ('PERSON-TEST',), {}],
         ]
         assert flatten_mock_calls(mock_dh) == [
@@ -544,7 +536,7 @@ def test_parse_invalid_cascading_failure(self, prepare_mocks):
         changed:        2009-07-24T17:00:00Z
         source:         TEST
         
-        ERROR: Authorisation for person PERSON-TEST failed: must by authenticated by one of: OTHER-MNT
+        ERROR: Object OTHER-MNT referenced in field mnt-by not found in database TEST - must reference mntner.
         
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         """)
@@ -652,7 +644,17 @@ def test_parse_invalid_single_failure_invalid_password(self, prepare_mocks):
 
     def test_parse_invalid_cascading_failure_invalid_password(self, prepare_mocks):
         mock_dq, mock_dh, mock_email = prepare_mocks
-        mock_dh.execute_query = lambda query: []
+
+        query_results = iter([
+            [],
+            [{'object_text': SAMPLE_MNTNER}],
+            [],
+            [{'object_text': SAMPLE_MNTNER}],
+            [{'object_text': SAMPLE_MNTNER}],
+            [{'object_text': SAMPLE_MNTNER}],
+            [{'object_text': SAMPLE_MNTNER}],
+        ])
+        mock_dh.execute_query = lambda query: next(query_results)
 
         rpsl_text = textwrap.dedent("""
         person:         Placeholder Person Object
@@ -698,10 +700,8 @@ def test_parse_invalid_cascading_failure_invalid_password(self, prepare_mocks):
             ['sources', (['TEST'],), {}], ['object_classes', (['inetnum'],), {}],
             ['rpsl_pk', ('80.16.151.184 - 80.16.151.191',), {}],
             ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
-            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}], ['rpsl_pks', ({'TEST-MNT'},), {}],
+            ['sources', (['TEST'],), {}], ['object_classes', (['mntner'],), {}],
+            ['rpsl_pks', ({'OTHER1-MNT', 'OTHER2-MNT'},), {}],
         ]
         assert flatten_mock_calls(mock_dh) == [
             ['commit', (), {}],
@@ -717,8 +717,8 @@ def test_parse_invalid_cascading_failure_invalid_password(self, prepare_mocks):
             Modify:        0
             Delete:        0
         Number of objects processed with errors:    3
-            Create:        3
-            Modify:        0
+            Create:        2
+            Modify:        1
             Delete:        0
 
         DETAILED EXPLANATION:
@@ -739,7 +739,7 @@ def test_parse_invalid_cascading_failure_invalid_password(self, prepare_mocks):
         ERROR: Authorisation for person PERSON-TEST failed: must by authenticated by one of: TEST-MNT
         
         ---
-        Create FAILED: [mntner] TEST-MNT
+        Modify FAILED: [mntner] TEST-MNT
         
         mntner:         TEST-MNT
         admin-c:        PERSON-TEST
@@ -750,6 +750,8 @@ def test_parse_invalid_cascading_failure_invalid_password(self, prepare_mocks):
         changed:        2016-10-05T10:41:15Z
         source:         TEST
         
+        ERROR: Authorisation for mntner TEST-MNT failed: must by authenticated by one of: TEST-MNT
+        ERROR: Authorisation for mntner TEST-MNT failed: must by authenticated by one of: TEST-MNT, OTHER1-MNT, OTHER2-MNT
         ERROR: Authorisation failed for the auth methods on this mntner object.
         
         ---

irrd/updates/tests/test_parser.py

@@ -345,7 +345,6 @@ def test_check_auth_valid_update_mntner(self, prepare_mocks):
         assert result_inetnum._check_auth()
         assert not result_inetnum.error_messages
 
-
     def test_check_auth_valid_create_mntner_referencing_self(self, prepare_mocks):
         mock_dq, mock_dh = prepare_mocks
 
@@ -354,7 +353,7 @@ def test_check_auth_valid_create_mntner_referencing_self(self, prepare_mocks):
         reference_validator = ReferenceValidator(mock_dh)
         auth_validator = AuthValidator(mock_dh)
 
-        result_mntner = parse_change_requests(SAMPLE_MNTNER + 'password: md5-password',
+        result_mntner = parse_change_requests(SAMPLE_MNTNER + 'override: override-password',
                                               mock_dh, auth_validator, reference_validator)[0]
         auth_validator.pre_approve([result_mntner])
 
@@ -365,9 +364,6 @@ def test_check_auth_valid_create_mntner_referencing_self(self, prepare_mocks):
             ['sources', (['TEST'],), {}],
             ['object_classes', (['mntner'],), {}],
             ['rpsl_pk', ('TEST-MNT',), {}],
-            ['sources', (['TEST'],), {}],
-            ['object_classes', (['mntner'],), {}],
-            ['rpsl_pks', ({'OTHER1-MNT', 'OTHER2-MNT', 'TEST-MNT'},), {}],
         ]
 
     def test_check_auth_invalid_create_mntner_referencing_self_wrong_password(self, prepare_mocks):
@@ -378,41 +374,15 @@ def test_check_auth_invalid_create_mntner_referencing_self_wrong_password(self,
         reference_validator = ReferenceValidator(mock_dh)
         auth_validator = AuthValidator(mock_dh)
 
-        result_mntner = parse_change_requests(SAMPLE_MNTNER + 'password: invalid-password',
+        result_mntner = parse_change_requests(SAMPLE_MNTNER + 'override: invalid-password',
                                               mock_dh, auth_validator, reference_validator)[0]
         auth_validator.pre_approve([result_mntner])
 
         assert not result_mntner._check_auth()
-        assert result_mntner.error_messages == ['Authorisation failed for the auth methods on this mntner object.']
-
-        assert flatten_mock_calls(mock_dq) == [
-            ['sources', (['TEST'],), {}],
-            ['object_classes', (['mntner'],), {}],
-            ['rpsl_pk', ('TEST-MNT',), {}],
-            ['sources', (['TEST'],), {}],
-            ['object_classes', (['mntner'],), {}],
-            ['rpsl_pks', ({'OTHER1-MNT', 'OTHER2-MNT', 'TEST-MNT'},), {}],
+        assert result_mntner.error_messages == [
+            'New mntner objects must be added by an administrator.',
         ]
 
-    def test_check_auth_invalid_create_mntner_referencing_self_with_dummy_passwords(self, prepare_mocks):
-        mock_dq, mock_dh = prepare_mocks
-
-        mock_dh.execute_query = lambda query: []
-
-        reference_validator = ReferenceValidator(mock_dh)
-        auth_validator = AuthValidator(mock_dh)
-
-        # Submit the mntner with dummy password values as would be returned by queries.
-        # This should not be allowed in new objects.
-        data = SAMPLE_MNTNER.replace('LEuuhsBJNFV0Q', PASSWORD_HASH_DUMMY_VALUE)
-        data = data.replace('$1$fgW84Y9r$kKEn9MUq8PChNKpQhO6BM.', PASSWORD_HASH_DUMMY_VALUE)
-        result_mntner = parse_change_requests(data + 'password: crypt-password',
-                                              mock_dh, auth_validator, reference_validator)[0]
-        auth_validator.pre_approve([result_mntner])
-
-        assert not result_mntner._check_auth()
-        assert result_mntner.error_messages == ['Authorisation failed for the auth methods on this mntner object.']
-
         assert flatten_mock_calls(mock_dq) == [
             ['sources', (['TEST'],), {}],
             ['object_classes', (['mntner'],), {}],
@@ -474,7 +444,6 @@ def test_check_auth_invalid_update_mntner_submits_new_object_with_mixed_dummy_ha
         result_mntner = parse_change_requests(data + 'password: md5-password',
                                               mock_dh, auth_validator, reference_validator)[0]
         auth_validator.pre_approve([result_mntner])
-        result_mntner._check_auth()
         assert not result_mntner.is_valid()
         assert result_mntner.error_messages == [
             'Either all password auth hashes in a submitted mntner must be dummy objects, or none.',

irrd/updates/validators.py

@@ -209,8 +209,11 @@ def process_auth(self, rpsl_obj_new: RPSLObject, rpsl_obj_current: Optional[RPSL
             result.mntners_notify = mntner_objs_new
 
         if isinstance(rpsl_obj_new, RPSLMntner):
-            # Dummy auth values are only permitted in existing objects, which are never pre-approved.
-            if rpsl_obj_new.has_dummy_auth_value() and rpsl_obj_new.pk() not in self._pre_approved:
+            if not rpsl_obj_current:
+                result.error_messages.add('New mntner objects must be added by an administrator.')
+                return result
+            # Dummy auth values are only permitted in existing objects
+            if rpsl_obj_new.has_dummy_auth_value():
                 if len(self.passwords) == 1:
                     logger.debug(f'Object {rpsl_obj_new} submitted with dummy hash values and single password, '
                                  f'replacing all hashes with currently supplied password.')