| Message ID | 1590709611.3515.22.camel@HansenPartnership.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,BUG] test_ncp.c failing | expand |
On 29-05-2020 01:46, James Bottomley wrote: > The problem seems to be openssl uses a mixed case name for the cipher > and EVP_CIPHER_name() is case sensitive. Applying the patch below > fixes this for openssl and gets make check to pass all tests, but I > rather wonder why this isn't part of cipher_kt_name() to prevent this > type of problem? This was my first thought when looking at the patch too. Probably because I wrote the cipher name translation code as one of my very first OpenVPN contributions, and didn't get the abstractions right at the time. Would be good to see if this can be refactored. Slightly annoying to test if that won't break corner cases though, because I think this part is not sufficiently covered by automated tests. -Steffan
Am 29.05.20 um 01:46 schrieb James Bottomley: > I'm getting this failure of test_ncp.c > > [ RUN ] test_check_ncp_ciphers_list > [ ERROR ] --- 0x7d67e8 != 0 > [ LINE ] --- test_ncp.c:65: error: Failure! > [ FAILED ] test_check_ncp_ciphers_list > > I'm building under openssl-1.1.0i > > The problem seems to be openssl uses a mixed case name for the cipher > and EVP_CIPHER_name() is case sensitive. Applying the patch below > fixes this for openssl and gets make check to pass all tests, but I > rather wonder why this isn't part of cipher_kt_name() to prevent this > type of problem? Without double checking if I remember correctly OpenSSL 1.1.0 is a corner case in this regard. OpenSSL 1.1.1 accepts mixed and non mixed case. I want to fix this oddity in a proper way and write at least a small unit test to ensure that always end up with the same cipher on the wire protocol (in IV_CIPHERS and in the options string). Arne
On Fri, 2020-06-05 at 00:37 +0200, Arne Schwabe wrote: > Am 29.05.20 um 01:46 schrieb James Bottomley: > > I'm getting this failure of test_ncp.c > > > > [ RUN ] test_check_ncp_ciphers_list > > [ ERROR ] --- 0x7d67e8 != 0 > > [ LINE ] --- test_ncp.c:65: error: Failure! > > [ FAILED ] test_check_ncp_ciphers_list > > > > I'm building under openssl-1.1.0i > > > > The problem seems to be openssl uses a mixed case name for the > > cipher and EVP_CIPHER_name() is case sensitive. Applying the patch > > below fixes this for openssl and gets make check to pass all tests, > > but I rather wonder why this isn't part of cipher_kt_name() to > > prevent this type of problem? > > Without double checking if I remember correctly OpenSSL 1.1.0 is a > corner case in this regard. OpenSSL 1.1.1 accepts mixed and non mixed > case. It was fixed by: commit fc196a5eb97dc3a5465c37a6761428ddd81b023d Author: Pauli <paul.dale@oracle.com> Date: Tue Sep 4 07:35:45 2018 +1000 Make OBJ_NAME case insensitive. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from https://github.com/openssl/openssl/pull/7089) So yes, all of 1.1.1 has this fix. James > I want to fix this oddity in a proper way and write at least a small > unit test to ensure that always end up with the same cipher on the > wire protocol (in IV_CIPHERS and in the options string). > > Arne >
Hi, On Thu, May 28, 2020 at 04:46:51PM -0700, James Bottomley wrote: > diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c > index 19432410..f58fa2ea 100644 > --- a/tests/unit_tests/openvpn/test_ncp.c > +++ b/tests/unit_tests/openvpn/test_ncp.c > @@ -48,7 +48,7 @@ static void > test_check_ncp_ciphers_list(void **state) > { > struct gc_arena gc = gc_new(); > - bool have_chacha = cipher_kt_get("CHACHA20-POLY1305"); > + bool have_chacha = cipher_kt_get(translate_cipher_name_from_openvpn("CHACHA20-POLY1305")); Am I right in assuming that this patch is no longer needed with the two cipher_kt_ translation patches I merged yesterday? gert
On Fri, 2020-06-12 at 08:23 +0200, Gert Doering wrote: > Hi, > > On Thu, May 28, 2020 at 04:46:51PM -0700, James Bottomley wrote: > > diff --git a/tests/unit_tests/openvpn/test_ncp.c > > b/tests/unit_tests/openvpn/test_ncp.c > > index 19432410..f58fa2ea 100644 > > --- a/tests/unit_tests/openvpn/test_ncp.c > > +++ b/tests/unit_tests/openvpn/test_ncp.c > > @@ -48,7 +48,7 @@ static void > > test_check_ncp_ciphers_list(void **state) > > { > > struct gc_arena gc = gc_new(); > > - bool have_chacha = cipher_kt_get("CHACHA20-POLY1305"); > > + bool have_chacha = > > cipher_kt_get(translate_cipher_name_from_openvpn("CHACHA20- > > POLY1305")); > > Am I right in assuming that this patch is no longer needed with the > two cipher_kt_ translation patches I merged yesterday? Yes, make check now passes on openSUSE_Leap_15.1 James
Hi, On Fri, Jun 12, 2020 at 07:49:07AM -0700, James Bottomley wrote: > > Am I right in assuming that this patch is no longer needed with the > > two cipher_kt_ translation patches I merged yesterday? > Yes, make check now passes on openSUSE_Leap_15.1 Cool. Marking as "superseded" in patchwork. One down, 73 to go... :) gert
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c index 19432410..f58fa2ea 100644 --- a/tests/unit_tests/openvpn/test_ncp.c +++ b/tests/unit_tests/openvpn/test_ncp.c @@ -48,7 +48,7 @@ static void test_check_ncp_ciphers_list(void **state) { struct gc_arena gc = gc_new(); - bool have_chacha = cipher_kt_get("CHACHA20-POLY1305"); + bool have_chacha = cipher_kt_get(translate_cipher_name_from_openvpn("CHACHA20-POLY1305"));