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

Add API to create PKCS#12 #486

Merged
merged 7 commits into from
Oct 29, 2024
Merged

Add API to create PKCS#12 #486

merged 7 commits into from
Oct 29, 2024

Conversation

gjcairo
Copy link
Contributor

@gjcairo gjcairo commented Oct 24, 2024

Motivation

It would be handy to provide an API to create PKCS#12 files from a list of NIOSSLCertificates and a NIOSSLPrivateKey.
This would be particularly useful when dealing with Network.framework/NIOTransportServices/Security.framework, which use SecIdentitys for SSL. Two particular use cases are #484 (comment) and grpc-swift-nio-transport, which would use this API for testing the NIOTS transport implementation.

Modifications

This PR adds a static method to NIOSSLPKCS12Bundle that creates a PKCS#12 file from the given array of certificates + private key, and returns it as an array of bytes.

Result

PKCS#12 files can be created using NIOSSL.

@gjcairo gjcairo requested a review from Lukasa October 24, 2024 09:00
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Oct 24, 2024
certificates: [NIOSSLCertificate],
privateKey: NIOSSLPrivateKey,
passphrase: Bytes,
name: Bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to provide a name, and I'd argue we shouldn't bother. If we're going to enable this, it should be optional, and needs a different generic type parameter to the passphrase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just get rid of it, don't think it serves any purpose really.

Sources/NIOSSL/SSLPKCS12Bundle.swift Show resolved Hide resolved
let certificateChainStack = CNIOBoringSSL_sk_X509_new(nil)
for additionalCertificate in certificates.dropFirst() {
let result = additionalCertificate.withUnsafeMutableX509Pointer { certificate in
CNIOBoringSSL_sk_X509_push(certificateChainStack, certificate)
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets the refcount wrong. We need to increase the refcount of the certificate pointer here, using X509_up_ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the joys of manual memory management.
Should we decrease the refcount after we've created the PKCS12?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we're also missing the free of the sk_509. Done correctly, the free call is sk_X509_pop_free with the second argument as X509_free.

Sources/NIOSSL/SSLPKCS12Bundle.swift Show resolved Hide resolved
@gjcairo gjcairo requested a review from Lukasa October 28, 2024 21:30
}
}

defer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: as a matter of coding style it's a bit nicer to move this immediately after the sk_X509_new.

@gjcairo gjcairo requested a review from Lukasa October 29, 2024 10:02
let pkcs12 = try bundle.serialize(passphrase: "thisisagreatpassword".utf8)

// And then decode it into a NIOSSLPKCS12Bundle
let decoded = try NIOSSLPKCS12Bundle(buffer: pkcs12, passphrase: "thisisagreatpassword".utf8)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably try some non-matching passphrases.

@gjcairo gjcairo requested a review from Lukasa October 29, 2024 11:15
@gjcairo gjcairo enabled auto-merge (squash) October 29, 2024 14:19
@gjcairo gjcairo merged commit c7e9542 into apple:main Oct 29, 2024
43 checks passed
@gjcairo gjcairo deleted the make-pkcs12 branch October 29, 2024 15:01
@Lukasa Lukasa mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants