Message ID | 20230128223421.2207802-5-selva.nair@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | Improvements for cryptoapi.c | expand |
On Sat, Jan 28, 2023 at 05:34:21PM -0500, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@gmail.com> > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > src/openvpn/cryptoapi.c | 44 +++++++++++------------------------------ > 1 file changed, 12 insertions(+), 32 deletions(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index 6ff4fcb5..9fd5aea9 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) This seems to ask for a unittest. Regards,
Hi, On Wed, Feb 1, 2023 at 6:56 AM Frank Lichtenheld <frank@lichtenheld.com> wrote: > On Sat, Jan 28, 2023 at 05:34:21PM -0500, selva.nair@gmail.com wrote: > > From: Selva Nair <selva.nair@gmail.com> > > > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > > --- > > src/openvpn/cryptoapi.c | 44 +++++++++++------------------------------ > > 1 file changed, 12 insertions(+), 32 deletions(-) > > > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > > index 6ff4fcb5..9fd5aea9 100644 > > --- a/src/openvpn/cryptoapi.c > > +++ b/src/openvpn/cryptoapi.c > > @@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop, > HCERTSTORE cert_store) > > This seems to ask for a unittest. Would be nice indeed. But, I've never tried to enable cmocka in our Windows build -- does it work? If it does, I would be inclined to add tests for this as well as some other functions in cryptoapi.c Regards, Selva
Hi > > On Wed, Feb 1, 2023 at 6:56 AM Frank Lichtenheld <frank@lichtenheld.com> > wrote: > >> On Sat, Jan 28, 2023 at 05:34:21PM -0500, selva.nair@gmail.com wrote: >> > From: Selva Nair <selva.nair@gmail.com> >> > >> > Signed-off-by: Selva Nair <selva.nair@gmail.com> >> > --- >> > src/openvpn/cryptoapi.c | 44 +++++++++++------------------------------ >> > 1 file changed, 12 insertions(+), 32 deletions(-) >> > >> > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c >> > index 6ff4fcb5..9fd5aea9 100644 >> > --- a/src/openvpn/cryptoapi.c >> > +++ b/src/openvpn/cryptoapi.c >> > @@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop, >> HCERTSTORE cert_store) >> >> This seems to ask for a unittest. > > > Would be nice indeed. > > But, I've never tried to enable cmocka in our Windows build -- does it > work? If it does, I would be inclined to add tests for this as well as some > other functions in cryptoapi.c > Well, replying to myself: I tried building the unit tests using cmocka for Windows (cross-compiling using mingw-w64 with locally built cmocka). Unfortunately, none of the tests could be built out of the box because of missing dependencies, so I guess no one is running these tests on Windows. Good news is that I could get provider_testdriver to build and run on Windows with minor tweaks and a test for this patch could be easily written too. I can follow up with a test patch, but wonder whether it's worth spending time on if no one else is building these tests for Windows. Regards, Selva
> Well, replying to myself: > > I tried building the unit tests using cmocka for Windows > (cross-compiling using mingw-w64 with locally built cmocka). > Unfortunately, none of the tests could be built out of the box because > of missing dependencies, so I guess no one is running these tests on > Windows. > > Good news is that I could get provider_testdriver to build and run on > Windows with minor tweaks and a test for this patch could be easily > written too. > > I can follow up with a test patch, but wonder whether it's worth > spending time on if no one else is building these tests for Windows. I have a cmake build file for OpenVPN that actually builds part of the unit tests ("test_packet_id" "test_crypto" "test_ncp" "test_auth_token" "test_misc" "test_buffer" "test_provider" "test_pkt") and run them in the Github actions pipeline. But the cmake stuff is more "work in progress" and the general consensus was to stick with the autoconf approach. I looked into building the test binaries as part of the normal MSVC build but gave up on trying to figure out how to deal with MSVC and their solutions, especially since you can nowadays just point it at a CmakeLists.txt and it will pick that up just fine. If you want to take a look: Cmake build file: https://github.com/schwabe/openvpn/blob/bloom/CMakeLists.txt Running tests as part of GHA: https://github.com/schwabe/openvpn/blob/bloom/.github/workflows/build.yaml#L561 Arne
On Fri, Feb 3, 2023 at 5:25 AM Arne Schwabe <arne@rfc2549.org> wrote: > > > Well, replying to myself: > > > > I tried building the unit tests using cmocka for Windows > > (cross-compiling using mingw-w64 with locally built cmocka). > > Unfortunately, none of the tests could be built out of the box because > > of missing dependencies, so I guess no one is running these tests on > > Windows. > > > > Good news is that I could get provider_testdriver to build and run on > > Windows with minor tweaks and a test for this patch could be easily > > written too. > > > > I can follow up with a test patch, but wonder whether it's worth > > spending time on if no one else is building these tests for Windows. > > I have a cmake build file for OpenVPN that actually builds part of the > unit tests ("test_packet_id" "test_crypto" "test_ncp" "test_auth_token" > "test_misc" "test_buffer" "test_provider" "test_pkt") and run them in > the Github actions pipeline. But the cmake stuff is more "work in > progress" and the general consensus was to stick with the autoconf > approach. > > I looked into building the test binaries as part of the normal MSVC > build but gave up on trying to figure out how to deal with MSVC and > their solutions, especially since you can nowadays just point it at a > CmakeLists.txt and it will pick that up just fine. > > If you want to take a look: > > Cmake build file: > https://github.com/schwabe/openvpn/blob/bloom/CMakeLists.txt > > Running tests as part of GHA: > > https://github.com/schwabe/openvpn/blob/bloom/.github/workflows/build.yaml#L561 Thanks a lot for that. By adding win32-util.c and -lws2_32 where required, I can now build almost all tests using the autotools framework --- cmocka had to be cross-compiled using cmake which is a pain. I do not particularly like cmake though it's convenient for Windows MSVC build, so sticking to autoconf/automake for now. Unless the "consensus" is to move to cmake --- hope not :) Anyway, this is motivating to write some tests. Selva
> Thanks a lot for that. > > By adding win32-util.c and -lws2_32 where required, I can now build > almost all tests using the autotools framework --- cmocka had to be > cross-compiled using cmake which is a pain. > I do not particularly like cmake though it's convenient for Windows MSVC > build, so sticking to autoconf/automake for now. Unless the "consensus" > is to move to cmake --- hope not :) No. Other OpenVPN contributers have spoken out against CMake, so I never bothered to clean it up, so that the current cmake file is a mixture of clean stuff and hacks. It is pretty useful for me for doing remote build/debugging with Clion under macOS/Linux and also when I use Visual Studio or Clion under Windows. Even though I think that adding Cmake as unsupported build method is a good idea. This would allow others to have the same option/advantages like me that prefer cmake to autoconf/automake, this also does not seem to have a consensus, so I just maintain it for myself in my branch. Arne
Hi, On Sat, Feb 04, 2023 at 02:22:30PM +0100, Arne Schwabe wrote: > Even though I think that adding Cmake as > unsupported build method is a good idea. This would allow others to have > the same option/advantages like me that prefer cmake to > autoconf/automake, this also does not seem to have a consensus, so I > just maintain it for myself in my branch. I do not object to having "something cmake based" available for those that find it beneficial. I just do not want to move over completely, as I think on unix/linux, autoconf does a good job and *for me* cmake just adds complications. gert
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 6ff4fcb5..9fd5aea9 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) } else if (!strncmp(cert_prop, "THUMB:", 6)) { - const char *p; - int i, x = 0; find_type = CERT_FIND_HASH; find_param = &blob; - /* skip the tag */ - cert_prop += 6; - for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++) + int i = 0; + + for (const char *p = cert_prop + 6; *p && i < sizeof(hash); p += 2) { - if (*p >= '0' && *p <= '9') - { - x = (*p - '0') << 4; - } - else if (*p >= 'A' && *p <= 'F') + /* skip spaces */ + while (*p == ' ') { - x = (*p - 'A' + 10) << 4; + p++; } - else if (*p >= 'a' && *p <= 'f') + if (!*p) /* ending with spaces is not an error */ { - x = (*p - 'a' + 10) << 4; + break; } - if (!*++p) /* unexpected end of string */ + + if (!isxdigit(p[0]) || !isxdigit(p[1]) + || sscanf(p, "%2hhx", &hash[i++]) != 1) { - msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing <THUMB:%s>.", cert_prop); + msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing <%s>.", cert_prop); goto out; } - if (*p >= '0' && *p <= '9') - { - x += *p - '0'; - } - else if (*p >= 'A' && *p <= 'F') - { - x += *p - 'A' + 10; - } - else if (*p >= 'a' && *p <= 'f') - { - x += *p - 'a' + 10; - } - hash[i] = x; - /* skip any space(s) between hex numbers */ - for (p++; *p && *p == ' '; p++) - { - } } blob.cbData = i; }