Add Outbound Remote Signer implementation
#8754
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ViktorTigerstrom
changed the title
Outbound Remote Signer implementation
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
9 times, most recently
from
0525eb6 to
6abefde
Compare
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
2 times, most recently
from
b34174d to
aabced7
Compare
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
from
aabced7 to
cfe7fc3
Compare
|
Interesting PR 👀, does this somehow relate to the VLS project, a bit of time ago somebody told me that there is a proxy in the making to connect LND to the VLS signer project, wondering if there is some progress made in that direction ? |
Thanks a lot! I would say that the current PR is somewhat related to VLS in terms of future functionality it could enable. However, this PR only focuses on reversing the connection setup between the watch-only node and the signer node, so that the signer node makes the outbound connection to the watch-only node instead of the other way around. In a setup where mobile signer wallets use VLS for validation which are connected to watch-only nodes, this functionality is a necessary pre-requisite unless the mobile wallets are online constantly. However, this PR as it currently stand does not add any validation or enable an integration with VLS yet. The goal is to address such needs in future PRs. My hope is that the
I think the project you're referring to is: |
There was a problem hiding this comment.
Added a few comments for reviewers where it'd be especially helpful with feedback :)! Might add some more comments tomorrow if I think of something else.
One more area I'd appreciate to get some feedback from reviewers for is:
- Iit could be useful to let the sign requests that's passed over the stream from the watch-only node to the signer, also pass through the RPC interceptor on the signer node side. If that's something you think we should look into supporting, let me know!
lnrpc/walletrpc/walletkit_server.go
Outdated
| "/walletrpc.WalletKit/SignCoordinatorStreams": {{ | ||
| Entity: "onchain", | ||
| Action: "write", | ||
| }, { | ||
| Entity: "message", | ||
| Action: "write", | ||
| }, { | ||
| Entity: "signer", | ||
| Action: "generate", | ||
| }, { | ||
| Entity: "address", | ||
| Action: "read", | ||
| }}, |
There was a problem hiding this comment.
Wasn't really sure what kind of perms we'd want to require in the macaroon for the remote signer when opening the stream with the watch-only node. Therefore mimicked the perms of the inbound remote signer required permissions. Though as this is the other way around i.e. the remote signer is connecting to the watch-only node, that maybe doesn't makes sense as it's not strictly required.
Perhaps we'd like to add some new kind of entity for this specific case?
There was a problem hiding this comment.
I think adding a special permission just for the remote signer here probably makes sense.
There was a problem hiding this comment.
Ok cool, I added a new entity called remotesigner, with the action generate (I wasn't sure which action made most sense here, so feel free to suggest using read/write if you think that makes more sense).
lnrpc/walletrpc/walletkit.proto
Outdated
| connects to the watch-only lnd node, to initialize a handshake between | ||
| the nodes. | ||
| */ | ||
| bool signer_registration = 2; |
There was a problem hiding this comment.
Let me know if it makes sense to add some kind of versioning for the signer, so that the watch-only node can know what kind of functionality the signer supports.
There was a problem hiding this comment.
Hmm, good question. I don't think that adding an independent version here gives us much. I think (at least in the beginning), we'd want the signer and watch-only to be on the same main lnd version. So maybe we should send the full version string here and initially just compare it to the one running on the watch-only, then error out if it differs?
Or don't do anything yet other than specifying this field as a string, so we can do it in the future?
There was a problem hiding this comment.
Or don't do anything yet other than specifying this field as a string, so we can do it in the future?
Makes sense, thanks! I agree that it probably makes sense to just enable the support so that we can define the versioning idea when we actually decide on it in the future, so I edited this to be a string instead.
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
from
cfe7fc3 to
b48621e
Compare
There was a problem hiding this comment.
Awesome work on this PR. The commit structure is super nice and easy to follow.
I only did a first read-through to grasp the different areas the code touches. Haven't looked at all the new code in detail yet, but will follow up with a second round soon.
So this is mainly to checkpoint my initial comments. Will dig in deeper tomorrow.
lnrpc/walletrpc/walletkit.proto
Outdated
| connects to the watch-only lnd node, to initialize a handshake between | ||
| the nodes. | ||
| */ | ||
| bool signer_registration = 2; |
There was a problem hiding this comment.
Hmm, good question. I don't think that adding an independent version here gives us much. I think (at least in the beginning), we'd want the signer and watch-only to be on the same main lnd version. So maybe we should send the full version string here and initially just compare it to the one running on the watch-only, then error out if it differs?
Or don't do anything yet other than specifying this field as a string, so we can do it in the future?
lnrpc/walletrpc/walletkit_server.go
Outdated
| "/walletrpc.WalletKit/SignCoordinatorStreams": {{ | ||
| Entity: "onchain", | ||
| Action: "write", | ||
| }, { | ||
| Entity: "message", | ||
| Action: "write", | ||
| }, { | ||
| Entity: "signer", | ||
| Action: "generate", | ||
| }, { | ||
| Entity: "address", | ||
| Action: "read", | ||
| }}, |
There was a problem hiding this comment.
I think adding a special permission just for the remote signer here probably makes sense.
config_builder.go
Outdated
| cleanUpTasks []func() | ||
| cleanUp = func() { | ||
| for _, fn := range cleanUpTasks { | ||
| if fn == nil { |
There was a problem hiding this comment.
When would a cleanup function be nil? Shouldn't we be able to expect them always to be non-nil, the way we add them below?
There was a problem hiding this comment.
Yeah that's correct, currently they'll never be nil given the current code. This was a copy of the pattern used in the BuildWalletConfig function, and I guess it could be useful if we'd ever add a remote signer implementation that doesn't have any cleanup func.
But I'll remove the nil check if you think that's just adding extra complexity :)!
There was a problem hiding this comment.
'd ever add a remote signer implementation that doesn't have any cleanup func.
But in that case, that impl can just return func() {}. We can even require that via the comment in the API
I think better to have fewer nil checks where possible
There was a problem hiding this comment.
I also dont think you need a cleanup return value here. Just have a Stop on the RemoteSigner which performs a disconnect of its established connection. Then you just add that Stop call to your clean-up list
There was a problem hiding this comment.
also: pre-existing, but I think that currently NewRPCKeyRing (and now Build) is the only call in BuildChainControl besides NewChainControl that does some sort of "start" functionality. I'd argue that we really should keep these New* functions as constructors and then leave any starting for later. But again this is out of scope here since it is pre-existing.
lnwallet/rpcwallet/remote_signer.go
Outdated
| "signing node through RPC: %v", err) | ||
| } | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
Why is this in a defer? Since we're done now?
And not sure if we also want to report errors when not being able to disconnect?
So we might just be able to do return conn.Close() here instead?
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| select { | ||
| case <-s.quit: | ||
| return nil, ErrShuttingDown | ||
| default: | ||
| } | ||
|
|
||
| s.wg.Add(1) | ||
|
|
||
| return func() { | ||
| s.wg.Done() | ||
| }, nil |
There was a problem hiding this comment.
Hmm, there are a lot of layers with the mutexes and wait groups... Probably need to take another, closer look at those, to make sure we can't run into deadlocks...
There was a problem hiding this comment.
Please do :)! I've tried to be very careful to ensure that couldn't happen, so hopefully I haven't missed any scenario where that could occur! But some extra eyes on this would indeed be very helpful 🙏
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
3 times, most recently
from
915f062 to
6e965f8
Compare
This commit introduces a new `watchonlynode` namespace to the `Config` struct. It is designed to be configured on nodes acting as remote signers in a remote signer setup and defines how the connection with the watch-only node is established. Note that enabling the watch-only node configuration is not yet supported in this commit and will be enabled in a future update.
CancelOrQuit is used to create a cancellable context that will be cancelled if the passed quit channel is signalled.
The previous commits added the foundation for creating different types of remote signer connections and defined the RPC that an outbound remote signer would use to communicate with the watch-only node. We will now define the implementation that the outbound signer node will use to set up the stream to the watch-only node and process any sign request messages that the watch-only node sends to the signer node. This implementation is encapsulated in the `OutboundClient`. The `OutboundClient` establishes an outbound gRPC connection to the watch-only node to set up a stream between them. It then processes all requests sent by the watch-only node to the remote signer by forwarding them to the appropriate `walletrpc.WalletKitServer` and `signrpc.SignerServer`. An alternative implementation of `RemoteSignerClient`, called `NoOpClient`, is also provided. As the name implies, this client does not perform any operations. Note once again that this is the implementation for the signer node side, not the watch-only node.
Add a remote signer client builder that constructs either an outbound remote signer client or a No Op client, depending on the current configuration.
This commit adds a `RemoteSignerClient` instance to the main `lnd` server. The specific type of `RemoteSignerClient` used will depend on the configuration. If `watchonlynode.enable` is set to `true`, an `OutboundClient` instance will be used. Otherwise, a `NoOpClient` instance will be utilized.
As all the necessary pieces on the signer node side to let the remote signer make an outbound connection to the watch-only node are in place, the `lncfg` package can now permit the `watchonlynode.enable` setting to be configured. Note that we still haven't created the implementation on the watch-only node side to accept the connection and send the requests over the stream. This will be added in the upcoming commits.
This commit introduces configuration functionality on the watch-only node's side to support an inbound connection from an outbound remote signer. Note that enabling this option is not supported in this commit, but the ability to toggle it will be added in an upcoming commit.
This commit introduces an implementation for the watch-only node to send and receive messages over the `SignCoordinatorStreams` stream, which serves as the connection stream with an outbound remote signer. Previous commits added the `OutboundClient` implementation, defining the signer node's side of this functionality. The new implementation, called `SignCoordinator`, converts requests sent to the remote signer connection on the watch-only node side into the corresponding `SignCoordinatorStreams` request messages and transmits them over the stream. The requests we send to a remote signer are defined in the `RemoteSignerRequests` interface. When a response is received from the outbound remote signer, it is then converted back into the appropriate `walletrpc` or `signrpc` response. Additionally, the `SignCoordinator` includes functions to block and signal once the outbound remote signer has connected. Since requests cannot be processed before the connection to the outbound remote signer is established, any requests sent to the `SignCoordinator` will wait for the remote signer to connect before being processed.
As the previous commit implemented the foundation for the watch-only node to send and receive messages with an outbound remote signer (the `SignCoordinator` implementation), we can now wrap the communication in the `RemoteSignerConnection` interface, making it usable through the `RPCKeyRing`. This commit introduces the `InboundConnection` implementation to achieve that.
To accept incoming connections from the remote signer and use the remote signer stream for any required signatures on the watch-only node, we must allow the connection from the remote signer before any signatures are needed. Currently, we only allow requests through the `InterceptorChain` into the rpc-servers after the `WalletState` has been set to `RpcActive`. This status is only set once the main `RpcServer`, along with all sub-servers, have been fully started and populated with their dependencies. The problem is that we need signatures from the remote signer to create some of the dependencies for the sub-servers. Because of this, we need to let the remote signer connect before all dependencies are created. To enable this, we add a new `WalletState`, `AllowRemoteSigner`, which allows connection requests from a remote signer to pass through the `InterceptorChain` when the `AllowRemoteSigner` state is set. This state is set before the `RpcActive` state.
Change the `InterceptorChain` behavior to allow a remote signer to call the `walletrpc.SignCoordinatorStreams` while the `rpcState` is set to `allowRemoteSigner`. This state precedes the `rpcActive` state, which allows all RPCs. This change is necessary because `lnd` needs the remote signer to be connected before some of the internal dependencies for RPC sub-servers can be created. These dependencies must be inserted into the sub-servers before moving the `rpcState` to `rpcActive`.
The `SetServerActive` moves the `rpcState` from `rpcActive` to `serverActive`. Update the docs to correctly reflect that.
To enable an outbound remote signer to connect to lnd before all dependencies for the RPC sub-servers are created, we need to separate the process of adding dependencies to the sub-servers from created the sub-servers. Prior to this commit, the RPC sub-servers were created and enabled only after all dependencies were in place. Such a limitation prevents accepting an incoming connection request from an outbound remote signer (e.g., a `walletrpc.SignCoordinatorStreams` RPC call) to the `WalletKitServer` until all dependencies for the RPC sub-servers are created. However, this limitation would not work, as we need the remote signer in order to create some of the dependencies for the other RPC sub-servers. Therefore, we need to enable calls to at least the `WalletKitServer` and the main RPC server before creating the remaining dependencies. This commit refactors the logic for the main RPC server and sub-servers, allowing them to be enabled before dependencies are inserted into the sub-servers. The `WalletState` for the `InterceptorChain` is only set to `RpcActive` after all dependencies have been created and inserted, ensuring that RPC requests won't be allowed into the sub-servers before the dependencies exist. An upcoming commit will set the state to `AllowRemoteSigner` before all dependencies are created, enabling an outbound remote signer to connect when needed.
This commit adds the `RemoteSignerConnection` reference to the `WalletKit` `Config`, enabling it to be accessed from the `WalletKit` sub-server. When a remote signer connects by calling the `SignCoordinatorStreams` RPC endpoint, we need to pass the stream from the outbound remote signer to the `InboundRemoteSignerConnection` `AddConnection` function. This change ensures that the `InboundRemoteSignerConnection` `AddConnection` function is reachable from the `SignCoordinatorStreams` RPC endpoint implementation. Note that this field should only to be populated when the `RPCKeyRing` `RemoteSignerConnection` is an `InboundConnection` instance, as the only that connection type implements the `InboundRemoteSignerConnection` interface.
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
from
5c5c9c5 to
5a6381c
Compare
|
Rebased on master, and some minor lint fixes. |
With the ability to reach the `InboundRemoteSignerConnection` `AddConnection` function in the `WalletKit` sub-server, we now implement the `SignCoordinatorStreams` RPC endpoint.
This commit populates the `RemoteSignerConnection` reference in the `WalletKit` config before other dependencies are added. To ensure that an outbound remote signer can connect before other dependencies are created, and since we use this reference in the walletrpc `SignCoordinatorStreams` RPC, we must populate this dependency prior to other dependencies during the lnd startup process.
This commit populates the `RemoteSignerConnection` reference in the `WalletKit` config before other dependencies are added. To ensure that an outbound remote signer can connect before other dependencies are created, and since we use this reference in the walletrpc `SignCoordinatorStreams` RPC, we must populate this dependency prior to other dependencies during the lnd startup process.
Previous commits added functionality to handle the incoming connection from an outbound remote signer and ensured that the outbound remote signer could connect before any signatures from the remote signer are needed. However, one issue still remains: we need to ensure that we wait for the outbound remote signer to connect when starting lnd before executing any code that requires the remote signer to be connected. This commit adds a `ReadySignal` function to the `WalletController` that returns a channel, which will signal once the wallet is ready to be used. For an `InboundConnection`, this channel will only signal once the outbound remote signer has connected. This can then be used to ensure that lnd waits for the outbound remote signer to connect during the startup process.
With the functionality in place to allow an outbound remote signer to connect before any signatures are needed and the ability to wait for this connection, this commit enables the functionality to wait for the remote signer to connect before proceeding with the startup process. This includes setting the `WalletState` in the `InterceptorChain` to `AllowRemoteSigner` before waiting for the outbound remote signer to connect.
With all the necessary components on the watch-only node side in place to support usage of an outbound remote signer, the `lncfg` package can now permit the `remotesigner.allowinboundconnection` setting to be configured. This commit also adds support for the building `InboundConnection` instances in the `RemoteSignerConnectionBuilder`.
With support for the outbound remote signer now added, we update the documentation to detail how to enable the use of this new remote signer type.
Update release notes to include information about the support for the new outbound remote signer type.
Update the harness to allow creating a watch-only node without starting it. This is useful for tests that need to create a watch-only node prior to starting it, such as tests that use an outbound remote signer.
testOutboundRSMacaroonEnforcement tests that a valid macaroon including the `remotesigner` entity is required to connect to a watch-only node that uses an outbound remote signer, while the watch-only node is in the state (WalletState_ALLOW_REMOTE_SIGNER) where it waits for the signer to connect.
This commit fixes that word wrapping for the deriveCustomScopeAccounts function docs, and ensures that it wraps at 80 characters or less.
ViktorTigerstrom
force-pushed
the
2024-05-add-outbound-remote-signer
branch
from
5a6381c to
022ba39
Compare
This PR introduces the functionality to utilize an alternative remote signer implementation, in which the remote signer node establishes an outbound connection to the watch-only node.
In the existing remote signer implementation, the watch-only node establishes an outbound connection to the remote signer, which accepts an inbound connection. The implementation introduced by this PR eliminates the need for the remote signer to allow any inbound connections.
To enable an outbound remote signer using the functionality introduced in this PR, please run
make release-install& follow these steps:https://github.com/ViktorTigerstrom/lnd/blob/2024-05-add-outbound-remote-signer/docs/remote-signing.md#outbound-remote-signer-example
Note:
This PR does not address the requirement for the remote signer to remain online while the watch-only node is operational. Currently, all RPC requests sent to the Remote signer will fail if it goes offline, and the health monitor will then proceed to shutdown the watch-only node. Additionally, this PR does not implement any validation on the remote signer side, i.e. the Remote Signer will blindly sign whatever is sent to it.
These issues will be addressed in future PRs.
Final note:
I plan resolve any CI issues ASAP, so reviewers can await those fixes before starting the review if preferred. I also intend to add some review comments on a few points where feedback would be particularly helpful.