On 2016-07-19 17:03, Martin Basti wrote:
> 
> 
> On 12.07.2016 16:45, Christian Heimes wrote:
>> Custodia's server.keys file contain the private RSA keys for encrypting
>> and signing Custodia messages. The file was created with permission 644
>> and is only secured by permission 700 of the directory
>> /etc/ipa/custodia. The installer and upgrader ensure that the file
>> has 600.
>>
>> The server.keys file and all keys are now removed when during
>> uninstallation of a server, too.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1353936
>> https://fedorahosted.org/freeipa/ticket/6015
>> https://fedorahosted.org/freeipa/ticket/6056
>>
>>
> NACK
> 
> ipa-server-install --uninstall doesn't work

I fixed it by splitting up uninstallation into two parts:

1) the server_del plugin takes care of the LDAP entries
2) CustodiaInstance.uninstall() removes the local key file

From 77541f4d70e3fa02a058c0812d4062a34e0bebf4 Mon Sep 17 00:00:00 2001
From: Christian Heimes <chei...@redhat.com>
Date: Fri, 8 Jul 2016 20:06:57 +0200
Subject: [PATCH] Secure permission and cleanup Custodia server.keys

Custodia's server.keys file contain the private RSA keys for encrypting
and signing Custodia messages. The file was created with permission 644
and is only secured by permission 700 of the directory
/etc/ipa/custodia. The installer and upgrader ensure that the file
has 600.

The server.keys file and all keys are now removed when during
uninstallation of a server, too.

https://bugzilla.redhat.com/show_bug.cgi?id=1353936
https://fedorahosted.org/freeipa/ticket/6015
https://fedorahosted.org/freeipa/ticket/6056
---
 ipalib/constants.py                   |  1 +
 ipapython/secrets/kem.py              | 60 +++++++++++++++++++++++++++++++----
 ipaserver/install/custodiainstance.py | 25 +++++++++++----
 ipaserver/plugins/server.py           | 31 ++++++++++++++++++
 4 files changed, 104 insertions(+), 13 deletions(-)

diff --git a/ipalib/constants.py b/ipalib/constants.py
index 0574bb3aa457dd79a6d64f6b8a6b57161d32da92..9b351e260f15211330521453b3ffcd41433a04bb 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -124,6 +124,7 @@ DEFAULT_CONFIG = (
     ('container_locations', DN(('cn', 'locations'), ('cn', 'etc'))),
     ('container_ca', DN(('cn', 'cas'), ('cn', 'ca'))),
     ('container_dnsservers', DN(('cn', 'servers'), ('cn', 'dns'))),
+    ('container_custodia', DN(('cn', 'custodia'), ('cn', 'ipa'), ('cn', 'etc'))),
 
     # Ports, hosts, and URIs:
     ('xmlrpc_uri', 'http://localhost:8888/ipa/xml'),
diff --git a/ipapython/secrets/kem.py b/ipapython/secrets/kem.py
index d45efe8cc4fb63ae9d8c0b2c920fd1f9e5331a9d..863234e01bf6226ee5090c76450b0fe25c430ed6 100644
--- a/ipapython/secrets/kem.py
+++ b/ipapython/secrets/kem.py
@@ -15,6 +15,8 @@ from jwcrypto.jwk import JWK
 from ipapython.secrets.common import iSecLdap
 from binascii import unhexlify
 import ldap
+import errno
+import os
 
 
 IPA_REL_BASE_DN = 'cn=custodia,cn=ipa,cn=etc'
@@ -66,7 +68,7 @@ class KEMLdap(iSecLdap):
                                          'princ': principal})
         r = conn.search_s(self.keysbase, scope, ldap_filter)
         if len(r) != 1:
-            raise ValueError("Incorrect number of results (%d) searching for"
+            raise ValueError("Incorrect number of results (%d) searching for "
                              "public key for %s" % (len(r), principal))
         ipa_public_key = r[0][1]['ipaPublicKey'][0]
         jwk = self._parse_public_key(ipa_public_key)
@@ -139,11 +141,29 @@ class KEMLdap(iSecLdap):
             mods = [(ldap.MOD_REPLACE, 'ipaPublicKey', public_key)]
             conn.modify_s(dn, mods)
 
+    def remove_key(self, usage, principal):
+        conn = self.connect()
+        scope = ldap.SCOPE_SUBTREE
+
+        ldap_filter = self.build_filter(IPA_KEYS_QUERY,
+                                        {'usage': RFC5280_USAGE_MAP[usage],
+                                         'princ': principal})
+
+        r = conn.search_s(self.keysbase, scope, ldap_filter)
+        if not r:
+            return False
+        for entry in r:
+            dn = r[0][0]
+            conn.delete_s(dn)
+        return True
+
 
 def newServerKeys(path, keyid):
     skey = JWK(generate='RSA', use='sig', kid=keyid)
     ekey = JWK(generate='RSA', use='enc', kid=keyid)
-    with open(path, 'w+') as f:
+    with open(path, 'w') as f:
+        os.fchmod(f.fileno(), 0o600)
+        os.fchown(f.fileno(), 0, 0)
         f.write('[%s,%s]' % (skey.export(), ekey.export()))
     return [skey.get_op_key('verify'), ekey.get_op_key('encrypt')]
 
@@ -177,6 +197,9 @@ class IPAKEMKeys(KEMKeysStore):
             self.ldap_uri = conf.get('global', 'ldap_uri', None)
         self._server_keys = None
 
+    def get_principal(self, servicename):
+        return '%s/%s@%s' % (servicename, self.host, self.realm)
+
     def find_key(self, kid, usage):
         if kid is None:
             raise TypeError('Key ID is None, should be a SPN')
@@ -187,7 +210,7 @@ class IPAKEMKeys(KEMKeysStore):
         self.generate_keys('host')
 
     def generate_keys(self, servicename):
-        principal = '%s/%s@%s' % (servicename, self.host, self.realm)
+        principal = self.get_principal(servicename)
         # Neutralize the key with read if any
         self._server_keys = None
         # Generate private key and store it
@@ -197,6 +220,25 @@ class IPAKEMKeys(KEMKeysStore):
         ldapconn.set_key(KEY_USAGE_SIG, principal, pubkeys[0])
         ldapconn.set_key(KEY_USAGE_ENC, principal, pubkeys[1])
 
+    def remove_server_keys(self):
+        self.remove_keys('host')
+
+    def remove_keys(self, servicename):
+        principal = self.get_principal(servicename)
+        self._server_keys = None
+        # remove keys from LDAP
+        ldapconn = KEMLdap(self.ldap_uri)
+        ldapconn.remove_key(KEY_USAGE_SIG, principal)
+        ldapconn.remove_key(KEY_USAGE_ENC, principal)
+
+    def remove_key_file(self):
+        # remove server.keys file
+        try:
+            os.unlink(self.config['server_keys'])
+        except OSError as e:
+            if e.errno != errno.ENOENT:
+                raise
+
     @property
     def server_keys(self):
         if self._server_keys is None:
@@ -213,9 +255,13 @@ if __name__ == '__main__':
     IKK = IPAKEMKeys({'paths': '/',
                       'server_keys': '/etc/ipa/custodia/server.keys'})
     IKK.generate_server_keys()
+    principal = IKK.get_principal('host')
+    print(IKK.find_key(principal, KEY_USAGE_SIG))
+
     print(('SIG', IKK.server_keys[0].export_public()))
     print(('ENC', IKK.server_keys[1].export_public()))
-    print(IKK.find_key('host/%s@%s' % (IKK.host, IKK.realm),
-                       usage=KEY_USAGE_SIG))
-    print(IKK.find_key('host/%s@%s' % (IKK.host, IKK.realm),
-                       usage=KEY_USAGE_ENC))
+    print(IKK.find_key(principal, usage=KEY_USAGE_SIG))
+    print(IKK.find_key(principal, usage=KEY_USAGE_ENC))
+
+    IKK.remove_server_keys()
+    print(IKK.find_key(principal, usage=KEY_USAGE_ENC))
diff --git a/ipaserver/install/custodiainstance.py b/ipaserver/install/custodiainstance.py
index fd30430bbf9c39e7153986999199474cfca60d09..6f16b5e1cfd938328e6f68bfe2d562f8d65df266 100644
--- a/ipaserver/install/custodiainstance.py
+++ b/ipaserver/install/custodiainstance.py
@@ -15,6 +15,7 @@ from jwcrypto.common import json_decode
 import functools
 import shutil
 import os
+import stat
 import tempfile
 import pwd
 
@@ -47,10 +48,8 @@ class CustodiaInstance(SimpleServiceInstance):
                         LDAP_URI=installutils.realm_to_ldapi_uri(self.realm),
                         UID=httpd_info.pw_uid, GID=httpd_info.pw_gid)
         conf = ipautil.template_file(template, sub_dict)
-        fd = open(self.config_file, "w+")
-        fd.write(conf)
-        fd.flush()
-        fd.close()
+        with open(self.config_file, "w") as f:
+            f.write(conf)
 
     def create_instance(self, dm_password=None):
         suffix = ipautil.realm_to_suffix(self.realm)
@@ -65,14 +64,28 @@ class CustodiaInstance(SimpleServiceInstance):
         sysupgrade.set_upgrade_state('custodia', 'installed', True)
 
     def __gen_keys(self):
-        KeyStore = IPAKEMKeys({'server_keys': self.server_keys,
+        keystore = IPAKEMKeys({'server_keys': self.server_keys,
                                'ldap_uri': self.ldap_uri})
-        KeyStore.generate_server_keys()
+        keystore.generate_server_keys()
+
+    def __remove_keys(self):
+        keystore = IPAKEMKeys({'server_keys': self.server_keys})
+        # LDAP entries are removed in server_del plugin.
+        keystore.remove_key_file()
 
     def upgrade_instance(self):
         if not sysupgrade.get_upgrade_state("custodia", "installed"):
             root_logger.info("Custodia service is being configured")
             self.create_instance()
+        mode = os.stat(self.server_keys).st_mode
+        if stat.S_IMODE(mode) != 0o600:
+            root_logger.info("Secure server.keys mode")
+            os.chmod(self.server_keys, 0o600)
+
+    def uninstall(self):
+        super(CustodiaInstance, self).uninstall()
+        root_logger.info("Remove Custodia keys")
+        self.__remove_keys()
 
     def create_replica(self, master_host_name):
         suffix = ipautil.realm_to_suffix(self.realm)
diff --git a/ipaserver/plugins/server.py b/ipaserver/plugins/server.py
index b245dcf72a2f9f32f52ec9acf68d96c69d6169c5..56936545ad79340184d5da8514644f2c44281456 100644
--- a/ipaserver/plugins/server.py
+++ b/ipaserver/plugins/server.py
@@ -609,6 +609,34 @@ class server_del(LDAPDelete):
                     message=_("Failed to remove server %(master)s from server "
                               "list: %(err)s") % dict(master=master, err=e)))
 
+    def _remove_server_custodia_keys(self, ldap, master):
+        """
+        Delete all Custodia encryption and signing keys
+        """
+        conn = self.Backend.ldap2
+        env = self.api.env
+
+        member_principal = "*/{}@{}".format(master, env.realm)
+        member_filter = ldap.make_filter_from_attr('memberPrincipal',
+                                                   member_principal)
+        custodia_subtree = DN(env.container_custodia, env.basedn)
+
+        try:
+            entries = conn.get_entries(custodia_subtree,
+                                       ldap.SCOPE_SUBTREE,
+                                       filter=member_filter)
+            if len(entries) != 0:
+                for entry in entries:
+                    conn.delete_entry(entry)
+        except errors.NotFound:
+            pass
+        except Exception as e:
+            self.add_message(
+                messages.ServerRemovalWarning(
+                    message=_(
+                        "Failed to clean up Custodia keys for "
+                        "%(master)s: %(err)s") % dict(master=master, err=e)))
+
     def _remove_server_host_services(self, ldap, master):
         """
         delete server kerberos key and all its svc principals
@@ -682,6 +710,9 @@ class server_del(LDAPDelete):
         # remove the references to master's ldap/http principals
         self._remove_server_principal_references(pkey)
 
+        # remove Custodia encryption and signing keys
+        self._remove_server_custodia_keys(ldap, pkey)
+
         # finally destroy all Kerberos principals
         self._remove_server_host_services(ldap, pkey)
 
-- 
2.7.4

Attachment: signature.asc
Description: OpenPGP digital signature

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to