| Message ID | 6a6ede658da2e35b718a9ccb733b132a955ffefe.1515792908.git.logout@free.fr |
|---|---|
| State | Rejected |
| Headers | show |
| Series | None | expand |
Hi, On 12-01-18 22:37, 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). Checking double is a bit silly, but we can fix that by simply removing the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to remove it from the compat wrapper.) I'm not sure that moving the variables outside the if() scope actually improves the code. At least I think the original flow is easier to read. Mostly due to the #ifdef noise, but still. In a path that's not performance-critical, I prefer readability over performance. > Signed-off-by: Emmanuel Deloget <logout@free.fr> > > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index 711bba11..7943fb2c 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 = NULL; > + DSA *dsa = NULL; > #ifndef OPENSSL_NO_EC > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL)) > + EC_KEY *ec = NULL; > + > + if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != 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 = EVP_PKEY_get0_RSA(pkey)) != NULL) > + { > + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", > + RSA_bits(rsa)); > + } > + else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != NULL) > + { > + openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA", > + DSA_bits(dsa)); > + } > + > EVP_PKEY_free(pkey); > } > X509_free(cert); > -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hello Steffan, On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <steffan@karger.me> wrote: > Hi, > > On 12-01-18 22:37, 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). > > Checking double is a bit silly, but we can fix that by simply removing > the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to > remove it from the compat wrapper.) > > I'm not sure that moving the variables outside the if() scope actually > improves the code. At least I think the original flow is easier to > read. Mostly due to the #ifdef noise, but still. In a path that's not > performance-critical, I prefer readability over performance. > Well, to be honest, I was definitely not trying to make any kind of performance gain :) The whole idea was to make code easier to read and to remove some now weird duplication of functions (the kind of duplication that will come and bites you later). For the variables outside the ifs, the next C standard should allow us to write something like: if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... That should be easier to read (but then, I'm not sure when this will be considered as widespread ; I guess we're slated for 2025 or so :)) Anyway, moving the variables within the if means we're going to call EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But even if I'm not very comfortable with this, I can still propose a patch that des it (it's lighter than the currently proposed change). You tell me :) > > > Signed-off-by: Emmanuel Deloget <logout@free.fr> > > > > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > > index 711bba11..7943fb2c 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 = NULL; > > + DSA *dsa = NULL; > > #ifndef OPENSSL_NO_EC > > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && > (EVP_PKEY_get0_EC_KEY(pkey) != NULL)) > > + EC_KEY *ec = NULL; > > + > > + if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != 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 = EVP_PKEY_get0_RSA(pkey)) != NULL) > > + { > > + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", > > + RSA_bits(rsa)); > > + } > > + else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != NULL) > > + { > > + openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA", > > + DSA_bits(dsa)); > > + } > > + > > EVP_PKEY_free(pkey); > > } > > X509_free(cert); > > > > -Steffan > Best regards, -- Emmanuel Deloget <div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hello Steffan, </div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <span dir="ltr"><<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</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 12-01-18 22:37, 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> </span>Checking double is a bit silly, but we can fix that by simply removing<br> the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to<br> remove it from the compat wrapper.)<br> <br> I'm not sure that moving the variables outside the if() scope actually<br> improves the code. At least I think the original flow is easier to<br> read. Mostly due to the #ifdef noise, but still. In a path that's not<br> performance-critical, I prefer readability over performance.<br></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">Well, to be honest, I was definitely not trying to make any kind of performance gain :)</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">The whole idea was to make code easier to read and to remove some now weird duplication of functions (the kind of duplication that will come and bites you later). </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">For the variables outside the ifs, the next C standard should allow us to write something like:</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {</div><div class="gmail_default" style="font-family:monospace,monospace">...</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">That should be easier to read (but then, I'm not sure when this will be considered as widespread ; I guess we're slated for 2025 or so :))</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">Anyway, moving the variables within the if means we're going to call EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But even if I'm not very comfortable with this, I can still propose a patch that des it (it's lighter than the currently proposed change). You tell me :)</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"> <div><div class="gmail-h5"><br> > Signed-off-by: Emmanuel Deloget <<a href="mailto:logout@free.fr">logout@free.fr</a>><br> ><br> > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c<br> > index 711bba11..7943fb2c 100644<br> > --- a/src/openvpn/ssl_openssl.c<br> > +++ b/src/openvpn/ssl_openssl.c<br> > @@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)<br> > EVP_PKEY *pkey = X509_get_pubkey(cert);<br> > if (pkey != NULL)<br> > {<br> > - if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) && (EVP_PKEY_get0_RSA(pkey) != NULL))<br> > - {<br> > - RSA *rsa = EVP_PKEY_get0_RSA(pkey);<br> > - openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",<br> > - RSA_bits(rsa));<br> > - }<br> > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) && (EVP_PKEY_get0_DSA(pkey) != NULL))<br> > - {<br> > - DSA *dsa = EVP_PKEY_get0_DSA(pkey);<br> > - openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",<br> > - DSA_bits(dsa));<br> > - }<br> > + RSA *rsa = NULL;<br> > + DSA *dsa = NULL;<br> > #ifndef OPENSSL_NO_EC<br> > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL))<br> > + EC_KEY *ec = NULL;<br> > +<br> > + if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != NULL)<br> > {<br> > - EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey);<br> > const EC_GROUP *group = EC_KEY_get0_group(ec);<br> > const char* curve;<br> ><br> > @@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)<br> ><br> > openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve: %s",<br> > EC_GROUP_order_bits(group), curve);<br> > -<br> > - }<br> > + } else<br> > #endif<br> > + if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL)<br> > + {<br> > + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",<br> > + RSA_bits(rsa));<br> > + }<br> > + else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != NULL)<br> > + {<br> > + openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",<br> > + DSA_bits(dsa));<br> > + }<br> > +<br> > EVP_PKEY_free(pkey);<br> > }<br> > X509_free(cert);<br> ><br> <br> </div></div>-Steffan<br></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">Best regards, </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></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Mon, Jan 15, 2018 at 5:33 PM, Emmanuel Deloget <logout@free.fr> wrote: > Hello Steffan, > > On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <steffan@karger.me> > wrote: > >> Hi, >> >> On 12-01-18 22:37, 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). >> >> Checking double is a bit silly, but we can fix that by simply removing >> the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to >> remove it from the compat wrapper.) >> >> I'm not sure that moving the variables outside the if() scope actually >> improves the code. At least I think the original flow is easier to >> read. Mostly due to the #ifdef noise, but still. In a path that's not >> performance-critical, I prefer readability over performance. >> > > > Well, to be honest, I was definitely not trying to make any kind of > performance gain :) > > The whole idea was to make code easier to read and to remove some now > weird duplication of functions (the kind of duplication that will come and > bites you later). > > For the variables outside the ifs, the next C standard should allow us to > write something like: > > if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { > ... > > That should be easier to read (but then, I'm not sure when this will be > considered as widespread ; I guess we're slated for 2025 or so :)) > > Anyway, moving the variables within the if means we're going to call > EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But > even if I'm not very comfortable with this, I can still propose a patch > that des it (it's lighter than the currently proposed change). You tell me > :) > Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I was targeting only 1.1.0+ that's how most of those checks would have been done, isn't? Makes life easier, code simpler as I have said before.. Just throwing in my 0.02c, feel free to ignore. Best, Selva <div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 15, 2018 at 5:33 PM, Emmanuel Deloget <span dir="ltr"><<a href="mailto:logout@free.fr" target="_blank">logout@free.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div style="font-family:monospace,monospace">Hello Steffan, </div><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <span dir="ltr"><<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</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="m_1484023647284146023gmail-"><br> On 12-01-18 22:37, 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> </span>Checking double is a bit silly, but we can fix that by simply removing<br> the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to<br> remove it from the compat wrapper.)<br> <br> I'm not sure that moving the variables outside the if() scope actually<br> improves the code. At least I think the original flow is easier to<br> read. Mostly due to the #ifdef noise, but still. In a path that's not<br> performance-critical, I prefer readability over performance.<br></blockquote><div><br></div></span><div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">Well, to be honest, I was definitely not trying to make any kind of performance gain :)</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">The whole idea was to make code easier to read and to remove some now weird duplication of functions (the kind of duplication that will come and bites you later). </div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">For the variables outside the ifs, the next C standard should allow us to write something like:</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {</div><div style="font-family:monospace,monospace">...</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">That should be easier to read (but then, I'm not sure when this will be considered as widespread ; I guess we're slated for 2025 or so :))</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">Anyway, moving the variables within the if means we're going to call EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But even if I'm not very comfortable with this, I can still propose a patch that des it (it's lighter than the currently proposed change). You tell me :)</div></div></div></div></div></blockquote><div><br></div><div>Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I was targeting only 1.1.0+ that's how most of those checks would have been done, isn't? Makes life easier, code simpler as I have said before..</div><div><br></div><div>Just throwing in my 0.02c, feel free to ignore.</div><div><br></div><div>Best,</div><div><br></div><div>Selva </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
Hello Selva, On Tue, Jan 16, 2018 at 12:10 AM, Selva Nair <selva.nair@gmail.com> wrote: > > > On Mon, Jan 15, 2018 at 5:33 PM, Emmanuel Deloget <logout@free.fr> wrote: > >> Hello Steffan, >> >> On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <steffan@karger.me> >> wrote: >> >>> Hi, >>> >>> On 12-01-18 22:37, 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). >>> >>> Checking double is a bit silly, but we can fix that by simply removing >>> the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to >>> remove it from the compat wrapper.) >>> >>> I'm not sure that moving the variables outside the if() scope actually >>> improves the code. At least I think the original flow is easier to >>> read. Mostly due to the #ifdef noise, but still. In a path that's not >>> performance-critical, I prefer readability over performance. >>> >> >> >> Well, to be honest, I was definitely not trying to make any kind of >> performance gain :) >> >> The whole idea was to make code easier to read and to remove some now >> weird duplication of functions (the kind of duplication that will come and >> bites you later). >> >> For the variables outside the ifs, the next C standard should allow us to >> write something like: >> >> if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { >> ... >> >> That should be easier to read (but then, I'm not sure when this will be >> considered as widespread ; I guess we're slated for 2025 or so :)) >> >> Anyway, moving the variables within the if means we're going to call >> EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But >> even if I'm not very comfortable with this, I can still propose a patch >> that des it (it's lighter than the currently proposed change). You tell me >> :) >> > > Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I > was targeting only 1.1.0+ that's how most of those checks would have been > done, isn't? Makes life easier, code simpler as I have said before.. > > Just throwing in my 0.02c, feel free to ignore. > > Best, > > Selva > OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY without any key inside. For example, you can do EVP_PKEY *pkey = EVP_PKEY_new(); EVP_PKEY_set_type(pkey, EVP_PKEY_RSA); This is plainly valid: the key is prepared for later use. So testing for EVP_PKEY_id() is not sufficient to make sure that current and future code won't mess with us. To be extra sure, one can do if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... } Which is exactly what the current code does. Yet the idea (at least mine) is to remove that to simplify code a bit. Now, I agree, this is quite easy to read and to understand. We have 3 solutions: 1) keep the code as is if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) { ... } 2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA() if (EVP_PKEY_get0_RSA(pkey) != NULL) { RSA *rsa = EVP_PKEY_get0_RSA(pkey); ... } 3) rework it as proposed in the patch RSA *rsa = NULL; if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... } While I prefer the latest, I can understand that it might not be the best solution. I don't have a strong opinion on the subject (that's why it's a separate patch ;)). Best regards, -- Emmanuel Deloget <div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hello Selva, </div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 16, 2018 at 12:10 AM, 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"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="gmail-">On Mon, Jan 15, 2018 at 5:33 PM, Emmanuel Deloget <span dir="ltr"><<a href="mailto:logout@free.fr" target="_blank">logout@free.fr</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"><div dir="ltr"><div style="font-family:monospace,monospace">Hello Steffan, </div><div class="gmail_extra"><br><div class="gmail_quote"><span>On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <span dir="ltr"><<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</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-m_5510807250704300764m_1484023647284146023gmail-"><br> On 12-01-18 22:37, 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> </span>Checking double is a bit silly, but we can fix that by simply removing<br> the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to<br> remove it from the compat wrapper.)<br> <br> I'm not sure that moving the variables outside the if() scope actually<br> improves the code. At least I think the original flow is easier to<br> read. Mostly due to the #ifdef noise, but still. In a path that's not<br> performance-critical, I prefer readability over performance.<br></blockquote><div><br></div></span><div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">Well, to be honest, I was definitely not trying to make any kind of performance gain :)</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">The whole idea was to make code easier to read and to remove some now weird duplication of functions (the kind of duplication that will come and bites you later). </div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">For the variables outside the ifs, the next C standard should allow us to write something like:</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {</div><div style="font-family:monospace,monospace">...</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">That should be easier to read (but then, I'm not sure when this will be considered as widespread ; I guess we're slated for 2025 or so :))</div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">Anyway, moving the variables within the if means we're going to call EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But even if I'm not very comfortable with this, I can still propose a patch that des it (it's lighter than the currently proposed change). You tell me :)</div></div></div></div></div></blockquote><div><br></div></span><div>Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I was targeting only 1.1.0+ that's how most of those checks would have been done, isn't? Makes life easier, code simpler as I have said before..</div><div><br></div><div>Just throwing in my 0.02c, feel free to ignore.</div><div><br></div><div>Best,</div><div><br></div><div>Selva </div></div></div></div></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">OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY without any key inside. For example, you can do<br></div></div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace"> EVP_PKEY *pkey = EVP_PKEY_new();</div><div class="gmail_default" style="font-family:monospace,monospace"> EVP_PKEY_set_type(pkey, <span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">EVP_PKEY_RSA);</span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">This is plainly valid: the key is prepared for later use. </span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">So testing for EVP_PKEY_id() is not sufficient to make sure that current and future code won't mess with us. To be extra sure, one can do </span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... }</span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">Which is exactly what the current code does. Yet the idea (at least mine) is to remove that to simplify code a bit. </span></div><div class="gmail_default" style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre">Now, I agree, this is quite easy to read and to understand. </span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre">We have 3 solutions: </span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre">1) keep the code as is</span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) { ... }</span><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre">2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA()</span></font></div><div class="gmail_default"><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> if (EVP_PKEY_get0_RSA(pkey) != NULL) { </span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> RSA *rsa = </span><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">EVP_PKEY_get0_RSA(pkey);</span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> ...</span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> }</span><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre"><br></span></font></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">3) rework it as proposed in the patch</span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> RSA *rsa = NULL;</span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"> if ((rsa = </span><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }</span><br></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre"><br></span></div><div class="gmail_default"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre">While I prefer the latest, I can understand that it might not be the best solution. I don't have a strong opinion on the subject (that's why it's a separate patch ;)).</span></div></div><br></div><div class="gmail_extra"><div class="gmail_default" style="font-family:monospace,monospace">Best regards, </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">-- Emmanuel Deloget</div><br></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Mon, Jan 15, 2018 at 7:56 PM, Emmanuel Deloget <logout@free.fr> wrote: > Hello Selva, > > On Tue, Jan 16, 2018 at 12:10 AM, Selva Nair <selva.nair@gmail.com> wrote: > >> >> >> Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I >> was targeting only 1.1.0+ that's how most of those checks would have been >> done, isn't? Makes life easier, code simpler as I have said before.. >> >> Just throwing in my 0.02c, feel free to ignore. >> >> Best, >> >> Selva >> > > > OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY > without any key inside. For example, you can do > > EVP_PKEY *pkey = EVP_PKEY_new(); > EVP_PKEY_set_type(pkey, EVP_PKEY_RSA); > > This is plainly valid: the key is prepared for later use. > > So testing for EVP_PKEY_id() is not sufficient to make sure that current > and future code won't mess with us. > That was not my intent. Use EVP_KEY_id() to determine the type and choose the code path. Then when ready, get the key pointer and check it. Of course there may be situations where its possible to get the pointer and check it in one line as in your case (3) below. Anyway, calling EVP_PKEY_get0_RSA() multiple times is lame. Now think of cases where we may have to delegate the job to separate functions depending on the key type. To me it looks natural to do that using a switch(EVP_PKEY_id()) or if-else blocks to decide which function to call and then get the key and check it inside the function. Anyway, I will leave it at that. > To be extra sure, one can do > > if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... } > > Which is exactly what the current code does. Yet the idea (at least mine) > is to remove that to simplify code a bit. > > Now, I agree, this is quite easy to read and to understand. > > We have 3 solutions: > > 1) keep the code as is > > if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) { > ... } > In this case the call to EVP_PKEY_id() is redundant, isn't it? Or am I missing something? If the latter returns non-null then its indeed an RSA key (I'm assuming the 1.1.0 or the corrected compat layer behaviour here). > > > 2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA() > > if (EVP_PKEY_get0_RSA(pkey) != NULL) { > RSA *rsa = EVP_PKEY_get0_RSA(pkey); > ... > } > This looks bad to me. > > > 3) rework it as proposed in the patch > > RSA *rsa = NULL; > if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... } > In this particular case this may be the best approach but there are situations where it may not be the natural choice. Hence the "if it were up to me EVP_KEY_id() will stay" musing...:) Thanks, Selva <div dir="ltr">Hi,<div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 15, 2018 at 7:56 PM, Emmanuel Deloget <span dir="ltr"><<a href="mailto:logout@free.fr" target="_blank">logout@free.fr</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"><div dir="ltr"><div style="font-family:monospace,monospace">Hello Selva, </div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="gmail-h5">On Tue, Jan 16, 2018 at 12:10 AM, 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"><div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-m_-9197147997010814971gmail-"><div><br></div></span><div>Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I was targeting only 1.1.0+ that's how most of those checks would have been done, isn't? Makes life easier, code simpler as I have said before..</div><div><br></div><div>Just throwing in my 0.02c, feel free to ignore.</div><div><br></div><div>Best,</div><div><br></div><div>Selva </div></div></div></div></blockquote><div><br></div></div></div><div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace">OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY without any key inside. For example, you can do<br></div></div><div style="font-family:monospace,monospace"><br></div><div style="font-family:monospace,monospace"> EVP_PKEY *pkey = EVP_PKEY_new();</div><div style="font-family:monospace,monospace"> EVP_PKEY_set_type(pkey, <span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">EVP_<wbr>PKEY_RSA);</span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">This is plainly valid: the key is prepared for later use. </span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">So testing for EVP_PKEY_id() is not sufficient to make sure that current and future code won't mess with us. </span></div></div></div></div></blockquote><div><br></div><div>That was not my intent.</div><div><br></div><div>Use EVP_KEY_id() to determine the type and choose the code path. Then when ready, get the key pointer and check it. Of course there may be situations where its possible to get the pointer and check it in one line as in your case (3) below. </div><div><br></div><div>Anyway, calling EVP_PKEY_get0_RSA() multiple times is lame. Now think of cases where we may have to delegate the job to separate functions depending on the key type. To me it looks natural to do that using a switch(EVP_PKEY_id()) or if-else blocks to decide which function to call and then get the key and check it inside the function. Anyway, I will leave it at that.</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">To be extra sure, one can do </span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... }</span><span style="font-family:arial,sans-serif"> </span></div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">Which is exactly what the current code does. Yet the idea (at least mine) is to remove that to simplify code a bit. </span></div><div style="font-family:monospace,monospace"><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap">Now, I agree, this is quite easy to read and to understand. </span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap">We have 3 solutions: </span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap">1) keep the code as is</span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) { ... }</span></div></div></div></div></blockquote><div><br></div><div>In this case the call to EVP_PKEY_id() is redundant, isn't it? Or am I missing something? If the latter returns non-null then its indeed an RSA key (I'm assuming the 1.1.0 or the corrected compat layer behaviour here).</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap">2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA()</span></font></div><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> if (EVP_PKEY_get0_RSA(pkey) != NULL) { </span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> RSA *rsa = </span><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">EVP_PKEY_get0_RSA(pkey);</span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> ...</span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> }</span></div></div></div></div></blockquote><div><br></div><div>This looks bad to me.</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><font color="#24292e" face="SFMono-Regular, Consolas, Liberation Mono, Menlo, Courier, monospace"><span style="font-size:12px;white-space:pre-wrap"><br></span></font></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">3) rework it as proposed in the patch</span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"><br></span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> RSA *rsa = NULL;</span></div><div><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap"> if ((rsa = </span><span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;white-space:pre-wrap">EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }</span></div></div></div></div></blockquote><div><br></div><div>In this particular case this may be the best approach but there are situations where it may not be the natural choice. Hence the "if it were up to me EVP_KEY_id() will stay" musing...:)</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></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
Hi, On Tue, Jan 16, 2018 at 01:56:11AM +0100, Emmanuel Deloget wrote: > 3) rework it as proposed in the patch > > RSA *rsa = NULL; > if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... } I do not particularily like assignments in if() clauses, especially when the variable is declared right above it. So if we go for (3), my favourite would be RSA *rsa = EVP_PKEY_get0_RSA(pkey); if (rsa != NULL) { ... } but this is just a side remark. The question on "1, 2 or 3" I'll defer to you folks that are maintaing this code (Steffan and Selva, mostly :) ). gert
Hi, On 15 January 2018 at 23:33, Emmanuel Deloget <logout@free.fr> wrote: > For the variables outside the ifs, the next C standard should allow us to > write something like: > > if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { Yeah, such a shame that this didn't make it into C11. Scoping a variable to an if block is often useful. If only for error checking like if(int err = foo()) { handle error }. Anyway, we can't have it :( On 16 January 2018 at 09:26, Gert Doering <gert@greenie.muc.de> wrote: > On Tue, Jan 16, 2018 at 01:56:11AM +0100, Emmanuel Deloget wrote: >> 3) rework it as proposed in the patch >> >> RSA *rsa = NULL; >> if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... } > > I do not particularily like assignments in if() clauses, especially when > the variable is declared right above it. So if we go for (3), my > favourite would be > > RSA *rsa = EVP_PKEY_get0_RSA(pkey); > if (rsa != NULL) { ... } > > > but this is just a side remark. The question on "1, 2 or 3" I'll defer > to you folks that are maintaing this code (Steffan and Selva, mostly :) ). This indeed looks a bit better that initializing to NULL if we're going to take the variables outside the scope. Seems I've been overruled by majority regarding the way to go, and with the suggestion from cron2 taken into account I think 3 is the way to go. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hello, On Wed, Jan 17, 2018 at 1:16 PM, Steffan Karger <steffan@karger.me> wrote: > Hi, > > On 15 January 2018 at 23:33, Emmanuel Deloget <logout@free.fr> wrote: > > For the variables outside the ifs, the next C standard should allow us to > > write something like: > > > > if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { > > Yeah, such a shame that this didn't make it into C11. Scoping a > variable to an if block is often useful. If only for error checking > like if(int err = foo()) { handle error }. Anyway, we can't have it > :( > > On 16 January 2018 at 09:26, Gert Doering <gert@greenie.muc.de> wrote: > > On Tue, Jan 16, 2018 at 01:56:11AM +0100, Emmanuel Deloget wrote: > >> 3) rework it as proposed in the patch > >> > >> RSA *rsa = NULL; > >> if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... } > > > > I do not particularily like assignments in if() clauses, especially when > > the variable is declared right above it. So if we go for (3), my > > favourite would be > > > > RSA *rsa = EVP_PKEY_get0_RSA(pkey); > > if (rsa != NULL) { ... } > > > > > > but this is just a side remark. The question on "1, 2 or 3" I'll defer > > to you folks that are maintaing this code (Steffan and Selva, mostly :) > ). > > This indeed looks a bit better that initializing to NULL if we're > going to take the variables outside the scope. > > Seems I've been overruled by majority regarding the way to go, and > with the suggestion from cron2 taken into account I think 3 is the way > to go. > > -Steffan > I'm OK with this (3) variation :) I'm respinning the patches. BR, -- Emmanuel Deloget <div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hello, </div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 17, 2018 at 1:16 PM, Steffan Karger <span dir="ltr"><<a href="mailto:steffan@karger.me" target="_blank">steffan@karger.me</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br> <span class=""><br> On 15 January 2018 at 23:33, Emmanuel Deloget <<a href="mailto:logout@free.fr">logout@free.fr</a>> wrote:<br> > For the variables outside the ifs, the next C standard should allow us to<br> > write something like:<br> ><br> > if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {<br> <br> </span>Yeah, such a shame that this didn't make it into C11. Scoping a<br> variable to an if block is often useful. If only for error checking<br> like if(int err = foo()) { handle error }. Anyway, we can't have it<br> :(<br> <span class=""><br> On 16 January 2018 at 09:26, Gert Doering <<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>> wrote:<br> > On Tue, Jan 16, 2018 at 01:56:11AM +0100, Emmanuel Deloget wrote:<br> >> 3) rework it as proposed in the patch<br> >><br> >> RSA *rsa = NULL;<br> >> if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }<br> ><br> > I do not particularily like assignments in if() clauses, especially when<br> > the variable is declared right above it. So if we go for (3), my<br> > favourite would be<br> ><br> > RSA *rsa = EVP_PKEY_get0_RSA(pkey);<br> > if (rsa != NULL) { ... }<br> ><br> ><br> > but this is just a side remark. The question on "1, 2 or 3" I'll defer<br> > to you folks that are maintaing this code (Steffan and Selva, mostly :) ).<br> <br> </span>This indeed looks a bit better that initializing to NULL if we're<br> going to take the variables outside the scope.<br> <br> Seems I've been overruled by majority regarding the way to go, and<br> with the suggestion from cron2 taken into account I think 3 is the way<br> to go.<br> <span class="HOEnZb"><font color="#888888"><br> -Steffan<br> </font></span></blockquote></div><br></div><div class="gmail_extra"><div class="gmail_default" style="font-family:monospace,monospace">I'm OK with this (3) variation :)</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">I'm respinning the patches. </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">BR, </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">-- Emmanuel Deloget</div><br></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 711bba11..7943fb2c 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 = NULL; + DSA *dsa = NULL; #ifndef OPENSSL_NO_EC - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) && (EVP_PKEY_get0_EC_KEY(pkey) != NULL)) + EC_KEY *ec = NULL; + + if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != 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 = EVP_PKEY_get0_RSA(pkey)) != NULL) + { + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA", + RSA_bits(rsa)); + } + else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != 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>