Bug 2107380

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;
         }
     }

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.

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.

(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

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

Anton, would you like to open a pull request with your patch?

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?

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.

Upstream PR https://github.com/SSSD/sssd/pull/6351

Thanks for this PR Anton, I'll keep an eye on this!

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

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