RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 2107380 - sssd timezone issues sudonotafter [rhel-7.9.z]
Summary: sssd timezone issues sudonotafter [rhel-7.9.z]
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: sssd
Version: 7.9
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: rc
: ---
Assignee: Pavel Březina
QA Contact: shridhar
URL:
Whiteboard: sync-to-jira
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-07-14 21:06 UTC by Jeremy Absher
Modified: 2022-12-13 11:19 UTC (History)
8 users (show)

Fixed In Version: sssd-1.16.5-10.el7_9.14
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-12-13 11:19:42 UTC
Target Upstream Version:
Embargoed:
pm-rhel: mirror+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Github SSSD sssd issues 6354 0 None open SUDO: Timezone issues with sudoNotBefore and sudoNotAfter 2022-09-20 18:10:57 UTC
Github SSSD sssd pull 6351 0 None open SUDO: Fix timezone issues with sudoNotBefore and sudoNotAfter 2022-09-20 18:10:56 UTC
Red Hat Issue Tracker RHELPLAN-127795 0 None None None 2022-07-14 21:06:56 UTC
Red Hat Issue Tracker SSSD-5034 0 None None None 2022-09-16 14:51:47 UTC
Red Hat Product Errata RHBA-2022:8950 0 None None None 2022-12-13 11:19:46 UTC

Comment 3 Anton Bobrov 2022-07-15 13:17:25 UTC
diff --git a/src/db/sysdb_sudo.c b/src/db/sysdb_sudo.c
index 1626b612d..391eb8ed6 100644
--- a/src/db/sysdb_sudo.c
+++ b/src/db/sysdb_sudo.c
@@ -76,9 +76,18 @@ static errno_t sysdb_sudo_convert_time(const char *str, time_t *unix_time)
     for (format = formats; *format != NULL; format++) {
         /* strptime() may leave some fields uninitialized */
         memset(&tm, 0, sizeof(struct tm));
+        /* Let underlying implementation figure out DST */ 
+        tm.tm_isdst = -1;
         tret = strptime(str, *format, &tm);
         if (tret != NULL && *tret == '\0') {
-            *unix_time = mktime(&tm);
+            /* Convert broken-down time to local time */
+            if (tm.tm_gmtoff == 0) {
+                *unix_time = timegm(&tm);
+            } else {
+                long offset = tm.tm_gmtoff;
+                tm.tm_gmtoff = 0;
+                *unix_time = timegm(&tm) + offset;
+            }
             return EOK;
         }
     }

Comment 4 Anton Bobrov 2022-07-15 13:25:08 UTC
The patch above should make sssd sudo respect generalized time as specified in related before/after attributes. The problem with the current implementation is that it essentially treats them as local time with no regard to TZ and DST. There might be a cleaner way of doing this (or maybe it should even be addressed in related glibc code) but this patch works with a limited manual testing so its a good starting point to figure out how this should be addressed.

Comment 5 Anton Bobrov 2022-07-15 13:28:00 UTC
And speaking of testing it would be great to add some basic unit tests in this area, they should catch things like that to begin with and also help troubleshooting as those TZ and DST related issues can be rather confusing.

Comment 12 Pavel Březina 2022-09-09 10:45:50 UTC
(In reply to Anton Bobrov from comment #3)
> diff --git a/src/db/sysdb_sudo.c b/src/db/sysdb_sudo.c
> index 1626b612d..391eb8ed6 100644
> --- a/src/db/sysdb_sudo.c
> +++ b/src/db/sysdb_sudo.c
> @@ -76,9 +76,18 @@ static errno_t sysdb_sudo_convert_time(const char *str,
> time_t *unix_time)
>      for (format = formats; *format != NULL; format++) {
>          /* strptime() may leave some fields uninitialized */
>          memset(&tm, 0, sizeof(struct tm));
> +        /* Let underlying implementation figure out DST */ 
> +        tm.tm_isdst = -1;
>          tret = strptime(str, *format, &tm);
>          if (tret != NULL && *tret == '\0') {
> -            *unix_time = mktime(&tm);
> +            /* Convert broken-down time to local time */
> +            if (tm.tm_gmtoff == 0) {
> +                *unix_time = timegm(&tm);
> +            } else {
> +                long offset = tm.tm_gmtoff;
> +                tm.tm_gmtoff = 0;
> +                *unix_time = timegm(&tm) + offset;
> +            }
>              return EOK;
>          }
>      }

Honestly, I find the time manipulation functions always puzzling and the man pages are silent about tm_gmtoff. This is ten years old code so I don't really have lots of memory here but if I read the code correctly then:

- We get the current time using time(NULL) which returns the epoch timestamp (ie UTC+0)
- We get the string representation from LDAP and convert it into tm and then to time_t, we want to get the time in UTC+0
  - I'm not really sure how and if strptime populates tm_gmtoff but from little googling:
  - [1] "This field describes the time zone that was used to compute this broken-down time value, including any adjustment for daylight saving; it is the number of seconds that you must add to UTC to get local time. You can also think of this as the number of seconds east of UTC. For example, for U.S. Eastern Standard Time, the value is -5*60*60. The tm_gmtoff field is derived from BSD and is a GNU library extension; it is not visible in a strict ISO C environmen"
  - From this it seems that strptime populates tm as UTC and adds time offset to tm_gmtoff (ie strptime() == UTC and strptime() + tm_gmtoff == original time)

mktime converts to local time, which is wrong
adding offset seems wrong to me since the time should already be in UTC+0
so perhaps we just want to replace mktime with timegm?

[1] https://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html

Comment 13 Anton Bobrov 2022-09-09 12:01:47 UTC
yeah, they are pretty confusing. the way I have arrived at that patch is basically by debugging it and looking at live behavior in addition to bits and pieces that the docs mention here and there. i suggest you do the same by isolating affected code (sysdb_sudo_convert_time etc) as standalone program and just stepping thru it with a few test values like the ones in the bug description. that should make it somewhat clear to you how it actually works and behaves, it did for me.

wrt your broken down time comment. at the GNU manual link you have provided, right at the top it says, quote:

"A broken-down time value is always relative to a choice of time zone, and it also indicates which time zone that is."

(In reply to Pavel Březina from comment #12)

> Honestly, I find the time manipulation functions always puzzling and the man
> pages are silent about tm_gmtoff. This is ten years old code so I don't
> really have lots of memory here but if I read the code correctly then:

> mktime converts to local time, which is wrong
> adding offset seems wrong to me since the time should already be in UTC+0
> so perhaps we just want to replace mktime with timegm?
> 
> [1]
> https://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html

Comment 14 Pavel Březina 2022-09-12 09:06:41 UTC
Anton, would you like to open a pull request with your patch?

Comment 15 Anton Bobrov 2022-09-13 13:02:04 UTC
sure, will do. i do not have a unit test for this however and i'm not even sure if it can be tested via unit test or would require a functional test, depending on how you guys test these things (haven't looked into that).

(In reply to Pavel Březina from comment #14)
> Anton, would you like to open a pull request with your patch?

Comment 16 Pavel Březina 2022-09-13 14:03:55 UTC
Unit test is preferable, we use cmocka framework for that. It can be possible to mock the glibc functions using --wrap to get expected inputs, see test_dyndns.c (source file and in makefile) for example.

Comment 18 Anton Bobrov 2022-09-19 17:07:35 UTC
Upstream PR https://github.com/SSSD/sssd/pull/6351

Comment 19 René Jr Purcell 2022-09-19 18:05:47 UTC
Thanks for this PR Anton, I'll keep an eye on this!

Comment 20 Alexey Tikhonov 2022-09-27 17:57:24 UTC
Pushed PR: https://github.com/SSSD/sssd/pull/6351

* `master`
    * 0198f64ce231e9608b14152c64426fb9e015fd33 -  SUDO: Fix timezone issues with sudoNotBefore and sudoNotAfter

* `sssd-1-16`
    * 1bb93f70de9907d88b2ebc5c6ffee14417d90fee -  SUDO: Fix timezone issues with sudoNotBefore and sudoNotAfter

* `sssd-2-7`
    * 9a8b9252b2f15b3aa78dde484da6fad05cb44bfa - SUDO: Fix timezone issues with sudoNotBefore and sudoNotAfter

Comment 45 errata-xmlrpc 2022-12-13 11:19:42 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (sssd bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2022:8950


Note You need to log in before you can comment on or make changes to this bug.