URL: https://github.com/freeipa/freeipa/pull/5733
Author: rcritten
Title: #5733: Revert the way hosts/services certs are searched for
Action: opened
PR body:
"""
Revert the way hosts/services certs are searched for
This reverts a change that caused all service certs to be
revoked if any other service was deleted.
There is special handling when passing in host, service or
user into cert_find. The _ldap_search method creates
filters for the object which later in cert_find removes
unrelated certificates.
https://pagure.io/freeipa/issue/7835
Signed-off-by: Rob Crittenden <rcrit...@redhat.com>
"""
To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/5733/head:pr5733
git checkout pr5733
From bb56b0543d72917ee7ffede5749b30ee1203aa44 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Fri, 23 Apr 2021 17:04:39 +0000
Subject: [PATCH 1/2] Revert the way hosts/services certs are searched for
This reverts a change that caused all service certs to be
revoked if any other service was deleted.
There is special handling when passing in host, service or
user into cert_find. The _ldap_search method creates
filters for the object which later in cert_find removes
unrelated certificates.
https://pagure.io/freeipa/issue/7835
Signed-off-by: Rob Crittenden <rcrit...@redhat.com>
---
ipaserver/plugins/cert.py | 16 +++++++++++++--
ipaserver/plugins/host.py | 2 +-
ipaserver/plugins/service.py | 39 +++++++++++++-----------------------
3 files changed, 29 insertions(+), 28 deletions(-)
diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 7decee439c1..f63f8f9031b 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -1650,8 +1650,7 @@ def _ca_search(self, raw, pkey_only, exactly, **options):
'validnotafter_from', 'validnotafter_to',
'validnotbefore_from', 'validnotbefore_to',
'issuedon_from', 'issuedon_to',
- 'revokedon_from', 'revokedon_to',
- 'status'):
+ 'revokedon_from', 'revokedon_to',):
try:
value = options[name]
except KeyError:
@@ -1688,6 +1687,11 @@ def _ca_search(self, raw, pkey_only, exactly, **options):
elif len(users) == 1 and not services and not hosts:
ra_options['subject'] = users[0]
+ # Make an exception for status so we can only retrieve VALID
+ # certificates when searching for hosts and services.
+ if 'status' in options:
+ ra_options['status'] = options.get('status')
+
try:
ca_enabled_check(self.api)
except errors.NotFound:
@@ -1733,6 +1737,14 @@ def _ca_search(self, raw, pkey_only, exactly, **options):
return result, False, complete
def _ldap_search(self, all, pkey_only, no_members, **options):
+ """Search for certificates directly in the LDAP tree
+
+ This is only done when one of the three special options
+ user, host, service are passed in.
+
+ The effect of this is that objects not requested will be
+ filtered out later because complete will be True.
+ """
ldap = self.api.Backend.ldap2
filters = []
diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 8d5cf3c6003..8c84a9e3bc7 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -872,7 +872,7 @@ def pre_callback(self, ldap, dn, *keys, **options):
if self.api.Command.ca_is_enabled()['result']:
certs = self.api.Command.cert_find(
- subject=fqdn, status='VALID'
+ host=keys, status='VALID'
)['result']
revoke_certs(certs)
diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 27d3fc8db2f..b77a47b002f 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -825,16 +825,10 @@ def pre_callback(self, ldap, dn, *keys, **options):
# custom services allow them to manage them.
check_required_principal(ldap, keys[-1])
if self.api.Command.ca_is_enabled()['result']:
- # only try to revoke certs for valid principals
- try:
- subject = keys[-1].hostname
- except ValueError:
- pass
- else:
- certs = self.api.Command.cert_find(
- subject=subject, status='VALID'
- )['result']
- revoke_certs(certs)
+ certs = self.api.Command.cert_find(
+ service=keys, status='VALID'
+ )['result']
+ revoke_certs(certs)
return dn
@@ -1108,21 +1102,16 @@ def execute(self, *keys, **options):
done_work = False
if self.api.Command.ca_is_enabled()['result']:
- try:
- subject = keys[-1].hostname
- except ValueError:
- pass
- else:
- certs = self.api.Command.cert_find(
- subject=subject, status='VALID'
- )['result']
-
- if len(certs) > 0:
- revoke_certs(certs)
- # Remove the usercertificate altogether
- entry_attrs['usercertificate'] = None
- ldap.update_entry(entry_attrs)
- done_work = True
+ certs = self.api.Command.cert_find(
+ service=keys, status='VALID'
+ )['result']
+
+ if len(certs) > 0:
+ revoke_certs(certs)
+ # Remove the usercertificate altogether
+ entry_attrs['usercertificate'] = None
+ ldap.update_entry(entry_attrs)
+ done_work = True
self.obj.get_password_attributes(ldap, dn, entry_attrs)
if entry_attrs['has_keytab']:
From 9f38ec61efc9be0f1115d2f23f16e72a24f2d3a8 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Fri, 23 Apr 2021 17:35:30 +0000
Subject: [PATCH 2/2] ipatests: Test that service-del revokes the right certs
The cert-find code can be tricky. When deleting a service
check that its certificate is revoked but also check whether
another service for the same host is not.
https://pagure.io/freeipa/issue/7835
Signed-off-by: Rob Crittenden <rcrit...@redhat.com>
---
ipatests/test_integration/test_cert.py | 37 ++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/ipatests/test_integration/test_cert.py b/ipatests/test_integration/test_cert.py
index b6bb2f08ac0..6c8b2952d86 100644
--- a/ipatests/test_integration/test_cert.py
+++ b/ipatests/test_integration/test_cert.py
@@ -168,6 +168,43 @@ def test_ipa_getcert_san_aci(self):
ipaddrs = ext.value.get_values_for_type(x509.IPAddress)
assert ipaddrs == [ipaddress.ip_address(self.clients[0].ip)]
+ def test_service_del_revocation(self):
+ """Test that removing a service revokes its certificate
+
+ This removes the service created in the previous test
+ and ensures that other certificates are not revoked when
+ one is.
+ """
+ hostname = self.clients[0].hostname
+ certfile = os.path.join(paths.OPENSSL_CERTS_DIR, "testservice.pem")
+
+ cmd_arg = ['ipa-getcert', 'list', '-f', certfile]
+ result = self.clients[0].run_command(cmd_arg)
+ request_id = re.findall(r'\d+', result.stdout_text)
+
+ # This assumes that the cert was issued in the previous test
+ certdata = self.clients[0].get_file_contents(certfile)
+ cert = x509.load_pem_x509_certificate(
+ certdata, default_backend()
+ )
+ serial = cert.serial_number
+
+ # Baseline: the cert should not be revoked yet
+ result = self.master.run_command(["ipa", "cert-show", str(serial)])
+ assert "Revoked: False" in result.stdout_text
+
+ # Delete the service and confirm the cert is revoked
+ self.master.run_command(["ipa", "service-del", f"test/{hostname}"])
+ result = self.master.run_command(["ipa", "cert-show", str(serial)])
+ assert "Revoked: True" in result.stdout_text
+
+ # Get the web cert so we can ensure it isn't revoked
+ certfile = self.master.get_file_contents(paths.HTTPD_CERT_FILE)
+ cert = x509.load_certificate_list(certfile)
+ serial = cert[0].serial_number
+ result = self.master.run_command(["ipa", "cert-show", str(serial)])
+ assert "Revoked: False" in result.stdout_text
+
def test_getcert_list_profile(self):
"""
Test that getcert list command displays the profile
_______________________________________________
FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org
To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/freeipa-devel@lists.fedorahosted.org
Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
