diff mbox series

[Openvpn-devel,4/4] cryptoapi.c: simplify parsing of thumbprint hex string

Message ID 20230128223421.2207802-5-selva.nair@gmail.com
State Superseded
Headers
Series Improvements for cryptoapi.c |

Commit Message

Selva Nair Jan. 28, 2023, 10:34 p.m. UTC 
  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(-)

Comments

Frank Lichtenheld Feb. 1, 2023, 11:56 a.m. UTC | #1 
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,
Selva Nair Feb. 2, 2023, 2:45 a.m. UTC | #2 
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
Selva Nair Feb. 3, 2023, 5 a.m. UTC | #3 
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
Arne Schwabe Feb. 3, 2023, 10:25 a.m. UTC | #4 
> 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
Selva Nair Feb. 3, 2023, 4:18 p.m. UTC | #5 
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
Arne Schwabe Feb. 4, 2023, 1:22 p.m. UTC | #6 
> 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
Gert Doering Feb. 4, 2023, 2:36 p.m. UTC | #7 
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 mbox series

Patch

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;
     }