fullsailor / pkcs7

Implements a subset of PKCS#7/Crytpographic Message Syntax (rfc2315, rfc5652)
MIT License
123 stars 201 forks source link

correctly marshal degenerate certificates #7

Closed groob closed 8 years ago

groob commented 8 years ago

The marshalCertificates function introduced in https://github.com/fullsailor/pkcs7/commit/2585af45975b11f1d7502bb6c01556c29efb54ce#diff-f6d905a9fe95414d2307bb69be1c110eR653 addresses the parsing issue introduced in #4, but caused DegenerateCertificate to not marshal the data correctly.

Additionally, some cases, like SCEP can require that a whole certificate chain to be marshaled as a degenerate SignedData type. https://tools.ietf.org/html/draft-gutmann-scep-02#section-5.2.1.1.2

This PR addresses both issues.

fullsailor commented 8 years ago

Hi @groob, thanks for your discovery and detailed PR. I'm disappointed, that the tests didn't catch this regression.

I'm hesitant to break compatibility with other consumers. We should fix this with the old method signature (DegenerateCertificate(cert []byte) ([]byte, error)) as well as adding the new signature to support encoding more than a single certificate.

What do you think about these changes?

I think with this change we technically don't need the new multiple cert function, but its a good addition I'm happy to keep. Let me know if I can help in any way.

groob commented 8 years ago

I made the changes as you suggested. I also removed the new DegenerateCertificates function and made a note in the DegenerateCertificate function comment that a certificate chain can be used in place of a single cert.

I also added a test helper that can be used to check the pkcs7 output against openssl pkcs7. It should catch a future regression.

I noticed that asn1.Marshal and asn1.Unmarshal error returns are ignored several times throughout the library. I would like to address the cases where errors are not checked/returned in a separate PR. Let me know what you think.

fullsailor commented 8 years ago

Looks great. Testing against openssl is a clever idea.