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

Fri Mar 2 18:28:49 UTC 2012

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-rename-sudo-ldif.patch
Type: text/x-patch
Size: 2994 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120302/d99043da/attachment.bin>