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

[Jan Cholasta]

On 2.3.2012 19:43, Rob Crittenden wrote:
> Martin Kosek wrote:
>> 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
>
> Good idea, rolled into my patch.
>
> I hadn't fixed Honza's mod problem completely either. 0 was essentially
> a special case because of the way I was looking to see if a sudoorder
> change was being requested. I fixed that too and added a specific test
> case for it.
>
> rob

ACK.

Honza

-- 
Jan Cholasta