[Freeipa-devel] [WIP] Time-Based HBAC Policies
Martin Basti
mbasti at redhat.com
Wed Mar 9 16:24:24 UTC 2016
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
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
+ # 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.
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 =
"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";)',
+ ],
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160309/b9b5781f/attachment.htm>
More information about the Freeipa-devel
mailing list