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 #2661

Merged

Conversation

github-actions[bot]
Copy link
Contributor

Cherry pick from the original PR:


⚠️ Conflicts during cherry-pick:


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 and 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 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. .

@github-actions github-actions bot added the cherry-pick PR is cherry-picking changes from another banch label Mar 14, 2024
Copy link
Contributor Author

github-actions bot commented Mar 15, 2024

Test Results

2 981 tests  +1   2 860 ✔️ +1   2m 41s ⏱️ -1s
   516 suites ±0      121 💤 ±0 
   516 files   ±0          0 ±0 

Results for commit 5bf4a28. ± Comparison against base commit bb07ad1.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
com.wire.kalium.logic.data.e2ei.E2EIRepositoryTest ‑ givenACMEFederationApiSucceed_whenFetchACMECertificates_thenItSucceed[jvm]
com.wire.kalium.api.common.ACMEApiTest ‑ givingASuccessfulResponse_whenGettingACMEFederationCertificateChain_thenAllCertificatesShouldBeParsed[jvm]
com.wire.kalium.logic.data.e2ei.E2EIRepositoryTest ‑ givenACMEFederationApiSucceeds_whenFetchACMECertificates_thenAllCertificatesAreRegistered[jvm]

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Mar 15, 2024

Datadog Report

All test runs 7555e12 🔗

2 Total Test Services: 0 Failed, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Wall Time Test Service View
kalium-ios 0 0 0 2280 51 4m 39.1s Link
kalium-jvm 0 0 0 2860 121 9m 8.05s Link

@ohassine ohassine added this pull request to the merge queue Mar 15, 2024
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 58.74%. Comparing base (bb07ad1) to head (5bf4a28).

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2661      +/-   ##
=============================================
- Coverage      58.74%   58.74%   -0.01%     
  Complexity         7        7              
=============================================
  Files           1189     1189              
  Lines          46287    46300      +13     
  Branches        4376     4377       +1     
=============================================
+ Hits           27192    27199       +7     
- Misses         17145    17148       +3     
- Partials        1950     1953       +3     
Files Coverage Δ
...re/kalium/network/api/base/unbound/acme/ACMEApi.kt 25.95% <66.66%> (+2.87%) ⬆️
.../com/wire/kalium/logic/data/e2ei/E2EIRepository.kt 78.84% <83.33%> (+0.41%) ⬆️
...um/logic/feature/e2ei/usecase/EnrollE2EIUseCase.kt 92.59% <33.33%> (-1.70%) ⬇️
...lium/network/api/base/unbound/acme/ACMEResponse.kt 41.66% <40.00%> (-0.13%) ⬇️

... and 2 files with indirect coverage changes


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 bb07ad1...5bf4a28. Read the comment docs.

Merged via the queue into develop with commit 762d34b Mar 15, 2024
17 checks passed
@ohassine ohassine deleted the fix/failure-fetching-federated-certificate-chain-cherry-pick branch March 15, 2024 08:59
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