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

[Martin Basti]

On 08/13/2015 12:09 PM, Fraser Tweedale wrote:
> 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
Pushed to:
ipa-4-2: 9ca156c85919108d0c13718384dc196075364398
master: 27988f1b836874d6b1df0659bc95390636caeb78