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

ipatests: healthcheck: test if system is FIPS enabled #6368

Closed
wants to merge 1 commit into from
Closed

ipatests: healthcheck: test if system is FIPS enabled #6368

wants to merge 1 commit into from

Conversation

b3lix
Copy link
Contributor

@b3lix b3lix commented Jul 18, 2022

Test if FIPS is enabled and the check exists.

Related: https://pagure.io/freeipa/issue/8951

Signed-off-by: Erik Belko ebelko@redhat.com

@miskopo miskopo self-requested a review July 18, 2022 10:10
@miskopo miskopo added the ipa-4-9 Mark for backport to ipa 4.9 label Jul 18, 2022
@miskopo miskopo requested a review from rcritten July 18, 2022 10:10
@miskopo miskopo mentioned this pull request Jul 18, 2022
Closed
rcritten
rcritten reviewed Jul 18, 2022
View reviewed changes
ipatests/test_integration/test_ipahealthcheck.py
assert returncode == 0

cmd = self.master.run_command(['fips-mode-setup', '--is-enabled'],
raiseonerr=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you pass source ipahealthcheck/meta/core and check MetaCheck to run_healthcheck then it will only run the check that contains the fips output so there will be less to go through and a bit faster.

rcritten
rcritten reviewed Jul 18, 2022
View reviewed changes
ipatests/test_integration/test_ipahealthcheck.py Outdated
elif check["kw"]["fips"] == "enabled":
assert returncode == 0
else:
assert returncode == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than only executing when fips is a keyword, I'd look for the specific check that should contain this. Otherwise if for some reason ipa-healthcheck is broken and not returning MetaCheck then this test will not catch it.

I think it's worthwhile to also run this with FIPS enabled. There are "fake" enable/disable methods for fips in the TestIntegration class that may be sufficient. They are currently unused AFAICT so no guarantees they will do the right thing.

Copy link
Member

@miskopo miskopo Jul 18, 2022 •
edited

Choose a reason for hiding this comment

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

@rcritten the test is running in true FIPS env[1] with both FIPS enabled and disabled, however, I concur it should run with both in one pass.

The runs are not visible currently, as the CI has an issue with space and doesn't trigger temp commits.

[1] introduced with freeipa/freeipa-pr-ci#452

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I looked but missed that in the test definition. Testing both in the temp commit would be great, just reference the passing tests for both.

@mrizwan93 mrizwan93 added the re-run Trigger a new run of PR-CI label Jul 19, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 19, 2022
@rcritten rcritten added the re-run Trigger a new run of PR-CI label Jul 19, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 19, 2022
miskopo
miskopo requested changes Jul 20, 2022
View reviewed changes
Copy link
Member

@miskopo miskopo left a comment

Choose a reason for hiding this comment

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

Please, see inline comments

ipatests/prci_definitions/temp_commit.yaml Outdated Show resolved Hide resolved
ipatests/prci_definitions/temp_commit.yaml Outdated Show resolved Hide resolved
@miskopo miskopo added the re-run Trigger a new run of PR-CI label Jul 20, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 20, 2022
miskopo
miskopo requested changes Jul 20, 2022
View reviewed changes
ipatests/test_integration/test_ipahealthcheck.py Outdated
returncode = cmd.returncode

# If this produces KeyError, the check does not exist
if check["kw"]["fips"] == "disabled":
Copy link
Member

Choose a reason for hiding this comment

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

check is a one element list, therefore to access the dictionary inside, you need to pass an index first, in this case check[0]["kw"]["fips"]

ipatests/test_integration/test_ipahealthcheck.py Outdated
# If this produces KeyError, the check does not exist
if check["kw"]["fips"] == "disabled":
assert returncode == 2
elif check["kw"]["fips"] == "enabled":
Copy link
Member

Choose a reason for hiding this comment

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

same as on line 361

ipatests/test_integration/test_ipahealthcheck.py Outdated
raiseonerr=False)
returncode = cmd.returncode

# If this produces KeyError, the check does not exist
Copy link
Member

Choose a reason for hiding this comment

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

Accordingly, if the list of the checks is empty (i.e. the MetaCheck does not exist), this will fail on IndexError

@miskopo miskopo added the re-run Trigger a new run of PR-CI label Jul 20, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 20, 2022
@miskopo miskopo mentioned this pull request Jul 21, 2022
Closed
@b3lix b3lix requested a review from miskopo July 22, 2022 10:03
@miskopo miskopo added the re-run Trigger a new run of PR-CI label Jul 22, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 22, 2022
@miskopo miskopo added the re-run Trigger a new run of PR-CI label Jul 25, 2022
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 25, 2022
@miskopo
Copy link
Member

miskopo commented Jul 25, 2022

Azure issue caused most probably by #6342 (comment), since the same PR-CI test is running fine, I believe it's fine to ignore the failures as of now.

@b3lix
Copy link
Contributor Author

b3lix commented Jul 25, 2022 •
edited

@miskopo
Copy link
Member

miskopo commented Jul 25, 2022

@rcritten the fips and non-fips checks passed, ready for a review

@rcritten
Copy link
Contributor

Docs failing to build is due to https://pagure.io/freeipa/issue/9208

@rcritten
Copy link
Contributor

This change looks fine, nice work.

@b3lix
ipatests: healthcheck: test if system is FIPS enabled
f779a10 
Test if FIPS is enabled and the check exists.

Related: https://pagure.io/freeipa/issue/8951

Signed-off-by: Erik Belko <ebelko@redhat.com>
@miskopo miskopo added ack Pull Request approved, can be merged ipa-4-10 Mark for backport to ipa 4.10 pushed Pull Request has already been pushed labels Jul 27, 2022
@miskopo
Copy link
Member

miskopo commented Jul 27, 2022

master:

  • fc5de82 ipatests: healthcheck: test if system is FIPS enabled

@miskopo miskopo closed this Jul 27, 2022
This was referenced Jul 27, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcritten rcritten rcritten left review comments

@miskopo miskopo Awaiting requested review from miskopo

Assignees

@b3lix b3lix

Labels
ack Pull Request approved, can be merged ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
5 participants
@b3lix @miskopo @rcritten @mrizwan93 @freeipa-pr-ci