Check that the expected NSS token matches the current FIPS state#364
Merged
frasertweedale merged 1 commit intofreeipa:masterfrom Sep 18, 2025
Merged
Check that the expected NSS token matches the current FIPS state#364frasertweedale merged 1 commit intofreeipa:masterfrom
frasertweedale merged 1 commit intofreeipa:masterfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rcritten - I've reviewed your changes - here's some feedback:
- Remove the commented-out @patch decorators in the CertmongerFIPSTokensCheck tests to clean up unused code.
- Collapse the repetitive FIPS/non-FIPS & HSM token tests using pytest parametrization or fixtures to DRY up the test file.
- In CertmongerFIPTokensCheck.check, consider replacing the manual
failedflag with collecting errors into a list and yielding a final success only if the list is empty for clearer flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the commented-out @patch decorators in the CertmongerFIPSTokensCheck tests to clean up unused code.
- Collapse the repetitive FIPS/non-FIPS & HSM token tests using pytest parametrization or fixtures to DRY up the test file.
- In CertmongerFIPTokensCheck.check, consider replacing the manual `failed` flag with collecting errors into a list and yielding a final success only if the list is empty for clearer flow.
## Individual Comments
### Comment 1
<location> `tests/test_ipa_tracking.py:166` </location>
<code_context>
+ @patch('ipalib.install.certmonger.get_request_value')
+# @patch('ipahealthcheck.ipa.certs.get_expected_requests')
+# @patch('ipalib.install.certmonger._cm_dbus_object')
+ def test_nonfips_token_correct(self, mock_value, mock_fips):
+ mock_value.side_effect = [
+ 'FILE',
+ 'FILE',
+ 'NSSDB', 'NSS Certificate DB',
+ ]
+ mock_fips.return_value = False
+ set_requests(add=self.nss_tracking)
+# mock_requests.return_value=get_expected_requests()
+# mock_obj.side_effect=create_mock_dbus
+
+ framework = object()
+ registry.initialize(framework, config.Config)
+ f = CertmongerFIPTokensCheck(registry)
+
+ self.results = capture_results(f)
+
+ assert len(self.results) == 1
+ result = self.results.results[0]
+ assert result.result == constants.SUCCESS
+
+ @patch('ipaplatform.tasks.tasks.is_fips_enabled')
</code_context>
<issue_to_address>
Consider adding a test for the case where the token is missing or None.
All current tests assume the token is a valid string. Please add a test for cases where the token is None or an empty string to verify graceful handling and prevent unexpected exceptions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_nonfips_token_correct(self, mock_value, mock_fips):
mock_value.side_effect = [
'FILE',
'FILE',
'NSSDB', 'NSS Certificate DB',
]
mock_fips.return_value = False
set_requests(add=self.nss_tracking)
# mock_requests.return_value=get_expected_requests()
# mock_obj.side_effect=create_mock_dbus
=======
def test_nonfips_token_correct(self, mock_value, mock_fips):
mock_value.side_effect = [
'FILE',
'FILE',
'NSSDB', 'NSS Certificate DB',
]
mock_fips.return_value = False
set_requests(add=self.nss_tracking)
# mock_requests.return_value=get_expected_requests()
# mock_obj.side_effect=create_mock_dbus
@patch('ipalib.install.certmonger.get_request_value')
@patch('ipaplatform.tasks.tasks.is_fips_enabled')
def test_nonfips_token_missing_or_none(self, mock_fips, mock_value):
# Test with None token
mock_value.side_effect = [
'FILE',
'FILE',
None, None,
]
mock_fips.return_value = False
set_requests(add=self.nss_tracking)
framework = object()
registry.initialize(framework, config.Config)
f = CertmongerFIPTokensCheck(registry)
self.results = capture_results(f)
assert len(self.results) == 1
result = self.results.results[0]
# Adjust the expected result as appropriate for your check's behavior
assert result.result in (constants.SUCCESS, constants.WARNING, constants.ERROR)
# Test with empty string token
mock_value.side_effect = [
'FILE',
'FILE',
'', '',
]
mock_fips.return_value = False
set_requests(add=self.nss_tracking)
framework = object()
registry.initialize(framework, config.Config)
f = CertmongerFIPTokensCheck(registry)
self.results = capture_results(f)
assert len(self.results) == 1
result = self.results.results[0]
# Adjust the expected result as appropriate for your check's behavior
assert result.result in (constants.SUCCESS, constants.WARNING, constants.ERROR)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/ipahealthcheck/ipa/certs.py:1584` </location>
<code_context>
+ requires = ('dirsrv',)
+
+ @duration
+ def check(self):
+ # token names we care about
+ candidates = ('NSS FIPS 140-2 Certificate DB', 'NSS Certificate DB')
</code_context>
<issue_to_address>
Consider splitting the check() method into small helpers for expected token logic, DBus request fetching, and NSSDB filtering to clarify the main loop.
The new `check()` has three distinct concerns—figuring out the expected token, fetching/wrapping DBus requests, and filtering/generating results—all mixed together. Pull each concern into a tiny helper so the main loop reads straight through:
```python
class CertmongerFIPTokensCheck(IPAPlugin):
…
def _expected_token(self):
return (
'NSS FIPS 140-2 Certificate DB'
if tasks.is_fips_enabled()
else 'NSS Certificate DB'
)
def _fetch_requests(self):
cm = certmonger._certmonger()
for raw in cm.obj_if.get_requests():
yield certmonger._cm_dbus_object(
cm.bus, cm, raw,
certmonger.DBUS_CM_REQUEST_IF,
certmonger.DBUS_CM_IF,
True
)
def _nssdb_request_ids(self):
for req in self._fetch_requests():
nickname = req.prop_if.Get(certmonger.DBUS_CM_REQUEST_IF, 'nickname')
if not nickname:
continue
if certmonger.get_request_value(nickname, 'key-storage') != 'NSSDB':
continue
yield nickname
@duration
def check(self):
expected = self._expected_token()
failed = False
for req_id in self._nssdb_request_ids():
token = certmonger.get_request_value(req_id, 'key-token')
if token not in ('NSS FIPS 140-2 Certificate DB', 'NSS Certificate DB'):
continue
if token != expected:
failed = True
yield Result(
self, constants.ERROR,
key=req_id, token=token, expected_token=expected,
msg=(
f"certmonger request {req_id} has token {token} "
f"but expected {expected}"
)
)
if not failed:
yield Result(self, constants.SUCCESS, key='fipstokencheck')
```
This:
- Moves the FIPS-flag logic to `_expected_token()`
- Encapsulates all D-Bus plumbing in `_fetch_requests()`
- Filters down to NSSDB requests in `_nssdb_request_ids()`
- Leaves `check()` as a clear, single-pass loop over IDs
All original behavior is preserved, nesting is flattened, and each helper is trivial to unit-test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
frasertweedale
requested changes
Sep 8, 2025
tests/test_ipa_tracking.py
Outdated
Comment on lines
+172
to
+173
| # mock_requests.return_value=get_expected_requests() | ||
| # mock_obj.side_effect=create_mock_dbus |
Collaborator
There was a problem hiding this comment.
Can we tidy this up, or leave a comment to explain why these lines are commented out?
Collaborator
There was a problem hiding this comment.
Still got this (apparently?) vestigal commented-out code. Not a blocker. Leave it to you whether to remove or not. (Maybe it's there for a reason...)
Collaborator
|
Another thing: I did a copr build including this change, and the build failed due to test failure in the RHEL 9 buildroot. Transcript: https://download.copr.fedorainfracloud.org/results/ftweedal/ipa-healthcheck/rhel-9-x86_64/09550177-freeipa-healthcheck/builder-live.log.gz . rhel10 and all fedora builds were fine. The failing tests are |
Collaborator
|
It occurs because (as I understand it) older versions of unittest.mock (sometimes?) need to dereference the target object via the caller's namespace. Maybe it depends on load order... I dunno for sure, but this patch fixes it: diff --git a/tests/test_ipa_tracking.py b/tests/test_ipa_tracking.py
index 369948f..4333039 100644
--- a/tests/test_ipa_tracking.py
+++ b/tests/test_ipa_tracking.py
@@ -159,7 +159,7 @@ class TestFIPSTokens(BaseTest):
'key-token': 'NSS Certificate DB',
}
- @patch('ipaplatform.tasks.tasks.is_fips_enabled')
+ @patch('ipahealthcheck.ipa.certs.tasks.is_fips_enabled')
@patch('ipalib.install.certmonger.get_request_value')
def test_nonfips_token_correct(self, mock_value, mock_fips):
mock_value.side_effect = [
@@ -182,7 +182,7 @@ class TestFIPSTokens(BaseTest):
result = self.results.results[0]
assert result.result == constants.SUCCESS
- @patch('ipaplatform.tasks.tasks.is_fips_enabled')
+ @patch('ipahealthcheck.ipa.certs.tasks.is_fips_enabled')
@patch('ipalib.install.certmonger.get_request_value')
def test_nonfips_token_wrong(self, mock_value, mock_fips):
mock_value.side_effect = [
@@ -205,7 +205,7 @@ class TestFIPSTokens(BaseTest):
assert result.kw.get('key') == '9876'
assert result.kw.get('token') == 'NSS FIPS 140-2 Certificate DB'
- @patch('ipaplatform.tasks.tasks.is_fips_enabled')
+ @patch('ipahealthcheck.ipa.certs.tasks.is_fips_enabled')
@patch('ipalib.install.certmonger.get_request_value')
def test_fips_token_correct(self, mock_value, mock_fips):
mock_value.side_effect = [
@@ -226,7 +226,7 @@ class TestFIPSTokens(BaseTest):
result = self.results.results[0]
assert result.result == constants.SUCCESS
- @patch('ipaplatform.tasks.tasks.is_fips_enabled')
+ @patch('ipahealthcheck.ipa.certs.tasks.is_fips_enabled')
@patch('ipalib.install.certmonger.get_request_value')
def test_fips_token_wrong(self, mock_value, mock_fips):
mock_value.side_effect = [
@@ -251,7 +251,7 @@ class TestFIPSTokens(BaseTest):
assert result.kw.get('expected_token') == \
'NSS FIPS 140-2 Certificate DB'
- @patch('ipaplatform.tasks.tasks.is_fips_enabled')
+ @patch('ipahealthcheck.ipa.certs.tasks.is_fips_enabled')
@patch('ipalib.install.certmonger.get_request_value')
def test_hsm_token_non_fips(self, mock_value, mock_fips):
"""FIPS shouldn't make a difference as HSM tokens should be skipped"""
@@ -273,7 +273,7 @@ class TestFIPSTokens(BaseTest):
result = self.results.results[0]
assert result.result == constants.SUCCESS
- @patch('ipaplatform.tasks.tasks.is_fips_enabled')
+ @patch('ipahealthcheck.ipa.certs.tasks.is_fips_enabled')
@patch('ipalib.install.certmonger.get_request_value')
def test_hsm_token_fips(self, mock_value, mock_fips):
"""FIPS shouldn't make a difference as HSM tokens should be skipped""" |
rcritten
force-pushed
the
issue_342
branch
2 times, most recently
from
a0d624c to
d22e912
Compare
Collaborator
Author
|
I incorporated your test suggestion. Thanks for doing the leg work to figure that out. |
frasertweedale
approved these changes
Sep 16, 2025
Collaborator
frasertweedale
left a comment
frasertweedale
left a comment
There was a problem hiding this comment.
ACK - one remaining comment for your consideration but not a blocker.
tests/test_ipa_tracking.py
Outdated
Comment on lines
+172
to
+173
| # mock_requests.return_value=get_expected_requests() | ||
| # mock_obj.side_effect=create_mock_dbus |
Collaborator
There was a problem hiding this comment.
Still got this (apparently?) vestigal commented-out code. Not a blocker. Leave it to you whether to remove or not. (Maybe it's there for a reason...)
In order to be FIPS-compliant, and actually be able to renew certificates, the NSS token name needs to match the expectation based on the current FIPS state. The token name will change depending on the state. So if a user installs IPA and then switches to FIPS mode: 1. The system is non-compliant 2. Renewals will fail because the NSS Certificate DB token is not present in the database. Fixes: freeipa#342 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Collaborator
Author
|
Commented lines removed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to be FIPS-compliant, and actually be able to renew certificates, the NSS token name needs to match the expectation based on the current FIPS state. The token name will change depending on the state.
So if a user installs IPA and then switches to FIPS mode:
Fixes: #342
Summary by Sourcery
Add a new healthcheck to verify certmonger NSS token names against the current FIPS state and update tests to cover various token scenarios and HSM skips.
New Features:
Bug Fixes:
Tests: