Fix #408 - Force new AS-SETs to be structured hierarchically (#413)

irrd/integration_tests/run.py

@@ -28,9 +28,9 @@
 IRRD_ROOT_PATH = str(Path(__file__).resolve().parents[2])
 sys.path.append(IRRD_ROOT_PATH)
 
-AS_SET_REFERRING_OTHER_SET = """as-set:         AS-TESTREF
+AS_SET_REFERRING_OTHER_SET = """as-set:         AS65537:AS-TESTREF
 descr:          description
-members:        AS-SETTEST, AS65540
+members:        AS65537:AS-SETTEST, AS65540
 tech-c:         PERSON-TEST
 admin-c:        PERSON-TEST
 notify:         notify@example.com
@@ -358,13 +358,13 @@ def test_irrd_integration(self, tmpdir):
         assert query_result == '2001:db8::/48'
         query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!iRS-TEST')
         assert set(query_result.split(' ')) == {'192.0.2.0/24', '2001:db8::/48', 'RS-OTHER-SET'}
-        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!aAS-SETTEST')
+        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!aAS65537:AS-SETTEST')
         assert set(query_result.split(' ')) == {'192.0.2.0/24', '2001:db8::/48'}
-        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!aAS-TESTREF')
+        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!aAS65537:AS-TESTREF')
         assert set(query_result.split(' ')) == {'192.0.2.0/24', '2001:db8::/48'}
-        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!a4AS-TESTREF')
+        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!a4AS65537:AS-TESTREF')
         assert query_result == '192.0.2.0/24'
-        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!a6AS-TESTREF')
+        query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!a6AS65537:AS-TESTREF')
         assert query_result == '2001:db8::/48'
         query_result = whois_query_irrd('127.0.0.1', self.port_whois1, '!r192.0.2.0/24')
         assert 'example route' in query_result
@@ -395,9 +395,9 @@ def test_irrd_integration(self, tmpdir):
         assert query_result == '2001:db8::/48'
         query_result = whois_query_irrd('127.0.0.1', self.port_whois2, '!iRS-TEST')
         assert query_result == '2001:db8::/48 RS-OTHER-SET'
-        query_result = whois_query_irrd('127.0.0.1', self.port_whois2, '!aAS-SETTEST')
+        query_result = whois_query_irrd('127.0.0.1', self.port_whois2, '!aAS65537:AS-SETTEST')
         assert query_result == '2001:db8::/48'
-        query_result = whois_query_irrd('127.0.0.1', self.port_whois2, '!aAS-TESTREF')
+        query_result = whois_query_irrd('127.0.0.1', self.port_whois2, '!aAS65537:AS-TESTREF')
         assert query_result == '2001:db8::/48'
         query_result = whois_query('127.0.0.1', self.port_whois2, '-x 192.0.02.0/24')
         assert 'example route' not in query_result
@@ -409,11 +409,11 @@ def test_irrd_integration(self, tmpdir):
 
         # These queries should produce identical answers on both instances.
         for port in self.port_whois1, self.port_whois2:
-            query_result = whois_query_irrd('127.0.0.1', port, '!iAS-SETTEST')
+            query_result = whois_query_irrd('127.0.0.1', port, '!iAS65537:AS-SETTEST')
             assert set(query_result.split(' ')) == {'AS65537', 'AS65538', 'AS65539', 'AS-OTHERSET'}
-            query_result = whois_query_irrd('127.0.0.1', port, '!iAS-TESTREF')
-            assert set(query_result.split(' ')) == {'AS-SETTEST', 'AS65540'}
-            query_result = whois_query_irrd('127.0.0.1', port, '!iAS-TESTREF,1')
+            query_result = whois_query_irrd('127.0.0.1', port, '!iAS65537:AS-TESTREF')
+            assert set(query_result.split(' ')) == {'AS65537:AS-SETTEST', 'AS65540'}
+            query_result = whois_query_irrd('127.0.0.1', port, '!iAS65537:AS-TESTREF,1')
             assert set(query_result.split(' ')) == {'AS65537', 'AS65538', 'AS65539', 'AS65540'}
             query_result = whois_query_irrd('127.0.0.1', port, '!maut-num,as65537')
             assert 'AS65537' in query_result

irrd/rpsl/fields.py

@@ -312,7 +312,7 @@ class RPSLSetNameField(RPSLTextField):
     must be a valid set name for this specific set, i.e. start with the given prefix.
 
     The prefix provided is the expected prefix of the set name, e.g. 'RS' for
-    a route-set, or 'AS' for an as-set.R
+    a route-set, or 'AS' for an as-set.
     """
     keep_case = False
 

irrd/rpsl/parser.py

@@ -212,6 +212,15 @@ def generate_template(self):
         return template
 
     def clean(self) -> bool:
+        """
+        Additional cleaning steps for some objects.
+        """
+        return True
+
+    def clean_for_create(self) -> bool:
+        """
+        Additional cleaning steps for creations only.
+        """
         return True
 
     def _extract_attributes_values(self, text: str) -> None:

irrd/rpsl/rpsl_objects.py

@@ -13,6 +13,7 @@
                      RPSLReferenceListField, RPSLAuthField, RPSLRouteSetMembersField,
                      RPSLChangedField, RPSLURLField)
 from .parser import RPSLObject, UnknownRPSLObjectClassException
+from ..utils.validators import parse_as_number, ValidationError
 
 RPSL_ROUTE_OBJECT_CLASS_FOR_IP_VERSION = {
     4: 'route',
@@ -59,6 +60,16 @@ class RPSLAsSet(RPSLObject):
         ('source', RPSLGenericNameField()),
     ])
 
+    def clean_for_create(self) -> bool:
+        first_segment = self.pk().split(':')[0]
+        try:
+            parse_as_number(first_segment)
+            return True
+        except ValidationError as ve:
+            self.messages.error('AS set names must be hierarchical and the first component must '
+                                f'be an AS number, e.g. "AS65537:AS-EXAMPLE": {str(ve)}')
+        return False
+
 
 class RPSLAutNum(RPSLObject):
     fields = OrderedDict([

irrd/rpsl/tests/test_rpsl_objects.py

@@ -132,8 +132,9 @@ def test_parse(self):
         rpsl_text = object_sample_mapping[RPSLAsSet().rpsl_object_class]
         obj = rpsl_object_from_text(rpsl_text)
         assert obj.__class__ == RPSLAsSet
+        assert obj.clean_for_create()
         assert not obj.messages.errors()
-        assert obj.pk() == 'AS-SETTEST'
+        assert obj.pk() == 'AS65537:AS-SETTEST'
         assert obj.referred_strong_objects() == [
             ('admin-c', ['role', 'person'], ['PERSON-TEST']),
             ('tech-c', ['role', 'person'], ['PERSON-TEST']),
@@ -146,6 +147,15 @@ def test_parse(self):
         # Field parsing will cause our object to look slightly different than the original, hence the replace()
         assert obj.render_rpsl_text() == rpsl_text.replace('AS65538, AS65539', 'AS65538,AS65539')
 
+    def test_invalid_clean_for_create(self):
+        rpsl_text = object_sample_mapping[RPSLAsSet().rpsl_object_class]
+        rpsl_text = rpsl_text.replace('AS65537:AS-SETTEST', 'AS-SETTEST')
+        obj = rpsl_object_from_text(rpsl_text)
+        assert obj.__class__ == RPSLAsSet
+        assert not obj.messages.errors()
+        assert not obj.clean_for_create()
+        assert 'AS set names must be hierarchical and the first ' in obj.messages.errors()[0]
+
 
 class TestRPSLAutNum:
     def test_has_mapping(self):

irrd/updates/parser.py

@@ -217,6 +217,11 @@ def notification_targets(self) -> Set[str]:
         return targets
 
     def validate(self) -> bool:
+        if self.rpsl_obj_new and self.request_type == UpdateRequestType.CREATE:
+            if not self.rpsl_obj_new.clean_for_create():
+                self.error_messages += self.rpsl_obj_new.messages.errors()
+                self.status = UpdateRequestStatus.ERROR_PARSING
+                return False
         auth_valid = self._check_auth()
         if not auth_valid:
             return False

irrd/updates/tests/test_parser.py

@@ -18,7 +18,7 @@
 from irrd.utils.text import splitline_unicodesafe
 from ..parser import parse_change_requests
 from ..parser_state import UpdateRequestType, UpdateRequestStatus
-from ..validators import ReferenceValidator, AuthValidator
+from ..validators import ReferenceValidator, AuthValidator, ValidatorResult
 
 
 @pytest.fixture()
@@ -117,6 +117,30 @@ def test_non_authorative_source(self, prepare_mocks):
         assert not result.is_valid()
         assert result.error_messages == ['This instance is not authoritative for source TEST2']
 
+    def test_validates_for_create(self, prepare_mocks):
+        mock_dq, mock_dh = prepare_mocks
+
+        mock_dh.execute_query = lambda query: []
+
+        auth_validator = Mock()
+        invalid_auth_result = ValidatorResult()
+        invalid_auth_result.error_messages.add('error catch')
+        auth_validator.process_auth = lambda new, cur: invalid_auth_result
+
+        invalid_create_text = SAMPLE_AS_SET.replace('AS65537:AS-SETTEST', 'AS-SETTEST')
+        result = parse_change_requests(invalid_create_text, mock_dh, auth_validator, None)[0]
+
+        assert not result.validate()
+        assert result.status == UpdateRequestStatus.ERROR_PARSING
+        assert len(result.error_messages) == 1
+        assert 'AS set names must be hierarchical and the first' in result.error_messages[0]
+
+        # Test again with an UPDATE (which then fails on auth to stop)
+        mock_dh.execute_query = lambda query: [{'object_text': SAMPLE_AS_SET}]
+        result = parse_change_requests(invalid_create_text, mock_dh, auth_validator, None)[0]
+        assert not result.validate()
+        assert result.error_messages == ['error catch']
+
     def test_save_nonexistent_object(self, prepare_mocks):
         mock_dq, mock_dh = prepare_mocks
         mock_dh.execute_query = lambda query: []
@@ -1015,7 +1039,7 @@ def test_user_report(self, prepare_mocks):
         assert 'remarks: ' in report_inetnum  # full RPSL object should be included
         assert 'INFO: Address range 192' in report_inetnum
 
-        assert report_as_set == 'Create succeeded: [as-set] AS-SETTEST\n'
+        assert report_as_set == 'Create succeeded: [as-set] AS65537:AS-SETTEST\n'
 
         assert 'FAILED' in report_unknown
         assert 'ERROR: unknown object class' in report_unknown
@@ -1050,9 +1074,9 @@ def test_user_report(self, prepare_mocks):
         """).strip() + '\n'
 
         assert result_as_set.notification_target_report() == textwrap.dedent("""
-            Create succeeded for object below: [as-set] AS-SETTEST:
+            Create succeeded for object below: [as-set] AS65537:AS-SETTEST:
             
-            as-set:         AS-SETTEST
+            as-set:         AS65537:AS-SETTEST
             descr:          description
             members:        AS65538,AS65539
             members:        AS65537

irrd/utils/rpsl_samples.py

@@ -12,7 +12,7 @@
 remarks:        remark
 """
 
-SAMPLE_AS_SET = """as-set:         AS-SETTEST
+SAMPLE_AS_SET = """as-set:         AS65537:AS-SETTEST
 descr:          description
 members:        AS65538, AS65539
 members:        AS65537