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 support for PKCS#12 bundles. #23

Merged
merged 1 commit into from Jul 12, 2018

Conversation

Projects
None yet
3 participants
@Lukasa
Copy link
Contributor

commented Jul 10, 2018

Resolves #17.

Motivation:

As requested in #17, it's not uncommon to want to support PKCS#12 bundles.
This patch adds support for unpacking PKCS#12 bundles into useful data types
for NIOOpenSSL.

Modifications:

Added new OpenSSLPKCS12Bundle type.

Result:

PKCS#12 bundles can be used to distribute certs/keys for SwiftNIO.

/cc @pushkarnk

@Lukasa Lukasa added this to the 1.2.0 milestone Jul 10, 2018

@Lukasa Lukasa requested a review from weissi Jul 10, 2018

@Lukasa Lukasa force-pushed the Lukasa:cb-pkcs12-support branch 7 times, most recently from faac900 to 175b997 Jul 10, 2018

@weissi
Copy link
Member

left a comment

looks pretty good except for some scary lifetime stuff that might well be correct 🙃

///
/// If you have a PKCS12 bundle, you configure a `TLSConfiguration` like this:
///
/// let p12Bundle = OpenSSLPKCS12Bundle(file: pathToMyP12)

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

I think there's one space missing here, right? Needs 5 in total, one to separate it from /// and then four for code indent I think

}

if mlock(bufferPtr.baseAddress!, bufferPtr.count) != 0 {
throw IOError(errnoCode: errno, function: "mlock")

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

this isn't guaranteed to give you the right errno. Can we move into the Posix module?

This comment has been minimized.

Copy link
@Lukasa

Lukasa Jul 10, 2018

Author Contributor

No, Posix is internal to NIO. Why isn't it guaranteed to give me the right errno?

This comment has been minimized.

Copy link
@weissi

weissi Jul 11, 2018

Member

ok, as of our chat a minute ago: this should then have its own Posix wrapper following NIO's pattern

This comment has been minimized.

Copy link
@Lukasa

Lukasa Jul 12, 2018

Author Contributor

I agree, but are you open to me doing this as a follow-up PR? There are a few other sys calls in the package, I'd like to fix it all in one go.

This comment has been minimized.

Copy link
@weissi
// We need to allocate some memory and prevent it being swapped to disk while we use it.
// For that reason we use mlock.
// Note that here we use UnsafePointer and UnsafeBufferPointer. Ideally we'd just use
// the buffer pointer, but 4.0 doesn't have the APIs we want: specifically, .allocate

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

I think we created an extension for that, no?

This comment has been minimized.

Copy link
@Lukasa

Lukasa Jul 10, 2018

Author Contributor

No, just for deallocate without size, which in fact I use. ;)

defer {
// If munlock fails take out the process.
let rc = munlock(bufferPtr.baseAddress!, bufferPtr.count)
precondition(rc == 0, "munlock failed with \(rc), errno \(errno)")

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

also should go to Posix

This comment has been minimized.

Copy link
@Lukasa

Lukasa Jul 10, 2018

Author Contributor

Same note.

This comment has been minimized.

Copy link
@weissi

weissi Jul 11, 2018

Member

here too, because as you pointed our: EINTR could be a real issue here ...

let certStackSize = caCerts.map(CNIOOpenSSL_sk_X509_num) ?? 0
var certs = [OpenSSLCertificate]()
certs.reserveCapacity(Int(certStackSize) + 1)
certs.append(OpenSSLCertificate.fromUnsafePointer(pointer: actualCert))

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

I'm assuming the fromUnsafePointer(pointer: ...) functions are consuming a reference? If yes, could we file an issue to rewrite them to something that doesn't consume the reference and copies the contents instead. If OpenSSL doesn't have copying functions for that then the label needs to become better and needs to read fromUnsafePointer(transferringOwnership: actualCert) etc...

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

and we need a comment at each invocation site that they're non-standard and just take ownership of the passed pointer.

if let p12 = p12 {
try self.init(ref: p12, passphrase: passphrase)
} else {
throw OpenSSLError.unknownError(.init())

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

what's the .init() in there doing? Could we make it ActualType() ?

/// - parameters:
/// - file: The path to the PKCS#12 bundle on disk.
public init(file: String) throws {
try self.init(file: file, passphrase: nil as [UInt8]?)

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

not sure but I prefer Optional<[UInt8]>.none or [UInt8]?.none instead of nil as [UInt8]? but feel free to ignore

// Note that here we use UnsafePointer and UnsafeBufferPointer. Ideally we'd just use
// the buffer pointer, but 4.0 doesn't have the APIs we want: specifically, .allocate
// and .withMemoryRebound(to:) are only added to the buffer pointers in 4.1.
let bufferSize = Int(self.count) + 1

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

why does it allocate one larger and put a NULL in there? If that's necessary it should read withSecureCString or something maybe?

This comment has been minimized.

Copy link
@Lukasa

Lukasa Jul 10, 2018

Author Contributor

Ah, sorry, I see what you mean. Yeah, that's a better name.

let ptr = UnsafeMutablePointer<UInt8>.allocate(capacity: bufferSize)
let bufferPtr = UnsafeMutableBufferPointer(start: ptr, count: bufferSize)
defer {
ptr.deallocate()

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

the memory needs to be deinitialized too before deallocate

assert(nextIndex == (bufferPtr.endIndex - 1))

// Add a null terminator.
bufferPtr[nextIndex] = 0

This comment has been minimized.

Copy link
@weissi

weissi Jul 10, 2018

Member

I think this is technically wrong as you're writing to unintialised memory here. I think it should be bufferPtr.advanced(by: nextIndex).initialize(to: 0).

I realise that UInt8 is a primitive type that doesn't have any ref counts but imagine you'd have saved a type that needs ref counts. In that case this would non-deterministically crash. Because bufferPtr[nextIndex] = newValue would first read out the old value, decrement the ref count, the increment the ref count of newValue and then write it in there. initialize on the other hand doesn't touch the ref count of the memory but just increases newValue's ref count and then writes it into the memory (which then becomes initialised).

Yes, UInt8 doesn't have a ref count of course but still the same rules apply. Memory needs to be 1) allocated 2) initialised 3) written as often as the user pleases 4) deinitialised 5) deallocated

You're doing (1), most of (2) (except for the last byte), (3) and (5). So parts of (1) and (4) are missing

@Lukasa Lukasa force-pushed the Lukasa:cb-pkcs12-support branch 3 times, most recently from d04ed7d to d0acbc6 Jul 10, 2018

public init<Bytes: Collection>(file: String, passphrase: Bytes?) throws where Bytes.Element == UInt8 {
guard openSSLIsInitialized else { fatalError("Failed to initialize OpenSSL") }

let fileObject = fopen(file, "rb")

This comment has been minimized.

Copy link
@weissi

weissi Jul 11, 2018

Member

please handle NULL as at least fclose blows up on NULL

This comment has been minimized.

Copy link
@weissi

weissi Jul 11, 2018

Member

also: please add test case for /dev/null/does/not/exist or so

@Lukasa Lukasa force-pushed the Lukasa:cb-pkcs12-support branch from d0acbc6 to f8f55f6 Jul 12, 2018

@weissi

weissi approved these changes Jul 12, 2018

Copy link
Member

left a comment

cool, looks great. Follow-up for better syscall handling needed as we chatted about

Add support for PKCS#12 bundles.
Motivation:

As requested in #17, it's not uncommon to want to support PKCS#12 bundles.
This patch adds support for unpacking PKCS#12 bundles into useful data types
for NIOOpenSSL.

Modifications:

Added new OpenSSLPKCS12Bundle type.
Added new fromUnsafePointer(takingOwnership:) static functions to
    OpenSSLCertificate and OpenSSLPrivateKey.
Deprecated fromUnsafePointer(pointer:) on OpenSSLCertificate and
    OpenSSLPrivateKey.

Result:

PKCS#12 bundles can be used to distribute certs/keys for SwiftNIO.

@Lukasa Lukasa force-pushed the Lukasa:cb-pkcs12-support branch from f8f55f6 to 1cdb59a Jul 12, 2018

@weissi weissi merged commit c5e345b into apple:master Jul 12, 2018

1 check passed

pull request validation Build finished.
Details

@Lukasa Lukasa deleted the Lukasa:cb-pkcs12-support branch Jul 12, 2018

@pushkarnk

This comment has been minimized.

Copy link

commented Jul 16, 2018

@Lukasa Thanks a lot for your work on this! Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.