MBL-1157: Create PKCE code for code verifier and code challenge #1921

amy-at-kickstarter · 2024-01-30T16:49:54Z

📲 What

Added a utility for PKCE verification.

🤔 Why

PKCE is part of the native apps flow for oAuth - we create a secret (the code verifier) and a challenge (a hashed version) which gets sent to the server. This is used to verify that every step of the oAuth flow is legit. Later in the flow, the client has to prove that it was the one that generated the hash by providing the original secret.

For more, see: https://www.oauth.com/oauth2-servers/mobile-and-native-apps/authorization/

🛠 How

I used this example code, as well as the documentation above.

⏰ TODO

I'd like to add more tests to this, which I'll coordinate with Isa on the Android side. These tests are just to make sure the code is running.

amy-at-kickstarter · 2024-01-30T16:50:45Z

KsApi/PKCE.swift

+/// 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 {


This doesn't need to be a struct; I'm just doing it for the sake of namespace clarity. These methods are called like PKCE.createCodeVerifier(ofLength:).

amy-at-kickstarter · 2024-01-30T16:51:59Z

KsApi/PKCE.swift

+    }
+
+    guard let unwrappedData = hashData else {
+      throw ErrorType.UnableToCreateCodeChallenge


Somewhat annoyingly - there are a few cases in this code where I need to unwrap a nil coming from a system library. In the interest of making this bulletproof, I'm throwing errors if those unwraps fail - but I never expect that to happen, pragmatically.

If any part of PKCE fails, I think the correct solution is to abort the login process, and just let the user continue in a logged-out state.

amy-at-kickstarter · 2024-01-30T16:52:23Z

KsApi/PKCETest.swift

+@testable import KsApi
+import XCTest
+
+final class PKCETest: XCTestCase {


Some rough tests - we'll coordinate more examples later, to make sure iOS + Android have the same behavior.

amy-at-kickstarter · 2024-01-30T16:53:27Z

KsApi/PKCE.swift

+    var codeVerifier = ""
+
+    for _ in 0..<length {
+      guard let character = validCharacters.randomElement() else {


@Arkariang So this is something I wanted to verify - randomElement() uses the system random number generator. Is that going to be good enough for PKCE? The documentation says "cryptographically random" and it's not clear if it needs a more random kind of random than the system provides.

I hesitate to rely on the default random generator for crypto - all my crypto classes in college taught "never implement crypto yourself if you can avoid it". However, I did a little digging and it looks fairly safe.

Reasoning: https://developer.apple.com/documentation/swift/systemrandomnumbergenerator claims that the system default random number generator is cryptographically secure whenever possible. https://developer.apple.com/documentation/security/randomization_services, which is cryptographically secure, uses the system default as its source of randomness (unsure what it does if the system default isn't good enough). .randomElement uses the system default as well.

The main problem with this approach is that I'm not sure what happens if the system default isn't secure enough (which could happen if there isn't a lot of entropy to draw from - ex new computers booted up at the same time will probably have the same seed and get the same "random" numbers). It's possible that Apple's cryptographically secure random bytes method would throw an error instead in this case, and that that's what makes it actually cryptographically random. However, I don't know enough about PKCE to know if this would actually be a problem we need to handle; I expect our real users would have cryptographically random verifiers.

I think I'd recommend leaving this as-is for now and going to security office hours next week. Or looking for maybe 3rd party solutions for getting guaranteed cryptographic randomness. (Or using https://developer.apple.com/documentation/security/1399291-secrandomcopybytes, but then we can't specify what kinds of bytes we're okay with, as far as I can tell.)

I think it is refered to use some kind of secure hash algorithm, in my case for Android I'll use SHA-256

I think it is refered to use some kind of secure hash algorithm, in my case for Android I'll use SHA-256

Aah that's what I'm using for the code challenge. I saw in your example code that the code verifier uses SecureRandom.nextBytes(), and I was hoping to find an iOS equivalent.

I think I'd recommend leaving this as-is for now and going to security office hours next week.

Aha, thanks for digging into the system randomizer, that's super interesting. (And for finding SecRandomCopyBytes. I knew that existed somewhere but couldn't remember what it was called, for the life of me.) I think security office hours is a great idea - I'd love to get a more expert opinion on this.

ifosli · 2024-01-30T18:20:37Z

KsApi/PKCE.swift

+    var codeVerifier = ""
+
+    for _ in 0..<length {
+      guard let character = validCharacters.randomElement() else {


I hesitate to rely on the default random generator for crypto - all my crypto classes in college taught "never implement crypto yourself if you can avoid it". However, I did a little digging and it looks fairly safe.

Reasoning: https://developer.apple.com/documentation/swift/systemrandomnumbergenerator claims that the system default random number generator is cryptographically secure whenever possible. https://developer.apple.com/documentation/security/randomization_services, which is cryptographically secure, uses the system default as its source of randomness (unsure what it does if the system default isn't good enough). .randomElement uses the system default as well.

The main problem with this approach is that I'm not sure what happens if the system default isn't secure enough (which could happen if there isn't a lot of entropy to draw from - ex new computers booted up at the same time will probably have the same seed and get the same "random" numbers). It's possible that Apple's cryptographically secure random bytes method would throw an error instead in this case, and that that's what makes it actually cryptographically random. However, I don't know enough about PKCE to know if this would actually be a problem we need to handle; I expect our real users would have cryptographically random verifiers.

I think I'd recommend leaving this as-is for now and going to security office hours next week. Or looking for maybe 3rd party solutions for getting guaranteed cryptographic randomness. (Or using https://developer.apple.com/documentation/security/1399291-secrandomcopybytes, but then we can't specify what kinds of bytes we're okay with, as far as I can tell.)

amy-at-kickstarter · 2024-01-31T17:58:19Z

KsApi/PKCE.swift

+  case UnexpectedRuntimeError
+}
+
+public extension Data {


These utilities are translated from the examples in OpenID: https://github.com/openid/AppAuth-iOS/blob/aea7b8acd8b3fc261b8a42c998de33d76851f30b/Source/AppAuthCore/OIDTokenUtilities.m#L31-L55

amy-at-kickstarter · 2024-01-31T18:04:49Z

KsApi/PKCE.swift

+          throw PKCEError.UnexpectedRuntimeError
+        }
+
+        _ = SecRandomCopyBytes(kSecRandomDefault, numBytes, address)


@ifosli Thanks for reminding me what on earth this method was called - I was looking for it and couldn't find it 😅

amy-at-kickstarter · 2024-01-31T18:05:32Z

KsApi/PKCE.swift

+    do {
+      var buffer = Data(count: length)
+      try buffer.fillWithRandomSecureBytes()
+      return buffer.base64URLEncodedStringWithNoPadding()


This is how the RFC for OAuth suggests doing it - you create a shorter, cryptographically random buffer, and use base64 URL encoding to turn it into the valid character set.

amy-at-kickstarter · 2024-01-31T18:06:25Z

KsApi/PKCE.swift

+    }
+
+    guard let unwrappedData = hashData else {
+      throw PKCEError.UnexpectedRuntimeError


Alternatively, I could make these methods return a nullable Data? and handle it further down. Preferences?

ifosli

Apart from checking the return value of the secure bytes function, looks good to me!

ifosli · 2024-01-31T18:13:23Z

KsApi/PKCE.swift

+          throw PKCEError.UnexpectedRuntimeError
+        }
+
+        _ = SecRandomCopyBytes(kSecRandomDefault, numBytes, address)


Nit: I think we should confirm that the return type is errSecSuccess (https://developer.apple.com/documentation/security/1399291-secrandomcopybytes#discussion) and throw an error if not.

Good one; fixed!

ifosli · 2024-01-31T18:25:36Z

KsApi/PKCETest.swift

+    let uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+    let lowercase = "abcdefghijklmnopqrstuvwxyz"
+    let numbers = "0123456789"
+    let specials = "-._~"


Out of curiosity, how would you get . or ~ in the verifier?

Good question - the RFC mentioned that these were valid, but you're right, they're not part of base64url 🤷‍♀️

amy-at-kickstarter commented Jan 30, 2024

View reviewed changes

amy-at-kickstarter marked this pull request as ready for review January 30, 2024 16:53

amy-at-kickstarter requested review from ifosli, Arkariang, scottkicks and santiDotIO January 30, 2024 16:53

amy-at-kickstarter self-assigned this Jan 30, 2024

ifosli approved these changes Jan 30, 2024

View reviewed changes

amy-at-kickstarter added 2 commits January 31, 2024 12:56

MBL-1157: Create PKCE code for code verifier and code challenge

e7acbf2

Changes per review and discussion with security

0bee4d8

amy-at-kickstarter force-pushed the feat/adyer/MBL-1157/pkce branch from fb094ca to 0bee4d8 Compare January 31, 2024 17:56

amy-at-kickstarter commented Jan 31, 2024

View reviewed changes

amy-at-kickstarter requested a review from ifosli January 31, 2024 17:58

amy-at-kickstarter commented Jan 31, 2024

View reviewed changes

Handle result of SecRandomCopyBytes

0767a38

ifosli approved these changes Jan 31, 2024

View reviewed changes

amy-at-kickstarter merged commit dd545c0 into main Jan 31, 2024
5 checks passed

amy-at-kickstarter deleted the feat/adyer/MBL-1157/pkce branch January 31, 2024 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBL-1157: Create PKCE code for code verifier and code challenge #1921

MBL-1157: Create PKCE code for code verifier and code challenge #1921

amy-at-kickstarter commented Jan 30, 2024

amy-at-kickstarter Jan 30, 2024

amy-at-kickstarter Jan 30, 2024

amy-at-kickstarter Jan 30, 2024

amy-at-kickstarter Jan 30, 2024

ifosli Jan 30, 2024

Arkariang Jan 30, 2024

amy-at-kickstarter Jan 30, 2024 •

edited

ifosli Jan 30, 2024

amy-at-kickstarter Jan 31, 2024

amy-at-kickstarter Jan 31, 2024

amy-at-kickstarter Jan 31, 2024

amy-at-kickstarter Jan 31, 2024 •

edited

ifosli left a comment

ifosli Jan 31, 2024

amy-at-kickstarter Jan 31, 2024

ifosli Jan 31, 2024

amy-at-kickstarter Jan 31, 2024

MBL-1157: Create PKCE code for code verifier and code challenge #1921

MBL-1157: Create PKCE code for code verifier and code challenge #1921

Conversation

amy-at-kickstarter commented Jan 30, 2024

📲 What

🤔 Why

🛠 How

⏰ TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Jan 31, 2024 • edited

Choose a reason for hiding this comment

ifosli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Jan 30, 2024 •

edited

amy-at-kickstarter Jan 31, 2024 •

edited