Nik, 

Thanks for the review, the responses are inline.

On Wed, Mar 8, 2017 at 1:59 AM, Nikolai Kondrashov <Nikolai.Kondrashov@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-leave@lists.fedorahosted.org