Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added pkcs7 support (this PR was split, do not merge) #1598

Draft
wants to merge 128 commits into
base: master
Choose a base branch
from

Conversation

bkstein
Copy link
Contributor

@bkstein bkstein commented Feb 4, 2022

This branch extends the wrapper mainly for PKCS7 applications. I use it for:

  • creation of a CSR with a challenge password attribute
  • creation of a signed PKCS7 containing an enveloped PKCS7 with an encrypted CSR
  • extraction of certificates from a PKCS7

As this is my first contribution to rust-openssl, I'd appreciate a reviewer's opinion for this:

  • I understand, the PKCS7 struct should be opaque, but I had to expose the inner structure of openssl's PKCS7 to the wrapper. There is no other way to get access to the certificates in a PKCS7. openssl's api misses something like PKCS7_get0_certificates(). All I found was the implementation of openssl pkcs -in p7.pem -print-certs, which also reads the inner fields: certs = p7->d.sign->cert;

@bkstein bkstein changed the title Kletterstein/pkcs7 Added pkcs7 support Feb 4, 2022
@bkstein bkstein marked this pull request as ready for review February 7, 2022 13:41
@Skepfyr
Copy link
Collaborator

Skepfyr commented Jan 4, 2023

Hi, this PR is very large so reviewing and merging it will take a pretty large time commitment from one of us. I may be able to look at it at some point but I can't make any promises. Also, it's marked as a draft and has some git conflict markers in it, which makes me very hesitant to review. Is this ready (barring the merge conflicts)?

@bkstein
Copy link
Contributor Author

bkstein commented Jan 9, 2023

The only reason for the Draft marker is a race condition in OpenSSL, which is encountered by a test (asn1_type_type_compatibility). See #1730. I still want to analyze the problem. But note, that the code changes in rust-openssl are not affected. It's only some preparation code for the test, that crashes in multithreaded environments. This could also be a flaw in OpenSSL (actually, I assume it is).
In short: if I can see a realistic chance, that my PR might be reviewed and possibly be merged in the near future :), I offer to
analyze that race condition to make sure, we have no skeletons in the closet.
I will also resolve the merge conflicts (I do this anyway from time to time).
Would it help, if I separated the PR into multiple smaller PRs?

@Skepfyr
Copy link
Collaborator

Skepfyr commented Jan 10, 2023

Splitting it up into smaller PRs would be helpful. Obviously investigating that flaky test would be useful, but don't feel obligated.

@bkstein
Copy link
Contributor Author

bkstein commented Jan 12, 2023

@Skepfyr I will try to split the PR, but I'm not sure, if I will be able to detangle this without dropping something. My plan is to make 4 PRs:

  1. a small one with X509Purpose stuff, see Issues with current implementation of X509_PURPOSE constants #1782.
  2. one with the openssl-sys changes
  3. one for X509 certificates and other basics
  4. one for PKCS7 related changes
    Please note, that the changes will depend on each other. 4 on 3 on 2 on 1.

@Skepfyr
Copy link
Collaborator

Skepfyr commented Jan 13, 2023

That sounds reasonable. It's probably sensible to leave this PR up so that we can check it's all there once everything's merged.

@bkstein
Copy link
Contributor Author

bkstein commented Jan 13, 2023

@Skepfyr I completed the split. I think it's best to review the PR's in order:

@bkstein bkstein changed the title Added pkcs7 support Added pkcs7 support (this PR was split, do not merge) Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants