Message ID | 20230327114937.28246-1-selva.nair@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant | expand |
On Mon, Mar 27, 2023 at 07:49:37AM -0400, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@gmail.com> > > - Do not use non-literal initializers for static objects > - Replace empty initializer {} by {0} > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > To be applied after the test-pkcs11 patch set Tested MSVC build (in my cmake branch) and verified that this is a suitable replacement for my earlier attempt to fix same issue. Acked-By: Frank Lichtenheld <frank@lichtenheld.com> Regards,
On Mon, Mar 27, 2023 at 8:09 AM Frank Lichtenheld <frank@lichtenheld.com> wrote: > On Mon, Mar 27, 2023 at 07:49:37AM -0400, selva.nair@gmail.com wrote: > > From: Selva Nair <selva.nair@gmail.com> > > > > - Do not use non-literal initializers for static objects > > - Replace empty initializer {} by {0} > > > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > I should have referenced Frank's original patch with: Co-authored-by: Frank Lichtenheld <frank@lichtenheld.com> > --- > > To be applied after the test-pkcs11 patch set > > Tested MSVC build (in my cmake branch) and verified that this is a > suitable replacement for my earlier attempt to fix same issue. > > Acked-By: Frank Lichtenheld <frank@lichtenheld.com> > > Regards, > -- > Frank Lichtenheld >
Am 27.03.23 um 13:49 schrieb selva.nair@gmail.com: > From: Selva Nair <selva.nair@gmail.com> > > - Do not use non-literal initializers for static objects > - Replace empty initializer {} by {0} Should we go to a revision, I would suggest to not make something compliant to a compiler because that is assigning it way too much power and control. A terms such as "ready to be compiled with MSVC" or so would be in order, or making this code compliant with some wisely chosen version of the ISO 9899 standard, aka. standard C. > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > To be applied after the test-pkcs11 patch set > > tests/unit_tests/openvpn/cert_data.h | 6 ++--- > tests/unit_tests/openvpn/test_cryptoapi.c | 24 ++++++++++++++------ > tests/unit_tests/openvpn/test_pkcs11.c | 27 ++++++++++++++++------- > 3 files changed, 39 insertions(+), 18 deletions(-) > > diff --git a/tests/unit_tests/openvpn/cert_data.h b/tests/unit_tests/openvpn/cert_data.h > index 33de35ec..0886b071 100644 > --- a/tests/unit_tests/openvpn/cert_data.h > +++ b/tests/unit_tests/openvpn/cert_data.h > @@ -79,7 +79,7 @@ static const char *const cert2 = > "HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n" > "KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n" > "-----END CERTIFICATE-----\n"; > -static const char *const key2 = key1; > +#define key2 key1 So we are not getting rid of #defines yet? Can't we make this data somehow local? Personally, I would probably - at least outside unit tests - write (key1) instead of bare key, so that it's clear this is one name and can't be somehow extended. Whenever I see such code I wonder if it's not worthwhile to slowly walk towards C++17 where appropriate. I am not going to NAK or ACK this because I won't have time available to set up a compilation environment based on Frank's repo within the next three days, just commenting.
Hi, On Mon, Mar 27, 2023 at 9:59 AM Matthias Andree <matthias.andree@gmx.de> wrote: > Am 27.03.23 um 13:49 schrieb selva.nair@gmail.com: > > From: Selva Nair <selva.nair@gmail.com> > > > > - Do not use non-literal initializers for static objects > > - Replace empty initializer {} by {0} > > Should we go to a revision, I would suggest to not make something > compliant to a compiler because that is assigning it way too much power > and control. A terms such as "ready to be compiled with MSVC" or so > would be in order, or making this code compliant with some wisely chosen > version of the ISO 9899 standard, aka. standard C. > In this case "MSVC compliant" was arguably a poor choice of words as what the change does is to avoid non-C99 constructs. However it made sense to refer to MSVC in the context of what prompted this patch. That said, in a cross-platform code base one often has to make changes to please compilers just to get things to build. > Signed-off-by: Selva Nair <selva.nair@gmail.com> > > --- > > To be applied after the test-pkcs11 patch set > > > > tests/unit_tests/openvpn/cert_data.h | 6 ++--- > > tests/unit_tests/openvpn/test_cryptoapi.c | 24 ++++++++++++++------ > > tests/unit_tests/openvpn/test_pkcs11.c | 27 ++++++++++++++++------- > > 3 files changed, 39 insertions(+), 18 deletions(-) > > > > diff --git a/tests/unit_tests/openvpn/cert_data.h > b/tests/unit_tests/openvpn/cert_data.h > > index 33de35ec..0886b071 100644 > > --- a/tests/unit_tests/openvpn/cert_data.h > > +++ b/tests/unit_tests/openvpn/cert_data.h > > @@ -79,7 +79,7 @@ static const char *const cert2 = > > > "HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n" > > "KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n" > > "-----END CERTIFICATE-----\n"; > > -static const char *const key2 = key1; > > +#define key2 key1 > > So we are not getting rid of #defines yet? Can't we make this data > somehow local? Personally, I would probably - at least outside unit tests - write (key1) instead of bare key, so that it's clear this is one name and can't be somehow extended. I was not getting rid of any defines -- just trying to avoid defines if it can be done without jumping through too many hoops. Right, (key1) would be better. Instead of making key2 local, we can write it out fully without referring to the fact that key1 = key2. The latter was anyway a lazy man's choice, not relevant for the working of the tests. If anyone feels strongly I can submit a v2 with key2, key4, and cname4 fully written out like: static const char *const key2 = "-----BEGIN PRIVATE KEY-----\n" ....... etc.. static const char *cname4 = "ovpn-test-rsa1"; That may also make maintaining this data easier -- ie., any cert/key pair could be replaced without affecting others. Selva
Am 27.03.23 um 16:45 schrieb Selva Nair: > Hi, > > On Mon, Mar 27, 2023 at 9:59 AM Matthias Andree > <matthias.andree@gmx.de> wrote: > > Am 27.03.23 um 13:49 schrieb selva.nair@gmail.com: > > From: Selva Nair <selva.nair@gmail.com> > > > > - Do not use non-literal initializers for static objects > > - Replace empty initializer {} by {0} > > Should we go to a revision, I would suggest to not make something > compliant to a compiler because that is assigning it way too much > power > and control. A terms such as "ready to be compiled with MSVC" or so > would be in order, or making this code compliant with some wisely > chosen > version of the ISO 9899 standard, aka. standard C. > > > In this case "MSVC compliant" was arguably a poor choice of words as > what the change does is to avoid non-C99 constructs. However it made > sense to refer to MSVC in the context of what prompted this patch. > > That said, in a cross-platform code base one often has to make changes > to please compilers just to get things to build. I don't mind what we're doing here... just that I want to avoid the impression as though MSVC were setting the standards. There's always MinGW-GCC... That being said, here's a proposal: ----- Subject: Make cert_data.h and test_cryptoapi/pkcs11.c C99 compliant ...to fix compilation on MSVC 2143 <insert actual version and relevant C language settings here>: - Do not use non-literal initializers for static objects - Replace empty initializer {} by {0} > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > ----- > I was not getting rid of any defines -- just trying to avoid defines > if it can be done without jumping through too many hoops. Right, > (key1) would be better. > > Instead of making key2 local, we can write it out fully without > referring to the fact that key1 = key2. The latter was anyway a lazy > man's choice, not relevant for the working of the tests. > > If anyone feels strongly I can submit a v2 with key2, key4, and cname4 > fully written out like: That is not what I am proposing, redundance in source code is not a valid "fix" or approach... -> [1] It's just... I have been writing too much C++14 or C++17 code lately (on MSVC 2015, 2017, 2022 and with MS Build Tools 2019 because that was the customer's newest-limit) to want to mess with C limitations that serve their purpose on paper, but are getting in the way in practice. Of course then you have discussions about C++ linkage (the only reasonable solution is "avoid"), exceptions (serious discussion what and where) and about contributors. I am aware C++ is abhorred by some but I usually find they set their minds in the first decade of the millennium, literally before C++11, but that would also be the side door to sneak it in because people who believe their strange OS doesn't have a C++ compiler can still compile OpenVPN proper, just not unit tests. > That may also make maintaining this data easier -- ie., any cert/key > pair could be replaced without affecting others. [1] So I would go the pragmatic way here and just change once there is really a need to replace certificates. It's only a test after all.
On Mon, Mar 27, 2023 at 09:45:53PM +0200, Matthias Andree wrote: > Am 27.03.23 um 16:45 schrieb Selva Nair: > > Hi, > > > > On Mon, Mar 27, 2023 at 9:59 AM Matthias Andree > > <matthias.andree@gmx.de> wrote: > > > > Am 27.03.23 um 13:49 schrieb selva.nair@gmail.com: > > > From: Selva Nair <selva.nair@gmail.com> > > > > > > - Do not use non-literal initializers for static objects > > > - Replace empty initializer {} by {0} > > > > Should we go to a revision, I would suggest to not make something > > compliant to a compiler because that is assigning it way too much > > power > > and control. A terms such as "ready to be compiled with MSVC" or so > > would be in order, or making this code compliant with some wisely > > chosen > > version of the ISO 9899 standard, aka. standard C. > > > > > > In this case "MSVC compliant" was arguably a poor choice of words as > > what the change does is to avoid non-C99 constructs. However it made > > sense to refer to MSVC in the context of what prompted this patch. > > > > That said, in a cross-platform code base one often has to make changes > > to please compilers just to get things to build. > > I don't mind what we're doing here... just that I want to avoid the > impression as though MSVC were setting the standards. There's always > MinGW-GCC... GCC doesn't seem to care about any of this. Which doesn't really make it easier to write code that adheres to standards. Regards,
Am 28.03.23 um 10:47 schrieb Frank Lichtenheld: > On Mon, Mar 27, 2023 at 09:45:53PM +0200, Matthias Andree wrote: >> Am 27.03.23 um 16:45 schrieb Selva Nair: >>> Hi, >>> >>> On Mon, Mar 27, 2023 at 9:59 AM Matthias Andree >>> <matthias.andree@gmx.de> wrote: >>> >>> Am 27.03.23 um 13:49 schrieb selva.nair@gmail.com: >>> > From: Selva Nair <selva.nair@gmail.com> >>> > >>> > - Do not use non-literal initializers for static objects >>> > - Replace empty initializer {} by {0} >>> >>> Should we go to a revision, I would suggest to not make something >>> compliant to a compiler because that is assigning it way too much >>> power >>> and control. A terms such as "ready to be compiled with MSVC" or so >>> would be in order, or making this code compliant with some wisely >>> chosen >>> version of the ISO 9899 standard, aka. standard C. >>> >>> >>> In this case "MSVC compliant" was arguably a poor choice of words as >>> what the change does is to avoid non-C99 constructs. However it made >>> sense to refer to MSVC in the context of what prompted this patch. >>> >>> That said, in a cross-platform code base one often has to make changes >>> to please compilers just to get things to build. >> >> I don't mind what we're doing here... just that I want to avoid the >> impression as though MSVC were setting the standards. There's always >> MinGW-GCC... > > GCC doesn't seem to care about any of this. Which doesn't really make > it easier to write code that adheres to standards. Which is why it might be the easy route to a Windows executable. As long as the calling convention matches and there are no by-extension exceptions, even GCC and Windows objects should mix. OTOH I share the pain that I have yet to find that "disallow all extensions" switch, for portability checks. -pedantic-errors -std=c99 is going some way, but apparently not all the way. clang may be a bit pickier, and it will be really complaining about missing prototypes. It's still compliance with language standards though, not with compilers.
I have only loosely followed the discussion, but since this has an ACK *and* passes all GHA compiles and tests, this is good enough for me :-) I have added the "co-authored-by" as requested. Your patch has been applied to the master branch. commit 846951665a60424b98097ad0a77ec6cb1c3d05ac Author: Selva Nair Date: Mon Mar 27 07:49:37 2023 -0400 Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant Signed-off-by: Selva Nair <selva.nair@gmail.com> Acked-by: Frank Lichtenheld <frank@lichtenheld.com> Message-Id: <20230327114937.28246-1-selva.nair@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26525.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/tests/unit_tests/openvpn/cert_data.h b/tests/unit_tests/openvpn/cert_data.h index 33de35ec..0886b071 100644 --- a/tests/unit_tests/openvpn/cert_data.h +++ b/tests/unit_tests/openvpn/cert_data.h @@ -79,7 +79,7 @@ static const char *const cert2 = "HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n" "KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n" "-----END CERTIFICATE-----\n"; -static const char *const key2 = key1; +#define key2 key1 static const char *const hash2 = "FA18FD34BAABE47D6E2910E080F421C109CA97F5"; static const char *const cname2 = "ovpn-test-ec2"; @@ -159,8 +159,8 @@ static const char *const cert4 = "353PpJJ9s2b/Fqoc4d7udqhQogA7jqbayTKhJxbT134l2NzqDROzuS0kXbX8bXCi\n" "mXSa4c8=\n" "-----END CERTIFICATE-----\n"; -static const char *const key4 = key3; +#define key4 key3 static const char *const hash4 = "E1401D4497C944783E3D62CDBD2A1F69F5E5071E"; -static const char *const cname4 = cname3; /* same CN as that of cert3 */ +#define cname4 cname3 /* same CN as that of cert3 */ #endif /* CERT_DATA_H */ diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c b/tests/unit_tests/openvpn/test_cryptoapi.c index c8468103..2150b77c 100644 --- a/tests/unit_tests/openvpn/test_cryptoapi.c +++ b/tests/unit_tests/openvpn/test_cryptoapi.c @@ -99,17 +99,26 @@ static struct test_cert const char *const friendly_name; /* identifies certs loaded to the store -- keep unique */ const char *hash; /* SHA1 fingerprint */ int valid; /* nonzero if certificate has not expired */ -} certs[] = { - {cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", hash1, 1}, - {cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", hash2, 1}, - {cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", hash3, 1}, - {cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", hash4, 0}, - {} -}; +} certs[5]; static bool certs_loaded; static HCERTSTORE user_store; +/* Fill-in certs[] array */ +void +init_cert_data() +{ + struct test_cert certs_local[] = { + {cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", hash1, 1}, + {cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", hash2, 1}, + {cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", hash3, 1}, + {cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", hash4, 0}, + {0} + }; + assert(sizeof(certs_local) == sizeof(certs)); + memcpy(certs, certs_local, sizeof(certs_local)); +} + /* Lookup a certificate in our certificate/key db */ static struct test_cert * lookup_cert(const char *friendly_name) @@ -131,6 +140,7 @@ import_certs(void **state) { return; } + init_cert_data(); user_store = CertOpenStore(CERT_STORE_PROV_SYSTEM, 0, 0, CERT_SYSTEM_STORE_CURRENT_USER |CERT_STORE_OPEN_EXISTING_FLAG, L"MY"); assert_non_null(user_store); diff --git a/tests/unit_tests/openvpn/test_pkcs11.c b/tests/unit_tests/openvpn/test_pkcs11.c index ea394bea..df5f8c7e 100644 --- a/tests/unit_tests/openvpn/test_pkcs11.c +++ b/tests/unit_tests/openvpn/test_pkcs11.c @@ -112,13 +112,7 @@ static struct test_cert const char *const friendly_name; /* identifies certs loaded to the store -- keep unique */ uint8_t hash[HASHSIZE]; /* SHA1 fingerprint: computed and filled in later */ char *p11_id; /* PKCS#11 id -- filled in later */ -} certs[] = { - {cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", {}, NULL}, - {cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", {}, NULL}, - {cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", {}, NULL}, - {cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", {}, NULL}, - {} -}; +} certs[5]; static bool pkcs11_id_management; static char softhsm2_tokens_path[] = "softhsm2_tokens_XXXXXX"; @@ -127,6 +121,21 @@ int num_certs; static const char *pkcs11_id_current; struct env_set *es; +/* Fill-in certs[] array */ +void +init_cert_data() +{ + struct test_cert certs_local[] = { + {cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", {0}, NULL}, + {cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", {0}, NULL}, + {cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", {0}, NULL}, + {cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", {0}, NULL}, + {0} + }; + assert(sizeof(certs_local) == sizeof(certs)); + memcpy(certs, certs_local, sizeof(certs_local)); +} + /* Intercept get_user_pass for PIN and other prompts */ bool get_user_pass_cr(struct user_pass *up, const char *auth_file, const char *prefix, @@ -173,6 +182,7 @@ init(void **state) umask(0077); /* ensure all files and directories we create get user only access */ char config[256]; + init_cert_data(); if (!mkdtemp(softhsm2_tokens_path)) { fail_msg("make tmpdir using template <%s> failed (error = %d)", softhsm2_tokens_path, errno); @@ -416,7 +426,8 @@ test_tls_ctx_use_pkcs11(void **state) assert_non_null(pubkey); assert_non_null(privkey); #ifdef HAVE_XKEY_PROVIDER - digest_sign_verify(privkey, pubkey); /* this will exercise signing via pkcs11 backend */ + /* this will exercise signing via pkcs11 backend */ + assert_int_equal(digest_sign_verify(privkey, pubkey), 1); #else if (!SSL_CTX_check_private_key(tls_ctx.ctx)) {