This repository was archived by the owner on Apr 16, 2026. It is now read-only.
Merged
Conversation
6 tasks
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit 9ee6603. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed:
QueryEscapeencodes spaces as+in DSN credentials- DSN userinfo now uses Go's URL userinfo escaping so spaces become
%20instead of+and reserved characters are encoded correctly.
- DSN userinfo now uses Go's URL userinfo escaping so spaces become
- ✅ Fixed: Vault lease leaked when
extendLeaseForGrantfailsIssuenow best-effort revokes the generated Vault lease whenever lease extension fails before returning the error.
Preview (d87047c7e7)
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -262,6 +262,7 @@
| `ASB_VAULT_TOKEN` | Vault authentication token |
| `ASB_VAULT_NAMESPACE` | Vault namespace |
| `ASB_VAULT_ROLE` | Vault DB role name |
+| `ASB_VAULT_ALLOWED_ROLE_SUFFIXES` | Comma-separated allowed Vault DB role suffixes (default `_ro`) |
| `ASB_VAULT_DSN_TEMPLATE` | DSN template for rendered credentials |
| `ASB_BROWSER_ORIGIN` | Allowed browser origin (demo) |
| `ASB_BROWSER_USERNAME` | Browser credential username (demo) |
diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go
--- a/internal/bootstrap/service.go
+++ b/internal/bootstrap/service.go
@@ -191,42 +191,34 @@
})
}
- if vaultAddr := os.Getenv("ASB_VAULT_ADDR"); vaultAddr != "" {
- role := getenv("ASB_VAULT_ROLE", "analytics_ro")
- dsnTemplate := os.Getenv("ASB_VAULT_DSN_TEMPLATE")
- if dsnTemplate != "" {
- vaultClient := vaultdb.NewHTTPClient(vaultdb.HTTPClientConfig{
- BaseURL: vaultAddr,
- Token: os.Getenv("ASB_VAULT_TOKEN"),
- Namespace: os.Getenv("ASB_VAULT_NAMESPACE"),
- })
- connectorOptions = append(connectorOptions, resolver.WithVaultDB(vaultdb.NewConnector(vaultdb.Config{
- Client: vaultClient,
- RoleDSNs: map[string]string{
- role: dsnTemplate,
- },
- })))
- mustRegisterToolAndPolicy(ctx, logger, tools, engine, tenantID, core.Tool{
- TenantID: tenantID,
- Tool: "db",
- ManifestHash: "sha256:dev-db",
- RuntimeClass: core.RuntimeClassSidecar,
- AllowedDeliveryModes: []core.DeliveryMode{core.DeliveryModeWrappedSecret},
- AllowedCapabilities: []string{"db.read"},
- TrustTags: []string{"trusted", "db"},
- }, core.Policy{
- TenantID: tenantID,
- Capability: "db.read",
- ResourceKind: core.ResourceKindDBRole,
- AllowedDeliveryModes: []core.DeliveryMode{core.DeliveryModeWrappedSecret},
- DefaultTTL: 10 * time.Minute,
- MaxTTL: 30 * time.Minute,
- ApprovalMode: core.ApprovalModeNone,
- RequiredToolTags: []string{"trusted", "db"},
- Condition: `true`,
- })
- }
+ vaultConnector, err := newVaultDBConnector()
+ if err != nil {
+ cleanupRuntime()
+ cleanupRepository()
+ return nil, err
}
+ if vaultConnector != nil {
+ connectorOptions = append(connectorOptions, resolver.WithVaultDB(vaultConnector))
+ mustRegisterToolAndPolicy(ctx, logger, tools, engine, tenantID, core.Tool{
+ TenantID: tenantID,
+ Tool: "db",
+ ManifestHash: "sha256:dev-db",
+ RuntimeClass: core.RuntimeClassSidecar,
+ AllowedDeliveryModes: []core.DeliveryMode{core.DeliveryModeWrappedSecret},
+ AllowedCapabilities: []string{"db.read"},
+ TrustTags: []string{"trusted", "db"},
+ }, core.Policy{
+ TenantID: tenantID,
+ Capability: "db.read",
+ ResourceKind: core.ResourceKindDBRole,
+ AllowedDeliveryModes: []core.DeliveryMode{core.DeliveryModeWrappedSecret},
+ DefaultTTL: 10 * time.Minute,
+ MaxTTL: 30 * time.Minute,
+ ApprovalMode: core.ApprovalModeNone,
+ RequiredToolTags: []string{"trusted", "db"},
+ Condition: `true`,
+ })
+ }
delegationValidator, err := newDelegationValidator()
if err != nil {
@@ -518,6 +510,7 @@
}
func loadPublicKey(path string) (any, error) {
+ // #nosec G304 -- Operator-managed key file paths are supplied via trusted service configuration.
contents, err := os.ReadFile(path)
if err != nil {
return nil, err
@@ -550,6 +543,7 @@
}
func loadEd25519PrivateKey(path string) (ed25519.PrivateKey, error) {
+ // #nosec G304 -- Operator-managed key file paths are supplied via trusted service configuration.
contents, err := os.ReadFile(path)
if err != nil {
return nil, err
@@ -570,6 +564,7 @@
}
func loadRSAPrivateKey(path string) (*rsa.PrivateKey, error) {
+ // #nosec G304 -- Operator-managed key file paths are supplied via trusted service configuration.
contents, err := os.ReadFile(path)
if err != nil {
return nil, err
@@ -599,6 +594,46 @@
return fallback
}
+func csvEnvOr(key string, fallback []string) []string {
+ value := strings.TrimSpace(os.Getenv(key))
+ if value == "" {
+ return append([]string(nil), fallback...)
+ }
+ parts := strings.Split(value, ",")
+ values := make([]string, 0, len(parts))
+ for _, part := range parts {
+ trimmed := strings.TrimSpace(part)
+ if trimmed != "" {
+ values = append(values, trimmed)
+ }
+ }
+ if len(values) == 0 {
+ return append([]string(nil), fallback...)
+ }
+ return values
+}
+
+func newVaultDBConnector() (*vaultdb.Connector, error) {
+ vaultAddr := strings.TrimSpace(os.Getenv("ASB_VAULT_ADDR"))
+ dsnTemplate := strings.TrimSpace(os.Getenv("ASB_VAULT_DSN_TEMPLATE"))
+ if vaultAddr == "" || dsnTemplate == "" {
+ return nil, nil
+ }
+
+ role := getenv("ASB_VAULT_ROLE", "analytics_ro")
+ return vaultdb.NewConnector(vaultdb.Config{
+ AllowedRoleSuffixes: csvEnvOr("ASB_VAULT_ALLOWED_ROLE_SUFFIXES", []string{"_ro"}),
+ Client: vaultdb.NewHTTPClient(vaultdb.HTTPClientConfig{
+ BaseURL: vaultAddr,
+ Token: os.Getenv("ASB_VAULT_TOKEN"),
+ Namespace: os.Getenv("ASB_VAULT_NAMESPACE"),
+ }),
+ RoleDSNs: map[string]string{
+ role: dsnTemplate,
+ },
+ })
+}
+
func mustRegisterToolAndPolicy(ctx context.Context, logger *slog.Logger, tools *toolregistry.Registry, engine *policy.Engine, tenantID string, tool core.Tool, pol core.Policy) {
if err := tools.Put(ctx, tool); err != nil {
logger.Error("register tool", "tenant_id", tenantID, "tool", tool.Tool, "error", err)
diff --git a/internal/bootstrap/service_test.go b/internal/bootstrap/service_test.go
--- a/internal/bootstrap/service_test.go
+++ b/internal/bootstrap/service_test.go
@@ -66,3 +66,31 @@
t.Fatal("newApprovalNotifier() = nil, want configured notifier")
}
}
+
+func TestNewVaultDBConnectorUsesConfiguredRoleSuffixes(t *testing.T) {
+ t.Setenv("ASB_VAULT_ADDR", "https://vault.internal")
+ t.Setenv("ASB_VAULT_DSN_TEMPLATE", "postgres://{{username}}:{{password}}@db.internal/app")
+ t.Setenv("ASB_VAULT_ROLE", "analytics_readonly")
+ t.Setenv("ASB_VAULT_ALLOWED_ROLE_SUFFIXES", "_readonly,_reader")
+
+ connector, err := newVaultDBConnector()
+ if err != nil {
+ t.Fatalf("newVaultDBConnector() error = %v", err)
+ }
+ if connector == nil {
+ t.Fatal("newVaultDBConnector() = nil, want connector")
+ }
+}
+
+func TestNewVaultDBConnectorFailsForInvalidTemplates(t *testing.T) {
+ t.Setenv("ASB_VAULT_ADDR", "https://vault.internal")
+ t.Setenv("ASB_VAULT_DSN_TEMPLATE", "postgres://db.internal/app")
+
+ connector, err := newVaultDBConnector()
+ if err == nil || !strings.Contains(err.Error(), "placeholders") {
+ t.Fatalf("newVaultDBConnector() error = %v, want placeholder validation error", err)
+ }
+ if connector != nil {
+ t.Fatalf("newVaultDBConnector() = %#v, want nil", connector)
+ }
+}
diff --git a/internal/connectors/vaultdb/client.go b/internal/connectors/vaultdb/client.go
--- a/internal/connectors/vaultdb/client.go
+++ b/internal/connectors/vaultdb/client.go
@@ -6,25 +6,31 @@
"encoding/json"
"fmt"
"io"
+ "math"
"net/http"
"strings"
"time"
"github.com/evalops/asb/internal/core"
+ "github.com/evalops/service-runtime/resilience"
)
type HTTPClientConfig struct {
- BaseURL string
- Token string
- Namespace string
- Client *http.Client
+ BaseURL string
+ Token string
+ Namespace string
+ Client *http.Client
+ RenewRetry resilience.RetryConfig
+ RevokeRetry resilience.RetryConfig
}
type HTTPClient struct {
- baseURL string
- token string
- namespace string
- client *http.Client
+ baseURL string
+ token string
+ namespace string
+ client *http.Client
+ renewRetry resilience.RetryConfig
+ revokeRetry resilience.RetryConfig
}
func NewHTTPClient(cfg HTTPClientConfig) *HTTPClient {
@@ -32,11 +38,33 @@
if client == nil {
client = http.DefaultClient
}
+ revokeRetry := cfg.RevokeRetry
+ if revokeRetry.MaxAttempts == 0 {
+ revokeRetry.MaxAttempts = 4
+ }
+ if revokeRetry.InitialDelay == 0 {
+ revokeRetry.InitialDelay = 100 * time.Millisecond
+ }
+ if revokeRetry.MaxDelay == 0 {
+ revokeRetry.MaxDelay = time.Second
+ }
+ renewRetry := cfg.RenewRetry
+ if renewRetry.MaxAttempts == 0 {
+ renewRetry.MaxAttempts = 3
+ }
+ if renewRetry.InitialDelay == 0 {
+ renewRetry.InitialDelay = 100 * time.Millisecond
+ }
+ if renewRetry.MaxDelay == 0 {
+ renewRetry.MaxDelay = time.Second
+ }
return &HTTPClient{
- baseURL: strings.TrimRight(cfg.BaseURL, "/"),
- token: cfg.Token,
- namespace: cfg.Namespace,
- client: client,
+ baseURL: strings.TrimRight(cfg.BaseURL, "/"),
+ token: cfg.Token,
+ namespace: cfg.Namespace,
+ client: client,
+ renewRetry: renewRetry,
+ revokeRetry: revokeRetry,
}
}
@@ -51,7 +79,9 @@
if err != nil {
return nil, err
}
- defer response.Body.Close()
+ defer func() {
+ _ = response.Body.Close()
+ }()
body, err := io.ReadAll(response.Body)
if err != nil {
return nil, err
@@ -63,6 +93,7 @@
var payload struct {
LeaseID string `json:"lease_id"`
LeaseDuration int64 `json:"lease_duration"`
+ Renewable bool `json:"renewable"`
Data struct {
Username string `json:"username"`
Password string `json:"password"`
@@ -76,17 +107,79 @@
Password: payload.Data.Password,
LeaseID: payload.LeaseID,
LeaseDuration: time.Duration(payload.LeaseDuration) * time.Second,
+ Renewable: payload.Renewable,
}, nil
}
+func (c *HTTPClient) RenewLease(ctx context.Context, leaseID string, increment time.Duration) (*LeaseCredentials, error) {
+ return resilience.RetryValue(ctx, c.renewRetry, func(ctx context.Context) (*LeaseCredentials, error) {
+ return c.renewLeaseOnce(ctx, leaseID, increment)
+ })
+}
+
func (c *HTTPClient) RevokeLease(ctx context.Context, leaseID string) error {
+ return resilience.Retry(ctx, c.revokeRetry, func(ctx context.Context) error {
+ return c.revokeLeaseOnce(ctx, leaseID)
+ })
+}
+
+func (c *HTTPClient) renewLeaseOnce(ctx context.Context, leaseID string, increment time.Duration) (*LeaseCredentials, error) {
+ payload, err := json.Marshal(map[string]any{
+ "lease_id": leaseID,
+ "increment": int(math.Ceil(increment.Seconds())),
+ })
+ if err != nil {
+ return nil, resilience.Permanent(err)
+ }
+ request, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/v1/sys/leases/renew", bytes.NewReader(payload))
+ if err != nil {
+ return nil, resilience.Permanent(err)
+ }
+ c.applyHeaders(request)
+ request.Header.Set("Content-Type", "application/json")
+
+ response, err := c.client.Do(request)
+ if err != nil {
+ return nil, err
+ }
+ defer func() {
+ _ = response.Body.Close()
+ }()
+ body, err := io.ReadAll(response.Body)
+ if err != nil {
+ return nil, err
+ }
+ if response.StatusCode >= 400 {
+ wrapped := fmt.Errorf("%w: vault renew lease returned %d: %s", core.ErrForbidden, response.StatusCode, string(body))
+ if response.StatusCode == http.StatusTooManyRequests || response.StatusCode >= http.StatusInternalServerError {
+ return nil, wrapped
+ }
+ return nil, resilience.Permanent(wrapped)
+ }
+
+ var renewed struct {
+ LeaseID string `json:"lease_id"`
+ LeaseDuration int64 `json:"lease_duration"`
+ Renewable bool `json:"renewable"`
+ }
+ if err := json.Unmarshal(body, &renewed); err != nil {
+ return nil, resilience.Permanent(err)
+ }
+ return &LeaseCredentials{
+ LeaseID: renewed.LeaseID,
+ LeaseDuration: time.Duration(renewed.LeaseDuration) * time.Second,
+ Renewable: renewed.Renewable,
+ }, nil
+}
+
+func (c *HTTPClient) revokeLeaseOnce(ctx context.Context, leaseID string) error {
body, err := json.Marshal(map[string]string{"lease_id": leaseID})
if err != nil {
- return err
+ return resilience.Permanent(err)
}
request, err := http.NewRequestWithContext(ctx, http.MethodPut, c.baseURL+"/v1/sys/leases/revoke", bytes.NewReader(body))
if err != nil {
- return err
+ return resilience.Permanent(err)
}
c.applyHeaders(request)
request.Header.Set("Content-Type", "application/json")
@@ -95,13 +188,19 @@
if err != nil {
return err
}
- defer response.Body.Close()
+ defer func() {
+ _ = response.Body.Close()
+ }()
payload, err := io.ReadAll(response.Body)
if err != nil {
return err
}
if response.StatusCode >= 400 {
- return fmt.Errorf("%w: vault revoke lease returned %d: %s", core.ErrForbidden, response.StatusCode, string(payload))
+ wrapped := fmt.Errorf("%w: vault revoke lease returned %d: %s", core.ErrForbidden, response.StatusCode, string(payload))
+ if response.StatusCode == http.StatusTooManyRequests || response.StatusCode >= http.StatusInternalServerError {
+ return wrapped
+ }
+ return resilience.Permanent(wrapped)
}
return nil
}
diff --git a/internal/connectors/vaultdb/client_test.go b/internal/connectors/vaultdb/client_test.go
--- a/internal/connectors/vaultdb/client_test.go
+++ b/internal/connectors/vaultdb/client_test.go
@@ -5,9 +5,12 @@
"io"
"net/http"
"net/http/httptest"
+ "sync/atomic"
"testing"
+ "time"
"github.com/evalops/asb/internal/connectors/vaultdb"
+ "github.com/evalops/service-runtime/resilience"
)
func TestHTTPClient_GenerateCredentialsAndRevokeLease(t *testing.T) {
@@ -22,8 +25,19 @@
_, _ = w.Write([]byte(`{
"lease_id":"database/creds/analytics_ro/123",
"lease_duration":600,
+ "renewable":true,
"data":{"username":"dyn-user","password":"dyn-pass"}
}`))
+ case "/v1/sys/leases/renew":
+ body, _ := io.ReadAll(r.Body)
+ if string(body) != `{"increment":1800,"lease_id":"database/creds/analytics_ro/123"}` {
+ t.Fatalf("renew body = %s, want lease renew payload", string(body))
+ }
+ _, _ = w.Write([]byte(`{
+ "lease_id":"database/creds/analytics_ro/123",
+ "lease_duration":1800,
+ "renewable":true
+ }`))
case "/v1/sys/leases/revoke":
body, _ := io.ReadAll(r.Body)
if string(body) != `{"lease_id":"database/creds/analytics_ro/123"}` {
@@ -48,7 +62,80 @@
if lease.Username != "dyn-user" || lease.LeaseID != "database/creds/analytics_ro/123" {
t.Fatalf("lease = %#v, want parsed vault lease", lease)
}
+ renewed, err := client.RenewLease(context.Background(), lease.LeaseID, 30*time.Minute)
+ if err != nil {
+ t.Fatalf("RenewLease() error = %v", err)
+ }
+ if renewed.LeaseDuration != 30*time.Minute {
+ t.Fatalf("renewed lease = %#v, want renewed duration", renewed)
+ }
if err := client.RevokeLease(context.Background(), lease.LeaseID); err != nil {
t.Fatalf("RevokeLease() error = %v", err)
}
}
+
+func TestHTTPClient_RevokeLeaseRetriesTransientFailures(t *testing.T) {
+ t.Parallel()
+
+ var attempts atomic.Int32
+ server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ if r.URL.Path != "/v1/sys/leases/revoke" {
+ t.Fatalf("unexpected path %q", r.URL.Path)
+ }
+ attempt := attempts.Add(1)
+ if attempt < 3 {
+ http.Error(w, "vault warming up", http.StatusServiceUnavailable)
+ return
+ }
+ w.WriteHeader(http.StatusNoContent)
+ }))
+ defer server.Close()
+
+ client := vaultdb.NewHTTPClient(vaultdb.HTTPClientConfig{
+ BaseURL: server.URL,
+ Token: "vault-token",
+ Client: server.Client(),
+ RevokeRetry: resilience.RetryConfig{
+ MaxAttempts: 4,
+ InitialDelay: time.Millisecond,
+ MaxDelay: time.Millisecond,
+ },
+ })
+ if err := client.RevokeLease(context.Background(), "lease-123"); err != nil {
+ t.Fatalf("RevokeLease() error = %v", err)
+ }
+ if attempts.Load() != 3 {
+ t.Fatalf("revoke attempts = %d, want 3", attempts.Load())
+ }
+}
+
+func TestHTTPClient_RevokeLeaseDoesNotRetryPermanentFailures(t *testing.T) {
+ t.Parallel()
+
+ var attempts atomic.Int32
+ server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ if r.URL.Path != "/v1/sys/leases/revoke" {
+ t.Fatalf("unexpected path %q", r.URL.Path)
+ }
+ attempts.Add(1)
+ http.Error(w, "forbidden", http.StatusForbidden)
+ }))
+ defer server.Close()
+
+ client := vaultdb.NewHTTPClient(vaultdb.HTTPClientConfig{
+ BaseURL: server.URL,
+ Token: "vault-token",
+ Client: server.Client(),
+ RevokeRetry: resilience.RetryConfig{
+ MaxAttempts: 4,
+ InitialDelay: time.Millisecond,
+ MaxDelay: time.Millisecond,
+ },
+ })
+ if err := client.RevokeLease(context.Background(), "lease-123"); err == nil {
+ t.Fatal("RevokeLease() error = nil, want permanent error")
+ }
+ if attempts.Load() != 1 {
+ t.Fatalf("revoke attempts = %d, want 1", attempts.Load())
+ }
+}
diff --git a/internal/connectors/vaultdb/connector.go b/internal/connectors/vaultdb/connector.go
--- a/internal/connectors/vaultdb/connector.go
+++ b/internal/connectors/vaultdb/connector.go
@@ -3,6 +3,7 @@
import (
"context"
"fmt"
+ "net/url"
"strings"
"text/template"
"time"
@@ -15,30 +16,73 @@
Password string
LeaseID string
LeaseDuration time.Duration
+ Renewable bool
}
type Client interface {
GenerateCredentials(ctx context.Context, role string) (*LeaseCredentials, error)
+ RenewLease(ctx context.Context, leaseID string, increment time.Duration) (*LeaseCredentials, error)
RevokeLease(ctx context.Context, leaseID string) error
}
type Config struct {
- Client Client
- RoleDSNs map[string]string
+ AllowedRoleSuffixes []string
+ Client Client
+ RoleDSNs map[string]string
}
type Connector struct {
- client Client
- roleDSNs map[string]string
+ allowedRoleSuffixes []string
+ client Client
+ roleDSNs map[string]string
}
-func NewConnector(cfg Config) *Connector {
+func NewConnector(cfg Config) (*Connector, error) {
+ roleDSNs, err := normalizeRoleDSNs(cfg.RoleDSNs)
+ if err != nil {
+ return nil, err
+ }
return &Connector{
- client: cfg.Client,
- roleDSNs: cfg.RoleDSNs,
+ allowedRoleSuffixes: normalizeRoleSuffixes(cfg.AllowedRoleSuffixes),
+ client: cfg.Client,
+ roleDSNs: roleDSNs,
+ }, nil
+}
+
+func normalizeRoleDSNs(roleDSNs map[string]string) (map[string]string, error) {
+ if len(roleDSNs) == 0 {
+ return map[string]string{}, nil
}
+
+ normalized := make(map[string]string, len(roleDSNs))
+ for role, pattern := range roleDSNs {
+ renderPattern, err := normalizeDSNTemplate(pattern)
+ if err != nil {
+ return nil, fmt.Errorf("%w: role %q: %v", core.ErrInvalidRequest, role, err)
+ }
+ normalized[role] = renderPattern
+ }
+ return normalized, nil
}
+func normalizeRoleSuffixes(suffixes []string) []string {
+ if len(suffixes) == 0 {
+ return []string{"_ro"}
+ }
+
+ normalized := make([]string, 0, len(suffixes))
+ for _, suffix := range suffixes {
+ trimmed := strings.TrimSpace(suffix)
+ if trimmed != "" {
+ normalized = append(normalized, trimmed)
+ }
+ }
+ if len(normalized) == 0 {
+ return []string{"_ro"}
+ }
+ return normalized
+}
+
func (c *Connector) Kind() string {
return "vaultdb"
}
@@ -48,16 +92,7 @@
if err != nil {
return err
}
- if resource.Kind != core.ResourceKindDBRole {
- return fmt.Errorf("%w: vault db connector only supports db roles", core.ErrInvalidRequest)
- }
- if !strings.HasSuffix(resource.Name, "_ro") {
- return fmt.Errorf("%w: v1 only allows read-only db roles", core.ErrForbidden)
- }
- if _, ok := c.roleDSNs[resource.Name]; !ok {
- return fmt.Errorf("%w: no DSN template configured for %q", core.ErrNotFound, resource.Name)
- }
- return nil
+ return c.validateRole(resource.Kind, resource.Name)
}
func (c *Connector) Issue(ctx context.Context, req core.IssueRequest) (*core.IssuedArtifact, error) {
@@ -70,11 +105,22 @@
if req.Grant.DeliveryMode != core.DeliveryModeWrappedSecret {
return nil, fmt.Errorf("%w: vault db connector only supports wrapped secret delivery", core.ErrInvalidRequest)
}
+ if err := c.validateRole(req.Resource.Kind, req.Resource.Name); err != nil {
+ return nil, err
+ }
lease, err := c.client.GenerateCredentials(ctx, req.Resource.Name)
if err != nil {
return nil, err
}
+ extendedLease, leaseExpiresAt, err := c.extendLeaseForGrant(ctx, lease, req.Grant.ExpiresAt)
+ if err != nil {
+ if lease != nil && lease.LeaseID != "" {
+ _ = c.client.RevokeLease(ctx, lease.LeaseID)
+ }
+ return nil, err
+ }
+ lease = extendedLease
dsn, err := renderDSN(c.roleDSNs[req.Resource.Name], lease)
if err != nil {
return nil, err
@@ -83,16 +129,17 @@
return &core.IssuedArtifact{
Kind: core.ArtifactKindWrappedSecret,
Metadata: map[string]string{
- "artifact_id": "art_" + req.Grant.ID,
- "lease_id": lease.LeaseID,
- "db_role": req.Resource.Name,
+ "artifact_id": "art_" + req.Grant.ID,
+ "lease_id": lease.LeaseID,
+ "lease_expires_at": leaseExpiresAt.UTC().Format(time.RFC3339),
+ "db_role": req.Resource.Name,
},
SecretData: map[string]string{
"username": lease.Username,
"password": lease.Password,
"dsn": dsn,
},
- ExpiresAt: minTime(req.Grant.ExpiresAt, time.Now().Add(lease.LeaseDuration)),
+ ExpiresAt: minTime(req.Grant.ExpiresAt, leaseExpiresAt),
}, nil
}
@@ -107,23 +154,100 @@
return c.client.RevokeLease(ctx, leaseID)
}
-func renderDSN(pattern string, lease *LeaseCredentials) (string, error) {
- pattern = strings.ReplaceAll(pattern, "{{username}}", "{{.username}}")
- pattern = strings.ReplaceAll(pattern, "{{password}}", "{{.password}}")
- tpl, err := template.New("dsn").Parse(pattern)
+func (c *Connector) validateRole(kind core.ResourceKind, role string) error {
+ if kind != core.ResourceKindDBRole {
+ return fmt.Errorf("%w: vault db connector only supports db roles", core.ErrInvalidRequest)
+ }
+ if !c.allowedRole(role) {
+ return fmt.Errorf("%w: db role %q must match one of the allowed suffixes %v", core.ErrForbidden, role, c.allowedRoleSuffixes)
+ }
+ if _, ok := c.roleDSNs[role]; !ok {
+ return fmt.Errorf("%w: no DSN template configured for %q", core.ErrNotFound, role)
+ }
+ return nil
+}
+
+func (c *Connector) allowedRole(role string) bool {
+ for _, suffix := range c.allowedRoleSuffixes {
+ if strings.HasSuffix(role, suffix) {
+ return true
+ }
+ }
+ return false
+}
+
+func normalizeDSNTemplate(pattern string) (string, error) {
+ trimmed := strings.TrimSpace(pattern)
+ if !strings.Contains(trimmed, "{{username}}") || !strings.Contains(trimmed, "{{password}}") {
+ return "", fmt.Errorf("dsn template must include {{username}} and {{password}} placeholders")
+ }
+ trimmed = strings.ReplaceAll(trimmed, "{{username}}", "{{.username}}")
+ trimmed = strings.ReplaceAll(trimmed, "{{password}}", "{{.password}}")
+ if _, err := template.New("dsn").Parse(trimmed); err != nil {
+ return "", fmt.Errorf("parse dsn template: %v", err)
+ }
+ return trimmed, nil
+}
+
+func renderDSN(renderPattern string, lease *LeaseCredentials) (string, error) {
+ tpl, err := template.New("dsn").Parse(renderPattern)
if err != nil {
return "", fmt.Errorf("%w: parse dsn template: %v", core.ErrInvalidRequest, err)
}
var builder strings.Builder
if err := tpl.Execute(&builder, map[string]string{
- "username": lease.Username,
- "password": lease.Password,
+ "username": url.User(lease.Username).String(),
+ "password": strings.TrimPrefix(url.UserPassword("", lease.Password).String(), ":"),
}); err != nil {
return "", fmt.Errorf("%w: render dsn template: %v", core.ErrInvalidRequest, err)
}
return builder.String(), nil
}
+func (c *Connector) extendLeaseForGrant(ctx context.Context, lease *LeaseCredentials, grantExpiresAt time.Time) (*LeaseCredentials, time.Time, error) {
+ now := time.Now()
+ leaseExpiresAt := now.Add(lease.LeaseDuration)
+ if grantExpiresAt.IsZero() || !grantExpiresAt.After(leaseExpiresAt) {
+ return lease, leaseExpiresAt, nil
+ }
+ if lease.LeaseID == "" || !lease.Renewable {
+ return nil, time.Time{}, fmt.Errorf("%w: vault lease expires before the requested grant ttl and is not renewable", core.ErrForbidden)
+ }
+
+ remaining := time.Until(grantExpiresAt)
+ renewed, err := c.client.RenewLease(ctx, lease.LeaseID, remaining)
+ if err != nil {
+ return nil, time.Time{}, err
+ }
+ merged := mergeLeaseCredentials(lease, renewed)
+ leaseExpiresAt = time.Now().Add(merged.LeaseDuration)
+ if grantExpiresAt.After(leaseExpiresAt) {
+ return nil, time.Time{}, fmt.Errorf("%w: vault lease renewal for %q was capped before the requested grant ttl", core.ErrForbidden, lease.LeaseID)
+ }
+ return merged, leaseExpiresAt, nil
+}
+
+func mergeLeaseCredentials(current *LeaseCredentials, updated *LeaseCredentials) *LeaseCredentials {
+ if updated == nil {
+ return current
+ }
+ merged := *current
+ if updated.Username != "" {
+ merged.Username = updated.Username
+ }
+ if updated.Password != "" {
+ merged.Password = updated.Password
+ }
+ if updated.LeaseID != "" {
+ merged.LeaseID = updated.LeaseID
+ }
+ if updated.LeaseDuration > 0 {
+ merged.LeaseDuration = updated.LeaseDuration
+ }
+ merged.Renewable = updated.Renewable
+ return &merged
+}
+
func minTime(a time.Time, b time.Time) time.Time {
if a.IsZero() {
return b
diff --git a/internal/connectors/vaultdb/connector_test.go b/internal/connectors/vaultdb/connector_test.go
--- a/internal/connectors/vaultdb/connector_test.go
+++ b/internal/connectors/vaultdb/connector_test.go
@@ -2,6 +2,7 @@
import (
"context"
+ "strings"
"testing"
"time"
@@ -14,18 +15,22 @@
client := &fakeVaultClient{
lease: &vaultdb.LeaseCredentials{
- Username: "v-token-user",
- Password: "secret",
+ Username: "v token user",
+ Password: "secret with spaces:/?#[]@",
LeaseID: "database/creds/analytics_ro/123",
LeaseDuration: 10 * time.Minute,
... diff truncated: showing 800 of 1008 linesYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 79fd5b5. Configure here.
haasonsaas
force-pushed
the
codex/asb-vaultdb-hardening
branch
from
d87047c to
9233be2
Compare
Collaborator
Author
|
Addressed the remaining Bugbot finding in
|
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Summary
Closes #13
Validation
go test ./internal/connectors/vaultdb -count=1go test ./internal/bootstrap ./internal/connectors/vaultdb -count=1go test ./internal/connectors/vaultdb ./internal/bootstrap -race -count=1go test ./... -count=1GOTOOLCHAIN=go1.26.0 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3 run --timeout=5m ./internal/connectors/vaultdb ./internal/bootstrapgo run github.com/securego/gosec/v2/cmd/gosec@v2.22.4 ./internal/connectors/vaultdb ./internal/bootstrap