Remove X509_STORE_set_get_crl and X509_STORE_set_check_crl

gRPC is no longer using these, so remove them. They were impossible to
use correctly and are the cause of weird statefulness around
ctx->error_depth.

Once this CL sticks, we can follow up and clean up this a code a bit.

Update-Note: Some unused (and unusable) callbacks were removed.
Bug: 674
Change-Id: I8109dd6555d2ca056447c1b4f0aa28abe7af81b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68387
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/internal.h

@@ -341,8 +341,6 @@ struct x509_store_st {
 
   // Callbacks for various operations
   X509_STORE_CTX_verify_cb verify_cb;       // error callback
-  X509_STORE_CTX_get_crl_fn get_crl;        // retrieve CRL
-  X509_STORE_CTX_check_crl_fn check_crl;    // Check CRL validity
 
   CRYPTO_refcount_t references;
 } /* X509_STORE */;
@@ -374,8 +372,6 @@ struct x509_store_ctx_st {
 
   // Callbacks for various operations
   X509_STORE_CTX_verify_cb verify_cb;       // error callback
-  X509_STORE_CTX_get_crl_fn get_crl;        // retrieve CRL
-  X509_STORE_CTX_check_crl_fn check_crl;    // Check CRL validity
 
   // The following is built up
   int last_untrusted;     // index of last untrusted cert

crypto/x509/x509_lu.c

@@ -594,16 +594,6 @@ void X509_STORE_set_verify_cb(X509_STORE *ctx,
   ctx->verify_cb = verify_cb;
 }
 
-void X509_STORE_set_get_crl(X509_STORE *ctx,
-                            X509_STORE_CTX_get_crl_fn get_crl) {
-  ctx->get_crl = get_crl;
-}
-
-void X509_STORE_set_check_crl(X509_STORE *ctx,
-                              X509_STORE_CTX_check_crl_fn check_crl) {
-  ctx->check_crl = check_crl;
-}
-
 X509_STORE *X509_STORE_CTX_get0_store(const X509_STORE_CTX *ctx) {
   return ctx->ctx;
 }

crypto/x509/x509_vfy.c

@@ -117,6 +117,7 @@ static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x);
 static int crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer,
                           int *pcrl_score);
 static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score);
+static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl);
 static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x);
 
 static int internal_verify(X509_STORE_CTX *ctx);
@@ -769,17 +770,18 @@ static int check_cert(X509_STORE_CTX *ctx) {
   // Try to retrieve the relevant CRL. Note that |get_crl| sets
   // |current_crl_issuer| and |current_crl_score|, which |check_crl| then reads.
   //
-  // TODO(davidben): Remove these callbacks. gRPC currently sets them, but
-  // implements them incorrectly. It is not actually possible to implement
-  // |get_crl| from outside the library.
-  if (!ctx->get_crl(ctx, &crl, x)) {
+  // TODO(davidben): The awkward internal calling convention is a historical
+  // artifact of when these functions were user-overridable callbacks, even
+  // though there was no way to set them correctly. These callbacks have since
+  // been removed, so we can pass input and output parameters more directly.
+  if (!get_crl(ctx, &crl, x)) {
     ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL;
     ok = call_verify_cb(0, ctx);
     goto err;
   }
 
   ctx->current_crl = crl;
-  if (!ctx->check_crl(ctx, crl) ||  //
+  if (!check_crl(ctx, crl) ||  //
       !cert_crl(ctx, crl, x)) {
     goto err;
   }
@@ -1560,18 +1562,6 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
     ctx->verify_cb = null_callback;
   }
 
-  if (store->get_crl) {
-    ctx->get_crl = store->get_crl;
-  } else {
-    ctx->get_crl = get_crl;
-  }
-
-  if (store->check_crl) {
-    ctx->check_crl = store->check_crl;
-  } else {
-    ctx->check_crl = check_crl;
-  }
-
   return 1;
 
 err:

include/openssl/x509.h

@@ -5294,29 +5294,6 @@ OPENSSL_EXPORT void X509_STORE_set_verify_cb(
 #define X509_STORE_set_verify_cb_func(store, func) \
   X509_STORE_set_verify_cb((store), (func))
 
-typedef int (*X509_STORE_CTX_get_crl_fn)(X509_STORE_CTX *ctx, X509_CRL **crl,
-                                         X509 *x);
-typedef int (*X509_STORE_CTX_check_crl_fn)(X509_STORE_CTX *ctx, X509_CRL *crl);
-
-// X509_STORE_set_get_crl override's |store|'s logic for looking up CRLs.
-//
-// Do not use this function. It is temporarily retained to support one caller
-// and will be removed after that caller is fixed. It is not possible for
-// external callers to correctly implement this callback. The real
-// implementation sets some inaccessible internal state on |X509_STORE_CTX|.
-OPENSSL_EXPORT void X509_STORE_set_get_crl(X509_STORE *store,
-                                           X509_STORE_CTX_get_crl_fn get_crl);
-
-// X509_STORE_set_check_crl override's |store|'s logic for checking CRL
-// validity.
-//
-// Do not use this function. It is temporarily retained to support one caller
-// and will be removed after that caller is fixed. It is not possible for
-// external callers to correctly implement this callback. The real
-// implementation relies some inaccessible internal state on |X509_STORE_CTX|.
-OPENSSL_EXPORT void X509_STORE_set_check_crl(
-    X509_STORE *store, X509_STORE_CTX_check_crl_fn check_crl);
-
 // X509_STORE_CTX_set_chain configures |ctx| to use |sk| for untrusted
 // intermediate certificates to use in verification. This function is redundant
 // with the |chain| parameter of |X509_STORE_CTX_init|. Use the parameter