Nik, Thanks for the review, the responses are inline.
On Wed, Mar 8, 2017 at 1:59 AM, Nikolai Kondrashov < nikolai.kondras...@redhat.com> wrote: > 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? > Done > > 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. > Done. > > """ >> 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. > Done. > > >> 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? > Just seems strange to have everything commented using quotes and these are hashes, added them back in. > > 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? > Done. > > 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? > Can you clarify or give me an example of what you want? > # >> # 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. > Done > > 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? > No, just thought functions should be simple and clear, I appended the code after the 'if self.ca' state in the setup. > > 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") >> + config.PREFIX, 10389, LDAP_BASE_DN, "cn=admin", "Secret123") >> try: >> ds_inst.setup() >> except: >> ds_inst.teardown() >> raise >> request.addfinalizer(lambda: ds_inst.teardown()) >> + >> 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 > _______________________________________________ > sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org > To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org >
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
