Hi. To help in PR review it was suggested some explanation/considerations be added, so I try to elaborate below:
The direct issue that the patch tries to address is to restore the apparent intent of the XrdCryptosslASN1toUTC to convert the ASN1 date (with caveat noted later) to UTC, specifically returning the UTC value in the time_t type, presumably to be understood as the usual unix-utc-time-since-epoch:
I show the inputs and outputs of the existing XrdCryptosslASN1toUTC() function to illustrate how it currently works: I consider some chosen ASN1 dates (e.g. the string content of tsn1->data), for some dates chosen to cross the DST timezone changes for a region, e.g. NY in USA.
(with TZ="America/New_York" set in the environment)
The "Z" in the date string indicates the time is in UTC: In the list below I've evaluated the existing function, and then in parenthesis indicated this value minus the actual epoch-time for this date (as looked up elsewhere),
(localtime skips forward, DST comes into effect on 12 March):
230312001001Z => 1678597801 (result-real = 18000)
230312011001Z => 1678601401 (result-real = 18000)
230312021001Z => 1678608601 (result-real = 21600)
230312031001Z => 1678608601 (result-real = 18000)
230312041001Z => 1678612201 (result-real = 18000)
230312051001Z => 1678615801 (result-real = 18000)
(localtime skips backwards, DST will end 5 Nov):
231105001001Z => 1699161001 (result-real = 18000)
231105011001Z => 1699164601 (result-real = 18000)
231105021001Z => 1699168201 (result-real = 18000)
231105031001Z => 1699171801 (result-real = 18000)
231105041001Z => 1699175401 (result-real = 18000)
231105051001Z => 1699179001 (result-real = 18000)
the result of the function XrdCryptoTZCorr() for this TZ is -18000 (this value is independent of the date during the year on which it is evaluated). Most of the existing uses of the return value from XrdCryptosslASN1toUTC() involve comparing it to the result from time(0) where the "XrdCryptoTZCorr()" is subtracted from the time(0) before the compare. So in a roundabout way all the comparisons are correct except for those during the hour after the DST shift forward, where the XrdCryptosslASN1toUTC() is returning a date 1 hour later than intended.
The reported problems are understood to be due to the notBefore times (of the proxy part of the chain) that lie in this range being seen as sufficiently in the future to not be valid:
The patch then intends to
(i) return unix-time-since-epoch (i.e. utc, in the time_t type) time from XrdCryptosslASN1toUTC function
(ii) remove additional corrections applied to comparisons that involve the return value.
Other considerations: Not all places where the return value was used were corrected with XrdCryptoTZCorr(). i.e. the use of the CRL NextUpdate() date/time. So this patch will change what is evaluated at those places. However I believe this controls only a DEBUG warning message ("WARNING: CRL is expired: you should download the updated one"), and presumably we already are comparing this with various offsets (across our sites in various timezones).
The ASN1 notation has some types, including UTCTime and GeneralizedTime. (For the former there is a 2 digit year, and the later uses a four digit year starting with '20'). RFC5280 gives some additional restrictions on the ASN1 date used for x509, e.g. no fraction of seconds, always specify the 'Z' and, for dates before 2050 UTCTime must be used and after then GeneralizedTime must be used. Our XrdCryptosslASN1toUTC function assumes and will only parse the UTCTime type. I thought to leave this as is for now. The largest dates in the future I believe are for the expiration dates of CA; e.g. cern is currently using one with validy 2013-2023. So later this year I guess it will use one with a notAfter date 2033.
--
How it works: In the patch I added a comment about the independence of XrdCryptoTZCorr() with time of year, because I think there was some suspecion this calculation would vary over the year (and so not be suitable for the current evaluate-once approach). The function find the difference between two dates with two routes:
(i) time(0) ->localtime_r() -> mktime() => result is a time_t
(ii) time(0) -> gmtime_r() -> mktime() => result is a time_t
In route (i) the result should always be the same of the input, i.e. the current unix-utc-epoch time. The second interprets and struct tm, in UTC (i.e. hour/min/seconds in UTC, isdst=0) and converts with mktime(), which interpres as a localtime. i.e. a localtime with isdst=0. So the result is time difference between localtime (assuming DST=0) and UTC. (So does not change accross DST switch).
The existing XrdCryptosslASN1toUTC() also interprets the UTC string from tsn1->data as a localtime with mktime(). It is noted in the existing code-comment that mktime() is more portable than timegm(). timegm() would otherwise seem to be the natural choice here. However the DST conversion is left to mktime() to detect when to apply (setting tm_isdst=-1 on input), and then the application of this conversion is undone again (by the line "if (ltm.tm_isdst > 0) ...") using the returned value of tm_isdst.
e.g. here is the behaiour over the DST change forward:
mktime( 230312011001Z, tm_isdst=0) => (1678601401, tm_isdst=0)
mktime( 230312011001Z, tm_isdst=1) => (1678597801, tm_isdst=0)
mktime( 230312011001Z, tm_isdst=-1) => (1678601401, tm_isdst=0)
mktime ( 230312021001Z, tm_isdst=0 ) => (1678605001, tm_isdst=1)
mktime ( 230312021001Z, tm_isdst=1) => (1678601401, tm_isdst=0)
mktime ( 230312021001Z, tm_isdst=-1) => (1678605001, tm_isdst=1)
mktime ( 230312031001Z, tm_isdst=0) => (1678608601, tm_isdst=1)
mktime ( 230312031001Z, tm_isdst=1) => (1678605001, tm_isdst=1)
mktime ( 230312031001Z, tm_isdst=-1) => (1678605001, tm_isdst=1)
The tm_isdst=0 on input returns a time which advances by 3600 each hour. The same is true for tm_isdst=1 case (but this is 3600 different from the non-dst case). However the auto-detecting case of tm_isdst=-1 is more complicated, as it returns the time "02:10:01" with a value indicating it has been interpreted as no-DST, but the return indicates DST is in effect. This is presumably a subtle case with auto-detect on a local-time which does not actually occur, with "DST-in-effect" being different to "This was interpreted with DST-in-effect".
This PR patch changes the input to always specify tm_isdst=0 and to not use the returned value of tm_isdst: It is corrected by the XrdCryptoTZCorr() which always has the non-DST value.
Not shown is the re-evaluation of the output of the patched XrdCryptosslASN1toUTC() across the forwards-and-backwards DST change, but it was seen to be in agreement with the looked-up value of the unix-utc-time-since-epoch value.
As noted in #985 this logic in involved enough to be error prone: Using timegm() would have simplified things, and the suggestion to use openssl functions to compare ASN1 structures might well be preferable. Depending on the number of functions converted swapping to the openssl might change the signatures of several of these functions (e.g. to not pass around time_t, or presumably not to return time_t). However the purpose of the patch and to address the specific problem I kept the time_t approach.
--
Reply to this email directly or view it on GitHub:
https://github.com/xrootd/xrootd/pull/1958#issuecomment-1471710064
You are receiving this because you are subscribed to this thread.
Message ID: <[log in to unmask]>
########################################################################
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1
