[Freeipa-devel] [PATCH] 918, 919 update sudo schema

Fri Mar 2 19:02:38 UTC 2012

On Fri, 2012-03-02 at 20:01 +0100, Jan Cholasta wrote:
> On 2.3.2012 19:43, Rob Crittenden wrote:
> > Martin Kosek wrote:
> >> On Fri, 2012-03-02 at 11:40 -0500, Rob Crittenden wrote:
> >>> Jan Cholasta wrote:
> >>>> On 1.3.2012 20:57, Rob Crittenden wrote:
> >>>>> Rob Crittenden wrote:
> >>>>>> Jan Cholasta wrote:
> >>>>>>> On 17.1.2012 04:55, Rob Crittenden wrote:
> >>>>>>>> Jan Cholasta wrote:
> >>>>>>>>> Dne 13.1.2012 17:39, Rob Crittenden napsal(a):
> >>>>>>>>>> Jan Cholasta wrote:
> >>>>>>>>>>> Dne 14.12.2011 16:21, Rob Crittenden napsal(a):
> >>>>>>>>>>>> Jan Cholasta wrote:
> >>>>>>>>>>>>> Dne 14.12.2011 15:23, Rob Crittenden napsal(a):
> >>>>>>>>>>>>>> Jan Cholasta wrote:
> >>>>>>>>>>>>>>> Dne 14.12.2011 05:20, Rob Crittenden napsal(a):
> >>>>>>>>>>>>>>>> The sudo schema now defines sudoOrder, sudoNotBefore and
> >>>>>>>>>>>>>>>> sudoNotAfter
> >>>>>>>>>>>>>>>> but these weren't available in the sudorule plugin.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> I've added support for these. sudoOrder enforces uniqueness
> >>>>>>>>>>>>>>>> because
> >>>>>>>>>>>>>>>> duplicates are undefined.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> I also added support for a GeneralizedTime parameter type.
> >>>>>>>>>>>>>>>> This is
> >>>>>>>>>>>>>>>> similar to the existing AccessTime parameter but it only
> >>>>>>>>>>>>>>>> handles a
> >>>>>>>>>>>>>>>> single time value.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> You should parse the date/time part of the value with
> >>>>>>>>>>>>>>> time.strptime(timestr, '%Y%m%d%H%M%S') instead of doing it
> >>>>>>>>>>>>>>> manually,
> >>>>>>>>>>>>>>> that way you'll get most of the validation for free.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Yes but it gives a crappy error message, just saying that
> >>>>>>>>>>>>>> some
> >>>>>>>>>>>>>> data is
> >>>>>>>>>>>>>> left over not what is wrong.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> IMHO having a separate error message for every field in the
> >>>>>>>>>>>>> time
> >>>>>>>>>>>>> string
> >>>>>>>>>>>>> (like you do in the patch) is an overkill, simple "invalid
> >>>>>>>>>>>>> time"
> >>>>>>>>>>>>> and/or
> >>>>>>>>>>>>> "unknown time format" should suffice (we don't have errors
> >>>>>>>>>>>>> like
> >>>>>>>>>>>>> "invalid
> >>>>>>>>>>>>> 3rd octet" for IP adresses either).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Well, the work is done, hard to go back on a better error
> >>>>>>>>>>>> message.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Also, it would be nice to be able to enter the value in more
> >>>>>>>>>>>>>>> user-friendly format (e.g. "2011-12-14 13:01:25 +0100") and
> >>>>>>>>>>>>>>> normalize
> >>>>>>>>>>>>>>> that to LDAP generalized time.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> When dealing with time there are so many ways to input and
> >>>>>>>>>>>>>> display
> >>>>>>>>>>>>>> the
> >>>>>>>>>>>>>> same values this becomes difficult.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I'd expect that the times for these two attributes will be
> >>>>>>>>>>>>>> relatively
> >>>>>>>>>>>>>> simple and I somehow doubt users are going to want
> >>>>>>>>>>>>>> seconds, leap
> >>>>>>>>>>>>>> seconds
> >>>>>>>>>>>>>> or fractions, but we'll need to consider how to do it for
> >>>>>>>>>>>>>> future
> >>>>>>>>>>>>>> consistency (otherwise we could have a case where time is
> >>>>>>>>>>>>>> entered in
> >>>>>>>>>>>>>> one
> >>>>>>>>>>>>>> format for some attributes and another for others).
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> If we input in a nice way we need to output in the same way.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> We could make the preferred input/output time format
> >>>>>>>>>>>>> user-configurable,
> >>>>>>>>>>>>> defaulting to current locale time format. This format would be
> >>>>>>>>>>>>> used
> >>>>>>>>>>>>> for
> >>>>>>>>>>>>> output. For input, we could go over a list of formats
> >>>>>>>>>>>>> (first the
> >>>>>>>>>>>>> user-configured format, then current locale format, then a
> >>>>>>>>>>>>> handful of
> >>>>>>>>>>>>> "standard" formats like YYYY-MM-DD HH:MM:SS) and use the first
> >>>>>>>>>>>>> format
> >>>>>>>>>>>>> that can be successfully used to parse the time string.
> >>>>>>>>>>>>
> >>>>>>>>>>>> See how far you get into the rabbit hole with even this simple
> >>>>>>>>>>>> format?
> >>>>>>>>>>>
> >>>>>>>>>>> I don't mind, as long as it is the right thing to do (IMHO) :)
> >>>>>>>>>>>
> >>>>>>>>>>> Anyway, I think this could be done on the client side, so we
> >>>>>>>>>>> might
> >>>>>>>>>>> use
> >>>>>>>>>>> your patch without changes. However, I would prefer if the
> >>>>>>>>>>> parameter
> >>>>>>>>>>> class was more generic, so we could use it (hypothetically) to
> >>>>>>>>>>> store
> >>>>>>>>>>> time in some other way than LDAP generalized time attribute (at
> >>>>>>>>>>> least
> >>>>>>>>>>> name it DateTime please).
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Ok, I'm fine with that.
> >>>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> The LDAP GeneralizedTime needs to be either in GMT or include a
> >>>>>>>>>>>> differential. This gets us into the territory where the client
> >>>>>>>>>>>> could be
> >>>>>>>>>>>> in a different timezone than the server which leads us to
> >>>>>>>>>>>> why we
> >>>>>>>>>>>> dropped
> >>>>>>>>>>>> AccessTime in the first place.
> >>>>>>>>>>>
> >>>>>>>>>>> Speaking of time zones, the differential alone is not a
> >>>>>>>>>>> sufficient
> >>>>>>>>>>> time
> >>>>>>>>>>> zone description, as it doesn't account for DST. Is there a
> >>>>>>>>>>> way to
> >>>>>>>>>>> store
> >>>>>>>>>>> time in LDAP with full time zone name (just in case it's needed
> >>>>>>>>>>> sometime
> >>>>>>>>>>> in future)?
> >>>>>>>>>>
> >>>>>>>>>> There is no way to store DST in LDAP (probably for good reason).
> >>>>>>>>>> Oddly
> >>>>>>>>>> enough the older LDAP v3 RFC (2252) strongly recommends using
> >>>>>>>>>> only
> >>>>>>>>>> GMT
> >>>>>>>>>> but the RFC that obsoletes it (4517) does not include this.
> >>>>>>>>>
> >>>>>>>>> Thanks for the info.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> So I'd like the user to supply the
> >>>>>>>>>>>> timezone themselves so I don't have to guess (wrongly) and let
> >>>>>>>>>>>> them
> >>>>>>>>>>>> worry about differing timezones.
> >>>>>>>>>>>
> >>>>>>>>>>> We don't have to guess, IIRC there is a way to get the local
> >>>>>>>>>>> timezone
> >>>>>>>>>>> differential in both Python and JavaScript, so the client could
> >>>>>>>>>>> supply
> >>>>>>>>>>> it automatically if necessary.
> >>>>>>>>>>
> >>>>>>>>>> I was thinking more about non-IPA clients (like sudo and
> >>>>>>>>>> notBefore).
> >>>>>>>>>
> >>>>>>>>> I think this can still be done at least in CLI, but it could be
> >>>>>>>>> done in
> >>>>>>>>> a separate patch.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Updated patches attached.
> >>>>>>>>>>
> >>>>>>>>>> rob
> >>>>>>>>>
> >>>>>>>>> Patch 919 doesn't cleanly apply on current master (neither does
> >>>>>>>>> 916
> >>>>>>>>> BTW).
> >>>>>>>>>
> >>>>>>>>> Honza
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Rebased patch (and 916 too, separately).
> >>>>>>>>
> >>>>>>>> rob
> >>>>>>>
> >>>>>>> Patch 918:
> >>>>>>>
> >>>>>>> 1. LDAP generalized time allows you to omit minutes from time zone
> >>>>>>> differential, your code treats such values as invalid
> >>>>>>>
> >>>>>>> 2. IMO a better pattern could be used, such as
> >>>>>>> u'^([0-9]{2}){5,7}([.,][0-9]+)?([-+]([0-9]{2}){1,2}|Z)$'
> >>>>>>>
> >>>>>>> 3. 20120229000Z has malformed minutes, but the error message says
> >>>>>>> "Malformed seconds"
> >>>>>>>
> >>>>>>> 4. 20120229000+0000 has malformed minutes, but the error message
> >>>>>>> says
> >>>>>>> "Missing operator for differential or malformed time string"
> >>>>>>>
> >>>>>>> 5. 201202290000+0000 is valid generalized time, but it causes
> >>>>>>> "Missing
> >>>>>>> operator for differential or malformed time string" error
> >>>>>>>
> >>>>>>> 6. Invalid month/day combinations (such as 201202310000Z) are
> >>>>>>> treated as
> >>>>>>> valid
> >>>>>>>
> >>>>>>> 7. When + or - is missing, the error message says "Missing operator
> >>>>>>> ..."
> >>>>>>> - IMO a better term than "operator" is "sign" in this case
> >>>>>>>
> >>>>>>> 8. The DateTime docstring includes grammar definition for MINUS, but
> >>>>>>> not
> >>>>>>> for PLUS, DOT or COMMA.
> >>>>>>>
> >>>>>>> 9. I'm not too fond of the _rule_required hack. Can't the same
> >>>>>>> thing be
> >>>>>>> done in _validate_scalar?
> >>>>>>>
> >>>>>>> 10. pattern_errmsg should say "Must be of the form
> >>>>>>> YYYYMMDDHH[MM]Z or
> >>>>>>> YYYYMMDDHH[MM]{+|-}HHMM"
> >>>>>>>
> >>>>>>> There might be more bugs in validation that I didn't discover. I
> >>>>>>> would
> >>>>>>> suggest you to use a more pythonic approach for the validation code
> >>>>>>> (e.g. use partition() to split the time zone and fraction from
> >>>>>>> the rest
> >>>>>>> of the value, etc.), the current code is rather C-ish, hard to
> >>>>>>> read and
> >>>>>>> apparently error-prone.
> >>>>>>>
> >>>>>>>
> >>>>>>> Patch 919:
> >>>>>>>
> >>>>>>> 1. sudoorder uniqueness is not properly checked in ipa sudorule-mod:
> >>>>>>>
> >>>>>>> $ ipa sudorule-add rule1
> >>>>>>> -----------------------
> >>>>>>> Added Sudo Rule "rule1"
> >>>>>>> -----------------------
> >>>>>>> Rule name: rule1
> >>>>>>> Enabled: TRUE
> >>>>>>>
> >>>>>>> $ ipa sudorule-add rule2 --order=0
> >>>>>>> -----------------------
> >>>>>>> Added Sudo Rule "rule2"
> >>>>>>> -----------------------
> >>>>>>> Rule name: rule2
> >>>>>>> Enabled: TRUE
> >>>>>>> Sudo order: 0
> >>>>>>>
> >>>>>>> $ ipa sudorule-add rule3 --order=0
> >>>>>>> ipa: ERROR: invalid 'order': order must be a unique value (0 already
> >>>>>>> used by rule2)
> >>>>>>>
> >>>>>>> $ ipa sudorule-mod rule1 --order=0
> >>>>>>> --------------------------
> >>>>>>> Modified Sudo Rule "rule1"
> >>>>>>> --------------------------
> >>>>>>> Rule name: rule1
> >>>>>>> Enabled: TRUE
> >>>>>>> Sudo order: 0
> >>>>>>>
> >>>>>>> (now both rule1 and rule2 have sudoorder=0)
> >>>>>>>
> >>>>>>> 2. Shouldn't we check that sudonotbefore<= sudonotafter?
> >>>>>>>
> >>>>>>> 3. sudonotbefore param doc should say "Start of time interval for
> >>>>>>> which
> >>>>>>> the entry is valid (YYYYMMDDHH[MM]Z)"
> >>>>>>>
> >>>>>>> 4. sudonotafter param doc should say "End of time interval for which
> >>>>>>> the
> >>>>>>> entry is valid (YYYYMMDDHH[MM]Z)"
> >>>>>>>
> >>>>>>> 5. In 60ipasudo.ldif, the ipaSudoRule object class is missing the
> >>>>>>> sudoOrder, sudoNotBefore and sudoNotAfter attributes. Is this OK?
> >>>>>>>
> >>>>>>>
> >>>>>>> Both patches need to be rebased.
> >>>>>>>
> >>>>>>> Honza
> >>>>>>>
> >>>>>>
> >>>>>> Ok, lets take this one step at a time.
> >>>>>>
> >>>>>> This updated patch adds schema for all three attributes but just a
> >>>>>> Param
> >>>>>> for sudoOrder. I'll have to revisit DateTime for notbefore and
> >>>>>> notafter.
> >>>>>>
> >>>>>> I addressed the issue with order and added a test for it.
> >>>>>>
> >>>>>> Schema is fixed on new installs.
> >>>>
> >>>> ipa-server-install fails with:
> >>>>
> >>>> [02/Mar/2012:09:13:08 +0100] dse_read_one_file - The entry cn=schema in
> >>>> file /etc/dirsrv/slapd-IDM-LAB-BOS-REDHAT-COM/schema/60ipasudo.ldif
> >>>> (lineno: 1) is invalid, error code 21 (Invalid syntax) - object class
> >>>> ipaSudoRule: Unknown allowed attribute type "sudoNotBefore"
> >>>> [02/Mar/2012:09:13:08 +0100] dse - Please edit the file to correct the
> >>>> reported problems and then restart the server.
> >>>>
> >>>>>>
> >>>>>> rob
> >>>>>
> >>>>> And now with updated API.txt. Forgot to squash the commit.
> >>>>>
> >>>>> rob
> >>>>>
> >>>>
> >>>> Honza
> >>>>
> >>>
> >>> And now the right patch
> >>
> >> The issue with clean install is that the sudo attribute types are
> >> shipped in 60sudo.ldif schema, but our schema 60ipasudo.ldif which uses
> >> some attributeTypes defined in 60sudo.ldif is alphabetically before
> >> 60sudo.ldif and is this processed first. That's why upgrade worked and
> >> clean install did not.
> >>
> >> ACK if you squash with the attached patch which renames 60ipasudo.ldif
> >> to 65ipasudo.ldif so that our schema is processed _after_ the bundled
> >> 60ipasudo.ldif.
> >>
> >> Martin
> >
> > Good idea, rolled into my patch.
> >
> > I hadn't fixed Honza's mod problem completely either. 0 was essentially
> > a special case because of the way I was looking to see if a sudoorder
> > change was being requested. I fixed that too and added a specific test
> > case for it.
> >
> > rob
> 
> ACK.
> 
> Honza
> 

ACK from me as well. Installation is now ok, no redundant
attributetypes.

Martin