PKCS7 Parser - RFC 2315

[naynajain]

Hi,

This pull request is to add PKCS7 parser feature in mbedtls. This is following RFC 2315

The support is limited to parsing of SignedData content type. This is the base PKCS7 parser work done based on our current requirement and can easily be extended to full fledged parser over the time as needed. Currently, it doesn't handle CRLs which are optional in PKCS7 spec and looks for single signer info.

I had to do an additional fix to make the build work using CMake in gcc9.0. I had to disable -Wtype-limits error by adding -Wno-type-limits. This is a general patch to fix the error reported in SSL. It doesn't have anything to do with PKCS7, but is identified while testing pkcs7 build with cmake.

Status

READY

Migrations

There is no API change here. Its a new feature added.

Additional comments

Since the files - library/error.c and library/version_features.c and programs/test/query_config.c got generated by running the scripts, I have added them as separate commits.

There are few additional commits added separately which are on top of base PKCS7 work.

Steps to test or reproduce

1. Test suite is written which checks for both failure/success cases. They will get run as part of
   "make test"
   "cmake test"
   "cd tests && ./test_suite_pkcs7".

2. One can try to write a program to parse the pkcs7 based signature using:
   mbedtls_pkcs7_parse_der() and then mbedtls_pkcs7_signed_data_verify() function.

[gilles-peskine-arm]

Thank you for your contribution! I've made a first, superficial pass over it. Before we do an in-depth review:

• Please address the issues I raised (separating bug fixes; ).
• Please add a changelog entry file under ChangeLog.d for the new feature.
• Please address whatever's causing the travis-ci job to fail. Once this job passes, if there are failures in the jenkins jobs, which run on a private server, ping us to let you know what's failing.

I have not done an architectural review. I don't expect surprises here, since this is adding a module with a well-defined purpose.

Unfortunately, our bandwidth for reviews is full at the moment, so it may be a while before we can review and merge this code. Before contributing a major feature, it's a good idea to mention it on the mailing list first, to discuss the architecture and set expectations for the schedule.

[naynajain]

Thank you for your contribution! I've made a first, superficial pass over it. Before we do an in-depth review:

* Please address the issues I raised (separating bug fixes; ).

* Please add a changelog entry file under `ChangeLog.d` for the new feature.

* Please address whatever's causing the travis-ci job to fail. Once this job passes, if there are failures in the jenkins jobs, which run on a private server, ping us to let you know what's failing.

I have not done an architectural review. I don't expect surprises here, since this is adding a module with a well-defined purpose.

Unfortunately, our bandwidth for reviews is full at the moment, so it may be a while before we can review and merge this code. Before contributing a major feature, it's a good idea to mention it on the mailing list first, to discuss the architecture and set expectations for the schedule.

Thanks for your feedback and quick response.

I will include your feedback and create two PRs. One PR for PKCS7 parser feature and another for bug fix.

The bug fix PR will have following two fixes:
69252ab - mbedtls: Disables type-limit error. (After fixing the issue)
287d6d0 - mbedtls: x509: do not include time.h without MBEDTLS_HAVE_TIME

As per mailing list discussion, do you mean to post the patches in the mailing-list or a proposal mail for new feature referring this PR ?

Thanks & Regards,
- Nayna

[danh-arm]

As per mailing list discussion, do you mean to post the patches in the mailing-list or a proposal mail for new feature referring this PR ?

I think Gilles meant in future it's better to discuss big features on the mailing list before creating a PR to avoid wasting your time. Now you've already created the PR, that's not necessary but we might not have the bandwidth to review this properly for a while.

Hopefully the bug fix PR should be easier to absorb quickly.

[daverodgman]

LGTM. I think the test dependency issues @gilles-peskine-arm identified are still outstanding so maybe these could be addressed, but I'm happy either way.

[bensze01]

a17d038 Was needed to resolve another minor merge conflict in all.sh

[bensze01]

Everything looks good to me.
I've merged development once again, to resolve another all.sh conflict and added a Changelog for the feature.

[daverodgman]

Internal CI failure looks spurious, I'll re-run it

[bensze01]

CI Looks good. Merged.

[daverodgman]

Many thanks for your contributions and persistence @naynajain @nick-child-ibm. We're keen to pick up the generator PR #3970 as well - do you think you will have time to update it now that this part has been merged?

[nick-child-ibm]

Hey @daverodgman and others. Thanks for your persistence and immense help getting this merged.
We are still interested in getting #3970 merged. Apologies for the delays, been tied up with other work but I will try to make time for an update later this week. Please let me know if this is not acceptable or if there are any deadlines on your end.
Thanks again!