On Sat, Apr 06, 2024 at 12:14:13PM -0600, Bob Beck wrote:
> 
> *alternatively* the cache here was added mainly because timegm()
> gmtime() are kind of horribly inefficient and so repeatly beating on
> them mattered as demonstrated by claudio profiling things in
> rpki-client. At the time I added this cache in the glorious spirit of
> OpenSSL that no performance problem can't be solved by adding another
> thing to a sea of global state (and it worked, we avoided the time
> calls) ;)
> 
> What has changed now is we brought in the BoringSSL style time
> conversion using a julien date in seconds based on the unix epoch in
> zulu time, rather than the OpenSSL julien date conversion of days from
> some random year important only to time nerds for Byzantine reasons
> (<- dad joke), so we no longer need to use timegm() and gmtime().
> These BoringSSL style conversions aren't slow. So I don't believe
> caching these values will have significant performance impact anymore,
> and perhaps we could just remove the caching of these values entirely
> if people prefer.

Right. Unless removing this cache slows down rpki-client significantly,
I would prefer this.

> 
> On Sat, Apr 06, 2024 at 11:42:55AM -0600, Bob Beck wrote:
> > Populating the notbefore/notafter cache should not be fatal.
> > 
> > The addition of the cache of the parsed notbefore/notafter was not
> > meant to mark a certificate invalid at an earlier time, merely to
> > avoid re-parsing times repeatedly (in things like rpki-client)
> > 
> > This reverts to the earlier behaviour of saving the values as marked
> > invalid, so that they may be considered in certificate validation
> > later (which may then fail)
> > 
> > This should correct Mischa's strange ftp behavior.
> > 
> > We still have a long-standing bug here that has been exposed as to why
> > extensions parsing failues fail the handshake even when server
> > certificate validation is not requested - we really should not 
> > have to care if we are not validating the server certificate. but
> > this appears to be a long standing OpenSSL inconsistency - we 
> > should just not pile failures on here to make it worse.
> > 
> > ok?
> > 
> > ---
> >  lib/libcrypto/x509/x509_internal.h |  2 +-
> >  lib/libcrypto/x509/x509_purp.c     | 11 +++++++++--
> >  lib/libcrypto/x509/x509_verify.c   | 21 +++++----------------
> >  3 files changed, 15 insertions(+), 19 deletions(-)
> > 
> > diff --git a/lib/libcrypto/x509/x509_internal.h b/lib/libcrypto/x509/x509_internal.h
> > index 280d1ae46cf..d9da539a588 100644
> > --- a/lib/libcrypto/x509/x509_internal.h
> > +++ b/lib/libcrypto/x509/x509_internal.h
> > @@ -134,7 +134,7 @@ int x509_constraints_check(struct x509_constraints_names *names,
> >      struct x509_constraints_names *excluded, int *error);
> >  int x509_constraints_chain(STACK_OF(X509) *chain, int *error,
> >      int *depth);
> > -int x509_verify_cert_info_populate(X509 *cert);
> > +void x509_verify_cert_info_populate(X509 *cert);
> >  int x509_vfy_check_security_level(X509_STORE_CTX *ctx);
> >  
> >  __END_HIDDEN_DECLS
> > diff --git a/lib/libcrypto/x509/x509_purp.c b/lib/libcrypto/x509/x509_purp.c
> > index 53f4f2f9675..49d54f33516 100644
> > --- a/lib/libcrypto/x509/x509_purp.c
> > +++ b/lib/libcrypto/x509/x509_purp.c
> > @@ -559,8 +559,15 @@ x509v3_cache_extensions_internal(X509 *x)
> >  	if (!x509_extension_oids_are_unique(x))
> >  		x->ex_flags |= EXFLAG_INVALID;
> >  
> > -	if (!x509_verify_cert_info_populate(x))
> > -		x->ex_flags |= EXFLAG_INVALID;
> > +	/*
> > +	 * Populate a cache of notbefore/notafter values
> > +	 * to avoid re-parsing time later.
> > +	 *
> > +	 * These could be cached as invalid, and may fail
> > +	 * later in certificate validation, but should
> > +	 * not fail here.
> > +	 */
> > +	x509_verify_cert_info_populate(x);
> >  
> >  	x->ex_flags |= EXFLAG_SET;
> >  }
> > diff --git a/lib/libcrypto/x509/x509_verify.c b/lib/libcrypto/x509/x509_verify.c
> > index 19bb925d9c6..83841d2a4c0 100644
> > --- a/lib/libcrypto/x509/x509_verify.c
> > +++ b/lib/libcrypto/x509/x509_verify.c
> > @@ -84,29 +84,18 @@ x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notAfter,
> >   * Cache certificate hash, and values parsed out of an X509.
> >   * called from cache_extensions()
> >   */
> > -int
> > +void
> >  x509_verify_cert_info_populate(X509 *cert)
> >  {
> > -	const ASN1_TIME *notBefore, *notAfter;
> > -
> >  	/*
> >  	 * Parse and save the cert times, or remember that they
> >  	 * are unacceptable/unparsable.
> >  	 */
> > -
> >  	cert->not_before = cert->not_after = -1;
> > -
> > -	if ((notBefore = X509_get_notBefore(cert)) == NULL)
> > -		return 0;
> > -	if ((notAfter = X509_get_notAfter(cert)) == NULL)
> > -		return 0;
> > -
> > -	if (!x509_verify_asn1_time_to_time_t(notBefore, 0, &cert->not_before))
> > -		return 0;
> > -	if (!x509_verify_asn1_time_to_time_t(notAfter, 1, &cert->not_after))
> > -		return 0;
> > -
> > -	return 1;
> > +	(void) x509_verify_asn1_time_to_time_t(X509_get_notBefore(cert), 0,
> > +	    &cert->not_before);
> > +	(void) x509_verify_asn1_time_to_time_t(X509_get_notAfter(cert), 1,
> > +	    &cert->not_after);
> >  }
> >  
> >  struct x509_verify_chain *
> > -- 
> > 2.44.0
> >