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

keymanager/src/runtime: Verify and modify init request #5204

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

peternose
Copy link
Contributor

The init request was never verified against the consensus layer state and, therefore, was not trustworthy. To make this request more informative and easily verifiable against consensus, it was extended to include all key manager status fields.

@peternose peternose marked this pull request as ready for review February 27, 2023 23:56
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #5204 (10c1b48) into master (cf5e508) will decrease coverage by 0.14%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #5204      +/-   ##
==========================================
- Coverage   61.47%   61.33%   -0.14%     
==========================================
  Files         512      512              
  Lines       54051    54246     +195     
==========================================
+ Hits        33227    33273      +46     
- Misses      16611    16736     +125     
- Partials     4213     4237      +24     
Impacted Files Coverage Δ
go/keymanager/api/api.go 73.43% <ø> (ø)
go/registry/api/api.go 52.78% <0.00%> (-0.16%) ⬇️
go/runtime/host/protocol/types.go 54.54% <ø> (ø)
go/worker/compute/executor/committee/node.go 59.01% <22.22%> (-0.16%) ⬇️
go/runtime/host/multi/multi.go 45.52% <24.80%> (-21.38%) ⬇️
go/worker/keymanager/worker.go 63.34% <33.33%> (+1.96%) ⬆️
go/worker/common/committee/node.go 57.80% <40.00%> (-0.14%) ⬇️
go/runtime/host/sandbox/sandbox.go 56.65% <50.00%> (-0.20%) ⬇️
go/runtime/registry/host.go 49.50% <66.66%> (-0.20%) ⬇️
go/keymanager/api/secret.go 76.31% <78.57%> (+18.42%) ⬆️
... and 38 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -346,6 +349,7 @@ impl Default for Features {
schedule_control: None,
key_manager_quote_policy_updates: true,
key_manager_status_updates: true,
key_manager_master_secret_rotation: false,
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is false by default because it only makes sense for key manager runtimes? Otherwise it would make sense to have it default to true or introduce a different way of querying for key manager features (e.g. a separate local RPC for that) -- not sure if the complexity is worth it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this is false by default because it only makes sense for key manager runtimes?

True. As key manager is a runtime, I thought that I could use these features.

The init request was never verified against the consensus layer state and,
therefore, was not trustworthy. To make this request more informative and
easily verifiable against consensus, it was extended to include all key
manager status fields.
@peternose peternose force-pushed the peternose/feature/km_init_status branch from 3ed2036 to 10c1b48 Compare February 28, 2023 08:51
@peternose peternose merged commit 91bd6f8 into master Feb 28, 2023
@peternose peternose deleted the peternose/feature/km_init_status branch February 28, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants