fix: address Copilot review comments on PR #991

- dispatch: make check_role_permission async; remove dispatch-level blocking
  for KmipOperation-mapped ops (handlers enforce crypto_officer_users + explicit
  grants); LIFECYCLE_OPERATION_TAGS now allow through if user has explicit Create
  grant in DB -- fixes regression in test_crypto_officer_users
- access: role_for() only returns Administrator when require_ceremony=false;
  ceremony candidates are not elevated at dispatch before ceremony completes
- retrieve_object_utils: Administrator bypass limited to non-HSM objects
- join_split_key: add split_key_method validation; add crypto_officer check
  before database.create(); fix doc comment wording
- database_objects: find_all() propagates errors instead of swallowing
- locate_query: rewrite query_all_from_attributes with full attribute filters
- permissions_store: update revoke_administrator_activation doc contract
- routes/access: fix config key reference in error msg (administrator_users)
- documentation: replace Super-admin section with correct Administrator role docs
- remove dead super_admin_config.rs file
- CHANGELOG: fix log names and claims about rename completion

Author: Manuthor

CHANGELOG/feat_split_key.md

@@ -7,8 +7,9 @@
   (Shamir 1979, Blakley 1979), normative references (NIST SP 800-57 §4.3.2–§4.3.3, FIPS 140-3 §7.4,
   ANSI/INCITS 359-2004 §4.3.1), role operation tables, 4-phase Mermaid sequence diagrams, and
   permission evaluation flowchart. Registered in `documentation/mkdocs.yml`.
-- `documentation/docs/configuration/authorization.md`: added **Super-admin role** section
-  covering configuration, activation modes, ceremony flow, disable procedures, REST endpoints,
+- `documentation/docs/configuration/authorization.md`: replaced stale **Super-admin role** section
+  with correct **Administrator role** section covering `administrator_*` config keys, activation
+  modes, ceremony flow, disable procedures, actual REST endpoints (`/access/administrator/*`),
   and updated permission evaluation order table.
 
 ## Refactor
@@ -19,21 +20,25 @@
   ownership bypass (access any object without grant) in one coherent concept.
   References: NIST SP 800-57 Part 2 Rev 1 §4.3.2 (Dual Control), ANSI/INCITS 359-2004 §4.2.
 - **Renamed `super_admin` → `administrator` across the entire stack**: DB trait methods,
-  route paths, ceremony attributes, config fields, test infra, SQL table (`super_admin_activations`
-  → `administrator_activations`), SQL query names, test vector directories, and config files.
-  No remaining `super_admin`/`SuperAdmin`/`super-admin` references in `crate/` or `test_data/`.
+  route paths, ceremony attributes, config fields, test infra, SQL table
+  (`administrator_activations`), SQL query names, test vector directories, and config files.
+  Removed residual `SuperAdminConfig` dead code (`super_admin_config.rs`).
 
 ## Security
 
-- Super-admin access is audit-logged at `WARN` level (`SUPER_ADMIN_ACCESS`, `SUPER_ADMIN_DISABLED`).
+- Administrator access is audit-logged at `WARN` level (`ADMINISTRATOR_ACCESS`, `ADMINISTRATOR_DISABLED`).
 - Ceremony activation record stores a SHA-256 fingerprint of the reconstructed secret.
 - Config-only mode provides always-on access; ceremony mode provides defence-in-depth.
 - **Fail-secure unenrolled users**: when role enforcement is active, users not listed in any
   role now default to Operator (minimum privilege) instead of being unrestricted.
   Per NIST SP 800-57 Part 2 §4.4.2 (need-to-know / minimum necessary).
-- **Ceremony initiator check**: `JoinSplitKey` now verifies that all share owners are in
-  `administrator.users` before activating the Administrator ceremony. Prevents non-Admins
-  from bootstrapping a ceremony.
+- **Ceremony initiator check**: `JoinSplitKey` verifies the **assembling user** is in
+  `administrator_users` before activating the Administrator ceremony.
+- **Administrator bypass excludes HSM-backed keys**: HSM objects continue to use their own
+  admin rules; the Administrator bypass applies only to non-HSM Managed Objects.
+- **`role_for()` ceremony guard**: when `administrator_require_ceremony = true`, ceremony
+  candidates are not granted Administrator privileges at dispatch time until the DB-backed
+  `is_administrator()` check confirms the ceremony has been completed.
 - **Role separation validation**: `RoleConfig::validate()` rejects configs where a user
   appears in multiple role lists (FIPS 140-3 §7.4 separation of duty).
 

crate/access/src/access.rs

@@ -231,9 +231,19 @@ impl RoleConfig {
     /// Returns `None` when the user is not listed in any role **and** role enforcement
     /// is not active. When role enforcement is active, callers must treat `None` as
     /// [`Role::Operator`] (fail-secure default).
+    ///
+    /// **Note**: when `administrator.require_ceremony = true`, users in `administrator.users`
+    /// are candidates but are NOT returned as [`Role::Administrator`] here — they default to
+    /// [`Role::Operator`] until the ceremony activates. The async [`KMS::is_administrator()`]
+    /// method performs the DB-backed activation check for ownership bypass.
     #[must_use]
     pub fn role_for(&self, user: &str) -> Option<Role> {
-        if self.administrator.users.iter().any(|x| x == user) {
+        // Only grant Administrator at the dispatch level when ceremony is NOT required
+        // (config-only mode). When ceremony is required, the role is dormant until the
+        // DB-backed is_administrator() check confirms activation.
+        if !self.administrator.require_ceremony
+            && self.administrator.users.iter().any(|x| x == user)
+        {
             return Some(Role::Administrator);
         }
         if self

crate/interfaces/src/stores/permissions_store.rs

@@ -73,6 +73,7 @@ pub trait PermissionsStore {
     async fn is_administrator_activated(&self) -> InterfaceResult<bool>;
 
     /// Revoke the active ceremony record (set `revoked_at` to now).
-    /// Returns an error if no active record exists.
+    /// No-op if no active record exists (the SQL implementations do not check
+    /// affected row count — this is intentional for idempotent disable calls).
     async fn revoke_administrator_activation(&self, revoked_by: &str) -> InterfaceResult<()>;
 }

crate/server/src/config/command_line/super_admin_config.rs

@@ -21,6 +21,7 @@ use crate::{
             algorithm_policy::enforce_kmip_algorithm_policy_for_operation,
             attributes::get_attribute_list, check, mac::mac_verify, query::query as query_op,
         },
+        retrieve_object_utils::user_has_permission,
     },
     error::KmsError,
     kms_bail,
@@ -83,7 +84,8 @@ macro_rules! op {
 /// Map a TTLV operation tag string to a [`KmipOperation`] variant for role-based access control.
 ///
 /// Operations not present in the [`KmipOperation`] enum (e.g. `CreateKeyPair`, `CreateSplitKey`,
-/// `JoinSplitKey`, `Register`, `ReKeyKeyPair`) return `None` and are **not** role-gated.
+/// `JoinSplitKey`, `Register`, `ReKeyKeyPair`) return `None` here but may still be
+/// gated via [`LIFECYCLE_OPERATION_TAGS`].
 fn operation_tag_to_kmip_operation(tag: &str) -> Option<KmipOperation> {
     match tag {
         "Activate" => Some(KmipOperation::Activate),
@@ -113,17 +115,49 @@ fn operation_tag_to_kmip_operation(tag: &str) -> Option<KmipOperation> {
     }
 }
 
+/// Lifecycle operation tags that have no [`KmipOperation`] variant but must be restricted
+/// to `CryptoOfficer` (or Administrator) when role enforcement is active.
+///
+/// `CreateKeyPair`, `Register`, and `ReKeyKeyPair` create or replace Managed Objects and
+/// are therefore lifecycle operations equivalent to `Create`/`Import`/`Rekey`.
+/// `CreateSplitKey` produces new `SplitKey` share objects and is likewise lifecycle-scoped.
+///
+/// `JoinSplitKey` is intentionally omitted: it is needed by Administrator candidates (users
+/// in `administrator.users` with `require_ceremony = true`) to complete the split-key
+/// ceremony before they hold an active Administrator role.
+const LIFECYCLE_OPERATION_TAGS: &[&str] = &[
+    "CreateKeyPair",
+    "Register",
+    "ReKeyKeyPair",
+    "CreateSplitKey",
+];
+
 /// Enforce role-based access control before dispatching a KMIP operation.
 ///
-/// When role lists are configured on the server, a user assigned to `Operator` or `CryptoOfficer`
-/// can only call the operations permitted by their role's [`Role::allowed_operations`] set.
-/// `Administrator` users are always allowed through. Users not listed in any role default to
-/// `Operator` when role enforcement is active (fail-secure per NIST SP 800-57 Part 2 §4.4.2).
+/// ## Design
+///
+/// Operations with a [`KmipOperation`] variant (Create, Import, Certify, Rekey, Destroy, etc.)
+/// are **not** blocked at dispatch level — their individual handlers already enforce
+/// `crypto_officer_users` restrictions while respecting explicit per-user grants. Blocking them
+/// here would prevent a `CryptoOfficer` from delegating `Create` permission to an `Operator`.
+///
+/// The dispatch check only gates lifecycle operations that have **no** [`KmipOperation`] mapping
+/// (i.e. entries in [`LIFECYCLE_OPERATION_TAGS`]). For those, if the user is `Operator` and has
+/// no explicit `Create` permission in the database, the request is denied.
+///
+/// `Administrator` users are always allowed through.
+/// Users not listed in any role default to `Operator` when role enforcement is active
+/// (fail-secure per NIST SP 800-57 Part 2 §4.4.2).
 ///
 /// # Errors
 /// Returns [`KmsError::Unauthorized`] when the user's role does not permit the requested
 /// operation.
-fn check_role_permission(user: &str, operation_tag: &str, role_config: &RoleConfig) -> KResult<()> {
+async fn check_role_permission(
+    kms: &KMS,
+    user: &str,
+    operation_tag: &str,
+    role_config: &RoleConfig,
+) -> KResult<()> {
     // If no role lists are configured, skip role enforcement entirely.
     if !role_config.is_configured() {
         return Ok(());
@@ -137,12 +171,24 @@ fn check_role_permission(user: &str, operation_tag: &str, role_config: &RoleConf
     match effective_role {
         Role::Administrator => Ok(()),
         role => {
-            // Only gate operations that map to a KmipOperation enum variant.
-            if let Some(op) = operation_tag_to_kmip_operation(operation_tag) {
-                if !role.allowed_operations().contains(&op) {
+            // For operations that have a KmipOperation mapping, defer to the individual handler.
+            // Each handler already checks crypto_officer_users + explicit grants, which correctly
+            // allows delegated privileges (e.g. an Operator who was explicitly granted Create).
+            if operation_tag_to_kmip_operation(operation_tag).is_some() {
+                return Ok(());
+            }
+
+            // For lifecycle operations without a KmipOperation mapping (CreateKeyPair, Register,
+            // ReKeyKeyPair, CreateSplitKey), block Operators unless they have an explicit Create
+            // permission grant in the database.
+            if LIFECYCLE_OPERATION_TAGS.contains(&operation_tag) && matches!(role, Role::Operator) {
+                let has_create =
+                    user_has_permission(user, None, &KmipOperation::Create, kms).await?;
+                if !has_create {
                     kms_bail!(KmsError::Unauthorized(format!(
-                        "User `{user}` (role: {role:?}) is not authorized to perform \
-                         operation `{operation_tag}`"
+                        "User `{user}` (role: Operator) is not authorized to perform \
+                         operation `{operation_tag}` (lifecycle operation requires CryptoOfficer \
+                         role or explicit Create grant)"
                     )))
                 }
             }
@@ -180,7 +226,7 @@ async fn dispatch_inner(
     operation_tag: &str,
 ) -> KResult<Operation> {
     // Enforce role-based access control before any other check.
-    check_role_permission(user, operation_tag, &kms.params.role_config)?;
+    check_role_permission(kms, user, operation_tag, &kms.params.role_config).await?;
 
     // For operations where the request carries algorithm choices, validate them
     // before executing any cryptographic action.

crate/server/src/core/operations/dispatch.rs

@@ -36,7 +36,7 @@ use crate::{
 ///
 /// If all provided shares carry the `x-cosmian-administrator-ceremony` vendor attribute,
 /// **and** the number of shares satisfies the threshold recorded in those shares, **and**
-/// all share owners are listed in `administrator.users`, then
+/// the **assembling user** (caller) is listed in `administrator.users`, then
 /// `activate_administrator_ceremony()` is called on the database to persist the activation record
 /// that enables the Administrator role for ceremony-protected deployments.
 pub(crate) async fn join_split_key(
@@ -81,6 +81,16 @@ pub(crate) async fn join_split_key(
     let cryptographic_algorithm = first_sk.key_block.cryptographic_algorithm;
     let cryptographic_length = first_sk.key_block.cryptographic_length;
 
+    // Validate that the declared split method in the request matches the shares.
+    // Per KMIP 2.1 semantics, rejecting mismatches prevents confusing/misleading requests.
+    if request.split_key_method != method {
+        kms_bail!(KmsError::InvalidRequest(format!(
+            "JoinSplitKey: request declares split key method {:?} \
+             but shares use {method:?}",
+            request.split_key_method
+        )));
+    }
+
     // Validate threshold
     let shares_count = i32::try_from(owms.len()).unwrap_or(i32::MAX);
     if shares_count < threshold {
@@ -176,6 +186,35 @@ pub(crate) async fn join_split_key(
         }
     }
 
+    // Enforce the same Create/Import restriction as create.rs / import.rs.
+    // Administrator ceremony candidates (users in administrator.users) are exempt because
+    // they need JoinSplitKey to activate their role before they hold Administrator privileges.
+    let is_ceremony_candidate = kms
+        .params
+        .role_config
+        .administrator
+        .users
+        .iter()
+        .any(|u| u == user);
+    if !is_ceremony_candidate {
+        if let Some(ref co_users) = kms.params.role_config.crypto_officer_users {
+            let has_create_permission = crate::core::retrieve_object_utils::user_has_permission(
+                user,
+                None,
+                &KmipOperation::Create,
+                kms,
+            )
+            .await?;
+            if !has_create_permission && !co_users.iter().any(|u| u == user) {
+                kms_bail!(KmsError::Unauthorized(
+                    "JoinSplitKey: user does not have permission to create objects \
+                     (CryptoOfficer role or explicit Create grant required)"
+                        .to_owned()
+                ));
+            }
+        }
+    }
+
     // Store the reconstructed key
     let mut tags: HashSet<String> = HashSet::new();
     tags.insert("reconstructed-split-key".to_owned());

crate/server/src/core/operations/join_split_key.rs

@@ -221,8 +221,9 @@ pub(crate) async fn user_has_permission(
     };
 
     // Administrator bypass: if the user is an active Administrator, grant access to
-    // everything (except HSM-backed keys, which are governed by HSM admin rules).
-    if kms.is_administrator(user).await? {
+    // all non-HSM objects. HSM-backed keys are governed by the HSM admin rules below
+    // and are therefore excluded from this bypass.
+    if has_prefix(id).is_none() && kms.is_administrator(user).await? {
         warn!(
             "ADMINISTRATOR_ACCESS: administrator {user} bypassed normal permission check on {id} for {operation_type:?}"
         );

crate/server/src/core/retrieve_object_utils.rs

@@ -285,7 +285,7 @@ pub(crate) async fn disable_administrator(
     if !cfg.require_ceremony {
         return Err(crate::error::KmsError::InvalidRequest(
             "Config-only Administrator cannot be disabled at runtime. Remove the user from \
-             `[administrator].users` in kms.toml and restart the server."
+             `administrator_users` in kms.toml and restart the server."
                 .to_owned(),
         ));
     }

crate/server/src/routes/access.rs

@@ -337,11 +337,8 @@ impl Database {
         let map = self.objects.read().await;
         let mut results: Vec<(String, State, Attributes)> = Vec::new();
         for db in map.values() {
-            results.extend(
-                db.find_all(researched_attributes, state, vendor_id)
-                    .await
-                    .unwrap_or(vec![]),
-            );
+            let partial = db.find_all(researched_attributes, state, vendor_id).await?;
+            results.extend(partial);
         }
         Ok(results)
     }