diff --git a/SOURCES/0020-Add-support-for-the-DNS-URI-type.patch b/SOURCES/0020-Add-support-for-the-DNS-URI-type.patch new file mode 100644 index 0000000..b5ed6e7 --- /dev/null +++ b/SOURCES/0020-Add-support-for-the-DNS-URI-type.patch @@ -0,0 +1,573 @@ +From 3ae8b103974d7e6aca4e2c5f093e63b67f25a6dd Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Fri, 18 Mar 2022 16:53:20 -0400 +Subject: [PATCH] Add support for the DNS URI type + +URI records are not required but if they exist they are +validated. + +https://github.com/freeipa/freeipa-healthcheck/issues/222 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/ipa/idns.py | 60 ++++++++- + tests/test_ipa_dns.py | 228 +++++++++++++++++++++++++++------ + 2 files changed, 242 insertions(+), 46 deletions(-) + +diff --git a/src/ipahealthcheck/ipa/idns.py b/src/ipahealthcheck/ipa/idns.py +index 4aa7008..64d5ddd 100644 +--- a/src/ipahealthcheck/ipa/idns.py ++++ b/src/ipahealthcheck/ipa/idns.py +@@ -11,12 +11,21 @@ from ipahealthcheck.core.plugin import Result, duration + from ipahealthcheck.core import constants + + from ipalib import api +-from dns import resolver ++from dns.resolver import query as resolve + + + logger = logging.getLogger() + + ++def query_uri(uri): ++ try: ++ answers = resolve(uri, rdatatype.URI) ++ except DNSException as e: ++ logger.debug("DNS record not found: %s", e.__class__.__name__) ++ answers = [] ++ return answers ++ ++ + @registry + class IPADNSSystemRecordsCheck(IPAPlugin): + """ +@@ -32,6 +41,10 @@ class IPADNSSystemRecordsCheck(IPAPlugin): + """Combine the SRV record and target into a unique name.""" + return srv + ":" + target + ++ def uri_to_name(self, uri, target): ++ """Combine the SRV record and target into a unique name.""" ++ return uri + ":" + target ++ + @duration + def check(self): + from ipapython.dnsutil import query_srv +@@ -43,6 +56,7 @@ class IPADNSSystemRecordsCheck(IPAPlugin): + # collect the list of expected values + txt_rec = dict() + srv_rec = dict() ++ uri_rec = dict() + a_rec = list() + aaaa_rec = list() + +@@ -63,6 +77,15 @@ class IPADNSSystemRecordsCheck(IPAPlugin): + a_rec.append(rd.to_text()) + elif rd.rdtype == rdatatype.AAAA: + aaaa_rec.append(rd.to_text()) ++ elif rd.rdtype == rdatatype.URI: ++ if name.ToASCII() in uri_rec: ++ uri_rec[name.ToASCII()].append( ++ rd.target.decode('utf-8') ++ ) ++ else: ++ uri_rec[name.ToASCII()] = [ ++ rd.target.decode('utf-8') ++ ] + else: + logger.error("Unhandler rdtype %d", rd.rdtype) + +@@ -95,10 +118,39 @@ class IPADNSSystemRecordsCheck(IPAPlugin): + msg='Expected SRV record missing', + key=self.srv_to_name(srv, host)) + ++ for uri in uri_rec: ++ logger.debug("Search DNS for URI record of %s", uri) ++ answers = query_uri(uri) ++ hosts = uri_rec[uri] ++ for answer in answers: ++ logger.debug("DNS record found: %s", answer) ++ try: ++ hosts.remove(answer.target.decode('utf-8')) ++ yield Result( ++ self, constants.SUCCESS, ++ key=self.uri_to_name( ++ uri, answer.target.decode('utf-8') ++ ) ++ ) ++ except ValueError: ++ yield Result( ++ self, constants.WARNING, ++ msg='Unexpected URI entry in DNS', ++ key=self.uri_to_name( ++ uri, answer.target.decode('utf-8') ++ ) ++ ) ++ for host in hosts: ++ yield Result( ++ self, constants.WARNING, ++ msg='Expected URI record missing', ++ key=self.uri_to_name(uri, host) ++ ) ++ + for txt in txt_rec: + logger.debug("Search DNS for TXT record of %s", txt) + try: +- answers = resolver.query(txt, rdatatype.TXT) ++ answers = resolve(txt, rdatatype.TXT) + except DNSException as e: + logger.debug("DNS record not found: %s", e.__class__.__name__) + answers = [] +@@ -121,7 +173,7 @@ class IPADNSSystemRecordsCheck(IPAPlugin): + qname = "ipa-ca." + api.env.domain + "." + logger.debug("Search DNS for A record of %s", qname) + try: +- answers = resolver.query(qname, rdatatype.A) ++ answers = resolve(qname, rdatatype.A) + except DNSException as e: + logger.debug("DNS record not found: %s", e.__class__.__name__) + answers = [] +@@ -155,7 +207,7 @@ class IPADNSSystemRecordsCheck(IPAPlugin): + qname = "ipa-ca." + api.env.domain + "." + logger.debug("Search DNS for AAAA record of %s", qname) + try: +- answers = resolver.query(qname, rdatatype.AAAA) ++ answers = resolve(qname, rdatatype.AAAA) + except DNSException as e: + logger.debug("DNS record not found: %s", e.__class__.__name__) + answers = [] +diff --git a/tests/test_ipa_dns.py b/tests/test_ipa_dns.py +index 91b15c2..28243b6 100644 +--- a/tests/test_ipa_dns.py ++++ b/tests/test_ipa_dns.py +@@ -27,6 +27,15 @@ from ipaserver.dns_data_management import ( + IPA_DEFAULT_ADTRUST_SRV_REC + ) + ++try: ++ # pylint: disable=unused-import ++ from ipaserver.dns_data_management import IPA_DEFAULT_MASTER_URI_REC # noqa ++except ImportError: ++ has_uri_support = False ++else: ++ has_uri_support = True ++ ++ + try: + # pylint: disable=unused-import + from ipaserver.install.installutils import resolve_rrsets_nss # noqa: F401 +@@ -79,6 +88,45 @@ def query_srv(qname, ad_records=False): + return rdlist + + ++def query_uri(hosts): ++ """ ++ Return a list containing two answers, one for each uri type ++ """ ++ answers = [] ++ if version.MAJOR < 2 or (version.MAJOR == 2 and version.MINOR == 0): ++ m = message.Message() ++ elif version.MAJOR == 2 and version.MINOR > 0: ++ m = message.QueryMessage() # pylint: disable=E1101 ++ m = message.make_response(m) # pylint: disable=E1101 ++ ++ rdtype = rdatatype.URI ++ for name in ('_kerberos.', '_kpasswd.'): ++ qname = DNSName(name + m_api.env.domain) ++ qname = qname.make_absolute() ++ if version.MAJOR < 2: ++ # pylint: disable=unexpected-keyword-arg ++ answer = Answer(qname, rdataclass.IN, rdtype, m, ++ raise_on_no_answer=False) ++ # pylint: enable=unexpected-keyword-arg ++ else: ++ if version.MAJOR == 2 and version.MINOR > 0: ++ question = rrset.RRset(qname, rdataclass.IN, rdtype) ++ m.question = [question] ++ answer = Answer(qname, rdataclass.IN, rdtype, m) ++ ++ rl = [] ++ for host in hosts: ++ rlist = rrset.from_text_list( ++ qname, 86400, rdataclass.IN, ++ rdatatype.URI, ['0 100 "krb5srv:m:tcp:%s."' % host, ++ '0 100 "krb5srv:m:udp:%s."' % host, ] ++ ) ++ rl.extend(rlist) ++ answer.rrset = rl ++ answers.append(answer) ++ return answers ++ ++ + def gen_addrs(rdtype=rdatatype.A, num=1): + """Generate sequential IP addresses for the ipa-ca A record lookup""" + ips = [] +@@ -188,16 +236,19 @@ class TestDNSSystemRecords(BaseTest): + + 1. The query_srv() override returns the set of configured + servers for each type of SRV record. +- 2. fake_query() overrides dns.resolver.query to simulate ++ 2. fake_query() overrides ipahealthcheck.ipa.idns.resolve to simulate + A, AAAA and TXT record lookups. + """ + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_single(self, mock_query, mock_query_srv, mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_single(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Test single CA master, all SRV records""" + mock_query.side_effect = fake_query_one + mock_query_srv.side_effect = query_srv([m_api.env.host]) ++ mock_query_uri.side_effect = query_uri([m_api.env.host]) + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)) + ] +@@ -219,7 +270,11 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 10 ++ if has_uri_support: ++ expected = 14 ++ else: ++ expected = 10 ++ assert len(self.results) == expected + + for result in self.results.results: + assert result.result == constants.SUCCESS +@@ -228,13 +283,19 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_two(self, mock_query, mock_query_srv, mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_two(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Test two CA masters, all SRV records""" + mock_query_srv.side_effect = query_srv([ + m_api.env.host, + 'replica.' + m_api.env.domain + ]) ++ mock_query_uri.side_effect = query_uri([ ++ m_api.env.host, ++ 'replica.' + m_api.env.domain ++ ]) + mock_query.side_effect = fake_query_two + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)), +@@ -267,7 +328,11 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 19 ++ if has_uri_support: ++ expected = 27 ++ else: ++ expected = 19 ++ assert len(self.results) == expected + + for result in self.results.results: + assert result.result == constants.SUCCESS +@@ -276,14 +341,21 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_three(self, mock_query, mock_query_srv, mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_three(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Test three CA masters, all SRV records""" + mock_query_srv.side_effect = query_srv([ + m_api.env.host, + 'replica.' + m_api.env.domain, + 'replica2.' + m_api.env.domain + ]) ++ mock_query_uri.side_effect = query_uri([ ++ m_api.env.host, ++ 'replica.' + m_api.env.domain, ++ 'replica2.' + m_api.env.domain ++ ]) + mock_query.side_effect = fake_query_three + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)), +@@ -325,7 +397,11 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 28 ++ if has_uri_support: ++ expected = 40 ++ else: ++ expected = 28 ++ assert len(self.results) == expected + + for result in self.results.results: + assert result.result == constants.SUCCESS +@@ -334,15 +410,21 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_three_mixed(self, mock_query, mock_query_srv, +- mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_three_mixed(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Test three masters, only one with a CA, all SRV records""" + mock_query_srv.side_effect = query_srv([ + m_api.env.host, + 'replica.' + m_api.env.domain, + 'replica2.' + m_api.env.domain + ]) ++ mock_query_uri.side_effect = query_uri([ ++ m_api.env.host, ++ 'replica.' + m_api.env.domain, ++ 'replica2.' + m_api.env.domain ++ ]) + mock_query.side_effect = fake_query_one + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)), +@@ -382,7 +464,11 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 24 ++ if has_uri_support: ++ expected = 36 ++ else: ++ expected = 24 ++ assert len(self.results) == expected + + for result in self.results.results: + assert result.result == constants.SUCCESS +@@ -390,9 +476,10 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_missing_server(self, mock_query, mock_query_srv, +- mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_missing_server(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Drop one of the masters from query_srv + + This will simulate missing SRV records and cause a number of +@@ -403,6 +490,11 @@ class TestDNSSystemRecords(BaseTest): + 'replica.' + m_api.env.domain + # replica2 is missing + ]) ++ mock_query_uri.side_effect = query_uri([ ++ m_api.env.host, ++ 'replica.' + m_api.env.domain, ++ 'replica2.' + m_api.env.domain ++ ]) + mock_query.side_effect = fake_query_three + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)), +@@ -444,21 +536,30 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 28 ++ if has_uri_support: ++ expected = 40 ++ else: ++ expected = 28 ++ assert len(self.results) == expected + + ok = get_results_by_severity(self.results.results, constants.SUCCESS) + warn = get_results_by_severity(self.results.results, constants.WARNING) +- assert len(ok) == 21 +- assert len(warn) == 7 ++ if has_uri_support: ++ assert len(ok) == 33 ++ assert len(warn) == 7 ++ else: ++ assert len(ok) == 21 ++ assert len(warn) == 7 + + for result in warn: + assert result.kw.get('msg') == 'Expected SRV record missing' + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_missing_ipa_ca(self, mock_query, mock_query_srv, +- mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_missing_ipa_ca(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Drop one of the masters from query_srv + + This will simulate missing SRV records and cause a number of +@@ -469,6 +570,11 @@ class TestDNSSystemRecords(BaseTest): + 'replica.' + m_api.env.domain, + 'replica2.' + m_api.env.domain + ]) ++ mock_query_uri.side_effect = query_uri([ ++ m_api.env.host, ++ 'replica.' + m_api.env.domain, ++ 'replica2.' + m_api.env.domain ++ ]) + mock_query.side_effect = fake_query_two + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)), +@@ -510,12 +616,20 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 28 ++ if has_uri_support: ++ expected = 40 ++ else: ++ expected = 28 ++ assert len(self.results) == expected + + ok = get_results_by_severity(self.results.results, constants.SUCCESS) + warn = get_results_by_severity(self.results.results, constants.WARNING) +- assert len(ok) == 26 +- assert len(warn) == 2 ++ if has_uri_support: ++ assert len(ok) == 38 ++ assert len(warn) == 2 ++ else: ++ assert len(ok) == 26 ++ assert len(warn) == 2 + + for result in warn: + assert re.match( +@@ -527,9 +641,10 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_extra_srv(self, mock_query, mock_query_srv, +- mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_extra_srv(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """An extra SRV record set exists, report it. + + Add an extra master to the query_srv() which will generate +@@ -541,6 +656,11 @@ class TestDNSSystemRecords(BaseTest): + 'replica2.' + m_api.env.domain, + 'replica3.' + m_api.env.domain + ]) ++ mock_query_uri.side_effect = query_uri([ ++ m_api.env.host, ++ 'replica.' + m_api.env.domain, ++ 'replica2.' + m_api.env.domain, ++ ]) + mock_query.side_effect = fake_query_three + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)), +@@ -584,12 +704,20 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 35 ++ if has_uri_support: ++ expected = 47 ++ else: ++ expected = 35 ++ assert len(self.results) == expected + + ok = get_results_by_severity(self.results.results, constants.SUCCESS) + warn = get_results_by_severity(self.results.results, constants.WARNING) +- assert len(ok) == 28 +- assert len(warn) == 7 ++ if has_uri_support: ++ assert len(ok) == 40 ++ assert len(warn) == 7 ++ else: ++ assert len(ok) == 28 ++ assert len(warn) == 7 + + for result in warn: + assert result.kw.get('msg') == \ +@@ -597,12 +725,14 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_bad_realm(self, mock_query, mock_query_srv, +- mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_bad_realm(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + """Unexpected Kerberos TXT record""" + mock_query.side_effect = fake_query_one_txt + mock_query_srv.side_effect = query_srv([m_api.env.host]) ++ mock_query_uri.side_effect = query_uri([m_api.env.host]) + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)) + ] +@@ -624,12 +754,20 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 10 ++ if has_uri_support: ++ expected = 14 ++ else: ++ expected = 10 ++ assert len(self.results) == expected + + ok = get_results_by_severity(self.results.results, constants.SUCCESS) + warn = get_results_by_severity(self.results.results, constants.WARNING) +- assert len(ok) == 9 +- assert len(warn) == 1 ++ if has_uri_support: ++ assert len(ok) == 13 ++ assert len(warn) == 1 ++ else: ++ assert len(ok) == 9 ++ assert len(warn) == 1 + + result = warn[0] + assert result.kw.get('msg') == 'expected realm missing' +@@ -637,11 +775,13 @@ class TestDNSSystemRecords(BaseTest): + + @patch(resolve_rrsets_import) + @patch('ipapython.dnsutil.query_srv') +- @patch('dns.resolver.query') +- def test_dnsrecords_one_with_ad(self, mock_query, mock_query_srv, +- mock_rrset): ++ @patch('ipahealthcheck.ipa.idns.query_uri') ++ @patch('ipahealthcheck.ipa.idns.resolve') ++ def test_dnsrecords_one_with_ad(self, mock_query, mock_query_uri, ++ mock_query_srv, mock_rrset): + mock_query.side_effect = fake_query_one + mock_query_srv.side_effect = query_srv([m_api.env.host], True) ++ mock_query_uri.side_effect = query_uri([m_api.env.host]) + mock_rrset.side_effect = [ + resolve_rrsets(m_api.env.host, (rdatatype.A, rdatatype.AAAA)) + ] +@@ -664,7 +804,11 @@ class TestDNSSystemRecords(BaseTest): + + self.results = capture_results(f) + +- assert len(self.results) == 16 ++ if has_uri_support: ++ expected = 20 ++ else: ++ expected = 16 ++ assert len(self.results) == expected + + for result in self.results.results: + assert result.result == constants.SUCCESS +-- +2.31.1 + diff --git a/SOURCES/0021-Use-the-subject-base-from-the-IPA-configuration-not-.patch b/SOURCES/0021-Use-the-subject-base-from-the-IPA-configuration-not-.patch new file mode 100644 index 0000000..c53c901 --- /dev/null +++ b/SOURCES/0021-Use-the-subject-base-from-the-IPA-configuration-not-.patch @@ -0,0 +1,372 @@ +From ed5322daad5dc456e4958228835b33a32c7d1608 Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Tue, 29 Mar 2022 12:58:01 -0400 +Subject: [PATCH] Use the subject base from the IPA configuration, not REALM + +The expected certificates were hardcoded with O={REALM} which +would return false-positives if the customer defined their +own certificate subject base. + +Also add a search filter to only retrieve the certificate(s) we +want to examine rather than the entire contents. + +Fixes: https://github.com/freeipa/freeipa-healthcheck/issues/253 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/ipa/certs.py | 24 ++-- + tests/test_ipa_cert_match.py | 202 ++++++++++++++++++++++++-------- + 2 files changed, 166 insertions(+), 60 deletions(-) + +diff --git a/src/ipahealthcheck/ipa/certs.py b/src/ipahealthcheck/ipa/certs.py +index 2ef33f9..fd5e180 100644 +--- a/src/ipahealthcheck/ipa/certs.py ++++ b/src/ipahealthcheck/ipa/certs.py +@@ -707,12 +707,14 @@ class IPADogtagCertsMatchCheck(IPAPlugin): + + def match_ldap_nss_certs_by_subject(plugin, ldap, db, dn, + expected_nicks_subjects): +- entries = ldap.get_entries(dn) + all_ok = True + for nick, subject in expected_nicks_subjects.items(): ++ entries = ldap.get_entries( ++ dn, ++ filter=f'subjectname={subject}' ++ ) + cert = db.get_cert_from_db(nick) +- ok = any([cert in entry['userCertificate'] and +- subject == entry['subjectName'][0] ++ ok = any([cert in entry['userCertificate'] + for entry in entries + if 'userCertificate' in entry]) + if not ok: +@@ -740,26 +742,28 @@ class IPADogtagCertsMatchCheck(IPAPlugin): + db, dn, 'CACertificate', + casigning_nick) + ++ config = api.Command.config_show() ++ subject_base = config['result']['ipacertificatesubjectbase'][0] + expected_nicks_subjects = { + 'ocspSigningCert cert-pki-ca': +- 'CN=OCSP Subsystem,O=%s' % api.env.realm, ++ f'CN=OCSP Subsystem,{subject_base}', + 'subsystemCert cert-pki-ca': +- 'CN=CA Subsystem,O=%s' % api.env.realm, ++ f'CN=CA Subsystem,{subject_base}', + 'auditSigningCert cert-pki-ca': +- 'CN=CA Audit,O=%s' % api.env.realm, ++ f'CN=CA Audit,{subject_base}', + 'Server-Cert cert-pki-ca': +- 'CN=%s,O=%s' % (api.env.host, api.env.realm), ++ f'CN={api.env.host},{subject_base}', + } + + kra = krainstance.KRAInstance(api.env.realm) + if kra.is_installed(): + kra_expected_nicks_subjects = { + 'transportCert cert-pki-kra': +- 'CN=KRA Transport Certificate,O=%s' % api.env.realm, ++ f'CN=KRA Transport Certificate,{subject_base}', + 'storageCert cert-pki-kra': +- 'CN=KRA Storage Certificate,O=%s' % api.env.realm, ++ f'CN=KRA Storage Certificate,{subject_base}', + 'auditSigningCert cert-pki-kra': +- 'CN=KRA Audit,O=%s' % api.env.realm, ++ f'CN=KRA Audit,{subject_base}', + } + expected_nicks_subjects.update(kra_expected_nicks_subjects) + +diff --git a/tests/test_ipa_cert_match.py b/tests/test_ipa_cert_match.py +index 460e61a..70ef59e 100644 +--- a/tests/test_ipa_cert_match.py ++++ b/tests/test_ipa_cert_match.py +@@ -44,8 +44,15 @@ class mock_ldap: + def get_entries(self, base_dn, scope=SCOPE_SUBTREE, filter=None, + attrs_list=None, get_effective_rights=False, **kwargs): + if self.results is None: +- raise errors.NotFound(reason='test') +- return self.results.values() ++ raise errors.NotFound(reason='None') ++ if filter: ++ (attr, value) = filter.split('=', maxsplit=1) ++ for result in self.results.values(): ++ if result.get(attr)[0] == value: ++ return [result] ++ raise errors.NotFound(reason='Not found %s' % filter) ++ ++ return self.results + + + class mock_ldap_conn: +@@ -82,6 +89,10 @@ class TestIPACertMatch(BaseTest): + Mock(return_value=mock_ldap_conn()) + } + ++ trust = { ++ ('%s IPA CA' % m_api.env.realm): 'u,u,u' ++ } ++ + @patch('ipalib.x509.load_certificate_list_from_file') + @patch('ipaserver.install.certs.CertDB') + def test_certs_match_ok(self, mock_certdb, mock_load_cert): +@@ -92,11 +103,8 @@ class TestIPACertMatch(BaseTest): + 'cn=certificates,cn=ipa,cn=etc', + m_api.env.basedn), + CACertificate=[IPACertificate()]) +- trust = { +- ('%s IPA CA' % m_api.env.realm): 'u,u,u' +- } + +- mock_certdb.return_value = mock_CertDB(trust) ++ mock_certdb.return_value = mock_CertDB(self.trust) + mock_load_cert.return_value = [IPACertificate()] + + framework = object() +@@ -121,11 +129,8 @@ class TestIPACertMatch(BaseTest): + 'cn=certificates,cn=ipa,cn=etc', + m_api.env.basedn), + CACertificate=[IPACertificate()]) +- trust = { +- ('%s IPA CA' % m_api.env.realm): 'u,u,u' +- } + +- mock_certdb.return_value = mock_CertDB(trust) ++ mock_certdb.return_value = mock_CertDB(self.trust) + mock_load_cert.return_value = [IPACertificate(serial_number=2)] + + framework = object() +@@ -155,15 +160,54 @@ class TestIPACertMatch(BaseTest): + assert len(self.results) == 0 + + ++default_subject_base = [{ ++ 'result': ++ { ++ 'ipacertificatesubjectbase': [f'O={m_api.env.realm}'], ++ }, ++}] ++ ++custom_subject_base = [{ ++ 'result': ++ { ++ 'ipacertificatesubjectbase': ['OU=Eng,O=ACME'], ++ }, ++}] ++ ++ + class TestIPADogtagCertMatch(BaseTest): + patches = { + 'ipaserver.install.krainstance.KRAInstance': + Mock(return_value=KRAInstance()), + } ++ trust = { ++ 'ocspSigningCert cert-pki-ca': 'u,u,u', ++ 'caSigningCert cert-pki-ca': 'u,u,u', ++ 'subsystemCert cert-pki-ca': 'u,u,u', ++ 'auditSigningCert cert-pki-ca': 'u,u,Pu', ++ 'Server-Cert cert-pki-ca': 'u,u,u', ++ 'transportCert cert-pki-kra': 'u,u,u', ++ 'storageCert cert-pki-kra': 'u,u,u', ++ 'auditSigningCert cert-pki-kra': 'u,u,Pu', ++ } ++ ++ def get_dogtag_subjects(self, hostname, base): ++ subject_base = base[0]['result']['ipacertificatesubjectbase'][0] ++ return ( ++ f'CN=OCSP Subsystem,{subject_base}', ++ f'CN=CA Subsystem,{subject_base}', ++ f'CN=CA Audit,{subject_base}', ++ f'CN=%s,{subject_base}', ++ f'CN=KRA Transport Certificate,{subject_base}', ++ f'CN=KRA Storage Certificate,{subject_base}', ++ f'CN=KRA Audit,{subject_base}', ++ f'CN={hostname},{subject_base}', ++ ) + + @patch('ipaserver.install.certs.CertDB') + def test_certs_match_ok(self, mock_certdb): + """ Ensure match check is ok""" ++ m_api.Command.config_show.side_effect = default_subject_base + fake_conn = LDAPClient('ldap://localhost', no_schema=True) + pkidbentry = LDAPEntry(fake_conn, + DN('uid=pkidbuser,ou=people,o=ipaca'), +@@ -177,25 +221,9 @@ class TestIPADogtagCertMatch(BaseTest): + userCertificate=[IPACertificate()], + subjectName=['test']) + ldap_entries = [pkidbentry, casignentry] +- trust = { +- 'ocspSigningCert cert-pki-ca': 'u,u,u', +- 'caSigningCert cert-pki-ca': 'u,u,u', +- 'subsystemCert cert-pki-ca': 'u,u,u', +- 'auditSigningCert cert-pki-ca': 'u,u,Pu', +- 'Server-Cert cert-pki-ca': 'u,u,u', +- 'transportCert cert-pki-kra': 'u,u,u', +- 'storageCert cert-pki-kra': 'u,u,u', +- 'auditSigningCert cert-pki-kra': 'u,u,Pu', +- } +- +- dogtag_entries_subjects = ( +- 'CN=OCSP Subsystem,O=%s' % m_api.env.realm, +- 'CN=CA Subsystem,O=%s' % m_api.env.realm, +- 'CN=CA Audit,O=%s' % m_api.env.realm, +- 'CN=%s,O=%s' % (m_api.env.host, m_api.env.realm), +- 'CN=KRA Transport Certificate,O=%s' % m_api.env.realm, +- 'CN=KRA Storage Certificate,O=%s' % m_api.env.realm, +- 'CN=KRA Audit,O=%s' % m_api.env.realm, ++ ++ dogtag_entries_subjects = self.get_dogtag_subjects( ++ m_api.env.host, default_subject_base + ) + + for i, subject in enumerate(dogtag_entries_subjects): +@@ -206,7 +234,7 @@ class TestIPADogtagCertMatch(BaseTest): + subjectName=[subject]) + ldap_entries.append(entry) + +- mock_certdb.return_value = mock_CertDB(trust) ++ mock_certdb.return_value = mock_CertDB(self.trust) + + framework = object() + registry.initialize(framework, config.Config()) +@@ -223,6 +251,7 @@ class TestIPADogtagCertMatch(BaseTest): + @patch('ipaserver.install.certs.CertDB') + def test_certs_mismatch(self, mock_certdb): + """ Ensure mismatches are detected""" ++ m_api.Command.config_show.side_effect = default_subject_base + fake_conn = LDAPClient('ldap://localhost', no_schema=True) + pkidbentry = LDAPEntry(fake_conn, + DN('uid=pkidbuser,ou=people,o=ipaca'), +@@ -238,25 +267,9 @@ class TestIPADogtagCertMatch(BaseTest): + userCertificate=[IPACertificate()], + subjectName=['test']) + ldap_entries = [pkidbentry, casignentry] +- trust = { +- 'ocspSigningCert cert-pki-ca': 'u,u,u', +- 'caSigningCert cert-pki-ca': 'u,u,u', +- 'subsystemCert cert-pki-ca': 'u,u,u', +- 'auditSigningCert cert-pki-ca': 'u,u,Pu', +- 'Server-Cert cert-pki-ca': 'u,u,u', +- 'transportCert cert-pki-kra': 'u,u,u', +- 'storageCert cert-pki-kra': 'u,u,u', +- 'auditSigningCert cert-pki-kra': 'u,u,Pu', +- } +- +- dogtag_entries_subjects = ( +- 'CN=OCSP Subsystem,O=%s' % m_api.env.realm, +- 'CN=CA Subsystem,O=%s' % m_api.env.realm, +- 'CN=CA Audit,O=%s' % m_api.env.realm, +- 'CN=%s,O=%s' % (m_api.env.host, m_api.env.realm), +- 'CN=KRA Transport Certificate,O=%s' % m_api.env.realm, +- 'CN=KRA Storage Certificate,O=%s' % m_api.env.realm, +- 'CN=KRA Audit,O=%s' % m_api.env.realm, ++ ++ dogtag_entries_subjects = self.get_dogtag_subjects( ++ m_api.env.host, default_subject_base + ) + + for i, subject in enumerate(dogtag_entries_subjects): +@@ -267,7 +280,7 @@ class TestIPADogtagCertMatch(BaseTest): + subjectName=[subject]) + ldap_entries.append(entry) + +- mock_certdb.return_value = mock_CertDB(trust) ++ mock_certdb.return_value = mock_CertDB(self.trust) + + framework = object() + registry.initialize(framework, config.Config()) +@@ -280,3 +293,92 @@ class TestIPADogtagCertMatch(BaseTest): + assert result.result == constants.ERROR + assert result.source == 'ipahealthcheck.ipa.certs' + assert result.check == 'IPADogtagCertsMatchCheck' ++ ++ @patch('ipaserver.install.certs.CertDB') ++ def test_certs_match_ok_subject(self, mock_certdb): ++ """ Ensure match check is ok""" ++ m_api.Command.config_show.side_effect = custom_subject_base ++ fake_conn = LDAPClient('ldap://localhost', no_schema=True) ++ pkidbentry = LDAPEntry(fake_conn, ++ DN('uid=pkidbuser,ou=people,o=ipaca'), ++ userCertificate=[IPACertificate()], ++ subjectName=['test']) ++ casignentry = LDAPEntry(fake_conn, ++ DN('cn=%s IPA CA' % m_api.env.realm, ++ 'cn=certificates,cn=ipa,cn=etc', ++ m_api.env.basedn), ++ CACertificate=[IPACertificate()], ++ userCertificate=[IPACertificate()], ++ subjectName=['test']) ++ ldap_entries = [pkidbentry, casignentry] ++ ++ dogtag_entries_subjects = self.get_dogtag_subjects( ++ m_api.env.host, custom_subject_base ++ ) ++ ++ for i, subject in enumerate(dogtag_entries_subjects): ++ entry = LDAPEntry(fake_conn, ++ DN('cn=%i,ou=certificateRepository' % i, ++ 'ou=ca,o=ipaca'), ++ userCertificate=[IPACertificate()], ++ subjectName=[subject]) ++ ldap_entries.append(entry) ++ ++ mock_certdb.return_value = mock_CertDB(self.trust) ++ ++ framework = object() ++ registry.initialize(framework, config.Config()) ++ f = IPADogtagCertsMatchCheck(registry) ++ f.conn = mock_ldap(ldap_entries) ++ self.results = capture_results(f) ++ ++ assert len(self.results) == 3 ++ for result in self.results.results: ++ assert result.result == constants.SUCCESS ++ assert result.source == 'ipahealthcheck.ipa.certs' ++ assert result.check == 'IPADogtagCertsMatchCheck' ++ ++ @patch('ipaserver.install.certs.CertDB') ++ def test_certs_mismatch_subject(self, mock_certdb): ++ """ Ensure mismatches are detected""" ++ m_api.Command.config_show.side_effect = custom_subject_base ++ fake_conn = LDAPClient('ldap://localhost', no_schema=True) ++ pkidbentry = LDAPEntry(fake_conn, ++ DN('uid=pkidbuser,ou=people,o=ipaca'), ++ userCertificate=[IPACertificate( ++ serial_number=2 ++ )], ++ subjectName=['test']) ++ casignentry = LDAPEntry(fake_conn, ++ DN('cn=%s IPA CA' % m_api.env.realm, ++ 'cn=certificates,cn=ipa,cn=etc', ++ m_api.env.basedn), ++ CACertificate=[IPACertificate()], ++ userCertificate=[IPACertificate()], ++ subjectName=['test']) ++ ldap_entries = [pkidbentry, casignentry] ++ ++ dogtag_entries_subjects = self.get_dogtag_subjects( ++ m_api.env.host, custom_subject_base ++ ) ++ ++ for i, subject in enumerate(dogtag_entries_subjects): ++ entry = LDAPEntry(fake_conn, ++ DN('cn=%i,ou=certificateRepository' % i, ++ 'ou=ca,o=ipaca'), ++ userCertificate=[IPACertificate()], ++ subjectName=[subject]) ++ ldap_entries.append(entry) ++ ++ mock_certdb.return_value = mock_CertDB(self.trust) ++ ++ framework = object() ++ registry.initialize(framework, config.Config()) ++ f = IPADogtagCertsMatchCheck(registry) ++ f.conn = mock_ldap(ldap_entries) ++ self.results = capture_results(f) ++ ++ assert len(self.results) == 3 ++ result = self.results.results[0] ++ assert result.result == constants.ERROR ++ assert result.source == 'ipahealthcheck.ipa.certs' +-- +2.31.1 + diff --git a/SOURCES/0022-Allow-multiple-file-modes-in-the-FileChecker.patch b/SOURCES/0022-Allow-multiple-file-modes-in-the-FileChecker.patch new file mode 100644 index 0000000..a300f3e --- /dev/null +++ b/SOURCES/0022-Allow-multiple-file-modes-in-the-FileChecker.patch @@ -0,0 +1,178 @@ +From 96a780d75274031fd97167c1109fcb63efa7d5cd Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Tue, 5 Apr 2022 09:25:45 -0400 +Subject: [PATCH] Allow multiple file modes in the FileChecker + +There are some cases where a strict file mode is not +necessary. The kadmind.log is one. + +It is owned root:root so there is no real difference +between 0600 and 0640. So allow both. + +https://bugzilla.redhat.com/show_bug.cgi?id=2058239 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/core/files.py | 31 +++++++++++++++++++++++-------- + src/ipahealthcheck/ipa/files.py | 3 ++- + tests/test_core_files.py | 32 ++++++++++++++++++++++++++++---- + 3 files changed, 53 insertions(+), 13 deletions(-) + +diff --git a/src/ipahealthcheck/core/files.py b/src/ipahealthcheck/core/files.py +index 1b208d1..af03243 100644 +--- a/src/ipahealthcheck/core/files.py ++++ b/src/ipahealthcheck/core/files.py +@@ -16,13 +16,15 @@ class FileCheck: + files is a tuple of tuples. Each tuple consists of: + (path, expected_perm, expected_owner, expected_group) + +- perm is in the form of a POSIX ACL: e.g. 0440, 0770. ++ perm is a POSIX ACL as either a string or tuple: e.g. 0440, (0770,). + Owner and group are names, not uid/gid. + """ + + @duration + def check(self): + for (path, owner, group, mode) in self.files: ++ if not isinstance(mode, tuple): ++ mode = tuple((mode,)) + if not os.path.exists(path): + for type in ('mode', 'owner', 'group'): + key = '%s_%s' % (path.replace('/', '_'), type) +@@ -33,19 +35,32 @@ class FileCheck: + stat = os.stat(path) + fmode = str(oct(stat.st_mode)[-4:]) + key = '%s_mode' % path.replace('/', '_') +- if mode != fmode: +- if mode < fmode: ++ if fmode not in mode: ++ if len(mode) == 1: ++ modes = mode[0] ++ else: ++ modes = 'one of {}'.format(','.join(mode)) ++ if all(m < fmode for m in mode): + yield Result(self, constants.WARNING, key=key, +- path=path, type='mode', expected=mode, ++ path=path, type='mode', expected=modes, + got=fmode, + msg='Permissions of %s are too permissive: ' +- '%s and should be %s' % (path, fmode, mode)) +- if mode > fmode: ++ '%s and should be %s' % ++ (path, fmode, modes)) ++ elif all(m > fmode for m in mode): + yield Result(self, constants.ERROR, key=key, +- path=path, type='mode', expected=mode, ++ path=path, type='mode', expected=modes, + got=fmode, + msg='Permissions of %s are too restrictive: ' +- '%s and should be %s' % (path, fmode, mode)) ++ '%s and should be %s' % ++ (path, fmode, modes)) ++ else: ++ yield Result(self, constants.ERROR, key=key, ++ path=path, type='mode', expected=modes, ++ got=fmode, ++ msg='Permissions of %s are unexpected: ' ++ '%s and should be %s' % ++ (path, fmode, modes)) + else: + yield Result(self, constants.SUCCESS, key=key, + type='mode', path=path) +diff --git a/src/ipahealthcheck/ipa/files.py b/src/ipahealthcheck/ipa/files.py +index abfa52f..a372992 100644 +--- a/src/ipahealthcheck/ipa/files.py ++++ b/src/ipahealthcheck/ipa/files.py +@@ -118,7 +118,8 @@ class IPAFileCheck(IPAPlugin, FileCheck): + self.files.append((paths.IPA_CUSTODIA_AUDIT_LOG, + 'root', 'root', '0644')) + +- self.files.append((paths.KADMIND_LOG, 'root', 'root', '0600')) ++ self.files.append((paths.KADMIND_LOG, 'root', 'root', ++ ('0600', '0640'))) + self.files.append((paths.KRB5KDC_LOG, 'root', 'root', '0640')) + + inst = api.env.realm.replace('.', '-') +diff --git a/tests/test_core_files.py b/tests/test_core_files.py +index 8257f40..509e28e 100644 +--- a/tests/test_core_files.py ++++ b/tests/test_core_files.py +@@ -15,7 +15,8 @@ nobody = pwd.getpwnam('nobody') + + # Mock files to test + files = (('foo', 'root', 'root', '0660'), +- ('bar', 'nobody', 'nobody', '0664'),) ++ ('bar', 'nobody', 'nobody', '0664'), ++ ('zap', 'root', 'root', ('0664', '0640'),)) + + + def make_stat(mode=33200, uid=0, gid=0): +@@ -25,6 +26,13 @@ def make_stat(mode=33200, uid=0, gid=0): + mode = 0660 + owner = root + group = root ++ ++ Cheat sheet equivalents: ++ 0600 = 33152 ++ 0640 = 33184 ++ 0644 = 33188 ++ 0660 = 33200 ++ 0666 = 33206 + """ + return posix.stat_result((mode, 1, 42, 1, uid, gid, 0, 1, 1, 1,)) + +@@ -80,6 +88,11 @@ def test_files_group(mock_stat): + assert my_results.results[0].result == constants.WARNING + assert my_results.results[1].result == constants.SUCCESS + ++ assert my_results.results[2].result == constants.WARNING ++ assert my_results.results[2].kw.get('got') == 'nobody' ++ assert my_results.results[2].kw.get('expected') == 'root' ++ assert my_results.results[2].kw.get('type') == 'group' ++ + + @patch('os.stat') + def test_files_mode(mock_stat): +@@ -94,17 +107,28 @@ def test_files_mode(mock_stat): + assert my_results.results[0].result == constants.SUCCESS + assert my_results.results[1].result == constants.ERROR + +- mock_stat.return_value = make_stat(mode=33152) ++ mock_stat.return_value = make_stat(mode=33152) # 0600 + results = capture_results(f) + my_results = get_results(results, 'mode') + assert my_results.results[0].result == constants.ERROR + assert my_results.results[1].result == constants.ERROR ++ assert my_results.results[2].result == constants.ERROR + +- mock_stat.return_value = make_stat(mode=33206) ++ # Too restrictive ++ mock_stat.return_value = make_stat(mode=33206) # 0666 + results = capture_results(f) + my_results = get_results(results, 'mode') + assert my_results.results[0].result == constants.WARNING + assert my_results.results[1].result == constants.WARNING ++ assert my_results.results[2].result == constants.WARNING ++ ++ # Too restrictive with allowed multi-mode value ++ mock_stat.return_value = make_stat(mode=33184) # 0640 ++ results = capture_results(f) ++ my_results = get_results(results, 'mode') ++ assert my_results.results[0].result == constants.ERROR ++ assert my_results.results[1].result == constants.ERROR ++ assert my_results.results[2].result == constants.SUCCESS + + + @patch('os.path.exists') +@@ -118,7 +142,7 @@ def test_files_not_found(mock_exists): + + for type in ('mode', 'group', 'owner'): + my_results = get_results(results, type) +- assert len(my_results.results) == 2 ++ assert len(my_results.results) == len(f.files) + for result in my_results.results: + assert result.result == constants.SUCCESS + assert result.kw.get('msg') == 'File does not exist' +-- +2.31.1 + diff --git a/SOURCES/0023-Unify-command-line-options-and-configuration.patch b/SOURCES/0023-Unify-command-line-options-and-configuration.patch new file mode 100644 index 0000000..1388a72 --- /dev/null +++ b/SOURCES/0023-Unify-command-line-options-and-configuration.patch @@ -0,0 +1,245 @@ +From 71221179de4c5694a0032cc667891322d5016ee3 Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Tue Aug 31 15:55:04 2021 -0400 +Subject: Unify command-line options and configuration + +This makes it possible to add command-line options to the +configuration file. + +The config file is loaded then the command-line options are +merged in. The first one option set takes precedence. So if +an option, say output_type, is in the configuration file then +passing output-type on the command-line will not override it. +The workaround is to pass --config= to ipa-healthcheck in order +to not load the configuration file. + +This will allow for greater flexibility when running this automatically +without having to manually change test timer scripting directly. + +https://bugzilla.redhat.com/show_bug.cgi?id=1872467 + +Signed-off-by: Rob Crittenden +--- + man/man5/ipahealthcheck.conf.5 | 10 ++++++- + man/man8/ipa-healthcheck.8 | 3 +++ + src/ipahealthcheck/core/config.py | 2 +- + src/ipahealthcheck/core/core.py | 22 +++++++++++---- + src/ipahealthcheck/core/output.py | 4 +-- + tests/test_init.py | 6 ++--- + tests/test_options.py | 45 +++++++++++++++++++++++++++++++ + 7 files changed, 80 insertions(+), 12 deletions(-) + create mode 100644 tests/test_options.py + +diff --git a/man/man5/ipahealthcheck.conf.5 b/man/man5/ipahealthcheck.conf.5 +index 50d5506..8ff83c9 100644 +--- a/man/man5/ipahealthcheck.conf.5 ++++ b/man/man5/ipahealthcheck.conf.5 +@@ -36,6 +36,14 @@ The following options are relevant for the server: + .TP + .B cert_expiration_days\fR + The number of days left before a certificate expires to start displaying a warning. The default is 28. ++.TP ++All command\-line options may be included in the configuration file. Dashes must be converted to underscore for the configuration file, e.g. \-\-output\-type becomes output_type. All options, including those that don't make sense in a config file, like \-\-list\-sources, are allowed. Let the buyer beware. ++.TP ++The purpose of allowing command\-line options to be in the configuration file is for automation without having to tweak the automation script. For example, if you want the default output type to be human for the systemd timer automated runs, settting output_type=human in the configuration file will do this. When loading configuration the first option wins, so if any option is in the configuration file then it cannot be overridden by the command-line unless a different configuration file is specified (see \-\-config). ++.TP ++There may be conflicting exceptions. For example, if all=True is set in the configuration file, and the command\-line contains \-\-failures\-only, then only failures will be displayed because of the way the option evaluation is done. ++.TP ++Options that don't make sense for the configuration file include \-\-list\-sources and \-\-input\-file. + .SH "FILES" + .TP + .I /etc/ipahealthcheck/ipahealthcheck.conf +@@ -49,4 +57,4 @@ configuration file + cert_expiration_days=7 + + .SH "SEE ALSO" +-.BR ipa-healthcheck (8) ++.BR ipa\-healthcheck (8) +diff --git a/man/man8/ipa-healthcheck.8 b/man/man8/ipa-healthcheck.8 +index 9c97cfe..148a08b 100644 +--- a/man/man8/ipa-healthcheck.8 ++++ b/man/man8/ipa-healthcheck.8 +@@ -30,6 +30,9 @@ Display a list of the available sources and the checks associated with those sou + + .SS "OPTIONAL ARGUMENTS" + .TP ++\fB\-\-config\fR=\fIFILE\fR ++The configuration file to use. If an empty string is passed in then no configuration file is loaded. The default is /etc/ipahealthcheck/ipahealthcheck.conf. ++.TP + \fB\-\-source\fR=\fISOURCE\fR + Execute checks within the named source, or all sources in the given namespace. + .TP +diff --git a/src/ipahealthcheck/core/config.py b/src/ipahealthcheck/core/config.py +index 5b41cc3..14ae27f 100644 +--- a/src/ipahealthcheck/core/config.py ++++ b/src/ipahealthcheck/core/config.py +@@ -63,7 +63,7 @@ class Config: + """ + Merge variables from dict ``d`` into the configuration + +- The last one wins. ++ The first one wins. + + :param d: dict containing configuration + """ +diff --git a/src/ipahealthcheck/core/core.py b/src/ipahealthcheck/core/core.py +index 19f7818..98ca1d2 100644 +--- a/src/ipahealthcheck/core/core.py ++++ b/src/ipahealthcheck/core/core.py +@@ -160,6 +160,8 @@ def list_sources(plugins): + def add_default_options(parser, output_registry, default_output): + output_names = [plugin.__name__.lower() for + plugin in output_registry.plugins] ++ parser.add_argument('--config', dest='config', ++ default=None, help='Config file to load') + parser.add_argument('--verbose', dest='verbose', action='store_true', + default=False, help='Run in verbose mode') + parser.add_argument('--debug', dest='debug', action='store_true', +@@ -173,9 +175,10 @@ def add_default_options(parser, output_registry, default_output): + parser.add_argument('--check', dest='check', + default=None, + help='Check to execute, e.g. BazCheck') +- parser.add_argument('--output-type', dest='output', choices=output_names, ++ parser.add_argument('--output-type', dest='output_type', ++ choices=output_names, + default=default_output, help='Output method') +- parser.add_argument('--output-file', dest='outfile', default=None, ++ parser.add_argument('--output-file', dest='output_file', default=None, + help='File to store output') + + +@@ -262,7 +265,6 @@ class RunChecks: + add_output_options(self.parser, self.output_registry) + self.add_options() + options = parse_options(self.parser) +- self.options = options + rval = self.validate_options() + if rval: + return rval +@@ -273,10 +275,20 @@ class RunChecks: + if options.debug: + logger.setLevel(logging.DEBUG) + +- config = read_config(self.configfile) ++ if options.config is not None: ++ config = read_config(options.config) ++ else: ++ config = read_config(self.configfile) + if config is None: + return 1 + ++ # Unify config and options. One of these variables will be ++ # eventually deprecated in the future. This way all cli ++ # options can be set in config instead. ++ config.merge(vars(options)) ++ self.options = config ++ options = config ++ + rval = self.pre_check() + if rval is not None: + return rval +@@ -319,7 +331,7 @@ class RunChecks: + plugins.append(plugin) + + for out in self.output_registry.plugins: +- if out.__name__.lower() == options.output: ++ if out.__name__.lower() == options.output_type: + output = out(options) + break + +diff --git a/src/ipahealthcheck/core/output.py b/src/ipahealthcheck/core/output.py +index 784263d..61dffbe 100644 +--- a/src/ipahealthcheck/core/output.py ++++ b/src/ipahealthcheck/core/output.py +@@ -36,7 +36,7 @@ class Output: + which will render the results into a string for writing. + """ + def __init__(self, options): +- self.filename = options.outfile ++ self.filename = options.output_file + + # Non-required options in the framework, set logical defaults to + # pre 0.6 behavior with everything reported. +@@ -110,7 +110,7 @@ class JSON(Output): + + def __init__(self, options): + super(JSON, self).__init__(options) +- self.indent = options.indent ++ self.indent = int(options.indent) + + def generate(self, data): + output = json.dumps(data, indent=self.indent) +diff --git a/tests/test_init.py b/tests/test_init.py +index e18e03c..5f9e3e2 100644 +--- a/tests/test_init.py ++++ b/tests/test_init.py +@@ -10,12 +10,12 @@ from ipahealthcheck.core.output import output_registry + class RunChecks: + def run_healthcheck(self): + options = argparse.Namespace(check=None, debug=False, indent=2, +- list_sources=False, outfile=None, +- output='json', source=None, ++ list_sources=False, output_file=None, ++ output_type='json', source=None, + verbose=False) + + for out in output_registry.plugins: +- if out.__name__.lower() == options.output: ++ if out.__name__.lower() == options.output_type: + out(options) + break + +diff --git a/tests/test_options.py b/tests/test_options.py +new file mode 100644 +index 0000000..da1866f +--- /dev/null ++++ b/tests/test_options.py +@@ -0,0 +1,45 @@ ++# ++# Copyright (C) 2022 FreeIPA Contributors see COPYING for license ++# ++ ++import argparse ++import os ++import tempfile ++from unittest.mock import patch ++ ++from ipahealthcheck.core.core import RunChecks ++from ipahealthcheck.core.plugin import Results ++ ++options = argparse.Namespace(check=None, source=None, debug=False, ++ indent=2, list_sources=False, ++ output_type='json', output_file=None, ++ verbose=False, version=False, config=None) ++ ++ ++@patch('ipahealthcheck.core.core.run_service_plugins') ++@patch('ipahealthcheck.core.core.run_plugins') ++@patch('ipahealthcheck.core.core.parse_options') ++def test_options_merge(mock_parse, mock_run, mock_service): ++ """ ++ Test merging file-based and CLI options ++ """ ++ mock_service.return_value = (Results(), []) ++ mock_run.return_value = Results() ++ mock_parse.return_value = options ++ fd, config_path = tempfile.mkstemp() ++ os.close(fd) ++ with open(config_path, "w") as fd: ++ fd.write('[default]\n') ++ fd.write('output_type=human\n') ++ fd.write('indent=5\n') ++ ++ try: ++ run = RunChecks(['ipahealthcheck.registry'], config_path) ++ ++ run.run_healthcheck() ++ ++ # verify two valus that have defaults with our overriden values ++ assert run.options.output_type == 'human' ++ assert run.options.indent == '5' ++ finally: ++ os.remove(config_path) +-- +2.31.1 + diff --git a/SOURCES/0024-Convert-configuration-option-strings-into-native-dat.patch b/SOURCES/0024-Convert-configuration-option-strings-into-native-dat.patch new file mode 100644 index 0000000..64e88d0 --- /dev/null +++ b/SOURCES/0024-Convert-configuration-option-strings-into-native-dat.patch @@ -0,0 +1,148 @@ +From 883ef5b965bb7b02a701037748436f8daa9d0643 Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Fri Apr 29 11:42:53 2022 -0400 +Subject: Convert configuration option strings into native data + types + +When loading options from the configuration file they will all +be strings. This breaks existing boolean checks (if ) +and some assumptions about integer types (e.g. timeout, indent). + +So try to detect the data type, defaulting to remain as a string. + +Also hardcode some type validation for known keys to prevent +things like debug=foo (which would evaluate to True). + +https://bugzilla.redhat.com/show_bug.cgi?id=2079861 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/core/config.py | 47 ++++++++++++++++++++++++++++++- + src/ipahealthcheck/core/output.py | 2 +- + tests/test_config.py | 16 ++++++++++- + tests/test_options.py | 2 +- + 4 files changed, 63 insertions(+), 4 deletions(-) + +diff --git a/src/ipahealthcheck/core/config.py b/src/ipahealthcheck/core/config.py +index 14ae27f..3cd4e8e 100644 +--- a/src/ipahealthcheck/core/config.py ++++ b/src/ipahealthcheck/core/config.py +@@ -71,6 +71,27 @@ class Config: + self.__d[key] = d[key] + + ++def convert_string(value): ++ """ ++ Reading options from the configuration file will leave them as ++ strings. This breaks boolean values so attempt to convert them. ++ """ ++ if not isinstance(value, str): ++ return value ++ ++ if value.lower() in ( ++ "true", ++ "false", ++ ): ++ return value.lower() == 'true' ++ else: ++ try: ++ value = int(value) ++ except ValueError: ++ pass ++ return value ++ ++ + def read_config(config_file): + """ + Simple configuration file reader +@@ -100,6 +121,30 @@ def read_config(config_file): + items = parser.items(CONFIG_SECTION) + + for (key, value) in items: +- config[key] = value ++ if len(value) == 0 or value is None: ++ logging.error( ++ "Empty value for %s in %s [%s]", ++ key, config_file, CONFIG_SECTION ++ ) ++ return None ++ else: ++ # Try to do some basic validation. This is unfortunately ++ # hardcoded. ++ if key in ('all', 'debug', 'failures_only', 'verbose'): ++ if value.lower() not in ('true', 'false'): ++ logging.error( ++ "%s is not a valid boolean in %s [%s]", ++ key, config_file, CONFIG_SECTION ++ ) ++ return None ++ elif key in ('indent',): ++ if not isinstance(convert_string(value), int): ++ logging.error( ++ "%s is not a valid integer in %s [%s]", ++ key, config_file, CONFIG_SECTION ++ ) ++ return None ++ # Some rough type translation from strings ++ config[key] = convert_string(value) + + return config +diff --git a/src/ipahealthcheck/core/output.py b/src/ipahealthcheck/core/output.py +index 61dffbe..945969f 100644 +--- a/src/ipahealthcheck/core/output.py ++++ b/src/ipahealthcheck/core/output.py +@@ -110,7 +110,7 @@ class JSON(Output): + + def __init__(self, options): + super(JSON, self).__init__(options) +- self.indent = int(options.indent) ++ self.indent = options.indent + + def generate(self, data): + output = json.dumps(data, indent=self.indent) +diff --git a/tests/test_config.py b/tests/test_config.py +index 09c2188..655233c 100644 +--- a/tests/test_config.py ++++ b/tests/test_config.py +@@ -2,7 +2,7 @@ + # Copyright (C) 2019 FreeIPA Contributors see COPYING for license + # + +-from ipahealthcheck.core.config import read_config ++from ipahealthcheck.core.config import read_config, convert_string + import tempfile + + +@@ -60,3 +60,17 @@ def test_config_recursion(): + config._Config__d['_Config__d'] + except KeyError: + pass ++ ++ ++def test_convert_string(): ++ for value in ("s", "string", "BiggerString"): ++ assert convert_string(value) == value ++ ++ for value in ("True", "true", True): ++ assert convert_string(value) is True ++ ++ for value in ("False", "false", False): ++ assert convert_string(value) is False ++ ++ for value in ("10", "99999", 807): ++ assert convert_string(value) == int(value) +diff --git a/tests/test_options.py b/tests/test_options.py +index da1866f..00cdb7c 100644 +--- a/tests/test_options.py ++++ b/tests/test_options.py +@@ -40,6 +40,6 @@ def test_options_merge(mock_parse, mock_run, mock_service): + + # verify two valus that have defaults with our overriden values + assert run.options.output_type == 'human' +- assert run.options.indent == '5' ++ assert run.options.indent == 5 + finally: + os.remove(config_path) +-- +2.31.1 + diff --git a/SOURCES/0025-Limit-config-file-delimiters-to-catch-empty-values.patch b/SOURCES/0025-Limit-config-file-delimiters-to-catch-empty-values.patch new file mode 100644 index 0000000..ee21394 --- /dev/null +++ b/SOURCES/0025-Limit-config-file-delimiters-to-catch-empty-values.patch @@ -0,0 +1,33 @@ +From 6acb134da07140618a03651756f490074ced3cbc Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Thu Apr 28 08:46:02 2022 -0400 +Subject: Limit config file delimiters to =, catch empty values + +ConfigParser allows both = and : as a delimiter. Limit to +just = to match the configuration file man page. + +Don't allow empty values in options in the config file. + +https://bugzilla.redhat.com/show_bug.cgi?id=2079739 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/core/config.py | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/ipahealthcheck/core/config.py b/src/ipahealthcheck/core/config.py +index 3cd4e8e..e795a2d 100644 +--- a/src/ipahealthcheck/core/config.py ++++ b/src/ipahealthcheck/core/config.py +@@ -107,7 +107,7 @@ def read_config(config_file): + format(config_file)) + return config + +- parser = ConfigParser() ++ parser = ConfigParser(delimiters='=') + try: + parser.read(config_file) + except ParsingError as e: +-- +2.31.1 + diff --git a/SOURCES/0026-Relocate-eval-of-debug-verbose-in-case-they-are-set-.patch b/SOURCES/0026-Relocate-eval-of-debug-verbose-in-case-they-are-set-.patch new file mode 100644 index 0000000..9f8ed06 --- /dev/null +++ b/SOURCES/0026-Relocate-eval-of-debug-verbose-in-case-they-are-set-.patch @@ -0,0 +1,52 @@ +From f28a5fb06a72eca6dc107f11dfd0673ecb92daac Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Thu Apr 28 08:57:38 2022 -0400 +Subject: Relocate eval of debug/verbose in case they are set in + config file + +Since the configuration file allows options to be set we need +to evaluate them after the merge. + +Leaving version handling pre-config load since it makes no sense +within the config file. + +https://bugzilla.redhat.com/show_bug.cgi?id=2079861 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/core/core.py | 12 ++++++------ + 1 file changed, 6 insertions(+), 6 deletions(-) + +diff --git a/src/ipahealthcheck/core/core.py b/src/ipahealthcheck/core/core.py +index 98ca1d2..092bab9 100644 +--- a/src/ipahealthcheck/core/core.py ++++ b/src/ipahealthcheck/core/core.py +@@ -269,12 +269,6 @@ class RunChecks: + if rval: + return rval + +- if options.verbose: +- logger.setLevel(logging.INFO) +- +- if options.debug: +- logger.setLevel(logging.DEBUG) +- + if options.config is not None: + config = read_config(options.config) + else: +@@ -289,6 +283,12 @@ class RunChecks: + self.options = config + options = config + ++ if options.verbose: ++ logger.setLevel(logging.INFO) ++ ++ if options.debug: ++ logger.setLevel(logging.DEBUG) ++ + rval = self.pre_check() + if rval is not None: + return rval +-- +2.31.1 + diff --git a/SOURCES/0027-Validate-that-a-known-output-type-has-been-selected.patch b/SOURCES/0027-Validate-that-a-known-output-type-has-been-selected.patch new file mode 100644 index 0000000..6a9ff1f --- /dev/null +++ b/SOURCES/0027-Validate-that-a-known-output-type-has-been-selected.patch @@ -0,0 +1,54 @@ +From 663ae08a2374df76e93d007551fa5e143425d064 Mon Sep 17 00:00:00 2001 +From: Rob Crittenden +Date: Thu, 28 Apr 2022 08:33:35 -0400 +Subject: Validate that a known output-type has been selected + +A user may pass an unknown value in via the configuration file. + +https://bugzilla.redhat.com/show_bug.cgi?id=2079698 + +Signed-off-by: Rob Crittenden +--- + src/ipahealthcheck/core/constants.py | 2 -- + src/ipahealthcheck/core/core.py | 5 ++++- + 2 files changed, 4 insertions(+), 3 deletions(-) + +diff --git a/src/ipahealthcheck/core/constants.py b/src/ipahealthcheck/core/constants.py +index b6ee029..806bd46 100644 +--- a/src/ipahealthcheck/core/constants.py ++++ b/src/ipahealthcheck/core/constants.py +@@ -2,8 +2,6 @@ + # Copyright (C) 2019 FreeIPA Contributors see COPYING for license + # + +-DEFAULT_OUTPUT = 'json' +- + # Error reporting result + SUCCESS = 0 + WARNING = 10 +diff --git a/src/ipahealthcheck/core/core.py b/src/ipahealthcheck/core/core.py +index 092bab9..84aa8a4 100644 +--- a/src/ipahealthcheck/core/core.py ++++ b/src/ipahealthcheck/core/core.py +@@ -256,7 +256,7 @@ class RunChecks: + def run_healthcheck(self): + framework = object() + plugins = [] +- output = constants.DEFAULT_OUTPUT ++ output = None + + logger.setLevel(logging.WARNING) + +@@ -334,6 +334,9 @@ class RunChecks: + if out.__name__.lower() == options.output_type: + output = out(options) + break ++ if output is None: ++ print(f"Unknown output-type '{options.output_type}'") ++ return 1 + + if options.list_sources: + return list_sources(plugins) +-- +2.31.1 + diff --git a/SOURCES/0028-Restore-the-log-level-after-loading-the-resources.patch b/SOURCES/0028-Restore-the-log-level-after-loading-the-resources.patch new file mode 100644 index 0000000..2a2871e --- /dev/null +++ b/SOURCES/0028-Restore-the-log-level-after-loading-the-resources.patch @@ -0,0 +1,33 @@ +commit b0b782da304babcff3ef64374c79bc879cbdc303 +Author: Rob Crittenden +Date: Wed May 5 16:30:43 2021 -0400 + + Restore the log level after loading the resources + + It seems that loading the resources can affect the log level so + save it off before loading them and then restore it. + + I'm not entirely sure what is happening but this is an easy fix + and we can address it further later if we want. + + Signed-off-by: Rob Crittenden + +diff --git a/src/ipahealthcheck/core/core.py b/src/ipahealthcheck/core/core.p +y +index 4ba94be..2d00586 100644 +--- a/src/ipahealthcheck/core/core.py ++++ b/src/ipahealthcheck/core/core.py +@@ -21,12 +21,15 @@ logger = logging.getLogger() + + + def find_registries(entry_points): ++ # Loading the resources may reset the log level, save it. ++ log_level = logger.level + registries = {} + for entry_point in entry_points: + registries.update({ + ep.name: ep.resolve() + for ep in pkg_resources.iter_entry_points(entry_point) + }) ++ logger.setLevel(log_level) + return registries diff --git a/SPECS/ipa-healthcheck.spec b/SPECS/ipa-healthcheck.spec index baa4912..9be5dc3 100644 --- a/SPECS/ipa-healthcheck.spec +++ b/SPECS/ipa-healthcheck.spec @@ -8,7 +8,7 @@ Name: ipa-healthcheck Version: 0.7 -Release: 10%{?dist} +Release: 14%{?dist} Summary: Health check tool for IdM BuildArch: noarch License: GPLv3 @@ -34,6 +34,15 @@ Patch0016: 0016-tests-Generate-a-proper-not-valid-after-field.patch Patch0017: 0017-Fix-the-number-of-expected-results-in-the-fix-file-t.patch Patch0018: 0018-Don-t-collect-the-CRLManager-role-if-the-CA-is-not-c.patch Patch0019: 0019-Don-t-depend-on-IPA-status-when-suppressing-pki-chec.patch +Patch0020: 0020-Add-support-for-the-DNS-URI-type.patch +Patch0021: 0021-Use-the-subject-base-from-the-IPA-configuration-not-.patch +Patch0022: 0022-Allow-multiple-file-modes-in-the-FileChecker.patch +Patch0023: 0023-Unify-command-line-options-and-configuration.patch +Patch0024: 0024-Convert-configuration-option-strings-into-native-dat.patch +Patch0025: 0025-Limit-config-file-delimiters-to-catch-empty-values.patch +Patch0026: 0026-Relocate-eval-of-debug-verbose-in-case-they-are-set-.patch +Patch0027: 0027-Validate-that-a-known-output-type-has-been-selected.patch +Patch0028: 0028-Restore-the-log-level-after-loading-the-resources.patch Requires: %{name}-core = %{version}-%{release} Requires: ipa-server @@ -137,6 +146,18 @@ install -p -m644 %{_builddir}/%{project}-%{shortname}-%{version}/man/man5/%{long %changelog +* Wed May 25 2022 Rob Crittenden - 0.7-14 +- Add CLI options to healthcheck configuration file (#1872467) + +* Fri Apr 29 2022 Rob Crittenden - 0.7-13 +- Allow multiple file modes in the FileChecker (#2058239) + +* Thu Mar 31 2022 Rob Crittenden - 0.7-12 +- Use the subject base from the IPA configuration, not REALM (#2066308) + +* Fri Mar 18 2022 Rob Crittenden - 0.7-11 +- Add support for the DNS URI type (#2037847) + * Thu Feb 17 2022 Rob Crittenden - 0.7-10 - Don't depend on IPA status when suppressing pki checks (#2055316)