Tlog integration and packages

[Nikolai Kondrashov]

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

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

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&q...
/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'

[Jakub Hrozek]

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/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. 
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
>  
>  [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 
>  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 changes
>  
>     File 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 whoami
>  
>  SEE ALSO
>         tlog-rec.conf(5)
>  
>  AUTHOR
>         Nikolai Kondrashov <spbnick(a)gmail.com&gt;
>  
>  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: true
>  
>     file - 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(a)gmail.com&gt;
>  
>  Tlog                             February 2016                     tlog-rec(5) 
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/admin/lists/[e-mail]

[PATCH]: test ldap provider with TLS or SSL

[Jakub Hrozek]

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(a)redhat.com&gt;
>  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(a)redhat.com&gt;
>  +#
>  +# 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.country
> +            ca.get_subject().ST = self.state
> +            ca.get_subject().L = self.city
> +            ca.get_subject().O = self.organization
> +            ca.get_subject().OU = self.unit
> +            ca.get_subject().CN = self.subject
> +            ca.set_serial_number(self.index)
> +            ca.gmtime_adj_notBefore(0)
> +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> +            ca.set_issuer(ca.get_subject())
> +            ca.set_pubkey(key)
> +            ca.sign(key, 'sha1')

[PATCH]: test ldap provider with TLS or SSL

[Lukas Slebodnik]

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(a)redhat.com&gt;
> > 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(a)redhat.com&gt;
> > +#
> > +# 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

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

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(a)redhat.com&gt;
> > 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(a)redhat.com&gt;
> > +#
> > +# 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.country
> > +            ca.get_subject().ST = self.state
> > +            ca.get_subject().L = self.city
> > +            ca.get_subject().O = self.organization
> > +            ca.get_subject().OU = self.unit
> > +            ca.get_subject().CN = self.subject
> > +            ca.set_serial_number(self.index)
> > +            ca.gmtime_adj_notBefore(0)
> > +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> > +            ca.set_issuer(ca.get_subject())
> > +            ca.set_pubkey(key)
> > +            ca.sign(key, 'sha1')

[PATCH]: test ldap provider with TLS or SSL

[Jakub Hrozek]

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(a)redhat.com&gt;
> >>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(a)redhat.com&gt;
> >>+#
> >>+# 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.country
> >>+            ca.get_subject().ST = self.state
> >>+            ca.get_subject().L = self.city
> >>+            ca.get_subject().O = self.organization
> >>+            ca.get_subject().OU = self.unit
> >>+            ca.get_subject().CN = self.subject
> >>+            ca.set_serial_number(self.index)
> >>+            ca.gmtime_adj_notBefore(0)
> >>+            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> >>+            ca.set_issuer(ca.get_subject())
> >>+            ca.set_pubkey(key)
> >>+            ca.sign(key, 'sha1')
>

[Nikolai Kondrashov]

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

[Jakub Hrozek]

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

[Nikolai Kondrashov]

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

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

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



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(a)redhat.com&gt;
> >>> 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(a)redhat.com&gt;
> >>> +#
> >>> +# 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.country
> >>> +            ca.get_subject().ST = self.state
> >>> +            ca.get_subject().L = self.city
> >>> +            ca.get_subject().O = self.organization
> >>> +            ca.get_subject().OU = self.unit
> >>> +            ca.get_subject().CN = self.subject
> >>> +            ca.set_serial_number(self.index)
> >>> +            ca.gmtime_adj_notBefore(0)
> >>> +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> >>> +            ca.set_issuer(ca.get_subject())
> >>> +            ca.set_pubkey(key)
> >>> +            ca.sign(key, 'sha1')

[Jakub Hrozek]

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?

[Nikolai Kondrashov]

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

[PATCH]: test ldap provider with TLS or SSL

[Nikolai Kondrashov]

Hi Dan,

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

On 03/11/2016 06:29 AM, Dan Lavu wrote:
>  Updated patch is attached,
>
>  There were a few more packages I had to install to get CI running for Debian, should we
> had these to the makefile?
>
>  root@sssd2:~# apt-get python-openssl
>
>  dpkg
> -ihttp://ftp.us.debian.org/debian/pool/main/n/nss-wrapper/libnss-wrapper_1.1.2-1_amd64.deb
>  dpkg
> -ihttp://security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+deb8u4_amd64.deb
I'm not sure if libnss-wrapper is in Debian Testing, but we certainly
shouldn't be downloading packages from a URL, especially non-debian packages,
but use the repos configured on the machine.

>  Below are test runs against Debian and Fedora 
>  root@sssd2:~/sssd/x86_64# cat /etc/debian_version
>  8.3 
This is Debian Stable and we don't support that - we support Debian Testing.
Ping me if you need help installing that.

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

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

>  +
>  +    def __init__(self, subject=None, country='US', state='NC',
>  +                 city='Raleigh', organization='Red Hat Inc.',
> unit='SSSD', config_dir=config.SSL_PATH):
>  +        if subject is None:
>  +            self.subject = socket.gethostname()
>  +        self.country = country
>  +        self.state = state
>  +        self.city = city
>  +        self.organization = organization
>  +        self.unit = unit
>  +        self.hostname = socket.gethostname()
>  +        if config.SSL_PATH is not '/tmp':
>  +            self.config_dir = config_dir
>  +        else:
>  +            self.config_dir = config_dir + '/ssl' 
This kind of logic can confuse the class user, and make it impossible to
actually put files into /tmp if necessary.

I would replace this with just:

     self.config_dir = config_dir

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

>  +        self.csr_dir = self.config_dir + '/newcerts'
>  +        self.key_dir = self.config_dir + '/private'
>  +        self.cert_dir = self.config_dir + '/certs'
>  +
>  +        self.index = int(1000)
>  +
>  +
>  +    def setup(self):
>  +        """Setup CA using OpenSSL"""
>  +        cacert = self.hostname + '-ca.crt'
>  +        cakey = self.hostname + '-ca.key'
>  +
>  +        if not exists(join(self.cert_dir, cacert)) or not exists(join(self.key_dir,
> cakey)): 
It's better to use the fully-qualified name of the "join" function here so
it's obvious this is not just some generic "join". I.e. please write
"os.path.join" instead. Same goes for "exists".

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

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

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

>  +            ca.get_subject().C = self.country
>  +            ca.get_subject().ST = self.state
>  +            ca.get_subject().L = self.city
>  +            ca.get_subject().O = self.organization
>  +            ca.get_subject().OU = self.unit
>  +            ca.get_subject().CN = self.subject
>  +            ca.set_serial_number(self.index)
>  +            ca.gmtime_adj_notBefore(0)
>  +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
>  +            ca.set_issuer(ca.get_subject())
>  +            ca.set_pubkey(key)
>  +            ca.sign(key, 'sha1')
>  +
>  +            if not os.path.exists(self.csr_dir):
>  +                os.makedirs(self.csr_dir)
>  +            if not os.path.exists(self.key_dir):
>  +                os.makedirs(self.key_dir)
>  +            if not os.path.exists(self.cert_dir):
>  +                os.makedirs(self.cert_dir)
>  +
>  +            open(os.path.join(self.cert_dir, cacert), 'wt').write \
>  +                (crypto.dump_certificate(crypto.FILETYPE_PEM, ca))
>  +            open(os.path.join(self.key_dir, cakey), 'wt').write \
>  +                (crypto.dump_privatekey(crypto.FILETYPE_PEM, key)) 
Can we use "with" statement here, so that file is closed ASAP, instead of
waiting for GC?

>  +
>  +
>  +    def teardown(self):
>  +        """Teardown CA certificate files"""
>  +        if os.path.exists(self.config_dir) and not '/tmp': 
Hmm, this condition will always be false and the directory would never be
removed. I assume this is a piece of code trying to handle the special case
above and if that's to go then this one can simply be removed as well.

>  +            shutil.rmtree(self.config_dir)
>  +
>  +
>  +    def get_ca(self):
>  +        """Returns CA certificate in ASCII PEM
> encoding"""
>  +        ca = crypto.load_certificate(crypto.FILETYPE_PEM, open\
>  +                    (os.path.join(self.cert_dir, socket.gethostname() +
> '-ca.crt'), 'rt').read())
>  +        ca_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, ca)
>  +
>  +        return ca_crt 
This function name is confusing for the same reason as the "ca" variable
above. Plus it would be better not to return the certificate as text, unless
it's the main use.

>  +    def get_cert(self, csr, text=False):
>  +        """Retrieves certificate
>  +        required: csr -csr_type
>  +        optional: text
>  +            False - will return a pem object type (Default)
>  +            True - will return the certificate as ASCII
>  +        """
>  +        cacert = self.hostname + '-ca.crt'
>  +        cakey = self.hostname + '-ca.key' 
Instead of generating these again, perhaps it is better to put them into
variables at initialization time?

>  +        ca_crt = crypto.load_certificate(crypto.FILETYPE_PEM,
> open(os.path.join(self.cert_dir, cacert), 'rt').read())
>  +        ca_key = crypto.load_privatekey(crypto.FILETYPE_PEM,
> open(os.path.join(self.key_dir, cakey), 'rt').read()) 
Similarly with these paths. Besides, why not use the "get_ca" function here
(after renaming) and perhaps a similar one to retrieve the key?

>  +        cert = crypto.X509()
>  +        cert.set_subject(csr.get_subject())
>  +        cert.set_serial_number(self.index+1)
>  +        cert.gmtime_adj_notBefore(0)
>  +        cert.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
>  +        cert.set_issuer(ca_crt.get_subject())
>  +        cert.set_pubkey(csr.get_pubkey())
>  +        cert.sign(ca_key, 'sha1')
>  +
>  +        open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'),
> 'wt')\
>  +            .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
>  +        server_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, cert)
>  +
>  +        if text is True:
>  +            return server_crt
>  +        else:
>  +            return cert 
It is better to keep a function doing only one thing, and i.e. not implement
optional certificate conversion in this one. After all the caller can do it
themselves easily. So we can remove the "text" argument and not do this
conversion and branching.

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

>  +        server_csr =
> crypto.dump_certificate_request(crypto.FILETYPE_PEM, csr)
>  +
>  +        if text is True:
>  +            return server_csr
>  +        else:
>  +            return csr 
The same note about certificate conversion here.

>  \ No newline at end of file
>  diff --git a/src/tests/intg/config.py.m4 b/src/tests/intg/config.py.m4
>  index 563127c6ea895508308a4f94689cc4e26ca4cbde..141813fa7a7a5bc9c06e48dfc938885f4eb1d539
> 100644
>  --- a/src/tests/intg/config.py.m4
>  +++ b/src/tests/intg/config.py.m4
>  @@ -11,3 +11,4 @@ PID_PATH            = "pidpath"
>    PIDFILE_PATH        = PID_PATH + "/sssd.pid"
>    LOG_PATH            = "logpath"
>    MCACHE_PATH         = "mcpath"
>  +SSL_PATH            = "/tmp/ssl" 
config.py.m4 is for variables originating from build configuration.  As
SSL_PATH is just a constant it shouldn't really be here. As such the proper
place for it could be ca.py.

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

     SSL_PATH            = SYSCONFDIR + "/ssl"


>  diff --git a/src/tests/intg/ds_openldap.py
> b/src/tests/intg/ds_openldap.py
>  index fb230a081b58bd4e135585daa5a6ddf8f494861c..dbbbfea660d3c4e0f7c7546663cc8560522ffa37
> 100644
>  --- a/src/tests/intg/ds_openldap.py
>  +++ b/src/tests/intg/ds_openldap.py
>  @@ -27,8 +27,10 @@ import errno
>    import signal
>    import shutil
>    import sys
>  +import socket
>    from util import *
>    from ds import DS
>  +from ca import CA
>
>
>    def hash_password(password):
>  @@ -39,7 +41,7 @@ def hash_password(password):
>        return "{SSHA}" + base64.standard_b64encode(hash.digest() + salt)
>
>
>  -class DSOpenLDAP(DS):
>  +class DSOpenLDAP(DS, CA): 
I don't think a DS should be derived from a CA. CA is a separate entity.

>        """OpenLDAP directory server
> instance."""
>
>        def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
>  @@ -60,6 +62,9 @@ class DSOpenLDAP(DS):
>            self.conf_dir = self.dir + "/etc/ldap"
>            self.conf_slapd_d_dir = self.conf_dir + "/slapd.d"
>            self.data_dir = self.dir + "/var/lib/ldap"
>  +        self.ca_inst = CA() 
I don't think that's how inheritance is supposed to be used. Besides, it's
better to let the user create and supply a CA through an (optional) argument
instead. Then the user can choose whether to use encryption or not.

>  +
>  +
>
>        def _setup_config(self):
>            """Setup the instance initial configuration."""
>  @@ -73,6 +78,13 @@ class DSOpenLDAP(DS):
>            uid = os.geteuid()
>            gid = os.getegid()
>
>  +        self.ca_inst.setup() 
If we won't be creating the CA instance ourselves, then we shouldn't be
calling "setup", the user should call it instead, so e.g. a single CA can be
used by several LDAP servers.

>  +        self.ca_inst.get_cert(self.ca_inst.request_cert())
>  +        fqdn = socket.gethostname()
>  +        cert = os.path.join(self.ca_inst.cert_dir, fqdn + '.crt')
>  +        ca_cert = os.path.join(self.ca_inst.cert_dir, fqdn + '-ca.crt')
>  +        key = os.path.join(self.ca_inst.key_dir, fqdn + '.key') 
Those three should really be variables in CA, the user shouldn't have to know
how filenames inside CA are formed. Incapsulation!

>  +
>            #
>            # Add configuration
>            #
>  @@ -82,6 +94,9 @@ class DSOpenLDAP(DS):
>                cn: config
>                olcPidFile: {self.pid_path}
>                olcArgsFile: {args_file}
>  +            olcTLSCertificateKeyFile: {key}
>  +            olcTLSCertificateFile: {cert}
>  +            olcTLSCACertificatePath: {ca_cert}
>                # Read slapd.conf(5) for possible values
>                olcLogLevel: none
>
>  @@ -282,3 +297,5 @@ class DSOpenLDAP(DS):
>
>            for path in (self.conf_slapd_d_dir, self.run_dir, self.data_dir):
>                shutil.rmtree(path, True)
>  +
>  +        self.ca_inst.teardown() 
Similarly to "setup" note above.

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

Nick

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

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(a)redhat.com&gt; 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_a...
> > dpkg
> > -ihttp://
security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+deb8u4_...
>  I'm not sure if libnss-wrapper is in Debian Testing, but we certainly
>  shouldn't be downloading packages from a URL, especially non-debian
>  packages,
>  but use the repos configured on the machine.
>
> > Below are test runs against Debian and Fedora 
I understand that completely, but I do feel that we need to find a way that
executing bash contrib/ci/run to setup CI should run successfully without
any other additional work. I'm not familiar with Debian but I was not able
to find these packages in the base repositories. Also do we have a
bashrc_sssd equivalent for Debian?

> > root@sssd2:~/sssd/x86_64# cat /etc/debian_version
> > 8.3
>
>
>  This is Debian Stable and we don't support that - we support Debian 
Testing.
>  Ping me if you need help installing that. 
I'm testing and using the cloud-init openstack images, which is easy to
spin up, but why do we only support the testing release?
> > diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py
> > new file mode 100644
> > index
> >
0000000000000000000000000000000000000000..c7adb7a2f0c2ea7f342c9df1d6ad41dcb2fcc28e
> > --- /dev/null
> > +++ b/src/tests/intg/ca.py
> > @@ -0,0 +1,163 @@
> > +#
> > +# SSSD LOCAL domain tests
> > +#
> > +# Copyright (c) 2016 Red Hat, Inc.
> > +# Author: Dan Lavu&lt;dan(a)redhat.com&gt;
> > +#
> > +# This is free software; you can redistribute it and/or modify it
> > +# under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; version 2 only
> > +#
> > +# This program is distributed in the hope that it will be useful, but
> > +# WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +# General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see<http://www.gnu.org/licenses/>;.
> > +#
> > +
> > +from OpenSSL import crypto
> > +from os.path import exists, join
> > +
> > +import socket
> > +import os
> > +import shutil
> > +import config
> > +
> > +
> > +class CA():
>
>
>  We're not basing CA on anything so there is no real need for parens and
>  they're better be removed to avoid confusion.
>
> > +    """CA Class"""
>
>
>  This basically says what the line before it says, perhaps an expanded
>  explanation of the class function is in order.
>
> > +
> > +    def __init__(self, subject=None, country='US', state='NC',
> > +                 city='Raleigh', organization='Red Hat Inc.',
> > unit='SSSD', config_dir=config.SSL_PATH):
> > +        if subject is None:
> > +            self.subject = socket.gethostname()
> > +        self.country = country
> > +        self.state = state
> > +        self.city = city
> > +        self.organization = organization
> > +        self.unit = unit
> > +        self.hostname = socket.gethostname()
> > +        if config.SSL_PATH is not '/tmp':
> > +            self.config_dir = config_dir
> > +        else:
> > +            self.config_dir = config_dir + '/ssl'
>
>
>  This kind of logic can confuse the class user, and make it impossible to
>  actually put files into /tmp if necessary.
>
>  I would replace this with just:
>
>      self.config_dir = config_dir 
OK, I made a mistake with the SSL_PATH variable during my tests, and
entering the wrong value essentially performed a rm -rf / so I was trying
to avoid/protect against that, but with your comments about how that
variable should be assigned, this will not be a problem.

>  under /tmp or other world-writable directories, as the possibility of
> name
>  clashes is high there. I presume the "tempfile" module should be used
>  instead.
>
> > +        self.csr_dir = self.config_dir + '/newcerts'
> > +        self.key_dir = self.config_dir + '/private'
> > +        self.cert_dir = self.config_dir + '/certs'
> > +
> > +        self.index = int(1000)
> > +
> > +
> > +    def setup(self):
> > +        """Setup CA using OpenSSL"""
> > +        cacert = self.hostname + '-ca.crt'
> > +        cakey = self.hostname + '-ca.key'
> > +
> > +        if not exists(join(self.cert_dir, cacert)) or not
> > exists(join(self.key_dir, cakey)):
>
>
>  It's better to use the fully-qualified name of the "join" function here so
>  it's obvious this is not just some generic "join". I.e. please write
>  "os.path.join" instead. Same goes for "exists".
>
>  To prevent further accidental uses like this don't import specific 
functions
>  above, but just the module.
>
>  Also, please put the generated paths into variables and use them instead,
>  here
>  and further. This will help against typos.
>
> > +            key = crypto.PKey()
> > +            key.generate_key(crypto.TYPE_RSA, 2048)
> > +
> > +            ca = crypto.X509()
>
>
>  crypto.X509() doesn't create a CA, but a certificate. Therefore the 
variable
>  storing the result should be named accordingly (e.g.
> "cert", or "ca_cert",
>  not
>  "ca"). Plus having a "ca" variable inside a "CA" class is
> confusing.
>
> > +            ca.get_subject().C = self.country
> > +            ca.get_subject().ST = self.state
> > +            ca.get_subject().L = self.city
> > +            ca.get_subject().O = self.organization
> > +            ca.get_subject().OU = self.unit
> > +            ca.get_subject().CN = self.subject
> > +            ca.set_serial_number(self.index)
> > +            ca.gmtime_adj_notBefore(0)
> > +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> > +            ca.set_issuer(ca.get_subject())
> > +            ca.set_pubkey(key)
> > +            ca.sign(key, 'sha1')
> > +
> > +            if not os.path.exists(self.csr_dir):
> > +                os.makedirs(self.csr_dir)
> > +            if not os.path.exists(self.key_dir):
> > +                os.makedirs(self.key_dir)
> > +            if not os.path.exists(self.cert_dir):
> > +                os.makedirs(self.cert_dir)
> > +
> > +            open(os.path.join(self.cert_dir, cacert), 'wt').write \
> > +                (crypto.dump_certificate(crypto.FILETYPE_PEM, ca))
> > +            open(os.path.join(self.key_dir, cakey), 'wt').write \
> > +                (crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
>
>
>  Can we use "with" statement here, so that file is closed ASAP, instead of
>  waiting for GC?
>
> > +
> > +
> > +    def teardown(self):
> > +        """Teardown CA certificate files"""
> > +        if os.path.exists(self.config_dir) and not '/tmp':
>
>
>  Hmm, this condition will always be false and the directory would never be
>  removed. I assume this is a piece of code trying to handle the special 
case
>  above and if that's to go then this one can simply be removed as
> well.
>
> > +            shutil.rmtree(self.config_dir)
> > +
> > +
> > +    def get_ca(self):
> > +        """Returns CA certificate in ASCII PEM
> encoding"""
> > +        ca = crypto.load_certificate(crypto.FILETYPE_PEM, open\
> > +                    (os.path.join(self.cert_dir, socket.gethostname() +
> > '-ca.crt'), 'rt').read())
> > +        ca_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, ca)
> > +
> > +        return ca_crt
>
>
>  This function name is confusing for the same reason as the "ca" variable
>  above. Plus it would be better not to return the certificate as text, 
unless
>  it's the main use.
>
> > +    def get_cert(self, csr, text=False):
> > +        """Retrieves certificate
> > +        required: csr -csr_type
> > +        optional: text
> > +            False - will return a pem object type (Default)
> > +            True - will return the certificate as ASCII
> > +        """
> > +        cacert = self.hostname + '-ca.crt'
> > +        cakey = self.hostname + '-ca.key'
>
>
>  Instead of generating these again, perhaps it is better to put them into
>  variables at initialization time? 
Are they being generated again? or are you saying that because I'm creating
a new variable? The CA certs are just being read from the files that are on
the system, so the first run will be a certificate for itself but can scale
to multihost tests down the road.

> > +        ca_crt = crypto.load_certificate(crypto.FILETYPE_PEM,
> > open(os.path.join(self.cert_dir, cacert), 'rt').read())
> > +        ca_key = crypto.load_privatekey(crypto.FILETYPE_PEM,
> > open(os.path.join(self.key_dir, cakey), 'rt').read())
>
>
>  Similarly with these paths. Besides, why not use the "get_ca" function
here
>  (after renaming) and perhaps a similar one to retrieve the key?
I think I did this because it's a crypto object, but yes, it can certainly
be simplified, thanks.
> > +        cert = crypto.X509()
> > +        cert.set_subject(csr.get_subject())
> > +        cert.set_serial_number(self.index+1)
> > +        cert.gmtime_adj_notBefore(0)
> > +        cert.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> > +        cert.set_issuer(ca_crt.get_subject())
> > +        cert.set_pubkey(csr.get_pubkey())
> > +        cert.sign(ca_key, 'sha1')
> > +
> > +        open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'),
> > 'wt')\
> > +            .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
> > +        server_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, cert)
> > +
> > +        if text is True:
> > +            return server_crt
> > +        else:
> > +            return cert
>
>
>  It is better to keep a function doing only one thing, and i.e. not 
implement
>  optional certificate conversion in this one. After all the caller can
> do 
it
>  themselves easily. So we can remove the "text" argument and
> not do this
>  conversion and branching.
>
> > +
> > +
> > +    def request_cert(self, fqdn=None, text=False):
> > +        """Generates CSR
> > +        optional: fqdn, socket.gethostname() (Default)
> > +        optional: text
> > +            False - will return a pem object type (Default)
> > +            True - will return the certificate as ASCII
> > +        """
> > +        if fqdn is None:
> > +            fqdn = socket.getfqdn()
> > +
> > +        hostname = socket.gethostname()
> > +        key = crypto.PKey()
> > +        key.generate_key(crypto.TYPE_RSA, 2048)
> > +        csr = crypto.X509Req()
> > +        csr.get_subject().CN = fqdn
> > +        csr.set_pubkey(key)
> > +        csr.sign(key, 'sha1')
> > +
> > +        open(os.path.join(self.key_dir, hostname + '.key'), 'wt').\
> > +            write(crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
> > +        open(os.path.join(self.csr_dir, hostname + '.csr'), 'wt').\
> > +            write(crypto.dump_certificate_request(crypto.FILETYPE_PEM,
> > csr))
>
>
>  The same notes about certificate/key retrieval here.
>
> > +        server_csr = 
crypto.dump_certificate_request(crypto.FILETYPE_PEM,
> > csr)
> > +
> > +        if text is True:
> > +            return server_csr
> > +        else:
> > +            return csr
>
>
>  The same note about certificate conversion here.
>
> > \ No newline at end of file
> > diff --git a/src/tests/intg/config.py.m4 b/src/tests/intg/config.py.m4
> > index
> >
563127c6ea895508308a4f94689cc4e26ca4cbde..141813fa7a7a5bc9c06e48dfc938885f4eb1d539
> > 100644
> > --- a/src/tests/intg/config.py.m4
> > +++ b/src/tests/intg/config.py.m4
> > @@ -11,3 +11,4 @@ PID_PATH            = "pidpath"
> >   PIDFILE_PATH        = PID_PATH + "/sssd.pid"
> >   LOG_PATH            = "logpath"
> >   MCACHE_PATH         = "mcpath"
> > +SSL_PATH            = "/tmp/ssl"
>
>
>  config.py.m4 is for variables originating from build configuration.  As
>  SSL_PATH is just a constant it shouldn't really be here. As such the 
proper
>  place for it could be ca.py.
>
>  However, it is better to keep all the files tests create under the prefix
>  directory (to make cleanup easier and more reliable), in a place where
>  people
>  would expect them. I don't know where this specific configuration should 
be,
>  but if it's e.g. /etc/ssl normally, then you can write in
> config.py.m4:
>
>      SSL_PATH            = SYSCONFDIR + "/ssl"
>
>
> > diff --git a/src/tests/intg/ds_openldap.py 
b/src/tests/intg/ds_openldap.py
> > index
> >
fb230a081b58bd4e135585daa5a6ddf8f494861c..dbbbfea660d3c4e0f7c7546663cc8560522ffa37
> > 100644
> > --- a/src/tests/intg/ds_openldap.py
> > +++ b/src/tests/intg/ds_openldap.py
> > @@ -27,8 +27,10 @@ import errno
> >   import signal
> >   import shutil
> >   import sys
> > +import socket
> >   from util import *
> >   from ds import DS
> > +from ca import CA
> >
> >
> >   def hash_password(password):
> > @@ -39,7 +41,7 @@ def hash_password(password):
> >       return "{SSHA}" + base64.standard_b64encode(hash.digest() + salt)
> >
> >
> > -class DSOpenLDAP(DS):
> > +class DSOpenLDAP(DS, CA):
>
>
>  I don't think a DS should be derived from a CA. CA is a separate entity.
>
> >       """OpenLDAP directory server instance."""
> >
> >       def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
> > @@ -60,6 +62,9 @@ class DSOpenLDAP(DS):
> >           self.conf_dir = self.dir + "/etc/ldap"
> >           self.conf_slapd_d_dir = self.conf_dir + "/slapd.d"
> >           self.data_dir = self.dir + "/var/lib/ldap"
> > +        self.ca_inst = CA()
>
>
>  I don't think that's how inheritance is supposed to be used. Besides, it's
>  better to let the user create and supply a CA through an (optional) 
argument
>  instead. Then the user can choose whether to use encryption or not.
>
> > +
> > +
> >
> >       def _setup_config(self):
> >           """Setup the instance initial
> configuration."""
> > @@ -73,6 +78,13 @@ class DSOpenLDAP(DS):
> >           uid = os.geteuid()
> >           gid = os.getegid()
> >
> > +        self.ca_inst.setup()
>
>
>  If we won't be creating the CA instance ourselves, then we shouldn't be
>  calling "setup", the user should call it instead, so e.g. a single CA can
be
>  used by several LDAP servers.
>
> > +        self.ca_inst.get_cert(self.ca_inst.request_cert())
> > +        fqdn = socket.gethostname()
> > +        cert = os.path.join(self.ca_inst.cert_dir, fqdn + '.crt')
> > +        ca_cert = os.path.join(self.ca_inst.cert_dir, fqdn + '-ca.crt')
> > +        key = os.path.join(self.ca_inst.key_dir, fqdn + '.key')
>
>
>  Those three should really be variables in CA, the user shouldn't have to
>  know
>  how filenames inside CA are formed. Incapsulation!
>
> > +
> >           #
> >           # Add configuration
> >           #
> > @@ -82,6 +94,9 @@ class DSOpenLDAP(DS):
> >               cn: config
> >               olcPidFile: {self.pid_path}
> >               olcArgsFile: {args_file}
> > +            olcTLSCertificateKeyFile: {key}
> > +            olcTLSCertificateFile: {cert}
> > +            olcTLSCACertificatePath: {ca_cert}
> >               # Read slapd.conf(5) for possible values
> >               olcLogLevel: none
> >
> > @@ -282,3 +297,5 @@ class DSOpenLDAP(DS):
> >
> >           for path in (self.conf_slapd_d_dir, self.run_dir,
> > self.data_dir):
> >               shutil.rmtree(path, True)
> > +
> > +        self.ca_inst.teardown()
>
>
>  Similarly to "setup" note above.
>
>  Aside from everything else above, please don't forget to run pylint on the
>  code before submitting!
>
>  Nick
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  
https://lists.fedorahosted.org/admin/lists/[e-mail]

[PATCH]: test ldap provider with TLS or SSL

[Lukas Slebodnik]

On (11/03/16 17:20), Dan Lavu wrote:
> WOAH, that's a lot of comments! ;) Thanks for taking the time, I
> have some
> questions inline and I'll start working on improving the code.
>
> On Fri, Mar 11, 2016 at 1:16 PM, Nikolai Kondrashov <
> Nikolai.Kondrashov(a)redhat.com&gt; 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_a...
> >> dpkg
> >> -ihttp://
> security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+deb8u4_...
linux-libc-dev was installed as a dependency of other packages in my case.

> >
> >
> > I'm not sure if libnss-wrapper is in Debian Testing, but we certainly
> > shouldn't be downloading packages from a URL, especially non-debian
> > packages,
> > but use the repos configured on the machine.
> >
> >> Below are test runs against Debian and Fedora
>
> I understand that completely, but I do feel that we need to find a way that
> executing bash contrib/ci/run to setup CI should run successfully without
> any other additional work. 
All necessary steps are described in contrib/ci/README.md
(at least should be)

I was able to run contrib/ci/run on minimal debian:testing
without any problem. I had to have installed packages "git, sudo, lsb-release"
and configured sudoers as it is described in contrib/ci/README.md

> I'm not familiar with Debian but I was not able
> to find these packages in the base repositories. 
They were not in base because
nss-wrapper was added later to debian.
And it isn't simple to add new packages to debian:stable.
That's the reason why we want to support only debian testing.
It's much simpler to get new packages there.

> Also do we have a
> bashrc_sssd equivalent for Debian?
>  
No but contrib/ci/run should do all stuff.

> >
> >
> >> root@sssd2:~/sssd/x86_64# cat /etc/debian_version
> >> 8.3
> >
> >
> > This is Debian Stable and we don't support that - we support Debian
> Testing.
> > Ping me if you need help installing that.
>
> I'm testing and using the cloud-init openstack images, which is easy to
> spin up, but why do we only support the testing release? 
I explain it few paragraps
above.

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

> >
> >> diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py
> >> new file mode 100644
> >> index
> >>
> 0000000000000000000000000000000000000000..c7adb7a2f0c2ea7f342c9df1d6ad41dcb2fcc28e
> >> --- /dev/null
> >> +++ b/src/tests/intg/ca.py 
You need to add this file to Makefile
otherwise make distcheck will fail in contrib/ci script :-)

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 7394997..0af59b8 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -1,6 +1,7 @@
 dist_noinst_DATA = \
     config.py.m4 \
     sssd_id.py \
+    ca.py \
     ds.py \
     ds_openldap.py \
     ent.py \

> >> @@ -0,0 +1,163 @@
> >> +#
> >> +# SSSD LOCAL domain tests
> >> +#
> >> +# Copyright (c) 2016 Red Hat, Inc.
> >> +# Author: Dan Lavu&lt;dan(a)redhat.com&gt;
> >> +#
> >> +# This is free software; you can redistribute it and/or modify it
> >> +# under the terms of the GNU General Public License as published by
> >> +# the Free Software Foundation; version 2 only
> >> +#
> >> +# This program is distributed in the hope that it will be useful, but
> >> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> +# General Public License for more details.
> >> +#
> >> +# You should have received a copy of the GNU General Public License
> >> +# along with this program.  If not, see<http://www.gnu.org/licenses/>;.
> >> +#
> >> +
> >> +from OpenSSL import crypto
> >> +from os.path import exists, join
> >> +
> >> +import socket
> >> +import os
> >> +import shutil
> >> +import config
> >> +
> >> +
> >> +class CA():
> >
> >
> > We're not basing CA on anything so there is no real need for parens and
> > they're better be removed to avoid confusion.
> > 
Actually, we should extend object to be more python3 compatible.
e.g.

class CA(object):


> >> +            ca.get_subject().C = self.country
> >> +            ca.get_subject().ST = self.state
> >> +            ca.get_subject().L = self.city
> >> +            ca.get_subject().O = self.organization
> >> +            ca.get_subject().OU = self.unit
> >> +            ca.get_subject().CN = self.subject
> >> +            ca.set_serial_number(self.index)
> >> +            ca.gmtime_adj_notBefore(0)
> >> +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> >> +            ca.set_issuer(ca.get_subject())
> >> +            ca.set_pubkey(key)
> >> +            ca.sign(key, 'sha1')
> >> +
> >> +            if not os.path.exists(self.csr_dir):
> >> +                os.makedirs(self.csr_dir)
> >> +            if not os.path.exists(self.key_dir):
> >> +                os.makedirs(self.key_dir)
> >> +            if not os.path.exists(self.cert_dir):
> >> +                os.makedirs(self.cert_dir)
> >> +
> >> +            open(os.path.join(self.cert_dir, cacert), 'wt').write \
> >> +                (crypto.dump_certificate(crypto.FILETYPE_PEM, ca))
> >> +            open(os.path.join(self.key_dir, cakey), 'wt').write \
> >> +                (crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
> >
> >
> > Can we use "with" statement here, so that file is closed ASAP, instead of
> > waiting for GC?
> > 
+1 for "with" and extra variable for path. It's much simpler to
read
the code.
@see
-        open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'),
'wt')\
-            .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
+        crt_path = os.path.join(self.cert_dir, socket.gethostname() + '.crt')
+        with open(crt_path, "wt") as f:
+            f.write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))


> > Aside from everything else above, please don't forget to run
> pylint on the
> > code before submitting!
> > 
pylint has some false positives due to pytest but we do not have any
pep8 issues in integration tests.

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

LS

[PATCH]: test ldap provider with TLS or SSL

[Nikolai Kondrashov]

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(a)redhat.com <mailto:[e-mail]>> 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_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. 
Wow! Good :)

>   >> +    def get_cert(self, csr, text=False):
>   >> +        """Retrieves certificate
>   >> +        required: csr -csr_type
>   >> +        optional: text
>   >> +            False - will return a pem object type (Default)
>   >> +            True - will return the certificate as ASCII
>   >> +        """
>   >> +        cacert = self.hostname + '-ca.crt'
>   >> +        cakey = self.hostname + '-ca.key'
>   >
>   >
>   > Instead of generating these again, perhaps it is better to put them into
>   > variables at initialization time?
>
>  Are they being generated again? or are you saying that because I'm creating
>  a new variable? The CA certs are just being read from the files that are on
>  the system, so the first run will be a certificate for itself but can scale
>  to multihost tests down the road. 
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

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

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(a)redhat.com&gt; 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 Lavu&lt;dan(a)redhat.com&gt;
> > +#
> > +# This is free software; you can redistribute it and/or modify it
> > +# under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; version 2 only
> > +#
> > +# This program is distributed in the hope that it will be useful, but
> > +# WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +# General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see<http://www.gnu.org/licenses/>;.
> > +#
> > +
> > +from OpenSSL import crypto
> > +from os.path import exists, join
> > +
> > +import socket
> > +import os
> > +import shutil
> > +import config
> > +
> > +
> > +class CA():
> >
>
>  We're not basing CA on anything so there is no real need for parens and
>  they're better be removed to avoid confusion.
>
>  +    """CA Class"""
> >
>
>  This basically says what the line before it says, perhaps an expanded
>  explanation of the class function is in order.
>
>  +
> > +    def __init__(self, subject=None, country='US', state='NC',
> > +                 city='Raleigh', organization='Red Hat Inc.',
> > unit='SSSD', config_dir=config.SSL_PATH):
> > +        if subject is None:
> > +            self.subject = socket.gethostname()
> > +        self.country = country
> > +        self.state = state
> > +        self.city = city
> > +        self.organization = organization
> > +        self.unit = unit
> > +        self.hostname = socket.gethostname()
> > +        if config.SSL_PATH is not '/tmp':
> > +            self.config_dir = config_dir
> > +        else:
> > +            self.config_dir = config_dir + '/ssl'
> >
>
>  This kind of logic can confuse the class user, and make it impossible to
>  actually put files into /tmp if necessary.
>
>  I would replace this with just:
>
>      self.config_dir = config_dir
>
>  As a side note, it's better to not create a fixed-name files or directories
>  under /tmp or other world-writable directories, as the possibility of name
>  clashes is high there. I presume the "tempfile" module should be used
>  instead.
>
>  +        self.csr_dir = self.config_dir + '/newcerts'
> > +        self.key_dir = self.config_dir + '/private'
> > +        self.cert_dir = self.config_dir + '/certs'
> > +
> > +        self.index = int(1000)
> > +
> > +
> > +    def setup(self):
> > +        """Setup CA using OpenSSL"""
> > +        cacert = self.hostname + '-ca.crt'
> > +        cakey = self.hostname + '-ca.key'
> > +
> > +        if not exists(join(self.cert_dir, cacert)) or not
> > exists(join(self.key_dir, cakey)):
> >
>
>  It's better to use the fully-qualified name of the "join" function here so
>  it's obvious this is not just some generic "join". I.e. please write
>  "os.path.join" instead. Same goes for "exists".
>
>  To prevent further accidental uses like this don't import specific
>  functions
>  above, but just the module.
>
>  Also, please put the generated paths into variables and use them instead,
>  here
>  and further. This will help against typos.
>
>  +            key = crypto.PKey()
> > +            key.generate_key(crypto.TYPE_RSA, 2048)
> > +
> > +            ca = crypto.X509()
> >
>
>  crypto.X509() doesn't create a CA, but a certificate. Therefore the
>  variable
>  storing the result should be named accordingly (e.g. "cert", or
> "ca_cert",
>  not
>  "ca"). Plus having a "ca" variable inside a "CA" class is
> confusing.
>
>  +            ca.get_subject().C = self.country
> > +            ca.get_subject().ST = self.state
> > +            ca.get_subject().L = self.city
> > +            ca.get_subject().O = self.organization
> > +            ca.get_subject().OU = self.unit
> > +            ca.get_subject().CN = self.subject
> > +            ca.set_serial_number(self.index)
> > +            ca.gmtime_adj_notBefore(0)
> > +            ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> > +            ca.set_issuer(ca.get_subject())
> > +            ca.set_pubkey(key)
> > +            ca.sign(key, 'sha1')
> > +
> > +            if not os.path.exists(self.csr_dir):
> > +                os.makedirs(self.csr_dir)
> > +            if not os.path.exists(self.key_dir):
> > +                os.makedirs(self.key_dir)
> > +            if not os.path.exists(self.cert_dir):
> > +                os.makedirs(self.cert_dir)
> > +
> > +            open(os.path.join(self.cert_dir, cacert), 'wt').write \
> > +                (crypto.dump_certificate(crypto.FILETYPE_PEM, ca))
> > +            open(os.path.join(self.key_dir, cakey), 'wt').write \
> > +                (crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
> >
>
>  Can we use "with" statement here, so that file is closed ASAP, instead of
>  waiting for GC?
>
>  +
> > +
> > +    def teardown(self):
> > +        """Teardown CA certificate files"""
> > +        if os.path.exists(self.config_dir) and not '/tmp':
> >
>
>  Hmm, this condition will always be false and the directory would never be
>  removed. I assume this is a piece of code trying to handle the special case
>  above and if that's to go then this one can simply be removed as well.
>
>  +            shutil.rmtree(self.config_dir)
> > +
> > +
> > +    def get_ca(self):
> > +        """Returns CA certificate in ASCII PEM
> encoding"""
> > +        ca = crypto.load_certificate(crypto.FILETYPE_PEM, open\
> > +                    (os.path.join(self.cert_dir, socket.gethostname() +
> > '-ca.crt'), 'rt').read())
> > +        ca_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, ca)
> > +
> > +        return ca_crt
> >
>
>  This function name is confusing for the same reason as the "ca" variable
>  above. Plus it would be better not to return the certificate as text,
>  unless
>  it's the main use.
>
>  +    def get_cert(self, csr, text=False):
> > +        """Retrieves certificate
> > +        required: csr -csr_type
> > +        optional: text
> > +            False - will return a pem object type (Default)
> > +            True - will return the certificate as ASCII
> > +        """
> > +        cacert = self.hostname + '-ca.crt'
> > +        cakey = self.hostname + '-ca.key'
> >
>
>  Instead of generating these again, perhaps it is better to put them into
>  variables at initialization time?
>
>  +        ca_crt = crypto.load_certificate(crypto.FILETYPE_PEM,
> > open(os.path.join(self.cert_dir, cacert), 'rt').read())
> > +        ca_key = crypto.load_privatekey(crypto.FILETYPE_PEM,
> > open(os.path.join(self.key_dir, cakey), 'rt').read())
> >
>
>  Similarly with these paths. Besides, why not use the "get_ca" function here
>  (after renaming) and perhaps a similar one to retrieve the key?
>
>  +        cert = crypto.X509()
> > +        cert.set_subject(csr.get_subject())
> > +        cert.set_serial_number(self.index+1)
> > +        cert.gmtime_adj_notBefore(0)
> > +        cert.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
> > +        cert.set_issuer(ca_crt.get_subject())
> > +        cert.set_pubkey(csr.get_pubkey())
> > +        cert.sign(ca_key, 'sha1')
> > +
> > +        open(os.path.join(self.cert_dir, socket.gethostname() +
> > '.crt'), 'wt')\
> > +            .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
> > +        server_crt = crypto.dump_certificate(crypto.FILETYPE_PEM, cert)
> > +
> > +        if text is True:
> > +            return server_crt
> > +        else:
> > +            return cert
> >
>
>  It is better to keep a function doing only one thing, and i.e. not
>  implement
>  optional certificate conversion in this one. After all the caller can do it
>  themselves easily. So we can remove the "text" argument and not do this
>  conversion and branching.
>
>  +
> > +
> > +    def request_cert(self, fqdn=None, text=False):
> > +        """Generates CSR
> > +        optional: fqdn, socket.gethostname() (Default)
> > +        optional: text
> > +            False - will return a pem object type (Default)
> > +            True - will return the certificate as ASCII
> > +        """
> > +        if fqdn is None:
> > +            fqdn = socket.getfqdn()
> > +
> > +        hostname = socket.gethostname()
> > +        key = crypto.PKey()
> > +        key.generate_key(crypto.TYPE_RSA, 2048)
> > +        csr = crypto.X509Req()
> > +        csr.get_subject().CN = fqdn
> > +        csr.set_pubkey(key)
> > +        csr.sign(key, 'sha1')
> > +
> > +        open(os.path.join(self.key_dir, hostname + '.key'), 'wt').\
> > +            write(crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
> > +        open(os.path.join(self.csr_dir, hostname + '.csr'), 'wt').\
> > +            write(crypto.dump_certificate_request(crypto.FILETYPE_PEM,
> > csr))
> >
>
>  The same notes about certificate/key retrieval here.
>
>  +        server_csr = crypto.dump_certificate_request(crypto.FILETYPE_PEM,
> > csr)
> > +
> > +        if text is True:
> > +            return server_csr
> > +        else:
> > +            return csr
> >
>
>  The same note about certificate conversion here.
>
>  \ No newline at end of file
> > diff --git a/src/tests/intg/config.py.m4 b/src/tests/intg/config.py.m4
> > index
> 563127c6ea895508308a4f94689cc4e26ca4cbde..141813fa7a7a5bc9c06e48dfc938885f4eb1d539
> > 100644
> > --- a/src/tests/intg/config.py.m4
> > +++ b/src/tests/intg/config.py.m4
> > @@ -11,3 +11,4 @@ PID_PATH            = "pidpath"
> >   PIDFILE_PATH        = PID_PATH + "/sssd.pid"
> >   LOG_PATH            = "logpath"
> >   MCACHE_PATH         = "mcpath"
> > +SSL_PATH            = "/tmp/ssl"
> >
>
>  config.py.m4 is for variables originating from build configuration.  As
>  SSL_PATH is just a constant it shouldn't really be here. As such the proper
>  place for it could be ca.py.
>
>  However, it is better to keep all the files tests create under the prefix
>  directory (to make cleanup easier and more reliable), in a place where
>  people
>  would expect them. I don't know where this specific configuration should
>  be,
>  but if it's e.g. /etc/ssl normally, then you can write in config.py.m4:
>
>      SSL_PATH            = SYSCONFDIR + "/ssl"
>
>
>  diff --git a/src/tests/intg/ds_openldap.py b/src/tests/intg/ds_openldap.py
> > index
> fb230a081b58bd4e135585daa5a6ddf8f494861c..dbbbfea660d3c4e0f7c7546663cc8560522ffa37
> > 100644
> > --- a/src/tests/intg/ds_openldap.py
> > +++ b/src/tests/intg/ds_openldap.py
> > @@ -27,8 +27,10 @@ import errno
> >   import signal
> >   import shutil
> >   import sys
> > +import socket
> >   from util import *
> >   from ds import DS
> > +from ca import CA
> >
> >
> >   def hash_password(password):
> > @@ -39,7 +41,7 @@ def hash_password(password):
> >       return "{SSHA}" + base64.standard_b64encode(hash.digest() + salt)
> >
> >
> > -class DSOpenLDAP(DS):
> > +class DSOpenLDAP(DS, CA):
> >
>
>  I don't think a DS should be derived from a CA. CA is a separate entity.
>
>        """OpenLDAP directory server instance."""
> >
> >       def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
> > @@ -60,6 +62,9 @@ class DSOpenLDAP(DS):
> >           self.conf_dir = self.dir + "/etc/ldap"
> >           self.conf_slapd_d_dir = self.conf_dir + "/slapd.d"
> >           self.data_dir = self.dir + "/var/lib/ldap"
> > +        self.ca_inst = CA()
> >
>
>  I don't think that's how inheritance is supposed to be used. Besides, it's
>  better to let the user create and supply a CA through an (optional)
>  argument
>  instead. Then the user can choose whether to use encryption or not.
>
>  +
> > +
> >
> >       def _setup_config(self):
> >           """Setup the instance initial
> configuration."""
> > @@ -73,6 +78,13 @@ class DSOpenLDAP(DS):
> >           uid = os.geteuid()
> >           gid = os.getegid()
> >
> > +        self.ca_inst.setup()
> >
>
>  If we won't be creating the CA instance ourselves, then we shouldn't be
>  calling "setup", the user should call it instead, so e.g. a single CA can
>  be
>  used by several LDAP servers.
>
>  +        self.ca_inst.get_cert(self.ca_inst.request_cert())
> > +        fqdn = socket.gethostname()
> > +        cert = os.path.join(self.ca_inst.cert_dir, fqdn + '.crt')
> > +        ca_cert = os.path.join(self.ca_inst.cert_dir, fqdn + '-ca.crt')
> > +        key = os.path.join(self.ca_inst.key_dir, fqdn + '.key')
> >
>
>  Those three should really be variables in CA, the user shouldn't have to
>  know
>  how filenames inside CA are formed. Incapsulation!
>
>  +
> >           #
> >           # Add configuration
> >           #
> > @@ -82,6 +94,9 @@ class DSOpenLDAP(DS):
> >               cn: config
> >               olcPidFile: {self.pid_path}
> >               olcArgsFile: {args_file}
> > +            olcTLSCertificateKeyFile: {key}
> > +            olcTLSCertificateFile: {cert}
> > +            olcTLSCACertificatePath: {ca_cert}
> >               # Read slapd.conf(5) for possible values
> >               olcLogLevel: none
> >
> > @@ -282,3 +297,5 @@ class DSOpenLDAP(DS):
> >
> >           for path in (self.conf_slapd_d_dir, self.run_dir,
> > self.data_dir):
> >               shutil.rmtree(path, True)
> > +
> > +        self.ca_inst.teardown()
> >
>
>  Similarly to "setup" note above.
>
>  Aside from everything else above, please don't forget to run pylint on the
>  code before submitting!
>
>  Nick
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.
>  fedorahosted.org
>

[PATCH]: test ldap provider with TLS or SSL

[Nikolai Kondrashov]

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

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

Nik,

Updated.

Dan

On Tue, Mar 7, 2017 at 4:11 AM, Nikolai Kondrashov <
Nikolai.Kondrashov(a)redhat.com&gt; 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(a)lists.fedorahosted.org
>  To unsubscribe send an email to sssd-devel-leave(a)lists.fedorahosted.org
>

[PATCH]: test ldap provider with TLS or SSL

[Nikolai Kondrashov]

On 03/07/2017 05:05 PM, Dan Lavu wrote:
>  Updated. 
Thanks! Please see my comments below.

On 03/07/2017 05:05 PM, Dan Lavu wrote:
>  diff --git src/tests/intg/ds.py src/tests/intg/ds.py
>  index 66cb887..13c0778 100644
>  --- src/tests/intg/ds.py
>  +++ src/tests/intg/ds.py
>  @@ -23,7 +23,7 @@ import ldap
>   class DS:
>       """Abstract directory server instance."""
>
>  -    def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
>  +    def __init__(self, dir, port, base_dn, admin_rdn, admin_pw, ssl_port=None,
> ca=None):
>           """
>               Initialize the instance.
>
>  @@ -37,11 +37,17 @@ class DS:
>           """
>           self.dir = dir
>           self.port = port
>  -        self.ldap_url = "ldap://localhost:" + str(self.port)
>           self.base_dn = base_dn
>           self.admin_rdn = admin_rdn
>           self.admin_dn = admin_rdn + "," + base_dn
>           self.admin_pw = admin_pw
>  +        self.ssl_port = ssl_port
>  +        self.ca = ca
>  +
>  +        if self.ssl_port and self.ca:
>  +            self.ldap_url = "ldaps://localhost:" + str(self.ssl_port)
>  +        else:
>  +            self.ldap_url = "ldap://localhost:" + str(self.port) 
Can you make it always have LDAP URL in ldap_url, and LDAPS URL in ldaps_url,
if we have ssl_port and ca?

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_port
>  +        self.ca = ca 
You don't need to set ssl_port and ca here, that's done by the parent's
constructor. Please remove.

>       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_url
>  +
>  +        modlist = [
>  +            (ldap.MOD_ADD, "olcTLSCertificateKeyFile", [key]),
>  +            (ldap.MOD_ADD, "olcTLSCertificateFile", [cert]),
>  +            (ldap.MOD_ADD, "olcTLSCACertificatePath", [ca_cert])
>  +        ]
>  +        ldap_conn = ldap.initialize(ldapi_url)
>  +        ldap_conn.simple_bind_s(self.admin_rdn + ",cn=config", self.admin_pw)
>  +        ldap_conn.modify_s("cn=config", modlist)
>  +        ldap_conn.unbind_s()
>  +
>  +        if subprocess.call(['pkill', 'slapd']) != 0:
>  +            raise execfile("Failed to stop slapd")
>  +
>  +        if subprocess.call(["slapd", "-F", self.conf_slapd_d_dir,
> "-h", url_list]) != 0:
>  +            raise Exception("Failed to start slapd") 
Does this have to be a separate function?

>  diff --git src/tests/intg/test_ldap.py src/tests/intg/test_ldap.py
>  index 848cb41..ea2e05f 100644
>  --- src/tests/intg/test_ldap.py
>  +++ src/tests/intg/test_ldap.py
>  @@ -16,7 +16,7 @@
>   # You should have received a copy of the GNU General Public License
>   # along with this program.  If not, see <http://www.gnu.org/licenses/>;.
>   #
>  -import os
>  +import os.path
>   import stat
>   import pwd
>   import grp
>  diff --git src/tests/intg/test_sssctl.py src/tests/intg/test_sssctl.py
>  index 0df5d0b..3c2ba02 100644
>  --- src/tests/intg/test_sssctl.py
>  +++ src/tests/intg/test_sssctl.py
>  @@ -28,6 +28,7 @@ import signal
>   import ds_openldap
>   import ldap_ent
>   import config
>  +import socket
>   from util import unindent
>   import sssd_netgroup
>
>  @@ -38,14 +39,14 @@ LDAP_BASE_DN = "dc=example,dc=com"
>   def ds_inst(request):
>       """LDAP server instance fixture"""
>       ds_inst = ds_openldap.DSOpenLDAP(
>  -        config.PREFIX, 10389, LDAP_BASE_DN,
>  -        "cn=admin", "Secret123")
>  +        config.PREFIX, 10389, LDAP_BASE_DN, "cn=admin",
> "Secret123")
>       try:
>           ds_inst.setup()
>       except:
>           ds_inst.teardown()
>           raise
>       request.addfinalizer(lambda: ds_inst.teardown())
>  +
>       return ds_inst 
All of the changes in the above piece seem to be unrelated to the main change.
Please don't bundle those into the main commit.

Could you please consider addressing the above comments, and then we can
proceed with the rest.

Also, could you please send commits formatted with git format-patch, instead
of just git diffs? This way you can also submit several separate commits at
once, and make it easier for me and others to apply and handle them.

Thanks!

Nick

[PATCH]: test ldap provider with TLS or SSL

[Dan Lavu]

Nik,

Thanks for the review, the responses are inline.

On Wed, Mar 8, 2017 at 1:59 AM, Nikolai Kondrashov <
Nikolai.Kondrashov(a)redhat.com&gt; wrote:

>  On 03/07/2017 05:05 PM, Dan Lavu wrote:
>
> > Updated.
> >
>
>  Thanks! Please see my comments below.
>
>  On 03/07/2017 05:05 PM, Dan Lavu wrote:
>
> > diff --git src/tests/intg/ds.py src/tests/intg/ds.py
> > index 66cb887..13c0778 100644
> > --- src/tests/intg/ds.py
> > +++ src/tests/intg/ds.py
> > @@ -23,7 +23,7 @@ import ldap
> >  class DS:
> >      """Abstract directory server instance."""
> >
> > -    def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
> > +    def __init__(self, dir, port, base_dn, admin_rdn, admin_pw,
> > ssl_port=None, ca=None):
> >          """
> >              Initialize the instance.
> >
> > @@ -37,11 +37,17 @@ class DS:
> >          """
> >          self.dir = dir
> >          self.port = port
> > -        self.ldap_url = "ldap://localhost:" + str(self.port)
> >          self.base_dn = base_dn
> >          self.admin_rdn = admin_rdn
> >          self.admin_dn = admin_rdn + "," + base_dn
> >          self.admin_pw = admin_pw
> > +        self.ssl_port = ssl_port
> > +        self.ca = ca
> > +
> > +        if self.ssl_port and self.ca:
> > +            self.ldap_url = "ldaps://localhost:" + str(self.ssl_port)
> > +        else:
> > +            self.ldap_url = "ldap://localhost:" + str(self.port)
> >
>
>  Can you make it always have LDAP URL in ldap_url, and LDAPS URL in
>  ldaps_url,
>  if we have ssl_port and ca?
>  
Done

>  I.e.
>
>      self.ldap_url = "ldap://localhost:" + str(self.port)
>      if self.ssl_port and self.ca:
>          self.ldaps_url = "ldaps://localhost:" + str(self.ssl_port)
>
>
> >      def setup(self):
> >          """Setup the instance"""
> > diff --git src/tests/intg/ds_openldap.py src/tests/intg/ds_openldap.py
> > index 6a074c9..a5b2152 100644
> > --- src/tests/intg/ds_openldap.py
> > +++ src/tests/intg/ds_openldap.py
> > @@ -25,7 +25,10 @@ import os
> >  import errno
> >  import signal
> >  import shutil
> > +import socket
> > +import urllib
> >  import sys
> > +import ca_openssl
> >  from util import *
> >  from ds import DS
> >
> > @@ -47,7 +50,7 @@ def hash_password(password):
> >  class DSOpenLDAP(DS):
> >      """OpenLDAP directory server instance."""
> >
> > -    def __init__(self, dir, port, base_dn, admin_rdn, admin_pw):
> > +    def __init__(self, dir, port, base_dn, admin_rdn, admin_pw,
> > ssl_port=None, ca=False):
> >
>
>  I believe it should be ca=None here as well.
>  
Done.

>           """
> >              Initialize the instance.
> >
> > @@ -59,12 +62,14 @@ class DSOpenLDAP(DS):
> >              admin_rdn   Administrator DN, relative to BASE_DN.
> >              admin_pw    Administrator password.
> >          """
> > -        DS.__init__(self, dir, port, base_dn, admin_rdn, admin_pw)
> > +        DS.__init__(self, dir, port, base_dn, admin_rdn, admin_pw, ca)
> >          self.run_dir = self.dir + "/var/run/ldap"
> >          self.pid_path = self.run_dir + "/slapd.pid"
> >          self.conf_dir = self.dir + "/etc/ldap"
> >          self.conf_slapd_d_dir = self.conf_dir + "/slapd.d"
> >          self.data_dir = self.dir + "/var/lib/ldap"
> > +        self.ssl_port = ssl_port
> > +        self.ca = ca
> >
>
>  You don't need to set ssl_port and ca here, that's done by the parent's
>  constructor. Please remove.
>  
Done.

> >      def _setup_config(self):
> >          """Setup the instance initial
> configuration."""
> > @@ -78,9 +83,6 @@ class DSOpenLDAP(DS):
> >          uid = os.geteuid()
> >          gid = os.getegid()
> >
> > -        #
> > -        # Add configuration
> > -        #
> >
>
>  Why does this need to be removed?
>  
Just seems strange to have everything commented using quotes and these are
hashes, added them back in.

>           config = unindent("""
> >              dn: cn=config
> >              objectClass: olcGlobal
> > @@ -223,11 +225,23 @@ class DSOpenLDAP(DS):
> >                  break
> >              except ldap.SERVER_DOWN:
> >                  pass
> > -            attempt = attempt + 1
> > +            attempt += 1
> >
>
>  Could you please make this a separate commit to keep changes to the point?
>  
Done.

>               if attempt > 30:
> >                  raise Exception("Failed to start slapd")
> >              time.sleep(1)
> >
> > +        if self.ca:
> > +            self.ca_hostname = socket.gethostname()
> > +            self.ca_inst = ca_openssl.CAOpenSSL(config.PREFIX,
> > self.ca_hostname,
> > +                                                "US",
> > +                                                "NC",
> > +                                                "Raleigh",
> > +                                                "Red Hat Inc.",
> > +                                                "SSSD")
> > +            self.ca_inst.setup()
> > +            self.ca_inst.gen_cert(self.ca_inst.gen_csr(self.ca_hostname),
> > self.ca_hostname)
> > +            self.enable_ssl(self.ca_inst.ca_cert_file,
> > self.ca_inst.ca_cert_file, self.ca_inst.ca_cert_key_file)
> >
>
>  By "passing CA to the constructor" I meant pass an actual CA instance to
>  the
>  constructor. I.e. CA should be created and set up outside the DS, in the
>  tests. Can you do that?
>  
Can you clarify or give me an example of what you want?


>           #
> >          # Relax requirement of member attribute presence in groupOfNames
> >          #
> > @@ -277,7 +291,7 @@ class DSOpenLDAP(DS):
> >                  pid_file.close()
> >              attempt = 0
> >              while os.path.isfile(self.pid_path):
> > -                attempt = attempt + 1
> > +                attempt += 1
> >
>
>  Same here, please make a separate commit.
>  
Done


>                   if attempt > 30:
> >                      raise Exception("Failed to stop slapd")
> >                  time.sleep(1)
> > @@ -287,3 +301,29 @@ class DSOpenLDAP(DS):
> >
> >          for path in (self.conf_slapd_d_dir, self.run_dir, self.data_dir):
> >              shutil.rmtree(path, True)
> > +
> > +        if self.ca:
> > +            self.ca_inst.teardown()
> >
>
>  If you pass the CA from the outside, you no longer will have to care about
>  tearing it down.
>
>  +    def enable_ssl(self, ca_cert, cert, key):
> > +        """Enable SSL in OpenLDAP"""
> > +
> > +        ldapi_socket = self.run_dir + "/ldapi"
> > +        ldapi_url = "ldapi://" + url_quote(ldapi_socket, "")
> > +        url_list = ldapi_url + " " + self.ldap_url
> > +
> > +        modlist = [
> > +            (ldap.MOD_ADD, "olcTLSCertificateKeyFile", [key]),
> > +            (ldap.MOD_ADD, "olcTLSCertificateFile", [cert]),
> > +            (ldap.MOD_ADD, "olcTLSCACertificatePath", [ca_cert])
> > +        ]
> > +        ldap_conn = ldap.initialize(ldapi_url)
> > +        ldap_conn.simple_bind_s(self.admin_rdn + ",cn=config",
> > self.admin_pw)
> > +        ldap_conn.modify_s("cn=config", modlist)
> > +        ldap_conn.unbind_s()
> > +
> > +        if subprocess.call(['pkill', 'slapd']) != 0:
> > +            raise execfile("Failed to stop slapd")
> > +
> > +        if subprocess.call(["slapd", "-F",
> self.conf_slapd_d_dir, "-h",
> > url_list]) != 0:
> > +            raise Exception("Failed to start slapd")
> >
>
>  Does this have to be a separate function?
>  
No, just thought functions should be simple and clear, I appended the code
after the 'if self.ca' state in the setup.

>  diff --git src/tests/intg/test_ldap.py src/tests/intg/test_ldap.py
> > index 848cb41..ea2e05f 100644
> > --- src/tests/intg/test_ldap.py
> > +++ src/tests/intg/test_ldap.py
> > @@ -16,7 +16,7 @@
> >  # You should have received a copy of the GNU General Public License
> >  # along with this program.  If not, see <http://www.gnu.org/licenses/>;.
> >  #
> > -import os
> > +import os.path
> >  import stat
> >  import pwd
> >  import grp
> > diff --git src/tests/intg/test_sssctl.py src/tests/intg/test_sssctl.py
> > index 0df5d0b..3c2ba02 100644
> > --- src/tests/intg/test_sssctl.py
> > +++ src/tests/intg/test_sssctl.py
> > @@ -28,6 +28,7 @@ import signal
> >  import ds_openldap
> >  import ldap_ent
> >  import config
> > +import socket
> >  from util import unindent
> >  import sssd_netgroup
> >
> > @@ -38,14 +39,14 @@ LDAP_BASE_DN = "dc=example,dc=com"
> >  def ds_inst(request):
> >      """LDAP server instance fixture"""
> >      ds_inst = ds_openldap.DSOpenLDAP(
> > -        config.PREFIX, 10389, LDAP_BASE_DN,
> > -        "cn=admin", "Secret123")
> > +        config.PREFIX, 10389, LDAP_BASE_DN, "cn=admin",
> "Secret123")
> >      try:
> >          ds_inst.setup()
> >      except:
> >          ds_inst.teardown()
> >          raise
> >      request.addfinalizer(lambda: ds_inst.teardown())
> > +
> >      return ds_inst
> >
>
>  All of the changes in the above piece seem to be unrelated to the main
>  change.
>  Please don't bundle those into the main commit.
>
>  Could you please consider addressing the above comments, and then we can
>  proceed with the rest.
>
>  Also, could you please send commits formatted with git format-patch,
>  instead
>  of just git diffs? This way you can also submit several separate commits at
>  once, and make it easier for me and others to apply and handle them.
>
>  Thanks!
>
>
>  Nick
>  _______________________________________________
>  sssd-devel mailing list -- sssd-devel(a)lists.fedorahosted.org
>  To unsubscribe send an email to sssd-devel-leave(a)lists.fedorahosted.org
>

[PATCH]: test ldap provider with TLS or SSL

[Nikolai Kondrashov]

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(a)redhat.com
> <mailto:[e-mail]>> 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