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: Certificate pinning and proxy mode in WireAPI - WPB-10450 #2237

Merged
merged 34 commits into from
Dec 12, 2024

Conversation

samwyndham
Copy link
Contributor

@samwyndham samwyndham commented Dec 3, 2024

TaskWPB-10450 [iOS] Support certificate pinning

Issue

This PR adds a certificate pinning implementation to the WireAPI module. It is part of quick sync refactor. It doesn't attempt to use this new implementation in the app.

The code is essentially duplicated from WireTransport with some modifications. Main things to look at are:

  • ServerTrustValidator basically combines the existing ServerCertificateTrust.swift with security key generation code from TrustData.swift. See my comments for specific changes.
  • PinnedKey.swift will replace TrustData.swift. It is just a basic struct without code for reading the data from file, and generating security keys.

Testing

Run the automated tests.


Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

@echoes-hq echoes-hq bot added echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt. echoes/initiative: ios-sync-refactoring labels Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Test Results

   16 files     17 suites   13m 59s ⏱️
8 617 tests 8 564 ✅ 31 💤 22 ❌
8 638 runs  8 606 ✅ 31 💤  1 ❌

For more details on these failures, see this check.

Results for commit 07d84e0.

♻️ This comment has been updated with latest results.

@samwyndham samwyndham requested review from jullianm and johnxnguyen and removed request for jullianm December 3, 2024 16:43
@samwyndham samwyndham changed the title feat: Certificate pinning in WireAPI - WPB-10450 feat: Certificate pinning in WireAPI - WPB-10450 Dec 3, 2024
@samwyndham samwyndham requested a review from jullianm December 3, 2024 16:44
@samwyndham samwyndham changed the title feat: Certificate pinning in WireAPI - WPB-10450 WIP - feat: Certificate pinning in WireAPI - WPB-10450 Dec 3, 2024
Copy link
Contributor

@jullianm jullianm left a comment

Choose a reason for hiding this comment

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

looks good

@netbe netbe added the release label Dec 11, 2024
Copy link
Collaborator

@johnxnguyen johnxnguyen left a comment

Choose a reason for hiding this comment

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

Looks good, nice work

@samwyndham samwyndham enabled auto-merge December 11, 2024 13:31
@datadog-wireapp
Copy link

datadog-wireapp bot commented Dec 11, 2024

Datadog Report

Branch report: feat/wire-api-cert-pinning-WPB-10450
Commit report: 288408b
Test service: wire-ios-mono

❌ 1 Failed (0 Known Flaky), 8316 Passed, 31 Skipped, 5m 51.05s Total Time
❄️ 1 New Flaky

❌ Failed Tests (1)

  • testThatItDoesSendConversationEventsInATeamConversationWhereYouAreGuest() - WireMockTransportTests - Details

    Expand for error
     XCTAssertEqual failed: ("0") is not equal to ("1") (/Users/admin/actions-runner/_work/wire-ios/wire-ios/wire-ios-mocktransport/Tests/Source/Push%20Events/MockTransportSessionTeamEventsTests.swift#EndingLineNumber=282&StartingLineNumber=282)
    

New Flaky Tests (1)

  • testThatItCallsExpirationHandlerOnCreatedActivities() - WireTransport-ios-tests - Last Failure

    Expand for error
     Asynchronous wait failed: Exceeded timeout of 0.5 seconds, with unfulfilled expectations: "The expiration handler is called.". (/Users/admin/actions-runner/_work/wire-ios/wire-ios/wire-ios-transport/Tests/Source/Background/BackgroundActivityFactoryTests.swift#EndingLineNumber=119&StartingLineNumber=119)
    

@samwyndham
Copy link
Contributor Author

samwyndham commented Dec 11, 2024

🤦 I accidentally merged https://github.com/wireapp/wire-ios/pull/2260 into this instead of waiting for this PR to merge to develop.

@samwyndham samwyndham added this pull request to the merge queue Dec 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 11, 2024
@samwyndham samwyndham added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 12, 2024
@netbe netbe changed the title feat: Certificate pinning in WireAPI - WPB-10450 feat: Certificate pinning and proxy mode in WireAPI - WPB-10450 Dec 12, 2024
@netbe netbe merged commit 9f69bb9 into develop Dec 12, 2024
6 checks passed
@netbe netbe deleted the feat/wire-api-cert-pinning-WPB-10450 branch December 12, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/initiative: ios-sync-refactoring echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt. release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants