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

Thu Mar 1 19:57:20 UTC 2012

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.
>
> rob

And now with updated API.txt. Forgot to squash the commit.

rob

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