[SSSD] Re: [PATCH]: test ldap provider with TLS or SSL

[Lukas Slebodnik]

On (11/03/16 17:20), Dan Lavu wrote:
> 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 <
> [e-mail]> 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

linux-libc-dev was installed as a dependency of other packages in my case.
> > 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.

All necessary steps are described in contrib/ci/README.md (at least should be)

I was able to run contrib/ci/run on minimal debian:testing
without any problem. I had to have installed packages "git, sudo, lsb-release"
and configured sudoers as it is described in contrib/ci/README.md
> I'm not familiar with Debian but I was not able
> to find these packages in the base repositories.

They were not in base because nss-wrapper was added later to debian.
And it isn't simple to add new packages to debian:stable.
That's the reason why we want to support only debian testing.
It's much simpler to get new packages there.
> Also do we have a
> bashrc_sssd equivalent for Debian?

No but contrib/ci/run should do all stuff.
> > > 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?

I explain it few paragraps above.

However you need to update CI dependencies.
diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh
index c9a8a63..ef5b89b 100644
--- a/contrib/ci/deps.sh
+++ b/contrib/ci/deps.sh
@@ -44,6 +44,7 @@ if [[ "$DISTRO_BRANCH" == -redhat-* ]]; then
         python-ldap
         rpm-build
         uid_wrapper
+        pyOpenSSL
     )
     _DEPS_LIST_SPEC=`
         sed -e 's/@PACKAGE_VERSION@/0/g' \
@@ -114,6 +115,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then
         python-ldap
         ldap-utils
         slapd
+        python-openssl
     )
     DEPS_INTGCHECK_SATISFIED=true
 fi
> > > 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

You need to add this file to Makefile
otherwise make distcheck will fail in contrib/ci script :-)

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 7394997..0af59b8 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -1,6 +1,7 @@
 dist_noinst_DATA = \
     config.py.m4 \
     sssd_id.py \
+    ca.py \
     ds.py \
     ds_openldap.py \
     ent.py \
> > > @@ -0,0 +1,163 @@
> > > +#
> > > +# SSSD LOCAL domain tests
> > > +#
> > > +# Copyright (c) 2016 Red Hat, Inc.
> > > +# Author: Dan Lavu
> > > +#
> > > +# 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, seehttp://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.

Actually, we should extend object to be more python3 compatible.
e.g.

class CA(object):
> > >        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?

+1 for "with" and extra variable for path. It's much simpler to read
the code.
@see
-        open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'), 'wt')\
-            .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
+        crt_path = os.path.join(self.cert_dir, socket.gethostname() + '.crt')
+        with open(crt_path, "wt") as f:
+            f.write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
> > Aside from everything else above, please don't forget to run pylint on the
> > code before submitting!

pylint has some false positives due to pytest but we do not have any
pep8 issues in integration tests.

sh$ pep8 src/tests/intg/
sh$ echo $?
0

LS