| Message ID | 20210818143051.19402-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] Replace DIY AES with openssl if openssl is available | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 8/18/21 4:30 PM, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > This allows to leverage the openssl implementation which can use > hardware crypto on supported platforms. > > UUID generation speed is improved by ~ 12% on an AMD Ryzen with > support for AES instructions. > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> Hi, Anton. Thanks for working on this. The problem with this implementation is that we're loosing unit-tests for our own AES library. I think, we still need to test it (i.e. by tests/aes128.at). And we need to test that we're invoking openssl correctly in the same testsuite. Would be great to avoid extra CI job for --disable-ssl case. Few comments inline. Best regards, Ilya Maximets. > --- > lib/aes128.c | 34 ++++++++++++++++++++++++++++++++++ > lib/aes128.h | 16 ++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/lib/aes128.c b/lib/aes128.c > index 98447d14b..207925b58 100644 > --- a/lib/aes128.c > +++ b/lib/aes128.c > @@ -28,6 +28,39 @@ > > #include "util.h" > > +#ifdef HAVE_OPENSSL > + > + > + Too much empty space. > +#include <openssl/conf.h> > +#include <openssl/evp.h> > +#include <openssl/err.h> > +#include <string.h> > +#include "entropy.h" > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(aes); > + > +void aes128_schedule(struct aes128 *aes, const uint8_t key[16]) > +{ > + uint8_t iv[16]; > + aes->ctx = EVP_CIPHER_CTX_new(); > + memset(iv, 0, sizeof iv); > + if (EVP_EncryptInit_ex(aes->ctx, EVP_aes_128_cbc(), NULL, key, iv) != 1) { > + VLOG_FATAL("Encryption init failed"); Would be great to have a better error message here, explaining what happened in a bit more details. > + } > +} > + > +void aes128_encrypt(const struct aes128 *aes, const void *plain, void *cipher) > +{ > + int len; > + if (1 != EVP_EncryptUpdate(aes->ctx, cipher, &len, plain, 16)) { > + VLOG_FATAL("Encryption failed"); Same here. > + } > +} > + > +#else > + > static const uint32_t Te0[256] = { > 0xc66363a5U, 0xf87c7c84U, 0xee777799U, 0xf67b7b8dU, > 0xfff2f20dU, 0xd66b6bbdU, 0xde6f6fb1U, 0x91c5c554U, > @@ -507,3 +540,4 @@ aes128_encrypt(const struct aes128 *aes, const void *input_, void *output_) > ^ rk[3]); > put_u32(output + 12, s3); > } > +#endif > diff --git a/lib/aes128.h b/lib/aes128.h > index f0f55d7cf..efa71c764 100644 > --- a/lib/aes128.h > +++ b/lib/aes128.h > @@ -25,12 +25,28 @@ > #ifndef AES128_H > #define AES128_H > > +#include <config.h> Header files should not include config.h. > #include <stdint.h> > > +#ifdef HAVE_OPENSSL > + > +#include <openssl/conf.h> > +#include <openssl/evp.h> > +#include <openssl/err.h> > +#include <string.h> > + > +struct aes128 { > + EVP_CIPHER_CTX *ctx; > +}; > + > +#else > + > struct aes128 { > uint32_t rk[128/8 + 28]; > }; > > +#endif > + > void aes128_schedule(struct aes128 *, const uint8_t key[16]); > void aes128_encrypt(const struct aes128 *, const void *, void *); > >
On 8/18/21 6:40 PM, Ilya Maximets wrote: > On 8/18/21 4:30 PM, anton.ivanov@cambridgegreys.com wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> This allows to leverage the openssl implementation which can use >> hardware crypto on supported platforms. >> >> UUID generation speed is improved by ~ 12% on an AMD Ryzen with >> support for AES instructions. >> >> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > Hi, Anton. > > Thanks for working on this. The problem with this implementation > is that we're loosing unit-tests for our own AES library. > I think, we still need to test it (i.e. by tests/aes128.at). And we > need to test that we're invoking openssl correctly in the same > testsuite. Would be great to avoid extra CI job for --disable-ssl > case. For the context, I was working on the similar patch for SHA1 and it has the same problem that I didn't solve yet. That's why I still didn't post it. > > Few comments inline. > > Best regards, Ilya Maximets. > >> --- >> lib/aes128.c | 34 ++++++++++++++++++++++++++++++++++ >> lib/aes128.h | 16 ++++++++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/lib/aes128.c b/lib/aes128.c >> index 98447d14b..207925b58 100644 >> --- a/lib/aes128.c >> +++ b/lib/aes128.c >> @@ -28,6 +28,39 @@ >> >> #include "util.h" >> >> +#ifdef HAVE_OPENSSL >> + >> + >> + > > Too much empty space. > >> +#include <openssl/conf.h> >> +#include <openssl/evp.h> >> +#include <openssl/err.h> >> +#include <string.h> >> +#include "entropy.h" >> +#include "openvswitch/vlog.h" >> + >> +VLOG_DEFINE_THIS_MODULE(aes); >> + >> +void aes128_schedule(struct aes128 *aes, const uint8_t key[16]) >> +{ >> + uint8_t iv[16]; >> + aes->ctx = EVP_CIPHER_CTX_new(); >> + memset(iv, 0, sizeof iv); >> + if (EVP_EncryptInit_ex(aes->ctx, EVP_aes_128_cbc(), NULL, key, iv) != 1) { >> + VLOG_FATAL("Encryption init failed"); > > Would be great to have a better error message here, explaining > what happened in a bit more details. > > >> + } >> +} >> + >> +void aes128_encrypt(const struct aes128 *aes, const void *plain, void *cipher) >> +{ >> + int len; >> + if (1 != EVP_EncryptUpdate(aes->ctx, cipher, &len, plain, 16)) { >> + VLOG_FATAL("Encryption failed"); > > Same here. > >> + } >> +} >> + >> +#else >> + >> static const uint32_t Te0[256] = { >> 0xc66363a5U, 0xf87c7c84U, 0xee777799U, 0xf67b7b8dU, >> 0xfff2f20dU, 0xd66b6bbdU, 0xde6f6fb1U, 0x91c5c554U, >> @@ -507,3 +540,4 @@ aes128_encrypt(const struct aes128 *aes, const void *input_, void *output_) >> ^ rk[3]); >> put_u32(output + 12, s3); >> } >> +#endif >> diff --git a/lib/aes128.h b/lib/aes128.h >> index f0f55d7cf..efa71c764 100644 >> --- a/lib/aes128.h >> +++ b/lib/aes128.h >> @@ -25,12 +25,28 @@ >> #ifndef AES128_H >> #define AES128_H >> >> +#include <config.h> > > Header files should not include config.h. > >> #include <stdint.h> >> >> +#ifdef HAVE_OPENSSL >> + >> +#include <openssl/conf.h> >> +#include <openssl/evp.h> >> +#include <openssl/err.h> >> +#include <string.h> >> + >> +struct aes128 { >> + EVP_CIPHER_CTX *ctx; >> +}; >> + >> +#else >> + >> struct aes128 { >> uint32_t rk[128/8 + 28]; >> }; >> >> +#endif >> + >> void aes128_schedule(struct aes128 *, const uint8_t key[16]); >> void aes128_encrypt(const struct aes128 *, const void *, void *); >> >> >
On 18/08/2021 17:40, Ilya Maximets wrote: > On 8/18/21 4:30 PM, anton.ivanov@cambridgegreys.com wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> This allows to leverage the openssl implementation which can use >> hardware crypto on supported platforms. >> >> UUID generation speed is improved by ~ 12% on an AMD Ryzen with >> support for AES instructions. >> >> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> > Hi, Anton. > > Thanks for working on this. The problem with this implementation > is that we're loosing unit-tests for our own AES library. > I think, we still need to test it (i.e. by tests/aes128.at). They work for me here (Debian buster). I do not understand why they failed in the automated tests by the robot. > And we > need to test that we're invoking openssl correctly in the same > testsuite. Would be great to avoid extra CI job for --disable-ssl > case. It is a compile level choice, it will be difficult to test them at the same time. > > Few comments inline. > > Best regards, Ilya Maximets. > >> --- >> lib/aes128.c | 34 ++++++++++++++++++++++++++++++++++ >> lib/aes128.h | 16 ++++++++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/lib/aes128.c b/lib/aes128.c >> index 98447d14b..207925b58 100644 >> --- a/lib/aes128.c >> +++ b/lib/aes128.c >> @@ -28,6 +28,39 @@ >> >> #include "util.h" >> >> +#ifdef HAVE_OPENSSL >> + >> + >> + > Too much empty space. > >> +#include <openssl/conf.h> >> +#include <openssl/evp.h> >> +#include <openssl/err.h> >> +#include <string.h> >> +#include "entropy.h" >> +#include "openvswitch/vlog.h" >> + >> +VLOG_DEFINE_THIS_MODULE(aes); >> + >> +void aes128_schedule(struct aes128 *aes, const uint8_t key[16]) >> +{ >> + uint8_t iv[16]; >> + aes->ctx = EVP_CIPHER_CTX_new(); >> + memset(iv, 0, sizeof iv); >> + if (EVP_EncryptInit_ex(aes->ctx, EVP_aes_128_cbc(), NULL, key, iv) != 1) { >> + VLOG_FATAL("Encryption init failed"); > Would be great to have a better error message here, explaining > what happened in a bit more details. Ugh... Openssl error handling... OK. Fine, I will add it. > > >> + } >> +} >> + >> +void aes128_encrypt(const struct aes128 *aes, const void *plain, void *cipher) >> +{ >> + int len; >> + if (1 != EVP_EncryptUpdate(aes->ctx, cipher, &len, plain, 16)) { >> + VLOG_FATAL("Encryption failed"); > Same here. Ack. > >> + } >> +} >> + >> +#else >> + >> static const uint32_t Te0[256] = { >> 0xc66363a5U, 0xf87c7c84U, 0xee777799U, 0xf67b7b8dU, >> 0xfff2f20dU, 0xd66b6bbdU, 0xde6f6fb1U, 0x91c5c554U, >> @@ -507,3 +540,4 @@ aes128_encrypt(const struct aes128 *aes, const void *input_, void *output_) >> ^ rk[3]); >> put_u32(output + 12, s3); >> } >> +#endif >> diff --git a/lib/aes128.h b/lib/aes128.h >> index f0f55d7cf..efa71c764 100644 >> --- a/lib/aes128.h >> +++ b/lib/aes128.h >> @@ -25,12 +25,28 @@ >> #ifndef AES128_H >> #define AES128_H >> >> +#include <config.h> > Header files should not include config.h. We need to know if OPENSSL is used or not because the contents of the aes structure depend on it. On the other hand, IIRC aes is opaque to users, so it may work if it is not declared explicitly. I will try that tomorrow. > >> #include <stdint.h> >> >> +#ifdef HAVE_OPENSSL >> + >> +#include <openssl/conf.h> >> +#include <openssl/evp.h> >> +#include <openssl/err.h> >> +#include <string.h> >> + >> +struct aes128 { >> + EVP_CIPHER_CTX *ctx; >> +}; >> + >> +#else >> + >> struct aes128 { >> uint32_t rk[128/8 + 28]; >> }; >> >> +#endif >> + >> void aes128_schedule(struct aes128 *, const uint8_t key[16]); >> void aes128_encrypt(const struct aes128 *, const void *, void *); >> >> >
On 18/08/2021 18:44, Anton Ivanov wrote: > On 18/08/2021 17:40, Ilya Maximets wrote: >> On 8/18/21 4:30 PM, anton.ivanov@cambridgegreys.com wrote: >>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> >>> This allows to leverage the openssl implementation which can use >>> hardware crypto on supported platforms. >>> >>> UUID generation speed is improved by ~ 12% on an AMD Ryzen with >>> support for AES instructions. >>> >>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> Hi, Anton. >> >> Thanks for working on this. The problem with this implementation >> is that we're loosing unit-tests for our own AES library. >> I think, we still need to test it (i.e. by tests/aes128.at). > > They work for me here (Debian buster). I do not understand why they > failed in the automated tests by the robot. Sorry, misunderstood your point. I see your point now. One option may be to reorganize it in the following manner. 1. Rename the original functions. 2. Have the API functions call the newly renamed ones for the non openssl case. 3. Change the test to call the renamed ones directly instead of using the wrappers. This way we retain the test cases. but we do not test that we have integrated openssl correctly. > >> And we >> need to test that we're invoking openssl correctly in the same >> testsuite. Would be great to avoid extra CI job for --disable-ssl >> case. > > It is a compile level choice, it will be difficult to test them at the > same time. > >> >> Few comments inline. >> >> Best regards, Ilya Maximets. >> >>> --- >>> lib/aes128.c | 34 ++++++++++++++++++++++++++++++++++ >>> lib/aes128.h | 16 ++++++++++++++++ >>> 2 files changed, 50 insertions(+) >>> >>> diff --git a/lib/aes128.c b/lib/aes128.c >>> index 98447d14b..207925b58 100644 >>> --- a/lib/aes128.c >>> +++ b/lib/aes128.c >>> @@ -28,6 +28,39 @@ >>> #include "util.h" >>> +#ifdef HAVE_OPENSSL >>> + >>> + >>> + >> Too much empty space. >> >>> +#include <openssl/conf.h> >>> +#include <openssl/evp.h> >>> +#include <openssl/err.h> >>> +#include <string.h> >>> +#include "entropy.h" >>> +#include "openvswitch/vlog.h" >>> + >>> +VLOG_DEFINE_THIS_MODULE(aes); >>> + >>> +void aes128_schedule(struct aes128 *aes, const uint8_t key[16]) >>> +{ >>> + uint8_t iv[16]; >>> + aes->ctx = EVP_CIPHER_CTX_new(); >>> + memset(iv, 0, sizeof iv); >>> + if (EVP_EncryptInit_ex(aes->ctx, EVP_aes_128_cbc(), NULL, key, >>> iv) != 1) { >>> + VLOG_FATAL("Encryption init failed"); >> Would be great to have a better error message here, explaining >> what happened in a bit more details. > > Ugh... Openssl error handling... > > OK. Fine, I will add it. > >> >> >>> + } >>> +} >>> + >>> +void aes128_encrypt(const struct aes128 *aes, const void *plain, >>> void *cipher) >>> +{ >>> + int len; >>> + if (1 != EVP_EncryptUpdate(aes->ctx, cipher, &len, plain, 16)) { >>> + VLOG_FATAL("Encryption failed"); >> Same here. > > Ack. > >> >>> + } >>> +} >>> + >>> +#else >>> + >>> static const uint32_t Te0[256] = { >>> 0xc66363a5U, 0xf87c7c84U, 0xee777799U, 0xf67b7b8dU, >>> 0xfff2f20dU, 0xd66b6bbdU, 0xde6f6fb1U, 0x91c5c554U, >>> @@ -507,3 +540,4 @@ aes128_encrypt(const struct aes128 *aes, const >>> void *input_, void *output_) >>> ^ rk[3]); >>> put_u32(output + 12, s3); >>> } >>> +#endif >>> diff --git a/lib/aes128.h b/lib/aes128.h >>> index f0f55d7cf..efa71c764 100644 >>> --- a/lib/aes128.h >>> +++ b/lib/aes128.h >>> @@ -25,12 +25,28 @@ >>> #ifndef AES128_H >>> #define AES128_H >>> +#include <config.h> >> Header files should not include config.h. > > We need to know if OPENSSL is used or not because the contents of the > aes structure depend on it. > > On the other hand, IIRC aes is opaque to users, so it may work if it > is not declared explicitly. I will try that tomorrow. > >> >>> #include <stdint.h> >>> +#ifdef HAVE_OPENSSL >>> + >>> +#include <openssl/conf.h> >>> +#include <openssl/evp.h> >>> +#include <openssl/err.h> >>> +#include <string.h> >>> + >>> +struct aes128 { >>> + EVP_CIPHER_CTX *ctx; >>> +}; >>> + >>> +#else >>> + >>> struct aes128 { >>> uint32_t rk[128/8 + 28]; >>> }; >>> +#endif >>> + >>> void aes128_schedule(struct aes128 *, const uint8_t key[16]); >>> void aes128_encrypt(const struct aes128 *, const void *, void *); >>> >> >
diff --git a/lib/aes128.c b/lib/aes128.c index 98447d14b..207925b58 100644 --- a/lib/aes128.c +++ b/lib/aes128.c @@ -28,6 +28,39 @@ #include "util.h" +#ifdef HAVE_OPENSSL + + + +#include <openssl/conf.h> +#include <openssl/evp.h> +#include <openssl/err.h> +#include <string.h> +#include "entropy.h" +#include "openvswitch/vlog.h" + +VLOG_DEFINE_THIS_MODULE(aes); + +void aes128_schedule(struct aes128 *aes, const uint8_t key[16]) +{ + uint8_t iv[16]; + aes->ctx = EVP_CIPHER_CTX_new(); + memset(iv, 0, sizeof iv); + if (EVP_EncryptInit_ex(aes->ctx, EVP_aes_128_cbc(), NULL, key, iv) != 1) { + VLOG_FATAL("Encryption init failed"); + } +} + +void aes128_encrypt(const struct aes128 *aes, const void *plain, void *cipher) +{ + int len; + if (1 != EVP_EncryptUpdate(aes->ctx, cipher, &len, plain, 16)) { + VLOG_FATAL("Encryption failed"); + } +} + +#else + static const uint32_t Te0[256] = { 0xc66363a5U, 0xf87c7c84U, 0xee777799U, 0xf67b7b8dU, 0xfff2f20dU, 0xd66b6bbdU, 0xde6f6fb1U, 0x91c5c554U, @@ -507,3 +540,4 @@ aes128_encrypt(const struct aes128 *aes, const void *input_, void *output_) ^ rk[3]); put_u32(output + 12, s3); } +#endif diff --git a/lib/aes128.h b/lib/aes128.h index f0f55d7cf..efa71c764 100644 --- a/lib/aes128.h +++ b/lib/aes128.h @@ -25,12 +25,28 @@ #ifndef AES128_H #define AES128_H +#include <config.h> #include <stdint.h> +#ifdef HAVE_OPENSSL + +#include <openssl/conf.h> +#include <openssl/evp.h> +#include <openssl/err.h> +#include <string.h> + +struct aes128 { + EVP_CIPHER_CTX *ctx; +}; + +#else + struct aes128 { uint32_t rk[128/8 + 28]; }; +#endif + void aes128_schedule(struct aes128 *, const uint8_t key[16]); void aes128_encrypt(const struct aes128 *, const void *, void *);