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

Sign dht record with scAddr #1810

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Sign dht record with scAddr #1810

merged 4 commits into from
Oct 11, 2023

Conversation

bitwiseguy
Copy link
Contributor

Closes #1509

The implementation in this PR uses the following flow to sign the dht record with the state-channel address:

  1. messageservice sends signatureRequest on a new channel (requests includes a dedicated response channel)
  2. New case statement on engine's run loop listens to that channel
  3. engine processes the signatureRequest by retrieving the secretKey from the store, then returns the signature on the dedicated response channel

This implementation (especially the use of a one-time, dedicated response channel instead of a simple function call and return value) feels a bit hacky to me. However, it seemed like the best way to perform signatures while keeping the messageservice decoupled from the engine and store.

@netlify
Copy link

netlify bot commented Oct 5, 2023

👷 Deploy Preview for nitrodocs processing.

Name Link
🔨 Latest commit 6be805d
🔍 Latest deploy log https://app.netlify.com/sites/nitrodocs/deploys/6526b2317839b00009c73bdb

Comment on lines +12 to +13
// SignRequests returns a chan for receiving signature requests from the message service
SignRequests() <-chan p2pms.SignatureRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the general messageservice interface to depend on a type from the p2pms package? Maybe we should hoist this type up to this package, instead? That would also prevent test-messageservice depending on p2pms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few conflicting things related to the changes in this PR. I am happy to make the change you suggested, but I want to explain some of the finer details first to give the full picture of this implementation:

The p2pms.SignatureRequest struct is specifically for a DHT record that needs to be signed by the private key held in the store. I wanted to use a strongly typed struct so that we restrict what the private key is allowed to sign (instead of just arbitrarily signing byte strings). The test-messageservice does not use a DHT. However, I had to add the SignRequests method to the MessageService interface to allow the engine to read from that channel as part of its run routine. So the test-messageservice implements that method, but the method never gets used. We can hoist the p2pms.SignatureRequest, but it is a struct that is specific to the p2p-message-service and its use of customized DHT records.

fromChain <-chan chainservice.Event
fromMsg <-chan protocols.Message
fromLedger chan consensus_channel.Proposal
signRequests <-chan p2pms.SignatureRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we will be able to reuse this interface to solve these issues:
#1508
#1075

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue 1508

We will likely need to add another config option for the libp2p peerId private key (which is used to derive the peerId). We need to pass that private key to the libp2p config options in the following code:

	options := []libp2p.Option{
		libp2p.Identity(privateKey),
		...
	}

Right now the messageservice holds that private key in-memory. To improve consistency, we could put that private key in the store (as we do with the scAddr). We could then update the engine.handleSignRequests method to make both signatures on the DHT record:

  1. Sign the record using the scAddr private key
  2. Sign the record using the peerId private key
  3. Return both signatures to the messageservice so that the record can be published to the DHT

Issue 1075

I'm not sure how to resolve this issue. It seems that libp2p requires the peerId private key during its configuration, and the key is used within libp2p abstractions to sign DHT records and encrypt p2p communication. By using separate keys for the peerId and scAddr (as discussed above in issue 1508), we would at least mitigate risk by reducing responsibilities/purposes of each key

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for nitro-payment-demo canceled.

Name Link
🔨 Latest commit 6be805d
🔍 Latest deploy log https://app.netlify.com/sites/nitro-payment-demo/deploys/6526b23169a4f50008fe8cbd

@bitwiseguy bitwiseguy merged commit a0a1dfc into main Oct 11, 2023
4 of 5 checks passed
@bitwiseguy bitwiseguy deleted the scAddr-sign-dht-record branch October 11, 2023 14:40
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.

Sign DHT custom namespace messages with SC Key
2 participants