Merge pull request #1717

Bugfix: Clean up error handling in LDAP plugin, fix dependencies
bareos · Mar 7, 2024 · f719acf · f719acf
2 parents 36e38f9 + eb2dfb4
commit f719acf
Show file tree

Hide file tree

Showing 10 changed files with 135 additions and 67 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - filed: skip stripped top level directories [PR #1686]
 - jcr: fix some compiler warnings [PR #1648]
 - build: Fix debugsource RPM package generation [PR #1713]
+- Bugfix: Clean up error handling in LDAP plugin, fix dependencies [PR #1717]
 
 ### Removed
 - plugins: remove old deprecated postgres plugin [PR #1606]
@@ -105,4 +106,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [PR #1696]: https://github.com/bareos/bareos/pull/1696
 [PR #1708]: https://github.com/bareos/bareos/pull/1708
 [PR #1713]: https://github.com/bareos/bareos/pull/1713
+[PR #1717]: https://github.com/bareos/bareos/pull/1717
 [unreleased]: https://github.com/bareos/bareos/tree/master
diff --git a/core/src/plugins/filed/python/ldap/bareos-fd-ldap.py b/core/src/plugins/filed/python/ldap/bareos-fd-ldap.py
@@ -3,7 +3,7 @@
 # BAREOS - Backup Archiving REcovery Open Sourced
 #
 # Copyright (C) 2015-2015 Planets Communications B.V.
-# Copyright (C) 2015-2023 Bareos GmbH & Co. KG
+# Copyright (C) 2015-2024 Bareos GmbH & Co. KG
 #
 # This program is Free Software; you can redistribute it and/or
 # modify it under the terms of version three of the GNU Affero General Public
@@ -51,6 +51,21 @@ def _safe_encode(data):
     return data
 
 
+def _log_ldap_err(e, loglevel, txt):
+    desc = ""
+    info = ""
+    msg = e.args[0]
+    if "desc" in msg:
+        desc = msg["desc"]
+    if "info" in msg:
+        info = msg["info"]
+
+    bareosfd.JobMessage(
+        loglevel,
+        "{}: {} {}\n".format(txt, desc, info),
+    )
+
+
 @BareosPlugin
 class BareosFdPluginLDAP(BareosFdPluginBaseclass.BareosFdPluginBaseclass):
     """
@@ -60,8 +75,9 @@ class BareosFdPluginLDAP(BareosFdPluginBaseclass.BareosFdPluginBaseclass):
     def __init__(self, plugindef):
         bareosfd.DebugMessage(
             100,
-            "Constructor called in module %s with plugindef=%s\n"
-            % (__name__, plugindef),
+            "Constructor called in module {} with plugindef={}\n".format(
+                __name__, plugindef
+            ),
         )
         super(BareosFdPluginLDAP, self).__init__(plugindef, ["uri", "basedn"])
         self.ldap = BareosLDAPWrapper()
@@ -72,7 +88,7 @@ def parse_plugin_definition(self, plugindef):
         Parses the plugin arguments and validates the options.
         """
         bareosfd.DebugMessage(
-            100, "parse_plugin_definition() was called in module %s\n" % (__name__)
+            100, "parse_plugin_definition() was called in module {}\n".format(__name__)
         )
         super(BareosFdPluginLDAP, self).parse_plugin_definition(plugindef)
 
@@ -108,7 +124,7 @@ def start_restore_job(self):
 
     def start_restore_file(self, cmd):
         bareosfd.DebugMessage(
-            100, "BareosFdPluginLDAP:start_restore_file() called with %s\n" % (cmd)
+            100, "BareosFdPluginLDAP:start_restore_file() called with {}\n".format(cmd)
         )
 
         # It can happen that this is the first entry point on a restore
@@ -126,7 +142,7 @@ def create_file(self, restorepkt):
         to get the actual DN of the LDAP record
         """
         bareosfd.DebugMessage(
-            100, "BareosFdPluginLDAP:create_file() called with %s\n" % (restorepkt)
+            100, "BareosFdPluginLDAP:create_file() called with {}\n".format(restorepkt)
         )
         if restorepkt.type == bareosfd.FT_DIREND:
             restorepkt.create_status = bareosfd.CF_SKIP
@@ -136,15 +152,17 @@ def create_file(self, restorepkt):
         else:
             bareosfd.JobMessage(
                 bareosfd.M_FATAL,
-                "Request to restore illegal filetype %s\n" % (restorepkt.type),
+                "Request to restore illegal filetype {}\n".format(restorepkt.type),
             )
             return bareosfd.bRC_Error
 
         return bareosfd.bRC_OK
 
     def plugin_io(self, IOP):
         bareosfd.DebugMessage(
-            100, "BareosFdPluginLDAP:plugin_io() called with function %s\n" % (IOP.func)
+            100,
+            "BareosFdPluginLDAP:plugin_io() called"
+            "with function {}\n".format(IOP.func),
         )
 
         if IOP.func == bareosfd.IO_OPEN:
@@ -245,23 +263,18 @@ def connect_and_bind(self, options, bulk=False):
         except ldap.INVALID_CREDENTIALS:
             bareosfd.JobMessage(
                 bareosfd.M_FATAL,
-                "Failed to bind to LDAP uri due to invalid credentials %s\n"
-                % (options["uri"]),
+                "Failed to bind to LDAP uri due to invalid credentials {}\n".format(
+                    options["uri"]
+                ),
             )
 
             return bareosfd.bRC_Error
         except ldap.LDAPError as e:
-            if type(e.message) == dict and "desc" in e.message:
-                bareosfd.JobMessage(
-                    bareosfd.M_FATAL,
-                    "Failed to bind to LDAP uri %s: %s %s\n"
-                    % (options["uri"], e.message["desc"], e.message["info"]),
-                )
-            else:
-                bareosfd.JobMessage(
-                    bareosfd.M_FATAL,
-                    "Failed to bind to LDAP uri %s: %s\n" % (options["uri"], e),
-                )
+            _log_ldap_err(
+                e,
+                bareosfd.M_FATAL,
+                "Failed to bind to LDAP uri {}".format(options["uri"]),
+            )
 
             return bareosfd.bRC_Error
 
@@ -299,18 +312,12 @@ def prepare_backup(self, options):
                 options["basedn"], searchScope, searchFilter, attributeFilter
             )
         except ldap.LDAPError as e:
-            if type(e.message) == dict and "desc" in e.message:
-                bareosfd.JobMessage(
-                    bareosfd.M_FATAL,
-                    "Failed to execute LDAP search on LDAP uri %s: %s\n"
-                    % (options["uri"], e.message["desc"]),
-                )
-            else:
-                bareosfd.JobMessage(
-                    bareosfd.M_FATAL,
-                    "Failed to execute LDAP search on LDAP uri %s: %s\n"
-                    % (options["uri"], e),
-                )
+            _log_ldap_err(
+                e,
+                bareosfd.M_FATAL,
+                "Failed to execute LDAP search"
+                " on LDAP uri {}".format(options["uri"]),
+            )
 
             return bareosfd.bRC_Error
 
@@ -384,10 +391,15 @@ def get_next_file_to_backup(self, savepkt):
                 # if there is nothing return an error.
                 try:
                     res_type, res_data, res_msgid, res_controls = next(self.resultset)
-                    self.ldap_entries = collections.deque(res_data)
-                except ldap.NO_SUCH_OBJECT:
+                    if res_type == ldap.RES_SEARCH_ENTRY:
+                        self.ldap_entries = collections.deque(res_data)
+                except ldap.NO_SUCH_OBJECT as e:
+                    _log_ldap_err(e, bareosfd.M_FATAL, "error searching base DN")
                     return bareosfd.bRC_Error
                 except StopIteration:
+                    bareosfd.JobMessage(
+                        bareosfd.M_FATAL, "premature end of LDAP result set\n"
+                    )
                     return bareosfd.bRC_Error
 
             # Get the next entry from the result set.
@@ -473,7 +485,10 @@ def has_next_file(self):
                 try:
                     # Get the next result set
                     res_type, res_data, res_msgid, res_controls = next(self.resultset)
-                    self.ldap_entries = collections.deque(res_data)
+                    if res_type == ldap.RES_SEARCH_ENTRY:
+                        self.ldap_entries = collections.deque(res_data)
+                    else:
+                        self.ldap_entries = collections.deque()
 
                     # We expect something to be in the result set but better check
                     if self.ldap_entries:
@@ -499,7 +514,7 @@ def restore_entry(self):
             if self.dn != dn:
                 bareosfd.JobMessage(
                     bareosfd.M_INFO,
-                    "Restoring original DN %s as %s\n" % (dn, self.dn),
+                    "Restoring original DN {} as {}\n".format(dn, self.dn),
                 )
 
             if dn:
@@ -514,24 +529,19 @@ def restore_entry(self):
                             self.ld.delete_s(self.dn)
                             self.ld.add_s(self.dn, add_ldif)
                         except ldap.LDAPError as e:
-                            if type(e.message) == dict and "desc" in e.message:
-                                bareosfd.JobMessage(
-                                    bareosfd.M_ERROR,
-                                    "Failed to restore LDAP DN %s: %s\n"
-                                    % (self.dn, e.message["desc"]),
-                                )
-                            else:
-                                bareosfd.JobMessage(
-                                    bareosfd.M_ERROR,
-                                    "Failed to restore LDAP DN %s: %s\n" % (self.dn, e),
-                                )
+                            _log_ldap_err(
+                                e,
+                                bareosfd.M_ERROR,
+                                "Failed to restore LDAP DN {}".format(self.dn),
+                            )
+
                             self.ldif = None
                             return bareosfd.bRC_Error
                 else:
                     bareosfd.JobMessage(
                         bareosfd.M_ERROR,
-                        "Failed to restore LDAP DN %s no writable binding to LDAP exists\n"
-                        % (self.dn),
+                        "Failed to restore LDAP DN {}"
+                        " no writable binding to LDAP exists\n".format(self.dn),
                     )
                     self.ldif = None
                     return bareosfd.bRC_Error

diff --git a/debian/control b/debian/control
@@ -339,7 +339,7 @@ Package:        bareos-filedaemon-ldap-python-plugin
 Architecture:   any
 Section:        python
 Pre-Depends:    debconf (>= 1.4.30) | debconf-2.0
-Depends:        bareos-common (= ${binary:Version}), bareos-filedaemon-python3-plugin (= ${binary:Version}), python-ldap, ${misc:Depends}
+Depends:        bareos-common (= ${binary:Version}), bareos-filedaemon-python3-plugin (= ${binary:Version}), python3-ldap, ${misc:Depends}
 Description: Backup Archiving Recovery Open Sourced - file daemon LDAP plugin
  Bareos is a set of programs to manage backup, recovery and verification of
  data across a network of computers of different kinds.

diff --git a/debian/control.bareos-filedaemon-python-plugins-common b/debian/control.bareos-filedaemon-python-plugins-common
@@ -13,7 +13,7 @@ Package:        bareos-filedaemon-ldap-python-plugin
 Architecture:   any
 Section:        python
 Pre-Depends:    debconf (>= 1.4.30) | debconf-2.0
-Depends:        bareos-common (= ${binary:Version}), bareos-filedaemon-python3-plugin (= ${binary:Version}), python-ldap, ${misc:Depends}
+Depends:        bareos-common (= ${binary:Version}), bareos-filedaemon-python3-plugin (= ${binary:Version}), python3-ldap, ${misc:Depends}
 Description: Backup Archiving Recovery Open Sourced - file daemon LDAP plugin
  Bareos is a set of programs to manage backup, recovery and verification of
  data across a network of computers of different kinds.

diff --git a/docs/manuals/source/TasksAndConcepts/Plugins/FileDaemonPlugins/LDAPPlugin.rst.inc b/docs/manuals/source/TasksAndConcepts/Plugins/FileDaemonPlugins/LDAPPlugin.rst.inc
@@ -8,3 +8,13 @@ This plugin is intended to backup (and restore) the contents of a LDAP server.
 It uses normal LDAP operation for this. The package **bareos-filedaemon-ldap-python-plugin** 
 (:sinceVersion:`15.2.0: LDAP Plugin`) contains an example configuration file,
 that must be adapted to your environment.
+
+Please note that the plugin was tested against an OpenLDAP server. Other LDAP servers
+may behave differently and there might be problems when backing up or restoring objects.
+Most notably, it will not be possible to restore objects to an Active Directory server.
+
+On restore, if the object to be restored already exists on the LDAP server, it will
+be deleted first, then restored from the backup. This could cause problems if your
+LDAP server uses referential integrity (e.g. if a user object is
+restored, the LDAP server might remove the user from all groups when it is
+being deleted and recreated during the restore process).
diff --git a/systemtests/tests/py3plug-fd-ldap/CMakeLists.txt b/systemtests/tests/py3plug-fd-ldap/CMakeLists.txt
@@ -1,6 +1,6 @@
 #   BAREOS® - Backup Archiving REcovery Open Sourced
 #
-#   Copyright (C) 2021-2023 Bareos GmbH & Co. KG
+#   Copyright (C) 2021-2024 Bareos GmbH & Co. KG
 #
 #   This program is Free Software; you can redistribute it and/or
 #   modify it under the terms of version three of the GNU Affero General Public
@@ -39,6 +39,10 @@ set(SYSTEMTEST_LDAP_PASSWORD
 if(TARGET python3-fd)
   check_pymodule_available(3 ldap)
   if(PYMODULE_3_LDAP_FOUND AND SYSTEMTEST_LDAP_ADDRESS)
+    string(REPLACE ":" "\\\\:" SYSTEMTEST_LDAP_ADDRESS_ESCAPED
+                   ${SYSTEMTEST_LDAP_ADDRESS}
+    )
+    cmake_print_variables(SYSTEMTEST_LDAP_ADDRESS_ESCAPED)
     create_systemtest(${SYSTEMTEST_PREFIX} ${BASENAME})
   else()
     create_systemtest(${SYSTEMTEST_PREFIX} ${BASENAME} DISABLED)

diff --git a/systemtests/tests/py3plug-fd-ldap/etc/bareos/bareos-dir.d/fileset/PluginFailingTest.conf.in b/systemtests/tests/py3plug-fd-ldap/etc/bareos/bareos-dir.d/fileset/PluginFailingTest.conf.in
@@ -0,0 +1,10 @@
+FileSet {
+  Name = "PluginFailingTest"
+  Description = "Test the Plugin functionality with a Python Plugin."
+  Include {
+    Options {
+      Signature = XXH128
+    }
+    Plugin = "@python_module_name@:module_path=@python_plugin_module_src_test_dir@:module_name=bareos-fd-ldap:uri=ldap\\://@SYSTEMTEST_LDAP_ADDRESS_ESCAPED@:basedn=ou=backup,@SYSTEMTEST_LDAP_BASEDN@:bind_dn=@SYSTEMTEST_LDAP_BINDDN@:password=INVALID_PASSWORD"
+  }
+}
diff --git a/systemtests/tests/py3plug-fd-ldap/etc/bareos/bareos-dir.d/fileset/PluginTest.conf.in b/systemtests/tests/py3plug-fd-ldap/etc/bareos/bareos-dir.d/fileset/PluginTest.conf.in
@@ -5,6 +5,6 @@ FileSet {
     Options {
       Signature = XXH128
     }
-    Plugin = "@python_module_name@:module_path=@python_plugin_module_src_test_dir@:module_name=bareos-fd-ldap:uri=ldap\\://@SYSTEMTEST_LDAP_ADDRESS@:basedn=ou=backup,@SYSTEMTEST_LDAP_BASEDN@:bind_dn=@SYSTEMTEST_LDAP_BINDDN@:password=@SYSTEMTEST_LDAP_PASSWORD@"
+    Plugin = "@python_module_name@:module_path=@python_plugin_module_src_test_dir@:module_name=bareos-fd-ldap:uri=ldap\\://@SYSTEMTEST_LDAP_ADDRESS_ESCAPED@:basedn=ou=backup,@SYSTEMTEST_LDAP_BASEDN@:bind_dn=@SYSTEMTEST_LDAP_BINDDN@:password=@SYSTEMTEST_LDAP_PASSWORD@"
   }
 }
diff --git a/systemtests/tests/py3plug-fd-ldap/testrunner → ...s/tests/py3plug-fd-ldap/testrunner-backup b/systemtests/tests/py3plug-fd-ldap/testrunner → ...s/tests/py3plug-fd-ldap/testrunner-backup
@@ -8,40 +8,35 @@ set -u
 #
 TestName="$(basename "$(pwd)")"
 export TestName
-bucket_name=bareos-test
 
 JobName=backup-bareos-fd
+
 #shellcheck source=../environment.in
 . ./environment
 
+#shellcheck source=../scripts/functions
+. "${rscripts}"/functions
+
 testdata_opts=( \
   --address  "$SYSTEMTEST_LDAP_ADDRESS" \
   --basedn   "$SYSTEMTEST_LDAP_BASEDN" \
   --binddn   "$SYSTEMTEST_LDAP_BINDDN" \
   --password "$SYSTEMTEST_LDAP_PASSWORD"
 )
 
+start_test
+
 tar xzf ../../data/image.tgz
 ./testdata.py "${testdata_opts[@]}" \
   --clean \
   --populate \
   --dump-backup > backup-data-before.ldif
 
-JobName=backup-bareos-fd
-#shellcheck source=../scripts/functions
-. "${rscripts}"/functions
-"${rscripts}"/cleanup
-"${rscripts}"/setup
-
-
-start_test
-
 cat <<END_OF_DATA >$tmp/bconcmds
 @$out /dev/null
 messages
 @$out $tmp/log1.out
 setdebug level=100 storage=File
-label volume=TestVolume001 storage=File pool=Full
 run job=$JobName yes
 status director
 status client
@@ -60,10 +55,9 @@ messages
 quit
 END_OF_DATA
 
-run_bareos "$@"
+run_bconsole "$@"
 
 check_for_zombie_jobs storage=File
-stop_bareos
 
 check_two_logs