[Freeipa-devel] [PATCH] 0039 Prohibit deletion of included profiles

[Fraser Tweedale]

On Thu, Aug 13, 2015 at 12:31:27PM +0300, Alexander Bokovoy wrote:
> On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >On Thu, Aug 13, 2015 at 12:01:09PM +0300, Alexander Bokovoy wrote:
> >>On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >>>On Thu, Aug 13, 2015 at 09:53:35AM +0300, Alexander Bokovoy wrote:
> >>>>On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >>>>>The attached patch fixes
> >>>>>https://fedorahosted.org/freeipa/ticket/5198
> >>>>>
> >>>>>Thanks,
> >>>>>Fraser
> >>>>
> >>>>>From 0dd316bf0cbab7b6701bd69f142e82b30bee25b8 Mon Sep 17 00:00:00 2001
> >>>>>From: Fraser Tweedale <ftweedal at redhat.com>
> >>>>>Date: Thu, 13 Aug 2015 02:32:54 -0400
> >>>>>Subject: [PATCH] Prohibit deletion of included profiles
> >>>>>
> >>>>>Deletion of included profiles, including the default profile, should
> >>>>>not be allowed.  Detect this case and raise an error.
> >>>>>
> >>>>>Also update the included profiles collection to use namedtuple,
> >>>>>making it easier to access the various components.
> >>>>>
> >>>>>Fixes: https://fedorahosted.org/freeipa/ticket/5198
> >>>>>---
> >>>>>ipalib/plugins/certprofile.py | 13 +++++++++++--
> >>>>>ipapython/dogtag.py           |  8 +++++---
> >>>>>2 files changed, 16 insertions(+), 5 deletions(-)
> >>>>>
> >>>>>diff --git a/ipalib/plugins/certprofile.py b/ipalib/plugins/certprofile.py
> >>>>>index 1dd4f403ee4461b83c053eb36019a8896506bb81..03bdd28728dc864adcd7305ddbff34a23405e78f 100644
> >>>>>--- a/ipalib/plugins/certprofile.py
> >>>>>+++ b/ipalib/plugins/certprofile.py
> >>>>>@@ -3,6 +3,7 @@
> >>>>>#
> >>>>>
> >>>>>import re
> >>>>>+from operator import attrgetter
> >>>>>
> >>>>>from ipalib import api, Bool, File, Str
> >>>>>from ipalib import output, util
> >>>>>@@ -14,6 +15,7 @@ from ipalib.plugins.baseldap import (
> >>>>>from ipalib.request import context
> >>>>>from ipalib import ngettext
> >>>>>from ipalib.text import _
> >>>>>+from ipapython.dogtag import INCLUDED_PROFILES
> >>>>>from ipapython.version import API_VERSION
> >>>>>
> >>>>>from ipalib import errors
> >>>>>@@ -287,9 +289,16 @@ class certprofile_del(LDAPDelete):
> >>>>>    __doc__ = _("Delete a Certificate Profile.")
> >>>>>    msg_summary = _('Deleted profile "%(value)s"')
> >>>>>
> >>>>>-    def execute(self, *args, **kwargs):
> >>>>>+    def pre_callback(self, ldap, dn, *keys, **options):
> >>>>>        ca_enabled_check()
> >>>>>-        return super(certprofile_del, self).execute(*args, **kwargs)
> >>>>>+
> >>>>>+        if keys[0] in map(attrgetter('profile_id'), INCLUDED_PROFILES):
> >>>>>+            raise errors.ValidationError(name='profile_id',
> >>>>>+                error=_("Included profile '%(profile_id)s' cannot be deleted")
> >>>>>+                    % {'profile_id': keys[0]}
> >>>>>+            )
> >>>>>+
> >>>>>+        return dn
> >>>>I think you also want to protect the included profiles from renaming.
> >>>>
> >>>This is already the case.
> >>I'm also wondering about certprofile-mod changing the profile content
> >>and changing profileID there to point to existing profile. Would this
> >>affect CA operation?
> >>
> >Renaming profile / changing profile-id / pointing it to a different
> >profile is not possible.
> >
> >Changing profile content *is* currently possible.  Given that we
> >have custom profiles now, there is an argument to be made that we
> >should prevent profile-mod for updating the Dogtag configuration of
> >predefined profiles.
> >
> >If we did that, we would probably also want to allow admins to
> >change which is the default profile, i.e. changing the default to
> >some custom profile they added.
> >
> >And if we did that, then perhaps we should let them specify a
> >different default profile for users vs hosts/services!
> >
> >How deep does this rabbit hole go? :)
> All the above makes sense and should be done in terms of proper
> hardening and usability fixes. I don't think it is a bottomless hole,
> though, just a normal work we have to do to make certificate profiles
> nice and usable :)
> 
Right; I'll file tickets for these explored regions of the hole, and
leave the unexplored depths for another day.

> -- 
> / Alexander Bokovoy