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")
+ 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