add rsa key import methods to handle pem and der encoding directly #252

jpbland1 · 2023-01-11T23:55:37Z

ZD 14996

dgarske · 2023-01-13T01:38:01Z

examples/keygen/keyimport.c

@@ -33,16 +33,47 @@

 #ifndef WOLFTPM2_NO_WRAPPER

+static const char* kRsaKeyPrivPem =


Please add a comment about where this RSA private key came from. Like ./certs/server-key.pem?

I moved it to a file instead, I was just copying the way the der key was included raw

dgarske · 2023-01-13T01:39:53Z

examples/keygen/keyimport.c

@@ -55,6 +86,8 @@ int TPM2_Keyimport_Example(void* userCtx, int argc, char *argv[])
    TPM_ALG_ID paramEncAlg = TPM_ALG_NULL;
    WOLFTPM2_SESSION tpmSession;
    const char* outputFile = "keyblob.bin";
+    uint8_t derEncode = 0;


I like stdint.h types, but please use byte. Thanks

so is it only wolfBoot that uses uint types?

dgarske · 2023-01-13T01:40:44Z

examples/keygen/keyimport.c

+        }
+        else if (pemEncode == 1) {
+            rc = wolfTPM2_RsaPrivateKeyImportPem(&dev, &storage, &impKey,
+                kRsaKeyPrivPem, strlen(kRsaKeyPrivPem), NULL,


Please use (word32)XSTRLEN(kRsaKeyPrivPem).

dgarske · 2023-01-13T01:42:21Z

src/tpm2_wrap.c

+    word32  pSz = sizeof(p);
+    word32  qSz = sizeof(q);
+
+    if (dev && parentKey && keyBlob && input && inSz != 0)


Confusing logic. Would prefer init rc = 0 and set rc = BAD_FUNC_ARG; on failure.

saw it in some wolfssl function and thought it was concise, if it's not preferred I'll change it

dgarske · 2023-01-13T01:42:43Z

src/tpm2_wrap.c

+    byte d[RSA_MAX_SIZE / 8];
+    byte p[RSA_MAX_SIZE / 8];
+    byte q[RSA_MAX_SIZE / 8];
+    word32  eSz = sizeof(e);


Must cast these: word32 eSz = (word32)sizeof(e);

src/tpm2_wrap.c

dgarske · 2023-01-13T01:45:27Z

src/tpm2_wrap.c

+    /* der size is base 64 decode length */
+    derSz = inSz * 3 / 4 + 1;
+
+    derBuf = XMALLOC(derSz, NULL, DYNAMIC_TYPE_TMP_BUFFER);


derBuf = (byte*)XMALLOC

I need to remember that, you've corrected me on it before but I keep doing it

dgarske · 2023-01-13T01:45:55Z

src/tpm2_wrap.c

+    if (rc == 0)
+        rc = wc_KeyPemToDer((byte*)input, inSz, derBuf, derSz, pass);
+
+    /* returns the number of bytes*/


Nit: space between bytes*/

dgarske · 2023-01-13T01:46:20Z

src/tpm2_wrap.c

+        rc = wc_KeyPemToDer((byte*)input, inSz, derBuf, derSz, pass);
+
+    /* returns the number of bytes*/
+    if (rc > 0)


What about rc == 0 case?

can you have a der key with 0 byte length?I thought that would return bad arg error, I looked at the underlying function PemToDer and if it doesn't find an asn header it returns ASN_NO_PEM_HEADER which is less than 0

dgarske · 2023-01-13T01:47:48Z

examples/keygen/keyimport.c

@@ -77,6 +110,15 @@ int TPM2_Keyimport_Example(void* userCtx, int argc, char *argv[])
        else if (XSTRCMP(argv[argc-1], "-xor") == 0) {
            paramEncAlg = TPM_ALG_XOR;
        }
+        else if (XSTRCMP(argv[argc-1], "-pem") == 0) {


-pem is in here twice. Looks like copy/paste issue. This one should be deleted.

certs/example-rsa-key.pem

dgarske · 2023-01-23T21:32:54Z

examples/keygen/keyimport.c

+    byte derEncode = 0;
+    byte pemEncode = 0;
+    FILE* pemFile = NULL;
+    char pemBuf[2048];


Just use WOLFTPM2_MAX_BUFFER

dgarske · 2023-01-23T21:33:18Z

examples/keygen/keyimport.c

-            rc = wolfTPM2_RsaPrivateKeyImportPem(&dev, &storage, &impKey,
-                kRsaKeyPrivPem, strlen(kRsaKeyPrivPem), NULL,
-                TPM_ALG_NULL, TPM_ALG_NULL);
+            pemFile = fopen("./certs/example-rsa-key.pem", "r");


Please use XFOPEN

Add support to take the key filename as argument. Default to this example.

dgarske · 2023-01-23T21:33:37Z

examples/keygen/keyimport.c

-                TPM_ALG_NULL, TPM_ALG_NULL);
+            pemFile = fopen("./certs/example-rsa-key.pem", "r");
+
+            rc = fread(pemBuf, 1, 2048, pemFile);


Please use XFREAD and use sizeof(pemBuf)

dgarske · 2023-01-23T21:33:47Z

examples/keygen/keyimport.c

+                    pemBuf, rc, NULL, TPM_ALG_NULL, TPM_ALG_NULL);
+            }
+
+            fclose(pemFile);


dgarske · 2023-01-23T21:35:03Z

src/tpm2_wrap.c

+    if (derBuf == NULL)
+        return MEMORY_E;
+
+    if (rc == 0)


This rc == 0 is not needed. Remove please

examples/keygen/keyimport.c

dgarske · 2023-01-26T16:37:02Z

certs/include.am

@@ -7,4 +7,5 @@ EXTRA_DIST += \
 	     certs/ca-rsa.cnf \
 	     certs/ca-ecc.cnf \
 	     certs/wolf-ca-ecc-cert.pem \
-	     certs/wolf-ca-rsa-cert.pem 
+       certs/wolf-ca-rsa-cert.pem \


Nit: Indentation is off here.

dgarske · 2023-01-26T16:37:57Z

examples/keygen/keyimport.c

    printf("* -ecc: Use RSA or ECC for keys\n");
    printf("* -aes/xor: Use Parameter Encryption\n");
+    printf("* -pem/der: Key encoding type, none for binary\n");


NIT: I'd prefer -pem= support where the =name is optional.

dgarske · 2023-01-26T16:40:20Z

examples/keygen/keyimport.c

+    byte pemEncode = 0;
+    FILE* pemFile = NULL;
+    char pemBuf[WOLFTPM2_MAX_BUFFER];
+    char pemName[WOLFTPM2_MAX_BUFFER];


This is too long for filename and it should just be a char* with default that can be changed.

const char* pemName = "./certs/example-rsa-key.pem"; /* then later just set this to the argv */ pemName = (const char*)argv[i + 1];`

dgarske · 2023-01-26T16:41:09Z

examples/keygen/keyimport.c

+                printf("Failed to read pem file %s\n", pemName);
+
+            if (rc == 0)
+                rc = XFREAD(pemBuf, 1, sizeof(pemBuf), pemFile);


Add explicit cast to int:

examples/keygen/keyimport.c:167:22: error: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Werror,-Wshorten-64-to-32] rc = XFREAD(pemBuf, 1, sizeof(pemBuf), pemFile); ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

how do I test this, I need to set CC to clang?

dgarske · 2023-01-26T17:03:06Z

examples/keygen/keyimport.c

+                printf("Warning: No pem file specified, using default: %s\n", pemName);
+            }
+            else {
+                XMEMCPY(pemName, argv[i + 1], XSTRLEN(argv[i + 1]) + 1);


Please print the name of the file used in the console output.

examples/keygen/keyimport.c

src/tpm2_wrap.c

dgarske · 2023-01-26T17:04:03Z

src/tpm2_wrap.c

+    derSz = inSz * 3 / 4 + 1;
+
+    derBuf = (byte*)XMALLOC(derSz, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+


Remove the extra line here.

dgarske

Very close! Looks great, just a couple comments.

dgarske · 2023-01-27T17:31:35Z

src/tpm2_wrap.c

+    word32  pSz = (word32)sizeof(p);
+    word32  qSz = (word32)sizeof(q);
+
+    wc_InitRsaKey(key, NULL);


Please check the return code here. Only call free if rc == 0.

src/tpm2_wrap.c

add rsa key import methods to handle pem and der encoding directly

b57be63

jpbland1 requested a review from dgarske January 11, 2023 23:55

jpbland1 marked this pull request as ready for review January 11, 2023 23:55

dgarske self-assigned this Jan 12, 2023

dgarske requested changes Jan 13, 2023

View reviewed changes

dgarske assigned jpbland1 and unassigned dgarske Jan 13, 2023

update rsa key import functions based on pr comments

114940d

dgarske self-requested a review January 23, 2023 21:30

dgarske self-assigned this Jan 23, 2023

dgarske requested changes Jan 23, 2023

View reviewed changes

dgarske removed their assignment Jan 23, 2023

update pem keyimport based on pr comments

013154b

jpbland1 requested a review from dgarske January 25, 2023 02:02

dgarske self-assigned this Jan 25, 2023

dgarske requested changes Jan 26, 2023

View reviewed changes

dgarske removed their assignment Jan 26, 2023

update based on pr comments

24fb52d

jpbland1 requested a review from dgarske January 27, 2023 00:02

add rsa init statement

7feeed6

dgarske requested changes Jan 27, 2023

View reviewed changes

add check for rsa init and check for zero length der buffer

21edd65

jpbland1 requested a review from dgarske January 27, 2023 21:49

dgarske approved these changes Jan 27, 2023

View reviewed changes

dgarske assigned dgarske and unassigned jpbland1 Jan 27, 2023

dgarske merged commit 3a1ece8 into wolfSSL:master Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rsa key import methods to handle pem and der encoding directly #252

add rsa key import methods to handle pem and der encoding directly #252

jpbland1 commented Jan 11, 2023 •

edited

dgarske Jan 13, 2023

jpbland1 Jan 16, 2023

dgarske Jan 13, 2023

jpbland1 Jan 16, 2023

dgarske Jan 13, 2023

dgarske Jan 13, 2023

jpbland1 Jan 16, 2023

dgarske Jan 13, 2023

dgarske Jan 13, 2023

jpbland1 Jan 16, 2023

dgarske Jan 13, 2023

dgarske Jan 13, 2023

jpbland1 Jan 16, 2023

dgarske Jan 13, 2023

dgarske Jan 23, 2023

dgarske Jan 23, 2023

dgarske Jan 23, 2023

dgarske Jan 23, 2023

dgarske Jan 23, 2023

dgarske Jan 23, 2023

dgarske Jan 26, 2023

dgarske Jan 26, 2023

dgarske Jan 26, 2023

dgarske Jan 26, 2023

jpbland1 Jan 26, 2023

dgarske Jan 26, 2023

dgarske Jan 26, 2023

dgarske left a comment

dgarske Jan 27, 2023

		@@ -33,16 +33,47 @@

		#ifndef WOLFTPM2_NO_WRAPPER

		static const char* kRsaKeyPrivPem =

		derSz = inSz * 3 / 4 + 1;

		derBuf = (byte*)XMALLOC(derSz, NULL, DYNAMIC_TYPE_TMP_BUFFER);

add rsa key import methods to handle pem and der encoding directly #252

add rsa key import methods to handle pem and der encoding directly #252

Conversation

jpbland1 commented Jan 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbland1 commented Jan 11, 2023 •

edited