redhat-qe-security / scautolib Goto Github PK

Currently, there is no check anywhere for the duplicated user on user.add_user method.

When running the whole suite of unit tests that includes tests with IPA client and service restarts, the run fails on the test for SSSDConf class on the step of comparing values. This bug doesn't block any CI, and running this test without running IPA tests or as individual tests is passing.

does not have

Originally posted by @ep69 in #86 (comment)

Exit code of tested command should always be checked unless this check is turned off by user.
Reasoning is: We want our commands almost always to pass and it's good to have additional control to indicate any kind of failures. This would prevent similar false positives to those we observed before control of "reject" string was added.

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

Originally posted by @inikolcev in #76 (comment)

CLI option should have higher priority that values from config file. So CLI params can not only add values for not existing values, but also overwrite existing ones

Tried to execute SCAutilib.controller.Controller.setup_system() and this throughs the error. Seems that the lib directory is not created
Addin logs from ipython

SCAutolib:file.create.216 [WARNING] /etc/sssd/sssd.conf not present
SCAutolib:file.create.217 [WARNING] Creating sssd.conf based on the template
SCAutolib:file.create.220 [INFO] Updating /etc/sssd/sssd.conf with values from the template
---------------------------------------------------------------------------
FileNotFoundError                         Traceback (most recent call last)
<ipython-input-3-6692449aca3f> in <module>
----> 1 cnt.setup_system(True, False)

~/SCAutolib/SCAutolib/controller.py in setup_system(self, install_missing, gdm)
     98         logger.debug("Smart Card Support group in installed.")
     99 
--> 100         self.sssd_conf.create()
    101         self.sssd_conf.save()
    102 

~/SCAutolib/SCAutolib/models/file.py in create(self)
    222             self._default_parser.read_file(template)
    223 
--> 224         with self._backup_default.open("w") as bdefault:
    225             self._default_parser.write(bdefault)
    226 

/usr/lib64/python3.6/pathlib.py in open(self, mode, buffering, encoding, errors, newline)
   1181             self._raise_closed()
   1182         return io.open(str(self), mode, buffering, encoding, errors, newline,
-> 1183                        opener=self._opener)
   1184 
   1185     def read_bytes(self):

/usr/lib64/python3.6/pathlib.py in _opener(self, name, flags, mode)
   1035     def _opener(self, name, flags, mode=0o666):
   1036         # A stub for the opener argument to built-in open()
-> 1037         return self._accessor.open(self, flags, mode)
   1038 
   1039     def _raw_open(self, flags, mode=0o777):

/usr/lib64/python3.6/pathlib.py in wrapped(pathobj, *args)
    385         @functools.wraps(strfunc)
    386         def wrapped(pathobj, *args):
--> 387             return strfunc(str(pathobj), *args)
    388         return staticmethod(wrapped)
    389 

FileNotFoundError: [Errno 2] No such file or directory: '/etc/SCAutolib/backup/default_sssd.conf'

In the library we realy in IPA client home directory to be created to store there some configuration files. But client's home directory is not created automatically on user-add to the IPA server, but only after the first successful user login to the system. This causes problems while creating virtual cards for this user in the home directory of the IPA users.

The current implementation of this method (detection if IPA client is configured on the system) is not reliable enough, so it makes false positive results when testing and using the library.

We have a problem with CNF files that we use to create a CSR (for local users). The problem is that IPA user uses different logic for creating the CSR file (options are specified in the CLI openssl command). We need to fix this issue for a more consistent workflow between controller and models.

Change and order of files in following diff so added line will start with "+" and not with "-" :
https://github.com/x00Pavel/SCAutolib/blob/e95e56ceaf543aff5de722dae00fa95553b858bf/src/utils.py#L33

Find the absolute path to the user config file as this file is symlinked and this requires an absolute path

For more complex testcases we may need to edit multiple lines of config file or even multiple config files. The library should provide a way how to do this in systematic and user-friendly way.

Some tests are failing in atuhselect context on restoring previous authselect configuration. Maybe due to not existing previous configuration and as a result not existing file with this conf

__init__ method should parse CLI arguments, validate config file schema, and initialize (no real action in the system) objects based on configuration.

Two things here:

• --install-missing flag is working in the opposite way: if set, then the value is False (missing packages are not installed), and the opposite if not set (value is True)
  • Possible solution: change default value on line to False
• If the library is supposed to install missing packages, but all packages are installed, then the function for installing packages would be called with an empty list. That would result failer in for DNF command on this line
  • Possible solution: add check for empty list

Missing IPA section in config file causes an exception in the prepare command that calls setup_ipa_client method.

Possible solution:

• add check for IPA section in prepare method
• add check for IPA section directly in setup_ipa_client method

Idea: two controllers

To separate functionality for CLI and the tests, create two controllers for CLI and the tests.

Use cases:

• CLI controller is very "heavyweight" in sense of logic. tests do not need all this stuff. to not make mistakes in the method it makes sense to separate logic dedicated to tests and to CLI (tests controller would be more lightweight)
• Authselect. We don't need authselect in CLI commands in general. This model is used in the tests. So why overload this controller with auxiliary stuff?

Prepare command fails in FIPS mode on installing IPA client while connecting to the IPA server using Fabric.Connection() class.
Fabric use MD5 for connecting, but FIPS does not support MD5, so the command fails. Ther is a PR for this, but it is not accepted yet, so a fix is needed.

doesn't -> are not

Originally posted by @ep69 in #86 (comment)

https://github.com/x00Pavel/SCAutolib/blob/299532674f9b1ca618fbd33c5327ab06fad936f4/SCAutolib/models/CA.py#L113
This and following lines should be solved as call of create method rather than implementing it here.

are not exists -> do not exist

Originally posted by @ep69 in #86 (comment)

Add filling default values in schema from a various sources based on priority: CLI options -> ENV vars

Create a class for the standard creation of config files for that would be edited in the test.
There are already 4 places in the library where the same code is used:

1. Create ConfigParser object: 3 lines (instantiate + set a default value for the object)
2. set value: several lines depending on how much editing would be done
3. Write: 2 - 3 lines

All this can be replaced with a class that inherited ConfigParser class and all workflow would by in 3 lines:

1. Instatiate - all values are set in the constructor
2. Set value - method could take a list of fields that need to be edited
3. Write

This are my notes about a code I wrote down last week. It may be a bit chaotic... We may consider to make some changes based on this notes or we can just wait for refactoring and ignore it...

1. general setup:

• only 'check semodule' and install missing packages
• check semodule is called only in general setup
• does it mean it is only necessary for package installation?
  This makes me ask - what should and should not be in 'general setup'? Why is for example 'create_sssd_config' standalone?

2. setup_virt_cacard_:

• local user is created here (an it is not logical)
• we need to setup card to create local user
• addition of local user should be standalone/separate function (similarly to addition of IPA user) for the clarity of code

3. create_softhsm2_config:

• is called in 'create_sc' twice:
  • A) separately
  • B) as part of setup_virt_cacard_
  • ...and setup_virt_cacard_ is called only in create_sc so call of create_softhsm2_config can be removed either from A) or B)

Add uninstalling IPA client from current host + deleting all entries about it from the IPA server. Otherwise, the IPA server would contain a lot of inactive hosts that never would be used again.

Calling subprocess for any bash command is mostly inefficient when there is pythonic way/module to do same operation. That would be good from performance point of view to reduce number of subprocess calls. For example, for operation with packages there is rpm python module that can be used for quivering over RPM database, installing/upgrading packages; restarting systemd services can be done by direct signal to bus from dbus python module

In certain scenarios, it could be beneficial to check that exit code of command has exact (non-zero) value.
Example: We test that SC authentication fails if user types wrong pin. We evaluate presence of string "Authentication failure" and if this string is present, our test pass. However, we need to turn off (which is default) check of exit code because failing authentication will of course provide (expected) non-zero exit code. It would be nice to monitor exit code of failing authentication to make sure it has always the same value - it's failing the same way.

P.S.: This does not hurry.

Problem statement:

Each CLI command needs a configuration file for taking values from it. Solution via specifying @click.option decorator for each subcommand in a group seems to be not very clear, and maybe there is a better way to do such thing.

Task:

Investigate possibilities of python click module to make groups of commands with some shared options.
If support for such a feature is not very nice, look for another solution to this problem.

Expected result:

All subcommands would obtain the configuration file via the same source/context without specifying this as an option for each subcommand.

Pexpect has some limitations that for return code, not-expect. Maybe replace it with subprocess with matching stdout and return code

check_output expects that reject and expect parameters are lists of strings. In case user mistakenly provides string instead of list check_output does not notice it and behaves unexpectedly. check_output should check the type of the variables and raise an exception in case variables have unexpected types.

Another user-friendly solution: check_output accepts both - strings and lists of strings and if it gets string, it transform it to single element list.

When run script for seting up fails after IPA client install and executed again it fails on installation of IPA client.
Add exception handler for this case

As we have utils, we should define what should it contain (i.e. create module docstring) and consider moving some functions to utils. run is a good candidate.

doesn't -> is not

Originally posted by @ep69 in #86 (comment)

Add functionality for an editing configuration file that is passed to the library on prepare step and used during testing.
Two possible approaches:

1. Pass all required arguments to prepare command and update given configuration file with them
2. Add CLI command for editing given configuration file.
   • arguments: path to the config file in YAML format, key=value pairs that would be updated in the given file

This method should properly clean up configured stuff in the system. Consider what is really needs to be restored/removed (e.g. sssd.conf should be reverted to the original state before any calls of the library, delete the users with it's home directory). Add corresponding method to the models if needed.

I passwd argument is specified, run_cmd waits for "PIN for" or "Password" string. If it times out while waiting for pin/passwd the test should fail.

does not -> is not

Originally posted by @ep69 in #86 (comment)

Currently, there are recursive dependencies between Card and User (maybe also other modules). This leads to an exception RecursionError: maximum recursion depth exceeded while calling a Python object during unit testing. This need fix

missing type, but you are saying you don't want to fix docstrings..

Originally posted by @ep69 in #86 (comment)

To consider and discuss:
check_output (in utils.py) allows user to specify reject patterns - if these patterns are present in output of tested command, exception is raised - our test Fails. Currently only a few of our tests have any reject pattern defined. It may be valuable to define "universal reject patterns" - for example "^.+failure". If the presence of "universal reject pattern" should cause Failure of the test or just Warning message would be defined based on future experience.

When a user is deleted with force option, the entire user directory is removed. So, when trying to revoke a certificate for this user in method revoke_cert (which requires path to the existing certificate), revocation failed because the path for the user certificate is no longer valid (doesn't exist at all).

does -> is

Originally posted by @ep69 in #86 (comment)

When the configuration file contains only local CA specification then after running prepare SSSD service is not restarted which causes the card can't be used for authentication right after system setup.

Add parameter for return code (SUCCESS or FAIL)

Tracker of issue not yet covered by V2.

be able to work correctly or just to work correctly

Originally posted by @ep69 in #86 (comment)

Library configuration directory /etc/SCAutolib (or simply configuration files) should be removed in the cleanup phase because it can cause aliases while executing tests several times on the same machine.

When the values contain percent sign (e.g. %20 as the encoding of space), it's not correctly inserted into the file so. Single percent would cause an exception, doubling (as suggested in the official doc) of the percent sign still leads to wrong interpolation.

Suggestion

For SSSDConf use python3-sssdconfig module that is shipped with sssd package.

Add parameter that specifies what shouldn't be in the output

Suggestion: In case an exception is hit in check_output function, the function should print message summarizing results of all performed checks or at least exit code of tested command.
Reasoning: It is beneficial for tester to know exit code mainly in case of failure (expect pattern is not found).