Nik,
Thanks for the review, the responses are inline.
On Wed, Mar 8, 2017 at 1:59 AM, Nikolai Kondrashov <
Nikolai.Kondrashov(a)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(a)lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-leave(a)lists.fedorahosted.org