Am 01.04.20 um 23:50 schrieb w...@torlan.ru: > From: Maxim Plotnikov <w...@torlan.ru> > > Lack of this led people accepting multiple CAs to use capath, > which already supports multiple CRLs. But capath mode itself > is somewhat ugly: you have to create new file/symlink every time > CRL is updated, and there's no good way to clean them up without > restarting OpenVPN, since any gap in the sequence would cause it > to lose sync[1]. > > mbedtls crypto backends already loads multiple CRLs as is, so > it doesn't need this fix. > > The patch also includes some logging changes which I think are useful.
Feature-ACK but some changes to the code required.
> {
> - X509_CRL *crl = NULL;
> + int i = 0;
We can use C99 style definition. The init on top is just because we just
to have C89 style
> BIO *in = NULL;
>
> X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
> @@ -1079,21 +1079,38 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx
> *ssl_ctx, const char *crl_file,
> goto end;
> }
>
> - crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
> - if (crl == NULL)
> - {
> - msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
> - goto end;
> - }
> + for (i = 0;; i++) {
This for loop is a bit odd. Also the use of goto with this kind of for
loop is not the best to understand. Also since i is not really an index,
I would prefer a better named variable like num_crls_loaded or something
similar.
> + X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
> + if (crl == NULL)
> + {
> + unsigned long err = ERR_get_error();
I think we should use ERR_peek_error here, so we do not drop the error
message if the error is something else.
> + char buf[256];
> +
> + if (ERR_GET_REASON(err) == PEM_R_NO_START_LINE && i > 0) {
flip condition if (i > 0 && err == ...) sounds more logical to me
> + // PEM_R_NO_START_LINE can be considered equivalent to EOF.
> + //
> + // A file without any CRLs should still be considered an
> error,
> + // though. Hence i > 0.
Style: OpenVPN does not use C99/C++ style comments.
Also comment does not match what the code does. This is just a warning
and not an error.
> + goto end;
> + }
>
> - if (!X509_STORE_add_crl(store, crl))
> - {
> - msg(M_WARN, "CRL: cannot add %s to store", crl_file);
> - goto end;
> + ERR_error_string_n(err, buf, sizeof(buf));
> +
> + msg(M_WARN, "CRL: cannot read CRL from file %s: %s", crl_file,
> buf);
Use crypto_msg that handles the openssl error stack printing.
> + goto end;
> + }
> +
> + if (!X509_STORE_add_crl(store, crl))
> + {
> + msg(M_WARN, "CRL: cannot add %s to store", crl_file);
Same here.
> + X509_CRL_free(crl);
> + goto end;
> + }
> + X509_CRL_free(crl);
> }
>
> end:
> - X509_CRL_free(crl);
> + msg(M_INFO, "CRL: loaded %d CRLs from file %s", i, crl_file);
> BIO_free(in);
> }
>
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
