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

Fri Mar 2 18:43:42 UTC 2012

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