Univention Bugzilla – Bug 41574
Handling of userexpiry is unexact
Last modified: 2021-12-08 19:18:59 CET
From Bug #36210 (In reply to Stefan Gohmann from comment #4) > (In reply to Philipp Hahn from comment #3) > > Maybe important: The EC2 tests run in > > # cat /etc/timezone > > America/New_York > > Looks like a connector bug: > > >>> s42unix_time(unix2s4_time('2016-11-09')) > '10.11.16' > >>> > > See also Bug #32227. (In reply to Philipp Hahn from comment #2) > The OpenLDAP-access-log shows "shadowExpire" being modified +1 several times: > $ grep shadowExpire ~/BUG/36210_udm-shadowExpire.log > reqMod: shadowExpire:+ 16958 > reqMod: shadowExpire:= 16959 > reqMod: shadowExpire:= 16960 > reqMod: shadowExpire:= 16961 > reqMod: shadowExpire:= 16962 > reqMod: shadowExpire:= 16963 > reqMod: shadowExpire:= 16964 > reqMod: shadowExpire:= 16965 > reqMod: shadowExpire:= 16966 +++ This bug was initially created as a clone of Bug #36210 +++
(In reply to Stefan Gohmann from comment #4) > (In reply to Philipp Hahn from comment #3) > > Maybe important: The EC2 tests run in > > # cat /etc/timezone > > America/New_York > > Looks like a connector bug: > > >>> s42unix_time(unix2s4_time('2016-11-09')) > '10.11.16' Logging at the source code: >>> def unix2s4_time(l): ... d=116444736000000000L #difference between 1601 and 1970 ... return long(time.mktime(time.gmtime(time.mktime(time.strptime(l,"%Y-%m-%d"))+90000)))*10000000+d # 90000s are one day and one hour So it adds 1d1h, but why??? Also it converts a Y-m-d-string into a number; why does is need a mktime(gmtime()) extra-round? >>> def s42unix_time(l): ... d=116444736000000000L #difference between 1601 and 1970 ... return time.strftime("%d.%m.%y",time.gmtime((l-d)/10000000)) From <https://docs.python.org/2.7/library/time.html>: > Use the following functions to convert between time representations: > From | To | Use > seconds since the epoch | struct_time in UTC | gmtime() > seconds since the epoch | struct_time in local time | localtime() > struct_time in UTC | seconds since the epoch | calendar.timegm() > struct_time in local time | seconds since the epoch | mktime() So the mix of mktime() with gmtime() looks wrong and the "one hour" offset is only correct for winter in Europe. Verify the constants: >>> 116444736000000000L == long((datetime.date(1970,1,1) - datetime.date(1601,1,1)).total_seconds() * 1000 * 1000 * 10) True >>> 90000 == (24 + 1) * 60 * 60 True SEC_PER_DAY = 24 * 60 * 60 S4_TICK_PER_SEC = 10 * 1000 * 1000 # 100 ns DELTA_1601_1970 = long((datetime.date(1970,1,1) - datetime.date(1601,1,1)).total_seconds() * S4_TICK_PER_SEC)
Happend again in UCS@school jenkins environment: http://jenkins.knut.univention.de:8080/job/UCSschool%204.2/job/UCSschool%204.2%20Singleserver/ImportTests=ImportTests,SambaVersion=s4/27/testReport/junit/90_ucsschool/216_import-users_delete_variants/test/
Please reenable some code lines, that have been disabled in ucs-school-4.2/ucs-test-ucsschool/90_ucsschool/216_import-users_delete_variants if this bug has been fixed.
Created attachment 9095 [details] 41574-s4c-user-expiry-421.patch This fixes the timestamp conversions in the S4C. Those are no longer concerned with timezones. This, in conjunction with the fix for bug #36210, does not yet fix `90_ucsschool/216_import-users_delete_variants`. I need to investigate further.
(In reply to Lukas Oyen from comment #4) > This, in conjunction with the fix for bug #36210, does not yet fix > `90_ucsschool/216_import-users_delete_variants`. I need to investigate > further. It does, I must have tested it wrong.
Code rebased on 4.2-2 in branch loyen/41574-s4c-excact-user-expiry-422.
i think the test /usr/share/ucs-test/runner python -f /usr/share/ucs-test/60_umc/31_udm_user_authentication fails because of this issue
Created attachment 9407 [details] test_userexpiry_in_other_timezone.sh Testscript showing the problem.
Created attachment 9408 [details] test_userexpiry_in_other_timezone.sh Test only works with S4-Connector, now checks that it is installed.
Created attachment 9412 [details] userexpiry2accountExpires.patch The attached patch has this goal: 1. Fix auto-incrementing replication cycle in S4-Connector in other timezones: unix2s4_time statically added one hour. 2. Since input for userexpiry is interpreted as localtime when writing to LDAP, the LDAP data should be output as localtime too. Otherwise the S4-Connector, the Admin and our tests always need to convert again manually to their local timezone to avoid being off by one day when comparing to some other localtime value. 3. The AD interprets the accountExpires attribute as still allowing that day, userexpiry (or rather shadowExpire) is interpreted as excluding that day. So accountExpires should be set to userexpiry - 1.
(In reply to Arvid Requate from comment #10) > Created attachment 9412 [details] > userexpiry2accountExpires.patch > > The attached patch has this goal: > > 1. Fix auto-incrementing replication cycle in S4-Connector in other > timezones: > unix2s4_time statically added one hour. > > 2. Since input for userexpiry is interpreted as localtime when writing to > LDAP, the LDAP data should be output as localtime too. Otherwise the > S4-Connector, the Admin and our tests always need to convert again manually > to their local timezone to avoid being off by one day when comparing to some > other localtime value. patches look good, but i think there is a more underlying. We take a expiry date in the form of 2018-02-20 in localtime and convert this to utc, but as we (at least for krb5ValidEnd) ignore the hours, and minutes ... "time.strftime("%Y%m%d000000Z"..." we loose exactly this tz information expiry date 2018-02-20 localtime is -5, utc is +5 proper utc 20180220 05:00:00Z but we use 20180220000000Z (krb5ValidEnd), see the missing 5 hours now back to localtime with -5, hmm but that is the 2018-02-19 import time import calendar input_date = '2018-02-21' print input_date userexpiry_epoch = time.mktime(time.strptime(input_date, "%Y-%m-%d")) krb5ValidEnd = time.strftime("%Y%m%d000000Z", time.gmtime(userexpiry_epoch)) print krb5ValidEnd userexpiry_epoch = calendar.timegm(time.strptime(krb5ValidEnd, '%Y%m%d%H%M%SZ')) out = time.strftime("%Y-%m-%d", time.localtime(userexpiry_epoch)) print '000000Z ', out -> 2018-02-21 -> 20180221000000Z -> 000000Z 2018-02-20 so in goes 2018-02-20 and out come 2018-02-19, that does not seem right. What to do? (a) I think we could store the exact time in krb5ValidEnd, with hours and ..., that should do it as we now preserve the tz offset. (b) Or we could (as in windows) says the expiry date is the day after which a login is denied and convert the the expiry date day in utc + 23:59:59 Current default time zone: 'Asia/Dubai' Local time is now: Tue Feb 20 03:33:45 +04 2018. Universal Time is now: Mon Feb 19 23:33:45 UTC 2018. so if the tz offset is positiv utc can be yesterday, 2018-02-20 localtime -> to utc day (-4h) and add 23:59:59 -> 20180219235959Z utc but when converting back to localtime, the offset is added and we end up with 2018-02-20 (03:59:59, but this is ignored) again Current default time zone: 'America/New_York' Local time is now: Tue Feb 20 22:00:41 EST 2018. Universal Time is now: Wed Feb 21 03:00:41 UTC 2018. the other direction also works 2018-02-20 -> to utc (+5h) day and add 23:59:59 -> 20180220235959Z and back to localtime is -5 and we end up with 2018-02-20 (a) seems a bit more understandable to me so we have to decided how to store the expire date (1), we have to check the other attributes, sambaKickoffTime and shadowExpire (2) and we have to check how the services (heimdal, samba, pam) react to these new expire timestamps (3)
for UDM user expiry handling, see Bug # 36210 Here on this bug, we will only fix the s4/ad connector's accountExpires handling. During the sync to s4/ad UDM userexpiry is converted to a windows timestamp and 90000s are added. On the way back the 90000s are not subtracted and under certain circumstances this can lead to a sync loop.
uff, this timezone stuff drives me crazy i have merged arvids connector patches for unix2s4_time s42unix_time to the s4 connector (and to the ad connector) to s4: the local time from the user expiry date is converted to (AD) epoch with time.mktime back the reverse function time.localtime is used for the other direction, this should avoid sync loops i removed the timestamp + day stuff, in my tests with samba4 the login has been denied immediately aftert the accountexpiry date has been reached, so no need to add a day univention-s4-connector fceba3e7c5125a2e3486e539cfc4f5b847573717 univention-ad-connector 43e427dfcaf640c32842332deea3a98b90f986e3
Changelog: Failed, no changelog. Tests: The test case still fails: http://jenkins.knut.univention.de:8080/job/UCS-4.3/job/UCS-4.3-0/job/AutotestJoin/lastCompletedBuild/SambaVersion=s4,Systemrolle=master/testReport/60_umc/31_udm_user_authentication/test/ Code review: OK
reverted
(In reply to Felix Botner from comment #15) > reverted OK, this is b7a54e8d. As discussed, I still think we should fix it but it don't need to be part of the 4.3 release since it is not a regression. At least, as far as I could see. I'll reopen it and tag it to UCS 4.3-0-errata.
This issue has been filled against UCS 4.1. The maintenance with bug and security fixes for UCS 4.1 has ended on 5st of April 2018. Customers still on UCS 4.1 are encouraged to update to UCS 4.3. Please contact your partner or Univention for any questions. If this issue still occurs in newer UCS versions, please use "Clone this bug" or simply reopen the issue. In this case please provide detailed information on how this issue is affecting you.
customer affected
I rebased the branch to UCS 4.4-2: loyen/41574-s4c-excact-user-expiry-422. On top of it I took the renaming of the functions/cleanup from attachment 9412 [details]. Can we discuss what is wrong about the patch/approach from Lukas? Atm I think it still fits best, but am not sure how to integrate your/Arvids valid concerns: (In reply to Arvid Requate from comment #10) > Created attachment 9412 [details] > userexpiry2accountExpires.patch > > The attached patch has this goal: > > 1. Fix auto-incrementing replication cycle in S4-Connector in other > timezones: > unix2s4_time statically added one hour. This is already fixed by Bug #50202. The patch from Lukas removes the extra 86400 seconds, which is probably wrong. > 2. Since input for userexpiry is interpreted as localtime when writing to > LDAP, the LDAP data should be output as localtime too. Otherwise the > S4-Connector, the Admin and our tests always need to convert again manually > to their local timezone to avoid being off by one day when comparing to some > other localtime value. But we probably should not convert all the dates to localtime in users/user?! Instead we should make e.g. the UDM widget which displays a date has to convert that local time to UTC. The UDM REST API should also serve only pure UTC dates. The UDM-CLI client maybe should convert also local dates to UTC when accepting input and convert UTC to local dates when printing user information? > 3. The AD interprets the accountExpires attribute as still allowing that day, > userexpiry (or rather shadowExpire) is interpreted as excluding that day. > So accountExpires should be set to userexpiry - 1. I don't see where this is done in the patch? -» » » » shadowExpire = "%d" % long(time.mktime(time.strptime(self['userexpiry'], "%Y-%m-%d")) / 3600 / 24 + 1) +» » » » shadowExpire = "%d" % long(time.mktime(time.strptime(self['userexpiry'], "%Y-%m-%d")) / 3600 / 24) → is it this line? If yes, shouldn't this be done in the s4-connector code instead?
Regarding Comment 10: * We fixed Point 1 with Bug 50202 * Bug 53012 now has a suggested patch for point 2 (End of day error in accountExires plus localtime conversion) * Additionally there is Bug 46349 for the UDM specific question of timezone handling * I've now created Bug 54227 to clearly address the GUI differences between UMC/UDM and ADUC I think it makes sense to follow those specific bugs and close this omnibus bug.