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

Address a bunch of Issue, and fix a missing asn1:optional flag, add a Config for setting options, and Marshal/Unmarshal #39

Closed
wants to merge 24 commits into from

Conversation

pschou
Copy link

@pschou pschou commented Sep 14, 2022

Going through the pkcs12 implementation here made me consider that this was missing some things, such as getting the friendly name from a pkcs12 / trust store file. Created a set of Marshal/Unmarshal functions that provide/take algorithm configurations, they provide either a byte (for encoding) or a struct (for decoding) the pkcs12 structure.

The Algorithms are added to the Unmarshal output so the structure can be written back to a file via Marshalling into the same format.

@pschou pschou changed the title fix a missing asn1 optional flag fix a missing asn1 optional flag and add a _WithConfig Sep 15, 2022
@pschou pschou changed the title fix a missing asn1 optional flag and add a _WithConfig WIP: fix a missing asn1 optional flag and add a _WithConfig Sep 16, 2022
@pschou pschou changed the title WIP: fix a missing asn1 optional flag and add a _WithConfig Address a bunch of Issue, and fix a missing asn1:optional flag, add a Config for setting options, and Marshal/Unmarshal Sep 16, 2022
@pschou
Copy link
Author

pschou commented Oct 3, 2022

@AGWA Can you take a look at this and let me know what you think?

@vineet-garg
Copy link

vineet-garg commented Apr 3, 2023

Is this PR going to get in? I need support for pbeWithSHAAnd128BitRC2-CBC which this PR provides.
I created an issue for that: #43

@dmikusa
Copy link

dmikusa commented May 2, 2023

Same. This has some good fixes, can we get this merged & in a release? 🥺

@7ing
Copy link

7ing commented Jul 13, 2023

Bump it up, looking forward any movement on this PR.

@AGWA
Copy link
Member

AGWA commented Jul 13, 2023

Thanks @pschou for your PR. Unfortunately, this PR is extremely large and changes a lot of security-sensitive code and API surface and I just don't have the time to review it.

As I understand it, the most urgent features that people need are:

  • Encoding passwordless truststores (in the same format that Java 18 uses)
  • Encoding with modern algorithms (PBES2 with PBKDF2 and AES-256-CBC; the same algorithms that OpenSSL 3 uses by default)

I've begun working on a minimal, conservative change that adds the above features. I expect this to take much less time than reviewing this PR would.

CC @pivotal-david-osullivan @Tookmund

@AGWA AGWA added the feature label Jul 15, 2023
@AGWA
Copy link
Member

AGWA commented Jul 15, 2023

Closing in favor of #48. In the future, please consider:

  • Filing an issue for feedback before embarking on major rewrites, feature additions, or API changes.
  • Keeping PRs as short as possible, and splitting unrelated changes into different PRs.

(This is good advice for all open source projects, not just this one.)

@AGWA AGWA closed this Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants