Webcrypto Polyfill package#23
Webcrypto Polyfill package#23oktapp-aperture-okta[bot] merged 47 commits intojp-react-native-sdkfrom
Conversation
…rectness, and architecture improvements Critical fixes: - Fix Android exportKey compilation error (undefined keyPair variable) - Fix Android importKey type mismatch (KeyPair vs KeyPairEntry) - Fix iOS silent failure in getRandomValues (now calls fatalError on CSPRNG failure) - Fix iOS importKey key size calculation (derive from modulus, not DER blob) - Fix iOS ASN.1 DER parsing with proper length decoding (replaces hardcoded offsets) - Fix iOS hardcoded exponent with parsed value from key data Security & safety: - Eliminate all force-unwrap (as!) casts in iOS via typed KeyEntry struct - Add synchronized blocks to all Android keyStore accesses for thread safety - Add key usage validation in JS verify() to match WebCrypto spec - JS getRandomValues validates returned length matches requested length Architecture: - Migrate bridge serialization from number[]/[NSNumber]/ReadableArray to Base64 strings across all layers (iOS, Android, TypeScript spec, JS polyfill), reducing serialization overhead from ~400% to ~33% - Extract iOS RSA DER parsing/construction into standalone RSAKeyUtils.swift for testability - Replace Android hand-rolled X.509 DER construction (~60 lines) with platform RSAPublicKeySpec - Remove dead code (getCryptoAlg), fix typos, clean up unused parameters Infrastructure: - Fix Jest config (testMatch pattern, setupFiles path, test import paths) - Add unit tests for digest, getRandomValues, importKey, verify, and polyfill installation - Update mocks to match Base64 bridge interface - Remove private:true flag, remove nonexistent ./cpp from files array - Update Android SDK defaults to 35
Harden native crypto bridge with security, correctness, and architecture improvements
FeiChen-okta
left a comment
There was a problem hiding this comment.
Left some comments for the android implementation.
|
|
||
| @ReactMethod(isBlockingSynchronousMethod = true) | ||
| fun randomUUID(): String { | ||
| return UUID.randomUUID().toString() |
Android module is on the wrong path. Add biometric auth and fix the android implementation by using android keystore.
update the android components.
| companion object { | ||
| const val NAME = "WebCryptoBridge" | ||
|
|
||
| private val cryptoKeys = mutableMapOf<String, CryptoKey>() |
There was a problem hiding this comment.
Move out of companion object. Reloading this module will keep this since the companion object survives reloads.
There was a problem hiding this comment.
Also do you need a remove method? to clean up keys that are not needed?
There was a problem hiding this comment.
Removal is an excellent question. The WebCrypto API doesn't have a concept of removal. For now I think the keys clearing when the app closes is good enough. Standard usage shouldn't result in more than a few keys
| promise: Promise | ||
| ) { | ||
| runCatching { | ||
| if (algorithm != "SHA-256") { |
There was a problem hiding this comment.
can we just remove algorithm as a parameter if it only support SHA-256? Allowing anything else will just lead to error anyways.
There was a problem hiding this comment.
This may support other algorithms in the future
|
|
||
| dependencies { | ||
| implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | ||
| implementation 'com.facebook.react:react-native:+' |
There was a problem hiding this comment.
use a specific version to avoid supply chain risks.
There was a problem hiding this comment.
I attempted to address this, not sure if it did so successfully
Configures automated testing for packages/react-native-webcrypto-bridge: - Android: JUnit/Robolectric tests via Gradle on Linux executor - iOS: Swift Package Manager tests on macOS executor - Both jobs run in parallel with intelligent caching Jobs restore dependencies from cache based on build.gradle/Package.swift checksums, reducing build times by 50-70% on subsequent runs. Test results and artifacts are stored for CircleCI reporting.
Change xcode from 'latest' to '16.1' to match CircleCI schema requirements. Xcode version must be a specific version string, not 'latest'.
Change macos.m1 to macos.medium for valid Xcode 16.1 compatibility. Remove unnecessary environment variables.
Change cimg/android:latest to cimg/android:2024.12 (a valid versioned image). CircleCI requires specific image versions, not 'latest' tags.
The circleci/macos orb doesn't provide executor definitions. Use simple custom executors with direct Docker and macOS config.
There was a problem hiding this comment.
Is this supposed to be committed? Or was this cache file added accidentally?
There was a problem hiding this comment.
Same goes for this and the other files in the .idea directory
| // Platform-specific redirect URI - iOS uses single slash, Android uses double slash | ||
| const redirectUri = Platform.OS === 'ios' | ||
| ? 'com.oktapreview.jperreault-test:/callback' | ||
| : 'com.oktapreview.jperreault-test://callback'; |
There was a problem hiding this comment.
I don't think this is really platform specific, you can just change the OIDC config to match in the auth server configuration.
There was a problem hiding this comment.
This is just test app code - I'll clean this up in a follow up PR
| // scopes: ['openid', 'email', 'profile', 'offline_access'], | ||
| scopes: ['offline_access'], | ||
| scopes: ['openid', 'email', 'profile', 'offline_access'], | ||
| // scopes: ['offline_access'], |
There was a problem hiding this comment.
This is just test app code - I'll clean this up in a follow up PR
| extension String { | ||
| public var base64URLDecoded: String { convertToBase64URLDecoded() } | ||
|
|
||
| private func convertToBase64URLDecoded() -> String { | ||
| var result = replacingOccurrences(of: "-", with: "+") | ||
| .replacingOccurrences(of: "_", with: "/") | ||
|
|
||
| while result.count % 4 != 0 { | ||
| result.append(contentsOf: "=") | ||
| } | ||
|
|
||
| return result | ||
| } | ||
| } | ||
|
|
||
| extension Data { | ||
| public func base64URLEncodedString() -> String { | ||
| var base64 = self.base64EncodedString() | ||
| base64 = base64.replacingOccurrences(of: "+", with: "-") | ||
| base64 = base64.replacingOccurrences(of: "/", with: "_") | ||
| base64 = base64.replacingOccurrences(of: "=", with: "") | ||
| return base64 | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate code, this is already present in another Swift file
| s.summary = package["description"] | ||
| s.homepage = "https://github.com/okta/okta-client-javascript" | ||
| s.license = package["license"] | ||
| s.authors = { "Okta" => "jared.perreault@okta.com" } |
There was a problem hiding this comment.
The author name / email should be changed to our standard one used for our other libraries, as opposed to our own Okta email.
| "types": "dist/types/index.d.ts", | ||
| "react-native": "src/index.ts", | ||
| "source": "src/index.ts", | ||
| "author": "jared.perreault@okta.com", |
There was a problem hiding this comment.
The separate lock() / unlock() calls can be error prone if a developer messes something up in the future. NSLock conforms to the NSLocking protocol which provides a convenience withLock wrapper that ensures a block of code is run within a locked context.
We should try to use this instead, if possible.
ed58f5d
into
jp-react-native-sdk
Initial Setup of
@okta/react-native-platformMVP Impl of
@okta/react-native-webcrypto-bridge