Add remote signing binary #9293
Open
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
This commit adds additional config options to the RemoteSigner config, enabling a new alternative remote signer implementation. This new implementation sets up the connection between the remote signer and the watch-only node in the opposite way compared to the previously available implementation. In this setup, the remote signer will make an outbound connection to the watch-only node, whereas the previous version allowed an inbound connection from the watch-only node. Therefore, we call this remote signer type an "outbound remote signer." The actual implementation for this new version will be added in the commits following this one. The new version is temporarily disabled in the config validation until the implementation commits have been added.
The documentation for the DefaultRemoteSignerRPCTimeout constant incorrectly specified that the value was also used as the timeout for requests to and from the remote signer. However, the value is only used as the timeout when setting up the connection to the remote signer. This commit corrects the documentation to reflect the actual usage of the constant.
This commit introduces a new macaroon entity that grants the caller access to connect a remote signer to LND, provided the LND instance is configured to use an outbound remote signer.
To enable an outbound remote signer to connect to the watch-only lnd node, we add a SignCoordinatorStreams bi-directional streaming RPC endpoint. The stream created when the remote signer connects to this endpoint can be used to pass any requests to the remote signer and to receive the corresponding responses. We clearly define the types of requests and responses that can be sent over the stream, including all the requests that can be sent to the remote signer with the previous implementation. Those are the ones sent to the `signrpc.SignerClient` and `walletrpc.WalletKitClient` in the `lnwallet/rpcwallet.go` file. We also include messages for the required handshake between the remote signer and the watch-only node, and a message that the remote signer can send if it encounters an error while processing a request.
RemoteSigner is an interface that abstracts the communication with a remote signer. It extends the RemoteSignerRequests interface. Note that this is the interface for the remote signer on the watch-only node's side. As we'll add an outbound remote signer implementation in upcoming commits, we need this interface to abstract the commonalities of both the inbound and outbound remote signer implementations, so that the RPCKeyRing doesn't need to know which type it's using.
This commit wraps the current remote signer implementation in the new RemoteSigner interface within an InboundRemoteSigner struct.
|
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 (
|
`RemoteSignerBuilder` is a helper that creates instances of the RemoteSigner interface based on the lncfg.RemoteSigner config. It is intended to create different types of remote signers instances based on the SignerType specified in the config.
As the RemoteSignerBuilder can now create an InboundRemoteSigner that matches the functionality of the previous remote signer communication implementation, we refactor the rpcwallet package to use a RemoteSigner instance created by the RemoteSignerBuilder.
With the `RPCKeyRing` now having the `RemoteSigner` reference, we can use that reference to call the `Ping` implementation of the `RemoteSigner` interface for the health check of the remote signer. This allows different types of remote signers to specify their own implementation to verify if the remote signer is active.
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 wrapped as the `RemoteSignerClient`. The `RemoteSignerClient` will make an outbound gRPC connection to the watch-only node to set up the stream between them and then process all requests that the watch-only node sends to the remote signer by passing them on to the respective `walletrpc.WalletKitServer` and `signrpc.SignerServer`. In the future, we may have more than one implementation of the remote signer client beyond just an outbound remote signer client, and we might then turn the `RemoteSignerClient` into a broader interface and rename this specific implementation to the `OutboundSignerClient`. 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 RemoteSignerClient will only fully start if it's enabled by the configuration, i.e. the `remotesigner.signertype` is set to `signer`. As we may have more than one implementation of the remote signer client beyond just an outbound remote signer client in the future, we create an remote signer client instance for any configuration.
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, we can now enable the signerrole "signer-outbound" in the lncfg package. 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 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 `remoteSignerClient` implementation, defining the signer node's side of this functionality. The new implementation, called `SignCoordinator`, converts requests sent to the remote signer into the corresponding `SignCoordinatorStreams` request messages and transmits them over the stream. The requests we send to a remote signer are defined in the `RPCKeyRing` (`lnwallet/rpcwallet/rpcwallet.go`). 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 outbound remote signer connects, 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 this implementation in the `RemoteSigner` interface, making it usable through the `RPCKeyRing`. This commit introduces the `OutboundRemoteSigner` 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 `RemoteSigner` 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 `RemoteSigner` `Run` function. This change ensures that the `RemoteSigner` `Run` function is reachable from the `SignCoordinatorStreams` RPC endpoint implementation.
With the ability to reach the `RemoteSigner` `Run` function in the `WalletKit` sub-server, we now implement the `SignCoordinatorStreams` RPC endpoint.
This commit populates the `RemoteSigner` 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 `OutboundRemoteSigner`, 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, we can now enable the `watchonly-outbound` signerrole in the lncfg package. This commit also adds support for the `watchonly-outbound` signerrole in the `RemoteSignerBuilder`.
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.
In upcoming commits, we will introduce functionality to block non-whitelisted RPC calls when the node functions as a remote signer. Previously, when the node acted as an inbound remote signer, there were no configuration fields to indicate the node's role. This commit introduces `signer-inbound` and `signerrole` configuration options, enabling the node's role as an inbound remote signer to be explicitly signaled. To maintain backward compatibility with existing configurations, specifying the `signerrole` remains optional when the node acts as an inbound remote signer.
When the node acts as a remote signer, there is no need to expose all RPCs as the they shouldn't be used in this mode. This commit adds a whitelist of RPCs that are allowed to be called when the node is acting as a remote signer. This further improves the security of the remote signer node.
Since specifying the node as an inbound remote signer in the configuration now blocks non-whitelisted RPCs, we update the documentation to recommend setting this option in the remote signing docs.
ViktorTigerstrom
force-pushed
the
2024-11-remote-signer-binary
branch
from
f5cdfff
to
d2624fd
Compare
`lndsigner` is a new binary purposely built to function as a remote signer. The binary is a stripped-down version of `lnd` that only exposes the functionality needed for `lnd` to function as a remote signer. The purpose of this binary is to make the setup process easier for users configuring a remote signer. With this commit, `lndsigner` uses the standard `lnd` configuration file and enforces that the node does not bootstrap from the network, listen for incoming connections, or connect to chain backends. Additionally, users must set a `remotesigner.signerrole` that indicates that the node is being used as a remote signer. Future commits will introduce a dedicated, simplified configuration file for the `lndsigner` binary, containing only the necessary options to streamline the setup process for users.
This commit defines a new configuration file type, `SignerConfig`, intended for use by the `lndsigner` binary. `SignerConfig` is a simplified version of the `Config` type, containing only the fields necessary for `lndsigner` to operate. This commit does not yet add the functionality to load `SignerConfig` file into `lndsigner`. That will be implemented in the next commit.
This commit introduces the functionality to load the `SignerConfig` from the configuration file into `lndsigner`. The goal of `lndsigner` is to remain compatible with `lnd`'s `Main` function in `lnd.go` while serving as a stripped-down version of `lnd` with a simplified user experience during setup. To achieve this, the `SignerConfig` must be merged with the main `Config` struct upon loading. This commit implements that functionality.
Update the release script to include the `lndsigner` binary in the release. Each environment will have its own zipped folder containing `lndsigner` and an `lncli` binary built with fewer build tags, limiting the available RPCs.
ViktorTigerstrom
force-pushed
the
2024-11-remote-signer-binary
branch
from
d2624fd
to
bc0c0dc
Compare
Comment on lines
+295
to
+306
| // GeneralizedConfig is an interface that defines the functions a config struct | ||
| // must support to be passable in the generalizedConfigLoader function. | ||
| type GeneralizedConfig interface { | ||
| // GetShowVersion returns the current value for the ShowVersion field | ||
| GetShowVersion() bool | ||
|
|
||
| // GetLndDir returns the current value for the LndDir field | ||
| GetLndDir() string | ||
|
|
||
| // GetConfigFile returns the current value for the ConfigFile field | ||
| GetConfigFile() string | ||
| } |
There was a problem hiding this comment.
Let me know if you prefer the approach of this fixup commit that uses an interface instead of passing these functions as params to generalizedConfigLoader.
The reason why I didn't squash this commit into the the main commit is that I'd like to get some opinions on which version you prefer, as this approach in this commit is cleaner, but comes with the tradeoff that this adds 3 functions to the main Config struct, which is not so nice given that the cfg is used throughout lnd's codebase.
Comment on lines
+1065
to
+1076
| // The flag parser above is only aware of the flags in the preCfg | ||
| // definition, and not necessarily those in the Config struct. To handle | ||
| // this, we create a new flag parser for the Config struct, as it is | ||
| // passed to the ValidateConfig function. | ||
| // Since some flags in preCfg may not exist in Config, we use | ||
| // flags.IgnoreUnknown here to avoid errors for those flags. However, | ||
| // unknown flags are not allowed for the preCfg itself, as the earlier | ||
| // flag parser will error if such flags are present. | ||
| mergedFlagParser := flags.NewParser(cfg, flags.IgnoreUnknown) | ||
| if _, err := mergedFlagParser.Parse(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Let me know if you can think of a cleaner way of doing this, as this is arguably quite dirty.
| release-install: | ||
| @$(call print, "Installing release lnd and lncli.") | ||
| @$(call print, "Installing release lnd/lndsigner and lncli.") | ||
| env CGO_ENABLED=0 $(GOINSTALL) -v -trimpath -ldflags="$(RELEASE_LDFLAGS)" -tags="$(RELEASE_TAGS)" $(PKG)/cmd/lnd | ||
| env CGO_ENABLED=0 $(GOINSTALL) -v -trimpath -ldflags="$(RELEASE_LDFLAGS)" -tags="$(RELEASE_TAGS)" $(PKG)/cmd/lncli |
There was a problem hiding this comment.
Something to note here is that lncli will be built with the RELEASE_TAGS build tags, and not with the LND_SIGNER_TAGS that the actual lncli that's shipped along with lndsigner in the zip file. I.e. the lncli here will contain more functionality. IMO that's ok though, as it could be a bit confusing if we'd allow installing of 2 different lncli in this make command.
Potentially we could give an option to the executer of this command choose which version they'd like to install though.
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.
Based on #8754
This PR introduces a new binary to lnd releases at least temporarily called
lndsigner. Let me know if you have a better naming suggestion.lndsigneris a new binary purposely built to function as a remote signer. The binary is a stripped-down version oflndthat only exposes the functionality needed forlndto function as a remote signer, and comes with a simplified configuration file, but still runslnd'sMainfunction inlnd.go.The purpose of this binary is to make the setup process easier for users configuring a remote signer.