From 8943e57ee6ce56d3c78620ce7472e270f4b2f91b Mon Sep 17 00:00:00 2001
From: Douglas Viroel <viroel@gmail.com>
Date: Fri, 4 Dec 2020 15:10:05 +0000
Subject: [PATCH] [NetApp] Fix security service configuration for LDAP servers

This patch fixes some issues with LDAP client configuration on
ONTAP SVMs. With ldap security service, users should be able to
configure a LDAP client that can be used for authentication and
name mapping. The name service switch order remains: ldap,files.
Issues fixed:
- The driver now identifies when user provide a Active Directory
  domain or a Linux/Unix LDAP server IP and sets the correct schema.
- LDAP configuration parameter `servers` was replaced by `ldap-servers`
  in ONTAP 9.2, and now accepts host names too.
- Fix DNS configuration for LDAP security service
- User can now specify base search DN for LDAP queries, which can be
  mandatory for Unix/Linux servers, using the security service `ou`
  parameter.

Closes-Bug: #1916534
Change-Id: Ieaa53abbe50e7b708e508c132dfc4bb36b71a4f5
Signed-off-by: Douglas Viroel <viroel@gmail.com>
---
 .../netapp/dataontap/client/client_cmode.py   | 163 +++++++++++++++---
 .../drivers/netapp/dataontap/client/fakes.py  |  41 ++++-
 .../dataontap/client/test_client_cmode.py     | 114 +++++++++---
 ...dap-security-service-c8ee6d36598722cf.yaml |  17 ++
 4 files changed, 287 insertions(+), 48 deletions(-)
 create mode 100644 releasenotes/notes/bug-1916534-netapp-fix-ldap-security-service-c8ee6d36598722cf.yaml

diff --git a/manila/share/drivers/netapp/dataontap/client/client_cmode.py b/manila/share/drivers/netapp/dataontap/client/client_cmode.py
index 70ee191a6d..a750e88bf0 100644
--- a/manila/share/drivers/netapp/dataontap/client/client_cmode.py
+++ b/manila/share/drivers/netapp/dataontap/client/client_cmode.py
@@ -31,6 +31,7 @@ from manila.i18n import _
 from manila.share.drivers.netapp.dataontap.client import api as netapp_api
 from manila.share.drivers.netapp.dataontap.client import client_base
 from manila.share.drivers.netapp import utils as na_utils
+from manila import utils as manila_utils
 
 
 LOG = log.getLogger(__name__)
@@ -70,6 +71,7 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
         ontapi_1_2x = (1, 20) <= ontapi_version < (1, 30)
         ontapi_1_30 = ontapi_version >= (1, 30)
         ontapi_1_110 = ontapi_version >= (1, 110)
+        ontapi_1_120 = ontapi_version >= (1, 120)
         ontapi_1_140 = ontapi_version >= (1, 140)
         ontapi_1_150 = ontapi_version >= (1, 150)
 
@@ -91,6 +93,8 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
                                   supported=ontapi_1_140)
         self.features.add_feature('CIFS_DC_ADD_SKIP_CHECK',
                                   supported=ontapi_1_150)
+        self.features.add_feature('LDAP_LDAP_SERVERS',
+                                  supported=ontapi_1_120)
 
     def _invoke_vserver_api(self, na_element, vserver):
         server = copy.copy(self.connection)
@@ -1415,7 +1419,7 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
 
     @na_utils.trace
     def setup_security_services(self, security_services, vserver_client,
-                                vserver_name):
+                                vserver_name, timeout=30):
         api_args = {
             'name-mapping-switch': [
                 {'nmswitch': 'ldap'},
@@ -1431,7 +1435,8 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
 
         for security_service in security_services:
             if security_service['type'].lower() == 'ldap':
-                vserver_client.configure_ldap(security_service)
+                vserver_client.configure_ldap(security_service,
+                                              timeout=timeout)
 
             elif security_service['type'].lower() == 'active_directory':
                 vserver_client.configure_active_directory(security_service,
@@ -1500,22 +1505,97 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
         self.send_request('export-rule-create', export_rule_create_args)
 
     @na_utils.trace
-    def configure_ldap(self, security_service):
-        """Configures LDAP on Vserver."""
+    def _create_ldap_client(self, security_service):
+        ad_domain = security_service.get('domain')
+        ldap_servers = security_service.get('server')
+        bind_dn = security_service.get('user')
+        ldap_schema = 'RFC-2307'
+
+        if ad_domain:
+            if ldap_servers:
+                msg = _("LDAP client cannot be configured with both 'server' "
+                        "and 'domain' parameters. Use 'server' for Linux/Unix "
+                        "LDAP servers or 'domain' for Active Directory LDAP "
+                        "servers.")
+                LOG.exception(msg)
+                raise exception.NetAppException(msg)
+            # RFC2307bis, for MS Active Directory LDAP server
+            ldap_schema = 'MS-AD-BIS'
+            bind_dn = (security_service.get('user') + '@' + ad_domain)
+        else:
+            if not ldap_servers:
+                msg = _("LDAP client cannot be configured without 'server' "
+                        "or 'domain' parameters. Use 'server' for Linux/Unix "
+                        "LDAP servers or 'domain' for Active Directory LDAP "
+                        "server.")
+                LOG.exception(msg)
+                raise exception.NetAppException(msg)
+
+        if security_service.get('dns_ip'):
+            self.configure_dns(security_service)
+
         config_name = hashlib.md5(six.b(security_service['id'])).hexdigest()
+
         api_args = {
             'ldap-client-config': config_name,
-            'servers': {
-                'ip-address': security_service['server'],
-            },
             'tcp-port': '389',
-            'schema': 'RFC-2307',
-            'bind-password': security_service['password'],
+            'schema': ldap_schema,
+            'bind-dn': bind_dn,
+            'bind-password': security_service.get('password'),
         }
+
+        if security_service.get('ou'):
+            api_args['base-dn'] = security_service['ou']
+        if ad_domain:
+            # Active Directory LDAP server
+            api_args['ad-domain'] = ad_domain
+        else:
+            # Linux/Unix LDAP servers
+            if self.features.LDAP_LDAP_SERVERS:
+                servers_key, servers_key_type = 'ldap-servers', 'string'
+            else:
+                servers_key, servers_key_type = 'servers', 'ip-address'
+
+            api_args[servers_key] = []
+            for server in ldap_servers.split(','):
+                api_args[servers_key].append(
+                    {servers_key_type: server.strip()})
+
         self.send_request('ldap-client-create', api_args)
 
-        api_args = {'client-config': config_name, 'client-enabled': 'true'}
-        self.send_request('ldap-config-create', api_args)
+    @na_utils.trace
+    def _enable_ldap_client(self, client_config_name, timeout=30):
+        # ONTAP ldap query timeout is 3 seconds by default
+        interval = 3
+        retries = int(timeout / interval) or 1
+        api_args = {'client-config': client_config_name,
+                    'client-enabled': 'true'}
+
+        @manila_utils.retry(exception.ShareBackendException, interval=interval,
+                            retries=retries, backoff_rate=1)
+        def try_enable_ldap_client():
+            try:
+                self.send_request('ldap-config-create', api_args)
+            except netapp_api.NaApiError as e:
+                msg = _('Unable to enable ldap client configuration. Will '
+                        'retry the operation. Error details: %s') % e.message
+                LOG.warning(msg)
+                raise exception.ShareBackendException(msg=msg)
+
+        try:
+            try_enable_ldap_client()
+        except exception.ShareBackendException:
+            msg = _("Unable to enable ldap client configuration %s. "
+                    "Retries exhausted. Aborting.") % client_config_name
+            LOG.exception(msg)
+            raise exception.NetAppException(message=msg)
+
+    @na_utils.trace
+    def configure_ldap(self, security_service, timeout=30):
+        """Configures LDAP on Vserver."""
+        config_name = hashlib.md5(six.b(security_service['id'])).hexdigest()
+        self._create_ldap_client(security_service)
+        self._enable_ldap_client(config_name, timeout=timeout)
 
     @na_utils.trace
     def configure_active_directory(self, security_service, vserver_name):
@@ -1679,24 +1759,65 @@ class NetAppCmodeClient(client_base.NetAppBaseClient):
 
     @na_utils.trace
     def configure_dns(self, security_service):
+        """Configure DNS address and servers for a vserver."""
         api_args = {
-            'domains': {
-                'string': security_service['domain'],
-            },
+            'domains': [],
             'name-servers': [],
             'dns-state': 'enabled',
         }
+        # NOTE(dviroel): Read the current dns configuration and merge with the
+        # new one. This scenario is expected when 2 security services provide
+        # a DNS configuration, like 'active_directory' and 'ldap'.
+        current_dns_config = self.get_dns_config()
+        domains = set(current_dns_config.get('domains', []))
+        dns_ips = set(current_dns_config.get('dns-ips', []))
+
+        domains.add(security_service['domain'])
+        for domain in domains:
+            api_args['domains'].append({'string': domain})
+
         for dns_ip in security_service['dns_ip'].split(','):
-            api_args['name-servers'].append({'ip-address': dns_ip.strip()})
+            dns_ips.add(dns_ip.strip())
+        for dns_ip in dns_ips:
+            api_args['name-servers'].append({'ip-address': dns_ip})
 
         try:
-            self.send_request('net-dns-create', api_args)
-        except netapp_api.NaApiError as e:
-            if e.code == netapp_api.EDUPLICATEENTRY:
-                LOG.error("DNS exists for Vserver.")
+            if current_dns_config:
+                self.send_request('net-dns-modify', api_args)
             else:
-                msg = _("Failed to configure DNS. %s")
-                raise exception.NetAppException(msg % e.message)
+                self.send_request('net-dns-create', api_args)
+        except netapp_api.NaApiError as e:
+            msg = _("Failed to configure DNS. %s")
+            raise exception.NetAppException(msg % e.message)
+
+    @na_utils.trace
+    def get_dns_config(self):
+        """Read DNS servers and domains currently configured in the vserverĀ·"""
+        api_args = {}
+        try:
+            result = self.send_request('net-dns-get', api_args)
+        except netapp_api.NaApiError as e:
+            if e.code == netapp_api.EOBJECTNOTFOUND:
+                return {}
+            msg = _("Failed to retrieve DNS configuration. %s")
+            raise exception.NetAppException(msg % e.message)
+
+        dns_config = {}
+        attributes = result.get_child_by_name('attributes')
+        dns_info = attributes.get_child_by_name('net-dns-info')
+
+        dns_config['dns-state'] = dns_info.get_child_content(
+            'dns-state')
+        domains = dns_info.get_child_by_name(
+            'domains') or netapp_api.NaElement('None')
+        dns_config['domains'] = [domain.get_content()
+                                 for domain in domains.get_children()]
+
+        servers = dns_info.get_child_by_name(
+            'name-servers') or netapp_api.NaElement('None')
+        dns_config['dns-ips'] = [server.get_content()
+                                 for server in servers.get_children()]
+        return dns_config
 
     @na_utils.trace
     def set_preferred_dc(self, security_service):
diff --git a/manila/tests/share/drivers/netapp/dataontap/client/fakes.py b/manila/tests/share/drivers/netapp/dataontap/client/fakes.py
index cceb7f7339..15f3da1728 100644
--- a/manila/tests/share/drivers/netapp/dataontap/client/fakes.py
+++ b/manila/tests/share/drivers/netapp/dataontap/client/fakes.py
@@ -462,11 +462,26 @@ CIFS_SECURITY_SERVICE = {
     'server': '',
 }
 
-LDAP_SECURITY_SERVICE = {
+LDAP_LINUX_SECURITY_SERVICE = {
+    'id': 'fake_id',
     'type': 'ldap',
+    'user': 'fake_user',
     'password': 'fake_password',
     'server': 'fake_server',
+    'ou': 'fake_ou',
+    'dns_ip': None,
+    'domain': None
+}
+
+LDAP_AD_SECURITY_SERVICE = {
     'id': 'fake_id',
+    'type': 'ldap',
+    'user': 'fake_user',
+    'password': 'fake_password',
+    'domain': 'fake_domain',
+    'ou': 'fake_ou',
+    'dns_ip': 'fake_dns_ip',
+    'server': None,
 }
 
 KERBEROS_SECURITY_SERVICE = {
@@ -2727,6 +2742,30 @@ KERBEROS_CONFIG_GET_RESPONSE = etree.XML("""
     'vserver_name': VSERVER_NAME,
 })
 
+DNS_CONFIG_GET_RESPONSE = etree.XML("""
+  <results status="passed">
+    <attributes>
+      <net-dns-info>
+        <attempts>1</attempts>
+        <dns-state>enabled</dns-state>
+        <domains>
+          <string>fake_domain.com</string>
+        </domains>
+        <is-tld-query-enabled>true</is-tld-query-enabled>
+        <name-servers>
+          <ip-address>fake_dns_1</ip-address>
+          <ip-address>fake_dns_2</ip-address>
+        </name-servers>
+        <require-packet-query-match>true</require-packet-query-match>
+        <require-source-address-match>true</require-source-address-match>
+        <timeout>2</timeout>
+        <vserver-name>%(vserver_name)s</vserver-name>
+      </net-dns-info>
+    </attributes>
+  </results>""" % {
+    'vserver_name': VSERVER_NAME,
+})
+
 FAKE_VOL_XML = """<volume-info>
     <name>open123</name>
     <state>online</state>
diff --git a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py
index cedf7d0d61..cb9cab3f41 100644
--- a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py
+++ b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py
@@ -2326,7 +2326,7 @@ class NetAppClientCmodeTestCase(test.TestCase):
         self.mock_object(self.client, 'send_request')
         self.mock_object(self.vserver_client, 'configure_ldap')
 
-        self.client.setup_security_services([fake.LDAP_SECURITY_SERVICE],
+        self.client.setup_security_services([fake.LDAP_LINUX_SECURITY_SERVICE],
                                             self.vserver_client,
                                             fake.VSERVER_NAME)
 
@@ -2344,7 +2344,7 @@ class NetAppClientCmodeTestCase(test.TestCase):
         self.client.send_request.assert_has_calls([
             mock.call('vserver-modify', vserver_modify_args)])
         self.vserver_client.configure_ldap.assert_has_calls([
-            mock.call(fake.LDAP_SECURITY_SERVICE)])
+            mock.call(fake.LDAP_LINUX_SECURITY_SERVICE, timeout=30)])
 
     def test_setup_security_services_active_directory(self):
 
@@ -2509,22 +2509,38 @@ class NetAppClientCmodeTestCase(test.TestCase):
             mock.call('export-rule-create', export_rule_create_args),
             mock.call('export-rule-create', export_rule_create_args2)])
 
-    def test_configure_ldap(self):
+    @ddt.data(fake.LDAP_LINUX_SECURITY_SERVICE, fake.LDAP_AD_SECURITY_SERVICE)
+    def test_configure_ldap(self, sec_service):
+        self.client.features.add_feature('LDAP_LDAP_SERVERS')
 
         self.mock_object(self.client, 'send_request')
+        self.mock_object(self.client, 'configure_dns')
 
-        self.client.configure_ldap(fake.LDAP_SECURITY_SERVICE)
+        self.client.configure_ldap(sec_service)
 
-        config_name = hashlib.md5(
-            six.b(fake.LDAP_SECURITY_SERVICE['id'])).hexdigest()
+        config_name = hashlib.md5(six.b(sec_service['id'])).hexdigest()
 
         ldap_client_create_args = {
             'ldap-client-config': config_name,
-            'servers': {'ip-address': fake.LDAP_SECURITY_SERVICE['server']},
             'tcp-port': '389',
-            'schema': 'RFC-2307',
-            'bind-password': fake.LDAP_SECURITY_SERVICE['password']
+            'bind-password': sec_service['password'],
         }
+
+        if sec_service.get('domain'):
+            ldap_client_create_args['schema'] = 'MS-AD-BIS'
+            ldap_client_create_args['bind-dn'] = (
+                sec_service['user'] + '@' + sec_service['domain'])
+            ldap_client_create_args['ad-domain'] = sec_service['domain']
+        else:
+            ldap_client_create_args['schema'] = 'RFC-2307'
+            ldap_client_create_args['bind-dn'] = sec_service['user']
+            ldap_client_create_args['ldap-servers'] = [{
+                'string': sec_service['server']
+            }]
+
+        if sec_service.get('ou'):
+            ldap_client_create_args['base-dn'] = sec_service['ou']
+
         ldap_config_create_args = {
             'client-config': config_name,
             'client-enabled': 'true'
@@ -2534,6 +2550,32 @@ class NetAppClientCmodeTestCase(test.TestCase):
             mock.call('ldap-client-create', ldap_client_create_args),
             mock.call('ldap-config-create', ldap_config_create_args)])
 
+    @ddt.data({'server': None, 'domain': None},
+              {'server': 'fake_server', 'domain': 'fake_domain'})
+    @ddt.unpack
+    def test_configure_ldap_invalid_parameters(self, server, domain):
+        fake_ldap_sec_service = copy.deepcopy(fake.LDAP_AD_SECURITY_SERVICE)
+        fake_ldap_sec_service['server'] = server
+        fake_ldap_sec_service['domain'] = domain
+
+        self.assertRaises(exception.NetAppException,
+                          self.client.configure_ldap,
+                          fake_ldap_sec_service)
+
+    def test__enable_ldap_client_timeout(self):
+        mock_warning_log = self.mock_object(client_cmode.LOG, 'warning')
+        na_api_error = netapp_api.NaApiError(code=netapp_api.EAPIERROR)
+        mock_send_request = self.mock_object(
+            self.client, 'send_request', mock.Mock(side_effect=na_api_error))
+
+        self.assertRaises(exception.NetAppException,
+                          self.client._enable_ldap_client,
+                          'fake_config_name',
+                          timeout=6)
+
+        self.assertEqual(2, mock_send_request.call_count)
+        self.assertEqual(2, mock_warning_log.call_count)
+
     def test_configure_active_directory(self):
 
         self.mock_object(self.client, 'send_request')
@@ -2767,11 +2809,13 @@ class NetAppClientCmodeTestCase(test.TestCase):
     def test_configure_dns_for_active_directory(self):
 
         self.mock_object(self.client, 'send_request')
+        self.mock_object(self.client, 'get_dns_config',
+                         mock.Mock(return_value={}))
 
         self.client.configure_dns(fake.CIFS_SECURITY_SERVICE)
 
         net_dns_create_args = {
-            'domains': {'string': fake.CIFS_SECURITY_SERVICE['domain']},
+            'domains': [{'string': fake.CIFS_SECURITY_SERVICE['domain']}],
             'name-servers': [{
                 'ip-address': fake.CIFS_SECURITY_SERVICE['dns_ip']
             }],
@@ -2784,29 +2828,26 @@ class NetAppClientCmodeTestCase(test.TestCase):
     def test_configure_dns_multiple_dns_ip(self):
 
         self.mock_object(self.client, 'send_request')
+        self.mock_object(self.client, 'get_dns_config',
+                         mock.Mock(return_value={}))
         mock_dns_ips = ['10.0.0.1', '10.0.0.2', '10.0.0.3']
         security_service = fake.CIFS_SECURITY_SERVICE
         security_service['dns_ip'] = ', '.join(mock_dns_ips)
 
         self.client.configure_dns(security_service)
 
-        net_dns_create_args = {
-            'domains': {'string': security_service['domain']},
-            'dns-state': 'enabled',
-            'name-servers': [{'ip-address': dns_ip} for dns_ip in mock_dns_ips]
-        }
-
-        self.client.send_request.assert_has_calls([
-            mock.call('net-dns-create', net_dns_create_args)])
+        self.client.send_request.assert_called_once()
 
     def test_configure_dns_for_kerberos(self):
 
         self.mock_object(self.client, 'send_request')
+        self.mock_object(self.client, 'get_dns_config',
+                         mock.Mock(return_value={}))
 
         self.client.configure_dns(fake.KERBEROS_SECURITY_SERVICE)
 
         net_dns_create_args = {
-            'domains': {'string': fake.KERBEROS_SECURITY_SERVICE['domain']},
+            'domains': [{'string': fake.KERBEROS_SECURITY_SERVICE['domain']}],
             'name-servers': [{
                 'ip-address': fake.KERBEROS_SECURITY_SERVICE['dns_ip']
             }],
@@ -2817,15 +2858,19 @@ class NetAppClientCmodeTestCase(test.TestCase):
             mock.call('net-dns-create', net_dns_create_args)])
 
     def test_configure_dns_already_present(self):
-
-        self.mock_object(self.client,
-                         'send_request',
-                         self._mock_api_error(code=netapp_api.EDUPLICATEENTRY))
+        dns_config = {
+            'dns-state': 'enabled',
+            'domains': [fake.KERBEROS_SECURITY_SERVICE['domain']],
+            'dns-ips': [fake.KERBEROS_SECURITY_SERVICE['dns_ip']],
+        }
+        self.mock_object(self.client, 'get_dns_config',
+                         mock.Mock(return_value=dns_config))
+        self.mock_object(self.client, 'send_request')
 
         self.client.configure_dns(fake.KERBEROS_SECURITY_SERVICE)
 
         net_dns_create_args = {
-            'domains': {'string': fake.KERBEROS_SECURITY_SERVICE['domain']},
+            'domains': [{'string': fake.KERBEROS_SECURITY_SERVICE['domain']}],
             'name-servers': [{
                 'ip-address': fake.KERBEROS_SECURITY_SERVICE['dns_ip']
             }],
@@ -2833,17 +2878,34 @@ class NetAppClientCmodeTestCase(test.TestCase):
         }
 
         self.client.send_request.assert_has_calls([
-            mock.call('net-dns-create', net_dns_create_args)])
-        self.assertEqual(1, client_cmode.LOG.error.call_count)
+            mock.call('net-dns-modify', net_dns_create_args)])
 
     def test_configure_dns_api_error(self):
 
         self.mock_object(self.client, 'send_request', self._mock_api_error())
+        self.mock_object(self.client, 'get_dns_config',
+                         mock.Mock(return_value={}))
 
         self.assertRaises(exception.NetAppException,
                           self.client.configure_dns,
                           fake.KERBEROS_SECURITY_SERVICE)
 
+    def test_get_dns_configuration(self):
+        api_response = netapp_api.NaElement(
+            fake.DNS_CONFIG_GET_RESPONSE)
+        self.mock_object(self.client, 'send_request',
+                         mock.Mock(return_value=api_response))
+
+        result = self.client.get_dns_config()
+
+        expected_result = {
+            'dns-state': 'enabled',
+            'domains': ['fake_domain.com'],
+            'dns-ips': ['fake_dns_1', 'fake_dns_2']
+        }
+        self.assertEqual(expected_result, result)
+        self.client.send_request.assert_called_once_with('net-dns-get', {})
+
     @ddt.data(
         {
             'server': '',
diff --git a/releasenotes/notes/bug-1916534-netapp-fix-ldap-security-service-c8ee6d36598722cf.yaml b/releasenotes/notes/bug-1916534-netapp-fix-ldap-security-service-c8ee6d36598722cf.yaml
new file mode 100644
index 0000000000..0736ad388d
--- /dev/null
+++ b/releasenotes/notes/bug-1916534-netapp-fix-ldap-security-service-c8ee6d36598722cf.yaml
@@ -0,0 +1,17 @@
+---
+fixes:
+  - |
+    NetApp ONTAP driver is now fixed to properly configure SVM LDAP client when
+    configuration is provided through `ldap` security service. Now, the driver
+    chooses the correct LDAP schema based on the given security service
+    parameters. The `RFC-2307` schema will be set for Linux/Unix LDAP servers
+    and `RFC-2307bis` for Active Directory servers. When using a Linux/Unix
+    LDAP server, the security service should be configured setting the
+    `server` parameter with servers IPs or host names. For Active
+    Directory LDAP server, the domain information must be configured using the
+    the `domain` parameter. Users should provide at least one DNS server when
+    configuring servers by its host or domain names. The base search
+    `distinguished name` used for LDAP queries can now be configured using
+    security service `ou` parameter. Please refer to
+    `Launchpad Bug #1916534 <https://bugs.launchpad.net/manila/+bug/1916534>`_
+    for more details.