[Freeipa-devel] [WIP][PATCH] Time-Based HBAC Policies
Stanislav Laznicka
slaznick at redhat.com
Wed Mar 30 10:45:34 UTC 2016
On 03/09/2016 05:24 PM, Martin Basti wrote:
>
> On 04.03.2016 14:53, Stanislav Laznicka wrote:
>> Hello,
>>
>> So in the previous month and a bit I was reworking the time-based
>> policies according to the changes we agreed on
>> (http://pad.engineering.redhat.com/ipa-time-based-HBAC-design, line
>> 83). Let me briefly walk you through what was done (no TLDR, sorry,
>> but split the text in chapters):
>>
>> *Time rule templates*
>> In the attachment is the proposal how this could be done using
>> costemplates. Currently, the time rule templates have their own
>> directory in the realm tree. The idea is that it could be used for
>> both HBAC and Sudo rules so it needs to be in a location both should
>> be able to reach. Should we not want them used in Sudo rules, the
>> template directory could be moved to HBAC directory. There are also
>> some new permissions for accessing these time rule templates which
>> may need to be revised if the templates should be used both for sudo
>> and HBAC rules.
>>
>> *iCalendar format validation
>> *So there is an iCalendar string validation now. During its creation,
>> I came across several issues with python-icalendar which is basically
>> why it took me so long to write the validation. I made several fixes
>> to the python-icalendar library, most of them are already merged in
>> the repository master (https://github.com/collective/icalendar), one
>> should be pushed in the next library major release.
>>
>> My pull requests:
>> https://github.com/collective/icalendar/pull/175
>> https://github.com/collective/icalendar/pull/179
>> https://github.com/collective/icalendar/pull/180
>> https://github.com/collective/icalendar/pull/183
>> https://github.com/collective/icalendar/pull/189
>>
>> I still have one fix in the making, that one should force the strong
>> types in iCalendar as these are also missing in python-icalendar but
>> required by the RFC.
>>
>> Also, obviously, if you want to try the patches, you will need the
>> current python-icalendar implementation from Github. I haven't put
>> python-icalendar dependency into the .spec file yet for this reason.
>> *
>> **Summary
>> *We are now able to import iCalendar strings from files and more or
>> less be sure that the parts we need will be consistent with the RFC
>> 5545 (basically, we are only checking that VEVENT components are
>> correct, to bring strict checking to python-icalendar would take some
>> time and I believe I spent way too much time with it already (there
>> is an issue on their github page, though, it's 4 years old)).
>>
>> *TODO now
>> *0)**Update the design*
>> *1a) The hbacrule-*-accesstime should probably be split into 2
>> commands, one that reads iCalendar strings from files, and one that
>> creates those strings from "some kind of user input" (similarly for
>> timeruletemplates).
>> 1b) Create the format of user input we could expect for the second
>> kind of command from 1a). We need to be able to convert it to
>> iCalendar string and back so that we are able to present the data
>> stored on the server in human readable form.
>> http://jkbrzt.github.io/rrule/ NL part might be of help although it
>> aims mostly on RRULE property of VEVENT components, whereas we may
>> want to use DTEND, EXDATE, RDATE and DURATION as well to be able to
>> specify events more properly.
>> 2) Represent the HBAC time rules on SSSD side. I already have a
>> skeleton of this based on libical
>> (https://github.com/libical/libical), which hopefully seems to be
>> more viable than python-icalendar. I do not mean to do the validation
>> of received iCalendar string on the SSSD side anymore (at least not
>> in an excessive way), just get the required properties from VEVENT
>> components and evaluate them accordingly.
>>
>> *Discuss
>> *I would really appreciate your input on these topics:*
>> *1)**How to represent the iCalendar strings on the client side in CLI
>> (while thinking about WebUI as well)?
>> 2a) Do we want to use the time rules for Sudo rules as well?
>> 2b) If 2a), is the proposed location of time rule templates along
>> with the privileges ok?
>>
>> Standa
>>
>>
> Hello,
> thank you for the patchset, I have a few comments :)*
> ****
> ***1)
> +attributeTypes: (2.16.840.1.113730.3.8.11.72 NAME 'timeruleClass'
> DESC 'CNs of the timerule classes'
>
> OID above is registered as:
> 2.16.840.1.113730.3.8.11.72 accessTimeExclude
> Access time - exclude these values
I would like to stick with a known OID before we decide that the patch
is in final version but it's of course a right point until then.
> *
> ***2)*
> ***please add requires and buildrequires to specfile (python-icalendar)
Done.
>
> 3)
> Pylint is running, please wait ...
> ************* Module ipalib.plugins.hbacrule
> ipalib/plugins/hbacrule.py:166: [E1101(no-member), validate_icalfile]
> Instance of 'list' has no 'name' member)
> ipalib/plugins/hbacrule.py:175: [E1101(no-member), validate_icalfile]
> Instance of 'list' has no 'subcomponents' member)
> ipalib/plugins/hbacrule.py:177: [E1601(print-statement),
> validate_icalfile] print statement used)
> ipalib/plugins/hbacrule.py:190: [E1601(print-statement),
> validate_icalfile] print statement used)
>
> first two errors must be disabled by # pylint: disable=no-member
> because it is too complicated for pylint
>
> I'm pretty sure that print should not be in plugin implementation
Done, replaced the prints with root_logger.
>
> 4)
> PEP8
>
> ./ipalib/plugins/hbacrule.py:255:80: E501 line too long (216 > 79
> characters)
> ./ipalib/plugins/hbacrule.py:262:80: E501 line too long (225 > 79
> characters)
> ./ipalib/plugins/hbacrule.py:270:80: E501 line too long (251 > 79
> characters)*
> ***./ipalib/plugins/hbacrule.py:456:17: E127 continuation line
> over-indented for visual indent*
> ***./ipalib/plugins/hbacrule.py:646:80: E501 line too long (80 > 79
> characters)*
> ***./ipalib/plugins/hbacrule.py:657:1: E302 expected 2 blank lines,
> found 1
> ./ipalib/plugins/hbacrule.py:663:1: E302 expected 2 blank lines, found 1*
> ****
> ***./ipalib/plugins/hbacrule.py:177:80: E501 line too long (80 > 79
> characters)*
> ***./ipalib/plugins/hbacrule.py:215:80: E501 line too long (80 > 79
> characters)*
> *
Do you want me to fix this last 1 letter overflow as well? It would
probably look worse if I did.
> ****./ipalib/plugins/hbacrule.py:542:80: E501 line too long (127 > 79
> characters)
> ./ipalib/plugins/hbacrule.py:544:80: E501 line too long (127 > 79
> characters)
These two were not really caused by me but I can fix these PEP8 errors
along with some others in a separate patch if you want to.
> ****./ipalib/plugins/hbacrule.py:551:1: E303 too many blank lines (3)
>
> 5)
> Following imports in hbac rule should be before ipalib imports
>
> +import icalendar
> +from datetime import date
> +
>
Done.
> 6)
> + ical_errors = ('{comp}: {err}'
> + .format(comp=x, err=y) for x, y in comp.errors)
>
> it is not clear for me what is x, and y. A component of the component?
> can you named it better than x,y. We are not limited by length of
> identifiers in python too much :)
Good eye, it was actually a property of a component. Modified it a bit.
Components may appear in components too, though.
>
> /7)//
> //ugettext string is wrong (all places)//
> //+ error=_('There were errors parsing the iCalendar
> string:\n{errs}'//
> //+ .format(errs='\n'.join(ical_errors)))//
> //it should be//
> ////
> //error=_('There were errors parsing the iCalendar
> string:\n{errs}').format(errs='\n'.join(ical_errors))//
> /
//Had to go with the "%" syntax in the end because "Instance of
'Gettext' has no 'format' member". However, this brought kind of
inconsistency of how strings are handled in my code. Would it be better
to use the "%" syntax everywhere, then?
>
> 8)
> + # TODO: comp.required might be removed when
> + # https://github.com/collective/icalendar/pull/183 is merged
>
> I'm not fan of TODO's in code, you provides copr repo anyway, so
> please build package with this merge and remove TODO, we should create
> workaround when upstream refuse your patches.
Let me just leave that there as long as we're in WIP phase so that I
might eventually remove that part of code when the mentioned patch is
merged or just remove the comments if not.
> /
> ///9)
> + if api.env.context == 'cli':
> + if ics and os.path.exists(ics):
> + return
>
> The param is File class, so this check should eb done automatically
Done. I just followed the code from cert.py.
>
> 10)
> + icalstr = ics
>
> this statement is useless, please use ics directly
>
Done, of course.
> 11)
> +def validate_icalfile(ugettext, ics):
>
> This and other similar restriction are required by IPA or it is
> invalid icalfile? (sorry for question but I'm not familiar with
> icalendar enough yet)
It is an invalid icalfile.
> + error=_('A VEVENT component can\'t contain '
> + 'subcomponent "{}".'.format(sub.name))
> + )
>
> In second case (icalfile is invalid) shouldn't be this validation done
> in python-icalendar module instead of IPA validators?
I believe all (or most of) the checks should be done in
python-icalendar, actually. As I started working with libical again, I
am getting the impression that the iCalendar parsers are there only to
get the information from what seems to be an iCalendar string by any
means possible no matter how wrong the input is. I believe that theory
tells us that parser should be able to distinguish invalid input from
valid one, yet this is not true at all for python-icalendar and for
libical. While such behavior might be desirable in some cases, these
parsers should at least have means to check the validity of the input if
it's not performed by default. Yet they don't and I can say it literally
drives me insane.
>
> 12)
> + if 'DTEND' in comp.keys() and 'DURATION' in comp.keys():
> + raise errors.ValidationError(
> + name=name,
> + error=_('Both DURATION and DTEND set in a VEVENT.')
> + )
> +
> + elif 'DTEND' in comp.keys():
> + if type(comp['DTSTART'].dt) != type(comp['DTEND'].dt):
> + raise errors.ValidationError(
> + name=name,
> + error=_('Different types of DTSTART and DTEND '
> + 'component in VEVENT.')
> + )
>
> IMO following way is better for readability
> if 'DTEND' in comp.keys():
> if 'DURATION' in comp.keys():
> something1
> elif type(comp['DTSTART'].dt) != type(comp['DTEND'].dt):
> something2
You're right, modified it.
>
> 13)
> PATCH: Templating of access time rules for HBAC
> There is missing upgrade path for:
> +dn: cn=timeruleTemplates,$SUFFIX
> +dn: cn=cosTimerulesDef,cn=hbac,$SUFFIX
Should be ok now.
> *****
> ***14)
> + container_dn = DN(('cn', 'timeruletemplates'))
> Please define this in constants.py*
> *
Done.
> *
> *15)
> Your managed ACI are completely new, so there should not be 'replaces'
> definition (several times)
> + 'replaces': [
> + '(target =
> "ldap:///cn=*,cn=timeruletemplates,$SUFFIX")(version 3.0;acl
> "permission:Delete Time Rule Template";allow (delete) groupdn =
> "ldap:///cn=Delete Time Rule Template,cn=permissions,cn=pbac,$SUFFIX";)',
> + ],
> *
> *
Oh ok.
> **16)
> + 'System: Read Time Rule Template' should have also
> 'objectclass' as allowed attribute
Seems like it's already there.
> **
> /17)//
> ////
> ///+ File('accesstime', validate_icalfile,
> + cli_name='time',
> + label=_('Access time'),
> + ),
> I prefer to have file option named like icalfile, instead of
> accesstime, as you mentioned we may need to add more options to be UX
> friendly
>
> Should we have option, which will take ical string directly from CLI,
> IMO yes.
> (this applies for both, hbacrule, and timerule template)
While I might be wrong here, it seems that the api commands won't accept
newline as a part of a single input, separating the input as multivalued
by newlines. Typical iCalendar string is a lot of newlines.
I currently left the 'accesstime' option there. Probably, in the end,
there will have to be a separate method to load the content from an
iCalendar file and store it to the database. So the current accesstime
methods are mere placeholders.
>
> 18)
> IMO timeruletemplate should be in separate module, we may reuse this
> later, and I don't see why it should be in HBAC module*
> *
There's the validation part that's the same for both adding accesstime
from ical file to hbacrule and for adding it to timerule template. A
possible solution for splitting the modules would be to add a certain
class to parameters.py just for validation. Then timerule_templates
could have their own module as well as the validation would be usable in
multiple other places. There's probably another way around this, though.
Ideas are welcome :)
> *****
> ***19)
> + Str('timeruleclass*',
> + cli_name='class'),
>
> why option --class? it does not look descriptive enough for me. How
> about --timerule-template. Also timeruletemplate instead of
> timeruleclass looks better to me.
> *
> *
That seems right, modified it to timeruletemplate.
> ****20)
> + result = ldap.get_entry(DN(('cn',
> options['timeruleclass'][0]),
> + ('cn', 'timeruletemplates'),
> + api.env.basedn))
> Please use constants for container timeruletemplates
> result is unused var, remove it please.
Done.
> *****
> ***
> I have to look closer to icalendar and DS templates :)
> So review is not finished, but feel free to fix issues I listed.
For iCalendar, there's of course the RFC:
http://tools.ietf.org/html/rfc5545 and the Class of Service is
thoroughly described at
https://access.redhat.com/documentation/en-US/Red_Hat_Directory_Server/9.0/html/Administration_Guide/Advanced_Entry_Management-Assigning_Class_of_Service.html.
>
> Martin^2
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160330/7b12a5b6/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-HBAC-Access-Time-Rules-icalendar-format-validation.patch
Type: text/x-patch
Size: 15665 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160330/7b12a5b6/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Templating-of-access-time-rules-for-HBAC.patch
Type: text/x-patch
Size: 24104 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160330/7b12a5b6/attachment-0001.bin>
More information about the Freeipa-devel
mailing list