Improved detection of engine-provided private "classic" keys #19965
Conversation
Resolves openssl#17092 (?) Resolves openssl#17286 (?)
beldmit
added
branch: master
|
I wonder whether we have any other cases when we missed that the loaded key is legacy... |
t8m
added
tests: exempted
|
Please exclude #17286. It is not engine related! It is more generic. For instance code in https://gitlab.com/secsh/pkixssh/-/commits/master/ssh-pkcs11.c has following commits authored 7-8 mounts ago:
If I remember well OpenSMTPD issue fail into general issue. |
|
@petrovr sure. Let's deal with the engine case first. |
|
After review of code I'm not convinced that pull fixes #17092 - see #17092 (comment) . When I have spare time I will check it. May be after two weeks. In #17092 test scenario is: engine loaded and initialied first followed by call of d2i_PUBKEY where DER material is from random source. Result is internal structure with key manager and engine method. I guess that test case could be like this:
I have no time to test such case, sorry. |
t8m
removed
the
approval: otc review pending
|
None of the |
|
Once again, please exclude #17286. |
|
This pull does not change anything. With fprintf(stderr, "TRACE[OpenSSL].%s() [[ //before load_privkey\n", __func__);
pkey = e->load_privkey(e, key_id, ui_method, callback_data);
fprintf(stderr, "TRACE[OpenSSL].%s() ]] //after load_privkey\n", __func__);Result is: Remark : work-around in e_nss code now is modified to log if engine method is assigned to rds/dsa keys. At this point engine method and provider key manager are assigned to pkey structure. |
Done |
|
Without patch: After the patch: As you can see, the results are completely different. I kindly ask you to check again, because the error present in master is eliminated after my patch |
|
(1) Correction "post factum". (2) Correction "a priori". Let review RSA key constructors. This one with explicit engine parameter is out of interest. So the another solution is to stop to set engine key methods as default. (3) Real correction. Let assume properly created "foreign" pkey. When engine load key is called code may return pkey with key manager provided by engine. Why not? |
My approach seems to fix the issue when we lose the foreigness of the key. I think we don't lose anything (but avoid the current hack to preserve the foreigness in applications) |
Do I correctly understand that, when this approach is taken, you lose some external data that is crucial for dealing with engine-provided keys? If this data is not crucial, we are able to operate them using provider API. |
My fix is intended to fix the foreign flag. And I see it does it :) |
Properly created "foreign" key, AFAICS, may or may not have the data necessary for default provider - but should be still loaded and usable (which is not correct without my patch) or fail to load. |
Sorry, I don't get your point |
|
Dear @petrovr |
E_NSS engine provides 'classical' and 'store-based' load of keys. Both models call one and the same function() to constuct PKEY. I cannot see any reason to change this model. |
|
Thanks, got it! Which one is tested by the command line from here #19965 (comment) and is there a way to test the 2nd one via openssl command line or there is a special code to use? |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 154 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 185 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 216 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 247 days ago |
|
Ping @openssl/committers for the 2nd review |
| case EVP_PKEY_RSA: | ||
| { | ||
| RSA *rsa = EVP_PKEY_get1_RSA(pkey); | ||
| EVP_PKEY_set1_RSA(pkey, rsa); |
There was a problem hiding this comment.
Blank lines between declarations and code are missing.
paulidale
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
Merged, thanks. |
Resolves #17092 (?) Resolves #17286 (?) Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl/openssl#19965) (cherry picked from commit 2b74e75) Signed-off-by: fly2x <fly2x@hitls.org>
Resolves #17092 (?) Resolves #17286 (?) Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl/openssl#19965) Signed-off-by: fly2x <fly2x@hitls.org>
Resolves #17092 (?) Resolves #17286 (?) Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl/openssl#19965) (cherry picked from commit 2b74e75) Signed-off-by: fly2x <fly2x@hitls.org>
|
This change appears to have changed the pkcs11 mechanism used for RSA keys in the pkcs11_engine from libp11. openssl is now requesting RSA_NO_PADDING, which is converted to CKM_RSA_X_509 on the engine side. In particular, I have observed this using wpa_supplicant 802.1x authentication, which is using openssl/libp11 engine. This is undesirable as keys are not always accessible to be used without padding. Is this WAI, and I should look to libp11 for a fix to not indicate a "classic" key? |
Preserve pkey->pmeth_engine so that custom methods set on this key by an engine are preserved (for example in the libp11 pkcs11 engine). This fixes an issue was introduced with openssl#19965
Preserve pkey->pmeth_engine so that custom methods set on this key by an engine are preserved (for example in the libp11 pkcs11 engine). This fixes an issue was introduced with openssl#19965
Preserve pkey->pmeth_engine so that custom methods set on this key by an engine are preserved (for example in the libp11 pkcs11 engine). This fixes an issue was introduced with openssl#19965
Resolves #17092 (?)
Checklist