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

Update iOS available version to 10.3 #48

Merged
merged 4 commits into from
Jun 28, 2019
Merged

Update iOS available version to 10.3 #48

merged 4 commits into from
Jun 28, 2019

Conversation

Andrew-Lees11
Copy link
Contributor

Description

A user was trying to build Swfit-JWT using Carthage and it wouldn't compile.
They raised issue #64 which shows the compile error that:

'SecCertificateCopyPublicKey' is only available on iOS 10.3 or newer

This fix updates the @available tag to be:

@available(macOS 10.12, iOS 10.3, *)

Which allows the project to compile.

How Has This Been Tested?

The project was compiled and tested using the iOS simulator.

@Andrew-Lees11 Andrew-Lees11 assigned billabt and unassigned billabt Jun 26, 2019
@Andrew-Lees11 Andrew-Lees11 requested a review from billabt June 26, 2019 12:49
@billabt
Copy link
Collaborator

billabt commented Jun 26, 2019

The iOS 10.3 restriction only applies to that function, SecCertificateCopyPublicKey. Turns out that function is now deprecated.

A better solution is to wrap the function as follows replacing the code in the function createPublicKey in the CryptorRSAKey.swift source file with this:

	///
	/// Creates a public key by extracting it from certificate data.
	///
	/// - Parameters:
	/// 	- data:				`Data` representing the certificate.
	///
	/// - Returns:				New `PublicKey` instance.
	///
	@available(iOS 10.3, *)
	internal class func createPublicKey(data: Data) throws -> PublicKey {
		
		#if os(Linux)
			let certbio = BIO_new(BIO_s_mem())
			defer {
				BIO_free(certbio)
			}
		
			// Move the key data to BIO
			try data.withUnsafeBytes() { (buffer: UnsafeRawBufferPointer) in
				
				let len = BIO_write(certbio, buffer.baseAddress?.assumingMemoryBound(to: UInt8.self), Int32(data.count))
				guard len != 0 else {
					let source = "Couldn't create BIO reference from key data"
					if let reason = CryptorRSA.getLastError(source: source) {
						
						throw Error(code: ERR_ADD_KEY, reason: reason)
					}
					throw Error(code: ERR_ADD_KEY, reason: source + ": No OpenSSL error reported.")
				}
				
				// The below is equivalent of BIO_flush...
				BIO_ctrl(certbio, BIO_CTRL_FLUSH, 0, nil)
			}
			let cert = d2i_X509_bio(certbio, nil)
			if cert == nil {
				print("Error loading cert into memory\n")
				throw Error(code: ERR_CREATE_CERT_FAILED, reason: "Error loading cert into memory.")
			}
		
			// Extract the certificate's public key data.
			let evp_key: OpaquePointer? = .init(X509_get_pubkey(cert))
			if evp_key == nil {
				throw Error(code: ERR_CREATE_CERT_FAILED, reason: "Error getting public key from certificate")
			}
		
			return PublicKey(with: evp_key)
	
		#else
		
			// Create a DER-encoded X.509 certificate object from the DER data...
			let certificateData = SecCertificateCreateWithData(nil, data as CFData)
			guard let certData = certificateData else {
				
				throw Error(code: ERR_CREATE_CERT_FAILED, reason: "Unable to create certificate from certificate data.")
			}
		
			var key: SecKey? = nil
		
			if #available(macOS 10.14, iOS 12.0, *) {
				
				key = SecCertificateCopyKey(certData)
				if key == nil {
					
					throw Error(code: ERR_EXTRACT_PUBLIC_KEY_FAILED, reason: "Unable to extract public key from data.")
				}

			} else {
				
				#if os(macOS)
			
					// Now extract the public key from it...
					let status: OSStatus = withUnsafeMutablePointer(to: &key) { ptr in
						
						// Retrieves the public key from a certificate...
						SecCertificateCopyPublicKey(certData, UnsafeMutablePointer(ptr))
					}
					if status != errSecSuccess || key == nil {
						
						throw Error(code: ERR_EXTRACT_PUBLIC_KEY_FAILED, reason: "Unable to extract public key from data.")
					}
			
				#else
			
					key = SecCertificateCopyPublicKey(certData)
						
				#endif
			}
		
			return PublicKey(with: key!)
		
		#endif		
	}

This fixes the deprecation issue and well as the problem you're trying to solve. Also, changing that top level availability changes the public API for all the functions in this extension. Not something we want do in a minor point release.

I've got the code ready to go here, building an' passing my tests. Can you verify it on your simulator project and let me know? Then I'll check in the code I have and put a release out.

@Andrew-Lees11
Copy link
Contributor Author

Hey @billabt
I added your check for iOS 12 to use SecCertificateCopyKey.

Unfortunately you can't just wrap the function since everything that uses that function also needs to be @available. This results in the classes I have set as being iOS 10.3

This does not a change to the public API since on Cocoapods we already have a minimum iOS version of 10.3 and on Carthage it is currently not compiling (On any iOS) so we cannot break what is already broken.

I also added a guard statement instead of a force unwrap so we don't crash if SecCertificateCopyPublicKey fails.

With these changes it is still compiling and passing the tests.

@Andrew-Lees11
Copy link
Contributor Author

I have also added tvOS and watchOS so that they will compile and add an #if check for SecCertificateCopyKey.

Copy link
Collaborator

@billabt billabt left a comment

Choose a reason for hiding this comment

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

Changes requested, depending on answers to my questions...

Sources/CryptorRSA/CryptorRSAKey.swift Show resolved Hide resolved
Sources/CryptorRSA/CryptorRSAKey.swift Show resolved Hide resolved
Sources/CryptorRSA/CryptorRSAKey.swift Show resolved Hide resolved
@billabt billabt merged commit 97485e4 into master Jun 28, 2019
@billabt billabt deleted the ios10.3 branch June 28, 2019 19:06
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.

2 participants