feat(aggregator): Sign ancillary with GCP Kms #2431
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alenar
requested review from
sfauvel,
jpraynaud,
dlachaume,
turmelclem and
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for signing ancillary files using a key stored in Google Cloud Platform KMS. Key changes include:
- The implementation of a new signing service (AncillarySignerWithGcpKms) using the gcloud-kms crate.
- The addition of the GcpCryptoKeyVersionResourceName module to validate and parse GCP resource names.
- Updates to configuration, dependency injection, tests, and documentation to integrate GCP KMS as a new ancillary files signer.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mithril-aggregator/src/tools/mod.rs | Added constant DEFAULT_GCP_CREDENTIALS_JSON_ENV_VAR |
| mithril-aggregator/src/services/snapshotter/ancillary_signer/with_gcp_kms.rs | New signer service implementation using GCP KMS client |
| mithril-aggregator/src/services/snapshotter/ancillary_signer/mod.rs | Updated module imports and re-exports |
| mithril-aggregator/src/services/snapshotter/ancillary_signer/gcp_kms_resource_name.rs | Added resource name validation and parsing logic |
| mithril-aggregator/src/file_uploaders/gcp_uploader.rs | Updated use of environment variable constant in uploader logic |
| mithril-aggregator/src/dependency_injection/builder/protocol/artifacts.rs | Integrated GcpKms variant for ancillary files signer configuration |
| mithril-aggregator/src/configuration.rs | Updated configuration and added tests for GCP KMS-related settings |
| mithril-aggregator/Cargo.toml | Added gcloud-kms dependency |
| internal/mithril-doc/src/markdown_formatter.rs | Updated newline formatting in markdown output |
| docs/website/root/manual/develop/nodes/mithril-aggregator.md | Updated ancillary files signer configuration documentation |
mithril-aggregator/src/services/snapshotter/ancillary_signer/gcp_kms_resource_name.rs
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mithril-aggregator/src/services/snapshotter/ancillary_signer/gcp_kms_resource_name.rs
Fixed
Show fixed
Hide fixed
Alenar
force-pushed
the
djo/2362/fix-ancillary-signing-on-evolving-files
branch
from
c504bad to
3c73805
Compare
Alenar
force-pushed
the
djo/2362/ancillary-kms-signing
branch
from
80725e7 to
4413093
Compare
Alenar
force-pushed
the
djo/2362/fix-ancillary-signing-on-evolving-files
branch
from
3c73805 to
13b87bd
Compare
Alenar
force-pushed
the
djo/2362/ancillary-kms-signing
branch
from
4413093 to
05cf135
Compare
Alenar
force-pushed
the
djo/2362/fix-ancillary-signing-on-evolving-files
branch
from
13b87bd to
f872829
Compare
- use `gcloud_kms` crate as backend - define a `GcpCryptoKeyVersionResourceName` struct that's not stricly necessary, as we could pass a string directly, but allow format check at configuration time instead of at the last moment when the request is sent.
Alenar
force-pushed
the
djo/2362/ancillary-kms-signing
branch
from
05cf135 to
3b8e596
Compare
dlachaume
approved these changes
Apr 18, 2025
|
|
||
| fn from_str(s: &str) -> StdResult<Self> { | ||
| let error = format!( | ||
| "Invalid resource name: '{s}' does not match pattern 'projects/../locations/../keyRings/../cryptoKeys/../cryptoKeyVersions/..'" |
There was a problem hiding this comment.
Would it make sense to use the constants declared just above?
jpraynaud
approved these changes
Apr 22, 2025
| } | ||
|
|
||
| fn default_gcp_kms_credentials_json_env_var() -> String { | ||
| DEFAULT_GCP_CREDENTIALS_JSON_ENV_VAR.to_string() |
There was a problem hiding this comment.
Suggested change
| DEFAULT_GCP_CREDENTIALS_JSON_ENV_VAR.to_string() | |
| DEFAULT_GCP_CREDENTIALS_KMS_JSON_ENV_VAR.to_string() |
| return Err(anyhow!( | ||
| "Missing GOOGLE_APPLICATION_CREDENTIALS_JSON environment variable".to_string() | ||
| )); | ||
| if env::var(DEFAULT_GCP_CREDENTIALS_JSON_ENV_VAR).is_err() { |
There was a problem hiding this comment.
We need 2 different variables if we want to use 2 different service accounts:
Suggested change
| if env::var(DEFAULT_GCP_CREDENTIALS_JSON_ENV_VAR).is_err() { | |
| if env::var(DEFAULT_GCP_CREDENTIALS_CLOUD_STORAGE_JSON_ENV_VAR).is_err() { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Content
This PR add a new provider for signing ancillary files: GCP Kms.
Aggregator changes
AncillarySignerWithGcpKmsservicegcloud_kms, so testing would mean providing a server that would mock Gcp Kms responsesANCILLARY_FILES_SIGNER_CONFIGconfiguration key like this:{ "type": "gcp-kms", "resource_name": "projects/project_name/locations/_location_name/keyRings/key_ring_name/cryptoKeys/key_name/cryptoKeyVersions/key_version" }GcpCryptoKeyVersionResourceNameto verify gcp kms resource name provided from the configuration when reading the configuration (elseAncillarySignerWithGcpKmswould fail when computing the signature, far after the aggregator start).tools::DEFAULT_GCP_CREDENTIALS_JSON_ENV_VARconstant to avoid repetitions of theGOOGLE_APPLICATION_CREDENTIALS_JSONenv var namePre-submit checklist
Issue(s)
Relates to #2362