PKCS7 data support #7282

jpk233 · 2021-06-01T18:27:12Z

Problem

We're lacking support within the cryptoPAL libraries for certificate data in PKCS7 format.

Change overview

What's in this PR

Introduces new set of APIs - with OpenSSL - for working with PKCS7 data.

Testing

How was this tested? (at least one bullet point required)
* Unit tests were added for validating certificate extraction from PKCS7 format.
* An additional test file was added, X509_PKCS7Extraction_test_vectors.h, containing dummy certificate data to be used for the added unit test.

todo · 2021-06-01T18:27:14Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1587 to 1597 in a0256fc

    
           // TODO -> error value 
        
           VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           p7_type = OBJ_obj2nid(p7->type); 
        
           if (p7_type == NID_pkcs7_signed) 
        
           { 
        
               certs = p7->d.sign->cert; 
        
           } 
        
           else if (p7_type == NID_pkcs7_signedAndEnveloped) 
        
           { 
        
               certs = p7->d.signed_and_enveloped->cert;

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

todo · 2021-06-01T18:27:17Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1600 to 1610 in a0256fc

    
           // TODO -> error value 
        
           VerifyOrExit(certs != NULL, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           VerifyOrExit(static_cast<uint32_t>(sk_X509_num(certs)) <= *max_certs, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           *max_certs = static_cast<uint32_t>(sk_X509_num(certs)); 
        
           certs = X509_chain_up_ref(certs); 
        
           for (uint32_t i = 0; i < *max_certs; ++i) 
        
           { 
        
               size_t bytes_written          = 0;

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

todo · 2021-06-01T18:27:20Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1642 to 1652 in a0256fc

    
           // TODO -> error value 
        
           VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           p7_type = OBJ_obj2nid(p7->type); 
        
           if (p7_type == NID_pkcs7_signed) 
        
           { 
        
               certs = p7->d.sign->cert; 
        
           } 
        
           else if (p7_type == NID_pkcs7_signedAndEnveloped) 
        
           { 
        
               certs = p7->d.signed_and_enveloped->cert;

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

todo · 2021-06-01T18:27:23Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1655 to 1665 in a0256fc

    
           // TODO -> error value 
        
           VerifyOrExit(certs != NULL, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           VerifyOrExit(n_cert < static_cast<uint32_t>(sk_X509_num(certs)), err = CHIP_ERROR_INVALID_ARGUMENT); 
        
           certs = X509_chain_up_ref(certs); 
        
           { 
        
               size_t bytes_written          = 0; 
        
               unsigned char * pX509ListEnd  = reinterpret_cast<unsigned char *>(x509list); 
        
               unsigned char ** pX509ListAux = &pX509ListEnd;

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

todo · 2021-06-01T18:27:26Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1694 to 1704 in a0256fc

    
           // TODO -> error value 
        
           VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           p7_type = OBJ_obj2nid(p7->type); 
        
           if (p7_type == NID_pkcs7_signed) 
        
           { 
        
               certs = p7->d.sign->cert; 
        
           } 
        
           else if (p7_type == NID_pkcs7_signedAndEnveloped) 
        
           { 
        
               certs = p7->d.signed_and_enveloped->cert;

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

todo · 2021-06-01T18:27:29Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1707 to 1717 in a0256fc

    
               // TODO -> error value 
        
               VerifyOrExit(certs != NULL, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
               *n_certs = static_cast<uint32_t>(sk_X509_num(certs)); 
        
           exit: 
        
               BIO_free_all(bio_cert); 
        
               PKCS7_free(p7); 
        
               return err; 
        
           }

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

woody-apple · 2021-06-02T05:12:07Z

/rebase

todo · 2021-06-02T14:22:19Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1587 to 1597 in a36794b

    
           // TODO -> error value 
        
           VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           p7_type = OBJ_obj2nid(p7->type); 
        
           if (p7_type == NID_pkcs7_signed) 
        
           { 
        
               certs = p7->d.sign->cert; 
        
           } 
        
           else if (p7_type == NID_pkcs7_signedAndEnveloped) 
        
           { 
        
               certs = p7->d.signed_and_enveloped->cert;

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

todo · 2021-06-02T14:22:22Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1600 to 1610 in a36794b

    
           // TODO -> error value 
        
           VerifyOrExit(certs != NULL, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           VerifyOrExit(static_cast<uint32_t>(sk_X509_num(certs)) <= *max_certs, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           *max_certs = static_cast<uint32_t>(sk_X509_num(certs)); 
        
           certs = X509_chain_up_ref(certs); 
        
           for (uint32_t i = 0; i < *max_certs; ++i) 
        
           { 
        
               size_t bytes_written          = 0;

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

todo · 2021-06-02T14:22:25Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1642 to 1652 in a36794b

    
           // TODO -> error value 
        
           VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           p7_type = OBJ_obj2nid(p7->type); 
        
           if (p7_type == NID_pkcs7_signed) 
        
           { 
        
               certs = p7->d.sign->cert; 
        
           } 
        
           else if (p7_type == NID_pkcs7_signedAndEnveloped) 
        
           { 
        
               certs = p7->d.signed_and_enveloped->cert;

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

todo · 2021-06-02T14:22:28Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1655 to 1665 in a36794b

    
           // TODO -> error value 
        
           VerifyOrExit(certs != NULL, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           VerifyOrExit(n_cert < static_cast<uint32_t>(sk_X509_num(certs)), err = CHIP_ERROR_INVALID_ARGUMENT); 
        
           certs = X509_chain_up_ref(certs); 
        
           { 
        
               size_t bytes_written          = 0; 
        
               unsigned char * pX509ListEnd  = reinterpret_cast<unsigned char *>(x509list); 
        
               unsigned char ** pX509ListAux = &pX509ListEnd;

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

todo · 2021-06-02T14:22:31Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1694 to 1704 in a36794b

    
           // TODO -> error value 
        
           VerifyOrExit(p7 != nullptr, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
           p7_type = OBJ_obj2nid(p7->type); 
        
           if (p7_type == NID_pkcs7_signed) 
        
           { 
        
               certs = p7->d.sign->cert; 
        
           } 
        
           else if (p7_type == NID_pkcs7_signedAndEnveloped) 
        
           { 
        
               certs = p7->d.signed_and_enveloped->cert;

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

todo · 2021-06-02T14:22:34Z

-> error value

connectedhomeip/src/crypto/CHIPCryptoPALOpenSSL.cpp

Lines 1707 to 1717 in a36794b

    
               // TODO -> error value 
        
               VerifyOrExit(certs != NULL, err = CHIP_ERROR_WRONG_CERT_TYPE); 
        
               *n_certs = static_cast<uint32_t>(sk_X509_num(certs)); 
        
           exit: 
        
               BIO_free_all(bio_cert); 
        
               PKCS7_free(p7); 
        
               return err; 
        
           }

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

msandstedt · 2021-06-02T14:29:13Z

src/crypto/CHIPCryptoPAL.h

@@ -36,6 +36,8 @@
 namespace chip {
 namespace Crypto {

+const size_t kMax_x509_Certificate_Length = 1024;


Where does this come from?

So it appears the CSR (or elements thereof) is limited to 1024 bytes. I'm not seeing the actual certificate size limit in the spec though. I assume there has to be a limit. I'm just looking for confirmation that this is correctly set to that limit.

So now I see that this was changed to 600 bytes. And indeed, the spec does clarify that the root CA certificate must be 600 or fewer bytes. But do we have different limits for ICA or NOC? This part wasn't clear to me.

As far as I know, only root certificates have a defined max length, but this ensures standard compact sizing when dealing with a full certificate chain.

So I guess then I don't understand how we're supposed to succeed in this PR. It looks like the PR attempts to also store CA and leaf certs within kMax_x509_Certificate_Length bytes, even though this is only guaranteed to fit the root cert.

This doesn't seem like it will work. Am I missing something?

The 600 limit is for DAC and PAI and PAA which are DER.

For opcerts, RCAC, ICAC and NOC are supposed to be TLV certs and max 400 bytes.

todo · 2021-06-02T19:01:00Z

Add mbedTLS implementation for PKCS7 and Pubkey Extraction methods

connectedhomeip/src/crypto/tests/CHIPCryptoPALTest.cpp

Lines 1438 to 1448 in 9136fa8

    
           // TODO: Add mbedTLS implementation for PKCS7 and Pubkey Extraction methods 
        
           #if CHIP_CRYPTO_OPENSSL 
        
           static void TestX509_PKCS7Extraction(nlTestSuite * inSuite, void * inContext) 
        
           { 
        
               CHIP_ERROR err = CHIP_NO_ERROR; 
        
               X509DerCertificate x509list[3]; 
        
               uint32_t max_certs = sizeof(x509list) / sizeof(X509DerCertificate); 
        
               err = LoadCertsFromPKCS7(reinterpret_cast<const uint8_t *>(pem_pkcs7_blob), x509list, &max_certs); 
        
               NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

This comment was generated by todo based on a `TODO` comment in `9136fa8` in #7282. cc @jpk233.

src/crypto/CHIPCryptoPAL.h

src/crypto/CHIPCryptoPALOpenSSL.cpp

tcarmelveilleux

PKCS Fixing company for Chris #7 is not called-out anywhere in spec
OpCSR response has a CSR in PKCS Update cxx_rules #10
Credentials on device are supposed to be Matter TLV certificates (see 6.4. Operational Certificate Encoding in 0.7-core ballot version.

So the question really is: why is PKCS #7 format entering the picture at this time? What is the usage expected of introducing this in the codebase?

hawk248 · 2021-06-04T17:24:02Z

So the question really is: why is PKCS #7 format entering the picture at this time? What is the usage expected of introducing this in the codebase?

Most PKI systems deliver cert chain in PKCS7 format ... spec does not specify that but most Commissioners will benefit from this API.

tcarmelveilleux · 2021-06-04T18:07:48Z

So the question really is: why is PKCS #7 format entering the picture at this time? What is the usage expected of introducing this in the codebase?

Most PKI systems deliver cert chain in PKCS7 format ... spec does not specify that but most Commissioners will benefit from this API.

So is the point of this PR to add optional implementation-specific things that only some commissioners will use? Why would this code not only exist in a particular commissioning example? The code for how the chain resulting from a CSR comes in can be illustrated, but is really code that will be particular to every implementation. The PKCS 7 code added here does not appear to be used, just added to the library. Is there a subsequent PR that will introduce code depending on it?

hawk248 · 2021-06-04T22:57:01Z

So the question really is: why is PKCS #7 format entering the picture at this time? What is the usage expected of introducing this in the codebase?

Most PKI systems deliver cert chain in PKCS7 format ... spec does not specify that but most Commissioners will benefit from this API.

So is the point of this PR to add optional implementation-specific things that only some commissioners will use? Why would this code not only exist in a particular commissioning example? The code for how the chain resulting from a CSR comes in can be illustrated, but is really code that will be particular to every implementation. The PKCS 7 code added here does not appear to be used, just added to the library. Is there a subsequent PR that will introduce code depending on it?

PKCS7 is a generic standard to encode chain certificates just as pkcs10 provides a standard way to request a certificate. We also know that commissioners will not be just apps but can also be devices. So this will have utility across many implementations - otherwise we are paving the way for forks early on in the project. Do you propose a way for a comissioner to receive a certificate chain other than pkcs7 ? We will be happy to look at it if you provide implementation example.

And yes, we will push additional PR's that will make use of this function.

woody-apple · 2021-06-04T23:58:40Z

/rebase

… not currently support pkcs7

tcarmelveilleux

I approve this PR, conditional on ensuring that commissionee devices (accessories) do not see an increase flash cost due to the insertion of PKCS7 support, which only commissioner <-> CA communication would ever use, and only in some cases.

hawk248 · 2021-06-08T12:03:51Z

I approve this PR, conditional on ensuring that commissionee devices (accessories) do not see an increase flash cost due to the insertion of PKCS7 support, which only commissioner <-> CA communication would ever use, and only in some cases.

Thanks. Makes sense.

hawk248

👍

* Add APIs to work with PKCS7 data * Initial mbedTLS function support, not yet implemented as mbedtls does not currently support pkcs7 * Update mbedTLS tests, define certificate size as per spec * Update with Andy's suggestions * Fixed duplicate changes in cryptopal * Restyle changes

boring-cyborg bot added the crypto label Jun 1, 2021

jpk233 changed the title ~~Crypto~~ PKCS7 data support Jun 1, 2021

pullapprove bot requested review from andy31415, bhaskar-apple, bzbarsky-apple, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs and msandstedt June 1, 2021 18:38

pullapprove bot added the review - pending label Jun 1, 2021

msandstedt reviewed Jun 2, 2021

View reviewed changes

woody-apple approved these changes Jun 3, 2021

View reviewed changes

andy31415 reviewed Jun 3, 2021

View reviewed changes

src/crypto/CHIPCryptoPAL.h Outdated Show resolved Hide resolved

andy31415 reviewed Jun 3, 2021

View reviewed changes

src/crypto/CHIPCryptoPALOpenSSL.cpp Show resolved Hide resolved

tcarmelveilleux requested changes Jun 4, 2021

View reviewed changes

jpk233 added 4 commits June 5, 2021 00:18

Add APIs to work with PKCS7 data

63cb744

Initial mbedTLS function support, not yet implemented as mbedtls does…

7cc475e

… not currently support pkcs7

Update mbedTLS tests, define certificate size as per spec

26600f2

Update with Andy's suggestions

8f9b0c8

hawk248 requested a review from tcarmelveilleux June 6, 2021 21:08

tcarmelveilleux approved these changes Jun 7, 2021

View reviewed changes

msandstedt approved these changes Jun 7, 2021

View reviewed changes

chrisdecenzo approved these changes Jun 8, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jun 8, 2021

jpk233 added 2 commits June 8, 2021 14:29

Merge branch 'master' into crypto

4b461cd

Fixed duplicate changes in cryptopal

663f763

restyled-io bot mentioned this pull request Jun 8, 2021

Restyle PKCS7 data support #7463

Closed

Restyle changes

5091bb8

hawk248 approved these changes Jun 9, 2021

View reviewed changes

hawk248 merged commit 77e727c into project-chip:master Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PKCS7 data support #7282

PKCS7 data support #7282

jpk233 commented Jun 1, 2021

todo bot commented Jun 1, 2021

todo bot commented Jun 1, 2021

todo bot commented Jun 1, 2021

todo bot commented Jun 1, 2021

todo bot commented Jun 1, 2021

todo bot commented Jun 1, 2021

woody-apple commented Jun 2, 2021

todo bot commented Jun 2, 2021

todo bot commented Jun 2, 2021

todo bot commented Jun 2, 2021

todo bot commented Jun 2, 2021

todo bot commented Jun 2, 2021

todo bot commented Jun 2, 2021

msandstedt Jun 2, 2021

msandstedt Jun 2, 2021

msandstedt Jun 2, 2021

jpk233 Jun 3, 2021

msandstedt Jun 3, 2021

tcarmelveilleux Jun 4, 2021

todo bot commented Jun 2, 2021

tcarmelveilleux left a comment

hawk248 commented Jun 4, 2021

tcarmelveilleux commented Jun 4, 2021

hawk248 commented Jun 4, 2021 •

edited

woody-apple commented Jun 4, 2021

tcarmelveilleux left a comment

hawk248 commented Jun 8, 2021

hawk248 left a comment

PKCS7 data support #7282

PKCS7 data support #7282

Conversation

jpk233 commented Jun 1, 2021

Problem

Change overview

Testing

todo bot commented Jun 1, 2021

-> error value

This comment was generated by todo based on a TODO comment in a0256fc in #7282. cc @jpk233.

todo bot commented Jun 1, 2021

-> error value

This comment was generated by todo based on a TODO comment in a0256fc in #7282. cc @jpk233.

todo bot commented Jun 1, 2021

-> error value

This comment was generated by todo based on a TODO comment in a0256fc in #7282. cc @jpk233.

todo bot commented Jun 1, 2021

-> error value

This comment was generated by todo based on a TODO comment in a0256fc in #7282. cc @jpk233.

todo bot commented Jun 1, 2021

-> error value

This comment was generated by todo based on a TODO comment in a0256fc in #7282. cc @jpk233.

todo bot commented Jun 1, 2021

-> error value

This comment was generated by todo based on a TODO comment in a0256fc in #7282. cc @jpk233.

woody-apple commented Jun 2, 2021

todo bot commented Jun 2, 2021

-> error value

This comment was generated by todo based on a TODO comment in a36794b in #7282. cc @jpk233.

todo bot commented Jun 2, 2021

-> error value

This comment was generated by todo based on a TODO comment in a36794b in #7282. cc @jpk233.

todo bot commented Jun 2, 2021

-> error value

This comment was generated by todo based on a TODO comment in a36794b in #7282. cc @jpk233.

todo bot commented Jun 2, 2021

-> error value

This comment was generated by todo based on a TODO comment in a36794b in #7282. cc @jpk233.

todo bot commented Jun 2, 2021

-> error value

This comment was generated by todo based on a TODO comment in a36794b in #7282. cc @jpk233.

todo bot commented Jun 2, 2021

-> error value

This comment was generated by todo based on a TODO comment in a36794b in #7282. cc @jpk233.

msandstedt Jun 2, 2021

Choose a reason for hiding this comment

msandstedt Jun 2, 2021

Choose a reason for hiding this comment

msandstedt Jun 2, 2021

Choose a reason for hiding this comment

jpk233 Jun 3, 2021

Choose a reason for hiding this comment

msandstedt Jun 3, 2021

Choose a reason for hiding this comment

tcarmelveilleux Jun 4, 2021

Choose a reason for hiding this comment

todo bot commented Jun 2, 2021

Add mbedTLS implementation for PKCS7 and Pubkey Extraction methods

This comment was generated by todo based on a TODO comment in 9136fa8 in #7282. cc @jpk233.

tcarmelveilleux left a comment

Choose a reason for hiding this comment

hawk248 commented Jun 4, 2021

tcarmelveilleux commented Jun 4, 2021

hawk248 commented Jun 4, 2021 • edited

woody-apple commented Jun 4, 2021

tcarmelveilleux left a comment

Choose a reason for hiding this comment

hawk248 commented Jun 8, 2021

hawk248 left a comment

Choose a reason for hiding this comment

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a0256fc` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `a36794b` in #7282. cc @jpk233.

This comment was generated by todo based on a `TODO` comment in `9136fa8` in #7282. cc @jpk233.

hawk248 commented Jun 4, 2021 •

edited