hsmd: make get_per_commitment_point unconditionally safe by not returning secret #7178
Conversation
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
|
The short-term fix for the VLS crash is merged on our side as in https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/643 The matching "clean fix" on the VLS side to be matched w/ the last two commits of this PR is https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/641 |
| @@ -434,7 +434,7 @@ static struct io_plan *init_hsm(struct io_conn *conn, | |||
| struct secret *hsm_encryption_key; | |||
| struct bip32_key_version bip32_key_version; | |||
| u32 minversion, maxversion; | |||
| const u32 our_minversion = 4, our_maxversion = 5; | |||
| const u32 our_minversion = 4, our_maxversion = 6; | |||
There was a problem hiding this comment.
I am still thinking that this value should be equal to the constant values defined inside the common/hsm_version.h, but this is an off the book comment
|
Ack 1b98ea8 |
Changelog-None: shouldn't affect others HSM_MIN_VERSION is 5 which implies use of WIRE_HSMD_REVOKE_COMMITMENT_TX; prune branches that can't happen.
Changelog-None: internal to channeld Since we don't need a special path for early old_secrets from validate we can factor out duplicate code.
Changelog-None: channeld internal This factoring makes it much clearer which callers only need the pubkey and which only need the old_secret. VLS has merged a workaround which prevents crashing when fetching a per-commitment-point beyond the allowed range (the secret is just not returned in this case. https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/643 In HSM_VERSION 6 the semantic is cleaned up; get_per_commitment_point never returns a secret and safely be called on any commitment number.
Changelog-None: hsmd internals
Changelog-Changed: hsmd: HSM_VERSION 6: get_per_commitment_point does not imply index - 2 is revoked, makes it safe to call on any index.
Changelog-Changed: hsmd: the hsmd now supports HSM_VERSION 6 This is actually optional, everything would be ok leaving native hsmd support at HSM_VERSION 5 instead.
ksedgwic
force-pushed
the
hsmd-6-get-per-commit-point-no-secret
branch
from
1b98ea8
to
e1807b0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Removes returned old_secret from get_per_commitment_point making it safe to call on all commits
Motivation: When get_per_commitment_point returns the old secret it implies a revocation of index - 2. This causes a crash when VLS has validated a commitment but not seen the following revoke of the prior and the channel is asynchronously restarted. See https://gitlab.com/lightning-signer/validating-lightning-signer/-/issues/469
The last two commits in this PR are somewhat optional: by increasing the HSM_VERSION to 6 we can inform the signer that it should never return an old_secret (and therefore can't fail). The native hsmd implementation doesn't have the safety issue and it would be ok to leave it at HSM_VERSION 5. But it's cleaner to have consistent semantics.
All but the last two commits in the PR sequence are cleanup and reduction refactoring and worth considering even if an HSM_VERSION change is not considered.
Cleanup and refactoring Includes: