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.

Sign up for GitHub

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

Jump to bottom

feat: expose API for upgrading from basic to x509 credentials for a conversation WPB-10161 #714

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

feat: expose API for upgrading from basic to x509 credentials for a conversation WPB-10161 #714

wants to merge 7 commits into from

Conversation

typfel
Copy link
Member

@typfel typfel commented Oct 9, 2024

What's new in this PR

Expose API for upgrading from basic to x509 credentials for a conversation. This is useful for a clients to address the race condition, which can happen if they got added to a conversation with their basic credentials while upgrading to x509 credentials.

PR Submission Checklist for internal contributors
  • The PR Title
    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  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.

@typfel typfel requested a review from a team as a code owner October 9, 2024 12:22
@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.00000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 77.72%. Comparing base (52b94fe) to head (4e39b66).

Files with missing lines Patch % Lines
crypto/src/e2e_identity/rotate.rs 88.60% 9 Missing ⚠️
crypto-ffi/src/generic.rs 0.00% 8 Missing ⚠️
crypto/src/e2e_identity/conversation_state.rs 85.71% 1 Missing ⚠️
crypto/src/mls/conversation/own_commit.rs 83.33% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #714   +/-   ##
=======================================
  Coverage   77.71%   77.72%           
=======================================
  Files          98       98           
  Lines       19153    19245   +92     
=======================================
+ Hits        14885    14958   +73     
- Misses       4268     4287   +19
Files with missing lines Coverage Δ
crypto/src/e2e_identity/conversation_state.rs 82.08% <85.71%> (-0.04%) ⬇️
crypto/src/mls/conversation/own_commit.rs 85.88% <83.33%> (-0.13%) ⬇️
crypto-ffi/src/generic.rs 0.00% <0.00%> (ø)
crypto/src/e2e_identity/rotate.rs 85.52% <88.60%> (+0.22%) ⬆️

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 52b94fe...4e39b66. Read the comment docs.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024 •
edited
Loading

🐰 Bencher Report

Branchfeat/e2ei-rotate-WPB-10161
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
Commit add f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		11,274,000.00
Commit add f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		691,960.00
Commit add f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		2,951,400.00
Commit add f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		5,230,800.00
Commit add f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		7,889,800.00
Commit add f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		9,561,600.00
Commit add f(number clients)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		1,008,300,000.00
Commit add f(number clients)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		817,910.00
Commit add f(number clients)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		78,666,000.00
Commit add f(number clients)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		215,070,000.00
Commit add f(number clients)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		424,900,000.00
Commit add f(number clients)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		686,750,000.00
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		110,080,000.00
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		21,952,000.00
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		39,542,000.00
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		54,050,000.00
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		72,881,000.00
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		87,548,000.00
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
⚠️ NO THRESHOLD		10,919,000.00
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
⚠️ NO THRESHOLD		108,700,000.00
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
⚠️ NO THRESHOLD		27,953,000.00
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
⚠️ NO THRESHOLD		50,206,000.00
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
⚠️ NO THRESHOLD		69,149,000.00
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
⚠️ NO THRESHOLD		87,988,000.00
Commit remove f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		19,215,000.00
Commit remove f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		530,170.00
Commit remove f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		2,235,000.00
Commit remove f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		4,972,100.00
Commit remove f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		9,663,000.00
Commit remove f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		13,834,000.00
Commit remove f(number clients)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		21,277,000.00
Commit remove f(number clients)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		128,630,000.00
Commit remove f(number clients)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		106,560,000.00
Commit remove f(number clients)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		85,087,000.00
Commit remove f(number clients)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		63,456,000.00
Commit remove f(number clients)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		42,607,000.00
Commit update f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		127,900,000.00
Commit update f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		687,670.00
Commit update f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		26,496,000.00
Commit update f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		52,604,000.00
Commit update f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		78,560,000.00
Commit update f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		103,530,000.00
Count KeyPackage/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		3,458,000.00
Count KeyPackage/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		413,560.00
Count KeyPackage/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		756,480.00
Count KeyPackage/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		1,430,700.00
Count KeyPackage/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		2,085,600.00
Count KeyPackage/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		2,793,300.00
Create group/cs1/mem📈 view plot
⚠️ NO THRESHOLD		294,490.00
Decrypt f(msg size)/cs1/mem/10📈 view plot
⚠️ NO THRESHOLD		207,460.00
Decrypt f(msg size)/cs1/mem/10010📈 view plot
⚠️ NO THRESHOLD		275,480.00
Decrypt f(msg size)/cs1/mem/2010📈 view plot
⚠️ NO THRESHOLD		223,010.00
Decrypt f(msg size)/cs1/mem/4010📈 view plot
⚠️ NO THRESHOLD		234,350.00
Decrypt f(msg size)/cs1/mem/6010📈 view plot
⚠️ NO THRESHOLD		260,120.00
Decrypt f(msg size)/cs1/mem/8010📈 view plot
⚠️ NO THRESHOLD		274,510.00
Encrypt f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		988,880.00
Encrypt f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		273,950.00
Encrypt f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		415,480.00
Encrypt f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		568,550.00
Encrypt f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		705,710.00
Encrypt f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		856,500.00
Encrypt f(msg size)/cs1/mem/10📈 view plot
⚠️ NO THRESHOLD		934,440.00
Encrypt f(msg size)/cs1/mem/10010📈 view plot
⚠️ NO THRESHOLD		990,980.00
Encrypt f(msg size)/cs1/mem/2010📈 view plot
⚠️ NO THRESHOLD		939,910.00
Encrypt f(msg size)/cs1/mem/4010📈 view plot
⚠️ NO THRESHOLD		950,900.00
Encrypt f(msg size)/cs1/mem/6010📈 view plot
⚠️ NO THRESHOLD		985,450.00
Encrypt f(msg size)/cs1/mem/8010📈 view plot
⚠️ NO THRESHOLD		1,013,700.00
Generate KeyPackage f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		191,780,000.00
Generate KeyPackage f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		498,080.00
Generate KeyPackage f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		22,138,000.00
Generate KeyPackage f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		64,250,000.00
Generate KeyPackage f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		110,690,000.00
Generate KeyPackage f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		149,150,000.00
Join from external commit f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		235,010,000.00
Join from external commit f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		1,398,300.00
Join from external commit f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		48,450,000.00
Join from external commit f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		95,318,000.00
Join from external commit f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		143,740,000.00
Join from external commit f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		188,940,000.00
Join from welcome f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		110,460,000.00
Join from welcome f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		917,880.00
Join from welcome f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		22,964,000.00
Join from welcome f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		44,883,000.00
Join from welcome f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		67,028,000.00
Join from welcome f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		88,818,000.00
Mls vs Proteus: add/MLS/mem/1📈 view plot
⚠️ NO THRESHOLD		703,520.00
Mls vs Proteus: add/MLS/mem/101📈 view plot
⚠️ NO THRESHOLD		1,801,800.00
Mls vs Proteus: add/MLS/mem/21📈 view plot
⚠️ NO THRESHOLD		940,170.00
Mls vs Proteus: add/MLS/mem/41📈 view plot
⚠️ NO THRESHOLD		1,172,100.00
Mls vs Proteus: add/MLS/mem/61📈 view plot
⚠️ NO THRESHOLD		1,320,400.00
Mls vs Proteus: add/MLS/mem/81📈 view plot
⚠️ NO THRESHOLD		1,655,900.00
Mls vs Proteus: add/Proteus/mem/1📈 view plot
⚠️ NO THRESHOLD		427,130.00
Mls vs Proteus: add/Proteus/mem/101📈 view plot
⚠️ NO THRESHOLD		34,935,000.00
Mls vs Proteus: add/Proteus/mem/21📈 view plot
⚠️ NO THRESHOLD		7,331,400.00
Mls vs Proteus: add/Proteus/mem/41📈 view plot
⚠️ NO THRESHOLD		14,217,000.00
Mls vs Proteus: add/Proteus/mem/61📈 view plot
⚠️ NO THRESHOLD		21,095,000.00
Mls vs Proteus: add/Proteus/mem/81📈 view plot
⚠️ NO THRESHOLD		28,028,000.00
Mls vs Proteus: encrypt/MLS/mem/1📈 view plot
⚠️ NO THRESHOLD		261,450.00
Mls vs Proteus: encrypt/MLS/mem/101📈 view plot
⚠️ NO THRESHOLD		325,570.00
Mls vs Proteus: encrypt/MLS/mem/21📈 view plot
⚠️ NO THRESHOLD		258,500.00
Mls vs Proteus: encrypt/MLS/mem/41📈 view plot
⚠️ NO THRESHOLD		281,450.00
Mls vs Proteus: encrypt/MLS/mem/61📈 view plot
⚠️ NO THRESHOLD		302,240.00
Mls vs Proteus: encrypt/MLS/mem/81📈 view plot
⚠️ NO THRESHOLD		315,340.00
Mls vs Proteus: encrypt/Proteus/mem/1📈 view plot
⚠️ NO THRESHOLD		178,470.00
Mls vs Proteus: encrypt/Proteus/mem/101📈 view plot
⚠️ NO THRESHOLD		8,226,000.00
Mls vs Proteus: encrypt/Proteus/mem/21📈 view plot
⚠️ NO THRESHOLD		1,808,100.00
Mls vs Proteus: encrypt/Proteus/mem/41📈 view plot
⚠️ NO THRESHOLD		3,410,300.00
Mls vs Proteus: encrypt/Proteus/mem/61📈 view plot
⚠️ NO THRESHOLD		5,011,100.00
Mls vs Proteus: encrypt/Proteus/mem/81📈 view plot
⚠️ NO THRESHOLD		6,837,700.00
Mls vs Proteus: remove/MLS/mem/1📈 view plot
⚠️ NO THRESHOLD		13,749,000.00
Mls vs Proteus: remove/MLS/mem/101📈 view plot
⚠️ NO THRESHOLD		1,736,700.00
Mls vs Proteus: remove/MLS/mem/21📈 view plot
⚠️ NO THRESHOLD		11,282,000.00
Mls vs Proteus: remove/MLS/mem/41📈 view plot
⚠️ NO THRESHOLD		8,895,800.00
Mls vs Proteus: remove/MLS/mem/61📈 view plot
⚠️ NO THRESHOLD		6,510,400.00
Mls vs Proteus: remove/MLS/mem/81📈 view plot
⚠️ NO THRESHOLD		4,105,900.00
Mls vs Proteus: remove/Proteus/mem/1📈 view plot
⚠️ NO THRESHOLD		90,292.00
Mls vs Proteus: remove/Proteus/mem/101📈 view plot
⚠️ NO THRESHOLD		569,100.00
Mls vs Proteus: remove/Proteus/mem/21📈 view plot
⚠️ NO THRESHOLD		186,940.00
Mls vs Proteus: remove/Proteus/mem/41📈 view plot
⚠️ NO THRESHOLD		280,710.00
Mls vs Proteus: remove/Proteus/mem/61📈 view plot
⚠️ NO THRESHOLD		381,930.00
Mls vs Proteus: remove/Proteus/mem/81📈 view plot
⚠️ NO THRESHOLD		474,600.00
Mls vs Proteus: update/MLS/mem/1📈 view plot
⚠️ NO THRESHOLD		699,890.00
Mls vs Proteus: update/MLS/mem/101📈 view plot
⚠️ NO THRESHOLD		13,723,000.00
Mls vs Proteus: update/MLS/mem/21📈 view plot
⚠️ NO THRESHOLD		3,461,600.00
Mls vs Proteus: update/MLS/mem/41📈 view plot
⚠️ NO THRESHOLD		6,059,800.00
Mls vs Proteus: update/MLS/mem/61📈 view plot
⚠️ NO THRESHOLD		8,569,200.00
Mls vs Proteus: update/MLS/mem/81📈 view plot
⚠️ NO THRESHOLD		11,303,000.00
Mls vs Proteus: update/Proteus/mem/1📈 view plot
⚠️ NO THRESHOLD		453,480.00
Mls vs Proteus: update/Proteus/mem/101📈 view plot
⚠️ NO THRESHOLD		36,014,000.00
Mls vs Proteus: update/Proteus/mem/21📈 view plot
⚠️ NO THRESHOLD		7,472,300.00
Mls vs Proteus: update/Proteus/mem/41📈 view plot
⚠️ NO THRESHOLD		14,506,000.00
Mls vs Proteus: update/Proteus/mem/61📈 view plot
⚠️ NO THRESHOLD		21,448,000.00
Mls vs Proteus: update/Proteus/mem/81📈 view plot
⚠️ NO THRESHOLD		28,413,000.00
🐰 View full continuous benchmarking report in Bencher

@istankovic
Copy link
Member

Could you please distribute formatting-related hunks from 06b5bce to commits that originally touched that code?

SimonThormeyer
SimonThormeyer approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@SimonThormeyer SimonThormeyer left a comment

Choose a reason for hiding this comment

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

Just a nitpick, otherwise it looks good!

@echoes-hq echoes-hq bot added echoes: bugs Technical or functional defects in the product echoes: features End-user visible changes intended to create customer value echoes/initiative: federation-wire-cloud Activate Federation with MLS on Wire Cloud and removed echoes: bugs Technical or functional defects in the product labels Oct 10, 2024
istankovic
istankovic reviewed Oct 10, 2024
View reviewed changes
crypto/src/e2e_identity/rotate.rs
init_count.epoch_encryption_keypair,
final_count.epoch_encryption_keypair
);
assert_eq!(init_count.key_package, final_count.key_package);
Copy link
Member

Choose a reason for hiding this comment

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

This means a search for a basic credential key package would fail now, right?

Copy link
Member Author

@typfel typfel Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean this count doesn't distinguish. In this test the key packages aren't touched. 10 key packages are generated when the centrals are created but that's it.

Honestly now that I'm looking at it I don't see the point of asserting that the key_package or epoch_encryption_keypair count hasn't changed.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, then I would omit those checks in order to avoid confusion. 👍

@istankovic
Copy link
Member

The hunk(s) modifying crypto-ffi/src/generic.rs in commit 961e61a should really be incorporated into commit b8b5ef9, which introduces that code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@istankovic istankovic istankovic left review comments

@SimonThormeyer SimonThormeyer SimonThormeyer approved these changes

Assignees
No one assigned
Labels
echoes: features End-user visible changes intended to create customer value echoes/initiative: federation-wire-cloud Activate Federation with MLS on Wire Cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@typfel @codecov-commenter @istankovic @SimonThormeyer