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

Feat/integrate smile security module #286

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

robin-smileid
Copy link
Contributor

@robin-smileid robin-smileid commented Jan 31, 2025

Story: https://app.shortcut.com/smileid/story/14751/sign-entire-request-payload-on-the-ios-v10-sdk

Summary

Integrates the SmileIDSecurity module to ensure protection of sensitive information

Known Issues

Test Instructions

Set useSandbox = true in

SmileID.initialize(config: config, useSandbox: false)
and download a smile_config.json from https://portal.dev.smileid.co/ (the backend change is currently deployed to the dev server). Then:

  • do a smart selfie enrollment and check that the correct request headers are set
  • do a enhanced kyc enrollment and check that the correct request headers are set
  • do a biometric key enrollment and check that the correct request headers are set

Screenshot

@robin-smileid robin-smileid self-assigned this Jan 31, 2025
Copy link

github-actions bot commented Jan 31, 2025

Warnings
⚠️ The source files were changed, but the tests remain unmodified. Consider updating or adding to the tests to match the source changes.
⚠️

Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift#L185 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L121 - Function should have complexity 10 or less: currently complexity equals 19 (cyclomatic_complexity)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L219 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L354 - Line should be 120 characters or less: currently 160 characters (line_length)

⚠️

Sources/SmileID/Classes/DocumentVerification/Model/OrchestratedDocumentVerificationViewModel.swift#L378 - Line should be 120 characters or less: currently 184 characters (line_length)

⚠️

Sources/SmileID/Classes/Networking/ServiceRunnable.swift#L146 - Variable name should be between 3 and 40 characters long: 'vc' (identifier_name)

⚠️

Sources/SmileID/Classes/Networking/ServiceRunnable.swift#L147 - Line should be 120 characters or less: currently 121 characters (line_length)

⚠️

Sources/SmileID/Classes/Networking/ServiceRunnable.swift#L149 - Variable name should be between 3 and 40 characters long: 'vc' (identifier_name)

⚠️

Sources/SmileID/Classes/Networking/ServiceRunnable.swift#L449 - Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

⚠️

Sources/SmileID/Classes/Networking/ServiceRunnable.swift#L463 - where clauses are preferred over a single if inside a for. (for_where)

⚠️

Sources/SmileID/Classes/SelfieCapture/SelfieViewModel.swift#L230 - TODOs should be resolved (Use mouth deformation as an al...). (todo)

⚠️

Sources/SmileID/Classes/SmileID.swift#L161 - Function should have complexity 10 or less: currently complexity equals 11 (cyclomatic_complexity)

⚠️

Sources/SmileID/Classes/SmileID.swift#L188 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

Generated by 🚫 Danger Swift against e038402

Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

Hey @robin-ankele looking good

  1. Let's undo signing
  2. I think the info from the secure sdk can be logged, can we not move some operations we've been doing lower level like upload and zip

@robin-smileid robin-smileid marked this pull request as ready for review February 24, 2025 11:32
@robin-smileid robin-smileid requested a review from a team as a code owner February 24, 2025 11:32
@prfectionist
Copy link

prfectionist bot commented Feb 24, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The code handles security-related information like MAC signatures and timestamps. The error handling in addSecurityInfo that silently continues without security info (line 156-159 in LocalStorage.swift) could potentially allow requests to proceed without proper security measures in place. This should be reviewed to ensure it doesn't compromise the security model.

⚡ Recommended focus areas for review

Error Handling
The error handling in the addSecurityInfo method silently continues without security info on error. This could mask security issues and should be reviewed.

Security Validation
The addSecurityInfo method creates security files without validating the input files exist or are valid. Should add validation of input parameters.

Breaking Change
The MultipartBody struct has been significantly changed, removing the key property and changing initialization parameters. This could break existing code.

@prfectionist
Copy link

prfectionist bot commented Feb 24, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Security
Improve error handling for security-related operations by distinguishing between critical and non-critical errors

The error handling in addSecurityInfo silently catches all errors and returns nil.
Consider logging specific error types or propagating critical security-related
errors to better handle security issues.

Sources/SmileID/Classes/Helpers/LocalStorage.swift [141-159]

-do{
+do {
     let timestamp = Date().toISO8601WithMilliseconds()
     let mac = try SmileIDCryptoManager.shared.sign(
         timestamp: timestamp,
         files: files
     )
     let securityInfo = SecurityInfo(
         timestamp: timestamp,
         mac: mac
     )
     let securityInfoJson = try createSecurityInfoFile(
         jobId: jobId,
         securityInfo: securityInfo
     )
     return securityInfoJson
+} catch let error as SmileIDCryptoError {
+    print("Security signing failed: \(error)")
+    throw error
 } catch {
-    print("Couldn't create security info. Continuing without it.")
+    print("Couldn't create security info: \(error). Continuing without it.")
     return nil
 }
Suggestion importance[1-10]: 8

Why: This is a critical security improvement that helps identify and properly handle security-related errors instead of silently failing, which could mask important security issues.

8
Possible issue
Add input validation to ensure required image data is not empty before initialization

Consider validating the input data (selfieImage and livenessImages) before encoding
to ensure they are not empty and meet size/format requirements.

Sources/SmileID/Classes/Networking/Models/SelfieRequest.swift [14-26]

 public init(
     selfieImage: Data,
     livenessImages: [Data],
     userId: String? = nil,
     partnerParams: [String: String]? = nil,
     callbackUrl: String? = nil,
     sandboxResult: Int? = nil,
     allowNewEnroll: Bool? = nil,
     failureReason: FailureReason? = nil,
     metadata: [Metadatum]
-) {
+) throws {
+    guard !selfieImage.isEmpty else {
+        throw SmileIDError.invalidRequestBody
+    }
+    guard !livenessImages.contains(where: { $0.isEmpty }) else {
+        throw SmileIDError.invalidRequestBody
+    }
     self.selfieImage = selfieImage
     self.livenessImages = livenessImages
Suggestion importance[1-10]: 7

Why: The suggestion adds important validation to prevent processing of invalid image data, which could lead to runtime errors or security vulnerabilities.

7
Enhancement
Remove unnecessary do-block since error handling is already managed by the calling function

Consider handling the case where SmileIDCryptoManager.shared.sign() fails by
propagating the error instead of silently continuing. This could hide
security-related issues.

Sources/SmileID/Classes/Networking/ServiceRunnable.swift [271-277]

-do{
-    let timestamp = Date().toISO8601WithMilliseconds()
-    let requestMac = try SmileIDCryptoManager.shared.sign(
-        timestamp: header.value,
-        headers: headers.toDictionary()
-    )
-    let signedHeaders = headers + [.requestMac(value: requestMac)]
+let timestamp = Date().toISO8601WithMilliseconds()
+let requestMac = try SmileIDCryptoManager.shared.sign(
+    timestamp: header.value,
+    headers: headers.toDictionary()
+)
+let signedHeaders = headers + [.requestMac(value: requestMac)]
Suggestion importance[1-10]: 3

Why: The suggestion correctly identifies redundant error handling, but the impact is minor since it's primarily a code style improvement that doesn't affect functionality.

3

@tobitech
Copy link
Contributor

Great job. I tested the jobs and worked fine. The request headers were present. I left a comment around preventing job submission when header is missing.

Copy link

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 12, 2025
@github-actions github-actions bot removed the Stale label Mar 18, 2025
Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

Small nits nothing to block this

@@ -108,6 +111,32 @@ extension ServiceRunnable {
headers.append(.timestamp(value: timestamp))
headers.append(.sourceSDK(value: "iOS"))
headers.append(.sourceSDKVersion(value: SmileID.version))
let timestamp = Date().toISO8601WithMilliseconds()
Copy link
Contributor

Choose a reason for hiding this comment

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

util func? I see it used in multiple places

Copy link
Contributor Author

@robin-smileid robin-smileid Mar 18, 2025

Choose a reason for hiding this comment

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

This is an extension to the Date object. Don't think we need an extra util function for that. Extensions are the way to do those things on iOS side as far as I am aware.

Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

Please fix lint

JNdhlovu
JNdhlovu previously approved these changes Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants