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: Add forward-secrecy to ephemeral keys #5158

Merged
merged 6 commits into from Feb 16, 2023

Conversation

peternose
Copy link
Contributor

@peternose peternose commented Jan 31, 2023

Task

See ADR21 for details. Changes:

  • When generating a secret (point 2) the enclave doesn't check if the secret has been published as this doesn't prevent multiple enclaves publishing a secret at the same time.
  • I prefer naming the entropy ephemeral secret to emphasize that it needs to be kept secret and to be in-line with master secret. Can change if we are planning to derive multiple secrets from the entropy.

TODO: Discuss/solve upgrade procedure.

Tests

No need to introduce a new feature as the new key manager node will silently ignore errors when sending new RPC requests to an outdated enclave.

@peternose peternose force-pushed the peternose/feature/ephemeral-keys-forward-secrecy branch 2 times, most recently from ef71046 to e7f6b5a Compare January 31, 2023 18:47
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #5158 (badd3c9) into master (dd8b3c1) will decrease coverage by 0.02%.
The diff coverage is 56.68%.

@@            Coverage Diff             @@
##           master    #5158      +/-   ##
==========================================
- Coverage   61.41%   61.40%   -0.02%     
==========================================
  Files         511      512       +1     
  Lines       53463    53980     +517     
==========================================
+ Hits        32836    33146     +310     
- Misses      16448    16603     +155     
- Partials     4179     4231      +52     
Impacted Files Coverage Δ
go/keymanager/api/policy_sgx.go 33.33% <ø> (ø)
go/p2p/rpc/client.go 71.61% <0.00%> (-0.64%) ⬇️
go/registry/api/api.go 52.94% <ø> (ø)
go/runtime/enclaverpc/api/api.go 0.00% <ø> (ø)
go/worker/keymanager/api/api.go 0.00% <ø> (ø)
go/worker/keymanager/status.go 0.00% <0.00%> (ø)
go/keymanager/api/grpc.go 18.11% <17.97%> (-0.32%) ⬇️
go/beacon/api/api.go 75.00% <50.00%> (-25.00%) ⬇️
...consensus/tendermint/apps/keymanager/keymanager.go 66.83% <50.00%> (+1.59%) ⬆️
go/keymanager/api/secret.go 57.89% <57.89%> (ø)
... and 22 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 marked this pull request as ready for review January 31, 2023 23:06
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.

We should probably amend the key manager node status (as exposed via e.g. control status) to include the status of ephemeral secret replication so that node operators can easily confirm whether the key manager node is fully operational.

go/consensus/tendermint/apps/keymanager/state/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/keymanager/transactions.go Outdated Show resolved Hide resolved
go/registry/api/api.go Outdated Show resolved Hide resolved
go/worker/keymanager/worker.go Show resolved Hide resolved
go/worker/keymanager/worker.go Show resolved Hide resolved
keymanager/src/crypto/kdf.rs Show resolved Hide resolved
keymanager/src/crypto/kdf.rs Show resolved Hide resolved
keymanager/src/runtime/methods.rs Outdated Show resolved Hide resolved
@peternose peternose force-pushed the peternose/feature/ephemeral-keys-forward-secrecy branch 4 times, most recently from f5cf32e to 13f602f Compare February 11, 2023 14:20
@peternose peternose force-pushed the peternose/feature/ephemeral-keys-forward-secrecy branch 2 times, most recently from cdcbd4b to 44fcba1 Compare February 14, 2023 09:19
Renamed MasterSecret so that it can be used for ephemeral secrets also.
Refactored kdf so that keys can be derived from an arbitrary secret.
Moved init_kdf function to methods.rs.
Refactored key manager methods so that context is the first parameter.
The RPC client retries RPC calls if they fail. This was also true if
the peer list was empty, introducing additional latencies.
@peternose peternose force-pushed the peternose/feature/ephemeral-keys-forward-secrecy branch from 44fcba1 to b3254c1 Compare February 16, 2023 09:06
@peternose peternose force-pushed the peternose/feature/ephemeral-keys-forward-secrecy branch 2 times, most recently from e612b99 to dec4a75 Compare February 16, 2023 12:11
Deriving ephemeral keys from the key manager's master secret did not guarantee
forward secrecy. In order to fulfill this requirement, we needed ephemeral
secrets that are randomly generated on every epoch and distributed securely
amongst enclave executors.
@peternose peternose force-pushed the peternose/feature/ephemeral-keys-forward-secrecy branch from dec4a75 to badd3c9 Compare February 16, 2023 14:44
@peternose peternose merged commit ed3cfb6 into master Feb 16, 2023
@peternose peternose deleted the peternose/feature/ephemeral-keys-forward-secrecy branch February 16, 2023 15:27
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

2 participants