RE: ITS #7161, ppolicy pwdFailureTime resolution should be better than 1 second

["Paul B. Henson" <henson@acm.org>]

> From: Howard Chu
> Sent: Friday, May 23, 2014 5:30 PM
>
> you have completely changed the semantics
> of the timestamp. The code explicitly used "now" - the current time when
the
> operation completed, and you have changed it to use op->o_time, which is
> the time when the operation was first received. You said all you wanted to
do
> was add microseconds, but this patch changes much more than that.

Yes, I almost went into more detail on that when I posted, I suppose I
probably should have.

During my testing, the seconds in op->o_time were never different than now
was set to by a call to time(). Assuming an operation never takes more than
one second to complete, if you are working with a one second granularity,
the start time and the stop time are roughly equivalent, never differing by
more than one second (ie, start at 10.8, stop at 11.2, use 11 rather than
10). What is the worst case amount of time it would take to perform a bind
operation? The change made was in ppolicy_bind_response, which unless I
misunderstand, is only called for the bind operation, so the change in
semantic time definition would only apply to binds, and impact the timestamp
stored in pwdFailureTime, whether or not a given pwdFailureTime value was
recent enough to consider, and whether or not a password was considered
expired.

Further, while the semantics did change, is the new definition just
different, or worse than the previous? Was there a specific reason it was
decided to mark the time of failure as when the bind operation completed as
opposed to when it began? What benefit does it provide?

I originally did intend to add microseconds to the existing call to time,
but after reviewing other uses of fractional seconds, such as in the
accesslog module, it seemed more convenient to use the op->o_tincr, which
while not time-based also allows you to distinguish operations at a
subsecond granularity. For the purpose of keeping track of the failures, it
seemed not so important you actually logged the exact microsecond the
failure occurred, but simply that you are able to distinguish multiple
failures within one second. I considered keeping the call to time(), and
simply adding op->o_tincr on to that, but then I would potentially be mixing
time intervals.

If you want to keep the same semantics of the time being at the end of the
operation rather than at the start, is it acceptable to call
ldap_pvt_gettime from this module, and then call lutil_tm2time to get a
lutil_timet containing both seconds and microseconds? ldap_pvt_gettime
involves a mutex, and seemed like would be less efficient to use than the
existing time and operation increment that was already present, given that a
second or two delta didn't really seem to make a difference for the use that
was made of the time.

> Aside from that, there's no reason to make a 2nd redundant call to
> slap_timestamp - just copy the result from timestr to failtimestr.

You're right, I did not consider that. I will make that change.

> Rejecting this patch.

Thank you for the feedback.