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

Fri Mar 2 16:40:07 UTC 2012

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-919-5-sudo.patch
Type: text/x-diff
Size: 15950 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120302/a092e049/attachment.bin>