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

fix: failure fetching federated certificate chain #2660

Merged

Conversation

vitorhugods
Copy link
Member


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Scenario

Given I am a User with E2EI enabled
And I have MLS conversations with federated users
And I get a E2EI certificate during login
When performing Slow Sync

What's happening

It fails during JoinExternalMLSConversations and ResolveOneOnOneConversations because CoreCrypto can't validate the identity of some users.
I get completely stuck in "Waiting for Connection"

What should happen

It should join all existing MLS conversations and resolve one on ones.
I should not get stuck

Causes

  1. We were not fetching the federation certificate chain and registering with CoreCrypto when enrolling into E2EI (only on a daily basis)
  2. We were not properly parsing the certificate list and passing one at a time to CoreCrypto. We were just passing the whole blob to it.

Solutions

  1. Call fetchFederationCertificates during E2EI enrollment
  2. Parse the certificates and pass them one at a time to CoreCrypto.

Testing

Test Coverage

  • I have added automated test to this contribution

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@vitorhugods vitorhugods enabled auto-merge (squash) March 14, 2024 22:04
Copy link
Contributor

github-actions bot commented Mar 14, 2024

Test Results

2 840 tests   2 711 ✔️  24s ⏱️
   485 suites     129 💤
   485 files           0

Results for commit 41f98cb.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Mar 14, 2024

Datadog Report

Branch report: fix/failure-fetching-federated-certificate-chain
Commit report: 4f04af4
Test service: kalium-jvm

✅ 0 Failed, 2841 Passed, 123 Skipped, 8m 41s Wall Time

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 66.66667% with 8 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (release/candidate@21dc44c). Click here to learn what that means.

Additional details and impacted files
@@                 Coverage Diff                  @@
##             release/candidate    #2660   +/-   ##
====================================================
  Coverage                     ?   58.35%           
  Complexity                   ?        7           
====================================================
  Files                        ?     1176           
  Lines                        ?    46160           
  Branches                     ?     4380           
====================================================
  Hits                         ?    26935           
  Misses                       ?    17264           
  Partials                     ?     1961           
Files Coverage Δ
.../com/wire/kalium/logic/sync/slow/SlowSyncWorker.kt 96.36% <100.00%> (ø)
...re/kalium/network/api/base/unbound/acme/ACMEApi.kt 25.95% <66.66%> (ø)
.../com/wire/kalium/logic/data/e2ei/E2EIRepository.kt 78.84% <83.33%> (ø)
...um/logic/feature/e2ei/usecase/EnrollE2EIUseCase.kt 92.59% <33.33%> (ø)
...lium/network/api/base/unbound/acme/ACMEResponse.kt 41.66% <40.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21dc44c...41f98cb. Read the comment docs.

@vitorhugods vitorhugods merged commit cccff38 into release/candidate Mar 14, 2024
17 checks passed
@vitorhugods vitorhugods deleted the fix/failure-fetching-federated-certificate-chain branch March 14, 2024 22:48
github-merge-queue bot pushed a commit that referenced this pull request Mar 15, 2024
Co-authored-by: Vitor Hugo Schwaab <[email protected]>
Co-authored-by: Oussama Hassine <[email protected]>
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.

4 participants