Closed raghuncstate closed 3 years ago
First of all, thanks a lot for the effort!
Of course there are some errors during build on travis. The easy one being tpm2-tss-engine-pmeth.h
to be added to the _SOURCES so distcheck does not fail anymore, but also some compiletime issues regarding const discards and things.
On a content side, could you help me with understanding the PR ? I see that only RSA is implemented right now, correct ? ECDSA would of course be a great addition for merging. Could you give me a pointer to the EVP_PKEY / EVP_PKEY_METHODS blog post or other description ?
NP! Thanks for the review. I'll fix the CI issues and push the changes. The const issue is because of openssl versions(I tried on openssl 1.1+). Will need ifdef's. You are correct that only RSA methods are implemented. EC/ECDSA PKEY methods, a test app, and adding control commands are next on the list :) (and hopefully followed by redirecting digests and symmetric key operations using EVP_CIPHER and EVP_MD!). I have not implemented them yet since i did not want to go too far down if this had something fundamentally wrong. Link to EVP_PKEY_METHOD description: https://www.openssl.org/docs/man1.0.2/man3/EVP_PKEY_METHOD.html.
I apologize if you already know what is written below but i'll try to answer briefly your question about understanding the PR. The general idea is to be able to intercept all crypto operations from openssl apps that use EVP interface and redirect to the TPM. This is similar to how the openssl command line tool redirects to the TPM using the engine, except that it will be great to have the ability to do this on ANY application that uses the openssl EVP* interface. A lot of code written in the OpenSSL source and many openssl applications tend to use the EVP* interface since EVP interface is agnostic to the type of keys(RSA, EC etc) and provides the same interface for all keys, digests, symmetric crypto etc. so the application code is usually unaware of concrete types(such as RSA or EC or SHA256), but is only concerned with crypto 'operations'. The types of keys, digests etc usually come from different config files or command line parameters and the same code will work with all types of keys. This change potentially allows many existing applications to redirect key generation and crypto operations to the TPM(on a provisioned TPM for now) by simply using the engine. Let me know if i can provide more clarity if it did not answer your question.
Merging #89 into master will decrease coverage by
10.15%
. The diff coverage is31.43%
.
@@ Coverage Diff @@
## master #89 +/- ##
===========================================
- Coverage 66.15% 55.99% -10.16%
===========================================
Files 9 11 +2
Lines 1099 1468 +369
===========================================
+ Hits 727 822 +95
- Misses 372 646 +274
Impacted Files | Coverage Δ | |
---|---|---|
src/tpm2-tss-engine-rsa-pmeth.c | 25.85% <25.85%> (ø) |
|
src/tpm2-tss-engine.c | 55.64% <50%> (+0.08%) |
:arrow_up: |
src/tpm2-tss-engine-pmeth.c | 75% <75%> (ø) |
|
src/tpm2-tss-engine-tcti.c | 13.22% <0%> (-39.67%) |
:arrow_down: |
src/tpm2-tss-engine-rsa.c | 67.43% <0%> (-3.02%) |
:arrow_down: |
src/tpm2-tss-engine-ecc.c | 62.38% <0%> (+0.38%) |
:arrow_up: |
src/tpm2-tss-engine-common.c | 79.91% <0%> (+0.42%) |
:arrow_up: |
... and 1 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 07e8eeb...0a6b45c. Read the comment docs.
i see PR#77 addresses symmetric encryption :)
Sounds very very nice. Please keep it up. I'll take this as prime feature for the 1.1 release around summer.
Thanks. Can you provide some directions on how i can fix the codecov issues ?
Codecov is basically complaining that code coverage in the unit and integration tests decreased by 6.3% and that the code you contributed is not visited by the tests. You can go here to see visited code: https://codecov.io/gh/tpm2-software/tpm2-tss-engine/commit/264474c3185fca771398ee5de7edddfc0143088a
e.g. the functions tpm2tss_pkey_rsa_copy(), tpm2tss_pkey_rsa_verifyrecover(), tpm2tss_pkey_rsa_ctrl(), tpm2tss_pkey_rsa_ctrl_str(), tpm2tss_pkey_rsa_keygen(), tpm2tss_pkey_asn1_priv_decode() and tpm2tss_pkey_asn1_priv_encode() are not used anywhere.
So if you create (preferably) integration tests that use these functions, then the codecov errors will go away. The higher the better of course... ;-)
aah. understood. Is it sufficient if i create scripts in the test directory ? it appears that the two scripts i created were run, otherwise we would have 0 code coverage on the new code. Will it work if i create a test application that exercises other code paths and put the test application binary in the test directory ?
Yes, integration-tests running on a tpm simulator (aka scripts) are preferred.
If there are special areas you cannot reach though (usually error conditions), you can go ahead and do unit tests. e.g.: https://github.com/tpm2-software/tpm2-tss-engine/blob/master/Makefile.am#L130 https://github.com/tpm2-software/tpm2-tss-engine/blob/master/test/error_tpm2-tss-engine-common.c
But putting scripts in there and adding them here is better: https://github.com/tpm2-software/tpm2-tss-engine/blob/master/Makefile.am#L116
Since 1.0.0 is out now, I want to start merging the backloged PRs. How are things going regarding test coverage ? Do you need help ? I'd like to have at least all functions visited in the test-suit to rule out any major errors.
Sure. Working on it now. Give me a few more days. :) When is your dead line to merge the backlogged PR's ?
Feature-freeze is June, but I'd like to extend it to ECC keys and implement some more key conversion functions on top, so this month would be nice.
sounds good. Few more days then :)
How is it going ?
starting now. how can i build the unit test case error_tpm2-tss-engine-common.c ? also do i have to use CMOCKA or can it be a regular linux application?
No need to use CMOCKA, I just found it to be useful. Regarding the first question, I'm not quite sure I understand it ?
@raghuncstate any updates ?
Something is not connected correctly here which seems to break EVP_Encrypt with RSA_PKCS1_OAEP_PADDING.
Here's a log of what happens when debug is enabled in tpm2-tss-engine.
Loaded key uses alg-id 1
Creating RSA key object.
Created RSA key object.
TPM2 Key loaded
rsa_priv_dec called for scheme 3 and input data(size=256):
3069281008:error:8008D06B:tpm2-tss-engine:rsa_priv_dec:Unknown padding scheme requested:src/tpm2-tss-engine-rsa.c:244:
3069281008:error:8007406F:tpm2-tss-engine:esys_auxctx_free:Some unknown error occured:src/tpm2-tss-engine-common.c:108:
openssl version: OpenSSL 1.1.1b 26 Feb 2019
It looks like the original pkey_rsa_decrypt function in crypto/rsa/rsa_pmeth.c is getting called instead of the engine's tpm2tss_pkey_rsa_decrypt function. This overrides the padding method to RSA_NO_PADDING.
Any ideas on what function isn't getting registered correctly?
Allowing the rsa_priv_dec function in tpm2-tss-engine-rsa.c to handle RSA_NO_PADDING with this patch seems to pass my tests, but i'm not very sure that this is the "correct" way to solve this problem.
index afa0085..a9fc28f 100644
--- a/src/tpm2-tss-engine-rsa.c
+++ b/src/tpm2-tss-engine-rsa.c
@@ -236,6 +236,9 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA * rsa,
case RSA_PKCS1_PADDING:
inScheme.scheme = TPM2_ALG_RSAES;
break;
+ case RSA_NO_PADDING:
+ inScheme.scheme = TPM2_ALG_NULL;
+ break;
case RSA_PKCS1_OAEP_PADDING:
inScheme.scheme = TPM2_ALG_OAEP;
inScheme.details.oaep.hashAlg = TPM2_ALG_SHA1;```
@paulbartell Thanks for the input. I hope whoever picks up this work finds it useful. If it shall be me, it'll take a few more months. If you're willing, I think this PR is abandoned and could use some love.
I played around with this and at least for me, the new tests just created plain software private keys but no TSS2 PRIVATE KEY files and I see no TPM action.
So I guess this just does not work ?
Something is not connected correctly here which seems to break EVP_Encrypt with RSA_PKCS1_OAEP_PADDING.
Here's a log of what happens when debug is enabled in tpm2-tss-engine.
Loaded key uses alg-id 1 Creating RSA key object. Created RSA key object. TPM2 Key loaded rsa_priv_dec called for scheme 3 and input data(size=256): 3069281008:error:8008D06B:tpm2-tss-engine:rsa_priv_dec:Unknown padding scheme requested:src/tpm2-tss-engine-rsa.c:244: 3069281008:error:8007406F:tpm2-tss-engine:esys_auxctx_free:Some unknown error occured:src/tpm2-tss-engine-common.c:108:
openssl version: OpenSSL 1.1.1b 26 Feb 2019
It looks like the original pkey_rsa_decrypt function in crypto/rsa/rsa_pmeth.c is getting called instead of the engine's tpm2tss_pkey_rsa_decrypt function. This overrides the padding method to RSA_NO_PADDING.
Any ideas on what function isn't getting registered correctly?
I get the same error with RSA_PKCS1_OAEP_PADDING . Is there a way to decrypt with RSA_PKCS1_OAEP_PADDING?
In my opinion, this does not work with the standard openssl genrsa
from any OpenSSL 1.x, which I thought was the intention. This patch is using the (newer, upper-level) EVP functions, while even in 1.1.1 is the genrsa based on the lower-level RSA functions. https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/apps/genrsa.c#L149
Something similar to this patch could work in 3.0.0 though as that implementation is based on the EVP functions: https://github.com/openssl/openssl/blob/master/apps/genrsa.c#L202
Seems very much stalled, so closing.
Add EVP_PKEY_METHODS to engine to allow for key generation through openssl genpkey utility. Implementing these methods also provides the ability to generate keys purely through the EVP_PKEY interface without requiring a special utility like tpm2tss-genkey. We can also add control commands in the EVP_PKEY_METHODS to have custom settings for passwords, parents for keys and other nuances related to generating TPM keys, all using the EVP_PKEY interface and vanialla openssl tools.