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

Fri Mar 2 19:16:03 UTC 2012

Martin Kosek wrote:
> On Fri, 2012-03-02 at 20:01 +0100, Jan Cholasta wrote:
>> 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
>>
>
> ACK from me as well. Installation is now ok, no redundant
> attributetypes.
>
> Martin
>

I made one minor change before pushing. I set the minvalue for sudoorder 
to 0, < 0 is undefined.

Pushed to master and ipa-2-2

rob