From 50e48c3bf1804caed4e5104306884be12e41d11a Mon Sep 17 00:00:00 2001 From: Anirudh Oppiliappan Date: Tue, 5 Aug 2025 20:29:03 +0300 Subject: [PATCH] spindle/secrets: rework openbao manager to use bao proxy Change-Id: qmsyszwmmzqsovtqklyxlrmlnyzypszt Signed-off-by: Anirudh Oppiliappan --- spindle/config/config.go | 6 +- spindle/secrets/openbao.go | 204 +++++++++----------------------- spindle/secrets/openbao_test.go | 143 +++++++++------------- spindle/secrets/policy.hcl | 19 ++- spindle/server.go | 16 +-- 5 files changed, 133 insertions(+), 255 deletions(-) diff --git a/spindle/config/config.go b/spindle/config/config.go index c6a5207..c4bdafd 100644 --- a/spindle/config/config.go +++ b/spindle/config/config.go @@ -28,10 +28,8 @@ type Secrets struct { } type OpenBaoConfig struct { - Addr string `env:"ADDR"` - RoleID string `env:"ROLE_ID"` - SecretID string `env:"SECRET_ID"` - Mount string `env:"MOUNT, default=spindle"` + ProxyAddr string `env:"PROXY_ADDR, default=http://127.0.0.1:8200"` + Mount string `env:"MOUNT, default=spindle"` } type Pipelines struct { diff --git a/spindle/secrets/openbao.go b/spindle/secrets/openbao.go index df49957..4c6cce2 100644 --- a/spindle/secrets/openbao.go +++ b/spindle/secrets/openbao.go @@ -6,7 +6,6 @@ import ( "log/slog" "path" "strings" - "sync" "time" "github.com/bluesky-social/indigo/atproto/syntax" @@ -16,10 +15,6 @@ import ( type OpenBaoManager struct { client *vault.Client mountPath string - roleID string - secretID string - stopCh chan struct{} - tokenMu sync.RWMutex logger *slog.Logger } @@ -31,37 +26,25 @@ func WithMountPath(mountPath string) OpenBaoManagerOpt { } } -func NewOpenBaoManager(address, roleID, secretID string, logger *slog.Logger, opts ...OpenBaoManagerOpt) (*OpenBaoManager, error) { - if address == "" { - return nil, fmt.Errorf("address cannot be empty") - } - if roleID == "" { - return nil, fmt.Errorf("role_id cannot be empty") - } - if secretID == "" { - return nil, fmt.Errorf("secret_id cannot be empty") +// NewOpenBaoManager creates a new OpenBao manager that connects to a Bao Proxy +// The proxyAddress should point to the local Bao Proxy (e.g., "http://127.0.0.1:8200") +// The proxy handles all authentication automatically via Auto-Auth +func NewOpenBaoManager(proxyAddress string, logger *slog.Logger, opts ...OpenBaoManagerOpt) (*OpenBaoManager, error) { + if proxyAddress == "" { + return nil, fmt.Errorf("proxy address cannot be empty") } config := vault.DefaultConfig() - config.Address = address + config.Address = proxyAddress client, err := vault.NewClient(config) if err != nil { return nil, fmt.Errorf("failed to create openbao client: %w", err) } - // Authenticate using AppRole - err = authenticateAppRole(client, roleID, secretID) - if err != nil { - return nil, fmt.Errorf("failed to authenticate with AppRole: %w", err) - } - manager := &OpenBaoManager{ client: client, mountPath: "spindle", // default KV v2 mount path - roleID: roleID, - secretID: secretID, - stopCh: make(chan struct{}), logger: logger, } @@ -69,136 +52,41 @@ func NewOpenBaoManager(address, roleID, secretID string, logger *slog.Logger, op opt(manager) } - go manager.tokenRenewalLoop() - - return manager, nil -} - -// authenticateAppRole authenticates the client using AppRole method -func authenticateAppRole(client *vault.Client, roleID, secretID string) error { - authData := map[string]interface{}{ - "role_id": roleID, - "secret_id": secretID, - } - - resp, err := client.Logical().Write("auth/approle/login", authData) - if err != nil { - return fmt.Errorf("failed to login with AppRole: %w", err) - } - - if resp == nil || resp.Auth == nil { - return fmt.Errorf("no auth info returned from AppRole login") - } - - client.SetToken(resp.Auth.ClientToken) - return nil -} - -// stop stops the token renewal goroutine -func (v *OpenBaoManager) Stop() { - close(v.stopCh) -} - -// tokenRenewalLoop runs in a background goroutine to automatically renew or re-authenticate tokens -func (v *OpenBaoManager) tokenRenewalLoop() { - ticker := time.NewTicker(30 * time.Second) // Check every 30 seconds - defer ticker.Stop() - - for { - select { - case <-v.stopCh: - return - case <-ticker.C: - ctx := context.Background() - if err := v.ensureValidToken(ctx); err != nil { - v.logger.Error("openbao token renewal failed", "error", err) - } - } - } -} - -// ensureValidToken checks if the current token is valid and renews or re-authenticates if needed -func (v *OpenBaoManager) ensureValidToken(ctx context.Context) error { - v.tokenMu.Lock() - defer v.tokenMu.Unlock() - - // check current token info - tokenInfo, err := v.client.Auth().Token().LookupSelf() - if err != nil { - // token is invalid, need to re-authenticate - v.logger.Warn("token lookup failed, re-authenticating", "error", err) - return v.reAuthenticate() + if err := manager.testConnection(); err != nil { + return nil, fmt.Errorf("failed to connect to bao proxy: %w", err) } - if tokenInfo == nil || tokenInfo.Data == nil { - return v.reAuthenticate() - } - - // check TTL - ttlRaw, ok := tokenInfo.Data["ttl"] - if !ok { - return v.reAuthenticate() - } - - var ttl int64 - switch t := ttlRaw.(type) { - case int64: - ttl = t - case float64: - ttl = int64(t) - case int: - ttl = int64(t) - default: - return v.reAuthenticate() - } - - // if TTL is less than 5 minutes, try to renew - if ttl < 300 { - v.logger.Info("token ttl low, attempting renewal", "ttl_seconds", ttl) - - renewResp, err := v.client.Auth().Token().RenewSelf(3600) // 1h - if err != nil { - v.logger.Warn("token renewal failed, re-authenticating", "error", err) - return v.reAuthenticate() - } - - if renewResp == nil || renewResp.Auth == nil { - v.logger.Warn("token renewal returned no auth info, re-authenticating") - return v.reAuthenticate() - } - - v.logger.Info("token renewed successfully", "new_ttl_seconds", renewResp.Auth.LeaseDuration) - } - - return nil + logger.Info("successfully connected to bao proxy", "address", proxyAddress) + return manager, nil } -// reAuthenticate performs a fresh authentication using AppRole -func (v *OpenBaoManager) reAuthenticate() error { - v.logger.Info("re-authenticating with approle") +// testConnection verifies that we can connect to the proxy +func (v *OpenBaoManager) testConnection() error { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - err := authenticateAppRole(v.client, v.roleID, v.secretID) + // try token self-lookup as a quick way to verify proxy works + // and is authenticated + _, err := v.client.Auth().Token().LookupSelfWithContext(ctx) if err != nil { - return fmt.Errorf("re-authentication failed: %w", err) + return fmt.Errorf("proxy connection test failed: %w", err) } - v.logger.Info("re-authentication successful") return nil } func (v *OpenBaoManager) AddSecret(ctx context.Context, secret UnlockedSecret) error { - v.tokenMu.RLock() - defer v.tokenMu.RUnlock() if err := ValidateKey(secret.Key); err != nil { return err } secretPath := v.buildSecretPath(secret.Repo, secret.Key) + v.logger.Debug("adding secret", "repo", secret.Repo, "key", secret.Key, "path", secretPath) - fmt.Println(v.mountPath, secretPath) - + // Check if secret already exists existing, err := v.client.KVv2(v.mountPath).Get(ctx, secretPath) if err == nil && existing != nil { + v.logger.Debug("secret already exists", "path", secretPath) return ErrKeyAlreadyPresent } @@ -210,19 +98,35 @@ func (v *OpenBaoManager) AddSecret(ctx context.Context, secret UnlockedSecret) e "created_by": secret.CreatedBy.String(), } - _, err = v.client.KVv2(v.mountPath).Put(ctx, secretPath, secretData) + v.logger.Debug("writing secret to openbao", "path", secretPath, "mount", v.mountPath) + resp, err := v.client.KVv2(v.mountPath).Put(ctx, secretPath, secretData) if err != nil { + v.logger.Error("failed to write secret", "path", secretPath, "error", err) return fmt.Errorf("failed to store secret in openbao: %w", err) } + v.logger.Debug("secret write response", "version", resp.VersionMetadata.Version, "created_time", resp.VersionMetadata.CreatedTime) + + v.logger.Debug("verifying secret was written", "path", secretPath) + readBack, err := v.client.KVv2(v.mountPath).Get(ctx, secretPath) + if err != nil { + v.logger.Error("failed to verify secret after write", "path", secretPath, "error", err) + return fmt.Errorf("secret not found after writing to %s/%s: %w", v.mountPath, secretPath, err) + } + + if readBack == nil || readBack.Data == nil { + v.logger.Error("secret verification returned empty data", "path", secretPath) + return fmt.Errorf("secret verification failed: empty data returned for %s/%s", v.mountPath, secretPath) + } + + v.logger.Info("secret added and verified successfully", "repo", secret.Repo, "key", secret.Key, "version", readBack.VersionMetadata.Version) return nil } func (v *OpenBaoManager) RemoveSecret(ctx context.Context, secret Secret[any]) error { - v.tokenMu.RLock() - defer v.tokenMu.RUnlock() secretPath := v.buildSecretPath(secret.Repo, secret.Key) + // check if secret exists existing, err := v.client.KVv2(v.mountPath).Get(ctx, secretPath) if err != nil || existing == nil { return ErrKeyNotFound @@ -233,15 +137,14 @@ func (v *OpenBaoManager) RemoveSecret(ctx context.Context, secret Secret[any]) e return fmt.Errorf("failed to delete secret from openbao: %w", err) } + v.logger.Debug("secret removed successfully", "repo", secret.Repo, "key", secret.Key) return nil } func (v *OpenBaoManager) GetSecretsLocked(ctx context.Context, repo DidSlashRepo) ([]LockedSecret, error) { - v.tokenMu.RLock() - defer v.tokenMu.RUnlock() repoPath := v.buildRepoPath(repo) - secretsList, err := v.client.Logical().List(fmt.Sprintf("%s/metadata/%s", v.mountPath, repoPath)) + secretsList, err := v.client.Logical().ListWithContext(ctx, fmt.Sprintf("%s/metadata/%s", v.mountPath, repoPath)) if err != nil { if strings.Contains(err.Error(), "no secret found") || strings.Contains(err.Error(), "no handler for route") { return []LockedSecret{}, nil @@ -266,10 +169,11 @@ func (v *OpenBaoManager) GetSecretsLocked(ctx context.Context, repo DidSlashRepo continue } - secretPath := path.Join(repoPath, key) + secretPath := fmt.Sprintf("%s/%s", repoPath, key) secretData, err := v.client.KVv2(v.mountPath).Get(ctx, secretPath) if err != nil { - continue // Skip secrets we can't read + v.logger.Warn("failed to read secret metadata", "path", secretPath, "error", err) + continue } if secretData == nil || secretData.Data == nil { @@ -308,15 +212,14 @@ func (v *OpenBaoManager) GetSecretsLocked(ctx context.Context, repo DidSlashRepo secrets = append(secrets, secret) } + v.logger.Debug("retrieved locked secrets", "repo", repo, "count", len(secrets)) return secrets, nil } func (v *OpenBaoManager) GetSecretsUnlocked(ctx context.Context, repo DidSlashRepo) ([]UnlockedSecret, error) { - v.tokenMu.RLock() - defer v.tokenMu.RUnlock() repoPath := v.buildRepoPath(repo) - secretsList, err := v.client.Logical().List(fmt.Sprintf("%s/metadata/%s", v.mountPath, repoPath)) + secretsList, err := v.client.Logical().ListWithContext(ctx, fmt.Sprintf("%s/metadata/%s", v.mountPath, repoPath)) if err != nil { if strings.Contains(err.Error(), "no secret found") || strings.Contains(err.Error(), "no handler for route") { return []UnlockedSecret{}, nil @@ -341,9 +244,10 @@ func (v *OpenBaoManager) GetSecretsUnlocked(ctx context.Context, repo DidSlashRe continue } - secretPath := path.Join(repoPath, key) + secretPath := fmt.Sprintf("%s/%s", repoPath, key) secretData, err := v.client.KVv2(v.mountPath).Get(ctx, secretPath) if err != nil { + v.logger.Warn("failed to read secret", "path", secretPath, "error", err) continue } @@ -355,7 +259,8 @@ func (v *OpenBaoManager) GetSecretsUnlocked(ctx context.Context, repo DidSlashRe valueStr, ok := data["value"].(string) if !ok { - continue // skip secrets without values + v.logger.Warn("secret missing value", "path", secretPath) + continue } createdAtStr, ok := data["created_at"].(string) @@ -389,10 +294,11 @@ func (v *OpenBaoManager) GetSecretsUnlocked(ctx context.Context, repo DidSlashRe secrets = append(secrets, secret) } + v.logger.Debug("retrieved unlocked secrets", "repo", repo, "count", len(secrets)) return secrets, nil } -// buildRepoPath creates an OpenBao path for a repository +// buildRepoPath creates a safe path for a repository func (v *OpenBaoManager) buildRepoPath(repo DidSlashRepo) string { // convert DidSlashRepo to a safe path by replacing special characters repoPath := strings.ReplaceAll(string(repo), "/", "_") @@ -401,7 +307,7 @@ func (v *OpenBaoManager) buildRepoPath(repo DidSlashRepo) string { return fmt.Sprintf("repos/%s", repoPath) } -// buildSecretPath creates an OpenBao path for a specific secret +// buildSecretPath creates a path for a specific secret func (v *OpenBaoManager) buildSecretPath(repo DidSlashRepo, key string) string { return path.Join(v.buildRepoPath(repo), key) } diff --git a/spindle/secrets/openbao_test.go b/spindle/secrets/openbao_test.go index ff66cc4..7cdd831 100644 --- a/spindle/secrets/openbao_test.go +++ b/spindle/secrets/openbao_test.go @@ -16,7 +16,6 @@ type MockOpenBaoManager struct { secrets map[string]UnlockedSecret // key: repo_key format shouldError bool errorToReturn error - stopped bool } func NewMockOpenBaoManager() *MockOpenBaoManager { @@ -33,14 +32,6 @@ func (m *MockOpenBaoManager) ClearError() { m.errorToReturn = nil } -func (m *MockOpenBaoManager) Stop() { - m.stopped = true -} - -func (m *MockOpenBaoManager) IsStopped() bool { - return m.stopped -} - func (m *MockOpenBaoManager) buildKey(repo DidSlashRepo, key string) string { return string(repo) + "_" + key } @@ -118,6 +109,11 @@ func createTestSecretForOpenBao(repo, key, value, createdBy string) UnlockedSecr } } +// Test MockOpenBaoManager interface compliance +func TestMockOpenBaoManagerInterface(t *testing.T) { + var _ Manager = (*MockOpenBaoManager)(nil) +} + func TestOpenBaoManagerInterface(t *testing.T) { var _ Manager = (*OpenBaoManager)(nil) } @@ -125,56 +121,46 @@ func TestOpenBaoManagerInterface(t *testing.T) { func TestNewOpenBaoManager(t *testing.T) { tests := []struct { name string - address string - roleID string - secretID string + proxyAddr string opts []OpenBaoManagerOpt expectError bool errorContains string }{ { - name: "empty address", - address: "", - roleID: "test-role-id", - secretID: "test-secret-id", + name: "empty proxy address", + proxyAddr: "", opts: nil, expectError: true, - errorContains: "address cannot be empty", + errorContains: "proxy address cannot be empty", }, { - name: "empty role_id", - address: "http://localhost:8200", - roleID: "", - secretID: "test-secret-id", + name: "valid proxy address", + proxyAddr: "http://localhost:8200", opts: nil, - expectError: true, - errorContains: "role_id cannot be empty", + expectError: true, // Will fail because no real proxy is running + errorContains: "failed to connect to bao proxy", }, { - name: "empty secret_id", - address: "http://localhost:8200", - roleID: "test-role-id", - secretID: "", - opts: nil, - expectError: true, - errorContains: "secret_id cannot be empty", + name: "with mount path option", + proxyAddr: "http://localhost:8200", + opts: []OpenBaoManagerOpt{WithMountPath("custom-mount")}, + expectError: true, // Will fail because no real proxy is running + errorContains: "failed to connect to bao proxy", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { logger := slog.New(slog.NewTextHandler(os.Stderr, nil)) - manager, err := NewOpenBaoManager(tt.address, tt.roleID, tt.secretID, logger, tt.opts...) + manager, err := NewOpenBaoManager(tt.proxyAddr, logger, tt.opts...) if tt.expectError { assert.Error(t, err) assert.Nil(t, manager) assert.Contains(t, err.Error(), tt.errorContains) } else { - // For valid configurations, we expect an error during authentication - // since we're not connecting to a real OpenBao server - assert.Error(t, err) - assert.Nil(t, manager) + assert.NoError(t, err) + assert.NotNil(t, manager) } }) } @@ -253,45 +239,6 @@ func TestWithMountPath(t *testing.T) { assert.Equal(t, "custom-mount", manager.mountPath) } -func TestOpenBaoManager_Stop(t *testing.T) { - // Create a manager with minimal setup - manager := &OpenBaoManager{ - mountPath: "test", - stopCh: make(chan struct{}), - } - - // Verify the manager implements Stopper interface - var stopper Stopper = manager - assert.NotNil(t, stopper) - - // Call Stop and verify it doesn't panic - assert.NotPanics(t, func() { - manager.Stop() - }) - - // Verify the channel was closed - select { - case <-manager.stopCh: - // Channel was closed as expected - default: - t.Error("Expected stop channel to be closed after Stop()") - } -} - -func TestOpenBaoManager_StopperInterface(t *testing.T) { - manager := &OpenBaoManager{} - - // Verify that OpenBaoManager implements the Stopper interface - _, ok := interface{}(manager).(Stopper) - assert.True(t, ok, "OpenBaoManager should implement Stopper interface") -} - -// Test MockOpenBaoManager interface compliance -func TestMockOpenBaoManagerInterface(t *testing.T) { - var _ Manager = (*MockOpenBaoManager)(nil) - var _ Stopper = (*MockOpenBaoManager)(nil) -} - func TestMockOpenBaoManager_AddSecret(t *testing.T) { tests := []struct { name string @@ -563,16 +510,6 @@ func TestMockOpenBaoManager_ErrorHandling(t *testing.T) { assert.NoError(t, err) } -func TestMockOpenBaoManager_Stop(t *testing.T) { - mock := NewMockOpenBaoManager() - - assert.False(t, mock.IsStopped()) - - mock.Stop() - - assert.True(t, mock.IsStopped()) -} - func TestMockOpenBaoManager_Integration(t *testing.T) { tests := []struct { name string @@ -628,3 +565,41 @@ func TestMockOpenBaoManager_Integration(t *testing.T) { }) } } + +func TestOpenBaoManager_ProxyConfiguration(t *testing.T) { + tests := []struct { + name string + proxyAddr string + description string + }{ + { + name: "default_localhost", + proxyAddr: "http://127.0.0.1:8200", + description: "Should connect to default localhost proxy", + }, + { + name: "custom_host", + proxyAddr: "http://bao-proxy:8200", + description: "Should connect to custom proxy host", + }, + { + name: "https_proxy", + proxyAddr: "https://127.0.0.1:8200", + description: "Should connect to HTTPS proxy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Log("Testing scenario:", tt.description) + logger := slog.New(slog.NewTextHandler(os.Stderr, nil)) + + // All these will fail because no real proxy is running + // but we can test that the configuration is properly accepted + manager, err := NewOpenBaoManager(tt.proxyAddr, logger) + assert.Error(t, err) // Expected because no real proxy + assert.Nil(t, manager) + assert.Contains(t, err.Error(), "failed to connect to bao proxy") + }) + } +} diff --git a/spindle/secrets/policy.hcl b/spindle/secrets/policy.hcl index 83ba917..f7261df 100644 --- a/spindle/secrets/policy.hcl +++ b/spindle/secrets/policy.hcl @@ -1,15 +1,22 @@ -# KV v2 data operations -path "spindle/data/*" { +# Allow full access to the spindle KV mount +path "spindle/*" { capabilities = ["create", "read", "update", "delete", "list"] } -# KV v2 metadata operations (needed for listing) +path "spindle/data/*" { + capabilities = ["create", "read", "update", "delete"] +} + path "spindle/metadata/*" { capabilities = ["list", "read", "delete"] } -# Root path access (needed for mount-level operations) -path "spindle/*" { - capabilities = ["list"] +# Allow listing mounts (for connection testing) +path "sys/mounts" { + capabilities = ["read"] } +# Allow token self-lookup (for health checks) +path "auth/token/lookup-self" { + capabilities = ["read"] +} diff --git a/spindle/server.go b/spindle/server.go index 5ae5444..a06fa9c 100644 --- a/spindle/server.go +++ b/spindle/server.go @@ -71,26 +71,18 @@ func Run(ctx context.Context) error { var vault secrets.Manager switch cfg.Server.Secrets.Provider { case "openbao": - if cfg.Server.Secrets.OpenBao.Addr == "" { - return fmt.Errorf("openbao address is required when using openbao secrets provider") - } - if cfg.Server.Secrets.OpenBao.RoleID == "" { - return fmt.Errorf("openbao role_id is required when using openbao secrets provider") - } - if cfg.Server.Secrets.OpenBao.SecretID == "" { - return fmt.Errorf("openbao secret_id is required when using openbao secrets provider") + if cfg.Server.Secrets.OpenBao.ProxyAddr == "" { + return fmt.Errorf("openbao proxy address is required when using openbao secrets provider") } vault, err = secrets.NewOpenBaoManager( - cfg.Server.Secrets.OpenBao.Addr, - cfg.Server.Secrets.OpenBao.RoleID, - cfg.Server.Secrets.OpenBao.SecretID, + cfg.Server.Secrets.OpenBao.ProxyAddr, logger, secrets.WithMountPath(cfg.Server.Secrets.OpenBao.Mount), ) if err != nil { return fmt.Errorf("failed to setup openbao secrets provider: %w", err) } - logger.Info("using openbao secrets provider", "address", cfg.Server.Secrets.OpenBao.Addr, "mount", cfg.Server.Secrets.OpenBao.Mount) + logger.Info("using openbao secrets provider", "proxy_address", cfg.Server.Secrets.OpenBao.ProxyAddr, "mount", cfg.Server.Secrets.OpenBao.Mount) case "sqlite", "": vault, err = secrets.NewSQLiteManager(cfg.Server.DBPath, secrets.WithTableName("secrets")) if err != nil { -- 2.43.0