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

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

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

jeffhubCB
Copy link
Contributor

@jeffhubCB jeffhubCB commented Apr 3, 2024

Why are these changes needed?

It is desirable to run an EigenDA Node without providing access to the Eigenlayer Node Operator ECDSA key. If EigenDA Node is started in a configuration that does not use this key, it should not require the configuration to still specify a key and attempt to decrypt it.

There are configuration options which drive a runtime-need for the ECDSA key.

  1. opting-in at startup
  2. automatic IP checking and updating

When started in a configuration that requires the ECDSA key but no key is provided, a startup error message results.
Screenshot 2024-04-03 at 9 26 32 AM
Screenshot 2024-04-03 at 9 27 46 AM

NOTE This only impacts node, not node-plugin. The ECDSA key is still required when running node-plugin commands.

Checks

  • I've made sure the lint is passing in this PR. (golangci-lint github action command)
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests (test-contracts and unit-tests github actions commands)
    • Integration tests
    • This PR is not tested :(

jeffhubCB and others added 2 commits April 3, 2024 09:03
if the ecdsa key is not needed, do not require it at runtime
if configuration requires the key, ensure it was provided
@@ -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

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

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.

Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

The implementation lgtm. Did you get a chance to test?

@jeffhubCB
Copy link
Contributor Author

The implementation lgtm. Did you get a chance to test?

I tested locally as much as I was able. I did not modify the inabox setup which would be a complete validation. I'm working on getting enough stake to be in the active set in Holesky, but that's unlikely to happen until tomorrow.

The screenshots of the startup error message were taken by using eigenda-operator-setup.
To remove the ECDSA key, the following modifications were made:
Remove
https://github.com/Layr-Labs/eigenda-operator-setup/blob/master/holesky/docker-compose.yml#L32
https://github.com/Layr-Labs/eigenda-operator-setup/blob/master/holesky/.env.example#L32
https://github.com/Layr-Labs/eigenda-operator-setup/blob/master/holesky/.env.example#L92
https://github.com/Layr-Labs/eigenda-operator-setup/blob/master/holesky/.env.example#L100

In this configuration, startup will fail with the following message:

application failed: node.ecdsa-key-file and node.ecdsa-key-password are required if node.public-ip-check-interval is > 0

Update this line to 0: https://github.com/Layr-Labs/eigenda-operator-setup/blob/master/holesky/.env.example#L65
In this configuration, startup will succeed.

Add this to .env:
NODE_REGISTER_AT_NODE_START=true
In this configuration, startup will fail with the following message:

application failed: node.ecdsa-key-file and node.ecdsa-key-password are required if node.register-at-node-start is enabled

The gap in testing is that at no point was my test node in the active set.

Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

Thanks for contribution!

@jianoaix jianoaix merged commit de976d1 into Layr-Labs:master Apr 4, 2024
5 checks passed
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.

3 participants