PR#50460: Issue 50396 - segfault when using pam passthru and addn plugins together - 389-ds-base

389-ds-base

#50460 Issue 50396 - segfault when using pam passthru and addn plugins together

Closed 3 years ago by spichugi. Opened 4 years ago by vashirov.

vashirov/389-ds-base addn-tests into master

Issue 50396 - segfault when using pam passthru and addn plugins together

Viktor Ashirov • 3 years ago

83e9f05

dirsrvtests/tests/suites/plugins/acceptance_test.py

file modified

+89 -1

		`@@ -29,6 +29,7 @@`
		`log = logging.getLogger(__name__)`

		`USER_DN = 'uid=test_user_1001,ou=people,dc=example,dc=com'`
		`+ USER_ADDN = 'test_user_1001'`
		`USER_PW = 'password'`
		`GROUP_DN = 'cn=group,' + DEFAULT_SUFFIX`
		`CONFIG_AREA = 'nsslapd-pluginConfigArea'`
		`@@ -1792,10 +1793,97 @@`
		`return`


		`+ def test_addn(topo, args=None):`
		`+ """Test AD DN plugin basic functionality`
		`+`
		`+ :id: 106ee95e-93ab-11e9-9dd6-3497f624ea11`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Create a test user`
		`+ 2. Bind using only the uid`
		`+ 3. Bind using uid@example.com`
		`+ 4. Bind using uid@example.org (second domain, non-default)`
		`+ 5. Bind using uid@nonexistent.domain`
		`+ 6. Bind as Root DN`
		`+ 7. Configure plugin`
		`+ 8. Create a default domain config entry`
		`+ 9. Create a second domain config entry`
		`+ 10. Enable plugin and restart the instance`
		`+ 11. Bind using only the uid`
		`+ 12. Bind using uid@example.com`
		`+ 13. Bind using uid@example.org (second domain, non-default)`
		`+ 14. Bind using uid@nonexistent.domain`
		`+ 15. Clean up`
		`+ :expectedresults:`
		`+ 1. Success`
		`+ 2. Failure`
		`+ 3. Failure`
		`+ 4. Failure`
		`+ 5. Failure`
		`+ 6. Success`
		`+ 7. Success`
		`+ 8. Success`
		`+ 9. Success`
		`+ 10. Success`
		`+ 11. Success`
		`+ 12. Success`
		`+ 13. Success`
		`+ 14. Failure`
		`+ 15. Success`
		`+ """`
		`+`
		`+ inst = topo[0]`
		`+`
		`+ users = UserAccounts(inst, DEFAULT_SUFFIX)`
		`+ user1 = users.create_test_user(uid=1001)`
		`+ user1.replace('userPassword', USER_PW)`
		`+`
		`+ with pytest.raises(ldap.LDAPError):`
		`+ assert inst.simple_bind_s(USER_ADDN, USER_PW)`
		`+ with pytest.raises(ldap.LDAPError):`
		`+ assert inst.simple_bind_s(f'{USER_ADDN}@example.com', USER_PW)`
		`+ with pytest.raises(ldap.LDAPError):`
		`+ assert inst.simple_bind_s(f'{USER_ADDN}@example.org', USER_PW)`
		`+ with pytest.raises(ldap.LDAPError):`
		`+ assert inst.simple_bind_s(f'{USER_ADDN}@nonexistent.domain', USER_PW)`
		`+ inst.simple_bind_s(DN_DM, PASSWORD)`
		`+`
		`+ # stop the plugin, and start it`
		`+ plugin = AddnPlugin(inst)`
		`+ plugin.create(rdn='addn')`
		`+ plugin.set_default_domain('example.com')`
		`+ addn_domains = AddnDomains(inst)`
		`+ addn_domains.create(`
		`+ properties={'cn': 'example.com',`
		`+ 'addn_base': 'ou=people,dc=example,dc=com',`
		`+ 'addn_filter': '(&(objectClass=account)(uid=%s))',})`
		`+ addn_domains.create(`
		`+ properties={'cn': 'example.org',`
		`+ 'addn_base': 'ou=people,dc=example,dc=com',`
		`+ 'addn_filter': '(&(objectClass=account)(uid=%s))',})`
		`+ plugin.enable()`
		`+`
		`+ if args == "restart":`
		`+ return`
		`+`
		`+ if args is None:`
		`+ inst.restart()`
		`+`
		`+ assert inst.simple_bind_s(USER_ADDN, USER_PW)`
		`+ assert inst.simple_bind_s(f'{USER_ADDN}@example.com', USER_PW)`
		`+ assert inst.simple_bind_s(f'{USER_ADDN}@example.org', USER_PW)`
		`+ with pytest.raises(ldap.LDAPError):`
		`+ assert inst.simple_bind_s(f'{USER_ADDN}@nonexistent.domain', USER_PW)`
		`+`
		`+ # Delete user`
		`+ inst.simple_bind_s(DN_DM, PASSWORD)`
		`+ user1.delete()`
		`+`
		`+`
		`# Array of test functions`
		`func_tests = [test_acctpolicy, test_attruniq, test_automember, test_dna,`
		`test_linkedattrs, test_memberof, test_mep, test_passthru,`
		`- test_referint, test_retrocl, test_rootdn]`
		`+ test_referint, test_retrocl, test_rootdn, test_addn]`


		`def check_all_plugins(topo, args="online"):`

dirsrvtests/tests/suites/plugins/regression_test.py

file added

+60

		`@@ -0,0 +1,60 @@`
		`+ # --- BEGIN COPYRIGHT BLOCK ---`
		`+ # Copyright (C) 2019 Red Hat, Inc.`
		`+ # All rights reserved.`
		`+ #`
		`+ # License: GPL (version 3 or any later version).`
		`+ # See LICENSE for details.`
		`+ # --- END COPYRIGHT BLOCK ---`
		`+ #`
		`+ import ldap`
		`+ import logging`
		`+ import pytest`
		`+ from lib389.plugins import PAMPassThroughAuthPlugin, AddnPlugin, AddnDomains`
		`+ from lib389.topologies import topology_st as topo`
		`+`
		`+ pytestmark = pytest.mark.tier1`
		`+`
		`+ log = logging.getLogger(__name__)`
		`+`
		`+ @pytest.mark.ds50396`
		`+ @pytest.mark.bz1701092`
		`+ def test_crash_in_pam_pta_plugin_when_user_doesnt_exist(topo):`
		`+ """Test that the server doesn't crash when user doesn't exist`
		`+`
		`+ :id: cb362b70-9424-11e9-a56a-54e1ad30572c`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Enable PAM PTA plugin`
		`+ 2. Enable and configure AD DN plugin`
		`+ 3. Restart the instance`
		`+ 4. Bind as non-existent user`
		`+ :expectedresults:`
		`+ 1. Success`
		`+ 2. Success`
		`+ 3. Success`
		`+ 4. Server shouldn't crash`
		`+ """`
		`+`
		`+ inst = topo.standalone`
		`+`
		`+ # Enable PAM PTA plugin`
		`+ pampta_plugin = PAMPassThroughAuthPlugin(inst)`
		`+ pampta_plugin.enable()`
		`+`
		`+ # Enable and configure AD DN plugin`
		`+ addn_plugin = AddnPlugin(inst)`
		`+ addn_plugin.create(rdn='addn')`
		`+ addn_plugin.set_default_domain('example.com')`
		`+ addn_domains = AddnDomains(inst)`
		`+ addn_domains.create(`
		`+ properties={'cn': 'example.com',`
		`+ 'addn_base': 'ou=people,dc=example,dc=com',`
		`+ 'addn_filter': '(&(objectClass=account)(uid=%s))',})`
		`+ addn_plugin.enable()`
		`+`
		`+ # Restart the instance to enable plugins`
		`+ inst.restart()`
		`+`
		`+ # Expect the following bind to fail, but not crash the server (ldap.SERVER_DOWN)`
		`+ with pytest.raises(ldap.OPERATIONS_ERROR):`
		`+ assert inst.simple_bind_s('bob@example.com', 'password')`

src/lib389/lib389/plugins.py

file modified

+86 -3

		`@@ -100,17 +100,100 @@`


		`class AddnPlugin(Plugin):`
		`- """An instance of addn plugin entry`
		`+ """An instance of ADDN plugin entry`

		`:param instance: An instance`
		`:type instance: lib389.DirSrv`
		`:param dn: Entry DN`
		`:type dn: str`
		`"""`
		`+ _plugin_properties = {`
		`+ 'nsslapd-pluginEnabled': 'off',`
		`+ 'nsslapd-pluginPath': 'libaddn-plugin',`
		`+ 'nsslapd-pluginInitfunc': 'addn_init',`
		`+ 'nsslapd-pluginType': 'preoperation',`
		`+ 'nsslapd-pluginId': 'addn',`
		`+ 'nsslapd-pluginVendor': '389 Project',`
		`+ 'nsslapd-pluginVersion': 'none',`
		`+ 'nsslapd-pluginDescription': 'Allow AD DN style bind names to LDAP',`
		`+ }`

		`def __init__(self, instance, dn="cn=addn,cn=plugins,cn=config"):`
		`super(AddnPlugin, self).__init__(instance, dn)`
		`- # Need to add wrappers to add domains to this.`
		`+ self._create_objectclasses = ['top', 'nsslapdplugin', 'extensibleObject']`
		`+`
		`+ def set_default_domain(self, attr):`
		`+ """Add addn_default_domain attribute"""`
		`+`
		`+ self.set('addn_default_domain', attr)`
		`+`
		`+ def get_default_domain(self):`
		`+ """Get addn_default_domain attribute"""`
		`+`
		`+ return self.get_attr_val_utf8('addn_default_domain')`
		`+`
		`+ class AddnDomain(DSLdapObject):`
		`+ """A single instance of ADDN plugin domain config entry`
		`+`
		`+ :param instance: An instance`
		`+ :type instance: lib389.DirSrv`
		`+ :param dn: Entry DN`
		`+ :type dn: str`
		`+ """`
		`+`
		`+ def __init__(self, instance, dn):`
		`+ super(AddnDomain, self).__init__(instance, dn)`
		`+ self._rdn_attribute = 'cn'`
		`+ self._must_attributes = ['cn','addn_base', 'addn_filter']`
		`+ self._create_objectclasses = ['top', 'extensibleObject']`
		`+ self._protected = False`
		`+`
		`+ class AddnDomains(DSLdapObjects):`
		`+ """A DSLdapObjects entity which represents ADDN plugin domain config entry`
		`+`
		`+ :param instance: An instance`
		`+ :type instance: lib389.DirSrv`
		`+ :param basedn: Base DN for all account entries below`
		`+ :type basedn: str`
		`+ """`
		`+`
		`+ def __init__(self, instance, basedn=None):`
		`+ super(AddnDomains, self).__init__(instance)`
		`+ self._objectclasses = ['top', 'extensibleObject']`
		`+ self._filterattrs = ['cn']`
		`+ self._childobject = AddnDomain`
		`+ # So we can set the configArea easily`
		`+ if basedn is None:`
		`+ basedn = "cn=addn,cn=plugins,cn=config"`
		`+ self._basedn = basedn`
		`+`
		`+ def list(self):`
		`+ """Get a list of children entries (DSLdapObject, Replica, etc.) using a base DN`
		`+ and objectClasses of our object (DSLdapObjects, Replicas, etc.)`
		`+`
		`+ :returns: A list of children entries`
		`+ """`
		`+`
		`+ # Filter based on the objectclasses and the basedn`
		`+ insts = None`
		`+ # This will yield and & filter for objectClass with as many terms as needed.`
		`+ filterstr = self._get_objectclass_filter()`
		`+ self._log.debug('list filter = %s' % filterstr)`
		`+ try:`
		`+ results = self._instance.search_ext_s(`
		`+ base=self._basedn,`
		`+ scope=self._scope,`
		`+ filterstr=filterstr,`
		`+ attrlist=self._list_attrlist,`
		`+ serverctrls=self._server_controls, clientctrls=self._client_controls,`
		`+ escapehatch='i am sure'`
		`+ )`
		`+ # def __init__(self, instance, dn=None):`
		`+ insts = [self._entry_to_instance(dn=r.dn, entry=r) for r in results]`
		`+ except ldap.NO_SUCH_OBJECT:`
		`+ # There are no objects to select from, se we return an empty array`
		`+ insts = []`
		`+ return insts`


		`class AttributeUniquenessPlugin(Plugin):`
		`@@ -1016,7 +1099,7 @@`


		`class SevenBitCheckPlugin(Plugin):`
		`- """An instance of addn plugin entry`
		`+ """An instance of 7-bit check plugin entry`

		`:param instance: An instance`
		`:type instance: lib389.DirSrv`

vashirov commented 4 years ago

Description:

Add missing test for addn plugin in acceptance test module
Add regression test for #50396

Relates: https://pagure.io/389-ds-base/issue/50396

Reviewed by: ???

vashirov commented 4 years ago

@firstyear, while adding acceptance tests for AD DN plugin, I found that we don't have the default ldif for it in template-dse.ldif. Do you think it should stay that way or we can add it there (off by default, of course)?

mreynolds commented 4 years ago

LGTM

firstyear commented 4 years ago

@vashirov I'm of two minds here. I really hate template-dse.ldif because it locks us into long-term configuration syntaxes because they aren't upgraded and come from old installs. Older installs also don't have new plugin configs added either so it's non obvious. template-dse.ldif is not documentation, it really should be "the minimal required server to make dscreate work".

I think maybe I'd prefer that dsconf ... plugin addn enable should be the correct way to "add" the required cn=config object, and we have an example ldif in documentation. I think that's the most sustainable approach long term.

vashirov commented 4 years ago

@firstyear, I'm fine with the current approach. I was asking because I want to have a more robust and consistent behavior of creating and configuring plugins. Since some plugins are already present, I can configure them right away. And for some, like AD DN, I need to call .create() first to ensure that the DN exists before modifying attributes there. So maybe it's more for lib389 to handle this correctly, since we can't guarantee for sure that the plugin's DN will be present in cn=config by default.

rebased onto 035720d3e5cdf21ffe550bf733133e2bc0058e95

4 years ago

rebased onto 28cbbea71bcdf62b53536544dcb4dd8b2d01a68a

4 years ago

vashirov commented 4 years ago

Dynamic plugins test suite fails because of this PR, investigating.

firstyear commented 4 years ago

@vashirov there is a stateful method in lib389, .ensure or something like this. It says "create or modify to make this thing look like this". I think that may be what you want here.

Really I think as a team we need to talk about getting our house in order and how we should be configuring and setting up the server and plugins, because it's such a fragmented and chaotic process today, lib389 has helped a lot, but we need to work out what's best here.

firstyear commented 4 years ago

Is there anything outstanding here before we can merge?

rebased onto a4a4d3b66d74884347cb6ba8c5f4e8b9c87e36c3

4 years ago

vashirov commented 3 years ago