<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> On 03/09/2016 05:24 PM, Martin Basti wrote: <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <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 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">https://github.com/collective/icalendar</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 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 </blockquote> 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. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 2) please add requires and buildrequires to specfile (python-icalendar) </blockquote> Done. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 </blockquote> Done, replaced the prints with root_logger. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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) </blockquote> Do you want me to fix this last 1 letter overflow as well? It would probably look worse if I did. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> ./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) </blockquote> 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. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> ./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 + </blockquote> Done. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 :) </blockquote> Good eye, it was actually a property of a component. Modified it a bit. Components may appear in components too, though. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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)) </blockquote> 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? <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 8) +�� # TODO: comp.required might be removed when +�� # <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> 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. </blockquote> 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. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 </blockquote> Done. I just followed the code from cert.py. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 10) +�� icalstr = ics this statement is useless, please use ics directly </blockquote> Done, of course. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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) </blockquote> It is an invalid icalfile. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> +�� 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? </blockquote> 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. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 </blockquote> You're right, modified it. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 </blockquote> Should be ok now. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 14) +�� container_dn = DN(('cn', 'timeruletemplates')) Please define this in constants.py </blockquote> Done. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 moz-do-not-send="true" 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>;)', +�� ], </blockquote> Oh ok. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 16) +�� 'System: Read Time Rule Template'� should have also 'objectclass' as allowed attribute </blockquote> Seems like it's already there. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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) </blockquote> 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. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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 </blockquote> 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 :) <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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. </blockquote> That seems right, modified it to timeruletemplate. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> 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. </blockquote> Done. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> I have to look closer to icalendar and DS templates :) So review is not finished, but feel free to fix issues I listed. </blockquote> For iCalendar, there's of course the RFC: <a class="moz-txt-link-freetext" href="http://tools.ietf.org/html/rfc5545"><a class="moz-txt-link-freetext" href="http://tools.ietf.org/html/rfc5545">http://tools.ietf.org/html/rfc5545</a></a> and� the Class of Service is thoroughly described at <a class="moz-txt-link-freetext" href="https://access.redhat.com/documentation/en-US/Red_Hat_Directory_Server/9.0/html/Administration_Guide/Advanced_Entry_Management-Assigning_Class_of_Service.html">https://access.redhat.com/documentation/en-US/Red_Hat_Directory_Server/9.0/html/Administration_Guide/Advanced_Entry_Management-Assigning_Class_of_Service.html</a>. <blockquote cite="mid:56E04E38.8080804@redhat.com" type="cite"> Martin^2 </blockquote> </body> </html>