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

Fri Mar 2 08:19:01 UTC 2012

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

-- 
Jan Cholasta