-
Notifications
You must be signed in to change notification settings - Fork 217
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
Adding notion of a recovery owner for network recovery #6705
Open
gaurav137
wants to merge
49
commits into
microsoft:main
Choose a base branch
from
gaurav137:dev/gsinha/recovery-owner
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,134
−201
Open
Changes from all commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
07b152f
Update
gaurav137 7cd6011
Update
gaurav137 580670c
Update
gaurav137 4df08bd
Update
gaurav137 491aed2
Update
gaurav137 41e16fd
Update
gaurav137 90cbdfb
Update
gaurav137 1c1bda9
Update
gaurav137 2f7ff7f
Update
gaurav137 7d0fe02
working
gaurav137 607b8a4
Update
gaurav137 986138a
Update
gaurav137 0ae745c
Update
gaurav137 33b736b
Update
gaurav137 96838b7
Update
gaurav137 7104982
Update
gaurav137 7639bf9
Update
gaurav137 8bd2d95
Update
gaurav137 e5021f1
Update
gaurav137 0dcc258
Test failure fixes
gaurav137 e0bc7a7
schema test fix
gaurav137 b28b452
Update samples/constitutions/default/actions.js
gaurav137 8dc3f06
picking main
gaurav137 01df49a
Update
gaurav137 7546e41
Update
gaurav137 754adc3
formatting fixes
gaurav137 123387b
Update
gaurav137 1228c1d
Update
gaurav137 12d965d
Merge branch 'main' into dev/gsinha/recovery-owner
achamayou 32aa899
Compilation fixes post picking latest changes from main
gaurav137 8c931b4
Merge branch 'main' into dev/gsinha/recovery-owner
gaurav137 ff6ba59
Refactoring LedgerSecretWrappingKey
gaurav137 476636f
Merge branch 'dev/gsinha/recovery-owner' of https://github.com/gaurav…
gaurav137 a2f79a9
Merge branch 'main' into dev/gsinha/recovery-owner
gaurav137 cab53eb
clang-format fixes
gaurav137 d5ffb66
Renaming methods to remove ambiguity
gaurav137 5f56142
Merge branch 'main' into dev/gsinha/recovery-owner
gaurav137 02e7df9
Taking comments
gaurav137 8416064
Merge branch 'dev/gsinha/recovery-owner' of https://github.com/gaurav…
gaurav137 f78144c
Updates
gaurav137 ae6f19a
Update
gaurav137 4691ac8
Merge branch 'main' into dev/gsinha/recovery-owner
gaurav137 9b18e9f
Updates
gaurav137 2b90ece
Update
gaurav137 b5c3c6a
Test code updates
gaurav137 d9f1308
Update
gaurav137 9f48285
Taking comments
gaurav137 969aaae
Merge branch 'main' into dev/gsinha/recovery-owner
gaurav137 fd8ee6a
Merge branch 'main' into dev/gsinha/recovery-owner
achamayou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,11 +130,14 @@ namespace ccf::gov::endpoints | |
params["share"].template get<std::string>()); | ||
|
||
size_t submitted_shares_count = 0; | ||
bool full_key_submitted = false; | ||
try | ||
{ | ||
submitted_shares_count = share_manager.submit_recovery_share( | ||
ctx.tx, member_id, raw_recovery_share); | ||
|
||
full_key_submitted = ShareManager::is_full_key(raw_recovery_share); | ||
|
||
OPENSSL_cleanse( | ||
raw_recovery_share.data(), raw_recovery_share.size()); | ||
} | ||
|
@@ -164,8 +167,13 @@ namespace ccf::gov::endpoints | |
submitted_shares_count, | ||
threshold); | ||
|
||
if (submitted_shares_count >= threshold) | ||
if (submitted_shares_count >= threshold || full_key_submitted) | ||
{ | ||
if (full_key_submitted) | ||
{ | ||
message += "\nFull recovery key successfully submitted"; | ||
} | ||
Comment on lines
+172
to
+175
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't add as suggestion because it affects the untouched lines above, but suggest that this is a replacement for the "x/n" message above when Something like:
|
||
|
||
message += "\nEnd of recovery procedure initiated"; | ||
GOV_INFO_FMT("{} - initiating recovery", message); | ||
|
||
|
@@ -196,6 +204,7 @@ namespace ccf::gov::endpoints | |
response_body["message"] = message; | ||
response_body["submittedCount"] = submitted_shares_count; | ||
response_body["recoveryThreshold"] = threshold; | ||
response_body["fullKeySubmitted"] = full_key_submitted; | ||
|
||
ctx.rpc_ctx->set_response_json(response_body, HTTP_STATUS_OK); | ||
return; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a
checkEnum()
that will be slightly more precise (and prevent a much-later error, if C++ code tries to deserialise an unknown/badly-cased string). It doesn't have the?
optional syntax, so I think it would be something like:(I'm not completely sure of the semantics here - should
recovery_role
benull
or"NonParticipant"
?)