<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 04.03.2016 14:53, Stanislav Laznicka wrote: </div> <blockquote cite="mid:56D9935D.9050507@redhat.com" type="cite"> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> Hello, So in the previous month and a bit I was reworking the time-based policies according to the changes we agreed on (<a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://pad.engineering.redhat.com/ipa-time-based-HBAC-design"><a class="moz-txt-link-freetext" href="http://pad.engineering.redhat.com/ipa-time-based-HBAC-design">http://pad.engineering.redhat.com/ipa-time-based-HBAC-design</a></a>, 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 (<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/collective/icalendar"><a class="moz-txt-link-freetext" href="https://github.com/collective/icalendar">https://github.com/collective/icalendar</a></a>), one should be pushed in the next library major release. My pull requests: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/collective/icalendar/pull/175">https://github.com/collective/icalendar/pull/175</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/collective/icalendar/pull/179">https://github.com/collective/icalendar/pull/179</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/collective/icalendar/pull/180">https://github.com/collective/icalendar/pull/180</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/collective/icalendar/pull/183">https://github.com/collective/icalendar/pull/183</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/collective/icalendar/pull/189">https://github.com/collective/icalendar/pull/189</a> 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. <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://jkbrzt.github.io/rrule/"><a class="moz-txt-link-freetext" href="http://jkbrzt.github.io/rrule/">http://jkbrzt.github.io/rrule/</a></a> 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 (<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://github.com/libical/libical">https://github.com/libical/libical</a>), 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 <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> 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 2) please add requires and buildrequires to specfile (python-icalendar) 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 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) ./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) ./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 + 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 :) 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)) 8) + # TODO: comp.required might be removed when + # <a class="moz-txt-link-freetext" href="https://github.com/collective/icalendar/pull/183">https://github.com/collective/icalendar/pull/183</a> 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. 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 10) + icalstr = ics this statement is useless, please use ics directly 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) + 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? 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 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 14) + container_dn = DN(('cn', 'timeruletemplates')) Please define this in constants.py 15) Your managed ACI are completely new, so there should not be 'replaces' definition (several times) + 'replaces': [ + '(target = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=*,cn=timeruletemplates,$SUFFIX">"ldap:///cn=*,cn=timeruletemplates,$SUFFIX"</a>)(version 3.0;acl "permission:Delete Time Rule Template";allow (delete) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=DeleteTimeRuleTemplate,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Delete Time Rule Template,cn=permissions,cn=pbac,$SUFFIX"</a>;)', + ], 16) + 'System: Read Time Rule Template' should have also 'objectclass' as allowed attribute 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) 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 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. 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. I have to look closer to icalendar and DS templates :) So review is not finished, but feel free to fix issues I listed. Martin^2 </body> </html>