V2 controller #76

x00Pavel · 2022-06-14T07:36:46Z

Implementation of Controller class.

The controller contains methods for system preparation and should act as a glue between CLI - models and test - models.

Design changes

None for now

Issue resolved/closed

this list might be updated as PR leaves and new feautres/bugs are fixed:

mahavrila · 2022-06-27T07:20:04Z

SCAutolib/models/file.py

+        :return: value of the key in section (if set)
+        """
+        if section is None:
+            # simple config files without sections


Support for getting values from section-less config is fragile and due to lack of "logic" in section-less configs this can not be reliably improved. This will return value for first occurrence, however, there can be multiple occurrences of the same key in section-less file. As we discussed earlier, section-less config files are problematic and we support them rather symbolically. There should be at least some warning saying "You are applying get method on non-standard config file. This method would return only the first match". However, this is problematic itself, because some tools may use last match rather than first. IMHO, better approach would be to check if the key is present in the config multiple times and raise an exception if true.

Can you please fix this via PR?

mahavrila · 2022-06-27T07:20:58Z

SCAutolib/models/file.py

+        file do not support sections (section for this method is not provided),
+        then file would be parsed in a simple way with `.readlines()` method and
+        using separator each line would be split into two parts: key and a
+        value. If key is matched, the value is striped and returned.


We should explicitly mention that first match is returned or rewrite this method (see below).

mahavrila · 2022-06-27T07:24:50Z

SCAutolib/models/file.py

@@ -414,3 +476,7 @@ def save(self):
            else:
                # in case set method was used
                self._default_parser.write(config)
+
+    @property


This method can be removed as it is already in parent class.

mahavrila · 2022-06-27T07:27:46Z

SCAutolib/models/file.py

@@ -53,6 +54,10 @@ def __init__(self, filepath: Union[str, Path], template: str = None):
        if template is not None:
            self._template = Path(template)

+    @property
+    def path(self):


Do we want to add simple one-line docstring for purposes of auto-generated documentation?

mahavrila · 2022-06-27T07:29:40Z

SCAutolib/models/file.py

@@ -144,6 +199,12 @@ def clean(self):
        except FileNotFoundError:
            logger.info(f"{self._conf_file} does not exist. Nothing to do.")

+    def backup(self):
+        new_name = f"{self._conf_file.name}.backup"


Docstring missing, please add at least one-liner so auto-generated documentation print something.

mahavrila · 2022-06-27T07:33:56Z

SCAutolib/models/file.py

@@ -144,6 +199,12 @@ def clean(self):
        except FileNotFoundError:
            logger.info(f"{self._conf_file} does not exist. Nothing to do.")

+    def backup(self):


We need to discuss back-ups as for example SSSDConf class has it's own backup mechanism and for other files we did not consider backing-up at all. If we keep this method here, SSSDConf class should return exception if we try to apply it.

Well, I don't see backup method in SSSD class. So, this means that backup there is done in some other method of the SSSD class (what is not good) or somewhere else (what is also not very good). To avoid usage of this parent class method SSSDConf class only should implement its own method called backup. I didn't check this because I made changes only related to the controller by itself and didn't look for side effects because this work would be much bigger. From this perspective, I would suggest you add this method via PR. What do you think about this?

mahavrila · 2022-06-27T07:34:54Z

SCAutolib/models/file.py

@@ -261,6 +322,7 @@ def save(self):
                # to empty parser object
                self._changes = ConfigParser()
                self._changes.optionxform = str
+        os.chmod(self._conf_file, 0o600)


I'm not sure if this should be done here or directly in controller. But if it happens here, docstring needs to be modified.

It should be done here because after this method is called it is expected that the file is in a ready-to-use state. So, will add docstring

mahavrila · 2022-06-27T07:46:49Z

SCAutolib/utils.py

@@ -0,0 +1,104 @@
+from cryptography.hazmat.primitives import serialization


Add module docstring please.

mahavrila · 2022-06-27T07:51:13Z

SCAutolib/utils.py

+
+        run(["semodule", "-i", f"{TEMPLATES_DIR}/virtcacard.cil"])
+
+        run(["systemctl", "restart", "pcscd"])


To discuss: Should we restart services from one place (controller) or from diverse methods and functions? IMHO, this should be controlled by controller.

This topic needs more discussion

mahavrila · 2022-06-27T07:57:18Z

SCAutolib/utils.py

+        return OSVersion.RHEL_9
+    elif "Red Hat Enterprise Linux release 8" in cnt:
+        return OSVersion.RHEL_8
+    else:


This will consider any other OS version as Fedora. For example RHEL-10 will be fedora and this may cause troubles in the future. In case there is no match fo RHEL-9 and RHEL-8, we should try to match Fedora and raise an exception if non of RHEL-9, RHEL-8 and Fedora is matched.

mahavrila · 2022-06-27T07:59:48Z

SCAutolib/utils.py

+    :param packages: list of packages to be installed
+    """
+    for pkg in packages:
+        logger.warning(f"Package {pkg} is not installed on the "


This function does not know if package is installed or not and just try to install it. Log message may be updated to reflect it.

Actually, everything is correct. this function is called after _check_packages, which returns a list of missing packages. But yes, this log message is redundant here, will remove it

mahavrila · 2022-06-27T08:01:04Z

SCAutolib/utils.py

+        out = run(["rpm", "-qa", pkg])
+        if pkg not in out.stdout:
+            logger.warning(f"Package {pkg} is required for the testing, "
+                           f"but doesn't present in the system")


is not instead of doesn't

mahavrila · 2022-06-27T08:58:32Z

SCAutolib/models/user.py

+            run(['userdel', '-f', self.username], check=True)
+        except KeyError:
+            pass
+        logger.info(f"User {self.username} does not present on the system")


is instead of does

mahavrila · 2022-06-27T09:00:26Z

SCAutolib/models/user.py

+            run(cmd, check=True)
+            cmd = ["passwd", self.username, "--stdin"]
+            run(cmd, input=self.password)
+        logger.info(f"User {self.username} presents ons the system")


is present instead of presents and fix typo in on

mahavrila · 2022-06-27T09:02:38Z

SCAutolib/models/user.py

+            pass
+        logger.info(f"User {self.username} does not present on the system")
+
+    def add_user(self, force=False):


If we are adding some logic in creating and deleting users, we may want to add also some unit-tests.

Yes. But again, the same resolution as for this comment, this PR is about the controller and I'm trying to not make it anormous

mahavrila · 2022-06-27T09:05:19Z

SCAutolib/models/user.py

@@ -167,3 +189,14 @@ def delete_user(self):
        run(['rm', '-r', '-f', f"/home/{self.username}"], check=True)
        logger.info(f"User {self.username} directory is removed.")
        logger.debug(r)
+
+    def gen_csr(self):


Docstring is missing.

mahavrila · 2022-06-27T09:35:26Z

SCAutolib/controller.py

@@ -0,0 +1,375 @@
+import json


module docstring missing

mahavrila · 2022-06-27T09:52:45Z

SCAutolib/controller.py

+        if install_missing:
+            _install_packages(missing)
+
+        run(['dnf', 'groupinstall', "Smart Card Support", '-y'])


We may want conditional execution here as Smart Card Support may already be installed at some systems.

no need to check it explicitly, because this would be skipped by the dnf if all packages are present

inikolchev · 2022-06-28T09:59:04Z

SCAutolib/controller.py

+
+        with self._lib_conf_path.open("r") as f:
+            self.lib_conf = json.load(f)
+            assert self.lib_conf, "Data are not loaded correctly."


Shouldn't this assert be after the validate_configuration function in the next line?

No, it shouldn't. This assert checks that json.load() loaded the file without problems. In validate_configuration there is schema.validate() method that would raise an expectation if the configuration doesn't match the schema of correctly loaded JSON file

inikolchev · 2022-06-28T10:01:18Z

SCAutolib/controller.py

+        """
+        ...
+
+    def setup_system(self, install_missing: bool, gdm: bool):


install_missing and gdm should default to False probably

yes, it is. These default values would be set in CLI and passed here in anyway

inikolchev · 2022-06-28T10:04:47Z

SCAutolib/controller.py

+                logger.info("Set force argument to True if you want to remove "
+                            "previous installation.")
+                return
+            self.ipa_ca.restore()


Why do we need ipa_ca.restore() here?

This will remove IPA client installation if force option is set

inikolchev · 2022-06-28T10:05:56Z

SCAutolib/controller.py

+            if not force:
+                logger.info("Set force argument to True if you want to remove "
+                            "previous installation.")
+                return


Shouldn't we raise an exception? The user expects IPA to be configured, but if an installation already exists and he didn't use 'force', we should probably raise an exception

I don't think so. Imagine that the user installed and configured IPA client on his own and wants to use the library with this configuration. Then it is not a problem that IPA is already installed.

inikolchev · 2022-06-28T10:09:52Z

SCAutolib/controller.py

+                                    cert=user_dict["cert"],
+                                    key=user_dict["key"])
+
+            new_user.add_user()


Don't we need create and save here also?

create and save for what? user is just added to the system. this action includes "creation" and "saving"

inikolchev · 2022-06-28T10:11:54Z

SCAutolib/controller.py

+    def cleanup(self):
+        ...


I suppose this is still in progress?

inikolchev · 2022-06-28T10:12:48Z

SCAutolib/controller.py

+        Validate schema of the configuration file. If some value doesn't present
+        in the config file, this value would be looked in the CLI parameters


I'm wondering if we should prioritize CLI parameters over config file, I think it might make more sense

Yes, makes sense. But let's leave this until the implementation of CLI commands would be in progress.

Created an Issue #84

inikolchev · 2022-06-28T10:17:39Z

SCAutolib/controller.py

+        with virtual smart card. Also, repository for installing virt_cacard
+        package is added in this method.


Shouldn't the virt_cacard repo be added in setup_system before we try to install virt_cacard?

Actually, it is added before installation here. But this call should be removed from this function

inikolchev · 2022-06-28T10:18:38Z

SCAutolib/controller.py

+        rmtree("/var/lib/sss/mc/*", ignore_errors=True)
+        rmtree("/var/lib/sss/db/*", ignore_errors=True)


Are these directories something that we can safely remove?

yes. it is cache directories

inikolchev · 2022-06-28T10:20:04Z

SCAutolib/models/card.py

    _nssdb: Path = None
    _template: Path = Path(TEMPLATES_DIR, "virt_cacard.service")
    _pattern = r"(pkcs11:model=PKCS%2315%20emulated;" \
               r"manufacturer=Common%20Access%20Card;serial=.*)"

-    def __init__(self, user: User, insert: bool = False,
-                 softhsm2_conf: Path = None):
+    def __init__(self, user, insert: bool = False, ):


trailing comma

Add setup methods

Move functions to static methods

Code cleanup

Add docs and code cleanup

x00Pavel · 2022-07-21T10:02:56Z

Closing this PR because it is too big. Several smaller PR's will follow

Add "setup" methods in Controller class #86
Dump and load methods #87
Futher will follow

x00Pavel self-assigned this Jun 14, 2022

mahavrila reviewed Jun 27, 2022

View reviewed changes

SCAutolib/controller.py

@@ -0,0 +1,375 @@

import json

Copy link

Collaborator

mahavrila Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module docstring missing

mahavrila reviewed Jun 27, 2022

View reviewed changes

inikolchev reviewed Jun 28, 2022

View reviewed changes

x00Pavel mentioned this pull request Jun 28, 2022

Prioritize CLI options over config file #84

Open

x00Pavel force-pushed the V2-Controller branch 3 times, most recently from a70d41c to 8521456 Compare July 18, 2022 07:44

x00Pavel mentioned this pull request Jul 18, 2022

Add "setup" methods in Controller class #86

Merged

x00Pavel added 5 commits July 19, 2022 10:35

Implement base for Controller class

3c995f9

Add setup methods

Update cards with Softhsm2 conf and linking to the user

ab9b26b

Add generation of CSR into the User class for IPA user

c35c478

Add generation of CSR to the local user class close #74

d06ff09

Add installation of SELinux module Closes #72

c525f45

x00Pavel added 21 commits July 19, 2022 10:37

Add side utility functions for the Controller in utils.py

49b33de

Move functions to static methods

Little bug fixes, close #79

92c50e7

Add get method and update set method with separator

f5c2d1d

Fix: use File model methods for editing general files

e7551e1

Fix: setup LocalCA

7ab9b7e

Fix: setup IPA client & user setup

490f062

Fix: minor fixes for setup flow

d517548

Update: tests for controller

bd69f87

Update: is_installed method for IPA CA

f31c8f4

Update logs and docstrings

3d9b949

Rename restore -> cleanup

f235a44

Fix detection of OS version

bb2783b

Fixes for CI and review

6ad79a2

Add dumping of files and some unit tests

01fbed6

Add dumping methods for objects used in tests

5c6f2c7

Fix loading the user with a card to tests

703b68a

Add context manager to the SSSDConf class

5f3637b

Code cleanup

Add exceptions and functions used in tests

52bd62d

Improvements for setting up the system

fffdf03

Add load method for CA's

1a85e4f

Fix: set user's password without expiration on the first login

0f55aad

Add docs and code cleanup

x00Pavel force-pushed the V2-Controller branch from 8521456 to 0f55aad Compare July 19, 2022 08:38

x00Pavel mentioned this pull request Jul 19, 2022

Dump and load methods #87

Merged

x00Pavel closed this Jul 21, 2022

		@@ -0,0 +1,104 @@
		from cryptography.hazmat.primitives import serialization


		run(["semodule", "-i", f"{TEMPLATES_DIR}/virtcacard.cil"])

		run(["systemctl", "restart", "pcscd"])

		Validate schema of the configuration file. If some value doesn't present
		in the config file, this value would be looked in the CLI parameters

		with virtual smart card. Also, repository for installing virt_cacard
		package is added in this method.

		rmtree("/var/lib/sss/mc/*", ignore_errors=True)
		rmtree("/var/lib/sss/db/*", ignore_errors=True)

V2 controller #76

V2 controller #76

Conversation

x00Pavel commented Jun 14, 2022 • edited Loading

Implementation of Controller class.

Design changes

Issue resolved/closed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahavrila Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x00Pavel Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x00Pavel commented Jul 21, 2022

x00Pavel commented Jun 14, 2022 •

edited

Loading

mahavrila Jun 27, 2022 •

edited

Loading

x00Pavel Jun 28, 2022 •

edited

Loading