Skip to content

Add PKCS#12#5625

Open
dmazzella wants to merge 1 commit into
randombit:masterfrom
dmazzella:pkcs#12
Open

Add PKCS#12#5625
dmazzella wants to merge 1 commit into
randombit:masterfrom
dmazzella:pkcs#12

Conversation

@dmazzella

Copy link
Copy Markdown
Contributor

Add PKCS#12/PFX Parser and Generator Support

Summary

This PR adds comprehensive PKCS#12 (PFX) support to Botan, enabling parsing and generation of the widely-used certificate/key.

Motivation

Currently, Botan users must rely on external tools or libraries to handle PKCS#12 files. This PR provides native support and solve #1449

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds first-class PKCS#12/PFX support to Botan, addressing the lack of native parsing/generation for certificate + private key bundles (Issue #1449) and integrating it into the library API, CLI, and test suite.

Changes:

  • Introduces a new public Botan::PKCS12 API with PKCS12_Options/PKCS12_Data plus DER parsing and PFX generation.
  • Adds PKCS#12 PBE/PBES2 encryption/decryption support used by the PKCS#12 implementation.
  • Adds CLI commands (pkcs12_export, pkcs12_import), documentation, and extensive unit + CLI tests (including fixture-based parsing tests).

Reviewed changes

Copilot reviewed 11 out of 44 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/lib/pkcs12/pkcs12.h Public API for PKCS#12 options, parsed output container, and parse/create entry points
src/lib/pkcs12/pkcs12.cpp Core PKCS#12 parsing (AuthSafe/SafeBags/MAC) and PFX generation logic
src/lib/pkcs12/info.txt Module definition for PKCS#12 feature gating and dependencies
src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.h Internal API for PKCS#12 PBE/PBES2 encrypt/decrypt helpers
src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.cpp Implementation of legacy PKCS#12 PBE and PBES2 integration
src/lib/pkcs12/pkcs12_pbe/info.txt Module definition for PKCS#12 PBE internals
src/cli/pkcs12.cpp New CLI commands for exporting/importing PFX files
src/tests/test_pkcs12.cpp New unit tests for PKCS#12 generation/parsing and fixture compatibility
src/scripts/test_cli.py Adds end-to-end CLI coverage for pkcs12_export/pkcs12_import
doc/api_ref/pkcs12.rst New API reference documentation for PKCS#12
doc/api_ref/contents.rst Adds PKCS#12 to the API reference index

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread doc/api_ref/pkcs12.rst Outdated
Comment thread src/cli/pkcs12.cpp

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@dmazzella
dmazzella requested a review from Copilot May 27, 2026 11:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@dmazzella
dmazzella requested a review from Copilot May 27, 2026 11:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 6 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread doc/api_ref/pkcs12.rst Outdated
Comment on lines +34 to +44
PKCS12_PBE_Params pkcs12_pbe_params_for_oid(const OID& oid) {
const OID sha1_3des = OID::from_string("PBE-SHA1-3DES");
if(oid == sha1_3des) {
return {sha1_3des, "TripleDES/CBC", 24};
}
const OID sha1_2des = OID::from_string("PBE-SHA1-2DES");
if(oid == sha1_2des) {
return {sha1_2des, "TripleDES/CBC", 16};
}
throw Decoding_Error(fmt("Unsupported PKCS#12 PBE algorithm: {}", oid.to_string()));
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change (also for functions below): the map from OID string to OID value is very fast now (since #4896) so I'd rather just see if(oid == OID::from_string("..."))

Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/scripts/test_cli.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@dmazzella
dmazzella requested a review from Copilot May 27, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@dmazzella
dmazzella requested a review from Copilot May 27, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 7 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread doc/api_ref/pkcs12.rst Outdated
Comment thread src/scripts/test_cli.py Outdated
Comment thread src/scripts/test_cli.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 6 comments.

Comment thread src/scripts/test_cli.py Outdated
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.cpp
Comment thread src/lib/pkcs12/pkcs12_pbe/info.txt Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/cli/pkcs12.cpp Outdated
Comment thread src/cli/pkcs12.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

src/tests/test_pkcs12.cpp:1

  • This test file uses Botan::X509_Time(...) later, but doesn’t include the most direct header for that type (botan/asn1_time.h). Relying on transitive includes can break with unrelated header changes; adding the explicit include would make the test more stable.

Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.cpp
Comment thread src/scripts/test_cli.py Outdated
Comment thread src/lib/pkcs12/pkcs12_pbe/info.txt Outdated
@dmazzella
dmazzella requested a review from Copilot May 27, 2026 14:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 5 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/scripts/test_cli.py
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.h Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@dmazzella
dmazzella requested a review from Copilot May 27, 2026 14:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 4 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.cpp
@dmazzella
dmazzella force-pushed the pkcs#12 branch 2 times, most recently from c13844d to f388c19 Compare May 27, 2026 15:16
@dmazzella
dmazzella requested a review from Copilot May 27, 2026 15:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 4 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/scripts/test_cli.py Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12.cpp

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 3 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.cpp
Comment thread src/scripts/test_cli.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 44 changed files in this pull request and generated 7 comments.

Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/scripts/test_cli.py
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.h Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp
Comment thread src/lib/pkcs12/pkcs12_pbe/pkcs12_pbe.cpp
Comment thread src/cli/pkcs12.cpp Outdated
@dmazzella
dmazzella force-pushed the pkcs#12 branch 2 times, most recently from 12f1f05 to 21d66b4 Compare May 27, 2026 17:58

@randombit randombit left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This was just a review of pkcs12.h on the general shape of the public API, haven't looked at the implementation yet at all.

Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
Comment thread src/lib/pkcs12/pkcs12.h Outdated
@dmazzella

Copy link
Copy Markdown
Contributor Author

Thanks. This was just a review of pkcs12.h on the general shape of the public API, haven't looked at the implementation yet at all.

All changes done

@dmazzella
dmazzella force-pushed the pkcs#12 branch 2 times, most recently from 554f12d to 6ee01df Compare June 5, 2026 11:32
Comment thread doc/api_ref/pkcs12.rst Outdated
@dmazzella
dmazzella force-pushed the pkcs#12 branch 2 times, most recently from 2a8468e to 81cadcd Compare June 30, 2026 14:02

@randombit randombit left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks overall looking really good. My big criticism would be in the tests where there are several taking ~20-50 lines to construct some kind of invalid p12 file, and then testing how parsing reacts to it. Tests of invalid/degenerate inputs are great! But the test input should just be pre-generated and stored in src/tests/data/pkcs12 with the rest, and then the test code is just a read+parse.

Also various minor nits. I see Copilot suggested caching OID values as static, not unreasonable but we've made lookups sufficiently fast now that I think it's not worth the cognitive complexity of doing anything further. I left comments on some but not all of the spots, grepping static const OID should find them all.

Comment thread doc/api_ref/pkcs12.rst Outdated
Comment thread src/tests/test_pkcs12.cpp Outdated
Comment thread src/cli/pkcs12.cpp Outdated
Comment thread src/examples/pkcs12_export_chain.cpp Outdated
Comment thread src/examples/pkcs12_export_chain.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/lib/pkcs12/pkcs12.cpp Outdated
Comment thread src/tests/test_pkcs12.cpp Outdated
Comment thread src/tests/test_pkcs12.cpp Outdated
Comment thread src/tests/test_pkcs12.cpp Outdated
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.

4 participants