URL: https://github.com/freeipa/freeipa/pull/893
Author: martbab
 Title: #893: smard card advises fixes + general improvements
Action: opened

PR body:
"""
Add some missing operations to the client/server smart card advises and fix
issues. Also provide more transparent generators of Bash control flow branches
and loops.

https://pagure.io/freeipa/issue/7036
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/893/head:pr893
git checkout pr893
From d50a6278ab151e0facda48a64006a48507ec6e25 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Wed, 21 Jun 2017 18:28:50 +0200
Subject: [PATCH 01/11] smart-card advise: configure systemwide NSS DB also on
 master

Previously the Smart card signing CA cert was uploaded to systemwide NSS
DB only on the client, but it need to be added also to the server.
Modify the advise plugins to allow for common configuration steps to
occur in both cases.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/plugins/smart_card_auth.py | 59 +++++++++++++++++------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index 5859e35093..0ee4808d47 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -10,8 +10,39 @@
 register = Registry()
 
 
+class common_smart_card_auth_config(Advice):
+    """
+    Common steps required to properly configure both server and client for
+    smart card auth
+    """
+
+    systemwide_nssdb = paths.NSS_DB_DIR
+    smart_card_ca_cert_variable_name = "SC_CA_CERT"
+
+    def check_and_set_ca_cert_path(self):
+        ca_path_variable = self.smart_card_ca_cert_variable_name
+        self.log.command("{}=$1".format(ca_path_variable))
+        self.log.exit_on_predicate(
+            '[ -z "${}" ]'.format(ca_path_variable),
+            ['You need to provide the path to the PEM file containing CA '
+             'signing the Smart Cards']
+        )
+        self.log.exit_on_predicate(
+            '[ ! -f "${}" ]'.format(ca_path_variable),
+            ['Invalid CA certificate filename: ${}'.format(ca_path_variable),
+             'Please check that the path exists and is a valid file']
+        )
+
+    def upload_smartcard_ca_certificate_to_systemwide_db(self):
+        self.log.command(
+            'certutil -d {} -A -i ${} -n "Smart Card CA" -t CT,C,C'.format(
+                self.systemwide_nssdb, self.smart_card_ca_cert_variable_name
+            )
+        )
+
+
 @register()
-class config_server_for_smart_card_auth(Advice):
+class config_server_for_smart_card_auth(common_smart_card_auth_config):
     """
     Configures smart card authentication via Kerberos (PKINIT) and for WebUI
     """
@@ -28,6 +59,7 @@ class config_server_for_smart_card_auth(Advice):
 
     def get_info(self):
         self.log.exit_on_nonroot_euid()
+        self.check_and_set_ca_cert_path()
         self.check_ccache_not_empty()
         self.check_hostname_is_in_masters()
         self.resolve_ipaca_records()
@@ -37,6 +69,7 @@ def get_info(self):
         self.record_httpd_ocsp_status()
         self.check_and_enable_pkinit()
         self.enable_ok_to_auth_as_delegate_on_http_principal()
+        self.upload_smartcard_ca_certificate_to_systemwide_db()
 
     def check_ccache_not_empty(self):
         self.log.comment('Check whether the credential cache is not empty')
@@ -162,11 +195,10 @@ def enable_ok_to_auth_as_delegate_on_http_principal(self):
 
 
 @register()
-class config_client_for_smart_card_auth(Advice):
+class config_client_for_smart_card_auth(common_smart_card_auth_config):
     """
     Configures smart card authentication on FreeIPA client
     """
-    smart_card_ca_cert_variable_name = "SC_CA_CERT"
 
     description = ("Instructions for enabling Smart Card authentication on "
                    " a single FreeIPA client. Configures Smart Card daemon, "
@@ -190,20 +222,6 @@ def get_info(self):
         self.run_authconfig_to_configure_smart_card_auth()
         self.restart_sssd()
 
-    def check_and_set_ca_cert_path(self):
-        ca_path_variable = self.smart_card_ca_cert_variable_name
-        self.log.command("{}=$1".format(ca_path_variable))
-        self.log.exit_on_predicate(
-            '[ -z "${}" ]'.format(ca_path_variable),
-            ['You need to provide the path to the PEM file containing CA '
-             'signing the Smart Cards']
-        )
-        self.log.exit_on_predicate(
-            '[ ! -f "${}" ]'.format(ca_path_variable),
-            ['Invalid CA certificate filename: ${}'.format(ca_path_variable),
-             'Please check that the path exists and is a valid file']
-        )
-
     def check_and_remove_pam_pkcs11(self):
         self.log.command('rpm -qi pam_pkcs11 > /dev/null')
         self.log.commands_on_predicate(
@@ -247,13 +265,6 @@ def add_pkcs11_module_to_systemwide_db(self):
             ]
         )
 
-    def upload_smartcard_ca_certificate_to_systemwide_db(self):
-        self.log.command(
-            'certutil -d {} -A -i ${} -n "Smart Card CA" -t CT,C,C'.format(
-                self.systemwide_nssdb, self.smart_card_ca_cert_variable_name
-            )
-        )
-
     def run_authconfig_to_configure_smart_card_auth(self):
         self.log.exit_on_failed_command(
             'authconfig --enablesmartcard --smartcardmodule=sssd --updateall',

From 46dfe240a6f5926dc7a30c43b7536dbcdea46a03 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Wed, 21 Jun 2017 18:52:57 +0200
Subject: [PATCH 02/11] smart-card advises: add steps to store smart card
 signing CA cert

On master, upload the CA certificate to IPA LDAP and NSS databases. On
both master and client run ipa-certupdate to update client-side CA
certificate bundles used as PKINIT anchors.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/plugins/smart_card_auth.py | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index 0ee4808d47..2fdf1ffa98 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -40,6 +40,20 @@ def upload_smartcard_ca_certificate_to_systemwide_db(self):
             )
         )
 
+    def install_smart_card_signing_ca_cert(self):
+        self.log.exit_on_failed_command(
+            'ipa-cacert-manage install ${}'.format(
+                self.smart_card_ca_cert_variable_name
+            ),
+            ['Failed to install external CA certificate to IPA']
+        )
+
+    def update_ipa_ca_certificate_store(self):
+        self.log.exit_on_failed_command(
+            'ipa-certupdate',
+            ['Failed to update IPA CA certificate database']
+        )
+
 
 @register()
 class config_server_for_smart_card_auth(common_smart_card_auth_config):
@@ -70,6 +84,7 @@ def get_info(self):
         self.check_and_enable_pkinit()
         self.enable_ok_to_auth_as_delegate_on_http_principal()
         self.upload_smartcard_ca_certificate_to_systemwide_db()
+        self.update_ipa_ca_certificate_store()
 
     def check_ccache_not_empty(self):
         self.log.comment('Check whether the credential cache is not empty')
@@ -219,6 +234,8 @@ def get_info(self):
         self.start_enable_smartcard_daemon()
         self.add_pkcs11_module_to_systemwide_db()
         self.upload_smartcard_ca_certificate_to_systemwide_db()
+        self.install_smart_card_signing_ca_cert()
+        self.update_ipa_ca_certificate_store()
         self.run_authconfig_to_configure_smart_card_auth()
         self.restart_sssd()
 

From 628937399dc2e3460e67fdbf994ed3a854dce304 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 10:06:21 +0200
Subject: [PATCH 03/11] Allow to pass in multiple CA cert paths to the smart
 card advises

If the user has a series of CA certificates required to verify smart
card certs (e.g. intermediary CAs and root CA) it is convenient to allow
for passing them to the advise scripts as a series of PEM files.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/plugins/smart_card_auth.py | 70 +++++++++++++++++++----------
 1 file changed, 47 insertions(+), 23 deletions(-)

diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index 2fdf1ffa98..8f9ae58f0d 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -17,36 +17,61 @@ class common_smart_card_auth_config(Advice):
     """
 
     systemwide_nssdb = paths.NSS_DB_DIR
-    smart_card_ca_cert_variable_name = "SC_CA_CERT"
+    smart_card_ca_certs_variable_name = "SC_CA_CERTS"
+    single_ca_cert_variable_name = 'ca_cert'
 
-    def check_and_set_ca_cert_path(self):
-        ca_path_variable = self.smart_card_ca_cert_variable_name
-        self.log.command("{}=$1".format(ca_path_variable))
+    def check_and_set_ca_cert_paths(self):
+        ca_paths_variable = self.smart_card_ca_certs_variable_name
+        single_ca_path_variable = self.single_ca_cert_variable_name
+
+        self.log.command("{}=$1".format(ca_paths_variable))
         self.log.exit_on_predicate(
-            '[ -z "${}" ]'.format(ca_path_variable),
-            ['You need to provide the path to the PEM file containing CA '
-             'signing the Smart Cards']
+            '[ -z "${}" ]'.format(ca_paths_variable),
+            ['You need to provide one or more paths to the PEM files '
+             'containing CAs signing the Smart Cards']
         )
+        self.log.command(
+            "for {} in ${}".format(
+                single_ca_path_variable, ca_paths_variable))
+        self.log.command("do")
         self.log.exit_on_predicate(
-            '[ ! -f "${}" ]'.format(ca_path_variable),
-            ['Invalid CA certificate filename: ${}'.format(ca_path_variable),
-             'Please check that the path exists and is a valid file']
+            '[ ! -f "${}" ]'.format(single_ca_path_variable),
+            ['Invalid CA certificate filename: ${}'.format(
+                single_ca_path_variable),
+             'Please check that the path exists and is a valid file'],
+            indent_spaces=2
         )
+        self.log.command("done")
 
-    def upload_smartcard_ca_certificate_to_systemwide_db(self):
+    def upload_smartcard_ca_certificates_to_systemwide_db(self):
+        self.log.command(
+            "for {} in ${}".format(
+                self.single_ca_cert_variable_name,
+                self.smart_card_ca_certs_variable_name))
+        self.log.command("do")
         self.log.command(
-            'certutil -d {} -A -i ${} -n "Smart Card CA" -t CT,C,C'.format(
-                self.systemwide_nssdb, self.smart_card_ca_cert_variable_name
-            )
+            'certutil -d {} -A -i ${} -n "Smart Card CA $(uuidgen)" '
+            '-t CT,C,C'.format(
+                self.systemwide_nssdb, self.single_ca_cert_variable_name
+            ),
+            indent_spaces=2
         )
+        self.log.command("done")
 
-    def install_smart_card_signing_ca_cert(self):
+    def install_smart_card_signing_ca_certs(self):
+        self.log.command(
+            "for {} in ${}".format(
+                self.single_ca_cert_variable_name,
+                self.smart_card_ca_certs_variable_name))
+        self.log.command("do")
         self.log.exit_on_failed_command(
             'ipa-cacert-manage install ${}'.format(
-                self.smart_card_ca_cert_variable_name
+                self.single_ca_cert_variable_name
             ),
-            ['Failed to install external CA certificate to IPA']
+            ['Failed to install external CA certificate to IPA'],
+            indent_spaces=2
         )
+        self.log.command("done")
 
     def update_ipa_ca_certificate_store(self):
         self.log.exit_on_failed_command(
@@ -73,7 +98,7 @@ class config_server_for_smart_card_auth(common_smart_card_auth_config):
 
     def get_info(self):
         self.log.exit_on_nonroot_euid()
-        self.check_and_set_ca_cert_path()
+        self.check_and_set_ca_cert_paths()
         self.check_ccache_not_empty()
         self.check_hostname_is_in_masters()
         self.resolve_ipaca_records()
@@ -83,7 +108,8 @@ def get_info(self):
         self.record_httpd_ocsp_status()
         self.check_and_enable_pkinit()
         self.enable_ok_to_auth_as_delegate_on_http_principal()
-        self.upload_smartcard_ca_certificate_to_systemwide_db()
+        self.upload_smartcard_ca_certificates_to_systemwide_db()
+        self.install_smart_card_signing_ca_certs()
         self.update_ipa_ca_certificate_store()
 
     def check_ccache_not_empty(self):
@@ -224,17 +250,15 @@ class config_client_for_smart_card_auth(common_smart_card_auth_config):
     pkcs11_shared_lib = '/usr/lib64/opensc-pkcs11.so'
     smart_card_service_file = 'pcscd.service'
     smart_card_socket = 'pcscd.socket'
-    systemwide_nssdb = paths.NSS_DB_DIR
 
     def get_info(self):
         self.log.exit_on_nonroot_euid()
-        self.check_and_set_ca_cert_path()
+        self.check_and_set_ca_cert_paths()
         self.check_and_remove_pam_pkcs11()
         self.install_opensc_and_dconf_packages()
         self.start_enable_smartcard_daemon()
         self.add_pkcs11_module_to_systemwide_db()
-        self.upload_smartcard_ca_certificate_to_systemwide_db()
-        self.install_smart_card_signing_ca_cert()
+        self.upload_smartcard_ca_certificates_to_systemwide_db()
         self.update_ipa_ca_certificate_store()
         self.run_authconfig_to_configure_smart_card_auth()
         self.restart_sssd()

From b3390ddef7c4a5a8cd431eed9d50302cedf887a9 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 13:18:54 +0200
Subject: [PATCH 04/11] add a class that tracks the indentation in the
 generated advises

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/base.py | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/ipaserver/advise/base.py b/ipaserver/advise/base.py
index ba412b8724..639fd1807f 100644
--- a/ipaserver/advise/base.py
+++ b/ipaserver/advise/base.py
@@ -76,6 +76,55 @@
 """
 
 
+class _IndentationTracker(object):
+    """
+    A simple wrapper that tracks the indentation level of the generated bash
+    commands
+    """
+    def __init__(self, spaces_per_indent=0):
+        if spaces_per_indent <= 0:
+            raise ValueError(
+                "Indentation increments cannot be zero or negative")
+        self.spaces_per_indent = spaces_per_indent
+        self._indentation_stack = []
+        self._total_indentation_level = 0
+
+    @property
+    def indentation_string(self):
+        """
+        return a string containing number of spaces corresponding to
+        indentation level
+        """
+        return " " * self._total_indentation_level
+
+    def indent(self):
+        """
+        track a single indentation of the generated code
+        """
+        self._indentation_stack.append(self.spaces_per_indent)
+        self._recompute_indentation_level()
+
+    def _recompute_indentation_level(self):
+        """
+        Track total indentation level of the generated code
+        """
+        self._total_indentation_level = sum(self._indentation_stack)
+
+    def dedent(self):
+        """
+        track a single dedentation of the generated code
+        dedents that would result in zero or negative indentation level will be
+        ignored
+        """
+        try:
+            self._indentation_stack.pop()
+        except IndexError:
+            # can not dedent any further
+            pass
+
+        self._recompute_indentation_level()
+
+
 class _AdviceOutput(object):
 
     def __init__(self):

From 2c3ad0da9cf38da2bfdf0905f545b293fb523216 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 13:20:05 +0200
Subject: [PATCH 05/11] delegate the indentation handling in advises to
 dedicated class

Indentation levels are now handled transparently by a dedicated class
and should not pollute the statement printing logic.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/base.py                    | 106 +++++++++++++++++++---------
 ipaserver/advise/plugins/smart_card_auth.py |  45 ++++++------
 2 files changed, 93 insertions(+), 58 deletions(-)

diff --git a/ipaserver/advise/base.py b/ipaserver/advise/base.py
index 639fd1807f..c320b002c8 100644
--- a/ipaserver/advise/base.py
+++ b/ipaserver/advise/base.py
@@ -19,6 +19,7 @@
 
 from __future__ import print_function
 
+from contextlib import contextmanager
 import os
 from textwrap import wrap
 
@@ -75,6 +76,8 @@
 # ./script.sh
 """
 
+DEFAULT_INDENTATION_INCREMENT = 2
+
 
 class _IndentationTracker(object):
     """
@@ -131,39 +134,77 @@ def __init__(self):
         self.content = []
         self.prefix = '# '
         self.options = None
+        self._indentation_tracker = _IndentationTracker(
+            spaces_per_indent=DEFAULT_INDENTATION_INCREMENT)
+
+    def indent(self):
+        """
+        Indent the statements by one level
+        """
+        self._indentation_tracker.indent()
+
+    def dedent(self):
+        """
+        Dedent the statements by one level
+        """
+        self._indentation_tracker.dedent()
+
+    @contextmanager
+    def indented_block(self):
+        self.indent()
+        try:
+            yield
+        finally:
+            self.dedent()
 
     def comment(self, line, wrapped=True):
         if wrapped:
-            for wrapped_line in wrap(line, 70):
-                self.content.append(self.prefix + wrapped_line)
+            self.append_wrapped_and_indented_comment(line)
         else:
-            self.content.append(self.prefix + line)
+            self.append_comment(line)
+
+    def append_wrapped_and_indented_comment(self, line, character_limit=70):
+        """
+        append wrapped and indented comment to the output
+        """
+        for wrapped_indented_line in wrap(
+                self.indent_statement(line), character_limit):
+            self.append_comment(wrapped_indented_line)
+
+    def append_comment(self, line):
+        self.append_statement(self.prefix + line)
+
+    def append_statement(self, statement):
+        """
+        Append a line to the generated content indenting it by tracked number
+        of spaces
+        """
+        self.content.append(self.indent_statement(statement))
+
+    def indent_statement(self, statement):
+        return '{indent}{statement}'.format(
+            indent=self._indentation_tracker.indentation_string,
+            statement=statement)
 
     def debug(self, line):
         if self.options.verbose:
             self.comment('DEBUG: ' + line)
 
-    def command(self, line, indent_spaces=0):
-        self.content.append(
-            '{}{}'.format(self._format_indent(indent_spaces), line))
-
-    def _format_indent(self, num_spaces):
-        return ' ' * num_spaces
+    def command(self, line):
+        self.append_statement(line)
 
-    def echo_error(self, error_message, indent_spaces=0):
-        self.command(
-            self._format_error(error_message), indent_spaces=indent_spaces)
+    def echo_error(self, error_message):
+        self.command(self._format_error(error_message))
 
     def _format_error(self, error_message):
         return 'echo "{}" >&2'.format(error_message)
 
     def exit_on_failed_command(self, command_to_run,
-                               error_message_lines, indent_spaces=0):
-        self.command(command_to_run, indent_spaces=indent_spaces)
+                               error_message_lines):
+        self.command(command_to_run)
         self.exit_on_predicate(
             '[ "$?" -ne "0" ]',
-            error_message_lines,
-            indent_spaces=indent_spaces)
+            error_message_lines)
 
     def exit_on_nonroot_euid(self):
         self.exit_on_predicate(
@@ -171,8 +212,7 @@ def exit_on_nonroot_euid(self):
             ["This script has to be run as root user"]
         )
 
-    def exit_on_predicate(self, predicate, error_message_lines,
-                          indent_spaces=0):
+    def exit_on_predicate(self, predicate, error_message_lines):
         commands_to_run = [
             self._format_error(error_message_line)
             for error_message_line in error_message_lines]
@@ -180,30 +220,26 @@ def exit_on_predicate(self, predicate, error_message_lines,
         commands_to_run.append('exit 1')
         self.commands_on_predicate(
             predicate,
-            commands_to_run,
-            indent_spaces=indent_spaces)
+            commands_to_run)
 
     def commands_on_predicate(self, predicate, commands_to_run_when_true,
-                              commands_to_run_when_false=None,
-                              indent_spaces=0):
+                              commands_to_run_when_false=None):
         if_command = 'if {}'.format(predicate)
-        self.command(if_command, indent_spaces=indent_spaces)
-        self.command('then', indent_spaces=indent_spaces)
-
-        indented_block_spaces = indent_spaces + 2
+        self.command(if_command)
+        self.command('then')
 
-        for command_to_run_when_true in commands_to_run_when_true:
-            self.command(
-                command_to_run_when_true, indent_spaces=indented_block_spaces)
+        with self.indented_block():
+            for command_to_run_when_true in commands_to_run_when_true:
+                self.command(
+                    command_to_run_when_true)
 
         if commands_to_run_when_false is not None:
-            self.command("else", indent_spaces=indent_spaces)
-            for command_to_run_when_false in commands_to_run_when_false:
-                self.command(
-                    command_to_run_when_false,
-                    indent_spaces=indented_block_spaces)
+            self.command("else")
+            with self.indented_block():
+                for command_to_run_when_false in commands_to_run_when_false:
+                    self.command(command_to_run_when_false)
 
-        self.command('fi', indent_spaces=indent_spaces)
+        self.command('fi')
 
 
 class Advice(Plugin):
diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index 8f9ae58f0d..f639c69bb4 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -34,13 +34,13 @@ def check_and_set_ca_cert_paths(self):
             "for {} in ${}".format(
                 single_ca_path_variable, ca_paths_variable))
         self.log.command("do")
-        self.log.exit_on_predicate(
-            '[ ! -f "${}" ]'.format(single_ca_path_variable),
-            ['Invalid CA certificate filename: ${}'.format(
-                single_ca_path_variable),
-             'Please check that the path exists and is a valid file'],
-            indent_spaces=2
-        )
+        with self.log.indented_block():
+            self.log.exit_on_predicate(
+                '[ ! -f "${}" ]'.format(single_ca_path_variable),
+                ['Invalid CA certificate filename: ${}'.format(
+                    single_ca_path_variable),
+                 'Please check that the path exists and is a valid file']
+            )
         self.log.command("done")
 
     def upload_smartcard_ca_certificates_to_systemwide_db(self):
@@ -49,13 +49,13 @@ def upload_smartcard_ca_certificates_to_systemwide_db(self):
                 self.single_ca_cert_variable_name,
                 self.smart_card_ca_certs_variable_name))
         self.log.command("do")
-        self.log.command(
-            'certutil -d {} -A -i ${} -n "Smart Card CA $(uuidgen)" '
-            '-t CT,C,C'.format(
-                self.systemwide_nssdb, self.single_ca_cert_variable_name
-            ),
-            indent_spaces=2
-        )
+        with self.log.indented_block():
+            self.log.command(
+                'certutil -d {} -A -i ${} -n "Smart Card CA $(uuidgen)" '
+                '-t CT,C,C'.format(
+                    self.systemwide_nssdb, self.single_ca_cert_variable_name
+                ),
+            )
         self.log.command("done")
 
     def install_smart_card_signing_ca_certs(self):
@@ -64,13 +64,13 @@ def install_smart_card_signing_ca_certs(self):
                 self.single_ca_cert_variable_name,
                 self.smart_card_ca_certs_variable_name))
         self.log.command("do")
-        self.log.exit_on_failed_command(
-            'ipa-cacert-manage install ${}'.format(
-                self.single_ca_cert_variable_name
-            ),
-            ['Failed to install external CA certificate to IPA'],
-            indent_spaces=2
-        )
+        with self.log.indented_block():
+            self.log.exit_on_failed_command(
+                'ipa-cacert-manage install ${}'.format(
+                    self.single_ca_cert_variable_name
+                ),
+                ['Failed to install external CA certificate to IPA']
+            )
         self.log.command("done")
 
     def update_ipa_ca_certificate_store(self):
@@ -218,8 +218,7 @@ def check_and_enable_pkinit(self):
         self.log.command('else')
         self.log.exit_on_failed_command(
             'ipa-pkinit-manage enable',
-            ['Failed to issue PKINIT certificates to local KDC'],
-            indent_spaces=2)
+            ['Failed to issue PKINIT certificates to local KDC'])
         self.log.command('fi')
 
     def enable_ok_to_auth_as_delegate_on_http_principal(self):

From e14f4724ecf3bb7f9fe9cf90ed2c493c06330e2e Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 15:00:00 +0200
Subject: [PATCH 06/11] advise: add an infrastructure for formatting Bash
 compound statements

A series of context managers simplify formatting of common compound
statements such as `if`, `else if`, `else` blocks.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/base.py | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/ipaserver/advise/base.py b/ipaserver/advise/base.py
index c320b002c8..940d87ed4c 100644
--- a/ipaserver/advise/base.py
+++ b/ipaserver/advise/base.py
@@ -128,6 +128,79 @@ def dedent(self):
         self._recompute_indentation_level()
 
 
+class CompoundStatement(object):
+    """
+    Wrapper around indented blocks of Bash statements.
+
+    Override `begin_statement` and `end_statement` methods to issue
+    opening/closing commands using the passed in _AdviceOutput instance
+    """
+
+    def __init__(self, advice_output):
+        self.advice_output = advice_output
+
+    def __enter__(self):
+        self.begin_statement()
+        self.advice_output.indent()
+
+    def begin_statement(self):
+        pass
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.advice_output.dedent()
+        self.end_statement()
+
+    def end_statement(self):
+        pass
+
+
+class IfBranch(CompoundStatement):
+    """
+    Base wrapper around `if` branch. The closing statement is empty so it
+    leaves trailing block that can be closed off or continued by else branches
+    """
+    def __init__(self, advice_output, conditional):
+        super(IfBranch, self).__init__(advice_output)
+        self.conditional = conditional
+
+    def begin_statement(self):
+        self.advice_output.command('if {}'.format(self.conditional))
+        self.advice_output.command('then')
+
+
+class ElseIfBranch(CompoundStatement):
+    """
+    Wrapper for `else if <CONDITIONAL>`
+    """
+    def __init__(self, advice_output, alternative_conditional):
+        super(ElseIfBranch, self).__init__(advice_output)
+        self.alternative_conditional = alternative_conditional
+
+    def begin_statement(self):
+        command = 'else if {}'.format(self.alternative_conditional)
+
+        self.advice_output.command(command)
+
+
+class ElseBranch(CompoundStatement):
+    """
+    Wrapper for final `else` block
+    """
+    def begin_statement(self):
+        self.advice_output.command('else')
+
+    def end_statement(self):
+        self.advice_output.command('fi')
+
+
+class UnbranchedIfStatement(IfBranch):
+    """
+    Plain `if` without branches
+    """
+    def end_statement(self):
+        self.advice_output.command('fi')
+
+
 class _AdviceOutput(object):
 
     def __init__(self):

From 228403c0f427a192616da8b25bfa0e712b2711c1 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 15:02:25 +0200
Subject: [PATCH 07/11] delegate formatting of compound Bash statements to
 dedicated classes

this simplifies handling compound statements using _AdviceOutput class.
The necessary statements are exposed as context managers and API for
most common constructs is provided.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/base.py | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/ipaserver/advise/base.py b/ipaserver/advise/base.py
index 940d87ed4c..581478fb75 100644
--- a/ipaserver/advise/base.py
+++ b/ipaserver/advise/base.py
@@ -286,33 +286,53 @@ def exit_on_nonroot_euid(self):
         )
 
     def exit_on_predicate(self, predicate, error_message_lines):
-        commands_to_run = [
-            self._format_error(error_message_line)
-            for error_message_line in error_message_lines]
+        with self.unbranched_if(predicate):
+            for error_message_line in error_message_lines:
+                self.command(self._format_error(error_message_line))
 
-        commands_to_run.append('exit 1')
-        self.commands_on_predicate(
-            predicate,
-            commands_to_run)
+            self.command('exit 1')
+
+    @contextmanager
+    def unbranched_if(self, predicate):
+        with self._compound_statement(UnbranchedIfStatement, predicate):
+            yield
+
+    @contextmanager
+    def _compound_statement(self, statement_cls, *args):
+        with statement_cls(self, *args):
+            yield
 
     def commands_on_predicate(self, predicate, commands_to_run_when_true,
                               commands_to_run_when_false=None):
-        if_command = 'if {}'.format(predicate)
-        self.command(if_command)
-        self.command('then')
+        if commands_to_run_when_false is not None:
+            if_statement = self.if_branch
+        else:
+            if_statement = self.unbranched_if
 
-        with self.indented_block():
+        with if_statement(predicate):
             for command_to_run_when_true in commands_to_run_when_true:
                 self.command(
                     command_to_run_when_true)
 
         if commands_to_run_when_false is not None:
-            self.command("else")
-            with self.indented_block():
+            with self.else_branch():
                 for command_to_run_when_false in commands_to_run_when_false:
                     self.command(command_to_run_when_false)
 
-        self.command('fi')
+    @contextmanager
+    def if_branch(self, predicate):
+        with self._compound_statement(IfBranch, predicate):
+            yield
+
+    @contextmanager
+    def else_branch(self):
+        with self._compound_statement(ElseBranch):
+            yield
+
+    @contextmanager
+    def else_if_branch(self, predicate):
+        with self._compound_statement(ElseIfBranch, predicate):
+            yield
 
 
 class Advice(Plugin):

From 318ac1faa95494242a46dbb3f3e382dd5a603a6f Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 15:03:45 +0200
Subject: [PATCH 08/11] Fix indentation of statements in Smart card advises

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/plugins/smart_card_auth.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index f639c69bb4..f2f7bf4954 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -162,13 +162,13 @@ def enable_nss_ocsp(self):
             predicate,
             [
                 self._interpolate_ocsp_directive_file_into_command(
-                    "  sed -i.ipabkp -r "
+                    "sed -i.ipabkp -r "
                     "'s/^#*[[:space:]]*{directive}[[:space:]]+(on|off)$"
                     "/{directive} on/' {filename}")
             ],
             commands_to_run_when_false=[
                 self._interpolate_ocsp_directive_file_into_command(
-                    "  sed -i.ipabkp '/<\/VirtualHost>/i {directive} on' "
+                    "sed -i.ipabkp '/<\/VirtualHost>/i {directive} on' "
                     "{filename}")
             ]
         )

From 0f609b41504d23bb5a023370d73a1688f6b28312 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 15:08:08 +0200
Subject: [PATCH 09/11] Use the compound statement formatting API for
 configuring PKINIT

Use `if_branch` and `else_branch` context managers instead of raw
`command` calls in the method that generates Bash snippet that
configures PKINIT on the master.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/plugins/smart_card_auth.py | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index f2f7bf4954..763f1ed740 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -211,15 +211,13 @@ def record_httpd_ocsp_status(self):
 
     def check_and_enable_pkinit(self):
         self.log.comment('check whether PKINIT is configured on the master')
-        self.log.command(
-            "if ipa-pkinit-manage status | grep -q 'enabled'")
-        self.log.command('then')
-        self.log.command('  echo "PKINIT already enabled"')
-        self.log.command('else')
-        self.log.exit_on_failed_command(
-            'ipa-pkinit-manage enable',
-            ['Failed to issue PKINIT certificates to local KDC'])
-        self.log.command('fi')
+        with self.log.if_branch(
+                "ipa-pkinit-manage status | grep -q 'enabled'"):
+            self.log.command('echo "PKINIT already enabled"')
+        with self.log.else_branch():
+            self.log.exit_on_failed_command(
+                'ipa-pkinit-manage enable',
+                ['Failed to issue PKINIT certificates to local KDC'])
 
     def enable_ok_to_auth_as_delegate_on_http_principal(self):
         self.log.comment('Enable OK-AS-DELEGATE flag on the HTTP principal')

From 0d153acf163659ceae7f80b919d04997521004b4 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 22 Jun 2017 15:30:41 +0200
Subject: [PATCH 10/11] smart card advises: use a wrapper around Bash `for`
 loops

Replace the raw `command` calls constructing the for loops in some
methods by a wrapper hiding this detail.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/base.py                    | 23 +++++++++++++++++++++++
 ipaserver/advise/plugins/smart_card_auth.py | 26 +++++++-------------------
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/ipaserver/advise/base.py b/ipaserver/advise/base.py
index 581478fb75..be72744170 100644
--- a/ipaserver/advise/base.py
+++ b/ipaserver/advise/base.py
@@ -201,6 +201,24 @@ def end_statement(self):
         self.advice_output.command('fi')
 
 
+class ForLoop(CompoundStatement):
+    """
+    Wrapper around the for loop
+    """
+    def __init__(self, advice_output, loop_variable, iterable):
+        super(ForLoop, self).__init__(advice_output)
+        self.loop_variable = loop_variable
+        self.iterable = iterable
+
+    def begin_statement(self):
+        self.advice_output.command(
+            'for {} in {}'.format(self.loop_variable, self.iterable))
+        self.advice_output.command('do')
+
+    def end_statement(self):
+        self.advice_output.command('done')
+
+
 class _AdviceOutput(object):
 
     def __init__(self):
@@ -334,6 +352,11 @@ def else_if_branch(self, predicate):
         with self._compound_statement(ElseIfBranch, predicate):
             yield
 
+    @contextmanager
+    def for_loop(self, loop_variable, iterable):
+        with self._compound_statement(ForLoop, loop_variable, iterable):
+            yield
+
 
 class Advice(Plugin):
     """
diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index 763f1ed740..ed722a9587 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -30,48 +30,36 @@ def check_and_set_ca_cert_paths(self):
             ['You need to provide one or more paths to the PEM files '
              'containing CAs signing the Smart Cards']
         )
-        self.log.command(
-            "for {} in ${}".format(
-                single_ca_path_variable, ca_paths_variable))
-        self.log.command("do")
-        with self.log.indented_block():
+        with self.log.for_loop(single_ca_path_variable,
+                               '${}'.format(ca_paths_variable)):
             self.log.exit_on_predicate(
                 '[ ! -f "${}" ]'.format(single_ca_path_variable),
                 ['Invalid CA certificate filename: ${}'.format(
                     single_ca_path_variable),
                  'Please check that the path exists and is a valid file']
             )
-        self.log.command("done")
 
     def upload_smartcard_ca_certificates_to_systemwide_db(self):
-        self.log.command(
-            "for {} in ${}".format(
+        with self.log.for_loop(
                 self.single_ca_cert_variable_name,
-                self.smart_card_ca_certs_variable_name))
-        self.log.command("do")
-        with self.log.indented_block():
+                '${}'.format(self.smart_card_ca_certs_variable_name)):
             self.log.command(
                 'certutil -d {} -A -i ${} -n "Smart Card CA $(uuidgen)" '
                 '-t CT,C,C'.format(
                     self.systemwide_nssdb, self.single_ca_cert_variable_name
-                ),
+                )
             )
-        self.log.command("done")
 
     def install_smart_card_signing_ca_certs(self):
-        self.log.command(
-            "for {} in ${}".format(
+        with self.log.for_loop(
                 self.single_ca_cert_variable_name,
-                self.smart_card_ca_certs_variable_name))
-        self.log.command("do")
-        with self.log.indented_block():
+                '${}'.format(self.smart_card_ca_certs_variable_name)):
             self.log.exit_on_failed_command(
                 'ipa-cacert-manage install ${}'.format(
                     self.single_ca_cert_variable_name
                 ),
                 ['Failed to install external CA certificate to IPA']
             )
-        self.log.command("done")
 
     def update_ipa_ca_certificate_store(self):
         self.log.exit_on_failed_command(

From cdeec3eb9101ba430e3ec27123c3cda82108804e Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 23 Jun 2017 15:47:48 +0200
Subject: [PATCH 11/11] smart card advise: use password when changing trust
 flags on HTTP cert

This is to prevent NSS asking for database password when operating in
FIPS 140 mode.

https://pagure.io/freeipa/issue/7036
---
 ipaserver/advise/plugins/smart_card_auth.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ipaserver/advise/plugins/smart_card_auth.py b/ipaserver/advise/plugins/smart_card_auth.py
index ed722a9587..9f743fc030 100644
--- a/ipaserver/advise/plugins/smart_card_auth.py
+++ b/ipaserver/advise/plugins/smart_card_auth.py
@@ -2,6 +2,8 @@
 # Copyright (C) 2017 FreeIPA Contributors see COPYING for license
 #
 
+import os
+
 from ipalib.plugable import Registry
 from ipaplatform.paths import paths
 from ipaserver.advise.base import Advice
@@ -169,6 +171,8 @@ def _format_command(self, fmt_line, directive, filename):
         return fmt_line.format(directive=directive, filename=filename)
 
     def mark_httpd_cert_as_trusted(self):
+        httpd_nss_database_pwd_file = os.path.join(
+            paths.HTTPD_ALIAS_DIR, 'pwdfile.txt')
         self.log.comment(
             'mark the HTTP certificate as trusted peer to avoid '
             'chicken-egg startup issue')
@@ -178,8 +182,9 @@ def mark_httpd_cert_as_trusted(self):
                 " cut -f 2 -d ' ')"))
 
         self.log.exit_on_failed_command(
-            'certutil -M -n $http_cert_nick -d "{}" -t "Pu,u,u"'.format(
-                paths.HTTPD_ALIAS_DIR),
+            'certutil -M -n $http_cert_nick -d "{}" -f {} -t "Pu,u,u"'.format(
+                paths.HTTPD_ALIAS_DIR,
+                httpd_nss_database_pwd_file),
             ['Can not set trust flags on HTTP certificate'])
 
     def _interpolate_nssnickname_directive_file_into_command(self, fmt_line):
_______________________________________________
FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org
To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org

Reply via email to