4.0: API usage issues deprecating `X509_cmp_timeframe()` and `X509_cmp_time()`

[DDvO]

#29152 recently brought semantic improvements, but I am unhappy with deprecating

int X509_cmp_time(const ASN1_TIME *s, time_t *t);
int X509_cmp_current_time(const ASN1_TIME *s);
int X509_cmp_timeframe(const X509_VERIFY_PARAM *vpm, const ASN1_TIME *start, const ASN1_TIME *end);

BTW, these deprecations, if they stay, should be mentioned in CHANGES.md.

Maintaining libSecUtils, I recently realized that this provides hick-ups to OpenSSL users,
which I worked around in siemens/libsecutils#70.

I wonder why the existing functions have not been kept (with the given improvements, of course).
In particular, it has been suggested to replace using X509_cmp_timeframe() by X509_check_certificate_times(), but this

• requires awkward changes to result checking and error reporting and
• does not work for checking the validity period of non-cert structures like CRLs,
  for which I came up with an ugly workaround like this:

int UTIL_cmp_timeframe(OPTIONAL const X509_VERIFY_PARAM *vpm,
                       OPTIONAL const ASN1_TIME *start, OPTIONAL const ASN1_TIME *end)
{
#if OPENSSL_VERSION_NUMBER < 0x40000000L
    return X509_cmp_timeframe(vpm, start, end);
#else
    X509 *dummy_cert = X509_new(); /* needed as a workaround for OpenSSL API restriction */
    int res = 1, error = X509_V_OK;

    if (dummy_cert != NULL) {
        (void)X509_set1_notBefore(dummy_cert, start);
        (void)X509_set1_notAfter(dummy_cert, end);
        res = X509_check_certificate_times(vpm, dummy_cert, &error);
        X509_free(dummy_cert);
    }
    return res == 1 ? 0 : error == X509_V_ERR_CERT_NOT_YET_VALID ? -1:
        error == X509_V_ERR_CERT_HAS_EXPIRED ? 1 : 0;
#endif
}

One option would be to retain at least X509_cmp_timeframe() (likely basing it and X509_check_certificate_times() on their improved common behavior).

[DDvO]

Also working around deprecation of X509_cmp_time() appears to be a mess, at least for OpenSSL-external users.

The new deprecation note in the HISTORY section of doc/man3/X509_check_certificate_times.pod,
which IMO wrongly dropped the line "X509_cmp_timeframe() was added in OpenSSL 3.0.",
says:

X509_cmp_timeframe(), X509_cmp_current_time(), and
X509_cmp_timeframe() were deprecated in OpenSSL 4.0

For replacement, consider using X509_check_certificate_times() for use
with X509 certificates. For applications checking individual ASN1_TIME
values, consider using ASN1_TIME_to_tm(3) with appropriate validity
checking of the I<ASN1_TIME> value for your application, and subsequent
comparison of either the resulting I<tm> structure, or conversion to
posix seconds via OPENSSL_tm_to_posix(3)

Easier said than done.

How to convert, using only the external OpenSSL API, including appropriate validity checking,
an ASN1_TIME pointer/value to struct tm, time_t, or even betterint64_t ?
If there was a convenient and safe way to do so,
one could then of course use the respective comparison function/operation, ideally just <= etc.

If the decision to deprecate X509_cmp_time() stays, a solution could be exporting from x509_vfy.c:

static int certificate_time_to_posix(const ASN1_TIME *ctm, int64_t *out_time)
{
    struct tm stm;

    if (!validate_certificate_time(ctm))
        return 0;

    if (!ASN1_TIME_to_tm(ctm, &stm))
        return 0;

    if (!OPENSSL_tm_to_posix(&stm, out_time))
        return 0;

    return 1;
}

to the OpenSSL API under some appropriate new name.

[DDvO]

Thinking a little more about this,

• it is API-wise inconsistent to export X509_check_certificate_times (renamed from ossl_x509_check_certificate_times) while not exporting also ossl_x509_check_crl_time(). Yet both of them are not that easy to use
• generalizing the new X509_cmp_timeframe() to cover also other structures besides certs would have the downside that the error codes used are tied to X509 only (e.g., X509_V_ERR_CERT_HAS_EXPIRED would appear strange when used with a CRL).

So I meanwhile suggest keeping both internal, deprecating X509_cmp_timeframe() for its somewhat broken semantics,
and adding instead e.g. int X509_cmp_time_range(const X509_VERIFY_PARAM *vpm, const ASN1_TIME *start, const ASN1_TIME *end)
returning

• -2 if start is malformed,
• 2 if end is malfomed,

and otherwise same as before with one adaptation:

• 0 if vpm is not NULL and the verification parameters do not contain X509_V_FLAG_USE_CHECK_TIME but do contain X509_V_FLAG_NO_CHECK_TIME,
• 1 if the end time is not NULL and the reference time is past the end time - note: here for symmetry and faithful treatment of notAfter, this does not cover equality with the end time
• -1 if the start time is not NULL and the reference time is before,
• 0 to indicate that the reference time is in range (implying that the end time is not before the start time if both are present), including equality with the start or end times.

Note: From this function it would be easy to derive a substitute for the deprecated X509_cmp_current_time().

[DDvO]

@bob-beck @Sashan @nhorman did you notice this issue?
I think @t8m will also be interested.

[DDvO]

Moreover, the following code now used in apps/lib/apps.c:

if (!X509_check_certificate_times(vpm, cert, &error)) {
        char msg[128];

        ERR_error_string_n(error, msg, sizeof(msg));
        warn_cert_msg(uri, cert, msg);
    }

goes wrong on error (yielding, e.g., error:0000000A:lib(0)::reason(10)) and should be replaced by

if (!X509_check_certificate_times(vpm, cert, &error))
    warn_cert_msg(uri, cert, X509_verify_cert_error_string(error));

because the error parameter from X509_check_certificate_times() is an X509 verification error code (like X509_V_ERR_CERT_NOT_YET_VALID), not an OpenSSL error queue error.

For apps/x509.c, the new code

        valid = X509_check_certificate_times(vpm, x, &error);
        if (!valid) {
            char msg[128];

            ERR_error_string_n(error, msg, sizeof(msg));
            BIO_printf(out, "%s\n", msg);
        }
        if (error == X509_V_ERR_CERT_HAS_EXPIRED) {
            BIO_printf(out, "Certificate will expire\n");
            expired = 1;
        } else {
            BIO_printf(out, "Certificate will not expire\n");
        }

should be replaced by, e.g.,

        if (!X509_check_certificate_times(vpm, x, &error)) {
            if (error == X509_V_ERR_CERT_HAS_EXPIRED) {
                BIO_printf(out, "Certificate will expire\n");
                expired = 1;
            } else {
                BIO_printf(out, "Certificate will not expire (or has invalid notBefore or notAfter field)\n");
            }
        }

because even when replacing ERR_error_string_n() by X509_verify_cert_error_string(),
its output (e.g., "certificate has expired") would not only overlap with the texts used with BIO_printf()`, but even be inadequate in the given context.

These glitches indicate right away that the new function API is not straightforward to use.
This is another reason why I suggest going for a different design, as outlined above.

[DDvO]

Ping @openssl/committers
This needs to be handled in time for the 4.0 release.

[npajkovsky]

@bob-beck can you please comment?

[DDvO]

Ping again @openssl/committers
This needs to be handled in time for the 4.0 release.