From a5284a198a79672b7f2f9a3da467b350039f4aeb Mon Sep 17 00:00:00 2001 From: vishalan Date: Thu, 12 Nov 2026 15:51:29 +0530 Subject: [PATCH 1/4] fix(security): harden dashboard race, password history, CID, CI supply chain (CTO-4878/4881/4882/4884/4885/4886/4889) - F-041 get_dashboard_data: DB membership check + row lock in one transaction - F-033 mark_revoked: return real status (200/400/500) instead of always 403 - F-034 RepeatedValidator: [:3] slice; password_changed stops storing duplicates - F-036 CID_GENERATOR: uuid4 instead of random.random() - F-037 CI: drop --ignore-installed for access-module requirements - F-038 CI: remove pre-commit autoupdate, pin hook revs to commit SHAs - F-030 Dockerfile: drop vim and curl from the image Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/pre-commit.yml | 1 - .github/workflows/unit-tests.yml | 2 +- .pre-commit-config-gh-action.yaml | 4 +- Access/tests/test_security_batch3.py | 96 ++++++++++++ Access/validators.py | 21 ++- Access/views.py | 5 +- Dockerfile | 2 +- EnigmaAutomation/settings.py | 5 +- bootprocess/tests/test_views_helper.py | 208 ++++++++----------------- bootprocess/views_helper.py | 35 +++-- 10 files changed, 203 insertions(+), 176 deletions(-) create mode 100644 Access/tests/test_security_batch3.py diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index b9ced062..95c966bf 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -13,5 +13,4 @@ jobs: run: | pip install pre-commit==2.21.0 pre-commit install --install-hooks --overwrite --config .pre-commit-config-gh-action.yaml - pre-commit autoupdate --config .pre-commit-config-gh-action.yaml pre-commit run --all-files --show-diff-on-failure --color always --config .pre-commit-config-gh-action.yaml diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index d3e1920c..971631d2 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -26,7 +26,7 @@ jobs: run: python -m scripts.clone_access_modules - name: Ensure access modules dependencies run: | - pip install -r Access/access_modules/requirements.txt --no-cache-dir --ignore-installed + pip install -r Access/access_modules/requirements.txt --no-cache-dir - name: Lint code base run: | pip install "setuptools<81" diff --git a/.pre-commit-config-gh-action.yaml b/.pre-commit-config-gh-action.yaml index 975c9a3d..fdb3cab7 100644 --- a/.pre-commit-config-gh-action.yaml +++ b/.pre-commit-config-gh-action.yaml @@ -2,7 +2,7 @@ # See https://pre-commit.com/hooks.html for more hooks repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.4.0 + rev: f71fa2c1f9cf5cb705f73dffe4b21f7c61470ba9 # v4.4.0 hooks: - id: check-merge-conflict - id: trailing-whitespace @@ -19,7 +19,7 @@ repos: files: \.(json|json.sample)$ args: [--autofix, --no-sort-keys] - repo: https://github.com/macisamuele/language-formatters-pre-commit-hooks - rev: v2.6.0 + rev: 4a72877d96875c49769109cf7881b44b8ad4e423 # v2.6.0 hooks: - id: pretty-format-yaml args: [--autofix] diff --git a/Access/tests/test_security_batch3.py b/Access/tests/test_security_batch3.py new file mode 100644 index 00000000..ccbdfd44 --- /dev/null +++ b/Access/tests/test_security_batch3.py @@ -0,0 +1,96 @@ +""" regression tests: F-033 mark_revoked status, F-034 password history, F-036 CID """ +import re +from types import SimpleNamespace +from unittest import mock +import pytest +from django.contrib.auth.models import User as AuthUser +from django.contrib.auth.hashers import make_password +from django.test import RequestFactory +from Access import views +from Access.models import StoredPassword +from Access.validators import RepeatedValidator +from EnigmaAutomation import settings + + +def _ops_request(query): + request = RequestFactory().get("/access/markRevoked", query) + perm = SimpleNamespace(label="ALLOW_USER_OFFBOARD") + request.user = mock.MagicMock() + request.user.is_authenticated = True + request.user.user.permissions = [perm] + return request + + +def test_mark_revoked_success_returns_200(): + """ F-033: success must not be reported as 403 """ + mapping = mock.MagicMock(request_id="r1") + with mock.patch.object( + views.UserAccessMapping, "get_unrevoked_accesses_by_request_id", + return_value=[mapping]): + response = views.mark_revoked(_ops_request({"requestId": "r1"})) + assert response.status_code == 200 + mapping.revoke.assert_called_once() + + +def test_mark_revoked_failure_returns_500(): + """ an exception during revoke is an error status, not 200 """ + with mock.patch.object( + views.UserAccessMapping, "get_unrevoked_accesses_by_request_id", + side_effect=RuntimeError("boom")): + response = views.mark_revoked(_ops_request({"requestId": "r1"})) + assert response.status_code == 500 + + +def test_mark_revoked_module_without_username_is_400(): + """ missing username on a module revoke is a client error """ + response = views.mark_revoked(_ops_request({"requestId": "module-ssh"})) + assert response.status_code == 400 + + +@pytest.fixture(name="auth_user") +def fixture_auth_user(db): + """ django auth user """ + del db + return AuthUser.objects.create(username="pwuser") + + +def _store(user, *passwords): + for pw in passwords: + StoredPassword.objects.create( + user=user, password=make_password(pw, hasher="pbkdf2_sha256")) + + +@pytest.mark.parametrize("history", [1, 3, 4]) +def test_recent_password_is_rejected_even_with_short_history(auth_user, history): + """ F-034: [:3:-1] skipped the check when <=4 passwords were stored """ + _store(auth_user, *[f"Old-pass-{i}!" for i in range(history)]) + from django.core.exceptions import ValidationError + with pytest.raises(ValidationError): + RepeatedValidator().validate(f"Old-pass-{history - 1}!", auth_user) + + +def test_new_password_is_accepted(auth_user): + """ a password not in the history passes """ + _store(auth_user, "Old-pass-0!") + assert RepeatedValidator().validate("Brand-new-1!", auth_user) is None + + +def test_password_older_than_three_is_allowed(auth_user): + """ only the last 3 are blocked """ + _store(auth_user, "p-a-1!", "p-b-2!", "p-c-3!", "p-d-4!") + assert RepeatedValidator().validate("p-a-1!", auth_user) is None + + +def test_password_changed_does_not_store_duplicates(auth_user): + """ the same password saved twice must produce one row """ + validator = RepeatedValidator() + validator.password_changed("Same-pass-1!", auth_user) + validator.password_changed("Same-pass-1!", auth_user) + assert StoredPassword.objects.filter(user=auth_user).count() == 1 + + +def test_cid_generator_is_unpredictable_and_unique(): + """ F-036: CID comes from uuid4 (os.urandom), not random.random() """ + ids = {settings.CID_GENERATOR() for _ in range(200)} + assert len(ids) == 200 + assert all(re.fullmatch(r"[0-9a-f]{32}", i) for i in ids) diff --git a/Access/validators.py b/Access/validators.py index 40a22671..b5e12878 100644 --- a/Access/validators.py +++ b/Access/validators.py @@ -53,7 +53,7 @@ def validate(self, password, user=None): if user is None: return None - user_list = StoredPassword.objects.filter(user=user).order_by('-id')[:3:-1] + user_list = StoredPassword.objects.filter(user=user).order_by('-id')[:3] password_list = [] for user in user_list: password_list.append(user.password) @@ -70,13 +70,18 @@ def password_changed(self, password, user=None): if user is None: return None - hashed_password = make_password(password, salt=None, hasher='pbkdf2_sha256') - saved_password = StoredPassword.objects.filter(user=user, password=hashed_password).first() - if saved_password is None: - saved_password = StoredPassword() - saved_password.user = user - saved_password.password = hashed_password - saved_password.save() + # make_password salts randomly, so the hash can never be matched by a + # lookup; compare against the most recent stored hash instead. + latest = StoredPassword.objects.filter(user=user).order_by('-id').first() + if latest is not None and check_password( + password, latest.password, setter=None, + preferred='pbkdf2_sha256'): + return None + saved_password = StoredPassword() + saved_password.user = user + saved_password.password = make_password( + password, salt=None, hasher='pbkdf2_sha256') + saved_password.save() def get_help_text(self): return _( diff --git a/Access/views.py b/Access/views.py index 007990bf..c54ac8c1 100644 --- a/Access/views.py +++ b/Access/views.py @@ -608,7 +608,7 @@ def mark_revoked(request): username = request.GET.get("username") if not username: json_response["error"] = "Username is invalid!" - status = 403 + status = 400 return JsonResponse(json_response, status=status) access_tag = request_id.split("-", 1)[1] user = User.get_user_from_username(username=username) @@ -636,7 +636,8 @@ def mark_revoked(request): except Exception as ex: logger.exception("Error Revoking User Access, Error: %s", traceback.format_exc()) json_response["error"] = "Error Revoking User Access" - return JsonResponse(json_response, status=403) + status = 500 + return JsonResponse(json_response, status=status) @login_required diff --git a/Dockerfile b/Dockerfile index 51134662..c6eafa0b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -9,7 +9,7 @@ ENV DJANGO_SETTINGS_MODULE=EnigmaAutomation.settings RUN DEBIAN_FRONTEND=noninteractive \ apt-get update -y \ && apt-get install --no-install-recommends -y \ - openssh-client curl procps git vim gcc linux-libc-dev libc6-dev build-essential \ + openssh-client procps git gcc linux-libc-dev libc6-dev build-essential \ && apt-get clean \ && apt-get autoremove -y diff --git a/EnigmaAutomation/settings.py b/EnigmaAutomation/settings.py index 5a0d5ef3..5fceb946 100644 --- a/EnigmaAutomation/settings.py +++ b/EnigmaAutomation/settings.py @@ -15,8 +15,7 @@ import json from pathlib import Path import os -import time -import random +import uuid import django from django.utils.translation import gettext django.utils.translation.ugettext = gettext @@ -90,7 +89,7 @@ "axes", ] CID_GENERATE = True -CID_GENERATOR = lambda: f"{time.time()}-{random.random()}" +CID_GENERATOR = lambda: uuid.uuid4().hex CID_HEADER = "X_CORRELATION_ID" CID_GENERATE = True CID_CONCATENATE_IDS = True diff --git a/bootprocess/tests/test_views_helper.py b/bootprocess/tests/test_views_helper.py index 35b74313..2eab62f9 100644 --- a/bootprocess/tests/test_views_helper.py +++ b/bootprocess/tests/test_views_helper.py @@ -1,150 +1,68 @@ -""" test file for views_helper """ -import threading +""" tests for bootprocess.views_helper (real database, no model mocking) """ import pytest -from bootprocess import views_helper +from django.contrib.auth.models import User as AuthUser from Access import models +from bootprocess import views_helper from EnigmaAutomation.settings import DEFAULT_ACCESS_GROUP -class MockAuthUser: - """ mocking AuthUser """ - def __init__(self, username="", user=""): - self.user = user - self.username = username - - -class MockRequest: - """ mocking Request """ - def __init__(self, username=""): - self.user = MockAuthUser(username) - - -@pytest.mark.parametrize( - ( - "_test_name, user_is_in_default_access_group," - " group_count" - ), - [ - # user is not part of default group and has respective count of git repo, - # dashboard, ssh machines and group accesses - ("UserInDefaultGroup", True, 40), - ("UserInDefaultGroup", False, 40), - ], -) -def test_get_dashboard_data(monkeypatch, _test_name, user_is_in_default_access_group, group_count): - """ method to test get_dashboard_data """ - class MockUserModelobj: - """ mocking method UserModelobj """ - def __init__(self, user="", gitusername="", name=""): - self.user = user - self.gitusername = gitusername - self.name = name - - def get(self, user__username=""): - """ mocked method get """ - self.user = user__username - return MockUserModelobj(user="username", gitusername="username") - - class MockGitAccessModelobj: - """ mocking GitAccessModelobj """ - def __init__(self, request_info=None): - del request_info - self.user = "" - - def filter(self, requester="", status=""): - """ mocked method filter """ - del requester - if status == "Approved": - return [MockGitAccessModelobj()] - return MockGitAccessModelobj() - - class MockGroupV2: - """ mocking class GroupV2 """ - def filter(self, name="", status=""): - """ mocked method filter """ - del name - if status == "Approved": - return [MockGroupV2()] - return MockGitAccessModelobj() - - class MockUserAccessMapping: - """ mocking UserAccessMapping """ - def filter(self, user="", status="", access__access_tag=""): - """ mocked method filter """ - del user, status - if access__access_tag == "other": - return [] - if access__access_tag == "github_access": - return [] - if access__access_tag == "ssh": - return [] - group = [] - for i in range(group_count): - group.append(i) - return group - - class Group: - """ class Group """ - def __init__(self): - self.name = "" - - class MockMembershipV2: - """ mocking class MembershipV2 """ - def __init__(self, is_owner=False, approver="", status="", user=""): - self.is_owner = is_owner - self.approver = approver - self.status = status - self.user = user - self.group = Group() - - def filter(self, user="", status="", group="", is_owner=False): - """ mocked method filter """ - del user, status, group - if is_owner: - return [MockMembershipV2()] - return MockMembershipV2() - - def only(self, _filter): - """ mocked method only """ - return MockMembershipV2() - - def create(self, *_, **__): - """ mocked method create """ - return MockMembershipV2() - - def save(self): - """ mocked method save """ - return "" - - def __str__(self): - if user_is_in_default_access_group: - return DEFAULT_ACCESS_GROUP - return "" - - def __len__(self): - return group_count - - class MockThread: - """ class for mocking Thread """ - def start(self): - """ method to mock start method of Thread """ - return True - - def mockgenerate_user_mappings(*_, **__): - return [] - - views_helper.generate_user_mappings = mockgenerate_user_mappings - - def mock_thread(*_, **__): - return MockThread() - - monkeypatch.setattr(threading, "Thread", mock_thread) - - models.MembershipV2.objects = MockMembershipV2() - models.User.objects = MockUserModelobj() - models.UserAccessMapping.objects = MockUserAccessMapping() - models.GroupV2.objects = MockGroupV2() - request = MockRequest(username="username1") - context = views_helper.get_dashboard_data(request) - assert context["regions"] == ["eu-central-1"] - assert context["groupCount"] == group_count +class _Request: + """ minimal request carrying the auth user """ + def __init__(self, auth_user): + self.user = auth_user + + +@pytest.fixture(name="env") +def fixture_env(monkeypatch): + """ default group with an owner, a new joiner, and a stubbed access grant """ + owner_auth = AuthUser.objects.create(username="owner") + owner = models.User.objects.get_or_create(user=owner_auth, defaults={"name": "owner"})[0] + group = models.GroupV2.objects.create( + group_id="g1", name=DEFAULT_ACCESS_GROUP, description="d", + requester=owner, status="Approved", approver=owner) + models.MembershipV2.objects.create( + membership_id="m-owner", user=owner, group=group, is_owner=True, + requested_by=owner, status="Approved") + joiner_auth = AuthUser.objects.create(username="joiner") + joiner = models.User.objects.get_or_create(user=joiner_auth, defaults={"name": "joiner"})[0] + + granted = [] + monkeypatch.setattr(views_helper, "generate_user_mappings", lambda *_: ["m"]) + monkeypatch.setattr(views_helper, "execute_group_access", granted.append) + return {"auth": joiner_auth, "user": joiner, "group": group, "granted": granted} + + +def _count(env): + return models.MembershipV2.objects.filter( + user=env["user"], group=env["group"]).count() + + +@pytest.mark.django_db +def test_new_user_is_added_to_default_group_once(env): + """ first dashboard load adds the membership and grants access """ + context = views_helper.get_dashboard_data(_Request(env["auth"])) + assert _count(env) == 1 + assert len(env["granted"]) == 1 + assert context["groupCount"] == 1 + + +@pytest.mark.django_db +def test_repeated_dashboard_loads_do_not_duplicate_membership(env): + """ F-041: a second load must not create a second membership or grant """ + views_helper.get_dashboard_data(_Request(env["auth"])) + views_helper.get_dashboard_data(_Request(env["auth"])) + assert _count(env) == 1 + assert len(env["granted"]) == 1 + + +@pytest.mark.django_db +def test_membership_check_is_not_string_based(env): + """ a lookalike group name must not count as being in the default group """ + other = models.GroupV2.objects.create( + group_id="g2", name=DEFAULT_ACCESS_GROUP + "-extra", description="d", + requester=env["user"], status="Approved", approver=env["user"]) + models.MembershipV2.objects.create( + membership_id="m-other", user=env["user"], group=other, + requested_by=env["user"], status="Approved") + views_helper.get_dashboard_data(_Request(env["auth"])) + assert _count(env) == 1 diff --git a/bootprocess/views_helper.py b/bootprocess/views_helper.py index 9676e817..7fa27843 100644 --- a/bootprocess/views_helper.py +++ b/bootprocess/views_helper.py @@ -2,6 +2,7 @@ import logging import json import datetime +from django.db import transaction from Access.models import ( User as access_user, MembershipV2, @@ -18,14 +19,20 @@ def get_dashboard_data(request): user = access_user.objects.get(user__username=request.user) # Add users to DEFAULT_ACCESS_GROUP if the user is not already on the group - user_membership = str( - MembershipV2.objects.filter(user=user).filter(status="Approved") - ) - if DEFAULT_ACCESS_GROUP not in user_membership: + # The check and create run in one transaction holding a row lock on the + # user, so concurrent dashboard loads cannot both add the membership. + user_mappings_list = None + with transaction.atomic(): + access_user.objects.select_for_update().get(pk=user.pk) + already_member = ( + MembershipV2.objects.filter(user=user, status="Approved") + .filter(group__name=DEFAULT_ACCESS_GROUP) + .exists() + ) group_objects = GroupV2.objects.filter(name=DEFAULT_ACCESS_GROUP).filter( status="Approved" ) - if len(group_objects) > 0: + if not already_member and len(group_objects) > 0: group = group_objects[0] group_members = ( MembershipV2.objects.filter(group=group) @@ -53,14 +60,16 @@ def get_dashboard_data(request): user_mappings_list = generate_user_mappings(user, group, member) member.save() - execute_group_access(user_mappings_list) - - logger.debug( - "Process has been started for the Approval of request - " - + membership_id - + " - Approver=" - + request.user.username - ) + # Grant access outside the transaction so the row lock is not held while + # the (slow, external) access modules run. + if user_mappings_list is not None: + execute_group_access(user_mappings_list) + logger.debug( + "Process has been started for the Approval of request - " + + membership_id + + " - Approver=" + + request.user.username + ) with open("instanceTypes.json", encoding="utf-8") as data_file: data = json.load(data_file) From 45338514e39437918b78ed44d20585af802bffc0 Mon Sep 17 00:00:00 2001 From: vishalan Date: Thu, 12 Nov 2026 15:53:40 +0530 Subject: [PATCH 2/4] revert(ci): keep --ignore-installed; removing it breaks six.moves on 3.12+ (CTO-4885 needs a proper fix) Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/unit-tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 971631d2..d3e1920c 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -26,7 +26,7 @@ jobs: run: python -m scripts.clone_access_modules - name: Ensure access modules dependencies run: | - pip install -r Access/access_modules/requirements.txt --no-cache-dir + pip install -r Access/access_modules/requirements.txt --no-cache-dir --ignore-installed - name: Lint code base run: | pip install "setuptools<81" From d937aa8ac6f61c2b222e1fb0717f1966cb766237 Mon Sep 17 00:00:00 2001 From: vishalan Date: Thu, 12 Nov 2026 15:56:10 +0530 Subject: [PATCH 3/4] fix(ci): constrain access-module requirements to core pins instead of --ignore-installed (F-037) Module requirements can no longer downgrade or override security-pinned core packages. Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/unit-tests.yml | 2 +- docker-entrypoint.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index d3e1920c..b678dc1e 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -26,7 +26,7 @@ jobs: run: python -m scripts.clone_access_modules - name: Ensure access modules dependencies run: | - pip install -r Access/access_modules/requirements.txt --no-cache-dir --ignore-installed + pip install -r Access/access_modules/requirements.txt -c requirements.txt --no-cache-dir - name: Lint code base run: | pip install "setuptools<81" diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index ee3794fb..a1e943e3 100755 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -30,7 +30,7 @@ log "===== Cloning Access Modules =====" python -m scripts.clone_access_modules 2>&1 | prepend log "===== Install requirements for access modules =====" -pip install -r Access/access_modules/requirements.txt --no-cache-dir --ignore-installed 2>&1 | prepend +pip install -r Access/access_modules/requirements.txt -c requirements.txt --no-cache-dir 2>&1 | prepend log "===== Ensure DB State =====" python manage.py createcachetable 2>&1 | prepend From b2bd14e31a61a79332a71a6d9652a8c7b7c5b694 Mon Sep 17 00:00:00 2001 From: vishalan Date: Thu, 12 Nov 2026 15:58:20 +0530 Subject: [PATCH 4/4] fix(deps): six 1.15.0 -> 1.17.0 (1.15 lacks six.moves on Python 3.12+) Co-Authored-By: Claude Sonnet 5.5 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 326d690e..4724090e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -77,7 +77,7 @@ requests-oauthlib==2.0.0 rsa==4.7 scandir==1.10.0 simplegeneric==0.8.1 -six==1.15.0 +six==1.17.0 social-auth-app-django==5.4.1 social-auth-core==5.0.2 soupsieve==2.8.4