Align cryptography.hazmat.primitives.serialization.pkcs7.serialize_certificates ASN.1 structure to openssl crl2pkcs7 -nocrl -certfile ...

[gesslerpd]

This change makes the pkcs7 output more agreeable with certain tools that expect similar ASN.1 structure to openssl outputs.

[alex]

Can you explain the motivation for this? I believe we wrote this to match our previous behavior, when we used OpenSSL. Is it possible different OpenSSL versions have different behavior?

FWIW the RFC indicates:

        3.   In the degenerate case where there are no signers
             on the content, the ContentInfo value being "signed" is
             irrelevant. It is recommended in that case that the content
             type of the ContentInfo value being "signed" be data, and
             the content field of the ContentInfo value be omitted.

So it seems that None may be preferred, but consumers are expected to allow either.

[gesslerpd]

@alex Yes I agree that both ASN.1 structures should be supported by consumers, I am dealing with a case in which that is not the case and a client of EST test server is rejecting pkcs7 data with this structure. I noticed that openssl crl2pkcs7 -nocrl -certfile ... always has None so thought I'd put a change here to align with the CLI since those are commonly used. I can fix the tests if you think the change is worth it, otherwise we can close this. Thanks for the feedback!

[alex]

The fact that we have existing tests that make assertions about the exact PKCS7 structure indicates to me that a) OpenSSL used to emit the same ASN.1 structure we do (since our PKCS7 impl used to use OpenSSL), so I'd like to understand when that changed, or if the CLI and C API are inconsistent, b) that there may be a compatibility hazard in making this change.

The RFC indicates that None is the preferred form, so I have a preference for that, but we need to understand what the implications are.

[gesslerpd]

@alex Here's what I found so let me know if I should spend time fixing the tests.

a) I believe this function in cryptography is most similar to openssl crl2pkcs7 -nocrl -certfile ... -certfile ... in openssl-land

Running openssl crl2pkcs7 -nocrl | openssl pkcs7 -inform PEM -print shows d.data: <ABSENT>

This is where they set the content_type to "data" in the CLI, leaving everything else as NULL references and the blame indicated this hasn't changed in 26 years https://github.com/openssl/openssl/blob/2f0b4974dfbd9bc71e1164e0742fc7fdb2b2b70e/apps/crl2pkcs7.c#L132

All other PKCS7 files without signer_info from openssl codebase also show this as d.data: <ABSENT>

https://github.com/openssl/openssl/blob/master/test/pkcs7.pem
curl https://raw.githubusercontent.com/openssl/openssl/af82623d32962b3eff5b0f0b0dedec5eb730b231/test/testp7.pem | openssl pkcs7 -inform PEM -print

https://github.com/openssl/openssl/blob/master/test/recipes/90-test_sslapi_data/dhparams.pem
curl https://raw.githubusercontent.com/openssl/openssl/af82623d32962b3eff5b0f0b0dedec5eb730b231/test/recipes/90-test_sslapi_data/dhparams.pem | openssl pkcs7 -inform PEM -print

b) I don't think there'd be a compatibility hazard since consumers are supposed to handle both and this is the preferred structure per RFC and seemingly OpenSSL

Let me know what you think, thanks!

[alex]

Diving into how we used to implement this (see 9da9af7), the reason for this behavior is (as far as I can tell):

• https://github.com/openssl/openssl/blob/master/crypto/pkcs7/pk7_smime.c#L30-L40 creates a PKCS7, sets its type to be signed, and then creates an inner PKCS7 with a type of data
• https://github.com/openssl/openssl/blob/master/crypto/pkcs7/pk7_lib.c#L66 creates that inner PKCS7 of type data
• When a data PKCS7 is created, its data field is always initialized to an empty string: https://github.com/openssl/openssl/blob/master/crypto/pkcs7/pk7_lib.c#L130

And so that's why we had the previous behavior.

As for whether we should change it: it sounds like OpenSSL will happily emit things in either form, so its likely most users will accept things in either form.

I'd like to hear @reaperhulk's thoughts, but prima facie this seems worth doing, which means we'll need to get these tests passing.

[reaperhulk]

I also spelunked the history a bit and have the same conclusions. Seems like it's fine to change this, although I'm sure we'll find some new exciting edge case after the next release 😄

[alex]

The fact that OpenSSL is capable of producing both of them gives me some confidence that clients will support both.

…

On Sat, Jun 22, 2024 at 10:34 AM Paul Kehrer ***@***.***> wrote: I also spelunked the history a bit and have the same conclusions. Seems like it's fine to change this, although I'm sure we'll find some new exciting edge case after the next release 😄 — Reply to this email directly, view it on GitHub <#11123 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBGY6GAJQ5MVFTYOGPDZIWDPXAVCNFSM6AAAAABJSKAR7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBUGA2TKMZVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[gesslerpd]

@alex @reaperhulk Thanks for the extra investigations for the different PKCS7 structure outputs, I have replaced the DER/PEM test vectors with the d.data: <ABSENT> equivalents.

[alex]

Thanks