Nikolai,

Since we last spoken, here is an update patch for review. Adding an optional ssl parameter to the DS class. When enabled, right now the tests fail because SSL is not configured properly on the client, but I'll get to that in my next patch? 

Dan

On Fri, Mar 11, 2016 at 1:16 PM, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> wrote:
Hi Dan,

Thanks a lot for your work! Please see my comments below.

On 03/11/2016 06:29 AM, Dan Lavu wrote:
Updated patch is attached,

There were a few more packages I had to install to get CI running for Debian, should we had these to the makefile?

root@sssd2:~# apt-get python-openssl

dpkg -ihttp://ftp.us.debian.org/debian/pool/main/n/nss-wrapper/libnss-wrapper_1.1.2-1_amd64.deb
dpkg -ihttp://security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+deb8u4_amd64.deb

I'm not sure if libnss-wrapper is in Debian Testing, but we certainly
shouldn't be downloading packages from a URL, especially non-debian packages,
but use the repos configured on the machine.

Below are test runs against Debian and Fedora

root@sssd2:~/sssd/x86_64# cat /etc/debian_version
8.3

This is Debian Stable and we don't support that - we support Debian Testing.
Ping me if you need help installing that.

diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py
new file mode 100644
index 0000000000000000000000000000000000000000..c7adb7a2f0c2ea7f342c9df1d6ad41dcb2fcc28e
--- /dev/null
+++ b/src/tests/intg/ca.py
@@ -0,0 +1,163 @@
+#
+# SSSD LOCAL domain tests
+#
+# Copyright (c) 2016 Red Hat, Inc.
+# Author: Dan Lavu<dan@redhat.com>
+#
+# This is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 only
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see<http://www.gnu.org/licenses/>.
+#
+
+from OpenSSL import crypto
+from os.path import exists, join
+
+import socket
+import os
+import shutil
+import config
+
+
+class CA():

We're not basing CA on anything so there is no real need for parens and
they're better be removed to avoid confusion.

+    """CA Class"""

This basically says what the line before it says, perhaps an expanded
explanation of the class function is in order.

+
+    def __init__(self, subject=None, country='US', state='NC',
+                 city='Raleigh', organization='Red Hat Inc.', unit='SSSD', config_dir=config.SSL_PATH):
+        if subject is None:
+            self.subject = socket.gethostname()
+        self.country = country
+        self.state = state
+        self.city = city
+        self.organization = organization
+        self.unit = unit
+        self.hostname = socket.gethostname()
+        if config.SSL_PATH is not '/tmp':
+            self.config_dir = config_dir
+        else:
+            self.config_dir = config_dir + '/ssl'

This kind of logic can confuse the class user, and make it impossible to
actually put files into /tmp if necessary.

I would replace this with just:

    self.config_dir = config_dir

As a side note, it's better to not create a fixed-name files or directories
under /tmp or other world-writable directories, as the possibility of name
clashes is high there. I presume the "tempfile" module should be used instead.

+        self.csr_dir = self.config_dir + '/newcerts'
+        self.key_dir = self.config_dir + '/private'
+        self.cert_dir = self.config_dir + '/certs'
+
+        self.index = int(1000)
+
+
+    def setup(self):
+        """Setup CA using OpenSSL"""
+        cacert = self.hostname + '-ca.crt'
+        cakey = self.hostname + '-ca.key'
+
+        if not exists(join(self.cert_dir, cacert)) or not exists(join(self.key_dir, cakey)):

It's better to use the fully-qualified name of the "join" function here so
it's obvious this is not just some generic "join". I.e. please write
"os.path.join" instead. Same goes for "exists".

To prevent further accidental uses like this don't import specific functions
above, but just the module.

Also, please put the generated paths into variables and use them instead, here
and further. This will help against typos.

+            key = crypto.PKey()
+            key.generate_key(crypto.TYPE_RSA, 2048)
+
+            ca = crypto.X509()

crypto.X509() doesn't create a CA, but a certificate. Therefore the variable
storing the result should be named accordingly (e.g. "cert", or "ca_cert", not
"ca"). Plus having a "ca" variable inside a "CA" class is confusing.

+            ca.get_subject().C = self.country
+            ca.get_subject().ST = self.state
+            ca.get_subject().L = self.city
+            ca.get_subject().O = self.organization
+            ca.get_subject().OU = self.unit
+            ca.get_subject().CN = self.subject
+            ca.set_serial_number(self.index)
+            ca.gmtime_adj_notBefore(0)
+            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
+            ca.set_issuer(ca.get_subject())
+            ca.set_pubkey(key)
+            ca.sign(key, 'sha1')
+
+            if not os.path.exists(self.csr_dir):
+                os.makedirs(self.csr_dir)
+            if not os.path.exists(self.key_dir):
+                os.makedirs(self.key_dir)
+            if not os.path.exists(self.cert_dir):
+                os.makedirs(self.cert_dir)
+
+            open(os.path.join(self.cert_dir, cacert), 'wt').write \
+                (crypto.dump_certificate(crypto.FILETYPE_PEM, ca))
+            open(os.path.join(self.key_dir, cakey), 'wt').write \
+                (crypto.dump_privatekey(crypto.FILETYPE_PEM, key))

Can we use "with" statement here, so that file is closed ASAP, instead of
waiting for GC?

+
+
+    def teardown(self):
+        """Teardown CA certificate files"""
+        if os.path.exists(self.config_dir) and not '/tmp':

Hmm, this condition will always be false and the directory would never be
removed. I assume this is a piece of code trying to handle the special case
above and if that's to go then this one can simply be removed as well.

+            shutil.rmtree(self.config_dir)
+
+
+    def get_ca(self):
+        """Returns CA certificate in ASCII PEM encoding"""
+        ca = crypto.load_certificate(crypto.FILETYPE_PEM, open\
+                    (os.path.join(self.cert_dir, socket.gethostname() + '-ca.crt'), 'rt').read())
+        ca_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, ca)
+
+        return ca_crt

This function name is confusing for the same reason as the "ca" variable
above. Plus it would be better not to return the certificate as text, unless
it's the main use.

+    def get_cert(self, csr, text=False):
+        """Retrieves certificate
+        required: csr -csr_type
+        optional: text
+            False - will return a pem object type (Default)
+            True - will return the certificate as ASCII
+        """
+        cacert = self.hostname + '-ca.crt'
+        cakey = self.hostname + '-ca.key'

Instead of generating these again, perhaps it is better to put them into
variables at initialization time?

+        ca_crt = crypto.load_certificate(crypto.FILETYPE_PEM, open(os.path.join(self.cert_dir, cacert), 'rt').read())
+        ca_key = crypto.load_privatekey(crypto.FILETYPE_PEM, open(os.path.join(self.key_dir, cakey), 'rt').read())

Similarly with these paths. Besides, why not use the "get_ca" function here
(after renaming) and perhaps a similar one to retrieve the key?

+        cert = crypto.X509()
+        cert.set_subject(csr.get_subject())
+        cert.set_serial_number(self.index+1)
+        cert.gmtime_adj_notBefore(0)
+        cert.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
+        cert.set_issuer(ca_crt.get_subject())
+        cert.set_pubkey(csr.get_pubkey())
+        cert.sign(ca_key, 'sha1')
+
+        open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'), 'wt')\
+            .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
+        server_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, cert)
+
+        if text is True:
+            return server_crt
+        else:
+            return cert

It is better to keep a function doing only one thing, and i.e. not implement
optional certificate conversion in this one. After all the caller can do it
themselves easily. So we can remove the "text" argument and not do this
conversion and branching.

+
+
+    def request_cert(self, fqdn=None, text=False):
+        """Generates CSR
+        optional: fqdn, socket.gethostname() (Default)
+        optional: text
+            False - will return a pem object type (Default)
+            True - will return the certificate as ASCII
+        """
+        if fqdn is None:
+            fqdn = socket.getfqdn()
+
+        hostname = socket.gethostname()
+        key = crypto.PKey()
+        key.generate_key(crypto.TYPE_RSA, 2048)
+        csr = crypto.X509Req()
+        csr.get_subject().CN = fqdn
+        csr.set_pubkey(key)
+        csr.sign(key, 'sha1')
+
+        open(os.path.join(self.key_dir, hostname + '.key'), 'wt').\
+            write(crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
+        open(os.path.join(self.csr_dir, hostname + '.csr'), 'wt').\
+            write(crypto.dump_certificate_request(crypto.FILETYPE_PEM, csr))

The same notes about certificate/key retrieval here.

+        server_csr = crypto.dump_certificate_request(crypto.FILETYPE_PEM, csr)
+
+        if text is True:
+            return server_csr
+        else:
+            return csr

The same note about certificate conversion here.

\ No newline at end of file
diff --git a/src/tests/intg/config.py.m4 b/src/tests/intg/config.py.m4
index 563127c6ea895508308a4f94689cc4e26ca4cbde..141813fa7a7a5bc9c06e48dfc938885f4eb1d539 100644
--- a/src/tests/intg/config.py.m4
+++ b/src/tests/intg/config.py.m4
@@ -11,3 +11,4 @@ PID_PATH            = "pidpath"
  PIDFILE_PATH        = PID_PATH + "/sssd.pid"
  LOG_PATH            = "logpath"
  MCACHE_PATH         = "mcpath"
+SSL_PATH            = "/tmp/ssl"

config.py.m4 is for variables originating from build configuration.  As
SSL_PATH is just a constant it shouldn't really be here. As such the proper
place for it could be ca.py.

However, it is better to keep all the files tests create under the prefix
directory (to make cleanup easier and more reliable), in a place where people
would expect them. I don't know where this specific configuration should be,
but if it's e.g. /etc/ssl normally, then you can write in config.py.m4:

    SSL_PATH            = SYSCONFDIR + "/ssl"


diff --git a/src/tests/intg/ds_openldap.py b/src/tests/intg/ds_openldap.py
index fb230a081b58bd4e135585daa5a6ddf8f494861c..dbbbfea660d3c4e0f7c7546663cc8560522ffa37 100644
--- a/src/tests/intg/ds_openldap.py
+++ b/src/tests/intg/ds_openldap.py
@@ -27,8 +27,10 @@ import errno
  import signal
  import shutil
  import sys
+import socket
  from util import *
  from ds import DS
+from ca import CA


  def hash_password(password):
@@ -39,7 +41,7 @@ def hash_password(password):
      return "{SSHA}" + base64.standard_b64encode(hash.digest() + salt)


-class DSOpenLDAP(DS):
+class DSOpenLDAP(DS, CA):

I don't think a DS should be derived from a CA. CA is a separate entity.

      """OpenLDAP directory server instance."""

      def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
@@ -60,6 +62,9 @@ class DSOpenLDAP(DS):
          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.ca_inst = CA()

I don't think that's how inheritance is supposed to be used. Besides, it's
better to let the user create and supply a CA through an (optional) argument
instead. Then the user can choose whether to use encryption or not.

+
+

      def _setup_config(self):
          """Setup the instance initial configuration."""
@@ -73,6 +78,13 @@ class DSOpenLDAP(DS):
          uid = os.geteuid()
          gid = os.getegid()

+        self.ca_inst.setup()

If we won't be creating the CA instance ourselves, then we shouldn't be
calling "setup", the user should call it instead, so e.g. a single CA can be
used by several LDAP servers.

+        self.ca_inst.get_cert(self.ca_inst.request_cert())
+        fqdn = socket.gethostname()
+        cert = os.path.join(self.ca_inst.cert_dir, fqdn + '.crt')
+        ca_cert = os.path.join(self.ca_inst.cert_dir, fqdn + '-ca.crt')
+        key = os.path.join(self.ca_inst.key_dir, fqdn + '.key')

Those three should really be variables in CA, the user shouldn't have to know
how filenames inside CA are formed. Incapsulation!

+
          #
          # Add configuration
          #
@@ -82,6 +94,9 @@ class DSOpenLDAP(DS):
              cn: config
              olcPidFile: {self.pid_path}
              olcArgsFile: {args_file}
+            olcTLSCertificateKeyFile: {key}
+            olcTLSCertificateFile: {cert}
+            olcTLSCACertificatePath: {ca_cert}
              # Read slapd.conf(5) for possible values
              olcLogLevel: none

@@ -282,3 +297,5 @@ class DSOpenLDAP(DS):

          for path in (self.conf_slapd_d_dir, self.run_dir, self.data_dir):
              shutil.rmtree(path, True)
+
+        self.ca_inst.teardown()

Similarly to "setup" note above.

Aside from everything else above, please don't forget to run pylint on the
code before submitting!

Nick
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org