| Message ID | 20180117131046.32402-1-logout@free.fr |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel,v3,1/2] OpenSSL: remove some EVP_PKEY type checks | expand |
Hi, On 17-01-18 14:10, Emmanuel Deloget wrote: > Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as > the same check is also performed in the later. > > We also make the code a bit better by not calling the various > EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to > avoid introducing yet another #ifndef OPENSSL_NO_EC in the code). > > Signed-off-by: Emmanuel Deloget <logout@free.fr> > --- > src/openvpn/ssl_openssl.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index 86318d4c..7061f536 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) > EVP_PKEY *pkey = X509_get_pubkey(cert); > if (pkey != NULL) > { > - if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) && (EVP_PKEY_get0_RSA(pkey) != NULL)) > - { > - RSA *rsa = EVP_PKEY_get0_RSA(pkey); > - openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", > - RSA_bits(rsa)); > - } > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) && (EVP_PKEY_get0_DSA(pkey) != NULL)) > - { > - DSA *dsa = EVP_PKEY_get0_DSA(pkey); > - openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA", > - DSA_bits(dsa)); > - } > + RSA *rsa = EVP_PKEY_get0_RSA(pkey); > + DSA *dsa = EVP_PKEY_get0_DSA(pkey); > #ifndef OPENSSL_NO_EC > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL)) > + EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); > + > + if (ec != NULL) > { > - EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); > const EC_GROUP *group = EC_KEY_get0_group(ec); > const char* curve; > > @@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) > > openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve: %s", > EC_GROUP_order_bits(group), curve); > - > - } > + } else > #endif > + if (rsa != NULL) > + { > + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", > + RSA_bits(rsa)); > + } > + else if (dsa != NULL) > + { > + openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA", > + DSA_bits(dsa)); > + } > + > EVP_PKEY_free(pkey); > } > X509_free(cert); > Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a non-foo key as an error. Running 'make check' with this patch and openssl 1.1.0 fails: Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope routines:EVP_PKEY_get0_DSA:expecting a dsa key Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope routines:EVP_PKEY_get0_EC_KEY:expecting a ec key Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) ) Sorry for not spotting this earlier, could have saved us quite a bit of work... -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger <steffan@karger.me> wrote: > Hi, > > On 17-01-18 14:10, Emmanuel Deloget wrote: >> Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as >> the same check is also performed in the later. >> >> We also make the code a bit better by not calling the various >> EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to >> avoid introducing yet another #ifndef OPENSSL_NO_EC in the code). >> >> Signed-off-by: Emmanuel Deloget <logout@free.fr> >> --- .. > Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a > non-foo key as an error. Running 'make check' with this patch and > openssl 1.1.0 fails: > > Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2 > ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA > Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope > routines:EVP_PKEY_get0_DSA:expecting a dsa key > Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope > routines:EVP_PKEY_get0_EC_KEY:expecting a ec key > Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error > > So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) ) > > Sorry for not spotting this earlier, could have saved us quite a bit of > work... I'm surprised that my argument about if (EVP_PKEY_id(foo) == ...EC..) { do EC stuff } else if (EVP_PKEY_id(foo) == ..RSA..) { do RSA stuff } or switch(EVP_PKEY_id(foo)) being stylistically better[tm] did not work! Still pleased by the end result. Now I get to spit out some of the "if (EVP_PKEY_get0_RSA(..))" that was pushed down my throat :). For patches already on the ML will do so after review. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hello, and sorry for the delay (things like 'real life', you know). On Sat, Jan 20, 2018 at 3:22 PM, Selva Nair <selva.nair@gmail.com> wrote: > Hi, > > On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger <steffan@karger.me> wrote: > > Hi, > > > > On 17-01-18 14:10, Emmanuel Deloget wrote: > >> Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as > >> the same check is also performed in the later. > >> > >> We also make the code a bit better by not calling the various > >> EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to > >> avoid introducing yet another #ifndef OPENSSL_NO_EC in the code). > >> > >> Signed-off-by: Emmanuel Deloget <logout@free.fr> > >> --- > > .. > > > Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a > > non-foo key as an error. Running 'make check' with this patch and > > openssl 1.1.0 fails: > > > > Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2 > > ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA > > Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope > > routines:EVP_PKEY_get0_DSA:expecting a dsa key > > Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope > > routines:EVP_PKEY_get0_EC_KEY:expecting a ec key > > Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error > > > > So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) ) > > > > Sorry for not spotting this earlier, could have saved us quite a bit of > > work... > I should have spotted that before I sent the change, but I only tested it with 1.0.2 and compile-tested it against 1.1. I'm going to refactor my test environment in order to avoid any further error like this one. > > I'm surprised that my argument about > > if (EVP_PKEY_id(foo) == ...EC..) { do EC stuff } > else if (EVP_PKEY_id(foo) == ..RSA..) { do RSA stuff } > > or switch(EVP_PKEY_id(foo)) > > being stylistically better[tm] did not work! > > Still pleased by the end result. Now I get to spit out some of the > "if (EVP_PKEY_get0_RSA(..))" that was pushed down my throat :). > For patches already on the ML will do so after review. > > I like the switch - I find it elegant - but it does not avoid an internal 'if', so the code would look like: switch (EVP_PKEY_id(foo)) { case ..RSA..: { RSA *rsa = EVP_PKEY_get0_RSA(foo); if (rsa) { openvpn_printf(...); } } break; case ... } If this is acceptable, I can respin patch 2. Otherwise, feel free to discard both patch 2 and patch 3 of the series as any change in patch 2 would not be a largen enhancement of the already existing code. > Selva > > Best regards, -- Emmanuel Deloget <div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hello, </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">and sorry for the delay (things like 'real life', you know). </div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jan 20, 2018 at 3:22 PM, Selva Nair <span dir="ltr"><<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <span class="gmail-"><br> On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger <<a href="mailto:steffan@karger.me">steffan@karger.me</a>> wrote:<br> > Hi,<br> ><br> > On 17-01-18 14:10, Emmanuel Deloget wrote:<br> >> Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as<br> >> the same check is also performed in the later.<br> >><br> >> We also make the code a bit better by not calling the various<br> >> EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to<br> >> avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).<br> >><br> >> Signed-off-by: Emmanuel Deloget <<a href="mailto:logout@free.fr">logout@free.fr</a>><br> >> ---<br> <br> </span>..<br> <span class="gmail-"><br> > Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a<br> > non-foo key as an error. Running 'make check' with this patch and<br> > openssl 1.1.0 fails:<br> ><br> > Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2<br> > ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA<br> > Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope<br> > routines:EVP_PKEY_get0_DSA:<wbr>expecting a dsa key<br> > Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope<br> > routines:EVP_PKEY_get0_EC_KEY:<wbr>expecting a ec key<br> > Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error<br> ><br> > So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) )<br> ><br> > Sorry for not spotting this earlier, could have saved us quite a bit of<br> > work...<br></span></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">I should have spotted that before I sent the change, but I only tested it with 1.0.2 and compile-tested it against 1.1. I'm going to refactor my test environment in order to avoid any further error like this one. </div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"> <br> </span>I'm surprised that my argument about<br> <br> if (EVP_PKEY_id(foo) == ...EC..) { do EC stuff }<br> else if (EVP_PKEY_id(foo) == ..RSA..) { do RSA stuff }<br> <br> or switch(EVP_PKEY_id(foo))<br> <br> being stylistically better[tm] did not work!<br> <br> Still pleased by the end result. Now I get to spit out some of the<br> "if (EVP_PKEY_get0_RSA(..))" that was pushed down my throat :).<br> For patches already on the ML will do so after review.<br> <span class="gmail-HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">I like the switch - I find it elegant - but it does not avoid an internal 'if', so the code would look like:</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">switch (EVP_PKEY_id(foo)) {</div><div class="gmail_default" style="font-family:monospace,monospace">case ..RSA..: {</div><div class="gmail_default" style="font-family:monospace,monospace"> RSA *rsa = EVP_PKEY_get0_RSA(foo);</div><div class="gmail_default" style="font-family:monospace,monospace"> if (rsa) {</div><div class="gmail_default" style="font-family:monospace,monospace"> openvpn_printf(...);</div><div class="gmail_default" style="font-family:monospace,monospace"> }</div><div class="gmail_default" style="font-family:monospace,monospace"> }</div><div class="gmail_default" style="font-family:monospace,monospace"> break;</div><div class="gmail_default" style="font-family:monospace,monospace">case ...</div><div class="gmail_default" style="font-family:monospace,monospace">}<br></div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace">If this is acceptable, I can respin patch 2. Otherwise, feel free to discard both patch 2 and patch 3 of the series as any change in patch 2 would not be a largen enhancement of the already existing code. </div><br></div><div><div class="gmail_default" style="font-family:monospace,monospace"><span style="font-family:arial,sans-serif"> </span><br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-HOEnZb"><font color="#888888"> Selva<br> </font></span><div class="gmail-HOEnZb"><div class="gmail-h5"><br></div></div></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">Best regards, </div><div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">-- Emmanuel Deloget</div></div><div class="gmail_default" style="font-family:monospace,monospace"></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 86318d4c..7061f536 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) EVP_PKEY *pkey = X509_get_pubkey(cert); if (pkey != NULL) { - if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) && (EVP_PKEY_get0_RSA(pkey) != NULL)) - { - RSA *rsa = EVP_PKEY_get0_RSA(pkey); - openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", - RSA_bits(rsa)); - } - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) && (EVP_PKEY_get0_DSA(pkey) != NULL)) - { - DSA *dsa = EVP_PKEY_get0_DSA(pkey); - openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA", - DSA_bits(dsa)); - } + RSA *rsa = EVP_PKEY_get0_RSA(pkey); + DSA *dsa = EVP_PKEY_get0_DSA(pkey); #ifndef OPENSSL_NO_EC - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL)) + EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); + + if (ec != NULL) { - EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); const EC_GROUP *group = EC_KEY_get0_group(ec); const char* curve; @@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve: %s", EC_GROUP_order_bits(group), curve); - - } + } else #endif + if (rsa != NULL) + { + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", + RSA_bits(rsa)); + } + else if (dsa != NULL) + { + openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA", + DSA_bits(dsa)); + } + EVP_PKEY_free(pkey); } X509_free(cert);
Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as the same check is also performed in the later. We also make the code a bit better by not calling the various EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to avoid introducing yet another #ifndef OPENSSL_NO_EC in the code). Signed-off-by: Emmanuel Deloget <logout@free.fr> --- src/openvpn/ssl_openssl.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)