Use OSSL_TIME more widely by paulidale · Pull Request #19082 · openssl/openssl

paulidale · 2022-08-29T04:20:55Z

This unifies out time handle and will help avoid 2038 problems.
Because we have some public APIs that accept time_t and struct timeval, it's impossible to completely convert.

Checklist

documentation is added or updated
tests are added or updated

include/internal/time.h

crypto/bio/bss_dgram.c

crypto/ts/ts_rsp_sign.c

include/internal/e_os.h

ssl/d1_lib.c

ssl/ssl_lib.c

ssl/ssl_local.h

ssl/ssl_sess.c

ssl/statem/extensions_clnt.c

tmshort

There are a few places where calculations with a constant are still performed, but there are macros/functions that could be used instead.

I would also suggest changing the SSL_METHOD get_timeout() function to return OSSL_TIME.

paulidale · 2022-09-01T23:28:52Z

I added the extra helper functions late in the process (when I got sick of repeating the same bits of code). I missed a few, thanks for pointing these out.

One thing I was struggling with was duration vs time. I considered adding an OSSL_DURATION to make this distinction typesafe.
If we ever switch to an Epoch that isn't based on Unix's 1970 (e.g. by using clock_gettime(2)), we will need to be very careful about conversions & it's easy to get it wrong (as I did above at least once). Maybe the second type should be introduced after all.

Existing uses of time_t and struct timeval are problematic because they do not distinguish between the two flavours.

tmshort · 2022-09-02T13:42:27Z

One thing I was struggling with was duration vs time. I considered adding an OSSL_DURATION to make this distinction typesafe.

I completely agree that duration vs "calendar time" are different but related, and are bound by the same type constraints... kinda. A duration is nominally positive. "Calendar time" can be negative. But both represent a time offset.

Edit: While a duration is positive, the difference between times could be negative (time a is before time b). So is this an abs()?

crypto/time.c

crypto/ts/ts_rsp_sign.c

ssl/ssl_sess.c

mattcaswell

Reconfirm

paulidale · 2022-09-13T11:13:54Z

Merged, thanks for the feedbacks.

Some of the recently added functions were not documents. This has been addressed. Also added utility functions for conversions between time_t, seconds and struct timeval to/from OSSL_TIME. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #19082)

This is instead of time_t and struct timeval. Some public APIs mandate a presence of these two types, but they are converted to OSSL_TIME internally. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #19082)

Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #19082)

Keep building it for libssl without exposing any symbols. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #19082)

Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #19082)

Some of the recently added functions were not documents. This has been addressed. Also added utility functions for conversions between time_t, seconds and struct timeval to/from OSSL_TIME. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

This is instead of time_t and struct timeval. Some public APIs mandate a presence of these two types, but they are converted to OSSL_TIME internally. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Keep building it for libssl without exposing any symbols. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Some of the recently added functions were not documents. This has been addressed. Also added utility functions for conversions between time_t, seconds and struct timeval to/from OSSL_TIME. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

This is instead of time_t and struct timeval. Some public APIs mandate a presence of these two types, but they are converted to OSSL_TIME internally. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Keep building it for libssl without exposing any symbols. Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

Reviewed-by: Todd Short <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#19082)

For historical reasons, the second argument of SSL_CTX_set_timeout is a signed integer, and Node.js has so far passed arbitrary (signed) int32_t values. However, new versions of OpenSSL have changed the handling of negative values inside SSL_CTX_set_timeout, and we should shield users of Node.js from both the old and the new behavior. Hence, reject any negative values by throwing an error from within createSecureContext. Refs: openssl/openssl#19082

For historical reasons, the second argument of SSL_CTX_set_timeout is a signed integer, and Node.js has so far passed arbitrary (signed) int32_t values. However, new versions of OpenSSL have changed the handling of negative values inside SSL_CTX_set_timeout, and we should shield users of Node.js from both the old and the new behavior. Hence, reject any negative values by throwing an error from within createSecureContext. Refs: openssl/openssl#19082 PR-URL: #53002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tim Perry <[email protected]>

For historical reasons, the second argument of SSL_CTX_set_timeout is a signed integer, and Node.js has so far passed arbitrary (signed) int32_t values. However, new versions of OpenSSL have changed the handling of negative values inside SSL_CTX_set_timeout, and we should shield users of Node.js from both the old and the new behavior. Hence, reject any negative values by throwing an error from within createSecureContext. Refs: openssl/openssl#19082 PR-URL: nodejs#53002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tim Perry <[email protected]>

For historical reasons, the second argument of SSL_CTX_set_timeout is a signed integer, and Node.js has so far passed arbitrary (signed) int32_t values. However, new versions of OpenSSL have changed the handling of negative values inside SSL_CTX_set_timeout, and we should shield users of Node.js from both the old and the new behavior. Hence, reject any negative values by throwing an error from within createSecureContext. Refs: openssl/openssl#19082 PR-URL: #53002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tim Perry <[email protected]>

paulidale self-assigned this Aug 29, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 29, 2022

levitte reviewed Aug 30, 2022

View reviewed changes

include/internal/time.h Outdated Show resolved Hide resolved

paulidale force-pushed the ossl-time branch from 21d6367 to 0e2804e Compare August 31, 2022 03:20

levitte reviewed Aug 31, 2022

View reviewed changes

crypto/bio/bss_dgram.c Show resolved Hide resolved

paulidale marked this pull request as ready for review August 31, 2022 23:49

paulidale force-pushed the ossl-time branch from 17018f9 to 841b8cc Compare August 31, 2022 23:50

paulidale added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending labels Aug 31, 2022

paulidale mentioned this pull request Sep 1, 2022

Fix use of time_t internally and APIs #19107

Closed

2 tasks