From 3f32b622fa9942e64db81924273f703567e657ed Mon Sep 17 00:00:00 2001
From: Samuel Cabrero <scabrero@suse.de>
Date: Wed, 8 Jan 2025 16:33:56 +0100
Subject: [PATCH 3/4] SYSDB: Allow multiple services associated to the same
 port

RFC6335 section 5 allow multiple services associated to the same
transport and port. This commit allows storing multiple service entries
in the cache for the same port and the lookup functions will return all
matching entries. The cache_req plugin will pick the first result.

Signed-off-by: Samuel Cabrero <scabrero@suse.de>
---
 src/db/sysdb_services.c | 59 ++++++++++-------------------------------
 src/tests/sysdb-tests.c | 54 ++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 61 deletions(-)

diff --git a/src/db/sysdb_services.c b/src/db/sysdb_services.c
index 9c608415f..40766bf0f 100644
--- a/src/db/sysdb_services.c
+++ b/src/db/sysdb_services.c
@@ -193,51 +193,21 @@ sysdb_store_service(struct sss_domain_info *domain,
 
     in_transaction = true;
 
-    /* Check that the port is unique
-     * If the port appears for any service other than
-     * the one matching the primary_name, we need to
-     * remove them so that getservbyport() can work
-     * properly. Last entry saved to the cache should
-     * always "win".
-     */
+    /* RFC6335 Section 5 allows more than one service associated with a
+     * particular transport protocol and port. In such case the names
+     * must be different so check that all entries have a name. */
     ret = sysdb_getservbyport(tmp_ctx, domain, port, NULL, &res);
     if (ret != EOK && ret != ENOENT) {
         goto done;
     } else if (ret != ENOENT) {
-        if (res->count != 1) {
-            /* Somehow the cache has multiple entries with
-             * the same port. This is corrupted. We'll delete
-             * them all to sort it out.
-             */
-            for (i = 0; i < res->count; i++) {
-                DEBUG(SSSDBG_TRACE_FUNC,
-                      "Corrupt cache entry [%s] detected. Deleting\n",
-                       ldb_dn_canonical_string(tmp_ctx,
-                                               res->msgs[i]->dn));
-
-                ret = sysdb_delete_entry(sysdb, res->msgs[i]->dn, true);
-                if (ret != EOK) {
-                    DEBUG(SSSDBG_MINOR_FAILURE,
-                          "Could not delete corrupt cache entry [%s]\n",
-                           ldb_dn_canonical_string(tmp_ctx,
-                                                   res->msgs[i]->dn));
-                    goto done;
-                }
-            }
-        } else {
-            /* Check whether this is the same name as we're currently
-             * saving to the cache.
-             */
-            name = ldb_msg_find_attr_as_string(res->msgs[0],
+        /* Check whether this is the same name as we're currently
+         * saving to the cache. */
+        for (i = 0; i < res->count; i++) {
+            name = ldb_msg_find_attr_as_string(res->msgs[i],
                                                SYSDB_NAME,
                                                NULL);
-            if (!name || strcmp(name, primary_name) != 0) {
-
-                if (!name) {
-                    DEBUG(SSSDBG_CRIT_FAILURE,
-                          "A service with no name?\n");
-                    /* Corrupted */
-                }
+            if (!name) {
+                DEBUG(SSSDBG_CRIT_FAILURE, "A service with no name?\n");
 
                 /* Either this is a corrupt entry or it's another service
                  * claiming ownership of this port. In order to account
@@ -247,22 +217,21 @@ sysdb_store_service(struct sss_domain_info *domain,
                       "Corrupt or replaced cache entry [%s] detected. "
                        "Deleting\n",
                        ldb_dn_canonical_string(tmp_ctx,
-                                               res->msgs[0]->dn));
+                                               res->msgs[i]->dn));
 
-                ret = sysdb_delete_entry(sysdb, res->msgs[0]->dn, true);
+                ret = sysdb_delete_entry(sysdb, res->msgs[i]->dn, true);
                 if (ret != EOK) {
                     DEBUG(SSSDBG_OP_FAILURE,
                           "Could not delete cache entry [%s]\n",
                            ldb_dn_canonical_string(tmp_ctx,
-                                                   res->msgs[0]->dn));
+                                                   res->msgs[i]->dn));
                 }
             }
         }
     }
     talloc_zfree(res);
 
-    /* Ok, ports should now be unique. Now look
-     * the service up by name to determine if we
+    /* Now look the service up by name to determine if we
      * need to update existing entries or modify
      * aliases.
      */
@@ -683,7 +652,7 @@ sysdb_svc_delete(struct sss_domain_info *domain,
 
     /* There should only be one matching entry,
      * but if there are multiple, we should delete
-     * them all to de-corrupt the DB.
+     * them all.
      */
     for (i = 0; i < res->count; i++) {
         ret = sysdb_delete_entry(sysdb, res->msgs[i]->dn, false);
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index 8d0552039..45fb9e3a8 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -4880,7 +4880,7 @@ void services_check_match(struct sysdb_test_ctx *test_ctx,
     const char *ret_name;
     int ret_port;
     struct ldb_result *res;
-    struct ldb_message *msg;
+    struct ldb_message *msg = NULL;
     struct ldb_message_element *el;
 
     if (by_name) {
@@ -4889,18 +4889,29 @@ void services_check_match(struct sysdb_test_ctx *test_ctx,
                                   NULL, &res);
         fail_if(ret != EOK, "sysdb_getservbyname error [%s]\n",
                              strerror(ret));
+        fail_if(res == NULL, "ENOMEM");
+        ck_assert_int_eq(res->count, 1);
+        msg = res->msgs[0];
     } else {
         /* Look up the newly-added service by port */
         ret = sysdb_getservbyport(test_ctx, test_ctx->domain, port, NULL,
                                   &res);
         fail_if(ret != EOK, "sysdb_getservbyport error [%s]\n",
                              strerror(ret));
+        fail_if(res == NULL, "ENOMEM");
+        /* Port lookups can return multiple results. Select the correct one */
+        for (i = 0; i < res->count; i++) {
+            ret_name = ldb_msg_find_attr_as_string(res->msgs[i], SYSDB_NAME, NULL);
+            fail_if(ret_name == NULL, "ENOENT");
+            if (strcmp(ret_name, primary_name) == 0) {
+                msg = res->msgs[i];
+                break;
+            }
+        }
     }
-    fail_if(res == NULL, "ENOMEM");
-    ck_assert_int_eq(res->count, 1);
+    fail_if(msg == NULL, "ENOMEM");
 
     /* Make sure the returned entry matches */
-    msg = res->msgs[0];
     ret_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
     fail_if(ret_name == NULL, "Cannot find attribute: " SYSDB_NAME);
     fail_unless(strcmp(ret_name, primary_name) == 0,
@@ -5075,7 +5086,7 @@ START_TEST(test_sysdb_store_services)
                               primary_name, port,
                               aliases, protocols);
 
-    /* Change the service name */
+    /* Add a second service using the same port */
     ret = sysdb_store_service(test_ctx->domain,
                               alt_primary_name, port,
                               aliases, protocols,
@@ -5086,34 +5097,42 @@ START_TEST(test_sysdb_store_services)
                               alt_primary_name, port,
                               aliases, protocols);
 
+    services_check_match_name(test_ctx,
+                              primary_name, port,
+                              aliases, protocols);
+
     /* Search by port and make sure the results match */
     services_check_match_port(test_ctx,
-                              alt_primary_name, port,
+                              primary_name, port,
                               aliases, protocols);
 
-
-    /* Change it back */
-    ret = sysdb_store_service(test_ctx->domain,
-                              primary_name, port,
-                              aliases, protocols,
-                              NULL, NULL, 1, 1);
-    fail_if (ret != EOK, "[%s]", strerror(ret));
+    services_check_match_port(test_ctx,
+                              alt_primary_name, port,
+                              aliases, protocols);
 
     /* Change the port number */
     ret = sysdb_store_service(test_ctx->domain,
-                              primary_name, altport,
+                              alt_primary_name, altport,
                               aliases, protocols,
                               NULL, NULL, 1, 1);
     fail_if (ret != EOK, "[%s]", strerror(ret));
 
     /* Search by name and make sure the results match */
     services_check_match_name(test_ctx,
-                              primary_name, altport,
+                              alt_primary_name, altport,
+                              aliases, protocols);
+
+    services_check_match_name(test_ctx,
+                              primary_name, port,
                               aliases, protocols);
 
     /* Search by port and make sure the results match */
     services_check_match_port(test_ctx,
-                              primary_name, altport,
+                              alt_primary_name, altport,
+                              aliases, protocols);
+
+    services_check_match_port(test_ctx,
+                              primary_name, port,
                               aliases, protocols);
 
     /* TODO: Test changing aliases and protocols */
@@ -5127,6 +5146,9 @@ START_TEST(test_sysdb_store_services)
      * doesn't like adding and deleting the same entry in a
      * single transaction.
      */
+    ret = sysdb_svc_delete(test_ctx->domain, NULL, port, NULL);
+    fail_if(ret != EOK, "[%s]", strerror(ret));
+
     ret = sysdb_svc_delete(test_ctx->domain, NULL, altport, NULL);
     fail_if(ret != EOK, "[%s]", strerror(ret));
 
-- 
2.35.3

