security(key_management): stop Key Management Admin group from granting system admin

spp_key_management/README.rst

@@ -165,6 +165,20 @@ External Python dependencies: ``cryptography``
 Changelog
 =========
 
+19.0.2.0.1
+~~~~~~~~~~
+
+- fix(security): stop the Key Management Admin group from granting full
+  system administration. ``implied_ids`` grants the implied groups to
+  members, so the previous ``base.group_system`` link escalated any
+  holder of the key management role to a Settings/System administrator.
+  The admin group now implies Key Operator instead, key admins keep
+  field access to the KMS-wrapped key material the cloud providers read
+  in user context, and a migration strips the unsafe link from existing
+  databases. The Key Management menu moved from Settings to a top-level
+  menu: the Settings root is only visible to ERP managers, so key admins
+  who are no longer system administrators could not reach it there.
+
 19.0.2.0.0
 ~~~~~~~~~~
 

spp_key_management/__manifest__.py

@@ -4,7 +4,7 @@
     "name": "OpenSPP Key Management",
     "summary": "Centralized cryptographic key management with pluggable providers",
     "category": "OpenSPP/Identity",
-    "version": "19.0.2.0.0",
+    "version": "19.0.2.0.1",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "https://github.com/OpenSPP/OpenSPP2",

spp_key_management/migrations/19.0.2.0.1/post-migration.py

@@ -0,0 +1,45 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""Remove the base.group_system implication from the Key Management Admin group.
+
+Earlier versions shipped ``spp_key_management.group_key_admin`` with
+``implied_ids`` linking ``base.group_system``. ``implied_ids`` GRANTS the
+implied groups to members, so any user given the key management role
+silently became a full Settings/System administrator. Removing the link
+from the XML only stops adding it on fresh installs - it never removes an
+existing relation - so strip it here. Odoo computes effective membership
+as a transitive closure of ``implied_ids``, so removing the link
+immediately revokes the escalated privileges from affected users. (The
+replacement implication - admin implies operator - is applied by the
+regular data load; the groups file is not noupdate.)
+"""
+
+import logging
+
+from odoo import SUPERUSER_ID, Command, api
+
+_logger = logging.getLogger(__name__)
+
+
+def migrate(cr, version):
+    if not version:
+        return
+
+    env = api.Environment(cr, SUPERUSER_ID, {})
+    group = env.ref("spp_key_management.group_key_admin", raise_if_not_found=False)
+    system = env.ref("base.group_system", raise_if_not_found=False)
+    if not group or not system or system not in group.implied_ids:
+        return
+
+    # all_user_ids also counts members of any group implying key admin, so
+    # the log reflects everyone who actually held the escalation.
+    affected = len(group.all_user_ids)
+    group.write({"implied_ids": [Command.unlink(system.id)]})
+    _logger.warning(
+        "Removed the base.group_system implication from the Key Management "
+        "Admin group; %s user(s) held the group and lose the transitively "
+        "granted system administration rights. Audit changes made by these "
+        "users while escalated, grant base.group_system explicitly where it "
+        "is genuinely intended, and consider rotating data keys if key "
+        "custody policy separates platform admins from key admins.",
+        affected,
+    )

spp_key_management/models/encryption_key.py

@@ -43,7 +43,11 @@ class EncryptionKey(models.Model):
     )
     encrypted_key = fields.Char(
         required=True,
-        groups="base.group_system",
+        # Key admins need field access because the AWS KMS, Azure Key Vault
+        # and GCP KMS providers read the cached wrapped key in the calling
+        # user's context. The value is ciphertext (wrapped by the KMS);
+        # plaintext key material is never stored here.
+        groups="base.group_system,spp_key_management.group_key_admin",
         help="Base64-encoded encrypted key material",
     )
     algorithm = fields.Char(

spp_key_management/readme/HISTORY.md

@@ -1,3 +1,16 @@
+### 19.0.2.0.1
+
+- fix(security): stop the Key Management Admin group from granting full
+  system administration. `implied_ids` grants the implied groups to members,
+  so the previous `base.group_system` link escalated any holder of the key
+  management role to a Settings/System administrator. The admin group now
+  implies Key Operator instead, key admins keep field access to the
+  KMS-wrapped key material the cloud providers read in user context, and a
+  migration strips the unsafe link from existing databases. The Key
+  Management menu moved from Settings to a top-level menu: the Settings
+  root is only visible to ERP managers, so key admins who are no longer
+  system administrators could not reach it there.
+
 ### 19.0.2.0.0
 
 - Initial migration to OpenSPP2

spp_key_management/security/security_groups.xml

@@ -15,17 +15,9 @@
         <field name="sequence">20</field>
     </record>
 
-    <!-- Key Admin - Full control over key management -->
-    <record id="group_key_admin" model="res.groups">
-        <field name="name">Key Management Admin</field>
-        <field name="privilege_id" ref="privilege_key_admin" />
-        <field
-            name="comment"
-        >Full access to key management. Can configure providers, rotate keys, and view all key operations.</field>
-        <field name="implied_ids" eval="[Command.link(ref('base.group_system'))]" />
-    </record>
-
-    <!-- Key Operator - Can use keys but not manage -->
+    <!-- Key Operator - Can use keys but not manage.
+         Defined before the admin group so the admin's implied_ids ref
+         resolves on fresh install. -->
     <record id="group_key_operator_officer" model="res.groups">
         <field name="name">Key Operator</field>
         <field name="privilege_id" ref="privilege_key_operator_officer" />
@@ -34,4 +26,28 @@
         >Can use encryption keys for operations but cannot manage providers or rotate keys.</field>
         <field name="implied_ids" eval="[Command.link(ref('base.group_user'))]" />
     </record>
+
+    <!-- Key Admin - Full control over key management.
+         Must never imply base.group_system: implied_ids GRANTS the implied
+         groups to members, so that link would silently promote anyone
+         holding this key management role to a full Settings/System
+         administrator. System admins get their own explicit ACL rows on
+         the key models instead.
+
+         Deliberately NOT linked into spp_security.group_spp_admin (the
+         pattern groups_admin.xml invites domain modules to follow): key
+         custody is intentionally separated from platform administration,
+         so OpenSPP admins do not automatically gain key control. Do not
+         "fix" this to match the other domain modules. -->
+    <record id="group_key_admin" model="res.groups">
+        <field name="name">Key Management Admin</field>
+        <field name="privilege_id" ref="privilege_key_admin" />
+        <field
+            name="comment"
+        >Full access to key management. Can configure providers, rotate keys, and view all key operations.</field>
+        <field
+            name="implied_ids"
+            eval="[Command.link(ref('group_key_operator_officer'))]"
+        />
+    </record>
 </odoo>

spp_key_management/static/description/index.html

@@ -548,6 +548,21 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.1</h1>
+<ul class="simple">
+<li>fix(security): stop the Key Management Admin group from granting full
+system administration. <tt class="docutils literal">implied_ids</tt> grants the implied groups to
+members, so the previous <tt class="docutils literal">base.group_system</tt> link escalated any
+holder of the key management role to a Settings/System administrator.
+The admin group now implies Key Operator instead, key admins keep
+field access to the KMS-wrapped key material the cloud providers read
+in user context, and a migration strips the unsafe link from existing
+databases. The Key Management menu moved from Settings to a top-level
+menu: the Settings root is only visible to ERP managers, so key admins
+who are no longer system administrators could not reach it there.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_key_management/tests/__init__.py

@@ -3,3 +3,4 @@
 from . import test_key_manager
 from . import test_asymmetric_key
 from . import test_security
+from . import test_key_admin_group

spp_key_management/tests/test_key_admin_group.py

@@ -0,0 +1,162 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""Tests for the Key Management security groups."""
+
+import importlib.util
+from pathlib import Path
+
+from odoo import Command
+from odoo.exceptions import AccessError
+from odoo.tests import TransactionCase, tagged
+
+
+@tagged("post_install", "-at_install")
+class TestKeyAdminGroup(TransactionCase):
+    """Key Management Admin gates control of the platform's encryption
+    keys (providers, rotation, key records).
+
+    The group must NEVER imply base.group_system: implied_ids grants the
+    implied groups to members, so such a link would turn the key
+    management role into full Settings/System administration.
+    """
+
+    def _create_user(self, login, groups):
+        return self.env["res.users"].create(
+            {
+                "name": login,
+                "login": login,
+                "group_ids": [Command.link(g.id) for g in groups],
+            }
+        )
+
+    def test_key_admin_group_exists(self):
+        group = self.env.ref("spp_key_management.group_key_admin", raise_if_not_found=False)
+        self.assertTrue(group, "spp_key_management.group_key_admin must exist")
+        self.assertEqual(group.name, "Key Management Admin")
+
+    def test_key_admin_does_not_grant_system(self):
+        """Granting Key Management Admin must not escalate to system admin."""
+        group = self.env.ref("spp_key_management.group_key_admin")
+        system = self.env.ref("base.group_system")
+        self.assertNotIn(system, group.implied_ids)
+        # Guard the transitive closure too, so the escalation cannot come
+        # back through an intermediate group.
+        self.assertNotIn(system, group.all_implied_ids)
+
+        user = self._create_user(
+            "key_admin_only",
+            [self.env.ref("base.group_user"), group],
+        )
+        self.assertTrue(user.has_group("spp_key_management.group_key_admin"))
+        self.assertFalse(
+            user.has_group("base.group_system"),
+            "Key Management Admin membership must not confer system administration",
+        )
+
+    def test_key_admin_implies_operator(self):
+        """Admin is a superset of operator (and stays an internal user)."""
+        group = self.env.ref("spp_key_management.group_key_admin")
+        operator = self.env.ref("spp_key_management.group_key_operator_officer")
+        internal = self.env.ref("base.group_user")
+        self.assertIn(operator, group.all_implied_ids)
+        self.assertIn(internal, group.all_implied_ids)
+
+    def test_key_admin_reads_encrypted_key(self):
+        """Key admins keep field access to the KMS-wrapped key material
+        (the AWS/Azure providers read it in the calling user's context);
+        operators do not."""
+        key = self.env["spp.encryption.key"].create(
+            {
+                "key_id": "test-field-gate",
+                "encrypted_key": "d3JhcHBlZA==",
+            }
+        )
+        admin_user = self._create_user(
+            "key_admin_field",
+            [
+                self.env.ref("base.group_user"),
+                self.env.ref("spp_key_management.group_key_admin"),
+            ],
+        )
+        operator_user = self._create_user(
+            "key_operator_field",
+            [
+                self.env.ref("base.group_user"),
+                self.env.ref("spp_key_management.group_key_operator_officer"),
+            ],
+        )
+        self.assertEqual(key.with_user(admin_user).encrypted_key, "d3JhcHBlZA==")
+        with self.assertRaises(AccessError):
+            key.with_user(operator_user).encrypted_key  # noqa: B018
+
+    def test_system_admin_retains_key_access(self):
+        """System administrators keep key access without holding the key
+        admin group: the field gate and menu root both list
+        base.group_system, and the ACL grants system its own rows. This
+        pins the other half of the design - removing the escalation must
+        not cost real system admins anything."""
+        key = self.env["spp.encryption.key"].create(
+            {
+                "key_id": "test-system-access",
+                "encrypted_key": "d3JhcHBlZA==",
+            }
+        )
+        system_user = self._create_user(
+            "key_system_admin",
+            [self.env.ref("base.group_system")],
+        )
+        # Access is retained via base.group_system, NOT via the key group.
+        self.assertFalse(system_user.has_group("spp_key_management.group_key_admin"))
+        self.assertEqual(key.with_user(system_user).encrypted_key, "d3JhcHBlZA==")
+        root = self.env.ref("spp_key_management.menu_key_management_root")
+        menus = self.env["ir.ui.menu"].with_user(system_user).load_menus(False)
+        self.assertIn(root.id, menus)
+
+    def test_key_admin_sees_key_management_menu(self):
+        """A user holding only the key admin role must be able to reach the
+        Key Management menu. Odoo drops menus whose ancestors are invisible,
+        so parenting under Settings (base.group_erp_manager) would hide the
+        whole subtree from dedicated key-custody users."""
+        user = self._create_user(
+            "key_admin_menu",
+            [
+                self.env.ref("base.group_user"),
+                self.env.ref("spp_key_management.group_key_admin"),
+            ],
+        )
+        root = self.env.ref("spp_key_management.menu_key_management_root")
+        # load_menus is what the web client uses; unlike _visible_menu_ids
+        # it also prunes menus whose ancestors are invisible ("not related
+        # to an app"), which is exactly the failure mode being pinned.
+        menus = self.env["ir.ui.menu"].with_user(user).load_menus(False)
+        self.assertIn(
+            root.id,
+            menus,
+            "Key Management menu must be reachable for key admins",
+        )
+
+    def test_migration_removes_escalation(self):
+        """The 19.0.2.0.1 migration strips base.group_system from the
+        group on databases installed before the fix (removing the XML
+        line alone never unlinks an existing relation)."""
+        migration_path = Path(__file__).parent.parent / "migrations" / "19.0.2.0.1" / "post-migration.py"
+        spec = importlib.util.spec_from_file_location("spp_key_management_migration_19_0_2_0_1", migration_path)
+        migration = importlib.util.module_from_spec(spec)
+        spec.loader.exec_module(migration)
+
+        group = self.env.ref("spp_key_management.group_key_admin")
+        system = self.env.ref("base.group_system")
+        # Recreate the released (vulnerable) state.
+        group.write({"implied_ids": [Command.link(system.id)]})
+        self.assertIn(system, group.implied_ids)
+
+        with self.assertLogs("spp_key_management_migration_19_0_2_0_1", level="WARNING") as capture:
+            migration.migrate(self.env.cr, "19.0.2.0.0")
+        self.assertIn("Removed the base.group_system implication", capture.output[0])
+
+        group.invalidate_recordset()
+        self.assertNotIn(system, group.implied_ids)
+        self.assertNotIn(system, group.all_implied_ids)
+
+        # Idempotent: a second run finds the link absent and stays silent.
+        with self.assertNoLogs("spp_key_management_migration_19_0_2_0_1", level="WARNING"):
+            migration.migrate(self.env.cr, "19.0.2.0.0")

spp_key_management/views/menu.xml

@@ -1,10 +1,14 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <odoo>
-    <!-- Main Menu: Key Management -->
+    <!-- Main Menu: Key Management.
+         Top-level on purpose: the Settings root (base.menu_administration)
+         is only visible to base.group_erp_manager, and Odoo prunes menus
+         whose ancestors are invisible - parenting under Settings would
+         hide Key Management from dedicated key admins who are not system
+         administrators. -->
     <menuitem
         id="menu_key_management_root"
         name="Key Management"
-        parent="base.menu_administration"
         sequence="760"
         groups="group_key_admin,base.group_system"
     />