Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace make_time function to work after 2050 #209

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

msalle
Copy link
Member

@msalle msalle commented Jan 17, 2023

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

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 gridcf#208
Copy link
Member

@fscheiner fscheiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@msalle
Copy link
Member Author

msalle commented Jan 19, 2023

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).

@msalle msalle requested a review from fscheiner January 19, 2023 14:24
Copy link
Member

@fscheiner fscheiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me*. IIUC with the usage of time_t we are nowadays safe beyond 2038 for 64bit architectures (and GNU/Linux: time_t is defined as __TIME64_T_TYPE in /usr/include/x86_64-linux-gnu/bitstime64.h on my Ubuntu 20.04), but 32bit architectures and other OSes (e.g. *BSDs) might not.


*) Only for the soname value in packaging/fedora/globus-gsi-cert-utils.spec I'm not sure about. So let's wait for clarifcation by Mattias (@ellert) before merging this.

packaging/fedora/globus-gsi-cert-utils.spec Show resolved Hide resolved
@fscheiner
Copy link
Member

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
Copy link
Member

fscheiner commented Jan 20, 2023

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
Copy link
Member Author

msalle commented Jan 20, 2023

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 msalle requested a review from ellert January 20, 2023 10:11
@msalle
Copy link
Member Author

msalle commented Jan 20, 2023

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
Copy link
Member

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
Copy link
Member

ellert commented Jan 21, 2023

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
Copy link
Member

Fine with me. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants