Check that IPA configuration has MS-PAC generation enabled

[abbra]

FreeIPA uses S4U extensions to operate on behalf of the user requesting IPA API access. S4U2Proxy extension requires presence of MS-PAC record in the Kerberos evidence ticket presented by the proxy service. This is mandatory since ~2023 to address Bronze bit CVE.

Check that 'ipa config-show' output includes 'MS-PAC' in the list of values for ipakrbauthzdata attribute.

[jrisc]

Everything looks good to me.

[rcritten]

I see what you are going for here, making this more future-proof, but the rest of the method is not generic at all. I think we're ok simplifying this to simply check for MS-PAC and if we need to extend it later we can do that.
The way it is now has the side-effect of returning both ERROR and SUCCESS if MS-PAC is not enabled.

[abbra]

Fun thing: I did the change you wanted (and added in the proposal below) but somehow forgot to amend the commit with that before opening the PR.

[rcritten]

Here are my proposed changes plus a couple of simple tests

diff --git a/src/ipahealthcheck/ipa/trust.py b/src/ipahealthcheck/ipa/trust.py
index b07522a..b215cd5 100644
--- a/src/ipahealthcheck/ipa/trust.py
+++ b/src/ipahealthcheck/ipa/trust.py
@@ -695,11 +695,11 @@ class IPAauthzdatapacCheck(IPAPlugin):
     def check(self):
         ipaconfig = api.Command.config_show(raw=True)
         krbauthzdata = ipaconfig['result'].get('ipakrbauthzdata', tuple())
-        for authzdata in ('MS-PAC', ):
-            if authzdata not in krbauthzdata:
-                yield Result(self, constants.ERROR,
-                             key=authzdata,
-                             error='access to IPA API will not work',
-                             msg='MS-PAC generation is not enabled '
-                             'in IPA configuration {key}: {error}')
-        yield Result(self, constants.SUCCESS, key='MS-PAC')
+        if 'MS-PAC' not in krbauthzdata:
+            yield Result(self, constants.ERROR,
+                         key='MS-PAC',
+                         error='access to IPA API will not work',
+                         msg='MS-PAC generation is not enabled '
+                         'in IPA configuration {key}: {error}')
+        else:
+            yield Result(self, constants.SUCCESS, key='MS-PAC')
diff --git a/tests/test_ipa_trust.py b/tests/test_ipa_trust.py
index 6c4754a..4055196 100644
--- a/tests/test_ipa_trust.py
+++ b/tests/test_ipa_trust.py
@@ -23,7 +23,8 @@ from ipahealthcheck.ipa.trust import (IPATrustAgentCheck,
                                       IPATrustControllerGroupSIDCheck,
                                       IPATrustControllerAdminSIDCheck,
                                       IPATrustControllerConfCheck,
-                                      IPATrustPackageCheck)
+                                      IPATrustPackageCheck,
+                                      IPAauthzdatapacCheck)
 
 from ipalib import errors
 from ipapython.dn import DN
@@ -1287,3 +1288,44 @@ class TestPackageCheck(BaseTest):
         assert result.source == 'ipahealthcheck.ipa.trust'
         assert result.check == 'IPATrustPackageCheck'
         sys.modules['ipaserver.install'] = save
+
+
+class TestConfiguration(BaseTest):
+
+    def test_ipakrbauthzdata_positive(self):
+        framework = object()
+        registry.initialize(framework, config.Config)
+        f = IPAauthzdatapacCheck(registry)
+
+        m_api.Command.config_show.side_effect = [{
+            'result': {
+                'ipakrbauthzdata': ['MS-PAC', 'nfs:NONE',]
+            }
+        }]
+        self.results = capture_results(f)
+
+        assert len(self.results) == 1
+
+        result = self.results.results[0]
+        assert result.result == constants.SUCCESS
+        assert result.source == 'ipahealthcheck.ipa.trust'
+        assert result.check == 'IPAauthzdatapacCheck'
+
+    def test_ipakrbauthzdata_negative(self):
+        framework = object()
+        registry.initialize(framework, config.Config)
+        f = IPAauthzdatapacCheck(registry)
+
+        m_api.Command.config_show.side_effect = [{
+            'result': {
+                'ipakrbauthzdata': ['nfs:NONE',]
+            }
+        }]
+        self.results = capture_results(f)
+
+        assert len(self.results) == 1
+
+        result = self.results.results[0]
+        assert result.result == constants.ERROR
+        assert result.source == 'ipahealthcheck.ipa.trust'
+        assert result.check == 'IPAauthzdatapacCheck'

[abbra]

Thank you for the tests! I applied the proposed changes to the same commit.

[rcritten]

Running lint on my local machine wants a space after the trailing comma:

'ipakrbauthzdata': ['MS-PAC', 'nfs:NONE', ]

I don't know why the github CI didn't pick it up. Diff version of python perhaps.

[rcritten]

Same as above, flake wants a space after the comma:

'ipakrbauthzdata': ['nfs:NONE', ]

[rcritten]

Just one minor thing that CI didn't pick up on