From e7acbf2ce5b66985501d334ae66fd23780ba9770 Mon Sep 17 00:00:00 2001 From: Amy Date: Tue, 30 Jan 2024 11:16:51 -0500 Subject: [PATCH 1/3] MBL-1157: Create PKCE code for code verifier and code challenge --- Kickstarter.xcodeproj/project.pbxproj | 9 +++++ KsApi/PKCE.swift | 58 +++++++++++++++++++++++++++ KsApi/PKCETest.swift | 20 +++++++++ 3 files changed, 87 insertions(+) create mode 100644 KsApi/PKCE.swift create mode 100644 KsApi/PKCETest.swift diff --git a/Kickstarter.xcodeproj/project.pbxproj b/Kickstarter.xcodeproj/project.pbxproj index 388d673e96..d7be73d310 100644 --- a/Kickstarter.xcodeproj/project.pbxproj +++ b/Kickstarter.xcodeproj/project.pbxproj @@ -1487,6 +1487,7 @@ E10BE8E72B151D2700F73DC9 /* BlockUserInputTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E10BE8E52B151CC800F73DC9 /* BlockUserInputTests.swift */; }; E10D06632ACF385E00470B5C /* FetchBackerProjectsQuery.json in Resources */ = {isa = PBXBuildFile; fileRef = E10D06622ACF385E00470B5C /* FetchBackerProjectsQuery.json */; }; E10D06652AD48C9C00470B5C /* FetchBackerProjectsQueryRequestForTests.graphql_test in Resources */ = {isa = PBXBuildFile; fileRef = E10D06642AD48C9C00470B5C /* FetchBackerProjectsQueryRequestForTests.graphql_test */; }; + E10F75E82B6937FA00024AD1 /* PKCETest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1EEED2A2B686829009976D9 /* PKCETest.swift */; }; E170B9112B20E83B001BEDD7 /* MockGraphQLClient+CombineTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E170B9102B20E83B001BEDD7 /* MockGraphQLClient+CombineTests.swift */; }; E1A1491E2ACDD76800F49709 /* FetchBackerProjectsQuery.graphql in Resources */ = {isa = PBXBuildFile; fileRef = E1A1491D2ACDD76700F49709 /* FetchBackerProjectsQuery.graphql */; }; E1A149202ACDD7BF00F49709 /* FetchProjectsEnvelope+FetchBackerProjectsQueryData.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1A1491F2ACDD7BF00F49709 /* FetchProjectsEnvelope+FetchBackerProjectsQueryData.swift */; }; @@ -1495,6 +1496,8 @@ E1A149272ACE063400F49709 /* FetchBackerProjectsQueryDataTemplate.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1A149262ACE063400F49709 /* FetchBackerProjectsQueryDataTemplate.swift */; }; E1AA8ABF2AEABBB100AC98BF /* Signal+Combine.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1EA34EE2AE1B28400942A04 /* Signal+Combine.swift */; }; E1BB25642B1E81AA000BD2D6 /* Publisher+Service.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1BB25632B1E81AA000BD2D6 /* Publisher+Service.swift */; }; + E1EEED292B684AA7009976D9 /* PKCE.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1EEED282B684AA7009976D9 /* PKCE.swift */; }; + E1FDB1E82AEAAC6100285F93 /* CombineTestObserver.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1FDB1E72AEAAC6100285F93 /* CombineTestObserver.swift */; }; /* End PBXBuildFile section */ /* Begin PBXContainerItemProxy section */ @@ -3063,6 +3066,8 @@ E1A149262ACE063400F49709 /* FetchBackerProjectsQueryDataTemplate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FetchBackerProjectsQueryDataTemplate.swift; sourceTree = ""; }; E1BB25632B1E81AA000BD2D6 /* Publisher+Service.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Publisher+Service.swift"; sourceTree = ""; }; E1EA34EE2AE1B28400942A04 /* Signal+Combine.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Signal+Combine.swift"; sourceTree = ""; }; + E1EEED282B684AA7009976D9 /* PKCE.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PKCE.swift; sourceTree = ""; }; + E1EEED2A2B686829009976D9 /* PKCETest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = PKCETest.swift; path = KsApi/PKCETest.swift; sourceTree = ""; }; E1FDB1E72AEAAC6100285F93 /* CombineTestObserver.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CombineTestObserver.swift; sourceTree = ""; }; /* End PBXFileReference section */ @@ -6138,6 +6143,7 @@ A7E06C701C5A6EB300EBDCC2 = { isa = PBXGroup; children = ( + E1EEED2A2B686829009976D9 /* PKCETest.swift */, 802800561C88F62500141235 /* Configs */, E10BE8CE2B02975C00F73DC9 /* RichPushNotifications */, A7E06DBC1C5C027800EBDCC2 /* Frameworks */, @@ -6506,6 +6512,7 @@ D67F29351F68333800E399A6 /* GraphSchema.swift */, D6C9A20D1F755FE200981E64 /* GraphSchemaTests.swift */, D01587761EEB2ED6006E7684 /* MockService.swift */, + E1EEED282B684AA7009976D9 /* PKCE.swift */, D01588261EEB2ED7006E7684 /* ServerConfig.swift */, D01588271EEB2ED7006E7684 /* Service.swift */, E1BB25632B1E81AA000BD2D6 /* Publisher+Service.swift */, @@ -8479,6 +8486,7 @@ E10BE8E02B1517FB00F73DC9 /* GraphAPI.BlockUserInput+BlockUserInput.swift in Sources */, D01588EF1EEB2ED7006E7684 /* MessageThread.swift in Sources */, 8A4E95392450FBB500A578CF /* CreditCardType.swift in Sources */, + E1EEED292B684AA7009976D9 /* PKCE.swift in Sources */, D0158A2F1EEB30A2006E7684 /* User.MemberDataTemplates.swift in Sources */, D015883D1EEB2ED7006E7684 /* Route.swift in Sources */, D0158A2B1EEB30A2006E7684 /* UpdateDraftTemplates.swift in Sources */, @@ -8787,6 +8795,7 @@ D01589A81EEB306D006E7684 /* EncodableTests.swift in Sources */, D0D10C111EEB4550005EBAD0 /* MessageThreadTests.swift in Sources */, 066C0ADE26CAEAFD0048F4D8 /* CreatePaymentSourceMutationTemplate.swift in Sources */, + E10F75E82B6937FA00024AD1 /* PKCETest.swift in Sources */, 06032A9426CEF4810013AAB9 /* FetchUserBackingsQueryTemplate.swift in Sources */, 06B9AF4326AF465000735908 /* UserCreditCards+UserFragmentTests.swift in Sources */, 066C0AE326CC0E410048F4D8 /* CreateBackingMutationTemplate.swift in Sources */, diff --git a/KsApi/PKCE.swift b/KsApi/PKCE.swift new file mode 100644 index 0000000000..8ae890f0de --- /dev/null +++ b/KsApi/PKCE.swift @@ -0,0 +1,58 @@ +import CryptoKit +import Foundation + +/// PKCE stands for Proof Key for Code Exchange. +/// The code verifier, and its associated challenge, are used to ensure that an oAuth request is valid. +/// See this documentation for more details: https://www.oauth.com/oauth2-servers/mobile-and-native-apps/authorization/ +public struct PKCE { + /// Creates a random alphanumeric string of the specified length + static func createCodeVerifier(ofLength length: Int) throws -> String { + let uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + let lowercase = "abcdefghijklmnopqrstuvwxyz" + let numbers = "0123456789" + let specials = "-._~" + + let validCharacters = uppercase + lowercase + numbers + specials + + var codeVerifier = "" + + for _ in 0.. String { + guard let stringData = verifier.data(using: .utf8) else { + throw ErrorType.UnableToCreateCodeChallenge + } + + let hash = SHA256.hash(data: stringData) + var hashData: Data? + + hash.withUnsafeBytes { pointer in + let dataPointer = pointer.bindMemory(to: UInt8.self) + hashData = Data(buffer: dataPointer) + } + + guard let unwrappedData = hashData else { + throw ErrorType.UnableToCreateCodeChallenge + } + + return unwrappedData.base64EncodedString() + } + + /// Errors that can occur in the PKCE flow. + /// Note that these are exceptional errors - effectively run time errors that we don't ever expect to happen. + /// However, because PKCE is a critical part of auth, it's worthwhile to be explicit if the unexpected ever *did* happen. + public enum ErrorType: Error { + case UnableToCreateCodeChallenge + case UnableToCreateCodeVerifier + } +} diff --git a/KsApi/PKCETest.swift b/KsApi/PKCETest.swift new file mode 100644 index 0000000000..751f192449 --- /dev/null +++ b/KsApi/PKCETest.swift @@ -0,0 +1,20 @@ +@testable import KsApi +import XCTest + +final class PKCETest: XCTestCase { + func testCodeChallenge_isValid() { + let res = try! PKCE.createCodeChallenge(fromVerifier: "foo") + XCTAssertEqual(res, "LCa0a2j/xo/5m0U8HTBBNBNCLXBkg7+g+YpeiGJm564=") + } + + func testCreateCodeVerifier_isCorrectLength() { + let verifier1 = try! PKCE.createCodeVerifier(ofLength: 1) + XCTAssertEqual(verifier1.count, 1) + + let verifier2 = try! PKCE.createCodeVerifier(ofLength: 32) + XCTAssertEqual(verifier2.count, 32) + + let verifier3 = try! PKCE.createCodeVerifier(ofLength: 64) + XCTAssertEqual(verifier3.count, 64) + } +} From 0bee4d8612f522c1d72550f019319f8ca20a1f4c Mon Sep 17 00:00:00 2001 From: Amy Date: Wed, 31 Jan 2024 12:56:09 -0500 Subject: [PATCH 2/3] Changes per review and discussion with security --- KsApi/PKCE.swift | 93 ++++++++++++++++++++++++++++---------------- KsApi/PKCETest.swift | 70 ++++++++++++++++++++++++++++----- 2 files changed, 120 insertions(+), 43 deletions(-) diff --git a/KsApi/PKCE.swift b/KsApi/PKCE.swift index 8ae890f0de..82909f9895 100644 --- a/KsApi/PKCE.swift +++ b/KsApi/PKCE.swift @@ -1,39 +1,42 @@ import CryptoKit import Foundation +import Security -/// PKCE stands for Proof Key for Code Exchange. -/// The code verifier, and its associated challenge, are used to ensure that an oAuth request is valid. -/// See this documentation for more details: https://www.oauth.com/oauth2-servers/mobile-and-native-apps/authorization/ -public struct PKCE { - /// Creates a random alphanumeric string of the specified length - static func createCodeVerifier(ofLength length: Int) throws -> String { - let uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - let lowercase = "abcdefghijklmnopqrstuvwxyz" - let numbers = "0123456789" - let specials = "-._~" - - let validCharacters = uppercase + lowercase + numbers + specials - - var codeVerifier = "" +enum PKCEError: Error { + case UnexpectedRuntimeError +} - for _ in 0.. String { + var encodedString = self.base64EncodedString() - codeVerifier += String(character) - } + // Convert base64 to base64url + encodedString = encodedString.replacingOccurrences(of: "+", with: "-") + encodedString = encodedString.replacingOccurrences(of: "/", with: "_") + // Strip padding + encodedString = encodedString.replacingOccurrences(of: "=", with: "") - return codeVerifier + return encodedString } - /// Creates a base-64 encoded SHA256 hash of the given string. - static func createCodeChallenge(fromVerifier verifier: String) throws -> String { - guard let stringData = verifier.data(using: .utf8) else { - throw ErrorType.UnableToCreateCodeChallenge + mutating func fillWithRandomSecureBytes() throws { + do { + let numBytes = self.count + try self.withUnsafeMutableBytes { rawPointer in + let pointer = rawPointer.bindMemory(to: UInt8.self) + guard let address = pointer.baseAddress else { + throw PKCEError.UnexpectedRuntimeError + } + + _ = SecRandomCopyBytes(kSecRandomDefault, numBytes, address) + } + } catch { + throw error } + } - let hash = SHA256.hash(data: stringData) + func sha256Hash() throws -> Data { + let hash = SHA256.hash(data: self) var hashData: Data? hash.withUnsafeBytes { pointer in @@ -42,17 +45,39 @@ public struct PKCE { } guard let unwrappedData = hashData else { - throw ErrorType.UnableToCreateCodeChallenge + throw PKCEError.UnexpectedRuntimeError } - return unwrappedData.base64EncodedString() + return unwrappedData } +} - /// Errors that can occur in the PKCE flow. - /// Note that these are exceptional errors - effectively run time errors that we don't ever expect to happen. - /// However, because PKCE is a critical part of auth, it's worthwhile to be explicit if the unexpected ever *did* happen. - public enum ErrorType: Error { - case UnableToCreateCodeChallenge - case UnableToCreateCodeVerifier +/// PKCE stands for Proof Key for Code Exchange. +/// The code verifier, and its associated challenge, are used to ensure that an oAuth request is valid. +/// See this documentation for more details: https://www.oauth.com/oauth2-servers/mobile-and-native-apps/authorization/ +public struct PKCE { + /// Creates a random alphanumeric string of the specified length + static func createCodeVerifier(byteLength length: Int) throws -> String { + do { + var buffer = Data(count: length) + try buffer.fillWithRandomSecureBytes() + return buffer.base64URLEncodedStringWithNoPadding() + } catch _ { + throw PKCEError.UnexpectedRuntimeError + } + } + + /// Creates a base-64 encoded SHA256 hash of the given string. + static func createCodeChallenge(fromVerifier verifier: String) throws -> String { + guard let stringData = verifier.data(using: .utf8) else { + throw PKCEError.UnexpectedRuntimeError + } + + do { + let hash = try stringData.sha256Hash() + return hash.base64URLEncodedStringWithNoPadding() + } catch { + throw PKCEError.UnexpectedRuntimeError + } } } diff --git a/KsApi/PKCETest.swift b/KsApi/PKCETest.swift index 751f192449..0702fac445 100644 --- a/KsApi/PKCETest.swift +++ b/KsApi/PKCETest.swift @@ -2,19 +2,71 @@ import XCTest final class PKCETest: XCTestCase { - func testCodeChallenge_isValid() { + func testData_RandomSecureBytes_overwritesOriginalData() { + let buffer1 = Data(repeating: 0, count: 32) + var buffer2 = Data(repeating: 0, count: 32) + var buffer3 = Data(repeating: 0, count: 32) + + XCTAssertEqual(buffer1, buffer2) + XCTAssertEqual(buffer2, buffer3) + + try! buffer2.fillWithRandomSecureBytes() + try! buffer3.fillWithRandomSecureBytes() + + XCTAssertNotEqual( + buffer1, + buffer2, + "Buffer filled with random data should not be equal to buffer filled with zeroes" + ) + XCTAssertNotEqual( + buffer1, + buffer3, + "Buffer filled with random data should not be equal to buffer filled with zeroes" + ) + XCTAssertNotEqual(buffer2, buffer3, "Two randomly filled buffers should not be equal") + } + + func testData_SHA256Hash_isCorrectValue() { + let testString = "Hello, world. I am a string." + let stringData = testString.data(using: .utf8) + + let hash = try! stringData!.sha256Hash() + + let expectedHashString = "c1c6864039f380248d30a73525c8351427c7fb468d58b7b1005f3e3727a042a1" + let actualHashString = hash.map { String(format: "%02x", $0) }.joined() + + XCTAssertEqual(expectedHashString, actualHashString) + } + + func testData_Base64URLEncodedString_isCorrectValue() { + let testString = "Hello, world. I am a string." + + let expectedEncodedString = "SGVsbG8sIHdvcmxkLiBJIGFtIGEgc3RyaW5nLg" + let actualEncodedString = testString.data(using: .utf8)!.base64URLEncodedStringWithNoPadding() + + XCTAssertEqual(expectedEncodedString, actualEncodedString) + } + + func testPKCECodeChallenge_isValid() { + // TODO: Add more examples. let res = try! PKCE.createCodeChallenge(fromVerifier: "foo") - XCTAssertEqual(res, "LCa0a2j/xo/5m0U8HTBBNBNCLXBkg7+g+YpeiGJm564=") + XCTAssertEqual(res, "LCa0a2j_xo_5m0U8HTBBNBNCLXBkg7-g-YpeiGJm564") } - func testCreateCodeVerifier_isCorrectLength() { - let verifier1 = try! PKCE.createCodeVerifier(ofLength: 1) - XCTAssertEqual(verifier1.count, 1) + func testPKCECreateCodeVerifier_matchesRequirements() { + let verifier = try! PKCE.createCodeVerifier(byteLength: 32) + XCTAssertEqual(verifier.count, 43) + + let uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + let lowercase = "abcdefghijklmnopqrstuvwxyz" + let numbers = "0123456789" + let specials = "-._~" - let verifier2 = try! PKCE.createCodeVerifier(ofLength: 32) - XCTAssertEqual(verifier2.count, 32) + let validCharacters = CharacterSet(charactersIn: uppercase + lowercase + numbers + specials) - let verifier3 = try! PKCE.createCodeVerifier(ofLength: 64) - XCTAssertEqual(verifier3.count, 64) + for character in verifier { + let unichar = character.unicodeScalars.first! + XCTAssertTrue(validCharacters.contains(unichar), "\(unichar) not in valid character set") + } } } From 0767a38610f9e9483cb1559d05ebc34cdb835232 Mon Sep 17 00:00:00 2001 From: Amy Date: Wed, 31 Jan 2024 13:09:10 -0500 Subject: [PATCH 3/3] Handle result of SecRandomCopyBytes --- KsApi/PKCE.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/KsApi/PKCE.swift b/KsApi/PKCE.swift index 82909f9895..c5c6902c44 100644 --- a/KsApi/PKCE.swift +++ b/KsApi/PKCE.swift @@ -28,7 +28,11 @@ public extension Data { throw PKCEError.UnexpectedRuntimeError } - _ = SecRandomCopyBytes(kSecRandomDefault, numBytes, address) + let result = SecRandomCopyBytes(kSecRandomDefault, numBytes, address) + + if result != errSecSuccess { + throw PKCEError.UnexpectedRuntimeError + } } } catch { throw error