Review feedback

idm_admin and admin should be exempted from the denied names process,
as these values will already be denied due to attribute uniqueness.
Additionally improved the denied names check to only validate the
name during a change, not during a modifification. This way entries
that become denied can get themself out of the pickle.

.github/workflows/docker_build_kanidm.yml

@@ -35,9 +35,15 @@ jobs:
     needs:
       - set_tag_values
     steps:
-      - uses: actions/checkout@v4
+      - name: Checkout repository
+        uses: actions/checkout@v4
+        with:
+          persist-credentials: false
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v3
+      - name: Docker metadata
+        id: meta
+        uses: docker/metadata-action@v5
       - name: Build kanidm
         uses: docker/build-push-action@v6
         with:
@@ -47,6 +53,9 @@ jobs:
           build-args: |
             "KANIDM_FEATURES="
           file: tools/Dockerfile
+          context: .
+          labels: ${{ steps.meta.outputs.labels }}
+          annotations: ${{ steps.meta.outputs.annotations }}
           # Must use OCI exporter for multi-arch: https://github.com/docker/buildx/pull/1813
           outputs: type=oci,dest=/tmp/kanidm-docker.tar
       - name: Upload artifact
@@ -60,8 +69,8 @@ jobs:
     # This step is split so that we don't apply "packages: write" permission
     # except when uploading the final Docker image to GHCR.
     runs-on: ubuntu-latest
-    if: ( github.ref_type == 'tag' || github.ref == 'refs/heads/master' ) && github.repository == 'kanidm/kanidm'
-    needs: kanidm_build
+    if: ( github.ref_type == 'tag' || github.ref == 'refs/heads/master' )
+    needs: [kanidm_build, set_tag_values]
     permissions:
       packages: write
 
@@ -78,4 +87,4 @@ jobs:
           echo "${{ secrets.GITHUB_TOKEN }}" | \
             oras login -u "${{ github.actor }}" --password-stdin ghcr.io
           oras copy --from-oci-layout "/tmp/kanidm-docker.tar:devel" \
-            "ghcr.io/${{ github.repository_owner }}/kanidm:devel"
+            "ghcr.io/${{ needs.set_tag_values.outputs.owner_lc }}/kanidm:devel"

.github/workflows/docker_build_kanidmd.yml

@@ -35,27 +35,15 @@ jobs:
     runs-on: ubuntu-latest
     needs: set_tag_values
     steps:
-      - uses: actions/checkout@v4
+      - name: Checkout repository
+        uses: actions/checkout@v4
+        with:
+          persist-credentials: false
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v3
       - name: Docker metadata
         id: meta
         uses: docker/metadata-action@v5
-        with:
-          # list of Docker images to use as base name for tags
-          # images: |
-          #   kanidm/kanidmd
-          #   ghcr.io/username/app
-          # generate Docker tags based on the following events/attributes
-          tags: |
-            type=schedule
-            type=ref,event=branch
-            type=ref,event=pr
-            type=semver,pattern={{version}}
-            type=semver,pattern={{major}}.{{minor}}
-            type=semver,pattern={{major}}
-            type=sha
-
       - name: Build kanidmd
         uses: docker/build-push-action@v6
         with:
@@ -64,6 +52,9 @@ jobs:
           # build-args: |
           #  "KANIDM_BUILD_OPTIONS=-j1"
           file: server/Dockerfile
+          context: .
+          labels: ${{ steps.meta.outputs.labels }}
+          annotations: ${{ steps.meta.outputs.annotations }}
           # Must use OCI exporter for multi-arch: https://github.com/docker/buildx/pull/1813
           outputs: type=oci,dest=/tmp/kanidmd-docker.tar
       - name: Upload artifact
@@ -77,8 +68,8 @@ jobs:
     # This step is split so that we don't apply "packages: write" permission
     # except when uploading the final Docker image to GHCR.
     runs-on: ubuntu-latest
-    if: ( github.ref_type== 'tag' || github.ref == 'refs/heads/master' ) && github.repository == 'kanidm/kanidm'
-    needs: kanidmd_build
+    if: ( github.ref_type== 'tag' || github.ref == 'refs/heads/master' )
+    needs: [kanidmd_build, set_tag_values]
     permissions:
       packages: write
 
@@ -95,4 +86,4 @@ jobs:
           echo "${{ secrets.GITHUB_TOKEN }}" | \
             oras login -u "${{ github.actor }}" --password-stdin ghcr.io
           oras copy --from-oci-layout "/tmp/kanidmd-docker.tar:devel" \
-            "ghcr.io/${{ github.repository_owner }}/kanidmd:devel"
+            "ghcr.io/${{ needs.set_tag_values.outputs.owner_lc }}/kanidmd:devel"

.github/workflows/docker_build_radiusd.yml

@@ -35,17 +35,26 @@ jobs:
     runs-on: ubuntu-latest
     needs: set_tag_values
     steps:
-      - uses: actions/checkout@v4
+      - name: Checkout repository
+        uses: actions/checkout@v4
+        with:
+          persist-credentials: false
       - name: Set up QEMU
         uses: docker/setup-qemu-action@v3
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v3
+      - name: Docker metadata
+        id: meta
+        uses: docker/metadata-action@v5
       - name: Build radius
         uses: docker/build-push-action@v6
         with:
           platforms: linux/arm64,linux/amd64
           tags: ghcr.io/${{ needs.set_tag_values.outputs.owner_lc }}/radius:devel,ghcr.io/${{ needs.set_tag_values.outputs.owner_lc }}/radius:${{ needs.set_tag_values.outputs.ref_name}}
           file: rlm_python/Dockerfile
+          context: .
+          labels: ${{ steps.meta.outputs.labels }}
+          annotations: ${{ steps.meta.outputs.annotations }}
           # Must use OCI exporter for multi-arch: https://github.com/docker/buildx/pull/1813
           outputs: type=oci,dest=/tmp/radius-docker.tar
       - name: Upload artifact
@@ -59,8 +68,8 @@ jobs:
     # This step is split so that we don't apply "packages: write" permission
     # except when uploading the final Docker image to GHCR.
     runs-on: ubuntu-latest
-    if: ( github.ref_type == 'tag' || github.ref == 'refs/heads/master' ) && github.repository == 'kanidm/kanidm'
-    needs: radius_build
+    if: ( github.ref_type == 'tag' || github.ref == 'refs/heads/master' )
+    needs: [radius_build, set_tag_values]
     permissions:
       packages: write
 
@@ -79,4 +88,4 @@ jobs:
           echo "${{ secrets.GITHUB_TOKEN }}" | \
             oras login -u "${{ github.actor }}" --password-stdin ghcr.io
           oras copy --from-oci-layout "/tmp/radius-docker.tar:devel" \
-            "ghcr.io/${{ github.repository_owner }}/radius:devel"
+            "ghcr.io/${{ needs.set_tag_values.outputs.owner_lc }}/radius:devel"

server/lib/src/constants/schema.rs

@@ -208,6 +208,16 @@ pub static ref SCHEMA_ATTR_DENIED_NAME: SchemaAttribute = SchemaAttribute {
     ..Default::default()
 };
 
+pub static ref SCHEMA_ATTR_DENIED_NAME_DL10: SchemaAttribute = SchemaAttribute {
+    uuid: UUID_SCHEMA_ATTR_DENIED_NAME,
+    name: Attribute::DeniedName,
+    description: "Iname values that are not allowed to be used in 'name'.".to_string(),
+
+    syntax: SyntaxType::Utf8StringIname,
+    multivalue: true,
+    ..Default::default()
+};
+
 pub static ref SCHEMA_ATTR_DOMAIN_TOKEN_KEY: SchemaAttribute = SchemaAttribute {
     uuid: UUID_SCHEMA_ATTR_DOMAIN_TOKEN_KEY,
     name: Attribute::DomainTokenKey,

server/lib/src/plugins/valuedeny.rs

@@ -10,7 +10,7 @@ impl Plugin for ValueDeny {
         "plugin_value_deny"
     }
 
-    #[instrument(level = "debug", name = "base_pre_create_transform", skip_all)]
+    #[instrument(level = "debug", name = "denied_names_pre_create_transform", skip_all)]
     #[allow(clippy::cognitive_complexity)]
     fn pre_create_transform(
         qs: &mut QueryServerWriteTransaction,
@@ -19,9 +19,25 @@ impl Plugin for ValueDeny {
     ) -> Result<(), OperationError> {
         let denied_names = qs.denied_names();
 
+        if denied_names.is_empty() {
+            // Nothing to check.
+            return Ok(());
+        }
+
         let mut pass = true;
 
         for entry in cand {
+            // If the entry doesn't have a uuid, it's invalid anyway and will fail schema.
+            if let Some(e_uuid) = entry.get_uuid() {
+                // SAFETY - Thanks to JpWarren blowing his nipper clean off, we need to
+                // assert that the break glass and system accounts are NOT subject to
+                // this process.
+                if e_uuid < DYNAMIC_RANGE_MINIMUM_UUID {
+                    // These entries are exempt
+                    continue;
+                }
+            }
+
             if let Some(name) = entry.get_ava_single_iname(Attribute::Name) {
                 if denied_names.contains(name) {
                     pass = false;
@@ -37,27 +53,24 @@ impl Plugin for ValueDeny {
         }
     }
 
-    #[instrument(level = "debug", name = "base_pre_modify", skip_all)]
     fn pre_modify(
         qs: &mut QueryServerWriteTransaction,
-        _pre_cand: &[Arc<EntrySealedCommitted>],
+        pre_cand: &[Arc<EntrySealedCommitted>],
         cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
         _me: &ModifyEvent,
     ) -> Result<(), OperationError> {
-        Self::modify(qs, cand)
+        Self::modify(qs, pre_cand, cand)
     }
 
-    #[instrument(level = "debug", name = "base_pre_modify", skip_all)]
     fn pre_batch_modify(
         qs: &mut QueryServerWriteTransaction,
-        _pre_cand: &[Arc<EntrySealedCommitted>],
+        pre_cand: &[Arc<EntrySealedCommitted>],
         cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
         _me: &BatchModifyEvent,
     ) -> Result<(), OperationError> {
-        Self::modify(qs, cand)
+        Self::modify(qs, pre_cand, cand)
     }
 
-    #[instrument(level = "debug", name = "base::verify", skip_all)]
     fn verify(qs: &mut QueryServerReadTransaction) -> Vec<Result<(), ConsistencyError>> {
         let denied_names = qs.denied_names().clone();
 
@@ -68,7 +81,15 @@ impl Plugin for ValueDeny {
             match qs.internal_search(filt) {
                 Ok(entries) => {
                     for entry in entries {
-                        results.push(Err(ConsistencyError::DeniedName(entry.get_uuid())));
+                        let e_uuid = entry.get_uuid();
+                        // SAFETY - Thanks to JpWarren blowing his nipper clean off, we need to
+                        // assert that the break glass accounts are NOT subject to this process.
+                        if e_uuid < DYNAMIC_RANGE_MINIMUM_UUID {
+                            // These entries are exempt
+                            continue;
+                        }
+
+                        results.push(Err(ConsistencyError::DeniedName(e_uuid)));
                     }
                 }
                 Err(err) => {
@@ -83,17 +104,37 @@ impl Plugin for ValueDeny {
 }
 
 impl ValueDeny {
+    #[instrument(level = "debug", name = "denied_names_modify", skip_all)]
     fn modify(
         qs: &mut QueryServerWriteTransaction,
-        cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
+        pre_cand: &[Arc<EntrySealedCommitted>],
+        cand: &mut [EntryInvalidCommitted],
     ) -> Result<(), OperationError> {
         let denied_names = qs.denied_names();
 
+        if denied_names.is_empty() {
+            // Nothing to check.
+            return Ok(());
+        }
+
         let mut pass = true;
 
-        for entry in cand {
-            if let Some(name) = entry.get_ava_single_iname(Attribute::Name) {
-                if denied_names.contains(name) {
+        for (pre_entry, post_entry) in pre_cand.iter().zip(cand.iter()) {
+            // If the entry doesn't have a uuid, it's invalid anyway and will fail schema.
+            let e_uuid = pre_entry.get_uuid();
+            // SAFETY - Thanks to JpWarren blowing his nipper clean off, we need to
+            // assert that the break glass accounts are NOT subject to this process.
+            if e_uuid < DYNAMIC_RANGE_MINIMUM_UUID {
+                // These entries are exempt
+                continue;
+            }
+
+            let pre_name = pre_entry.get_ava_single_iname(Attribute::Name);
+            let post_name = post_entry.get_ava_single_iname(Attribute::Name);
+
+            if let Some(name) = post_name {
+                // Only if the name is changing, and is denied.
+                if pre_name != post_name && denied_names.contains(name) {
                     pass = false;
                     error!(?name, "name denied by system configuration");
                 }
@@ -117,10 +158,10 @@ mod tests {
 
         let me_inv_m = ModifyEvent::new_internal_invalid(
             filter!(f_eq(Attribute::Uuid, PVUUID_SYSTEM_CONFIG.clone())),
-            ModifyList::new_list(vec![Modify::Present(
-                Attribute::DeniedName,
-                Value::new_iname("tobias"),
-            )]),
+            ModifyList::new_list(vec![
+                Modify::Present(Attribute::DeniedName, Value::new_iname("tobias")),
+                Modify::Present(Attribute::DeniedName, Value::new_iname("ellie")),
+            ]),
         );
         assert!(server_txn.modify(&me_inv_m).is_ok());
 
@@ -148,30 +189,103 @@ mod tests {
 
     #[qs_test]
     async fn test_valuedeny_modify(server: &QueryServer) {
-        setup_name_deny(server).await;
-
+        // Create an entry that has a name which will become denied to test how it
+        // interacts.
         let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap();
-        let t_uuid = Uuid::new_v4();
+        let e_uuid = Uuid::new_v4();
         assert!(server_txn
             .internal_create(vec![entry_init!(
                 (Attribute::Class, EntryClass::Object.to_value()),
                 (Attribute::Class, EntryClass::Account.to_value()),
                 (Attribute::Class, EntryClass::Person.to_value()),
-                (Attribute::Name, Value::new_iname("newname")),
-                (Attribute::Uuid, Value::Uuid(t_uuid)),
-                (Attribute::Description, Value::new_utf8s("Tobias")),
-                (Attribute::DisplayName, Value::new_utf8s("Tobias"))
+                (Attribute::Name, Value::new_iname("ellie")),
+                (Attribute::Uuid, Value::Uuid(e_uuid)),
+                (Attribute::Description, Value::new_utf8s("Ellie Meow")),
+                (Attribute::DisplayName, Value::new_utf8s("Ellie Meow"))
             ),])
             .is_ok());
 
-        // Now mod it
+        assert!(server_txn.commit().is_ok());
 
+        setup_name_deny(server).await;
+
+        let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap();
+
+        // Attempt to mod ellie.
+
+        // Can mod a different attribute
         assert!(server_txn
             .internal_modify_uuid(
-                t_uuid,
+                e_uuid,
+                &ModifyList::new_purge_and_set(Attribute::DisplayName, Value::new_utf8s("tobias"))
+            )
+            .is_ok());
+
+        // Can't mod to another invalid name.
+        assert!(server_txn
+            .internal_modify_uuid(
+                e_uuid,
                 &ModifyList::new_purge_and_set(Attribute::Name, Value::new_iname("tobias"))
             )
             .is_err());
+
+        // Can mod to a valid name.
+        assert!(server_txn
+            .internal_modify_uuid(
+                e_uuid,
+                &ModifyList::new_purge_and_set(
+                    Attribute::Name,
+                    Value::new_iname("miss_meowington")
+                )
+            )
+            .is_ok());
+
+        // Now mod from the valid name to an invalid one.
+        assert!(server_txn
+            .internal_modify_uuid(
+                e_uuid,
+                &ModifyList::new_purge_and_set(Attribute::Name, Value::new_iname("tobias"))
+            )
+            .is_err());
+
+        assert!(server_txn.commit().is_ok());
+    }
+
+    #[qs_test]
+    async fn test_valuedeny_jpwarren_special(server: &QueryServer) {
+        // Assert that our break glass accounts are exempt from this processing.
+        let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap();
+
+        let me_inv_m = ModifyEvent::new_internal_invalid(
+            filter!(f_eq(Attribute::Uuid, PVUUID_SYSTEM_CONFIG.clone())),
+            ModifyList::new_list(vec![
+                Modify::Present(Attribute::DeniedName, Value::new_iname("admin")),
+                Modify::Present(Attribute::DeniedName, Value::new_iname("idm_admin")),
+            ]),
+        );
+        assert!(server_txn.modify(&me_inv_m).is_ok());
+        assert!(server_txn.commit().is_ok());
+
+        let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap();
+
+        assert!(server_txn
+            .internal_modify_uuid(
+                UUID_IDM_ADMIN,
+                &ModifyList::new_purge_and_set(
+                    Attribute::DisplayName,
+                    Value::new_utf8s("Idm Admin")
+                )
+            )
+            .is_ok());
+
+        assert!(server_txn
+            .internal_modify_uuid(
+                UUID_ADMIN,
+                &ModifyList::new_purge_and_set(Attribute::DisplayName, Value::new_utf8s("Admin"))
+            )
+            .is_ok());
+
+        assert!(server_txn.commit().is_ok());
     }
 
     #[qs_test]

server/lib/src/server/migrations.rs

@@ -427,7 +427,10 @@ impl QueryServerWriteTransaction<'_> {
         // =========== Apply changes ==============
 
         // Now update schema
-        let idm_schema_changes = [SCHEMA_CLASS_DOMAIN_INFO_DL10.clone().into()];
+        let idm_schema_changes = [
+            SCHEMA_ATTR_DENIED_NAME_DL10.clone().into(),
+            SCHEMA_CLASS_DOMAIN_INFO_DL10.clone().into(),
+        ];
 
         idm_schema_changes
             .into_iter()