Hi everyone,
I'd like to continue the discussion of tlog integration, and also present you the first release of tlog - a development preview, which has the configuration interface necessary to implement the integration:
https://github.com/spbnick/tlog/releases/tag/v1
You're more than welcome to download RPMs, install, read tlog-rec(8) and tlog-rec.conf(5), and experiment! Building from the Git tree and the tarball works as well, if you're so inclined. I'm also attaching those manpages for convenience.
Here are the integration plans so far, as discussed with Jakub on our devconf.cz trip meetings and before on the list. Jakub, please correct me or add details.
* We follow the route similar to that taken by SELinux rule control implementation [1][2]. I.e. store the configuration in LDAP HBAC rules, write to files on the client side and then specify them to tlog upon user login.
However, I'm also rather fond of the idea of specifying the whole configuration through an environment variable instead of through a file referenced by an environment variable - it's not big at all, and we'll avoid the hassle of managing the files.
I implemented support for both in tlog (was easy).
* We'll have to make nss_sss report user's shell as tlog-rec (how?) and specify the actual shell to tlog-rec via an environment variable, through pam_sss (with SSS_PAM_ENV_ITEM messages). I.e.:
* Nss_sss would always report tlog-rec as the user's shell.
* During login (e.g. through "login" or "sshd") pam_sss would add a variable to the user environment, containing, or pointing at, a tlog-rec configuration (TLOG_REC_CONF_TEXT or TLOG_REC_CONF_FILE). That configuration would contain the user's actual shell. I can also implement support for a separate variable just for the shell (TLOG_REC_SHELL?) to simplify the implementation for the start.
* Tlog-rec would read the system-wide configuration and overlay it with the one specified in the environment, adding the specific user shell, and then would spawn it.
Please also see the draft integration design page [3] for reference. I hope to refine and extend it in the coming weeks to match FreeIPA standards.
Please chime in and suggest, object, discuss!
Also, please report tlog bugs at https://github.com/spbnick/tlog/issues
Thank you!
Nick
[1]: http://www.freeipa.org/page/SELinux_user_mapping [2]: http://www.freeipa.org/images/b/b9/Freeipa30_SELinuxUserMap.pdf [3]: http://www.freeipa.org/page/Session_Recording
Here is a patch for https://fedorahosted.org/sssd/ticket/2820
First real patch... criticisms to for what I need to improve on are welcome, including concepts that I should learn, thanks.
Test output below:
---------------------------------------------------------------------- Libraries have been installed in: /tmp/sssd-intg.sgV3vS4v/lib/sssd
If you ever happen to want to link against installed libraries in a given directory, LIBDIR, you must either use libtool, and specify the full pathname of the library, or use the '-LLIBDIR' flag during linking and do at least one of the following: - add LIBDIR to the 'LD_LIBRARY_PATH' environment variable during execution - add LIBDIR to the 'LD_RUN_PATH' environment variable during linking - use the '-Wl,-rpath -Wl,LIBDIR' linker flag - have your system administrator add LIBDIR to '/etc/ld.so.conf'
See any operating system documentation about shared libraries for more information, such as the ld(1) and ld.so(8) manual pages. ---------------------------------------------------------------------- /bin/mkdir -p '/tmp/sssd-intg.sgV3vS4v/lib' /bin/sh ./libtool --mode=install /bin/install -c libsss_sudo.la '/tmp/sssd-intg.sgV3vS4v/lib' libtool: install: /bin/install -c .libs/libsss_sudo.so /tmp/sssd-intg.sgV3vS4v/lib/libsss_sudo.so libtool: install: /bin/install -c .libs/libsss_sudo.lai /tmp/sssd-intg.sgV3vS4v/lib/libsss_sudo.la libtool: finish: PATH="/sbin:/bin:/usr/sbin:/usr/bin:/sbin" ldconfig -n /tmp/sssd-intg.sgV3vS4v/lib ---------------------------------------------------------------------- Libraries have been installed in: /tmp/sssd-intg.sgV3vS4v/lib
If you ever happen to want to link against installed libraries in a given directory, LIBDIR, you must either use libtool, and specify the full pathname of the library, or use the '-LLIBDIR' flag during linking and do at least one of the following: - add LIBDIR to the 'LD_LIBRARY_PATH' environment variable during execution - add LIBDIR to the 'LD_RUN_PATH' environment variable during linking - use the '-Wl,-rpath -Wl,LIBDIR' linker flag - have your system administrator add LIBDIR to '/etc/ld.so.conf'
See any operating system documentation about shared libraries for more information, such as the ld(1) and ld.so(8) manual pages. ---------------------------------------------------------------------- make install-data-hook make[5]: Entering directory '/root/sssd.git/x86_64/intg/bld' rm //tmp/sssd-intg.sgV3vS4v/lib/libnss_sss.so.2 \ //tmp/sssd-intg.sgV3vS4v/lib/libnss_sss.so mv //tmp/sssd-intg.sgV3vS4v/lib/libnss_sss.so.2.0.0 //tmp/sssd-intg.sgV3vS4v/lib/libnss_sss.so.2 if [ ! __LIBKRB5_DEFAULTS__ = "__LIBKRB5_DEFAULTS__" ]; then \ /bin/mkdir -p /__LIBKRB5_DEFAULTS__ ; \ fi make[5]: Leaving directory '/root/sssd.git/x86_64/intg/bld' make[4]: Leaving directory '/root/sssd.git/x86_64/intg/bld' make[3]: Leaving directory '/root/sssd.git/x86_64/intg/bld' Making install in src/tests/cwrap make[3]: Entering directory '/root/sssd.git/x86_64/intg/bld/src/tests/cwrap' make[4]: Entering directory '/root/sssd.git/x86_64/intg/bld/src/tests/cwrap' make[4]: Nothing to be done for 'install-exec-am'. make[4]: Nothing to be done for 'install-data-am'. make[4]: Leaving directory '/root/sssd.git/x86_64/intg/bld/src/tests/cwrap' make[3]: Leaving directory '/root/sssd.git/x86_64/intg/bld/src/tests/cwrap' Making install in src/tests/intg make[3]: Entering directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg' make[4]: Entering directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg' make[4]: Nothing to be done for 'install-exec-am'. make[4]: Nothing to be done for 'install-data-am'. make[4]: Leaving directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg' make[3]: Leaving directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg' make[2]: Leaving directory '/root/sssd.git/x86_64/intg/bld' make[1]: Leaving directory '/root/sssd.git/x86_64/intg/bld' make[1]: Entering directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg' m4 -D "prefix=`/tmp/sssd-intg.sgV3vS4v'" \ -D "sysconfdir=`/tmp/sssd-intg.sgV3vS4v/etc'" \ -D "dbpath=`/tmp/sssd-intg.sgV3vS4v/var/lib/sss/db'" \ -D "pidpath=`/tmp/sssd-intg.sgV3vS4v/var/run'" \ -D "logpath=`/tmp/sssd-intg.sgV3vS4v/var/log/sssd'" \ -D "mcpath=`/tmp/sssd-intg.sgV3vS4v/var/lib/sss/mc'" \ /root/sssd.git/x86_64/../src/tests/intg/config.py.m4 > config.py : "Create directory for emulated root's D-Bus cookies." : "See http://dbus.freedesktop.org/doc/dbus-specification.html#auth-mechanisms" /bin/mkdir -p -m 0700 root/.dbus-keyrings echo "root:x:0:0:root:/root/sssd.git/x86_64/intg/bld/src/tests/intg/root:/bin/bash"
passwd
echo "root:x:0:" > group pipepath="/tmp/sssd-intg.sgV3vS4v/var/lib/sss/pipes"; \ if test ${#pipepath} -gt 80; then \ echo "error: Pipe directory path too long," \ "D-Bus won't be able to open sockets" >&2; \ exit 1; \ fi set -e; \ cd "/root/sssd.git/x86_64/../src/tests/intg"; \ nss_wrapper=$(pkg-config --libs nss_wrapper); \ uid_wrapper=$(pkg-config --libs uid_wrapper); \ PATH="$(dirname -- /sbin/slapd):$PATH" \ PATH="/tmp/sssd-intg.sgV3vS4v/sbin:/tmp/sssd-intg.sgV3vS4v/bin:$PATH" \ PATH="/root/sssd.git/x86_64/intg/bld/src/tests/intg:/root/sssd.git/x86_64/../src/tests/intg:$PATH" \ PYTHONPATH="/root/sssd.git/x86_64/intg/bld/src/tests/intg:/root/sssd.git/x86_64/../src/tests/intg" \ LDB_MODULES_PATH="/tmp/sssd-intg.sgV3vS4v/lib/ldb" \ LD_PRELOAD="$nss_wrapper $uid_wrapper" \ NSS_WRAPPER_PASSWD="/root/sssd.git/x86_64/intg/bld/src/tests/intg/passwd" \ NSS_WRAPPER_GROUP="/root/sssd.git/x86_64/intg/bld/src/tests/intg/group" \ NSS_WRAPPER_MODULE_SO_PATH="/tmp/sssd-intg.sgV3vS4v/lib/libnss_sss.so.2" \ NSS_WRAPPER_MODULE_FN_PREFIX="sss" \ UID_WRAPPER=1 \ UID_WRAPPER_ROOT=1 \ fakeroot /bin/python2 /bin/py.test -v --tb=native . ============================================================= test session starts ============================================================== platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.7.3 -- /bin/python2 rootdir: /root/sssd.git/src/tests/intg, inifile: collected 73 items
ent_test.py::test_assert_passwd_by_name PASSED ent_test.py::test_assert_passwd_by_uid PASSED ent_test.py::test_assert_passwd_list PASSED ent_test.py::test_assert_each_passwd_by_name PASSED ent_test.py::test_assert_each_passwd_by_uid PASSED ent_test.py::test_assert_each_passwd_with_name PASSED ent_test.py::test_assert_each_passwd_with_uid PASSED ent_test.py::test_assert_passwd PASSED ent_test.py::test_group_member_matching PASSED ent_test.py::test_assert_group_by_name PASSED ent_test.py::test_assert_group_by_gid PASSED ent_test.py::test_assert_group_list PASSED ent_test.py::test_assert_each_group_by_name PASSED ent_test.py::test_assert_each_group_by_gid PASSED ent_test.py::test_assert_each_group_with_name PASSED ent_test.py::test_assert_each_group_with_gid PASSED ent_test.py::test_assert_group PASSED ldap_local_override_test.py::test_simple_user_override PASSED ldap_local_override_test.py::test_root_user_override PASSED ldap_local_override_test.py::test_replace_user_override PASSED ldap_local_override_test.py::test_remove_user_override PASSED ldap_local_override_test.py::test_imp_exp_user_override PASSED ldap_local_override_test.py::test_show_user_override PASSED ldap_local_override_test.py::test_find_user_override PASSED ldap_local_override_test.py::test_simple_group_override PASSED ldap_local_override_test.py::test_root_group_override PASSED ldap_local_override_test.py::test_replace_group_override PASSED ldap_local_override_test.py::test_remove_group_override PASSED ldap_local_override_test.py::test_imp_exp_group_override PASSED ldap_local_override_test.py::test_regr_2802_override PASSED ldap_local_override_test.py::test_regr_2757_override PASSED ldap_local_override_test.py::test_regr_2790_override PASSED ldap_test.py::test_regression_ticket2163 PASSED ldap_test.py::test_sanity_rfc2307 PASSED ldap_test.py::test_sanity_rfc2307_bis PASSED ldap_test.py::test_refresh_after_cleanup_task PASSED ldap_test.py::test_add_remove_user PASSED ldap_test.py::test_add_remove_group_rfc2307 PASSED ldap_test.py::test_add_remove_group_rfc2307_bis PASSED ldap_test.py::test_add_remove_membership_rfc2307 PASSED ldap_test.py::test_add_remove_membership_rfc2307_bis PASSED ldap_test.py::test_override_homedir PASSED ldap_test.py::test_fallback_homedir PASSED ldap_test.py::test_override_shell PASSED ldap_test.py::test_shell_fallback PASSED ldap_test.py::test_default_shell PASSED ldap_test.py::test_vetoed_shells PASSED test_local_domain.py::test_wrong_LC_ALL PASSED test_memory_cache.py::test_getpwnam PASSED test_memory_cache.py::test_getpwnam_with_mc PASSED test_memory_cache.py::test_getgrnam_simple PASSED test_memory_cache.py::test_getgrnam_simple_with_mc PASSED test_memory_cache.py::test_getgrnam_membership PASSED test_memory_cache.py::test_getgrnam_membership_with_mc PASSED test_memory_cache.py::test_initgroups PASSED test_memory_cache.py::test_initgroups_with_mc PASSED test_memory_cache.py::test_initgroups_fqname_with_mc PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc1 PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc2 PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc3 PASSED test_memory_cache.py::test_invalidation_of_gids_after_initgroups PASSED test_memory_cache.py::test_initgroups_without_change_in_membership PASSED test_memory_cache.py::test_invalidate_user_before_stop PASSED test_memory_cache.py::test_invalidate_user_after_stop PASSED test_memory_cache.py::test_invalidate_users_before_stop PASSED test_memory_cache.py::test_invalidate_users_after_stop PASSED test_memory_cache.py::test_invalidate_group_before_stop PASSED test_memory_cache.py::test_invalidate_group_after_stop PASSED test_memory_cache.py::test_invalidate_groups_before_stop PASSED test_memory_cache.py::test_invalidate_groups_after_stop PASSED test_memory_cache.py::test_invalidate_everything_before_stop PASSED test_memory_cache.py::test_invalidate_everything_after_stop PASSED test_memory_cache.py::test_removed_mc PASSED
========================================================= 73 passed in 203.32 seconds ========================================================== rm -f /tmp/sssd-intg.sgV3vS4v/var/log/sssd/* make[1]: Leaving directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg'
On Thu, Feb 25, 2016 at 05:18:09PM -0500, Dan Lavu wrote:
Here is a patch for https://fedorahosted.org/sssd/ticket/2820
First real patch... criticisms to for what I need to improve on are welcome, including concepts that I should learn, thanks.
Thanks a lot for the patch!
See my comments inline:
From 529adb3e0d763a8ee9ba9b4c5b13f933d723e8de Mon Sep 17 00:00:00 2001 From: Dan Lavu dlavu@redhat.com Date: Fri, 5 Feb 2016 08:51:07 -0500 Subject: [PATCH] Adding SSL encryption to integration tests.
src/tests/intg/ca.py | 166 ++++++++++++++++++++++++++++++++++++++++++ src/tests/intg/ds_openldap.py | 14 ++++ 2 files changed, 180 insertions(+) create mode 100644 src/tests/intg/ca.py
diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py new file mode 100644 index 0000000000000000000000000000000000000000..a44a92e5d5053338dabd7d8d82d2b1d50ec7594e --- /dev/null +++ b/src/tests/intg/ca.py @@ -0,0 +1,166 @@ +# +# 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 fnmatch
+class CA:
It would be nice to use the new-style classes, so class CA(object)
- """CA Class"""
- def __init__(self, subject=None, country=None, state=None,
city=None, organization=None, unit=None, config_dir=None):if subject is None:self.subject = socket.gethostname()if country is None:self.country = 'US'if state is None:self.state = 'NC'if city is None:self.city = 'Raleigh'if organization is None:self.organization = 'Red Hat'if unit is None:self.unit = 'SSSD'if config_dir is None:self.config_dir = '/etc/pki'
/etc/pki is not writable unless you're root. We should store the certs in another directory writable by any user. Maybe this is something Nikolai (CC) could help us with, I know we use fakeroot to set up the directory structure, but I'm fuzzy on the details, so I don't know myself which part of the tests we should exactly touch..
Also, does the /etc/pki path exists on Debian and other distributions or is it Red Hat-centric?
When we have this done, hopefully we can remove the use of 'ldap_auth_disable_tls_never_use_in_production' from our tests?
self.hostname = socket.gethostname()
This is maybe something to fix in a later iteration of the patch, but I wonder if it was useful to override the hostname to something else than what gethostname() reports. Not sure at the moment..
self.csr_dir = self.config_dir + '/CA/newcerts'self.key_dir = self.config_dir + '/tls/private'self.cert_dir = self.config_dir + '/tls/certs'self.index = int(1000)- def setup(self):
"""Setup CA using OpenSSL"""cacert = socket.gethostname() + '-ca.crt'cakey = socket.gethostname() + '-ca.key'
Instead of using socket.gethostname(), maybe using self.hostname would be better here (and elsewhere) ?
if not exists(join(self.cert_dir, cacert)) or not exists(join(self.key_dir, cakey)):key = crypto.PKey()key.generate_key(crypto.TYPE_RSA, 2048)ca = crypto.X509()ca.get_subject().C = self.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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')
On (26/02/16 11:53), Jakub Hrozek wrote:
On Thu, Feb 25, 2016 at 05:18:09PM -0500, Dan Lavu wrote:
Here is a patch for https://fedorahosted.org/sssd/ticket/2820
First real patch... criticisms to for what I need to improve on are welcome, including concepts that I should learn, thanks.
Thanks a lot for the patch!
See my comments inline:
From 529adb3e0d763a8ee9ba9b4c5b13f933d723e8de Mon Sep 17 00:00:00 2001 From: Dan Lavu dlavu@redhat.com Date: Fri, 5 Feb 2016 08:51:07 -0500 Subject: [PATCH] Adding SSL encryption to integration tests.
src/tests/intg/ca.py | 166 ++++++++++++++++++++++++++++++++++++++++++ src/tests/intg/ds_openldap.py | 14 ++++ 2 files changed, 180 insertions(+) create mode 100644 src/tests/intg/ca.py
diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py new file mode 100644 index 0000000000000000000000000000000000000000..a44a92e5d5053338dabd7d8d82d2b1d50ec7594e --- /dev/null +++ b/src/tests/intg/ca.py @@ -0,0 +1,166 @@ +# +# 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 fnmatch
+class CA:
It would be nice to use the new-style classes, so class CA(object)
- """CA Class"""
- def __init__(self, subject=None, country=None, state=None,
city=None, organization=None, unit=None, config_dir=None):if subject is None:self.subject = socket.gethostname()if country is None:self.country = 'US'
I'm nt python expert but might use default arguments http://www.tutorialspoint.com/python/python_functions.htm -> Default arguments
if state is None:self.state = 'NC'if city is None:self.city = 'Raleigh'if organization is None:self.organization = 'Red Hat'if unit is None:self.unit = 'SSSD'if config_dir is None:self.config_dir = '/etc/pki'/etc/pki is not writable unless you're root. We should store the certs in another directory writable by any user. Maybe this is something Nikolai (CC) could help us with, I know we use fakeroot to set up the directory structure, but I'm fuzzy on the details, so I don't know myself which part of the tests we should exactly touch..
The constant SYSCONFDIR from module "config" might be used. and it would be storedin our custom prefix (somewhere in /tmp ....)
We have more constants there. "m4 template" is here src/tests/intg/config.py.m4
LS
I've made most of the the suggested changes but I'm going to take sometime and get the test running on Debian as well (Mostly to find out if /etc/pki is a Red Hat thing or not). Fedora and Debian are the only distros we are testing/supporting against correct? Also wondering if the ci setup issue I'm seeing applies to apt.
Dan
On 2/26/16 5:53 AM, Jakub Hrozek wrote:
On Thu, Feb 25, 2016 at 05:18:09PM -0500, Dan Lavu wrote:
Here is a patch for https://fedorahosted.org/sssd/ticket/2820
First real patch... criticisms to for what I need to improve on are welcome, including concepts that I should learn, thanks.
Thanks a lot for the patch!
See my comments inline:
From 529adb3e0d763a8ee9ba9b4c5b13f933d723e8de Mon Sep 17 00:00:00 2001 From: Dan Lavu dlavu@redhat.com Date: Fri, 5 Feb 2016 08:51:07 -0500 Subject: [PATCH] Adding SSL encryption to integration tests.
src/tests/intg/ca.py | 166 ++++++++++++++++++++++++++++++++++++++++++ src/tests/intg/ds_openldap.py | 14 ++++ 2 files changed, 180 insertions(+) create mode 100644 src/tests/intg/ca.py
diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py new file mode 100644 index 0000000000000000000000000000000000000000..a44a92e5d5053338dabd7d8d82d2b1d50ec7594e --- /dev/null +++ b/src/tests/intg/ca.py @@ -0,0 +1,166 @@ +# +# 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 fnmatch
+class CA:
It would be nice to use the new-style classes, so class CA(object)
- """CA Class"""
- def __init__(self, subject=None, country=None, state=None,
city=None, organization=None, unit=None, config_dir=None):if subject is None:self.subject = socket.gethostname()if country is None:self.country = 'US'if state is None:self.state = 'NC'if city is None:self.city = 'Raleigh'if organization is None:self.organization = 'Red Hat'if unit is None:self.unit = 'SSSD'if config_dir is None:self.config_dir = '/etc/pki'/etc/pki is not writable unless you're root. We should store the certs in another directory writable by any user. Maybe this is something Nikolai (CC) could help us with, I know we use fakeroot to set up the directory structure, but I'm fuzzy on the details, so I don't know myself which part of the tests we should exactly touch..
Also, does the /etc/pki path exists on Debian and other distributions or is it Red Hat-centric?
When we have this done, hopefully we can remove the use of 'ldap_auth_disable_tls_never_use_in_production' from our tests?
self.hostname = socket.gethostname()This is maybe something to fix in a later iteration of the patch, but I wonder if it was useful to override the hostname to something else than what gethostname() reports. Not sure at the moment..
self.csr_dir = self.config_dir + '/CA/newcerts'self.key_dir = self.config_dir + '/tls/private'self.cert_dir = self.config_dir + '/tls/certs'self.index = int(1000)- def setup(self):
"""Setup CA using OpenSSL"""cacert = socket.gethostname() + '-ca.crt'cakey = socket.gethostname() + '-ca.key'Instead of using socket.gethostname(), maybe using self.hostname would be better here (and elsewhere) ?
if not exists(join(self.cert_dir, cacert)) or not exists(join(self.key_dir, cakey)):key = crypto.PKey()key.generate_key(crypto.TYPE_RSA, 2048)ca = crypto.X509()ca.get_subject().C = self.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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')
On Sun, Feb 28, 2016 at 08:19:57PM -0500, Dan Lavu wrote:
I've made most of the the suggested changes but I'm going to take sometime and get the test running on Debian as well (Mostly to find out if /etc/pki is a Red Hat thing or not). Fedora and Debian are the only distros we are testing/supporting against correct?
Yes, we support RHEL >= 6, Fedora (all supported versions) and Debian Testing.
Also wondering if the ci setup issue I'm seeing applies to apt.
Dan
On 2/26/16 5:53 AM, Jakub Hrozek wrote:
On Thu, Feb 25, 2016 at 05:18:09PM -0500, Dan Lavu wrote:
Here is a patch for https://fedorahosted.org/sssd/ticket/2820
First real patch... criticisms to for what I need to improve on are welcome, including concepts that I should learn, thanks.
Thanks a lot for the patch!
See my comments inline:
From 529adb3e0d763a8ee9ba9b4c5b13f933d723e8de Mon Sep 17 00:00:00 2001 From: Dan Lavu dlavu@redhat.com Date: Fri, 5 Feb 2016 08:51:07 -0500 Subject: [PATCH] Adding SSL encryption to integration tests.
src/tests/intg/ca.py | 166 ++++++++++++++++++++++++++++++++++++++++++ src/tests/intg/ds_openldap.py | 14 ++++ 2 files changed, 180 insertions(+) create mode 100644 src/tests/intg/ca.py
diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py new file mode 100644 index 0000000000000000000000000000000000000000..a44a92e5d5053338dabd7d8d82d2b1d50ec7594e --- /dev/null +++ b/src/tests/intg/ca.py @@ -0,0 +1,166 @@ +# +# 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 fnmatch
+class CA:
It would be nice to use the new-style classes, so class CA(object)
- """CA Class"""
- def __init__(self, subject=None, country=None, state=None,
city=None, organization=None, unit=None, config_dir=None):if subject is None:self.subject = socket.gethostname()if country is None:self.country = 'US'if state is None:self.state = 'NC'if city is None:self.city = 'Raleigh'if organization is None:self.organization = 'Red Hat'if unit is None:self.unit = 'SSSD'if config_dir is None:self.config_dir = '/etc/pki'/etc/pki is not writable unless you're root. We should store the certs in another directory writable by any user. Maybe this is something Nikolai (CC) could help us with, I know we use fakeroot to set up the directory structure, but I'm fuzzy on the details, so I don't know myself which part of the tests we should exactly touch..
Also, does the /etc/pki path exists on Debian and other distributions or is it Red Hat-centric?
When we have this done, hopefully we can remove the use of 'ldap_auth_disable_tls_never_use_in_production' from our tests?
self.hostname = socket.gethostname()This is maybe something to fix in a later iteration of the patch, but I wonder if it was useful to override the hostname to something else than what gethostname() reports. Not sure at the moment..
self.csr_dir = self.config_dir + '/CA/newcerts'self.key_dir = self.config_dir + '/tls/private'self.cert_dir = self.config_dir + '/tls/certs'self.index = int(1000)- def setup(self):
"""Setup CA using OpenSSL"""cacert = socket.gethostname() + '-ca.crt'cakey = socket.gethostname() + '-ca.key'Instead of using socket.gethostname(), maybe using self.hostname would be better here (and elsewhere) ?
if not exists(join(self.cert_dir, cacert)) or not exists(join(self.key_dir, cakey)):key = crypto.PKey()key.generate_key(crypto.TYPE_RSA, 2048)ca = crypto.X509()ca.get_subject().C = self.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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')
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... dpkg -ihttp://security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+deb...
Below are test runs against Debian and Fedora
fakeroot /usr/bin/python2 /usr/bin/py.test -v --tb=native . ============================================================================== test session starts =============================================================================== platform linux2 -- Python 2.7.9 -- py-1.4.25 -- pytest-2.6.3 -- /usr/bin/python2 collected 73 items
ent_test.py::test_assert_passwd_by_name PASSED ent_test.py::test_assert_passwd_by_uid PASSED ent_test.py::test_assert_passwd_list PASSED ent_test.py::test_assert_each_passwd_by_name PASSED ent_test.py::test_assert_each_passwd_by_uid PASSED ent_test.py::test_assert_each_passwd_with_name PASSED ent_test.py::test_assert_each_passwd_with_uid PASSED ent_test.py::test_assert_passwd PASSED ent_test.py::test_group_member_matching PASSED ent_test.py::test_assert_group_by_name PASSED ent_test.py::test_assert_group_by_gid PASSED ent_test.py::test_assert_group_list PASSED ent_test.py::test_assert_each_group_by_name PASSED ent_test.py::test_assert_each_group_by_gid PASSED ent_test.py::test_assert_each_group_with_name PASSED ent_test.py::test_assert_each_group_with_gid PASSED ent_test.py::test_assert_group PASSED ldap_local_override_test.py::test_simple_user_override PASSED ldap_local_override_test.py::test_root_user_override PASSED ldap_local_override_test.py::test_replace_user_override PASSED ldap_local_override_test.py::test_remove_user_override PASSED ldap_local_override_test.py::test_imp_exp_user_override PASSED ldap_local_override_test.py::test_show_user_override PASSED ldap_local_override_test.py::test_find_user_override PASSED ldap_local_override_test.py::test_simple_group_override PASSED ldap_local_override_test.py::test_root_group_override PASSED ldap_local_override_test.py::test_replace_group_override PASSED ldap_local_override_test.py::test_remove_group_override PASSED ldap_local_override_test.py::test_imp_exp_group_override PASSED ldap_local_override_test.py::test_regr_2802_override PASSED ldap_local_override_test.py::test_regr_2757_override PASSED ldap_local_override_test.py::test_regr_2790_override PASSED ldap_test.py::test_regression_ticket2163 PASSED ldap_test.py::test_sanity_rfc2307 PASSED ldap_test.py::test_sanity_rfc2307_bis PASSED ldap_test.py::test_refresh_after_cleanup_task PASSED ldap_test.py::test_add_remove_user PASSED ldap_test.py::test_add_remove_group_rfc2307 PASSED ldap_test.py::test_add_remove_group_rfc2307_bis PASSED ldap_test.py::test_add_remove_membership_rfc2307 PASSED ldap_test.py::test_add_remove_membership_rfc2307_bis PASSED ldap_test.py::test_override_homedir PASSED ldap_test.py::test_fallback_homedir PASSED ldap_test.py::test_override_shell PASSED ldap_test.py::test_shell_fallback PASSED ldap_test.py::test_default_shell PASSED ldap_test.py::test_vetoed_shells PASSED test_local_domain.py::test_wrong_LC_ALL PASSED test_memory_cache.py::test_getpwnam PASSED test_memory_cache.py::test_getpwnam_with_mc PASSED test_memory_cache.py::test_getgrnam_simple PASSED test_memory_cache.py::test_getgrnam_simple_with_mc PASSED test_memory_cache.py::test_getgrnam_membership PASSED test_memory_cache.py::test_getgrnam_membership_with_mc PASSED test_memory_cache.py::test_initgroups PASSED test_memory_cache.py::test_initgroups_with_mc PASSED test_memory_cache.py::test_initgroups_fqname_with_mc PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc1 PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc2 PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc3 PASSED test_memory_cache.py::test_invalidation_of_gids_after_initgroups PASSED test_memory_cache.py::test_initgroups_without_change_in_membership PASSED test_memory_cache.py::test_invalidate_user_before_stop PASSED test_memory_cache.py::test_invalidate_user_after_stop PASSED test_memory_cache.py::test_invalidate_users_before_stop PASSED test_memory_cache.py::test_invalidate_users_after_stop PASSED test_memory_cache.py::test_invalidate_group_before_stop PASSED test_memory_cache.py::test_invalidate_group_after_stop PASSED test_memory_cache.py::test_invalidate_groups_before_stop PASSED test_memory_cache.py::test_invalidate_groups_after_stop PASSED test_memory_cache.py::test_invalidate_everything_before_stop PASSED test_memory_cache.py::test_invalidate_everything_after_stop PASSED test_memory_cache.py::test_removed_mc PASSED
========================================================================== 73 passed in 203.82 seconds =========================================================================== rm -f /tmp/sssd-intg.zncqC9vY/var/log/sssd/* make[1]: Leaving directory '/root/sssd/x86_64/intg/bld/src/tests/intg' root@sssd2:~/sssd/x86_64# cat /etc/debian_version 8.3
cd "/root/sssd.git/x86_64/../src/tests/intg"; \ nss_wrapper=$(pkg-config --libs nss_wrapper); \ uid_wrapper=$(pkg-config --libs uid_wrapper); \ PATH="$(dirname -- /usr/sbin/slapd):$PATH" \ PATH="/tmp/sssd-intg.icQ2aGpF/sbin:/tmp/sssd-intg.icQ2aGpF/bin:$PATH" \ PATH="/root/sssd.git/x86_64/intg/bld/src/tests/intg:/root/sssd.git/x86_64/../src/tests/intg:$PATH" \ PYTHONPATH="/root/sssd.git/x86_64/intg/bld/src/tests/intg:/root/sssd.git/x86_64/../src/tests/intg" \ LDB_MODULES_PATH="/tmp/sssd-intg.icQ2aGpF/lib/ldb" \ LD_PRELOAD="$nss_wrapper $uid_wrapper" \ NSS_WRAPPER_PASSWD="/root/sssd.git/x86_64/intg/bld/src/tests/intg/passwd" \ NSS_WRAPPER_GROUP="/root/sssd.git/x86_64/intg/bld/src/tests/intg/group" \ NSS_WRAPPER_MODULE_SO_PATH="/tmp/sssd-intg.icQ2aGpF/lib/libnss_sss.so.2" \ NSS_WRAPPER_MODULE_FN_PREFIX="sss" \ UID_WRAPPER=1 \ UID_WRAPPER_ROOT=1 \ fakeroot /usr/bin/python2 /usr/bin/py.test -v --tb=native . ============================================================================== test session starts =============================================================================== platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.7.3 -- /usr/bin/python2 rootdir: /root/sssd.git/src/tests/intg, inifile: collected 73 items
ent_test.py::test_assert_passwd_by_name PASSED ent_test.py::test_assert_passwd_by_uid PASSED ent_test.py::test_assert_passwd_list PASSED ent_test.py::test_assert_each_passwd_by_name PASSED ent_test.py::test_assert_each_passwd_by_uid PASSED ent_test.py::test_assert_each_passwd_with_name PASSED ent_test.py::test_assert_each_passwd_with_uid PASSED ent_test.py::test_assert_passwd PASSED ent_test.py::test_group_member_matching PASSED ent_test.py::test_assert_group_by_name PASSED ent_test.py::test_assert_group_by_gid PASSED ent_test.py::test_assert_group_list PASSED ent_test.py::test_assert_each_group_by_name PASSED ent_test.py::test_assert_each_group_by_gid PASSED ent_test.py::test_assert_each_group_with_name PASSED ent_test.py::test_assert_each_group_with_gid PASSED ent_test.py::test_assert_group PASSED ldap_local_override_test.py::test_simple_user_override PASSED ldap_local_override_test.py::test_root_user_override PASSED ldap_local_override_test.py::test_replace_user_override PASSED ldap_local_override_test.py::test_remove_user_override PASSED ldap_local_override_test.py::test_imp_exp_user_override PASSED ldap_local_override_test.py::test_show_user_override PASSED ldap_local_override_test.py::test_find_user_override PASSED ldap_local_override_test.py::test_simple_group_override PASSED ldap_local_override_test.py::test_root_group_override PASSED ldap_local_override_test.py::test_replace_group_override PASSED ldap_local_override_test.py::test_remove_group_override PASSED ldap_local_override_test.py::test_imp_exp_group_override PASSED ldap_local_override_test.py::test_regr_2802_override PASSED ldap_local_override_test.py::test_regr_2757_override PASSED ldap_local_override_test.py::test_regr_2790_override PASSED ldap_test.py::test_regression_ticket2163 PASSED ldap_test.py::test_sanity_rfc2307 PASSED ldap_test.py::test_sanity_rfc2307_bis PASSED ldap_test.py::test_refresh_after_cleanup_task PASSED ldap_test.py::test_add_remove_user PASSED ldap_test.py::test_add_remove_group_rfc2307 PASSED ldap_test.py::test_add_remove_group_rfc2307_bis PASSED ldap_test.py::test_add_remove_membership_rfc2307 PASSED ldap_test.py::test_add_remove_membership_rfc2307_bis PASSED ldap_test.py::test_override_homedir PASSED ldap_test.py::test_fallback_homedir PASSED ldap_test.py::test_override_shell PASSED ldap_test.py::test_shell_fallback PASSED ldap_test.py::test_default_shell PASSED ldap_test.py::test_vetoed_shells PASSED test_local_domain.py::test_wrong_LC_ALL PASSED test_memory_cache.py::test_getpwnam PASSED test_memory_cache.py::test_getpwnam_with_mc PASSED test_memory_cache.py::test_getgrnam_simple PASSED test_memory_cache.py::test_getgrnam_simple_with_mc PASSED test_memory_cache.py::test_getgrnam_membership PASSED test_memory_cache.py::test_getgrnam_membership_with_mc PASSED test_memory_cache.py::test_initgroups PASSED test_memory_cache.py::test_initgroups_with_mc PASSED test_memory_cache.py::test_initgroups_fqname_with_mc PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc1 PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc2 PASSED test_memory_cache.py::test_initgroups_case_insensitive_with_mc3 PASSED test_memory_cache.py::test_invalidation_of_gids_after_initgroups PASSED test_memory_cache.py::test_initgroups_without_change_in_membership PASSED test_memory_cache.py::test_invalidate_user_before_stop PASSED test_memory_cache.py::test_invalidate_user_after_stop PASSED test_memory_cache.py::test_invalidate_users_before_stop PASSED test_memory_cache.py::test_invalidate_users_after_stop PASSED test_memory_cache.py::test_invalidate_group_before_stop PASSED test_memory_cache.py::test_invalidate_group_after_stop PASSED test_memory_cache.py::test_invalidate_groups_before_stop PASSED test_memory_cache.py::test_invalidate_groups_after_stop PASSED test_memory_cache.py::test_invalidate_everything_before_stop PASSED test_memory_cache.py::test_invalidate_everything_after_stop PASSED test_memory_cache.py::test_removed_mc PASSED
========================================================================== 73 passed in 206.11 seconds =========================================================================== rm -f /tmp/sssd-intg.icQ2aGpF/var/log/sssd/* make[1]: Leaving directory '/root/sssd.git/x86_64/intg/bld/src/tests/intg' [root@sssd1 x86_64]# cat /etc/fedora-release Fedora release 23 (Twenty Three)
On 2/29/16 3:18 AM, Jakub Hrozek wrote:
On Sun, Feb 28, 2016 at 08:19:57PM -0500, Dan Lavu wrote:
I've made most of the the suggested changes but I'm going to take sometime and get the test running on Debian as well (Mostly to find out if /etc/pki is a Red Hat thing or not). Fedora and Debian are the only distros we are testing/supporting against correct?
Yes, we support RHEL >= 6, Fedora (all supported versions) and Debian Testing.
Also wondering if the ci setup issue I'm seeing applies to apt.
Dan
On 2/26/16 5:53 AM, Jakub Hrozek wrote:
On Thu, Feb 25, 2016 at 05:18:09PM -0500, Dan Lavu wrote:
Here is a patch for https://fedorahosted.org/sssd/ticket/2820
First real patch... criticisms to for what I need to improve on are welcome, including concepts that I should learn, thanks.
Thanks a lot for the patch!
See my comments inline:
From 529adb3e0d763a8ee9ba9b4c5b13f933d723e8de Mon Sep 17 00:00:00 2001 From: Dan Lavu dlavu@redhat.com Date: Fri, 5 Feb 2016 08:51:07 -0500 Subject: [PATCH] Adding SSL encryption to integration tests.
src/tests/intg/ca.py | 166 ++++++++++++++++++++++++++++++++++++++++++ src/tests/intg/ds_openldap.py | 14 ++++ 2 files changed, 180 insertions(+) create mode 100644 src/tests/intg/ca.py
diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py new file mode 100644 index 0000000000000000000000000000000000000000..a44a92e5d5053338dabd7d8d82d2b1d50ec7594e --- /dev/null +++ b/src/tests/intg/ca.py @@ -0,0 +1,166 @@ +# +# 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 fnmatch
+class CA:
It would be nice to use the new-style classes, so class CA(object)
- """CA Class"""
- def __init__(self, subject=None, country=None, state=None,
city=None, organization=None, unit=None, config_dir=None):if subject is None:self.subject = socket.gethostname()if country is None:self.country = 'US'if state is None:self.state = 'NC'if city is None:self.city = 'Raleigh'if organization is None:self.organization = 'Red Hat'if unit is None:self.unit = 'SSSD'if config_dir is None:self.config_dir = '/etc/pki'/etc/pki is not writable unless you're root. We should store the certs in another directory writable by any user. Maybe this is something Nikolai (CC) could help us with, I know we use fakeroot to set up the directory structure, but I'm fuzzy on the details, so I don't know myself which part of the tests we should exactly touch..
Also, does the /etc/pki path exists on Debian and other distributions or is it Red Hat-centric?
When we have this done, hopefully we can remove the use of 'ldap_auth_disable_tls_never_use_in_production' from our tests?
self.hostname = socket.gethostname()This is maybe something to fix in a later iteration of the patch, but I wonder if it was useful to override the hostname to something else than what gethostname() reports. Not sure at the moment..
self.csr_dir = self.config_dir + '/CA/newcerts'self.key_dir = self.config_dir + '/tls/private'self.cert_dir = self.config_dir + '/tls/certs'self.index = int(1000)- def setup(self):
"""Setup CA using OpenSSL"""cacert = socket.gethostname() + '-ca.crt'cakey = socket.gethostname() + '-ca.key'Instead of using socket.gethostname(), maybe using self.hostname would be better here (and elsewhere) ?
if not exists(join(self.cert_dir, cacert)) or not exists(join(self.key_dir, cakey)):key = crypto.PKey()key.generate_key(crypto.TYPE_RSA, 2048)ca = crypto.X509()ca.get_subject().C = self.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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')
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... dpkg -ihttp://security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+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 Lavudan@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, 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.
- """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 = countryself.state = stateself.city = cityself.organization = organizationself.unit = unitself.hostname = socket.gethostname()if config.SSL_PATH is not '/tmp':self.config_dir = config_direlse: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.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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 certificaterequired: csr -csr_typeoptional: textFalse - 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_crtelse: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 CSRoptional: fqdn, socket.gethostname() (Default)optional: textFalse - 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 = fqdncsr.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_csrelse: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
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 Lavudan@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, 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.
- """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 = countryself.state = stateself.city = cityself.organization = organizationself.unit = unitself.hostname = socket.gethostname()if config.SSL_PATH is not '/tmp':self.config_dir = config_direlse: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 notexists(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.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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_crtThis 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 certificaterequired: csr -csr_typeoptional: textFalse - 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_crtelse:return certIt 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 CSRoptional: fqdn, socket.gethostname() (Default)optional: textFalse - 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 = fqdncsr.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_csrelse:return csrThe 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
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 < 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
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 Lavudan@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, 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.countryca.get_subject().ST = self.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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
Hi Dan,
On 03/12/2016 12:20 AM, 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.
Sorry, missed your reply somehow. Answering the only question Lukas didn't answer, below.
On Fri, Mar 11, 2016 at 1:16 PM, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com mailto:Nikolai.Kondrashov@redhat.com> wrote:
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_dirOK, 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.
Wow! Good :)
- def get_cert(self, csr, text=False):
"""Retrieves certificaterequired: csr -csr_typeoptional: textFalse - 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.
I mean the filenames (not certificates) are being generated multiple times throughout the code, which easily leads to typos and broken code, and will be better to generate the filenames once, store it in a class member and refer to that instead.
Hit me on the IRC if anything is still unclear, or if you're unsure how to proceed. Make sure I can access/read the code you'll be referring to, though, please :)
Nick
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/li bnss-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..c7adb7a2f0c2ea7f34 2c9df1d6ad41dcb2fcc28e --- /dev/null +++ b/src/tests/intg/ca.py @@ -0,0 +1,163 @@ +# +# SSSD LOCAL domain tests +# +# Copyright (c) 2016 Red Hat, Inc. +# Author: Dan Lavudan@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, 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.
- """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 = countryself.state = stateself.city = cityself.organization = organizationself.unit = unitself.hostname = socket.gethostname()if config.SSL_PATH is not '/tmp':self.config_dir = config_direlse: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_dirAs 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 notexists(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.stateca.get_subject().L = self.cityca.get_subject().O = self.organizationca.get_subject().OU = self.unitca.get_subject().CN = self.subjectca.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_crtThis 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 certificaterequired: csr -csr_typeoptional: textFalse - 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_crtelse:return certIt 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 CSRoptional: fqdn, socket.gethostname() (Default)optional: textFalse - 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 = fqdncsr.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_csrelse:return csrThe 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
On 03/06/2017 09:34 PM, Dan Lavu wrote:
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?
Glad to see your progress!
Before I proceed with everything else, why don't you just let the DS accept a CA parameter as well as an SSL port?
Like this:
def __init__(self, dir, port, base_dn, admin_rdn, admin_pw, ssl_port=None, ca=None): # ... self.ssl_port = ssl_port self.ca = ca # ...
And then perhaps make it set up the SSL socket if the port is passed, in *addition* to the regular socket.
I.e. have this in the constructor:
self.ldap_url = "ldap://localhost:" + str(self.port) if self.ssl_port: self.ldaps_url = "ldaps://localhost:" + str(self.ssl_port)
Nick
Nik,
Updated.
Dan
On Tue, Mar 7, 2017 at 4:11 AM, Nikolai Kondrashov < Nikolai.Kondrashov@redhat.com> wrote:
On 03/06/2017 09:34 PM, Dan Lavu wrote:
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?
Glad to see your progress!
Before I proceed with everything else, why don't you just let the DS accept a CA parameter as well as an SSL port?
Like this:
def __init__(self, dir, port, base_dn, admin_rdn, admin_pw,ssl_port=None, ca=None): # ... self.ssl_port = ssl_port self.ca = ca # ...
And then perhaps make it set up the SSL socket if the port is passed, in *addition* to the regular socket.
I.e. have this in the constructor:
self.ldap_url = "ldap://localhost:" + str(self.port) if self.ssl_port: self.ldaps_url = "ldaps://localhost:" + str(self.ssl_port)Nick _______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-leave@lists.fedorahosted.org
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_portself.ca = caif 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_portself.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_urlmodlist = [(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")- 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
Nik,
Thanks for the review, the responses are inline.
On Wed, Mar 8, 2017 at 1:59 AM, Nikolai Kondrashov < Nikolai.Kondrashov@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_portself.ca = caif 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_portself.ca = caYou 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 += 1Could 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 += 1Same 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_urlmodlist = [(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")- 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
Hi Dan,
On 03/09/2017 12:33 PM, Dan Lavu wrote:
Thanks for the review, the responses are inline.
On Wed, Mar 8, 2017 at 1:59 AM, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com mailto:Nikolai.Kondrashov@redhat.com> wrote: 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.
Hmm, are you talking about docstrings: https://www.python.org/dev/peps/pep-0257/ ?
if attempt > 30: raise Exception("Failed to start slapd") time.sleep(1) + if self.ca <http://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?
Sure.
I'm suggesting making the DS interface support this usage:
@pytest.fixture(scope="module") def ca_inst(request): """CA instance fixture""" hostname = socket.gethostname() ca_inst = ca_openssl.CAOpenSSL(config.PREFIX, hostname, "US", "NC", "Raleigh", "Red Hat Inc.", "SSSD") try: ca_inst.setup() except: ca_inst.teardown() raise request.addfinalizer(lambda: ca_inst.teardown()) return ca_inst
@pytest.fixture(scope="module") def ds_inst(request, ca_inst): """LDAP server instance fixture""" ds_inst = ds_openldap.DSOpenLDAP( config.PREFIX, 10389, LDAP_BASE_DN, "cn=admin", "Secret123", ssl_port=10636, ca=ca_inst )
try: ds_inst.setup() except: ds_inst.teardown() raise request.addfinalizer(lambda: ds_inst.teardown()) return ds_inst
Nick
On Thu, Feb 25, 2016 at 03:09:25PM +0200, Nikolai Kondrashov wrote:
Hi everyone,
I'd like to continue the discussion of tlog integration, and also present you the first release of tlog - a development preview, which has the configuration interface necessary to implement the integration:
https://github.com/spbnick/tlog/releases/tag/v1You're more than welcome to download RPMs, install, read tlog-rec(8) and tlog-rec.conf(5), and experiment! Building from the Git tree and the tarball works as well, if you're so inclined. I'm also attaching those manpages for convenience.
OK, I will try to find some time to experiment with tlog, but probably won't happen until next week, though.
Here are the integration plans so far, as discussed with Jakub on our devconf.cz trip meetings and before on the list. Jakub, please correct me or add details.
- We follow the route similar to that taken by SELinux rule control implementation [1][2]. I.e. store the configuration in LDAP HBAC rules, write to files on the client side and then specify them to tlog upon user login.
This was an idea we had during a conversation on devconf. I think it would be nice to also bring this idea up with the IPA developers on the freeipa-devel list to get more feedback.
However, I'm also rather fond of the idea of specifying the whole configuration through an environment variable instead of through a file referenced by an environment variable - it's not big at all, and we'll avoid the hassle of managing the files.
I implemented support for both in tlog (was easy).
- We'll have to make nss_sss report user's shell as tlog-rec (how?) and specify the actual shell to tlog-rec via an environment variable, through pam_sss (with SSS_PAM_ENV_ITEM messages). I.e.:
We already can override the user's shell on a global basis from the config file and on a per-user basis with local overrides (or even IPA overrides on an IPA client)
- Nss_sss would always report tlog-rec as the user's shell.
That would work with the user overrides. pam_sss would also have to pass on the actual user's shell if the original shell was tlog, right?
- During login (e.g. through "login" or "sshd") pam_sss would add a variable to the user environment, containing, or pointing at, a tlog-rec configuration (TLOG_REC_CONF_TEXT or TLOG_REC_CONF_FILE). That configuration would contain the user's actual shell.
Just to be clear, the user's actuall shell wouldn't be bart of the tlog configuration, just passed on from sssd to tlog, right?
About the config file, should we add a sssd option to specify a tlog config? Also, can tlog fall-back to some compiled-in global config file in case no particular config file is needed? (Can tlog work with just the global config file and the user's original shell?)
I can also implement support for a separate variable just for the shell (TLOG_REC_SHELL?) to simplify the implementation for the start.
- Tlog-rec would read the system-wide configuration and overlay it with the one specified in the environment, adding the specific user shell, and then would spawn it.
Please also see the draft integration design page [3] for reference. I hope to refine and extend it in the coming weeks to match FreeIPA standards.
Please chime in and suggest, object, discuss!
Also, please report tlog bugs at https://github.com/spbnick/tlog/issues
Thank you!
Nick
tlog-rec(8) System Manager's Manual tlog-rec(8)
NAME tlog-rec - start a shell and log terminal I/O
SYNOPSIS tlog-rec [OPTION...] [CMD_FILE [CMD_ARG...]] tlog-rec -c [OPTION...] CMD_STRING [CMD_NAME [CMD_ARG...]]
DESCRIPTION Tlog-rec is a terminal I/O logging program. It starts a shell under a pseudo-TTY, connects it to the actual terminal and logs whatever passes between them including user input, program output, and terminal window size changes.
If no "-c" option is specified, then the first non-option argument CMD_FILE specifies the location of a shell script the shell should read and the following arguments (CMD_ARG) specify its arguments. If the "-c" option is specified, then a non-option argument CMD_STRING is required and should contain shell commands to execute, the following arguments can specify first the script name (CMD_NAME, i.e. argv[0]) and then its arguments (CMD_ARG). If no non-option arguments are encountered, then the shell is started interactively. Tlog-rec loads its parameters first from the systemwide configuration file /usr/local/etc/tlog/tlog-rec.conf, then from the file pointed at by TLOG_REC_CONF_FILE environment variable (if set), then from the con‐ tents of the TLOG_REC_CONF_TEXT environment variable (if set), and then from command-line options. Parameters from each of these sources over‐ ride the previous one in turn.OPTIONS General options -h, --help Output a command-line usage message and exit
-s, --shell=SHELL Spawn the specified SHELL -l, --login Make the shell a login shell -c, --command Execute shell commands --notice=TEXT Print TEXT message before starting recording --latency=SECONDS Cache captured data SECONDS seconds before logging Value minimum: 1 --payload=BYTES Limit encoded data to BYTES bytes Value minimum: 32 --writer=STRING Use STRING log writer (syslog/file, default syslog) Value should be one of: "syslog", "file"Logged data set options --log-input[=BOOL] Enable/disable logging user input
--log-output[=BOOL] Enable/disable logging program output --log-window[=BOOL] Enable/disable logging terminal window size changesFile writer options --file-path=FILE Log to FILE file
Syslog writer options --syslog-facility=STRING Log with STRING syslog facility
Value should be one of: "auth", "authpriv", "cron", "daemon", "ftp", "kern", "local0", "local1", "local2", "local3", "local4", "local5", "local6", "local7", "lpr", "mail", "news", "syslog", "user", "uucp" --syslog-priority=STRING Log with STRING syslog priority Value should be one of: "emerg", "alert", "crit", "err", "warn‐ ing", "notice", "info", "debug"ENVIRONMENT TLOG_REC_CONF_FILE Specifies the location of a configuration file to be read. The configuration parameters in this file override the ones in the systemwide configuration file /usr/local/etc/tlog/tlog-rec.conf.
TLOG_REC_CONF_TEXT Specifies the configuration text to be read. The configuration parameters in this variable override the ones in the file speci‐ fied with TLOG_REC_CONF_FILE.FILES /usr/local/etc/tlog/tlog-rec.conf The systemwide configuration file
EXAMPLES Start recording a login shell: tlog-rec -l
Start recording a zsh session: tlog-rec -s /usr/bin/zsh Record everything but user input: tlog-rec --log-input=off --log-output=on --log-window=on Ask the recorded shell to execute a command: tlog-rec -c whoamiSEE ALSO tlog-rec.conf(5)
AUTHOR Nikolai Kondrashov spbnick@gmail.com
Tlog February 2016 tlog-rec(8)
tlog-rec(5) File Formats Manual tlog-rec(5)
NAME tlog-rec.conf - tlog-rec configuration file
DESCRIPTION tlog-rec.conf is a JSON-format configuration file for tlog-rec program. Contrary to the strict JSON specification, both C and C++ style com‐ ments are allowed in the file.
The file must contain a single JSON object with the objects and fields described below. Almost all of them are optional and assume a default value. However, those that do require a value can still be omitted and specified to tlog-rec in other ways: through environment variables or command line.OBJECTS AND FIELDS Root object shell (string) The path to the shell executable that should be spawned.
Default: "/bin/bash" notice (string) A message which will be printed before starting recording and the user shell. Can be used to warn the user that the session is recorded. Default: "\nATTENTION! Your session is being recorded!\n\n" latency (integer) The data which does not exceed maximum payload stays in memory and is not logged until this number of seconds elapses. Minimum: 1 Default: 10 payload (integer) Maximum encoded data (payload) size per message, bytes. As soon as payload exceeds this number of bytes, it is formatted into a message and logged. Minimum: 32 Default: 2048 log (object) Logged data set object, see below. writer (string) The type of "log writer" to use for logging. The writer needs to be configured using its dedicated parameters. One of: "syslog", "file" Default: "syslog" file (object) File writer object, see below. syslog (object) Syslog writer object, see below.log - Logged data set object input (boolean) If specified as true, user input is logged.
Default: true output (boolean) If specified as true, terminal output is logged. Default: true window (boolean) If specified as true, terminal window size changes are logged. Default: truefile - File writer object path (string) The "file" writer log file path.
No default.syslog - Syslog writer object facility (string) Syslog facility the "syslog" writer should use for the messages.
One of: "auth", "authpriv", "cron", "daemon", "ftp", "kern", "local0", "local1", "local2", "local3", "local4", "local5", "local6", "local7", "lpr", "mail", "news", "syslog", "user", "uucp" Default: "authpriv" priority (string) Syslog priority the "syslog" writer should use for the messages. One of: "emerg", "alert", "crit", "err", "warning", "notice", "info", "debug" Default: "info"EXAMPLES A config specifying only a shell:
{ "shell": "/usr/bin/zsh" } A config disabling logging user input: { "log": { "input": false } } A config specifying logging to a file: { "writer": "file" "file" : { "path": "/var/log/tlog-rec.log" } }SEE ALSO tlog-rec(8), http://json.org/
AUTHOR Nikolai Kondrashov spbnick@gmail.com
Tlog February 2016 tlog-rec(5)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On 02/26/2016 12:37 PM, Jakub Hrozek wrote:
On Thu, Feb 25, 2016 at 03:09:25PM +0200, Nikolai Kondrashov wrote:
I'd like to continue the discussion of tlog integration, and also present you the first release of tlog - a development preview, which has the configuration interface necessary to implement the integration:
https://github.com/spbnick/tlog/releases/tag/v1You're more than welcome to download RPMs, install, read tlog-rec(8) and tlog-rec.conf(5), and experiment! Building from the Git tree and the tarball works as well, if you're so inclined. I'm also attaching those manpages for convenience.
OK, I will try to find some time to experiment with tlog, but probably won't happen until next week, though.
Thanks, Jakub!
Here are the integration plans so far, as discussed with Jakub on our devconf.cz trip meetings and before on the list. Jakub, please correct me or add details.
- We follow the route similar to that taken by SELinux rule control implementation [1][2]. I.e. store the configuration in LDAP HBAC rules, write to files on the client side and then specify them to tlog upon user login.
This was an idea we had during a conversation on devconf. I think it would be nice to also bring this idea up with the IPA developers on the freeipa-devel list to get more feedback.
Alright. I need to see and understand how this works myself before I can discuss this productively, though. So I'd better start digging into it.
It seems I'll have to implement it myself, so I'll have to know it anyway :)
However, I'm also rather fond of the idea of specifying the whole configuration through an environment variable instead of through a file referenced by an environment variable - it's not big at all, and we'll avoid the hassle of managing the files.
I implemented support for both in tlog (was easy).
- We'll have to make nss_sss report user's shell as tlog-rec (how?) and specify the actual shell to tlog-rec via an environment variable, through pam_sss (with SSS_PAM_ENV_ITEM messages). I.e.:
We already can override the user's shell on a global basis from the config file and on a per-user basis with local overrides (or even IPA overrides on an IPA client)
But we can't have these overrides configured centrally, on the IPA server? Otherwise they won't be called "local", right? If so, then this can only work for a tech preview and even then it would be better to have central configuration.
- Nss_sss would always report tlog-rec as the user's shell.
That would work with the user overrides. pam_sss would also have to pass on the actual user's shell if the original shell was tlog, right?
Yes.
- During login (e.g. through "login" or "sshd") pam_sss would add a variable to the user environment, containing, or pointing at, a tlog-rec configuration (TLOG_REC_CONF_TEXT or TLOG_REC_CONF_FILE). That configuration would contain the user's actual shell.
Just to be clear, the user's actuall shell wouldn't be bart of the tlog configuration, just passed on from sssd to tlog, right?
Tlog has global configuration (/etc/tlog/tlog-rec.conf) which specifies a shell to start. Which is /usr/bin/bash by default. However, whatever we pass through the environment overrides that.
Sssd doesn't need to change the global configuration at all. *All* parameters present there can be overriden through the environment variables mentioned above.
About the config file, should we add a sssd option to specify a tlog config?
If it's global, i.e. for all users/groups, then it doesn't make much sense, as the sysadmin can just change the global tlog config.
Also, can tlog fall-back to some compiled-in global config file in case no particular config file is needed? (Can tlog work with just the global config file and the user's original shell?)
Sure, it doesn't require those environment variables to be present. It will spawn the shell specified in its global config file. However, it would have no way of knowing what the *intended* user shell is.
Nick
On Mon, Feb 29, 2016 at 07:36:51PM +0200, Nikolai Kondrashov wrote:
On 02/26/2016 12:37 PM, Jakub Hrozek wrote:
On Thu, Feb 25, 2016 at 03:09:25PM +0200, Nikolai Kondrashov wrote:
I'd like to continue the discussion of tlog integration, and also present you the first release of tlog - a development preview, which has the configuration interface necessary to implement the integration:
https://github.com/spbnick/tlog/releases/tag/v1You're more than welcome to download RPMs, install, read tlog-rec(8) and tlog-rec.conf(5), and experiment! Building from the Git tree and the tarball works as well, if you're so inclined. I'm also attaching those manpages for convenience.
OK, I will try to find some time to experiment with tlog, but probably won't happen until next week, though.
Thanks, Jakub!
Here are the integration plans so far, as discussed with Jakub on our devconf.cz trip meetings and before on the list. Jakub, please correct me or add details.
- We follow the route similar to that taken by SELinux rule control implementation [1][2]. I.e. store the configuration in LDAP HBAC rules, write to files on the client side and then specify them to tlog upon user login.
This was an idea we had during a conversation on devconf. I think it would be nice to also bring this idea up with the IPA developers on the freeipa-devel list to get more feedback.
Alright. I need to see and understand how this works myself before I can discuss this productively, though. So I'd better start digging into it.
It seems I'll have to implement it myself, so I'll have to know it anyway :)
However, I'm also rather fond of the idea of specifying the whole configuration through an environment variable instead of through a file referenced by an environment variable - it's not big at all, and we'll avoid the hassle of managing the files.
I implemented support for both in tlog (was easy).
- We'll have to make nss_sss report user's shell as tlog-rec (how?) and specify the actual shell to tlog-rec via an environment variable, through pam_sss (with SSS_PAM_ENV_ITEM messages). I.e.:
We already can override the user's shell on a global basis from the config file and on a per-user basis with local overrides (or even IPA overrides on an IPA client)
But we can't have these overrides configured centrally, on the IPA server? Otherwise they won't be called "local", right? If so, then this can only work for a tech preview and even then it would be better to have central configuration.
- Nss_sss would always report tlog-rec as the user's shell.
That would work with the user overrides. pam_sss would also have to pass on the actual user's shell if the original shell was tlog, right?
Yes.
- During login (e.g. through "login" or "sshd") pam_sss would add a variable to the user environment, containing, or pointing at, a tlog-rec configuration (TLOG_REC_CONF_TEXT or TLOG_REC_CONF_FILE). That configuration would contain the user's actual shell.
Just to be clear, the user's actuall shell wouldn't be bart of the tlog configuration, just passed on from sssd to tlog, right?
Tlog has global configuration (/etc/tlog/tlog-rec.conf) which specifies a shell to start. Which is /usr/bin/bash by default. However, whatever we pass through the environment overrides that.
Sssd doesn't need to change the global configuration at all. *All* parameters present there can be overriden through the environment variables mentioned above.
About the config file, should we add a sssd option to specify a tlog config?
If it's global, i.e. for all users/groups, then it doesn't make much sense, as the sysadmin can just change the global tlog config.
Also, can tlog fall-back to some compiled-in global config file in case no particular config file is needed? (Can tlog work with just the global config file and the user's original shell?)
Sure, it doesn't require those environment variables to be present. It will spawn the shell specified in its global config file. However, it would have no way of knowing what the *intended* user shell is.
I was playing with tlog yesterday and for the 'local configuration' I suggest we start small and avoid adding too many options, because we'd have to support them for a long time.
It seems to me that all the options except the user's shell could just be set in the global tlog configuration. Maybe, if some users complain that they would like some per-user configs, tlog could offer a /etc/tlog.d directory where a file named after the user would override some settings, but I wouldn't go there before we hear some demand myself..
I was even wondering if it wasn't easiest to always set the original shell as a PAM env variable if a shell is overriden? If we did that, we could just use the existing options to override the shell either globally for all users or using the local overrides. If we don't want to set the env variable always, we could special-case tlog-rec, but I'm not a big fan of special-casing..
On 03/04/2016 12:54 PM, Jakub Hrozek wrote:
I was playing with tlog yesterday and for the 'local configuration' I suggest we start small and avoid adding too many options, because we'd have to support them for a long time.
Agreed.
It seems to me that all the options except the user's shell could just be set in the global tlog configuration. Maybe, if some users complain that they would like some per-user configs, tlog could offer a /etc/tlog.d directory where a file named after the user would override some settings, but I wouldn't go there before we hear some demand myself..
Yes, I wouldn't like tlog to have that kind of mechanism. At least not until we get feedback from users about how they would prefer to use it.
I was even wondering if it wasn't easiest to always set the original shell as a PAM env variable if a shell is overriden? If we did that, we could just use the existing options to override the shell either globally for all users or using the local overrides. If we don't want to set the env variable always, we could special-case tlog-rec, but I'm not a big fan of special-casing..
I too would prefer to have an env variable set always than have a special case. I.e. it would require setting TLOG_REC_CONF_TEXT to:
{"shell": "ACTUAL_JSON_ESCAPED_USER_SHELL"}
As an alternative I can add support for a TLOG_REC_SHELL variable which value wouldn't need escaping. Perhaps I'll do that in any case as it can be useful for others.
If having users local-override the shell to tlog-rec is suitable for the configuration approach for the start, then that should work.
Nick
On Thu, Mar 10, 2016 at 09:10:30PM +0200, Nikolai Kondrashov wrote:
On 03/04/2016 12:54 PM, Jakub Hrozek wrote:
I was playing with tlog yesterday and for the 'local configuration' I suggest we start small and avoid adding too many options, because we'd have to support them for a long time.
Agreed.
It seems to me that all the options except the user's shell could just be set in the global tlog configuration. Maybe, if some users complain that they would like some per-user configs, tlog could offer a /etc/tlog.d directory where a file named after the user would override some settings, but I wouldn't go there before we hear some demand myself..
Yes, I wouldn't like tlog to have that kind of mechanism. At least not until we get feedback from users about how they would prefer to use it.
I was even wondering if it wasn't easiest to always set the original shell as a PAM env variable if a shell is overriden? If we did that, we could just use the existing options to override the shell either globally for all users or using the local overrides. If we don't want to set the env variable always, we could special-case tlog-rec, but I'm not a big fan of special-casing..
I too would prefer to have an env variable set always than have a special case. I.e. it would require setting TLOG_REC_CONF_TEXT to:
{"shell": "ACTUAL_JSON_ESCAPED_USER_SHELL"}As an alternative I can add support for a TLOG_REC_SHELL variable which value wouldn't need escaping. Perhaps I'll do that in any case as it can be useful for others.
Is the escaping easy to do or would it require a dependency?
On 03/11/2016 12:21 PM, Jakub Hrozek wrote:
On Thu, Mar 10, 2016 at 09:10:30PM +0200, Nikolai Kondrashov wrote:
On 03/04/2016 12:54 PM, Jakub Hrozek wrote:
I was even wondering if it wasn't easiest to always set the original shell as a PAM env variable if a shell is overriden? If we did that, we could just use the existing options to override the shell either globally for all users or using the local overrides. If we don't want to set the env variable always, we could special-case tlog-rec, but I'm not a big fan of special-casing..
I too would prefer to have an env variable set always than have a special case. I.e. it would require setting TLOG_REC_CONF_TEXT to:
{"shell": "ACTUAL_JSON_ESCAPED_USER_SHELL"}As an alternative I can add support for a TLOG_REC_SHELL variable which value wouldn't need escaping. Perhaps I'll do that in any case as it can be useful for others.
Is the escaping easy to do or would it require a dependency?
It's easy to do. It's similar to C string escaping with minor differences. See http://json.org/
Nick
sssd-devel@lists.fedorahosted.org
-
Dan Lavu -
Dan Lavu -
Jakub Hrozek -
Lukas Slebodnik -
Nikolai Kondrashov