Vault integration for secret storage by canercidam · Pull Request #72 · flashbots/builder-hub

canercidam · 2026-03-05T01:58:01Z

📝 Summary

Integrates HashiCorp Vault for builder secret storage
Bug fix: Config was never returned after set
Including in docker-compose.yaml and testing above with integration tests

✅ I have run these commands

make lint
make test
go mod tidy

canercidam · 2026-03-05T03:06:40Z

application/service.go

 }

 func (b *BuilderHub) GetConfigWithSecrets(ctx context.Context, builderName string) ([]byte, error) {
-	_, err := b.dataAccessor.GetActiveConfigForBuilder(ctx, builderName)


@metachris Could you confirm if this was intentional or not? The config was set but never returned as part of responses along with the secrets - now that's fixed. I'd be happy to revert the changes for this bug fix if we think that config should be deprecated in favor of secrets.

@TymKh wrote that code and would know

It is kind of intentional but prob long overdue to remove/change. @bakhtin config in postgres is empty, right? all the data is in secrets

metachris · 2026-03-05T13:52:21Z

adapters/secrets/service.go

@@ -1,22 +1,24 @@
-// Package secrets contains logic for adapter to aws secrets manager
+// Package secrets implements secrets storage backends


rename this file something like aws_secrets_*? seems it's only for aws secrets now?

I need to make some changes to the file for consistency and git thinks that I've created a new file entirely if I rename it, too, so wanted to avoid renaming for now. (I agree about renaming).

metachris · 2026-03-05T13:53:34Z

cmd/httpserver/main.go

 		Name:    "secret-prefix",
 		Value:   "",
-		Usage:   "AWS Secret name",
+		Usage:   "AWS Secret name prefix (use with --aws-secrets)",


is --aws-secrets new? couldn't find another reference to it in this PR

No, this is a leftover from the local work in progress - removing!

ilyaluk · 2026-03-05T13:58:03Z

adapters/secrets/hashicorp_vault.go

+
+type VaultConfig struct {
+	Address      string // Vault server address (e.g., http://localhost:8200)
+	Token        string // Vault token for authentication


Please also support https://pkg.go.dev/github.com/hashicorp/vault/api/auth/kubernetes as an auth method in addition to plain token, as that's how pods authenticate in our Vault

Added! Please let me know if you have any suggestions.

Copilot

Pull request overview

Integrates HashiCorp Vault as an additional secrets storage backend for BuilderHub, updates secret-access APIs to be context-aware, and fixes config+secrets responses by merging stored builder config with secrets.

Changes:

Add a Vault-backed secrets adapter and wire Vault configuration/selection into the HTTP server CLI.
Fix “config never returned after set” by merging builder config with secret JSON in both builder and admin “full config” endpoints.
Update local docker-compose and CI integration tests to run with Vault enabled.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
scripts/ci/integration-test.sh	Stops previous compose stack and tweaks hurl invocation for CI/integration runs.
scripts/ci/e2e-test.hurl	Extends E2E assertions to ensure non-secret config is preserved alongside secrets.
ports/admin_handler.go	Updates secrets API to be context-aware and returns merged config+secrets for “full” config endpoint.
cmd/httpserver/main.go	Adds Vault CLI flags/env vars and selects secrets backend (mock/vault/aws).
adapters/secrets/hashicorp_vault.go	New Vault KV v2 secrets backend implementation.
adapters/secrets/service.go	Refactors AWS Secrets Manager adapter (naming + context-aware API) and aligns missing-secret error.
application/service.go	Introduces `MergeJSON` and updates config-with-secrets flow to return merged payload.
domain/inmemory_secret.go	Updates mock secrets service to accept context.
docker/docker-compose.yaml	Adds Vault dev container and configures builder-hub to use Vault in local compose.
docker/mock-proxy/Dockerfile	Adjusts COPY to match new build context.
.github/workflows/release.yaml	Updates proxy image build context/Dockerfile path for releases.
Makefile	Adds a docker-compose restart helper target.
go.mod / go.sum	Adds Vault client dependencies and updates module requirements.

Comments suppressed due to low confidence (1)

scripts/ci/integration-test.sh:36

The script disables set -e around the hurl run, but then checks $? only after set -e has been re-enabled. That means the failure check is reading the exit status of set -e (almost always 0), so integration test failures won’t be detected and the script can report success incorrectly. Capture the hurl exit code immediately (or use an if ! hurl ...; then ... fi pattern) before re-enabling set -e.

hurl --test scripts/ci/e2e-test.hurl -v
set -e

# Cleanup after tests
if [ $? -ne 0 ]; then

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-06T10:41:43Z

cmd/httpserver/main.go

+	} else if vaultEnabled && vaultToken != "" {
+		log.Info("using HashiCorp Vault for secrets",
+			"address", vaultAddress,
+			"secret_path", vaultSecretPath,
+			"mount_path", vaultMountPath)


When --vault-enabled is set but --vault-token is empty, this branch is skipped and the code silently falls through to another backend (AWS or in-memory). That contradicts the flag’s "overrides" semantics and makes misconfiguration easy to miss. Consider failing fast (return an error) when vault-enabled is true but required Vault settings (at least token) are missing.

Copilot · 2026-03-06T10:41:43Z

cmd/httpserver/main.go

+	} else {
+		log.Warn("no secrets backend configured - using mock in-memory storage",
+			"vault-enabled", vaultEnabled,
+			"secret-prefix-length", len(cCtx.String("secret-prefix")))
+		sm = domain.NewMockSecretService()


Falling back to the in-memory secrets service when no backend is configured is a high-risk default: it can cause secrets written via the admin API to be lost on restart and may mask production misconfiguration. Prefer returning an error unless --mock-secrets is explicitly enabled (or at least gate this fallback behind a clearly-dev-only flag/env).

Copilot · 2026-03-06T10:41:44Z

adapters/secrets/hashicorp_vault.go

+	// Verify connection by attempting a read (404 is fine — path may not exist yet)
+	ctx := context.Background()
+	_, verifyErr := client.Secrets.KvV2Read(ctx, cfg.SecretPrefix, vault.WithMountPath(cfg.MountPath))
+	if verifyErr != nil && !isVault404(verifyErr) {
+		return nil, fmt.Errorf("failed to verify Vault connection: %w", verifyErr)
+	}


NewHashicorpVaultService verifies connectivity using context.Background() with no timeout. If Vault is unreachable or DNS hangs, startup can block indefinitely. Use a context with timeout/deadline for the verification call (and consider making verification optional).

Copilot · 2026-03-06T10:41:44Z

application/service.go

+func MergeJSON(base, override json.RawMessage) (json.RawMessage, error) {
+	result := make(map[string]json.RawMessage)
+	if err := json.Unmarshal(base, &result); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal base: %w", err)
+	}


MergeJSON is new core behavior used to serve builder configs (config + secrets), but there are no unit tests covering merge behavior or error cases (invalid JSON, key collisions, etc.). Add focused tests (e.g., in application/service_test.go) to lock in the intended semantics.

Copilot · 2026-03-06T10:41:44Z

scripts/ci/e2e-test.hurl

@@ -40,6 +40,9 @@ HTTP 200
 # [Admin API] Create an empty builder configuration


This step is labeled as creating an "empty" builder configuration, but the payload now includes foo.bar. Update the comment to reflect that this request sets non-empty config data (or revert to an empty object if that was the intent).

Suggested change

# [Admin API] Create an empty builder configuration

# [Admin API] Create an initial builder configuration with test data

Copilot · 2026-03-06T10:41:44Z

ports/admin_handler.go

+	config, err := s.builderService.GetActiveConfigForBuilder(r.Context(), builderName)
 	if err != nil {
-		s.log.Error("failed to get config with secrets", "error", err)
+		s.log.Error("failed to get config for builder", "error", err)
 		w.WriteHeader(http.StatusInternalServerError)
 		return


GetFullConfigForBuilder treats any error from GetActiveConfigForBuilder as a 500. For consistency with GetActiveConfigForBuilder (which maps domain.ErrNotFound to 404), consider handling domain.ErrNotFound here as well so missing builders/configs return 404 rather than 500.

Copilot · 2026-03-06T10:41:45Z

cmd/httpserver/main.go

 		Name:    "secret-prefix",
 		Value:   "",
-		Usage:   "AWS Secret name",
+		Usage:   "AWS Secret name prefix (use with --aws-secrets)",


The flag help text mentions --aws-secrets, but no such CLI flag exists in this binary. Update the Usage string to reference the actual flag/env var (--secret-prefix / AWS_BUILDER_CONFIGS_SECRET_PREFIX) to avoid confusing operators.

Suggested change

Usage: "AWS Secret name prefix (use with --aws-secrets)",

Usage: "AWS Secret name prefix for configs (env: AWS_BUILDER_CONFIGS_SECRET_PREFIX)",

vault integration for secret storage

5581bed

canercidam changed the title ~~vault integration for secret storage~~ Vault integration for secret storage Mar 5, 2026

add package commit

8368b8f

canercidam requested a review from metachris March 5, 2026 02:01

fix linter error

226bc28

canercidam commented Mar 5, 2026

View reviewed changes

metachris reviewed Mar 5, 2026

View reviewed changes

ilyaluk reviewed Mar 5, 2026

View reviewed changes

metachris requested a review from Copilot March 6, 2026 10:36

Copilot started reviewing on behalf of metachris March 6, 2026 10:36 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

canercidam added 4 commits March 12, 2026 08:45

add k8s auth for vault and test with k8s mock in integration tests

1551e1d

switch to the more widely used and tested library

de18f2a

address feedback

cee6aa1

fix linter error

3eae862

		@@ -1,22 +1,24 @@
		// Package secrets contains logic for adapter to aws secrets manager
		// Package secrets implements secrets storage backends

		@@ -40,6 +40,9 @@ HTTP 200
		# [Admin API] Create an empty builder configuration

	# [Admin API] Create an empty builder configuration
	# [Admin API] Create an initial builder configuration with test data

	Usage: "AWS Secret name prefix (use with --aws-secrets)",
	Usage: "AWS Secret name prefix for configs (env: AWS_BUILDER_CONFIGS_SECRET_PREFIX)",

Conversation

canercidam commented Mar 5, 2026

📝 Summary

✅ I have run these commands

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants