Evp pkey nokex #453
Evp pkey nokex #453
Conversation
A truncated part of openssh#445
| RSA_get0_key(k->rsa, &n, NULL, NULL); | ||
| ASSERT_PTR_NE(k->pkey, NULL); | ||
| #if (OPENSSL_VERSION_NUMBER >= 0x30000000L) | ||
| EVP_PKEY_get_bn_param(k->pkey, OSSL_PKEY_PARAM_RSA_N, &n); |
There was a problem hiding this comment.
Does it make sense to have one routine, ssh_gen_bn_from_rsa and then move the several clusters of these ifdef/else/endif lines into one place?
There was a problem hiding this comment.
I am not sure I understand how it can be done. Both named params in new API and key params in old RSA API are not pretty convenient to deal with from an universal function
beldmit
force-pushed
the
evp_pkey_nokex
branch
from
d2df572
to
a2960c6
Compare
| #if (OPENSSL_VERSION_NUMBER >= 0x30000000L) | ||
| if (EVP_PKEY_eq(a->pkey, b->pkey) == 1) | ||
| return 1; | ||
| #else | ||
| if (EVP_PKEY_cmp(a->pkey, b->pkey) == 1) | ||
| return 1; | ||
| #endif |
There was a problem hiding this comment.
given these two have the same parameters and everything, I would prefer to introduce the compat define, rather than this ifdef at every place it is used. I think there used to be some ossl-compat.h, which could have something like this
#if (OPENSSL_VERSION_NUMBER < 0x30000000L)
#define EVP_PKEY_eq EVP_PKEY_cmp
#endif
and then you will just be able to use the new API everywhere.
In the end, the above code can be simplified to just return EVP_PKEY_cmp(a->pkey, b->pkey).
There was a problem hiding this comment.
I'm not sure I like the idea exactly as you propose it - it can make debugging harder. I also not sure about the final simplification because there can be negative values in case of serious errors and the calling code is expecting 1/0
There was a problem hiding this comment.
So this is a general comment about this entire change. I made it in 455 and this is convenienty a different PR so the comment is now lost, and I'll state it again here. The EVP_PKEY interface in 3.0 is not that different from the 1.1.1 level of API supported in libressl. This is an example of a needless change because 3.0 STILL HAS EVP_PKEY_cmp and can do this just as it did before.
The result here is a mess of #ifdef soup promoting the new API instead of the old one, merely because 3.0 is claiming to mark some funcitons as deprecated and the new api is preferred for new code. this is fine, and we could even have discussions later about ensure some of these API's were universally supported, but that will not happen if this goes in as #ifdef soup trying to make everything the newest 3.0 way..
As I mentioned, I'd love to see this in a form where we simply have this coverted to the 1.1.1 EVP_PKEY api first. and at that point, look at what the minimal differences are to get this to compile on 3.0, which i expect are absolutely not this.
There was a problem hiding this comment.
I don't see any problem with removing #ifdef in this particular function (and the same for EC). In general I'd say we need some compatibility layer.
| #if (OPENSSL_VERSION_NUMBER >= 0x30000000L) | ||
| if (EVP_PKEY_eq(a->pkey, b->pkey) == 1) | ||
| return 1; | ||
| #else | ||
| if (EVP_PKEY_cmp(a->pkey, b->pkey) == 1) | ||
| return 1; | ||
| #endif | ||
| return 0; |
There was a problem hiding this comment.
same here as for the ecdsa keys -- I would say that a compat define and simplification would help here.
beldmit
force-pushed
the
evp_pkey_nokex
branch
from
aa18851
to
fee1cd0
Compare
beldmit
force-pushed
the
evp_pkey_nokex
branch
from
1c1e72c
to
5b36041
Compare
| MKBN(ec256_x, bn_x); | ||
| MKBN(ec256_y, bn_y); | ||
| #if (OPENSSL_VERSION_NUMBER >= 0x30000000L) |
There was a problem hiding this comment.
So (this comment applies to all of the bag of ifdefs like this)
This define should not be used in this code like this. It precludes libressl from ever providing a compatible API
since both packages define OPENSSL_VERSION_NUMBER - Unless when providing 3.0 API you would prefer that we again jump the shark and sit on 0x40000000L or greater to get around this sort of ifdef mathemagics.
again, I reiterate this is doing too much at once and should just concentrate on the compatible to 1.1.1. EVP_PKEY changes, but if a define for 3.0 is to be used, please make it a define for OPENSSL 3.0 not numeric magic like this.
Reduced version of #445
RSA/EC in sshkey are moved to EVP API to some extent, key exchange is unttouched