diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5315066a..7b903538 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ exclude: 'docs|migrations|.git|.tox' -default_stages: [commit] +default_stages: [pre-commit] fail_fast: true repos: diff --git a/config/api_router.py b/config/api_router.py index e7f0ffbf..1705f3cc 100644 --- a/config/api_router.py +++ b/config/api_router.py @@ -13,12 +13,14 @@ router = DefaultRouter() -router.register("users", UserViewSet) -router.register("actiontypes", ActionTypeViewSet) -router.register("register_client", ClientViewSet) -router.register("entries", EntryViewSet) -router.register("ignore_entries", IgnoreEntryViewSet) -router.register("is_active", IsActiveViewSet, "is_active") +# The basename parameter is really only required for the IsActiveViewSet since that has a dynamic queryset. +# The others have a static queryset so they don't strictly require it. I added it for consistency. +router.register("users", UserViewSet, basename="user") +router.register("actiontypes", ActionTypeViewSet, basename="actiontype") +router.register("register_client", ClientViewSet, basename="client") +router.register("entries", EntryViewSet, basename="entry") +router.register("ignore_entries", IgnoreEntryViewSet, basename="ignoreentry") +router.register("is_active", IsActiveViewSet, basename="is_active") app_name = "api" urlpatterns = router.urls diff --git a/scram/route_manager/admin.py b/scram/route_manager/admin.py index 00d267b3..dfeb4224 100644 --- a/scram/route_manager/admin.py +++ b/scram/route_manager/admin.py @@ -3,7 +3,15 @@ from django.contrib import admin from simple_history.admin import SimpleHistoryAdmin -from .models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketMessage, WebSocketSequenceElement +from .models import ( + ActionType, + Client, + Entry, + IgnoreEntry, + Route, + WebSocketMessage, + WebSocketSequenceElement, +) class WhoFilter(admin.SimpleListFilter): @@ -52,8 +60,15 @@ class EntryAdmin(SimpleHistoryAdmin): search_fields = ["route", "comment"] +@admin.register(Client) +class ClientAdmin(admin.ModelAdmin): + """Configure the Client and how it shows up in the Admin site.""" + + list_display = ("client_name", "uuid", "registered_from_ip") + readonly_fields = ("uuid",) + + admin.site.register(IgnoreEntry, SimpleHistoryAdmin) admin.site.register(Route) -admin.site.register(Client) admin.site.register(WebSocketMessage) admin.site.register(WebSocketSequenceElement) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 2896b73c..3bc3df1a 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -45,11 +45,14 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" + uuid = serializers.UUIDField(required=False) + class Meta: """Maps to the Client model, and specifies the fields exposed by the API.""" model = Client - fields = ["hostname", "uuid"] + fields = ["client_name", "uuid", "registered_from_ip"] + read_only_fields = ["registered_from_ip"] class IsActiveSerializer(serializers.ModelSerializer): diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index b5d3d6f8..dae3cf2d 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -8,13 +8,15 @@ from django.conf import settings from django.core.exceptions import PermissionDenied from django.db.models import Q -from drf_spectacular.utils import extend_schema +from drf_spectacular.utils import OpenApiResponse, extend_schema from rest_framework import status, viewsets from rest_framework.exceptions import ValidationError from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.response import Response from simple_history.utils import update_change_reason +from scram.shared.shared_code import get_client_ip + from ..models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketSequenceElement from .exceptions import ActiontypeNotAllowed, IgnoredRoute, NoActiveEntryFound, PrefixTooLarge from .serializers import ( @@ -30,11 +32,15 @@ @extend_schema( - description="API endpoint for actiontypes", - responses={200: ActionTypeSerializer}, + description="API endpoint for actiontypes.", + responses={ + 200: OpenApiResponse(response=ActionTypeSerializer, description="Successful retrieval of actiontype(s)."), + 403: OpenApiResponse(description="Authentication credentials were not provided."), + 404: OpenApiResponse(description="The requested actiontype does not exist."), + }, ) class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): - """Lookup ActionTypes by name when authenticated, and bind to the serializer.""" + """Lookup ActionTypes by name, and bind to the serializer.""" queryset = ActionType.objects.all() permission_classes = (IsAuthenticated,) @@ -43,8 +49,17 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): @extend_schema( - description="API endpoint for ignore entries", - responses={200: IgnoreEntrySerializer}, + description="API endpoint for ignore entries.", + responses={ + 200: OpenApiResponse( + response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry." + ), + 201: OpenApiResponse(response=IgnoreEntrySerializer, description="Ignore entry successfully created."), + 204: OpenApiResponse(description="Ignore entry successfully deleted."), + 400: OpenApiResponse(description="Invalid data provided."), + 403: OpenApiResponse(description="Authentication credentials were not provided."), + 404: OpenApiResponse(description="The requested ignore entry does not exist."), + }, ) class IgnoreEntryViewSet(viewsets.ModelViewSet): """Lookup IgnoreEntries by route when authenticated, and bind to the serializer.""" @@ -56,20 +71,80 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): @extend_schema( - description="API endpoint for clients", - responses={200: ClientSerializer}, + description="API endpoint for clients.", + responses={ + 200: OpenApiResponse( + response=ClientSerializer, + description="Client already existed and was retrieved successfully.", + ), + 201: OpenApiResponse(response=ClientSerializer, description="Client successfully created."), + 400: OpenApiResponse( + description="Client with this name already exists with a different UUID, or client_name was not provided." + ), + }, ) class ClientViewSet(viewsets.ModelViewSet): - """Lookup Client by hostname on POSTs regardless of authentication, and bind to the serializer.""" + """Lookup Client by client_name on POSTs regardless of authentication, and bind to the serializer.""" queryset = Client.objects.all() # We want to allow a client to be registered from anywhere permission_classes = (AllowAny,) serializer_class = ClientSerializer - lookup_field = "hostname" + lookup_field = "client_name" http_method_names = ["post"] + def perform_create(self, serializer): + """Create a new Client, capturing the IP address it was created from.""" + ip = get_client_ip(self.request) + serializer.save(registered_from_ip=ip) + + def create(self, request, *args, **kwargs): + """Create a new Client while avoiding information leaks hopefully.""" + client_name = request.data.get("client_name") + request_uuid = request.data.get("uuid") + + if not client_name: + return Response({"detail": "client_name is required."}, status=status.HTTP_400_BAD_REQUEST) + + existing_client = self.queryset.filter(client_name=client_name).first() + + if existing_client: + if request_uuid and str(existing_client.uuid) == request_uuid: + # Idempotent success + serializer = self.get_serializer(existing_client) + return Response(serializer.data, status=status.HTTP_200_OK) + + # Log the conflict without leaking client_names to the user + logger.warning( + "Client named %s already exists with a different UUID", + client_name, + ) + return Response( + {"detail": "Invalid client registration request."}, + status=status.HTTP_400_BAD_REQUEST, + ) + + return super().create(request, *args, **kwargs) + +@extend_schema( + description="API endpoint to check if a route is active.", + parameters=[ + { + "name": "cidr", + "required": True, + "type": "string", + "description": "The CIDR network to check (e.g., 192.0.2.0/24).", + "in": "query", + } + ], + responses={ + 200: OpenApiResponse( + response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route." + ), + 400: OpenApiResponse(description="The 'cidr' parameter is missing or invalid."), + }, +) class IsActiveViewSet(viewsets.ReadOnlyModelViewSet): """Look up a route to see if SCRAM considers it active or deactivated.""" @@ -116,8 +191,15 @@ def list(self, request): @extend_schema( - description="API endpoint for entries", - responses={200: EntrySerializer}, + description="API endpoint for entries.", + responses={ + 200: OpenApiResponse(response=EntrySerializer, description="Successful retrieval of an entry/entries."), + 201: OpenApiResponse(response=EntrySerializer, description="Entry successfully created."), + 204: OpenApiResponse(description="Entry successfully deleted."), + 400: OpenApiResponse(description="The route is likely on the ignore list or the prefix is too large."), + 403: OpenApiResponse(description="The client is not authorized for this action."), + 404: OpenApiResponse(description="The requested entry does not exist."), + }, ) class EntryViewSet(viewsets.ModelViewSet): """Lookup Entry when authenticated, and bind to the serializer.""" @@ -139,18 +221,40 @@ def get_permissions(self): return super().get_permissions() def check_client_authorization(self, actiontype): - """Ensure that a given client is authorized to use a given actiontype.""" + """Ensure that a given client is authorized to use a given actiontype and IP address.""" uuid = self.request.data.get("uuid") if uuid: - authorized_actiontypes = Client.objects.filter(uuid=uuid).values_list( - "authorized_actiontypes__name", - flat=True, - ) - authorized_client = Client.objects.filter(uuid=uuid).values("is_authorized") - if not authorized_client or actiontype not in authorized_actiontypes: - logger.debug("Client: %s, actiontypes: %s", uuid, authorized_actiontypes) + try: + client = Client.objects.get(uuid=uuid) + except Client.DoesNotExist as client_dne: + msg = "Client does not exist" + raise PermissionDenied(msg) from client_dne + + # Check if client is authorized for the action type + if not client.is_authorized or actiontype not in client.authorized_actiontypes.values_list( + "name", flat=True + ): + logger.debug( + "Client: %s, actiontypes: %s", + uuid, + list(client.authorized_actiontypes.values_list("name", flat=True)), + ) logger.info("%s is not allowed to add an entry to the %s list.", uuid, actiontype) raise ActiontypeNotAllowed + + # Check if the client's IP address is allow listed + if client.registered_from_ip: + request_ip = self.get_client_ip() + if client.registered_from_ip != request_ip: + logger.warning( + "Client %s attempted to authorize from unauthorized IP %s (expected %s)", + uuid, + request_ip, + client.registered_from_ip, + ) + msg = "Request from unauthorized IP address %s" + raise PermissionDenied(msg) + elif not self.request.user.has_perm("route_manager.can_add_entry"): raise PermissionDenied diff --git a/scram/route_manager/migrations/0035_alter_client_uuid.py b/scram/route_manager/migrations/0035_alter_client_uuid.py new file mode 100644 index 00000000..39ff3bab --- /dev/null +++ b/scram/route_manager/migrations/0035_alter_client_uuid.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.26 on 2025-11-21 05:52 + +from django.db import migrations, models +import uuid + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0034_alter_entry_originating_scram_instance_and_more"), + ] + + operations = [ + migrations.AlterField( + model_name="client", + name="uuid", + field=models.UUIDField(default=uuid.uuid4, editable=False), + ), + ] diff --git a/scram/route_manager/migrations/0036_rename_hostname_client_client_name.py b/scram/route_manager/migrations/0036_rename_hostname_client_client_name.py new file mode 100644 index 00000000..1beafff8 --- /dev/null +++ b/scram/route_manager/migrations/0036_rename_hostname_client_client_name.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.26 on 2025-11-21 06:03 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0035_alter_client_uuid"), + ] + + operations = [ + migrations.RenameField( + model_name="client", + old_name="hostname", + new_name="client_name", + ), + ] diff --git a/scram/route_manager/migrations/0037_client_registered_from_ip.py b/scram/route_manager/migrations/0037_client_registered_from_ip.py new file mode 100644 index 00000000..6a734313 --- /dev/null +++ b/scram/route_manager/migrations/0037_client_registered_from_ip.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.26 on 2025-11-21 06:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0036_rename_hostname_client_client_name"), + ] + + operations = [ + migrations.AddField( + model_name="client", + name="registered_from_ip", + field=models.GenericIPAddressField(blank=True, null=True), + ), + ] diff --git a/scram/route_manager/migrations/0038_unique_client_uuid.py b/scram/route_manager/migrations/0038_unique_client_uuid.py new file mode 100644 index 00000000..7a38ec5e --- /dev/null +++ b/scram/route_manager/migrations/0038_unique_client_uuid.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.26 on 2025-11-21 06:17 + +from django.db import migrations, models +import uuid + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0037_client_registered_from_ip"), + ] + + operations = [ + migrations.AlterField( + model_name="client", + name="uuid", + field=models.UUIDField(default=uuid.uuid4, editable=False, unique=True), + ), + ] diff --git a/scram/route_manager/models.py b/scram/route_manager/models.py index 2f169cbd..3b5268ec 100644 --- a/scram/route_manager/models.py +++ b/scram/route_manager/models.py @@ -169,15 +169,16 @@ def __str__(self): class Client(models.Model): """Any client that would like to hit the API to add entries (e.g. Zeek).""" - hostname = models.CharField(max_length=50, unique=True) - uuid = models.UUIDField() + client_name = models.CharField(max_length=50, unique=True) + uuid = models.UUIDField(default=uuid_lib.uuid4, editable=False, unique=True) + registered_from_ip = models.GenericIPAddressField(null=True, blank=True) is_authorized = models.BooleanField(null=True, blank=True, default=False) authorized_actiontypes = models.ManyToManyField(ActionType) def __str__(self): - """Only display the hostname.""" - return str(self.hostname) + """Only display the client_name.""" + return str(self.client_name) channel_layer = get_channel_layer() diff --git a/scram/route_manager/tests/acceptance/features/client.feature b/scram/route_manager/tests/acceptance/features/client.feature index 493c4531..ad80e35a 100644 --- a/scram/route_manager/tests/acceptance/features/client.feature +++ b/scram/route_manager/tests/acceptance/features/client.feature @@ -1,9 +1,26 @@ Feature: We can register and use clients - Scenario: We can register a client + Scenario: We can register a client with a UUID provided When we register a client named authorized_client.es.net with the uuid of 0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3 Then we get a 201 status code + Scenario: We can register a client without a UUID provided + When we register a client named authorized_client.es.net and no UUID + Then we get a 201 status code + + Scenario: We can do an idempotent create of a client with UUID + When we register a client named authorized_client2.es.net with the uuid of 1e24e7c3-f746-401e-8ae6-3fc3ff3aa814 + Then we get a 201 status code + When we register a client named authorized_client2.es.net with the uuid of 1e24e7c3-f746-401e-8ae6-3fc3ff3aa814 + Then we get a 200 status code + + Scenario: We do not leak client names + When we register a client named authorized_client3.es.net with the uuid of 9b3b9fee-3658-4e70-a52d-662f3b2d68ab + Then we get a 201 status code + When we register a client named authorized_client3.es.net with the uuid of da6845fb-f8a6-44ec-846a-54f0ee78fcf8 + Then we get a 400 status code + + Scenario: We can add a block using an authorized client Given a client with block authorization When we're logged in diff --git a/scram/route_manager/tests/integration/environment.py b/scram/route_manager/tests/integration/environment.py index aca60e8e..6d072085 100644 --- a/scram/route_manager/tests/integration/environment.py +++ b/scram/route_manager/tests/integration/environment.py @@ -43,7 +43,7 @@ def before_feature(context, feature): client, _ = DjangoClient.objects.update_or_create( uuid=TEST_CLIENT_UUID, defaults={ - "hostname": TEST_CLIENT_HOSTNAME, + "client_name": TEST_CLIENT_HOSTNAME, "is_authorized": True, }, ) diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index dd980924..047d5f4b 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -17,7 +17,7 @@ def setUp(self): self.superuser = get_user_model().objects.create_superuser("admin", "admin@es.net", "admintestpassword") self.client.login(username="admin", password="admintestpassword") self.authorized_client = Client.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) @@ -110,7 +110,6 @@ def test_unauthenticated_users_have_no_create_access(self): "route": "192.0.2.4", "comment": "test", "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", - "who": "person", }, format="json", ) @@ -134,7 +133,7 @@ def setUp(self): """Set up test data.""" self.url = reverse("api:v1:is_active-list") self.authorized_client = Client.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) @@ -146,25 +145,41 @@ def setUp(self): # Active IPv4 route_v4 = Route.objects.create(route="192.0.2.100") Entry.objects.create( - route=route_v4, is_active=True, comment="test active", who="test", actiontype=self.actiontype + route=route_v4, + is_active=True, + comment="test active", + who="test", + actiontype=self.actiontype, ) # Active IPv6 route_v6 = Route.objects.create(route="2001:db8::1") Entry.objects.create( - route=route_v6, is_active=True, comment="test active v6", who="test", actiontype=self.actiontype + route=route_v6, + is_active=True, + comment="test active v6", + who="test", + actiontype=self.actiontype, ) # Deactivated IPv4 entry route_inactive = Route.objects.create(route="192.0.2.200") Entry.objects.create( - route=route_inactive, is_active=False, comment="inactive", who="test", actiontype=self.actiontype + route=route_inactive, + is_active=False, + comment="inactive", + who="test", + actiontype=self.actiontype, ) # Deactived IPv6 entry route_inactive = Route.objects.create(route="2001:db8::5") Entry.objects.create( - route=route_inactive, is_active=False, comment="inactive", who="test", actiontype=self.actiontype + route=route_inactive, + is_active=False, + comment="inactive", + who="test", + actiontype=self.actiontype, ) def test_active_ipv4_returns_true(self): diff --git a/scram/route_manager/tests/test_authorization.py b/scram/route_manager/tests/test_authorization.py index 596455c3..9f00dfbd 100644 --- a/scram/route_manager/tests/test_authorization.py +++ b/scram/route_manager/tests/test_authorization.py @@ -42,14 +42,14 @@ def setUp(self): ] self.authorized_client = ClientModel.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) self.authorized_client.authorized_actiontypes.set([1]) self.unauthorized_client = ClientModel.objects.create( - hostname="unauthorized_client.es.net", + client_name="unauthorized_client.es.net", uuid="91e134a5-77cf-4560-9797-6bbdbffde9f8", ) @@ -123,12 +123,18 @@ def test_unauthorized_after_group_removal(self): test_user.save() self.client.force_login(test_user) - response = self.client.post(reverse("route_manager:add"), {"route": "192.0.2.4/32", "actiontype": "block"}) + response = self.client.post( + reverse("route_manager:add"), + {"route": "192.0.2.4/32", "actiontype": "block"}, + ) self.assertEqual(response.status_code, 302) test_user.groups.set([]) - response = self.client.post(reverse("route_manager:add"), {"route": "192.0.2.5/32", "actiontype": "block"}) + response = self.client.post( + reverse("route_manager:add"), + {"route": "192.0.2.5/32", "actiontype": "block"}, + ) self.assertEqual(response.status_code, 302) diff --git a/scram/route_manager/tests/test_common/steps_common.py b/scram/route_manager/tests/test_common/steps_common.py index dea5ad50..403767a7 100644 --- a/scram/route_manager/tests/test_common/steps_common.py +++ b/scram/route_manager/tests/test_common/steps_common.py @@ -35,7 +35,7 @@ def create_authed_client(context, name): authorized_client, _ = Client.objects.update_or_create( uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", defaults={ - "hostname": "authorized_client.es.net", + "client_name": "authorized_client.es.net", "is_authorized": True, }, ) @@ -47,7 +47,7 @@ def create_authed_client(context, name): def create_unauthed_client(context, name): """Create a client that has no authorized action types.""" unauthorized_client = Client.objects.create( - hostname="unauthorized_client.es.net", + client_name="unauthorized_client.es.net", uuid="91e134a5-77cf-4560-9797-6bbdbffde9f8", ) unauthorized_client.authorized_actiontypes.set([]) @@ -211,13 +211,24 @@ def check_object(context, value, model): context.test.assertTrue(found) -@when("we register a client named {hostname} with the uuid of {uuid}") -def add_client(context, hostname, uuid): +@when("we register a client named {client_name} with the uuid of {uuid}") +def add_client(context, client_name, uuid): """Create a client with a specific UUID.""" context.response = context.test.client.post( reverse("api:v1:client-list"), { - "hostname": hostname, + "client_name": client_name, "uuid": uuid, }, ) + + +@when("we register a client named {client_name} and no UUID") +def add_client_no_uuid(context, client_name): + """Create a client without a UUID.""" + context.response = context.test.client.post( + reverse("api:v1:client-list"), + { + "client_name": client_name, + }, + ) diff --git a/scram/route_manager/tests/test_common/steps_ip.py b/scram/route_manager/tests/test_common/steps_ip.py index ad5c894e..e51e0262 100644 --- a/scram/route_manager/tests/test_common/steps_ip.py +++ b/scram/route_manager/tests/test_common/steps_ip.py @@ -54,7 +54,7 @@ def check_comment(context, value, comment): @then("we update the entry {value:S} with comment {comment}") def update_entry_comment(context, value, comment): """Update the entry with a new comment.""" - data = {"comment": comment, "who": context.client.hostname} + data = {"comment": comment, "who": context.client.client_name} context.response = context.test.client.put( reverse("api:v1:entry-detail", args=[value]), data=json.dumps(data), content_type="application/json" diff --git a/scram/route_manager/tests/test_views.py b/scram/route_manager/tests/test_views.py index 1433135a..7a59a4d0 100644 --- a/scram/route_manager/tests/test_views.py +++ b/scram/route_manager/tests/test_views.py @@ -54,5 +54,7 @@ def test_404(self): """Grab a bad URL.""" response = self.client.get("/foobarbaz") self.assertContains( - response, b'