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.

Sign up for GitHub

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

Jump to bottom

Enhance Open SCAP tests and add more profiles tests #18136

Closed
wants to merge 5 commits into from
Closed

Enhance Open SCAP tests and add more profiles tests #18136

wants to merge 5 commits into from

Conversation

vtrubovics
Copy link
Contributor

Enhance Open SCAP tests and add more profiles bash and ansible tests:
xccdf_org.ssgproject.content_profile_stig
xccdf_org.ssgproject.content_profile_cis
xccdf_org.ssgproject.content_profile_pci-dss-4
xccdf_org.ssgproject.content_profile_hipaa
xccdf_org.ssgproject.content_profile_anssi_bp28_high

External configuration, exclusions and expected results.
DS and ansible file modifications.
Better results analysis and display.
Results collection to log file.
Ansible playbook enhanced run and result analysis.

#x86 SP5 POO150863
oscap_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811273
oscap_hipaa.yaml https://openqa.suse.de/tests/12811274
oscap_cis.yaml https://openqa.suse.de/tests/12811275
oscap_pci_dss_4.yaml https://openqa.suse.de/tests/12811276
oscap_stig.yaml https://openqa.suse.de/tests/12811277
oscap_ansible_pci_dss_4.yaml https://openqa.suse.de/tests/12811278
oscap_ansible_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811279
oscap_ansible_hipaa.yaml https://openqa.suse.de/tests/12811280
oscap_ansible_stig.yaml https://openqa.suse.de/tests/12811281
oscap_ansible_cis.yaml https://openqa.suse.de/tests/12811282

#ARM SP4 POO150863
oscap_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811283
oscap_hipaa.yaml https://openqa.suse.de/tests/12811284
oscap_cis.yaml https://openqa.suse.de/tests/12811285
oscap_pci_dss_4.yaml https://openqa.suse.de/tests/12811286
oscap_stig.yaml https://openqa.suse.de/tests/12811287
oscap_ansible_pci_dss_4.yaml https://openqa.suse.de/tests/12811288
oscap_ansible_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811289
oscap_ansible_hipaa.yaml https://openqa.suse.de/tests/12811291
oscap_ansible_stig.yaml https://openqa.suse.de/tests/12811297
oscap_ansible_cis.yaml https://openqa.suse.de/tests/12811304

#ARM SP5 POO150863
oscap_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811343
oscap_hipaa.yaml https://openqa.suse.de/tests/12811344
oscap_cis.yaml https://openqa.suse.de/tests/12811345
oscap_pci_dss_4.yaml https://openqa.suse.de/tests/12811346
oscap_stig.yaml https://openqa.suse.de/tests/12811347
oscap_ansible_pci_dss_4.yaml https://openqa.suse.de/tests/12811348
oscap_ansible_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811349
oscap_ansible_hipaa.yaml https://openqa.suse.de/tests/12811350
oscap_ansible_stig.yaml https://openqa.suse.de/tests/12811351
oscap_ansible_cis.yaml https://openqa.suse.de/tests/12811352

#s390x SP4 POO150863
oscap_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811353
oscap_hipaa.yaml https://openqa.suse.de/tests/12811354
oscap_cis.yaml https://openqa.suse.de/tests/12811355
oscap_pci_dss_4.yaml https://openqa.suse.de/tests/12811356
oscap_stig.yaml https://openqa.suse.de/tests/12811357
oscap_ansible_pci_dss_4.yaml https://openqa.suse.de/tests/12811358
oscap_ansible_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811363
oscap_ansible_hipaa.yaml https://openqa.suse.de/tests/12811366
oscap_ansible_stig.yaml https://openqa.suse.de/tests/12811368
oscap_ansible_cis.yaml https://openqa.suse.de/tests/12811369

#s390x SP5 POO150863
oscap_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811373
oscap_hipaa.yaml https://openqa.suse.de/tests/12811374
oscap_cis.yaml https://openqa.suse.de/tests/12811375
oscap_pci_dss_4.yaml https://openqa.suse.de/tests/12811376
oscap_stig.yaml https://openqa.suse.de/tests/12811377
oscap_ansible_pci_dss_4.yaml https://openqa.suse.de/tests/12811378
oscap_ansible_anssi_bp_28_high.yaml https://openqa.suse.de/tests/12811379
oscap_ansible_hipaa.yaml https://openqa.suse.de/tests/12811380
oscap_ansible_stig.yaml https://openqa.suse.de/tests/12811381
oscap_ansible_cis.yaml https://openqa.suse.de/tests/12811382

@github-actions GitHub Actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

@vtrubovics vtrubovics force-pushed the POO150863 branch 4 times, most recently from fc3a1a2 to 9f60bfd Compare November 15, 2023 09:31
mdoucha
mdoucha reviewed Nov 15, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
@jknphy
Copy link
Contributor

jknphy commented Nov 22, 2023

Please squash all the commits in single one or split it in commits that has their own goal and description if apply.
Then would be informative to have the two first paragraph of the PR's 1st comment included in the resulting commit(s).

@vtrubovics
Enhance Open SCAP tests and add more profiles tests
d4033d6 
https://progress.opensuse.org/issues/150863

Fix of test_flags definition

Fix make tidy

Fix for get tests config

Fix sle version scope

Adding local sle version to functions

Fix slevesion and perlcritic 1

Reverted some code

Fixed 2nd perlcritic issue

Fix sle version in tests
@jknphy
Copy link
Contributor

jknphy commented Nov 22, 2023 •
edited

It would also be more clear if instead of having so many links in your PR's 1st comment, you would just provide 1 link with the whole matrix, when you use a tool like openqa-clone-custom-git-refspec it creates a branch for you when you can see everything and even to refine the url to show more products, it would avoid for you to go to links and edit everything for a change or re-run (if you have some initial wrong test suites, you might need to delete them and clean them up, but other than that inconvenient it is the standard way to provide test matrix for verification). As a bonus it helps with review, because you can search in all the runs.

jknphy
jknphy reviewed Nov 22, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 22, 2023
View reviewed changes
lib/oscap_tests.pm

sub replace_ansible_file {
Copy link
Contributor

Choose a reason for hiding this comment

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

functions in this library are extremely long and might need proper function documentation, Perldoc, there are multiple example in the repo, but I would recommend to leave this comment open and do it once all review regarding the library itself is concluded to avoid extra work, because we could consider split some functions but I need to take a closer look...

jknphy
jknphy reviewed Nov 23, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 23, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 23, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 23, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm
record_info("Copied ansible file", "Copied file $ansible_profile_ID to $full_ansible_file_path");
}
# scap-security-guide
elsif ($use_content_type == 1) {
Copy link
Contributor

@jknphy jknphy Nov 24, 2023 •
edited

Choose a reason for hiding this comment

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

instead of extensive comments, in general in the code some contants could be created on the top of the file in capital letters to represent those numeric values, kind of like enums in other languagues, leaving 0s and 1s like clear booleans or other numberic values as clear counters.

jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
jknphy
jknphy reviewed Nov 24, 2023
View reviewed changes
lib/oscap_tests.pm Outdated Show resolved Hide resolved
@jknphy
Copy link
Contributor

jknphy commented Nov 24, 2023

I think this is ok for some 1st review round. In summary:

  1. Too many similar files (test and yamls)-> using openQA settings should remediate that.
  2. One big library file (more than 1k lines) that should be split in small functions or even files and more readable code (the use of global variables doesn't help, function with clear parameters always does).

I'm wondering if it would be better to split this PR in multiple ones, to see single code path, for example focusing on one test, then it would be more feasible to review the library, otherwise it is very tricky, too many cases. But let's see how it looks after addressing the comments you think worth to address.

@vtrubovics
Copy link
Contributor Author

I think this is ok for some 1st review round. In summary:

1. Too many similar files (test and yamls)-> using openQA settings should remediate that.

2. One big library file (more than 1k lines) that should be split in small functions or even files and more readable code (the use of global variables doesn't help, function with clear parameters always does).

I'm wondering if it would be better to split this PR in multiple ones, to see single code path, for example focusing on one test, then it would be more feasible to review the library, otherwise it is very tricky, too many cases. But let's see how it looks after addressing the comments you think worth to address.

I trying to solve complex problem of testing SCAP profiles inside OpenQA. SCAP itself is quite big framework with big functionality.
I tried different approaches to implement required required (from hardening team) functionality with minimal complexity and good flexibility.
These tests will evolve with additional new requirements and remediation methods.

@tjyrinki
Copy link
Contributor

Superseded by #18503 which should be the same but with conflicts resolved.

@tjyrinki tjyrinki closed this Jan 23, 2024
@tjyrinki tjyrinki mentioned this pull request Jan 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jknphy jknphy jknphy left review comments

@mdoucha mdoucha mdoucha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@vtrubovics @jknphy @tjyrinki @mdoucha