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

Replace Guardian Key with abstracted Guardian Signer #4120

Conversation

pleasew8t
Copy link
Contributor

@pleasew8t pleasew8t commented Sep 19, 2024

This pull request introduces the GuardianSigner interface, defined in the guardiansigner package, which is meant to replace directly using the guardian key (private key) for data signing. In doing so, it becomes easier to introduce alternative signing mechanisms without too much modification of code outside of the new guardiansigner package. Additional signing mechanisms include HSMs or KMS's (such as AWS or GCP).

The changes made to the repository are summarised as follows:

  • Introduction of the new guardiansigner package, which includes a FileSigner implementation that works with the current guardian key.
  • Addition of the --guardianSignerUri commandline argument. Node operators can still make use of guardianKeyPath, as support is provided to translate the path to the appropriate guardianSignerUri. As additional signer implementations are introduced, more URI schemes will be added. But for now only file:// is supported, loading a private key from disk.
  • Replace data signing that used the guardian key with signing using a GuardianSigner.

Notable Code Change

One change in the PR that we are not yet certain about is the use of AddSignature defined in sdk/vaa/structs.go. The changes in node/pkg/adminrpc/adminserver.go removed the use of AddSignature and adds the signature manually, to make use of a GuardianSigner. There were some alternative ideas surrounding this:

An alternative idea was to make AddSignature accept a GuardianSigner instead of a private key. This did not seem right, as it would create a dependency on the node packages that seems unneccessary, and also modify what is defined as an SDK, potentially breaking functionality for other projects that might make use of the SDK.

Additional comment by @johnsaigle that is also worth considering: #4120 (comment)

Copy link
Collaborator

@banescusebi banescusebi left a comment

Choose a reason for hiding this comment

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

Just one edge case and some small nits

node/cmd/guardiand/node.go Show resolved Hide resolved
node/cmd/guardiand/node.go Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner.go Show resolved Hide resolved
Copy link
Contributor

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

Left some minor comments related to naming and clarity. Overall the implementation looks good

node/cmd/guardiand/node.go Show resolved Hide resolved
node/hack/accountant/send_obs.go Outdated Show resolved Hide resolved
node/pkg/adminrpc/adminserver.go Show resolved Hide resolved
node/pkg/adminrpc/adminserver.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/filesigner.go Show resolved Hide resolved
node/pkg/guardiansigner/generatedsigner.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner.go Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner_test.go Outdated Show resolved Hide resolved
@johnsaigle
Copy link
Contributor

johnsaigle commented Sep 19, 2024

AddSignature in sdk/vaa/structs.go is now unused outside of tests. Should we mark it as deprecated? Are there any other related functions that should be deprecated now? Or we could go further and delete the code if the intention is that this new mechanism should totally replace the old one.

Another approach would be to rewrite this function to use the new Guardian signer though it would be a breaking change to modify the function signature.

@pleasew8t pleasew8t marked this pull request as ready for review September 19, 2024 17:32
Copy link
Collaborator

@djb15 djb15 left a comment

Choose a reason for hiding this comment

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

Very cool!

node/cmd/guardiand/node.go Outdated Show resolved Hide resolved
node/cmd/guardiand/node.go Show resolved Hide resolved
node/pkg/accountant/accountant.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/filesigner.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner.go Show resolved Hide resolved
node/pkg/p2p/run_params.go Outdated Show resolved Hide resolved
bruce-riley
bruce-riley previously approved these changes Oct 2, 2024
@pleasew8t pleasew8t force-pushed the replace-guardian-key-with-abstracted-guardian-signer branch 2 times, most recently from 13e82f8 to 32fa31d Compare October 3, 2024 20:31
bruce-riley
bruce-riley previously approved these changes Oct 8, 2024
djb15
djb15 previously approved these changes Oct 8, 2024
SEJeff
SEJeff previously approved these changes Oct 14, 2024
Copy link
Collaborator

@SEJeff SEJeff left a comment

Choose a reason for hiding this comment

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

All of my suggestions here are in general nits or things that make future maintenance a bit nicer. This is a nice clean implementation from all I see here. Nicely done sir! 🥳

node/cmd/guardiand/node.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/filesigner.go Show resolved Hide resolved
node/pkg/guardiansigner/filesigner.go Show resolved Hide resolved
node/pkg/guardiansigner/generatedsigner.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/generatedsigner.go Show resolved Hide resolved
node/pkg/adminrpc/adminserver.go Outdated Show resolved Hide resolved
node/pkg/adminrpc/adminserver_test.go Outdated Show resolved Hide resolved
node/pkg/adminrpc/adminserver_test.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/guardiansigner_test.go Outdated Show resolved Hide resolved
@pleasew8t pleasew8t dismissed stale reviews from SEJeff, djb15, and bruce-riley via 5c19304 October 16, 2024 05:49
@pleasew8t pleasew8t force-pushed the replace-guardian-key-with-abstracted-guardian-signer branch from 5c19304 to 63ee311 Compare October 16, 2024 05:50
@pleasew8t pleasew8t merged commit e82db71 into wormhole-foundation:main Oct 17, 2024
24 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.

6 participants