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

node: add amazon kms and benchmark signers #4148

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pleasew8t
Copy link
Contributor

Amazon KMS Guardian Signer

This PR adds an Amazon AWS KMS Guardian signer, allowing observations to be signed using KMS! The new signer can be used by specifying the ARN of the KMS key to use, through the --guardianSignerUri commandline argument, as follows:

  • --guardianSignerUri=amazonkms://<ARN>

NOTE For the best possible performance, it is recommended that the Guardian be run from an EC2 instance that is in the same region as the KMS key.

The KMS key's spec should be ECC_SECG_P256K1, and should be enabled for signing. In order for the Guardian to authenticate against the KMS service, one of two options are available:

  • Create new API keys in the AWS console that are permissioned to use the KMS key for signing, and add the keys to the EC2 instance's ~/.aws/credentials file. (example here).
  • Create a role that is permissioned to use the KMS key and attach that role to the Guardian EC2 instance.

Copy link
Collaborator

@pires pires left a comment

Choose a reason for hiding this comment

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

Excited about trying this one 👏🏻

node/pkg/guardiansigner/amazonkms.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/amazonkms.go Outdated Show resolved Hide resolved
return &amazonKmsSigner, nil
}

func (a *AmazonKms) Sign(hash []byte) (signature []byte, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry if I failed to flag this before, but i think it would be ideal if both Sign and Verify were to have a context.Context parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I can just pass in the contexts that are available in the calling scope, and create a new one in scopes that don't have an existing context available?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better than having no Context so I'm good with it.

node/pkg/guardiansigner/amazonkms.go Outdated Show resolved Hide resolved
node/pkg/guardiansigner/amazonkms.go Show resolved Hide resolved
pubKey := b.innerSigner.PublicKey()

duration := time.Since(start)
fmt.Printf("Public key retrieval time: %v\n", duration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be exposed through a Prometheus histogram.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's an example of the definition of an histogram

observationChanDelay = promauto.NewHistogram(
and how to observe latency
observationChanDelay.Observe(float64(time.Since(m.Timestamp).Microseconds()))

"time"
)

type BenchmarkSigner struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, document this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC this means we can benchmark AWS KMS signer by passing its configuration to a signer of type benchmark, correct?

@pires pires added the node label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants