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

VAULT-29784: Skip connection verification on DB config read #28139

Merged
merged 10 commits into from
Aug 21, 2024
14 changes: 13 additions & 1 deletion builtin/logical/database/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,18 @@ func (b *databaseBackend) GetConnection(ctx context.Context, s logical.Storage,
return b.GetConnectionWithConfig(ctx, name, config)
}

func (b *databaseBackend) GetConnectionSkipVerify(ctx context.Context, s logical.Storage, name string) (*dbPluginInstance, error) {
config, err := b.DatabaseConfig(ctx, s, name)
if err != nil {
return nil, err
}

// Force the skip verifying the connection
config.VerifyConnection = false

return b.GetConnectionWithConfig(ctx, name, config)
}

func (b *databaseBackend) GetConnectionWithConfig(ctx context.Context, name string, config *DatabaseConfig) (*dbPluginInstance, error) {
// fast path, reuse the existing connection
dbi := b.connections.Get(name)
Expand Down Expand Up @@ -331,7 +343,7 @@ func (b *databaseBackend) GetConnectionWithConfig(ctx context.Context, name stri

initReq := v5.InitializeRequest{
Config: config.ConnectionDetails,
VerifyConnection: true,
VerifyConnection: config.VerifyConnection,
}
_, err = dbw.Initialize(ctx, initReq)
if err != nil {
Expand Down
21 changes: 15 additions & 6 deletions builtin/logical/database/path_config_connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ type DatabaseConfig struct {

RootCredentialsRotateStatements []string `json:"root_credentials_rotate_statements" structs:"root_credentials_rotate_statements" mapstructure:"root_credentials_rotate_statements"`

PasswordPolicy string `json:"password_policy" structs:"password_policy" mapstructure:"password_policy"`
PasswordPolicy string `json:"password_policy" structs:"password_policy" mapstructure:"password_policy"`
VerifyConnection bool `json:"verify_connection" structs:"verify_connection" mapstructure:"verify_connection"`
}

func (c *DatabaseConfig) SupportsCredentialType(credentialType v5.CredentialType) bool {
Expand Down Expand Up @@ -378,7 +379,7 @@ func (b *databaseBackend) connectionReadHandler() framework.OperationFunc {
delete(config.ConnectionDetails, "service_account_json")

resp := &logical.Response{}
if dbi, err := b.GetConnection(ctx, req.Storage, name); err == nil {
if dbi, err := b.GetConnectionSkipVerify(ctx, req.Storage, name); err == nil {
config.RunningPluginVersion = dbi.runningPluginVersion
if config.PluginVersion != "" && config.PluginVersion != config.RunningPluginVersion {
warning := fmt.Sprintf("Plugin version is configured as %q, but running %q", config.PluginVersion, config.RunningPluginVersion)
Expand All @@ -392,6 +393,7 @@ func (b *databaseBackend) connectionReadHandler() framework.OperationFunc {
}

resp.Data = structs.New(config).Map()
delete(resp.Data, "verify_connection")
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like something we could keep in the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on the fence on this as well. I agree we can probably keep it so I'll remove that line!

return resp, nil
}
}
Expand Down Expand Up @@ -422,15 +424,15 @@ func (b *databaseBackend) connectionDeleteHandler() framework.OperationFunc {
// both builtin and plugin database types.
func (b *databaseBackend) connectionWriteHandler() framework.OperationFunc {
return func(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
verifyConnection := data.Get("verify_connection").(bool)

name := data.Get("name").(string)
if name == "" {
return logical.ErrorResponse(respErrEmptyName), nil
}

// Baseline
config := &DatabaseConfig{}
config := &DatabaseConfig{
VerifyConnection: true,
}

entry, err := req.Storage.Get(ctx, fmt.Sprintf("config/%s", name))
if err != nil {
Expand All @@ -442,6 +444,13 @@ func (b *databaseBackend) connectionWriteHandler() framework.OperationFunc {
}
}

// If this value was provided as part of the request we want to set it to this value
if verifyConnectionRaw, ok := data.GetOk("verify_connection"); ok {
config.VerifyConnection = verifyConnectionRaw.(bool)
} else if req.Operation == logical.CreateOperation {
config.VerifyConnection = data.Get("verify_connection").(bool)
}

if pluginNameRaw, ok := data.GetOk("plugin_name"); ok {
config.PluginName = pluginNameRaw.(string)
} else if req.Operation == logical.CreateOperation {
Expand Down Expand Up @@ -509,7 +518,7 @@ func (b *databaseBackend) connectionWriteHandler() framework.OperationFunc {

initReq := v5.InitializeRequest{
Config: config.ConnectionDetails,
VerifyConnection: verifyConnection,
VerifyConnection: config.VerifyConnection,
}
initResp, err := dbw.Initialize(ctx, initReq)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions changelog/28139.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
secrets/database: Skip connection verification on reading existing DB connection configuration
```
Loading