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

Google Secret Manager support #928

Conversation

skisel-bt
Copy link
Contributor

@skisel-bt skisel-bt commented Oct 12, 2023

PR Description

This PR introduces the Google Cloud Platform's Secret Manager as an alternative backend option for storing BLS keys, extending the flexibility for users and organizations that operate within GCP's ecosystem. The addition of this backend doesn’t enhance existing functionality but provides an additional choice.

Key Additions:

CLI Arguments:

--gcp-secrets-enabled: Enable the GCP Secret Manager for storing BLS keys.
--gcp-project-id: (required) Define the GCP Project ID to interact with the Secret Manager APIs.
--gcp-secrets-filter: Specify filtering criteria for fetching secrets from the GCP Secret Manager. (Filter syntax)

Authentication:

We're utilizing Application Default Credentials (ADC) for authenticating interactions with GCP Secret Manager. ADC simplifies the acquisition of an access token, automatically finding credentials and managing token retrieval, allowing the authentication code to operate across various deployment options without modification.

Libraries Added:

Java GCP Secret Manager library and its dependencies have been incorporated, license has to be checked.

Batch Storage:

The keys are stored in batches, akin to the AWS backend implementation, to streamline performance and minimize API call overheads.

Testing:

Testing was performed by deploying a modified version of the application as a Docker container to the GCP Cloud Run service. Testing strategies included:

Validating the storage and retrieval of BLS keys using GCP Secret Manager as the backend.
Ensuring the application behaves as expected when deployed as a container in a cloud environment.
Verifying error handling, logging, and graceful failure mechanisms.

Fixed Issue(s)

Fixes #676

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Testing

  • I thought about testing these changes in a realistic/non-local environment.

@CLAassistant
Copy link

CLAassistant commented Oct 12, 2023

CLA assistant check
All committers have signed the CLA.

@skisel-bt skisel-bt changed the title Initial implementation for the fixed GCP Secret Manager Initial implementation for the GCP Secret Manager Oct 12, 2023
@skisel-bt skisel-bt changed the title Initial implementation for the GCP Secret Manager WIP implementation for the GCP Secret Manager Oct 12, 2023
@skisel-bt skisel-bt changed the title WIP implementation for the GCP Secret Manager WIP Google Secret Manager support Oct 12, 2023
@skisel-bt skisel-bt marked this pull request as ready for review October 12, 2023 19:21
@skisel-bt
Copy link
Contributor Author

Unfortunately I cannot add labels, as well as assign reviewers as per contributing guideline. Please suggest what to do in this case.

@jframe
Copy link
Contributor

jframe commented Oct 12, 2023

Unfortunately I cannot add labels, as well as assign reviewers as per contributing guideline. Please suggest what to do in this case.

Thanks for your contribution.

re: labels and assigning reviewers, that isn't necessary. I will assign some reviewers to have a look at your PR.

Copy link
Contributor

@usmansaleem usmansaleem left a comment

Choose a reason for hiding this comment

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

Would you able to add acceptance tests around bulkloading and/or list public keys dealing with GCP? Similar to https://github.com/Consensys/web3signer/blob/master/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/bulkloading/AwsSecretsManagerAcceptanceTest.java ... I'll work with our DevOps department to obtain GCP access to run these ATs on master build. These ATs would only work/run if appropriate environment variables are defined.

@skisel-bt
Copy link
Contributor Author

Would you able to add acceptance tests around bulkloading and/or list public keys dealing with GCP?

Yes, just pushed the update

@skisel-bt skisel-bt requested a review from usmansaleem October 13, 2023 16:35
@usmansaleem usmansaleem dismissed their stale review October 22, 2023 23:33

will re-review as the changes has been addressed

@skisel-bt skisel-bt force-pushed the skisel-bt/676-google-secret-manager-support branch from 876de61 to 99c03ac Compare October 29, 2023 12:19
@usmansaleem
Copy link
Contributor

Tested GcpSecretManagerAcceptanceTest acceptance test:

Eth2Runner | Bulk loading keys from GCP Secret Manager ... 
...
Eth2Runner | Keys loaded from GCP Secret Manager: [2], with error count: [0]

@usmansaleem usmansaleem added the doc-change-required Indicates an issue or PR that requires doc to be updated label Nov 3, 2023
Copy link
Contributor

@usmansaleem usmansaleem left a comment

Choose a reason for hiding this comment

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

LGTM

@usmansaleem
Copy link
Contributor

@skisel-bt PR looks good to me. Let me know if you are good to get it merge (the title of PR still has WIP in it :)).

@skisel-bt skisel-bt changed the title WIP Google Secret Manager support Google Secret Manager support Nov 3, 2023
@skisel-bt
Copy link
Contributor Author

@usmansaleem I am good to get it merged and I've also renamed the PR. If I understand correctly there is also doc change required, is there any help there I could do?

@usmansaleem usmansaleem merged commit 2f2ce68 into Consensys:master Nov 3, 2023
7 checks passed
@skisel-bt skisel-bt deleted the skisel-bt/676-google-secret-manager-support branch November 3, 2023 11:25
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Nov 28, 2023
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.

Google Secret Manager Support
5 participants