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.

Sign up for GitHub

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

feat(node): support execution without ecdsa key #438

Merged
merged 3 commits into from
Apr 4, 2024
Merged

feat(node): support execution without ecdsa key #438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21be6b6
feat(node): support execution without ecdsa key
jeffhubCB Apr 3, 2024
8faa559
Merge branch 'Layr-Labs:master' into node-runtime-without-ecdsa-key
jeffhubCB Apr 3, 2024
9da5d4d
Merge branch 'Layr-Labs:master' into node-runtime-without-ecdsa-key
jeffhubCB Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions node/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,19 +101,32 @@ func NewConfig(ctx *cli.Context) (*Config, error) {

testMode := ctx.GlobalBool(flags.EnableTestModeFlag.Name)

// Decrypt ECDSA key
// Configuration options that require the Node Operator ECDSA key at runtime
registerNodeAtStart := ctx.GlobalBool(flags.RegisterAtNodeStartFlag.Name)
pubIPCheckInterval := ctx.GlobalDuration(flags.PubIPCheckIntervalFlag.Name)
needECDSAKey := registerNodeAtStart || pubIPCheckInterval > 0
if registerNodeAtStart && (ctx.GlobalString(flags.EcdsaKeyFileFlag.Name) == "" || ctx.GlobalString(flags.EcdsaKeyPasswordFlag.Name) == "") {
return nil, fmt.Errorf("%s and %s are required if %s is enabled", flags.EcdsaKeyFileFlag.Name, flags.EcdsaKeyPasswordFlag.Name, flags.RegisterAtNodeStartFlag.Name)
}
if pubIPCheckInterval > 0 && (ctx.GlobalString(flags.EcdsaKeyFileFlag.Name) == "" || ctx.GlobalString(flags.EcdsaKeyPasswordFlag.Name) == "") {
return nil, fmt.Errorf("%s and %s are required if %s is > 0", flags.EcdsaKeyFileFlag.Name, flags.EcdsaKeyPasswordFlag.Name, flags.PubIPCheckIntervalFlag.Name)
}

var ethClientConfig geth.EthClientConfig
if !testMode {
keyContents, err := os.ReadFile(ctx.GlobalString(flags.EcdsaKeyFileFlag.Name))
if err != nil {
return nil, fmt.Errorf("could not read ECDSA key file: %v", err)
}
sk, err := keystore.DecryptKey(keyContents, ctx.GlobalString(flags.EcdsaKeyPasswordFlag.Name))
if err != nil {
return nil, fmt.Errorf("could not decrypt the ECDSA file: %s", ctx.GlobalString(flags.EcdsaKeyFileFlag.Name))
}
ethClientConfig = geth.ReadEthClientConfigRPCOnly(ctx)
ethClientConfig.PrivateKeyString = fmt.Sprintf("%x", crypto.FromECDSA(sk.PrivateKey))
if needECDSAKey {
// Decrypt ECDSA key
keyContents, err := os.ReadFile(ctx.GlobalString(flags.EcdsaKeyFileFlag.Name))
if err != nil {
return nil, fmt.Errorf("could not read ECDSA key file: %v", err)
}
sk, err := keystore.DecryptKey(keyContents, ctx.GlobalString(flags.EcdsaKeyPasswordFlag.Name))
if err != nil {
return nil, fmt.Errorf("could not decrypt the ECDSA file: %s", ctx.GlobalString(flags.EcdsaKeyFileFlag.Name))
}
ethClientConfig.PrivateKeyString = fmt.Sprintf("%x", crypto.FromECDSA(sk.PrivateKey))
}
} else {
ethClientConfig = geth.ReadEthClientConfig(ctx)
}
Expand Down Expand Up @@ -155,7 +168,7 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
EnableMetrics: ctx.GlobalBool(flags.EnableMetricsFlag.Name),
MetricsPort: ctx.GlobalString(flags.MetricsPortFlag.Name),
Timeout: timeout,
RegisterNodeAtStart: ctx.GlobalBool(flags.RegisterAtNodeStartFlag.Name),
RegisterNodeAtStart: registerNodeAtStart,
ExpirationPollIntervalSec: expirationPollIntervalSec,
EnableTestMode: testMode,
OverrideBlockStaleMeasure: ctx.GlobalInt64(flags.OverrideBlockStaleMeasureFlag.Name),
Expand All @@ -169,7 +182,7 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
BLSOperatorStateRetrieverAddr: ctx.GlobalString(flags.BlsOperatorStateRetrieverFlag.Name),
EigenDAServiceManagerAddr: ctx.GlobalString(flags.EigenDAServiceManagerFlag.Name),
PubIPProvider: ctx.GlobalString(flags.PubIPProviderFlag.Name),
PubIPCheckInterval: ctx.GlobalDuration(flags.PubIPCheckIntervalFlag.Name),
PubIPCheckInterval: pubIPCheckInterval,
ChurnerUrl: ctx.GlobalString(flags.ChurnerUrlFlag.Name),
NumBatchValidators: ctx.GlobalInt(flags.NumBatchValidatorsFlag.Name),
ClientIPHeader: ctx.GlobalString(flags.ClientIPHeaderFlag.Name),
Expand Down
8 changes: 4 additions & 4 deletions node/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ var (
}
EcdsaKeyFileFlag = cli.StringFlag{
Name: common.PrefixFlag(FlagPrefix, "ecdsa-key-file"),
Required: true,
Required: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you just set some dummy keys for these two flags on the machine running the node, and then use the real keys on other machine to opt-in/out? That way it doesn't even need to make this change?

The PubIPCheckIntervalFlag can be set 0 do disable the IP checker. Then there will be no need of (real) keys for node.

Copy link
Contributor Author

@jeffhubCB jeffhubCB Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Setting it to a dummy key is certainly an option, but I believe supporting this as a valid runtime option, versus expecting users that want to run in this configuration to know they can generate a dummy ECDSA key and use that, seems like a better user experience.
Additionally, by not providing a key at all it ensures failure on startup if the key is needed (given the changes in this PR), versus failure at a later point while running when the provided dummy key (to get eigenda node to start, because it won't otherwise) tries to use that key.

Copy link

Choose a reason for hiding this comment

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

@jianoaix Dummy keys is a confusing concept. Generally, my understanding is that an Operator can associate with any number of AVSs which are run separately - this arrangement should not require privileged access to the privkeys of the Operator at all. More than that though, if it did require access to the Operators private keys then the opt in/out process would be unnecessary. Please clarify if that was not the intent of the Operator AVS relationship.

In the context then that Operators and AVSs are intended to be separately owned entities, I would suggest that providing the Operator privkey be an option rather than a default with clear documentation about why you might want to do this. Even then though this is not really a clear separation of ownership nor does it communicate the intended relationships.

Dummy keys does not address this ambiguity of ownership, rather it reinforces it by requiring a user to know when to use dummy keys or when to use real keys (should be never) - in reality its seems you almost never want the AVS to have access to the Operators keys.

Copy link
Contributor Author

@jeffhubCB jeffhubCB Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are lifecycle operations needed to register/deregister an AVS that is consistent with all AVSes. Those operations require the Operator private key, but that does not equate to "the AVS should always have access to the Operator private key in its default running state". The separation and usage of the Operator ECDSA key, versus the AVS BLS key, should be much clearer than the current implementation demonstrates.

I believe node requires the private key today because of the way the codebase was originally structured, as well as the auto-socket-update (which can easily be disabled by default, with docs explaining how to enable it and the implications... gas fees, requirement to provide the ECDSA key to node w/ those security implications, etc). There are shared code paths between node, node-plugin, retriever and disperser. I believe only 2 of these need the ECDSA key... node-plugin (for opt-in/update-socket) and disperser. Because of this setup, the code was written to assume the ECDSA key is always required, leading to a situation where additional code can readily be added to leverage it at runtime when that should not be encouraged.

The scope of this PR wasn't intended to clean all of that up and formalize it more, it was an initial step so that the EigenDA Node Operator can be run without providing the Node Operator ECDSA key (or a dummy key), to increase security posture and ideally encourage further changes, in both documentation and implementation, that will enable a more secure deployment pattern and encourage the rest of the AVS community to do the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are valid points. Note I'm not against having a proper solution. I'm suggesting a way to workaround to quickly unblock the use case you are looking for.

Usage: "Path to the encrypted ecdsa private key",
EnvVar: common.PrefixEnvVar(EnvVarPrefix, "ECDSA_KEY_FILE"),
}
Expand All @@ -113,7 +113,7 @@ var (
}
EcdsaKeyPasswordFlag = cli.StringFlag{
Name: common.PrefixFlag(FlagPrefix, "ecdsa-key-password"),
Required: true,
Required: false,
Usage: "Password to decrypt ecdsa private key",
EnvVar: common.PrefixEnvVar(EnvVarPrefix, "ECDSA_KEY_PASSWORD"),
}
Expand Down Expand Up @@ -244,9 +244,7 @@ var requiredFlags = []cli.Flag{
QuorumIDListFlag,
DbPathFlag,
BlsKeyFileFlag,
EcdsaKeyFileFlag,
BlsKeyPasswordFlag,
EcdsaKeyPasswordFlag,
BlsOperatorStateRetrieverFlag,
EigenDAServiceManagerFlag,
PubIPProviderFlag,
Expand All @@ -266,6 +264,8 @@ var optionalFlags = []cli.Flag{
InternalRetrievalPortFlag,
ClientIPHeaderFlag,
ChurnerUseSecureGRPC,
EcdsaKeyFileFlag,
EcdsaKeyPasswordFlag,
}

func init() {
Expand Down
Loading