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

[Rob Crittenden]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-919-3-sudo.patch
Type: text/x-diff
Size: 10621 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120301/2d7d8490/attachment.bin>