Client certificate authentication by ShihabMehboob · Pull Request #34 · Kitura/SwiftyRequest

ShihabMehboob · 2018-07-25T08:43:40Z

This PR aims to add client certificate authentication, in relation to this issue: #33 (comment)

ianpartridge · 2018-07-26T13:08:51Z

.swift-version

@@ -1 +1 @@
-4.1.2
+4.1


ianpartridge · 2018-07-26T13:12:34Z

Sources/SwiftyRequest/RestRequest.swift

    // Check if there exists a self-signed certificate and whether it's a secure connection
    private let isSecure: Bool
    private let isSelfSigned: Bool
+    private let certificateName: String


Wouldn't this make more sense as an optional? Also, better named clientCertificateName? Also, comment needed.

ianpartridge · 2018-07-26T13:17:51Z

Sources/SwiftyRequest/RestRequest.swift

+                Log.warning(warning)
+                fallthrough
+            }
+            if let _ = NSData(contentsOfFile: Bundle.main.path(forResource: self.certificateName, ofType: "der") ?? "") {


Why create an NSData then throw it away?

ianpartridge · 2018-07-26T13:19:30Z

Sources/SwiftyRequest/RestRequest.swift

+                if let serverCertificate = SecTrustGetCertificateAtIndex(trust, 0) {
+                    let certificate: SecCertificate = serverCertificate
+                    var identity: SecIdentity? = nil
+                    let _: OSStatus = SecIdentityCreateWithCertificate(nil, certificate, &identity)


Shouldn't we check the result?

ianpartridge · 2018-07-26T13:19:32Z

Sources/SwiftyRequest/RestRequest.swift

+            }
+            if let _ = NSData(contentsOfFile: Bundle.main.path(forResource: self.certificateName, ofType: "der") ?? "") {
+                if let serverCertificate = SecTrustGetCertificateAtIndex(trust, 0) {
+                    let certificate: SecCertificate = serverCertificate


Why do we need to do this?

ianpartridge · 2018-08-06T10:35:15Z

Sources/SwiftyRequest/RestRequest.swift

+            #if !os(Linux)
+            if let certificatePath = self.certificatePath {
+                // Get the bundle path from the Certificates directory for a certificate that matches clientCertificateName's name
+                if let bundle = Bundle.path(forResource: self.clientCertificateName, ofType: "der", inDirectory: certificatePath) {


Please call this path.

ianpartridge · 2018-08-06T10:37:29Z

Sources/SwiftyRequest/RestRequest.swift

+                    // Convert the bundle path to NSData
+                    if let key: NSData = NSData(base64Encoded: bundle, options: .ignoreUnknownCharacters) {
+                        // Create a secure certificate from the NSData
+                        if let certificate: SecCertificate = SecCertificateCreateWithData(kCFAllocatorDefault, key) {


Do you need the : SecCertificate or would the compiler infer it?

ianpartridge · 2018-08-06T10:39:46Z

Sources/SwiftyRequest/RestRequest.swift

+                        if let certificate: SecCertificate = SecCertificateCreateWithData(kCFAllocatorDefault, key) {
+                            // Create a secure identity from the certificate
+                            var identity: SecIdentity? = nil
+                            let _: OSStatus = SecIdentityCreateWithCertificate(nil, certificate, &identity)


https://developer.apple.com/documentation/security/1401160-secidentitycreatewithcertificate says "Call the CFRelease function to release this object when you are finished with it." so I guess we need to do that?

When attempting to use it, it returns the following error: 'CFRelease' is unavailable: Core Foundation objects are automatically memory managed, suggesting that this is handled by itself.

OK cool. Bad docs!

ianpartridge · 2018-08-06T10:40:44Z

Sources/SwiftyRequest/RestRequest.swift

        switch (method, host) {
+        case (NSURLAuthenticationMethodClientCertificate, baseHost):
+            #if !os(Linux)
+            if let certificatePath = self.certificatePath {


Can you guard let in here to reduce the nesting?

ianpartridge · 2018-08-06T11:17:11Z

Sources/SwiftyRequest/RestRequest.swift

        let host = challenge.protectionSpace.host

-        guard let url = URLComponents(string: self.url), let baseHost = url.host else {
+        guard let url = URLComponents(string: self.url), let baseHost = url.host, let certificatePath = self.certificatePath else {


This doesn't look right, we'll never get to the switch if certificatePath is nil.

ianpartridge · 2018-08-06T12:14:40Z

Sources/SwiftyRequest/RestRequest.swift

+            }
+            // Get the bundle path from the Certificates directory for a certificate that matches clientCertificateName's name
+            if let path = Bundle.path(forResource: self.clientCertificateName, ofType: "der", inDirectory: certificatePath) {
+                // Convert the bundle path to NSData


This comment isn't correct, it's not the path that's being converted, it's the contents of the file. Would this be better?

// Read the certificate data from disk

ianpartridge · 2018-08-06T12:14:54Z

Sources/SwiftyRequest/RestRequest.swift

+            // Get the bundle path from the Certificates directory for a certificate that matches clientCertificateName's name
+            if let path = Bundle.path(forResource: self.clientCertificateName, ofType: "der", inDirectory: certificatePath) {
+                // Convert the bundle path to NSData
+                if let key: NSData = NSData(base64Encoded: path, options: .ignoreUnknownCharacters) {


I think the compiler can infer the type.

Do we really want to .ignoreUnknownCharacters? Would it be safer not to?

ianpartridge · 2018-08-06T15:31:36Z

Sources/SwiftyRequest/RestRequest.swift

        switch (method, host) {
+        case (NSURLAuthenticationMethodClientCertificate, baseHost):
+            #if !os(Linux)
+            guard let certificatePath = self.certificatePath else {


I think we should guard let clientCertificateName = self.clientCertificateName too, just in case someone passes us a path but no name?

ianpartridge · 2018-08-07T08:45:27Z

Sources/SwiftyRequest/RestRequest.swift

    /// - Parameters:
    ///   - url: URL string to use for network request
-    public init(method: HTTPMethod = .get, url: String, containsSelfSignedCert: Bool? = false) {
+    public init(method: HTTPMethod = .get, url: String, containsSelfSignedCert: Bool? = false, certificateName: String? = nil, certificatePath: String? = nil) {


Do you think it would be better to define a

public struct ClientCertificate { let name: String let path: String }

and pass one of those in here?

ianpartridge · 2018-08-07T12:20:45Z

Sources/SwiftyRequest/RestRequest.swift


        self.isSecure = url.contains("https")
        self.isSelfSigned = containsSelfSignedCert ?? false
+        self.clientCertificateName = clientCertificate?.name


Why not just store the passed in ClientCertificate?

self.clientCertificate = clientCertificate

Then we can add things to ClientCertificate if we need to, without having to update RestRequest as well.

ianpartridge · 2018-08-07T12:22:06Z

Sources/SwiftyRequest/RestRequest.swift


 }
+
+public struct ClientCertificate {


Comments needed in here.

ianpartridge · 2018-08-07T16:01:58Z

gen.sh

@@ -0,0 +1,17 @@
+podspec="Pod::Spec.new do |s|\ns.name        = \"$projectName\"\ns.version     = \"$TRAVIS_TAG\"\ns.summary     = \"$TRAVIS_DESCRIPTION\"\ns.homepage    = \"https://github.com/IBM-Swift/$projectName\"\ns.license     = { :type => \"Apache License, Version 2.0\" }\ns.author     = \"IBM\"\ns.module_name  = '$projectName'\ns.requires_arc = true\ns.ios.deployment_target = \"10.0\"\ns.source   = { :git => \"https://github.com/IBM-Swift/$projectName.git\", :tag => s.version }\ns.source_files = \"Sources/$projectName/*.swift\"\ns.pod_target_xcconfig =  {\n'SWIFT_VERSION' => '4.0.3',\n}"


I don't think this file should be here!

Sorry! Fixed.

ianpartridge · 2018-08-08T08:44:59Z

Sources/SwiftyRequest/RestRequest.swift

+/// Struct to store client certificate name and path
+public struct ClientCertificate {
+    /// The name for the client certificate
+    let name: String


Can you mark these public please.

…SwiftyRequest into clientCertificateAuth

neurosaurus · 2018-08-08T15:58:32Z

🎉

ShihabMehboob added 3 commits July 16, 2018 11:42

Initial 2waySSL client cert auth implementation

a2e94b9

Two way SSL enhancements

a06cdd0

Certs folder restructure

6619c5e

ianpartridge reviewed Jul 26, 2018

View reviewed changes

ShihabMehboob added 4 commits July 30, 2018 10:31

Minor adjustments to certificate creation logic

eab6f24

Minor adjustments to certificate creation logic

3f16d93

Minor adjustments, pass in a path to the certificate

fcde0ae

Certificate test restructure

dc134c5

ianpartridge requested changes Aug 6, 2018

View reviewed changes

Certificate test amendments

c25e6cc

ianpartridge requested changes Aug 6, 2018

View reviewed changes

Certificate test amendments

1cce015

ianpartridge reviewed Aug 6, 2018

View reviewed changes

Certificate test amendments

bf437fd

ianpartridge reviewed Aug 6, 2018

View reviewed changes

Certificate test amendments

37dd2d6

ianpartridge reviewed Aug 7, 2018

View reviewed changes

Certificate test amendments

fc20376

ianpartridge reviewed Aug 7, 2018

View reviewed changes

Sources/SwiftyRequest/RestRequest.swift

}

public struct ClientCertificate {

Copy link

Contributor

ianpartridge Aug 7, 2018

Choose a reason for hiding this comment

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

Comments needed in here.

Certificate test amendments

ee74534

ianpartridge reviewed Aug 7, 2018

View reviewed changes

ShihabMehboob and others added 2 commits August 8, 2018 09:31

Removed stray file

a98bf64

Merge branch 'master' into clientCertificateAuth

2668a69

ianpartridge reviewed Aug 8, 2018

View reviewed changes

ShihabMehboob added 2 commits August 8, 2018 09:54

Made clientCert vars public

613fc1b

Merge branch 'clientCertificateAuth' of https://github.com/IBM-Swift/…

8babde2

…SwiftyRequest into clientCertificateAuth

ianpartridge approved these changes Aug 8, 2018

View reviewed changes

ianpartridge merged commit 02c8e6a into master Aug 8, 2018

ianpartridge deleted the clientCertificateAuth branch August 8, 2018 09:20

djones6 mentioned this pull request May 10, 2019

Run SwiftyRequest tests against local test server #61

Merged

		@@ -0,0 +1,17 @@
		podspec="Pod::Spec.new do \|s\|\ns.name = \"$projectName\"\ns.version = \"$TRAVIS_TAG\"\ns.summary = \"$TRAVIS_DESCRIPTION\"\ns.homepage = \"https://github.com/IBM-Swift/$projectName\"\ns.license = { :type => \"Apache License, Version 2.0\" }\ns.author = \"IBM\"\ns.module_name = '$projectName'\ns.requires_arc = true\ns.ios.deployment_target = \"10.0\"\ns.source = { :git => \"https://github.com/IBM-Swift/$projectName.git\", :tag => s.version }\ns.source_files = \"Sources/$projectName/*.swift\"\ns.pod_target_xcconfig = {\n'SWIFT_VERSION' => '4.0.3',\n}"

		@@ -1 +1 @@
		4.1.2
		4.1

Conversation

ShihabMehboob commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShihabMehboob Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neurosaurus commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShihabMehboob Aug 6, 2018 •

edited

Loading

ianpartridge Aug 7, 2018 •

edited

Loading

neurosaurus commented Aug 8, 2018 •

edited

Loading