gridcf / gct

Grid Community Toolkit
Apache License 2.0
46 stars 30 forks source link

Replace make_time function to work after 2050 #209

Closed msalle closed 1 year ago

msalle commented 1 year ago

By using ASN1_TIME_diff() instead of manually parsing the data, we make globus_gsi_cert_utils_make_time() a lot simpler and also work for ASN1_GENERALIZEDTIME and not just ASN1_UTCTIME (i.e. it can use ASN1_TIME). ASN1_TIME_diff() requires OpenSSL >= 1.0.2. Also rework globus_gsi_cred_get_lifetime() to just use time(NULL) to get the current UNIX timestamp which means it no longer needs globus_gsi_cert_utils_make_time(). This fixes issue #208

msalle commented 1 year ago

Could you also also update the version numbers and packaging (RPM spec files and debian) in this PR?

done, not sure if these are all the files I needed to change. Also, I'm not a maintainer for the debian packages, so Mattias might need to take ownership of the changelog entries to prevent non-maintainer update issues (or make us all debian uploaders).

fscheiner commented 1 year ago

Could you also also update the version numbers and packaging (RPM spec files and debian) in this PR?

done,

Thanks!

not sure if these are all the files I needed to change. Also, I'm not a maintainer for the debian packages, so Mattias might need to take ownership of the changelog entries to prevent non-maintainer update issues (or make us all debian uploaders).

I think Mattias will adapt these changelogs for the actual Debian/Ubuntu packages anyhow, so we are safe to change them here in our name.

fscheiner commented 1 year ago

Btw, looks like we have our first difference between CentOS Stream 9 and Rocky Linux 9. The build build for CentOS Stream 9 doesn't work w/o #210 merged, the build for Rocky Linux 9 does work. I suspect the Docker images use different versions of OpenSSL, I'll check my VMs for any differences here.

UPDATE: Indeed, for CentOS Stream 9 we have:

$ openssl version
OpenSSL 3.0.7 1 Nov 2022 (Library: OpenSSL 3.0.7 1 Nov 2022)

...and for Rocky Linux 9 we have:

$ openssl version
OpenSSL 3.0.1 14 Dec 2021 (Library: OpenSSL 3.0.1 14 Dec 2021)
msalle commented 1 year ago

time_t

considering time_t this is a much wider problem all over gct, so we cannot fix that here. I think it is relatively ok nowadays for most common UNIX-likes. See https://en.wikipedia.org/wiki/Year_2038_problem#Implemented_solutions

msalle commented 1 year ago

Btw, looks like we have our first difference between CentOS Stream 9 and Rocky Linux 9. The build build for CentOS Stream 9 doesn't work w/o #210 merged, the build for Rocky Linux 9 does work. I suspect the Docker images use different versions of OpenSSL, I'll check my VMs for any differences here.

Bit off-topic, but see also https://indico.cern.ch/event/1224843/#12-linux-future-in-wlcg-status i.e. WLCG also seems to be moving away from CentOS Stream in favour of ALMA and Rocky.

fscheiner commented 1 year ago

Bit off-topic, but see also https://indico.cern.ch/event/1224843/#12-linux-future-in-wlcg-status i.e. WLCG also seems to be moving away from CentOS Stream in favour of ALMA and Rocky.

Yeah, I read about that recently in some Linux newsletter.

It's still good that we also build for CentOS Stream 9, because there might not always be a user that stumbles upon a mistake by me (see #207 and #210) and then the failing builds on CentOS Stream 9 would have uncovered it. ;-)

ellert commented 1 year ago

I think we should avoid soname changes if possible.

man ASN1_TIME_set says:

   The ASN1_TIME structure corresponds to the ASN.1 structure Time defined
   in RFC5280 et al. The time setting functions obey the rules outlined in
   RFC5280: if the date can be represented by UTCTime it is used, else
   GeneralizedTime is used.

   It is recommended that functions starting with ASN1_TIME be used
   instead of those starting with ASN1_UTCTIME or ASN1_GENERALIZEDTIME.
   The functions starting with ASN1_UTCTIME and ASN1_GENERALIZEDTIME act
   only on that specific time format. The functions starting with
   ASN1_TIME will operate on either format.

So changing the argument from const ASN1_UTCTIME * to a const ASN1_TIME * in globus_gsi_cert_utils_make_time() (which is the only function listed in a header file that is changed) should be fine.

The calls to globus_gsi_cert_utils_make_time() in gsi/callback/source/library/globus_gsi_callback.c already calls it using a const ASN1_TIME * pointer anyway...

The same is true for the calls to globus_gsi_cert_utils_make_time() in gsi/credential/source/library/globus_gsi_cred_handle.c and myproxy/source/ssl_utils.c, since X509_get_notAfter() and X509_get_notBefore() return a ASN1_TIME *.

So changing the type in the signature in the header actually reflects how the function is called now.

fscheiner commented 1 year ago

Fine with me. :-)