wbond / asn1crypto

Python ASN.1 library with a focus on performance and a pythonic API
MIT License
335 stars 140 forks source link

Add support for rfc2633 sMIME capabilities signed attr #215

Closed Hellzed closed 2 years ago

Hellzed commented 3 years ago

Hi, I'm using asn1crypto on a project that requires support for S/MIME Version 3 (rfc2633), especially the sMIME capabilities signed attribute. Right now, asn1crypto master only parses the sMIME capabilities signed attr into universal types (a set of sequences) with raw OIDs.

This also exposes some (probably buggy) behaviour from asn1crypto, where unknown signed attrs such as sMIME capabilities are empty and hoisted at the beginning of the signed attrs field when dumping. For an example, see this PKCS#7 signature before and after being parsed by asn1crypto (look for the sMIMECapabilities field and see how it was 1) moved, 2) emptied).

This pull request adds support for rfc2633 sMIME capabilities signed attr, to allow parsing it as a set of sequences of algorithm identifiers (as per the spec). This prevents the faulty parsing and dumping behaviour, only for this signed attr.

I need to add a couple of unit tests, could you provide guidance about this? I think new fixtures need to be generated for the CMS tests, but I'm not sure what's the most appropriate way.

Thank you for this incredibly useful library.

wbond commented 2 years ago

I fixed the bug you identified in 5a24aed1b99e7f51ba38183aebbcce86e6eb4d23. It was that if you try and force DER re-encoding of a Sequence that has no field info, it resulted in an empty value since it didn't known the canonical encoding.

Now if it hits such a situation, it preserves the existing encoding.

wbond commented 2 years ago

In terms of tests, ideally you can just add a fixture with a value encoded by another program and ensure asn1crypto can parse it.

Hellzed commented 2 years ago

I've added a sMIME capabilities parsing test on a PKCS#7 signature generated by Thunderbird 91.5.0.

I have also updated the cms module to better reflect sMIME capabilities semantics as per rfc2633:

A nice extra would have been to provide the algorithm name as capability_id attribute (when using SMIMECapabilityIdentifier.native) if the OID matches a known algorithm, but this doesn't seem actually useful for now.

wbond commented 2 years ago

This looks good - thanks! I have a commit to add the algorithm names, but for some reason GitHub isn't letting me push to your master branch even though the PR says I can. I'll add it after I merge.

wbond commented 2 years ago

You can see my tweaks at 4791c1edac3dda76d9a718f2a35347f10ba35028

Hellzed commented 2 years ago

Awesome, thank you!