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>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index 5018063..8d066dd 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -341,8 +341,6 @@
// 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 @@
// 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
diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
index c79c558..7867a80 100644
--- a/crypto/x509/x509_lu.c
+++ b/crypto/x509/x509_lu.c
@@ -594,16 +594,6 @@
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;
}
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 0ca8c82..9d1406f 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -117,6 +117,7 @@
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 @@
// 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 @@
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:
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index a072d6f..f5583d0 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -5294,29 +5294,6 @@
#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
