WOAH, that's a lot of comments! ;) Thanks for taking the time, I have some questions inline and I'll start working on improving the code.

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

I understand that completely, but I do feel that we need to find a way that executing bash contrib/ci/run to setup CI should run successfully without any other additional work. I'm not familiar with Debian but I was not able to find these packages in the base repositories. Also do we have a bashrc_sssd equivalent for Debian?

>
>
>> 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.

I'm testing and using the cloud-init openstack images, which is easy to spin up, but why do we only support the testing release?
>
>> 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

OK, I made a mistake with the SSL_PATH variable during my tests, and entering the wrong value essentially performed a rm -rf / so I was trying to avoid/protect against that, but with your comments about how that variable should be assigned, this will not be a problem.

> 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?

Are they being generated again? or are you saying that because I'm creating a new variable? The CA certs are just being read from the files that are on the system, so the first run will be a certificate for itself but can scale to multihost tests down the road.

>
>> +        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?
I think I did this because it's a crypto object, but yes, it can certainly be simplified, thanks.
>
>> +        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