Merge pull request #62 from esnet-security/topic/chriscummings/SCRAM-…

…56/add-some-tests

tests: add coverage and fix ci

.gitignore

@@ -160,6 +160,7 @@ typings/
 !.vscode/tasks.json
 !.vscode/launch.json
 !.vscode/extensions.json
+*.code-workspace
 
 
 # Provided default Pycharm Run/Debug Configurations should be tracked by git

.gitlab-ci.yml

@@ -7,7 +7,6 @@ include:
 stages:
 - lint
 - test
-- cleanup
 
 variables:
   POSTGRES_USER: scram
@@ -40,12 +39,18 @@ pytest:
   - make migrate
   - make run
   script:
+  - export COMPOSE_PROJECT_NAME=$CI_PIPELINE_ID
   - make coverage.xml
   artifacts:
     reports:
       coverage_report:
         coverage_format: cobertura
         path: coverage.xml
+  after_script:
+    - export COMPOSE_PROJECT_NAME=$CI_PIPELINE_ID
+    - make stop
+    - make clean
+
 
 gemnasium-dependency_scanning:
   variables:
@@ -61,17 +66,3 @@ code_quality_html:
 
 sast:
   stage: test
-
-final_clean:
-  image: docker:24.0.6-dind
-  services:
-  - docker:dind
-  before_script:
-  - apk add make
-  - export COMPOSE_PROJECT_NAME=$CI_PIPELINE_ID
-  stage: cleanup
-  rules:
-    - when: always # run even if something failed
-  script:
-    - make stop
-    - make clean

Makefile

@@ -22,17 +22,17 @@ compose.override.yml:
 ## behave-all: runs behave inside the containers against all of your features
 .Phony: behave-all
 behave-all: compose.override.yml
-	@docker compose run django coverage run -a manage.py behave --no-input --simple
+	@docker compose run --rm django coverage run -a manage.py behave --no-input --simple
 
 ## behave: runs behave inside the containers against a specific feature (append FEATURE=feature_name_here)
 .Phony: behave
 behave: compose.override.yml
-	@docker compose run django python manage.py behave --no-input --simple -i $(FEATURE)
+	@docker compose run --rm django python manage.py behave --no-input --simple -i $(FEATURE)
 
 ## behave-translator
 .Phony: behave-translator
 behave-translator: compose.override.yml
-	@docker compose exec -T translator /usr/local/bin/behave /app/acceptance/features
+	@docker compose exec -T translator /usr/local/bin/behave /app/tests/acceptance/features
 
 ## build: rebuilds all your containers or a single one if CONTAINER is specified
 .Phony: build
@@ -42,8 +42,8 @@ build: compose.override.yml
 
 ## coverage.xml: generate coverage from test runs
 coverage.xml: pytest behave-all behave-translator
-	@docker compose run django coverage report
-	@docker compose run django coverage xml
+	@docker compose run --rm django coverage report
+	@docker compose run --rm django coverage xml
 
 ## ci-test: runs all tests just like Gitlab CI does
 .Phony: ci-test
@@ -58,7 +58,7 @@ clean: compose.override.yml
 ## collect-static: run collect static admin command
 .Phony: collectstatic
 collectstatic: compose.override.yml
-	@docker compose run django python manage.py collectstatic
+	@docker compose run --rm django python manage.py collectstatic
 
 ## django-addr: get the IP and ephemeral port assigned to docker:8000
 .Phony: django-addr
@@ -101,18 +101,18 @@ list-routes: compose.override.yml
 ## migrate: makemigrations and then migrate
 .Phony: migrate
 migrate: compose.override.yml
-	@docker compose run django python manage.py makemigrations
-	@docker compose run django python manage.py migrate
+	@docker compose run --rm django python manage.py makemigrations
+	@docker compose run --rm django python manage.py migrate
 
 ## pass-reset: change admin's password
 .Phony: pass-reset
 pass-reset: compose.override.yml
-	@docker compose run django python manage.py changepassword admin
+	@docker compose run --rm django python manage.py changepassword admin
 
 ## pytest: runs pytest inside the containers
 .Phony: pytest
 pytest: compose.override.yml
-	@docker compose run django coverage run -m pytest
+	@docker compose run --rm django coverage run -m pytest
 
 ## run: brings up the containers as described in compose.override.yml
 .Phony: run
@@ -132,4 +132,4 @@ tail-log: compose.override.yml
 ## type-check: static type checking
 .Phony: type-check
 type-check: compose.override.yml
-	@docker compose run django mypy scram
+	@docker compose run --rm django mypy scram

compose/local/translator/Dockerfile

@@ -22,7 +22,9 @@ RUN mkdir /app \
 
 COPY ./translator/translator.py /app
 COPY ./translator/gobgp.py /app
-COPY ./translator/tests /app
+COPY ./translator/exceptions.py /app
+COPY ./translator/shared.py /app
+
 RUN chmod +x /app/translator.py
 
 WORKDIR /app

scram/route_manager/tests/test_websockets.py

@@ -183,5 +183,9 @@ def local_setUp(self):
             lambda ip, mask: {
                 "type": "translator_add",
                 "message": {"asn": 65550, "community": 100, "route": f"{ip}/{mask}"},
-            }
+            },
+            lambda ip, mask: {
+                "type": "translator_add",
+                "message": {"asn": 64496, "community": 4294967295, "route": f"{ip}/{mask}"},
+            },
         ]

translator/exceptions.py

@@ -0,0 +1,9 @@
+"""
+This module holds all of the exceptions we want to raise in our translators.
+"""
+
+
+class ASNError(TypeError):
+    """
+    ASNError provides an error class to use when there is an issue with an Autonomous System Number.
+    """

translator/gobgp.py

@@ -4,7 +4,9 @@
 import gobgp_pb2
 import gobgp_pb2_grpc
 import grpc
+from exceptions import ASNError
 from google.protobuf.any_pb2 import Any
+from shared import asn_is_valid
 
 _TIMEOUT_SECONDS = 1000
 DEFAULT_ASN = 65400
@@ -76,8 +78,8 @@ def _build_path(self, ip, event_data={}):
         as_path = Any()
         as_segment = None
 
-        # Make sure our asn is an acceptable number. This is the max as stated in rfc6996
-        assert 0 < asn < 4294967295
+        # Make sure our asn is an acceptable value.
+        asn_is_valid(asn)
         as_segment = [attribute_pb2.AsSegment(numbers=[asn])]
         as_segments = attribute_pb2.AsPathAttribute(segments=as_segment)
         as_path.Pack(as_segments)
@@ -88,6 +90,8 @@ def _build_path(self, ip, event_data={}):
         # Standard community
         # Since we pack both into the community string we need to make sure they will both fit
         if asn < 65536 and community < 65536:
+            # We bitshift ASN left by 16 so that there is room to add the community on the end of it. This is because
+            # GoBGP wants the community sent as a single integer.
             comm_id = (asn << 16) + community
             communities.Pack(attribute_pb2.CommunitiesAttribute(communities=[comm_id]))
         else:
@@ -120,8 +124,8 @@ def add_path(self, ip, event_data):
                 gobgp_pb2.AddPathRequest(table_type=gobgp_pb2.GLOBAL, path=path),
                 _TIMEOUT_SECONDS,
             )
-        except AssertionError:
-            logging.warning("ASN assertion failed")
+        except ASNError as e:
+            logging.warning(f"ASN assertion failed with error: {e}")
 
     def del_all_paths(self):
         logging.warning("Withdrawing ALL routes")
@@ -136,8 +140,8 @@ def del_path(self, ip, event_data):
                 gobgp_pb2.DeletePathRequest(table_type=gobgp_pb2.GLOBAL, path=path),
                 _TIMEOUT_SECONDS,
             )
-        except AssertionError:
-            logging.warning("ASN assertion failed")
+        except ASNError as e:
+            logging.warning(f"ASN assertion failed with error: {e}")
 
     def get_prefixes(self, ip):
         prefixes = [gobgp_pb2.TableLookupPrefix(prefix=str(ip.ip))]

translator/shared.py

@@ -0,0 +1,27 @@
+"""
+This module provides a location for code that we want to share between all translators.
+"""
+
+from exceptions import ASNError
+
+
+def asn_is_valid(asn: int) -> bool:
+    """
+    asn_is_valid makes sure that an ASN passed in is a valid 2 or 4 Byte ASN.
+
+    Args:
+        asn (int): The Autonomous System Number that we want to validate
+
+    Raises:
+        ASNError: If the ASN is not between 0 and 4294967295 or is not an integer.
+
+    Returns:
+        bool: _description_
+    """
+    if not isinstance(asn, int):
+        raise ASNError(f"ASN {asn} is not an Integer, has type {type(asn)}")
+    if not 0 < asn < 4294967295:
+        # This is the max as stated in rfc6996
+        raise ASNError(f"ASN {asn} is out of range. Must be between 0 and 4294967295")
+
+    return True

translator/tests/acceptance/environment.py

@@ -3,3 +3,4 @@
 
 def before_all(context):
     context.gobgp = GoBGP("gobgp:50051")
+    context.config.setup_logging()

translator/tests/acceptance/features/bgp.feature

@@ -4,23 +4,24 @@ Feature: block with BGP
   Scenario Outline: We can block an IP
     When we add <route> with <asn> and <community> to the block list
     Then <route> is blocked
+    Then we delete <route> with <asn> and <community> from the block list
     And <unblock_ip> is unblocked
 
     Examples: data
-    | route           |  asn       | community | unblock_ip   |
-    | 192.0.2.4/32    | 54321      | 444       | 192.0.2.5    |
-    | 192.0.2.10/32   | 4200000000 | 321       | 192.0.2.11   |
-    | 2001:DB8:A::/64 | 54321      | 444       | baba::       |
-    | 2001:DB8:B::/64 | 4200000000 | 321       | 2001:DB8::4  |
+      | route            | asn        | community  | unblock_ip  |
+      | 192.0.2.4/32     | 54321      | 444        | 192.0.2.5   |
+      | 192.0.2.10/32    | 4200000000 | 321        | 192.0.2.11  |
+      | 2001:DB8:A::/64  | 54321      | 444        | baba::      |
+      | 2001:DB8:B::/64  | 4200000000 | 321        | 2001:DB8::4 |
+      | 192.0.2.20/32    | 4200000000 | 4200000000 | 192.0.2.11  |
+      | 2001:DB8:C::/64  | 4200000000 | 4200000000 | 2001:DB8::4 |
+      | 2001:DB8:C::1/64 | 4200000000 | 4200000000 | 2001:DB8::5 |
 
-  Scenario Outline: We can block an IP
-    When we add <route> with <asn> and <community> to the block list
-    And we delete <route> with <asn> and <community> from the block list
-    Then <unblock_ip> is unblocked
+  Scenario Outline: Invalid ASNs fail
+    When <route> and <community> with invalid <asn> is sent
 
     Examples: data
-    | route           |  asn       | community | unblock_ip   |
-    | 192.0.2.4/32    | 54321      | 444       | 192.0.2.4    |
-    | 192.0.2.10/32   | 4200000000 | 321       | 192.0.2.11   |
-    | 2001:DB8:A::/64 | 54321      | 444       | 2001:DB8::1 |
-    | 2001:DB8:B::/64 | 4200000000 | 321       | 2001:DB8::4  |
+      | route            | asn                    | community |
+      | 2001:DB8:C::2/64 | 4242424242424242424242 | 100       |
+      | 2001:DB8:C::2/64 | -1                     | 100       |
+      | 2001:DB8:C::2/64 | 0                      | 100       |

translator/tests/acceptance/steps/actions.py

@@ -1,7 +1,11 @@
 import ipaddress
+import logging
 import time
 
 from behave import then, when
+from behave.log_capture import capture
+
+logging.basicConfig(level=logging.DEBUG)
 
 
 @when("we add {route} with {asn} and {community} to the block list")
@@ -11,7 +15,7 @@ def add_block(context, route, asn, community):
     context.gobgp.add_path(ip, event_data)
 
 
-@when("we delete {route} with {asn} and {community} from the block list")
+@then("we delete {route} with {asn} and {community} from the block list")
 def del_block(context, route, asn, community):
     ip = ipaddress.ip_interface(route)
     event_data = {"asn": int(asn), "community": int(community)}
@@ -31,6 +35,13 @@ def get_block_status(context, ip):
     return False
 
 
+@capture
+@when("{route} and {community} with invalid {asn} is sent")
+def asn_validation_fails(context, route, asn, community):
+    add_block(context, route, asn, community)
+    assert context.log_capture.find_event("ASN assertion failed")
+
+
 @then("{ip} is blocked")
 def check_block(context, ip):
     assert get_block_status(context, ip)