>> Feature-ACK but some changes to the code required.
> Thank you for the feedback! How should I send a new patch version? As a reply
> (with [PATCH v2]) to this thread, or as an independent message? I'm new to
> contributing patches through e-mail.
>>> + 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.
>
> backend_tls_ctx_reload_crl doesn't return an error (as it's void), and its
> caller never checks OpenSSL error stack. So as this function is, I think it
> should handle all errors itself, and leave the error stack clear.
Yes. But your patch throws that error code away. If there an error that
is not PEM_R_NO_START_LINE it will be silenty discarded. That is my
problem with using ERR_get_error there.
>>> + }
>>> +
>>> + if (!X509_STORE_add_crl(store, crl))
>>> + {
>>> + msg(M_WARN, "CRL: cannot add %s to store", crl_file);
>>
>> Same here.
>
> I can do this, but at the same time I'd like to note that output
Yeah. I think I my comment about ERR_get vs ERR_peek was a bit
misleading. I wanted to say that you should only consume error there if
it is the header error.
> becomes worse:
>
> Thu Apr 2 17:04:24 2020 Diffie-Hellman initialized with 2048 bit key
> Thu Apr 2 17:04:24 2020 OpenSSL: error:0909006C:PEM routines:get_name:no
> start line
> Thu Apr 2 17:04:24 2020 OpenSSL: error:0908F066:PEM
> routines:get_header_and_data:bad end line
> Thu Apr 2 17:04:24 2020 CRL: cannot read CRL from file combined_crl.pem
> Thu Apr 2 17:04:24 2020 CRL: loaded 1 CRLs from file combined_crl.pem
> Thu Apr 2 17:04:24 2020 Failed to extract curve from certificate
> (UNDEF), using secp384r1 instead
>
> The first error comes god knows where from, and the second error is
> actually related to CRL (I corrupted the END X509 CRL line on purpose).
> It's hard to figure out what the errors printed by crypto_msg are
> actually related to.
That is a very recent regression. It should not have been there. See
this (not yet commited) fix: https://patchwork.openvpn.net/patch/1071/
After reviewing your patch and writing that fix, I also realised how
similar the two pieces of code are. You can probably almost copy and
paste the (new) tls_ctx_add_extra_certs to the crl loading function.
> Compare with how it was in current edition of the patch:
>
> Thu Apr 2 17:07:13 2020 Diffie-Hellman initialized with 2048 bit key
> Thu Apr 2 17:07:13 2020 OpenSSL: error:0909006C:PEM routines:get_name:no
> start line
> Thu Apr 2 17:07:13 2020 CRL: cannot read CRL from file combined_crl.pem:
> error:0908F066:PEM routines:get_header_and_data:bad end line
> Thu Apr 2 17:07:13 2020 CRL: loaded 1 CRLs from file combined_crl.pem
>
Yeah but *always* having a scary error that translates to "No more crls
found in the file" is not very user friendly since most people will not
understand that this is to be expected.
Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
