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/churp: Fetch key shares and recover key #5715

Merged
merged 12 commits into from
Jul 9, 2024

Conversation

peternose
Copy link
Contributor

@peternose peternose commented Jun 5, 2024

Updates the key manager client to fetch key shares from shareholders and reconstruct the secret key, from which the state key can be derived.

Copy link

netlify bot commented Jun 5, 2024

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit 8812ba2
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-oasis-core/deploys/668d34175dd34100080d3af7

@peternose peternose force-pushed the peternose/feature/churp-key-shares branch 2 times, most recently from fc936de to 46befe0 Compare June 5, 2024 08:56
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 70.37037% with 16 lines in your changes missing coverage. Please review.

Project coverage is 65.49%. Comparing base (7caaced) to head (ff9ad93).
Report is 14 commits behind head on master.

Files Patch % Lines
go/consensus/cometbft/apps/keymanager/churp/txs.go 38.46% 4 Missing and 4 partials ⚠️
go/worker/keymanager/churp.go 80.00% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5715      +/-   ##
==========================================
+ Coverage   65.03%   65.49%   +0.45%     
==========================================
  Files         619      620       +1     
  Lines       63248    63308      +60     
==========================================
+ Hits        41134    41464     +330     
+ Misses      17292    16968     -324     
- Partials     4822     4876      +54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peternose peternose marked this pull request as ready for review June 5, 2024 09:24
go/oasis-test-runner/scenario/e2e/runtime/test_client.go Outdated Show resolved Hide resolved
go/worker/keymanager/churp.go Outdated Show resolved Hide resolved
keymanager/src/client/remote.rs Outdated Show resolved Hide resolved
keymanager/src/client/remote.rs Outdated Show resolved Hide resolved
keymanager/src/client/remote.rs Outdated Show resolved Hide resolved
keymanager/src/client/remote.rs Outdated Show resolved Hide resolved
runtime/src/consensus/state/beacon.rs Outdated Show resolved Hide resolved
@peternose peternose force-pushed the peternose/feature/churp-key-shares branch 4 times, most recently from 40a9928 to 81dfcd7 Compare June 28, 2024 15:13
@kostko kostko mentioned this pull request Jul 3, 2024
@peternose peternose force-pushed the peternose/feature/churp-key-shares branch 2 times, most recently from d99ccf3 to 486a70a Compare July 8, 2024 14:09
@peternose peternose requested a review from kostko July 8, 2024 14:39
go/worker/keymanager/churp.go Outdated Show resolved Hide resolved
keymanager/src/client/remote.rs Show resolved Hide resolved
keymanager/src/client/remote.rs Show resolved Hide resolved
tests/runtimes/simple-keyvalue/src/methods.rs Outdated Show resolved Hide resolved
go/worker/keymanager/churp.go Outdated Show resolved Hide resolved
secret-sharing/src/suites/p384.rs Show resolved Hide resolved
@@ -7,10 +7,6 @@ use p384::{

use super::{FieldDigest, GroupDigest};

/// Domain separation tag for encoding to NIST P-384 prime field or curve
/// using the SHA3-384 hash function.
const NIST_P384_SHA3_384_ENC_DST: &[u8] = b"P384_XMD:SHA3-384_SSWU_RO_";
Copy link
Member

Choose a reason for hiding this comment

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

Will not using standard DSTs make it harder for third-party implementations?

Copy link
Member

Choose a reason for hiding this comment

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

I guess if DSTs don't use something like length-prefix encoding for the components (e.g. like TupleHash), then it doesn't make it any easier as soon as there are custom DSTs. So in this case it is fine and this can be ignored.

Copy link
Contributor Author

@peternose peternose Jul 9, 2024

Choose a reason for hiding this comment

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

By harder for third party implementations you mean that it would be great to have standard dst in the package so that third party apps can easily construct full dst?

IEFT draft hashing to elliptic curves suggests name <app>-V<xx>-CS<yy>-<suiteID>-<enc> where <xx> and <yy> are two-digit numbers indicating the version and ciphersuite, respectively, and <suiteID> is the Suite ID of the encoding used in ciphersuite <yy>. I'm not sure what <yy> should be, but I guess in our case the dst should be oasis-core/keymanager/churp: V00-P384_XMD:SHA3-384_SSWU_RO_-<runtime_id>-<churp_id>-shareholder or CHURP-V00-P384_XMD:SHA3-384_SSWU_RO_-<runtime_id>-<churp_id>-shareholder. However, if we want to be consistent with our signature ctx, the tag should be oasis-core/keymanager/churp: sharehoder for runtime <id> for churp <id>. Not sure what is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

length-prefix encoding for the components

Tested locally, and I get the same result. I also checked the code, and length prefix or suffix (not sure what) is added only to the full dst (concatenated components).

@peternose peternose force-pushed the peternose/feature/churp-key-shares branch from 0a9f1d6 to 8812ba2 Compare July 9, 2024 12:59
@peternose peternose enabled auto-merge July 9, 2024 13:18
@peternose peternose merged commit ab5b0b7 into master Jul 9, 2024
3 of 5 checks passed
@peternose peternose deleted the peternose/feature/churp-key-shares branch July 9, 2024 13:34
@peternose peternose linked an issue Jul 11, 2024 that may be closed by this pull request
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.

CHURP key derivation
2 participants