On 03/07/2017 05:05 PM, Dan Lavu wrote:
Updated.
Thanks! Please see my comments below.
On 03/07/2017 05:05 PM, Dan Lavu wrote:
diff --git src/tests/intg/ds.py src/tests/intg/ds.py index 66cb887..13c0778 100644 --- src/tests/intg/ds.py +++ src/tests/intg/ds.py @@ -23,7 +23,7 @@ import ldap class DS: """Abstract directory server instance."""
- def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
- def __init__(self, dir, port, base_dn, admin_rdn, admin_pw, ssl_port=None, ca=None): """ Initialize the instance.
@@ -37,11 +37,17 @@ class DS: """ self.dir = dir self.port = port
self.ldap_url = "ldap://localhost:" + str(self.port) self.base_dn = base_dn self.admin_rdn = admin_rdn self.admin_dn = admin_rdn + "," + base_dn self.admin_pw = admin_pw
self.ssl_port = ssl_port
self.ca = ca
if self.ssl_port and self.ca:
self.ldap_url = "ldaps://localhost:" + str(self.ssl_port)
else:
self.ldap_url = "ldap://localhost:" + str(self.port)
Can you make it always have LDAP URL in ldap_url, and LDAPS URL in ldaps_url, if we have ssl_port and ca?
I.e.
self.ldap_url = "ldap://localhost:" + str(self.port) if self.ssl_port and self.ca: self.ldaps_url = "ldaps://localhost:" + str(self.ssl_port)
def setup(self): """Setup the instance"""
diff --git src/tests/intg/ds_openldap.py src/tests/intg/ds_openldap.py index 6a074c9..a5b2152 100644 --- src/tests/intg/ds_openldap.py +++ src/tests/intg/ds_openldap.py @@ -25,7 +25,10 @@ import os import errno import signal import shutil +import socket +import urllib import sys +import ca_openssl from util import * from ds import DS
@@ -47,7 +50,7 @@ def hash_password(password): class DSOpenLDAP(DS): """OpenLDAP directory server instance."""
- def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
- def __init__(self, dir, port, base_dn, admin_rdn, admin_pw, ssl_port=None, ca=False):
I believe it should be ca=None here as well.
""" Initialize the instance.
@@ -59,12 +62,14 @@ class DSOpenLDAP(DS): admin_rdn Administrator DN, relative to BASE_DN. admin_pw Administrator password. """
DS.__init__(self, dir, port, base_dn, admin_rdn, admin_pw)
DS.__init__(self, dir, port, base_dn, admin_rdn, admin_pw, ca) self.run_dir = self.dir + "/var/run/ldap" self.pid_path = self.run_dir + "/slapd.pid" self.conf_dir = self.dir + "/etc/ldap" self.conf_slapd_d_dir = self.conf_dir + "/slapd.d" self.data_dir = self.dir + "/var/lib/ldap"
self.ssl_port = ssl_port
self.ca = ca
You don't need to set ssl_port and ca here, that's done by the parent's constructor. Please remove.
def _setup_config(self): """Setup the instance initial configuration."""
@@ -78,9 +83,6 @@ class DSOpenLDAP(DS): uid = os.geteuid() gid = os.getegid()
#
# Add configuration
#
Why does this need to be removed?
config = unindent(""" dn: cn=config objectClass: olcGlobal
@@ -223,11 +225,23 @@ class DSOpenLDAP(DS): break except ldap.SERVER_DOWN: pass
attempt = attempt + 1
attempt += 1
Could you please make this a separate commit to keep changes to the point?
if attempt > 30: raise Exception("Failed to start slapd") time.sleep(1)
if self.ca:
self.ca_hostname = socket.gethostname()
self.ca_inst = ca_openssl.CAOpenSSL(config.PREFIX, self.ca_hostname,
"US",
"NC",
"Raleigh",
"Red Hat Inc.",
"SSSD")
self.ca_inst.setup()
self.ca_inst.gen_cert(self.ca_inst.gen_csr(self.ca_hostname), self.ca_hostname)
self.enable_ssl(self.ca_inst.ca_cert_file, self.ca_inst.ca_cert_file, self.ca_inst.ca_cert_key_file)
By "passing CA to the constructor" I meant pass an actual CA instance to the constructor. I.e. CA should be created and set up outside the DS, in the tests. Can you do that?
# # Relax requirement of member attribute presence in groupOfNames #
@@ -277,7 +291,7 @@ class DSOpenLDAP(DS): pid_file.close() attempt = 0 while os.path.isfile(self.pid_path):
attempt = attempt + 1
attempt += 1
Same here, please make a separate commit.
if attempt > 30: raise Exception("Failed to stop slapd") time.sleep(1)
@@ -287,3 +301,29 @@ class DSOpenLDAP(DS):
for path in (self.conf_slapd_d_dir, self.run_dir, self.data_dir): shutil.rmtree(path, True)
if self.ca:
self.ca_inst.teardown()
If you pass the CA from the outside, you no longer will have to care about tearing it down.
- def enable_ssl(self, ca_cert, cert, key):
"""Enable SSL in OpenLDAP"""
ldapi_socket = self.run_dir + "/ldapi"
ldapi_url = "ldapi://" + url_quote(ldapi_socket, "")
url_list = ldapi_url + " " + self.ldap_url
modlist = [
(ldap.MOD_ADD, "olcTLSCertificateKeyFile", [key]),
(ldap.MOD_ADD, "olcTLSCertificateFile", [cert]),
(ldap.MOD_ADD, "olcTLSCACertificatePath", [ca_cert])
]
ldap_conn = ldap.initialize(ldapi_url)
ldap_conn.simple_bind_s(self.admin_rdn + ",cn=config", self.admin_pw)
ldap_conn.modify_s("cn=config", modlist)
ldap_conn.unbind_s()
if subprocess.call(['pkill', 'slapd']) != 0:
raise execfile("Failed to stop slapd")
if subprocess.call(["slapd", "-F", self.conf_slapd_d_dir, "-h", url_list]) != 0:
raise Exception("Failed to start slapd")
Does this have to be a separate function?
diff --git src/tests/intg/test_ldap.py src/tests/intg/test_ldap.py index 848cb41..ea2e05f 100644 --- src/tests/intg/test_ldap.py +++ src/tests/intg/test_ldap.py @@ -16,7 +16,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see http://www.gnu.org/licenses/. # -import os +import os.path import stat import pwd import grp diff --git src/tests/intg/test_sssctl.py src/tests/intg/test_sssctl.py index 0df5d0b..3c2ba02 100644 --- src/tests/intg/test_sssctl.py +++ src/tests/intg/test_sssctl.py @@ -28,6 +28,7 @@ import signal import ds_openldap import ldap_ent import config +import socket from util import unindent import sssd_netgroup
@@ -38,14 +39,14 @@ LDAP_BASE_DN = "dc=example,dc=com" def ds_inst(request): """LDAP server instance fixture""" ds_inst = ds_openldap.DSOpenLDAP(
config.PREFIX, 10389, LDAP_BASE_DN,
"cn=admin", "Secret123")
try: ds_inst.setup() except: ds_inst.teardown() raise request.addfinalizer(lambda: ds_inst.teardown())config.PREFIX, 10389, LDAP_BASE_DN, "cn=admin", "Secret123")
- return ds_inst
All of the changes in the above piece seem to be unrelated to the main change. Please don't bundle those into the main commit.
Could you please consider addressing the above comments, and then we can proceed with the rest.
Also, could you please send commits formatted with git format-patch, instead of just git diffs? This way you can also submit several separate commits at once, and make it easier for me and others to apply and handle them.
Thanks!
Nick