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

Add support for ephemeral keys in key manager #4888

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

peternose
Copy link
Contributor

Add API methods like the following:

  • get_ephemeral_key(epoch, key_pair_id)
  • get_public_ephemeral_key(epoch, key_pair_id)

Those would derive special ephemeral X25519 key pairs separate from the other runtime keys and where the epoch number is used as part of the derivation, but would only derive them if the epoch was not too far back in the past based on consensus layer state as seen by the key manager. Initially the epoch check may rely on untrusted consensus state provided by the key manager host node.

@peternose peternose changed the title Peternose/feature/ephemeral keys Add support for ephemeral keys in key manager Aug 11, 2022
@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from 8146848 to 84d35ac Compare August 12, 2022 09:27
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Some early comments on the current draft.

keymanager-api-common/src/api.rs Show resolved Hide resolved
keymanager-api-common/src/api.rs Outdated Show resolved Hide resolved
keymanager-api-common/src/api.rs Outdated Show resolved Hide resolved
keymanager-lib/src/methods.rs Outdated Show resolved Hide resolved
@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from 84d35ac to f90aa5f Compare August 12, 2022 12:23
@codecov
Copy link

codecov bot commented Aug 13, 2022

Codecov Report

Merging #4888 (d80f77d) into master (7fa7883) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

❗ Current head d80f77d differs from pull request most recent head 4102e8e. Consider uploading reports for the commit 4102e8e to get more accurate results

@@            Coverage Diff             @@
##           master    #4888      +/-   ##
==========================================
- Coverage   66.60%   66.34%   -0.27%     
==========================================
  Files         464      464              
  Lines       50977    50977              
==========================================
- Hits        33954    33820     -134     
- Misses      12833    12955     +122     
- Partials     4190     4202      +12     
Impacted Files Coverage Δ
go/worker/keymanager/worker.go 65.31% <100.00%> (+0.21%) ⬆️
go/worker/keymanager/status.go 0.00% <0.00%> (-82.61%) ⬇️
go/worker/keymanager/api/api.go 0.00% <0.00%> (-32.44%) ⬇️
go/consensus/tendermint/apps/staking/auth.go 62.96% <0.00%> (-14.82%) ⬇️
go/registry/api/grpc.go 38.66% <0.00%> (-6.40%) ⬇️
go/beacon/api/grpc.go 21.71% <0.00%> (-6.29%) ⬇️
go/consensus/tendermint/abci/prune.go 76.78% <0.00%> (-6.25%) ⬇️
go/worker/common/p2p/dispatch.go 71.52% <0.00%> (-5.56%) ⬇️
go/storage/api/metrics.go 76.66% <0.00%> (-5.00%) ⬇️
.../worker/compute/executor/committee/transactions.go 84.09% <0.00%> (-4.55%) ⬇️
... and 34 more

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

@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch 16 times, most recently from 5c952ac to af18566 Compare August 16, 2022 09:41
@peternose peternose marked this pull request as ready for review August 16, 2022 10:16
@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from a50d16b to 7a6194e Compare August 18, 2022 07:58
@peternose peternose requested a review from kostko August 18, 2022 08:28
@kostko
Copy link
Member

kostko commented Aug 19, 2022

Given that #4806 has been merged, please rebase on master.

@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from 7a6194e to c8713bf Compare August 19, 2022 07:39
Copy link
Contributor

@Yawning Yawning left a comment

Choose a reason for hiding this comment

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

My inner tinfoil hat says that the master secret used to derive ephemeral keys should be distinct and have nothing to do with the master secret used to derive the long term keys.

(And yes, I am aware that the current way of doing things is fine as long as KMAC is.)

tests/runtimes/simple-keyvalue/src/methods.rs Show resolved Hide resolved
@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from c8713bf to 24c061d Compare August 24, 2022 17:04
@Yawning
Copy link
Contributor

Yawning commented Aug 25, 2022

Should we generate test vectors for the KDF so we don't inadvertently break it? (Since it's deterministic this should be possible)

@kostko
Copy link
Member

kostko commented Aug 25, 2022

Yeah that's a good idea. They should probably be added as Rust unit tests for the KDF.

@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from 6c710da to f4cfbf3 Compare August 25, 2022 13:06
@peternose peternose requested a review from Yawning August 25, 2022 16:15
@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch 2 times, most recently from 2e028e0 to d80f77d Compare August 26, 2022 22:51
@peternose peternose force-pushed the peternose/feature/ephemeral-keys branch from d80f77d to 4102e8e Compare August 31, 2022 19:38
@peternose peternose merged commit 15c1d95 into master Sep 1, 2022
@peternose peternose deleted the peternose/feature/ephemeral-keys branch September 1, 2022 11:50
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.

None yet

3 participants