Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added a fixture for integration tests to check selinux denials #312

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Archana-PandeyM
Copy link
Collaborator

No description provided.

@Archana-PandeyM Archana-PandeyM marked this pull request as draft November 7, 2024 11:01
Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

This will make all test fail even if some other RHEL package has SELinux misconfigured and gets run. Which I guess is good, but it could poison our results for some time until it gets fixed.

Would it make sense to only check lines that might be related to our tools (e.g. insights-client, gpg, python, ...?)

Comment on lines 67 to 75
result = logged_run(options, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
if "<no matches>" not in str(result.stdout):
lines = str(result.stdout).split("\n")
for line in lines:
words = line.split()
if "denied" in words:
assert "permissive=1" in words, "SELinux AVC denials found"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could simplify this a bit by passing text=True to logged_run(). This would be an equivalent:

result = logged_run(options, capture_output=True, text=True)
for line in result.stdout.split("\n"):
    if "denied" in line:
        assert "permissive=1" in line, "SELinux AVC denials found"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am ok to use capture_output=True instead of subprocess.PIPE. Only reason for using the later was to avoid python version issues. I think capture_output is available in version > python 3.7. But it should not affect the tests, I will modify the command. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@m-horky Like I suspsected 'capture_output' is failing on RHEL8 tests(python 3.6), so I am reverting the change back to original commit.

@ptoscano
Copy link
Contributor

ptoscano commented Nov 7, 2024

While the idea is good, I think this ought to be done rather in pytest-client-tools:

  • the fixture can run potentially after fixtures, so missing commands
  • pytest-client-tools, as it is a pytest plugin, knows already about the execution time of the test, and thus it is possible to properly restrict the lookup of the SELinux denials for each test specifically (see the checkpoints in ausearch)
  • pytest-client-tools also already has logs for each test, and the log with SELinux denials would be an additional one

@Archana-PandeyM
Copy link
Collaborator Author

While the idea is good, I think this ought to be done rather in pytest-client-tools:

  • the fixture can run potentially after fixtures, so missing commands
  • pytest-client-tools, as it is a pytest plugin, knows already about the execution time of the test, and thus it is possible to properly restrict the lookup of the SELinux denials for each test specifically (see the checkpoints in ausearch)
  • pytest-client-tools also already has logs for each test, and the log with SELinux denials would be an additional one

Thanks for feedback, I agree that we should put it in pytest_client_tools. Eventually we would add such checks in rhc repo as well. So would you suggest keeping this fixture in pytest_client_tools/plugin.py ?

@Archana-PandeyM
Copy link
Collaborator Author

This will make all test fail even if some other RHEL package has SELinux misconfigured and gets run. Which I guess is good, but it could poison our results for some time until it gets fixed.

Would it make sense to only check lines that might be related to our tools (e.g. insights-client, gpg, python, ...?)

Yes I planned to do so.

@ptoscano
Copy link
Contributor

ptoscano commented Nov 7, 2024

I agree that we should put it in pytest_client_tools. Eventually we would add such checks in rhc repo as well. So would you suggest keeping this fixture in pytest_client_tools/plugin.py ?

Not as a fixture, no. There are already hooks that track certain parts of the tests execution flow, and the SELinux checks would need to be added there:

  • before starting the test, a checkpoint needs to be created with ausearch
  • when a test finishes, get the list of denials between the checkpoint and the test completion, logging the result

In pytest-client-tools there are already the bits to handle per-test stuff, so this should not be complicated to add.

@Archana-PandeyM
Copy link
Collaborator Author

I agree that we should put it in pytest_client_tools. Eventually we would add such checks in rhc repo as well. So would you suggest keeping this fixture in pytest_client_tools/plugin.py ?

Not as a fixture, no. There are already hooks that track certain parts of the tests execution flow, and the SELinux checks would need to be added there:

  • before starting the test, a checkpoint needs to be created with ausearch
  • when a test finishes, get the list of denials between the checkpoint and the test completion, logging the result

In pytest-client-tools there are already the bits to handle per-test stuff, so this should not be complicated to add.

Would you suggest me to implement this in pytest_client_tools, Can you help me point to the bits that handle per-test stuff because I tried but could not figure out.

@ptoscano
Copy link
Contributor

Would you suggest me to implement this in pytest_client_tools, Can you help me point to the bits that handle per-test stuff because I tried but could not figure out.

Sure.

Start from the current code in plugin.py, in particular the hooks (see upstream documentation here [1]):

  • pytest_runtest_protocol is used when running a test; in pytest_client_tools a new per-text item, the internal class NodeRunningData (part of the util module): that class holds few things, including the log of the test
  • pytest_runtest_logfinish is used when a test ends; in pytest_client_tools this means that the item is dropped, after handling the bits of it -- e.g. closing the log file and archiving it in the artifacts of the test (see the internal ArtifactsCollector class)

So in practice I think that:

  • the NodeRunningData class should create a new checkpoint in ausearch, to mark the time when the test start (so denials before that point are ignored), and keep a reference to it
  • in pytest_runtest_logfinish, ausearch should extract the denials before the checkpoint, save the output in a file (e.g. selinux.log, audit.log, something like that) and archive that
  • possibly do all the above only if SELinux is enabled (see the selinuxenabled executable)

[1] https://docs.pytest.org/en/stable/reference/reference.html#hooks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants