Support for more cert subject OIDs and raw subject access #1734

embhorn · 2018-08-01T20:28:51Z

Cert subjects OIDs that are not supported in the parser are skipped. So in order to support copying a subject to a new cert, the wc_SetSubjectRaw() method is added.

Also adding support for the businessCategory, jurisdiction of incorporation country, and jurisdiction of incorporation state OIDs.

embhorn · 2018-08-03T16:39:40Z

retest this please

dgarske

@embhorn : Just a few minor things. Great work!

dgarske · 2018-08-03T21:31:01Z

wolfcrypt/src/asn.c

@@ -4236,6 +4252,75 @@ static int GetName(DecodedCert* cert, int nameType)

            cert->srcIdx += strLen;
        }
+    #ifdef WOLFSSL_CERT_EXT
+        else if ((0 == memcmp(&cert->source[cert->srcIdx],


Please use XMEMCMP here.

dgarske · 2018-08-03T21:31:42Z

wolfcrypt/src/asn.c

+        else if ((0 == memcmp(&cert->source[cert->srcIdx],
+                    "\x2b\x06\x01\x04\x01\x82\x37\x3c\x02\x01", 10)) &&
+                 ((cert->source[cert->srcIdx + 10] == 0x3) ||
+                  (cert->source[cert->srcIdx + 10] == 0x2)))


Can you add a define for these consts so its easier to understand? Including the places 0x03 and 0x02 are used in the if logic below.

dgarske · 2018-08-03T21:33:32Z

wolfcrypt/src/asn.c

@@ -11075,6 +11172,17 @@ int wc_MakeSelfCert(Cert* cert, byte* buffer, word32 buffSz,

 #ifdef WOLFSSL_CERT_EXT

+/* Get raw subject from cert, which may contain OIDs not parsed by Decode. */
+int wc_GetSubjectRaw(byte **subjectRaw, Cert *cert)


Can you also add some docs for this API in doc/dox_comments/header_files/asn_public.h?

dgarske · 2018-08-03T21:34:52Z

wolfcrypt/src/asn.c

+    }
+#else
+    /* Fields are not accessible */
+    ret = -1;


In the !IGNORE_NAME_CONSTRAINT case the previous ret is overwritten even in the ParseCertRelative case.

dgarske · 2018-08-03T21:35:42Z

wolfcrypt/src/asn.c

+    DecodedCert decoded[1];
+#endif
+
+    if (derSz < 0) {


Can you also add a bad arg check for sbjRaw == NULL?

dgarske · 2018-08-03T21:36:38Z

wolfcrypt/test/test.c

@@ -389,7 +389,7 @@ static void myFipsCb(int ok, int err, const char* hash)
    #ifdef BENCH_EMBEDDED
        static byte gTestMemory[10000];
    #elif defined(USE_FAST_MATH) && !defined(ALT_ECC_SIZE)
-        static byte gTestMemory[130000];
+        static byte gTestMemory[140000];


Was this increase required even in the !WOLFSSL_CERT_EXT case? If its just for the WOLFSSL_CERT_EXT case I'd like to only increase it for that build case.

dgarske · 2018-08-03T21:37:55Z

wolfssl/wolfcrypt/memory.h

-        /* default size of chunks of memory to seperate into
-         * having session certs enabled makes a 21k SSL struct */
+        /* default size of chunks of memory to separate into
+         * having session certs enabled makes a 24k SSL struct */


Can you describe why this increase was required? If its just for the WOLFSSL_CERT_EXT case maybe we should only increase it for that build case.

The increased memory requirement was due to the addition of the raw subject and issuer fields in the cert structure, and the newly supported OIDs in the certName structure.

dgarske · 2018-08-03T21:38:29Z

wolfcrypt/src/asn.c

+/* Set cert raw subject from DER buffer */
+int wc_SetSubjectRaw(Cert* cert, const byte* der, int derSz)
+{
+    return SetSubjectRawFromCert(cert->sbjRaw, der, derSz);


Can we add a NULL check for cert?

dgarske · 2018-08-03T21:40:02Z

wolfcrypt/src/asn.c

+{
+    int rc = 0;
+    if ((subjectRaw != NULL) && (cert != NULL)) {
+        *subjectRaw = cert->sbjRaw;


Looks like this is just returning the pointer to it. Make sure folks know this pointer only lives as long as Cert is available.

dgarske

@embhorn : Changes look great! For this to be complete I'd like to see at least API unit test for wc_SetSubjectRaw and wc_GetSubjectRaw. Also you added wolfCrypt tests for building a cert with the new business and jurisdiction fields, but can we add a test certificate for parsing these in ./certs? Thanks!

embhorn · 2018-08-10T21:07:18Z

retest this please

dgarske · 2018-08-10T22:21:20Z

certs/test/cert-ext-joi.pem

@@ -0,0 +1,76 @@
+Certificate:


Make sure and add any new files to the include.am.

dgarske

Thanks Eric! Great addition!

…ith email name in CSR. (Thanks to Forum post https://www.wolfssl.com/forums/post4137.html). Failed examples: ``` 145:d=5 hl=2 l= 16 prim: EOC 0000 - 69 6e 66 6f 40 77 6f 6c-66 73 73 6c 2e 63 6f 6d info@wolfssl.com ``` ``` SET { 138 23: SEQUENCE { 140 3: OBJECT IDENTIFIER objectClass (2 5 4 0) : Error: Spurious EOC in definite-length item. ``` Success Examples: ``` 140:d=5 hl=2 l= 9 prim: OBJECT :emailAddress 151:d=5 hl=2 l= 16 prim: IA5STRING :info@wolfssl.com ``` ``` SET { 138 29: SEQUENCE { 140 9: OBJECT IDENTIFIER emailAddress (1 2 840 113549 1 9 1) 151 16: IA5String 'info@wolfssl.com' ```

embhorn added 4 commits July 25, 2018 16:47

Add businessCategory OID

0ae43b7

Raw subject support methods

4514a08

Support for jurisdiction OIDs

3be008d

Fix from review

5279b41

dgarske assigned embhorn Aug 2, 2018

Wrap in WOLFSSL_CERT_EXT

d09a571

Fix from review

3e85f9c

embhorn assigned dgarske and unassigned embhorn Aug 3, 2018

dgarske requested changes Aug 3, 2018

View reviewed changes

embhorn added 3 commits August 4, 2018 17:50

Updates from review

b10e7f2

Merge branch 'master' into zd4130

607dea4

Update from review

46800b5

embhorn assigned embhorn and unassigned dgarske Aug 6, 2018

Update from review

7a30e8a

dgarske requested changes Aug 6, 2018

View reviewed changes

embhorn added 3 commits August 8, 2018 08:04

API test

27dd4de

Adding tests

eba3441

Fix test

090f061

Merge branch 'master' of https://github.com/wolfssl/wolfssl into zd4130

05b08e5

dgarske reviewed Aug 10, 2018

View reviewed changes

Fix merge

031424a

dgarske approved these changes Aug 12, 2018

View reviewed changes

dgarske merged commit bb574d2 into wolfSSL:master Aug 12, 2018

embhorn deleted the zd4130 branch August 13, 2018 13:35

dgarske mentioned this pull request Aug 31, 2018

Fix for CSR generation email value #1805

Merged

embhorn mentioned this pull request Sep 19, 2018

Revert addition of OIDs to cnf #1840

Merged

kaleb-himes added a commit to kaleb-himes/wolfssl that referenced this pull request Sep 19, 2018

White space updates and revert cnf changes in lieu of PR wolfSSL#1734

f3fd67c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for more cert subject OIDs and raw subject access #1734

Support for more cert subject OIDs and raw subject access #1734

embhorn commented Aug 1, 2018

embhorn commented Aug 3, 2018

dgarske left a comment

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske Aug 3, 2018

embhorn Aug 4, 2018

dgarske left a comment

embhorn commented Aug 10, 2018

dgarske Aug 10, 2018

embhorn Aug 10, 2018

dgarske left a comment

Support for more cert subject OIDs and raw subject access #1734

Support for more cert subject OIDs and raw subject access #1734

Conversation

embhorn commented Aug 1, 2018

embhorn commented Aug 3, 2018

dgarske left a 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

embhorn commented Aug 10, 2018

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