diff --git a/.nextchanges/bundles/sensitive-variables.md b/.nextchanges/bundles/sensitive-variables.md new file mode 100644 index 00000000000..c650b0f9ab5 --- /dev/null +++ b/.nextchanges/bundles/sensitive-variables.md @@ -0,0 +1 @@ +* Add `sensitive: true` for bundle variables. A variable used as the value of a secret is sensitive by default. `bundle validate -o json` redacts only sensitive variables instead of every string variable, and a sensitive variable can only be referenced from a sensitive resource field (such as a secret's `value`) or from another sensitive variable. ([#7288](https://github.com/databricks/cli/pull/7288)) diff --git a/.nextchanges/notable-changes/sensitive-variables.md b/.nextchanges/notable-changes/sensitive-variables.md new file mode 100644 index 00000000000..0fb708dd471 --- /dev/null +++ b/.nextchanges/notable-changes/sensitive-variables.md @@ -0,0 +1 @@ +* A variable used as the value of a secret is now sensitive: referencing it from any other field, such as a job parameter or a secret's `comment`, is an error because the value would be used in plain text. Mark other secret-holding variables with `sensitive: true` to get the same protection. ([#7288](https://github.com/databricks/cli/pull/7288)) diff --git a/acceptance/bundle/python/secrets-support/databricks.yml b/acceptance/bundle/python/secrets-support/databricks.yml index 4b4f4fe3924..4ac6d512329 100644 --- a/acceptance/bundle/python/secrets-support/databricks.yml +++ b/acceptance/bundle/python/secrets-support/databricks.yml @@ -6,6 +6,8 @@ sync: {paths: []} # don't need to copy files variables: secret_value: description: The value of the secret + python_secret_value: + description: The value of the secret added from Python python: resources: diff --git a/acceptance/bundle/python/secrets-support/output.txt b/acceptance/bundle/python/secrets-support/output.txt index ed0e0dc9fca..16df4152e64 100644 --- a/acceptance/bundle/python/secrets-support/output.txt +++ b/acceptance/bundle/python/secrets-support/output.txt @@ -1,5 +1,5 @@ ->>> uv run [UV_ARGS] -q [CLI] bundle validate --var secret_value=my-secret-value --output json +>>> uv run [UV_ARGS] -q [CLI] bundle validate --var secret_value=my-secret-value --var python_secret_value=my-python-secret-value --output json { "experimental": { "python": { @@ -28,5 +28,17 @@ "value": "[redacted]" } } + }, + "variables": { + "python_secret_value": { + "description": "The value of the secret added from Python", + "sensitive": true, + "value": "[redacted]" + }, + "secret_value": { + "description": "The value of the secret", + "sensitive": true, + "value": "[redacted]" + } } } diff --git a/acceptance/bundle/python/secrets-support/resources.py b/acceptance/bundle/python/secrets-support/resources.py index b46f8586a64..740381c153b 100644 --- a/acceptance/bundle/python/secrets-support/resources.py +++ b/acceptance/bundle/python/secrets-support/resources.py @@ -10,7 +10,7 @@ def load_resources() -> Resources: "catalog_name": "main", "schema_name": "default", "name": "my_secret_2", - "value": "${var.secret_value}", + "value": "${var.python_secret_value}", "expire_time": "2030-01-01T00:00:00Z", }, ) diff --git a/acceptance/bundle/python/secrets-support/script b/acceptance/bundle/python/secrets-support/script index 3864446e68b..f154b361c95 100644 --- a/acceptance/bundle/python/secrets-support/script +++ b/acceptance/bundle/python/secrets-support/script @@ -1,6 +1,7 @@ # secrets require value to be a variable reference, so supply it at validate time. -trace uv run $UV_ARGS -q $CLI bundle validate --var secret_value=my-secret-value --output json | \ - jq "pick(.experimental.python, .resources)" +# The variable used only by the secret added from Python must be redacted too. +trace uv run $UV_ARGS -q $CLI bundle validate --var secret_value=my-secret-value --var python_secret_value=my-python-secret-value --output json | \ + contains.py '!my-secret-value' '!my-python-secret-value' | jq "pick(.experimental.python, .resources, .variables)" rm -fr .databricks __pycache__ diff --git a/acceptance/bundle/refschema/output/root.txt b/acceptance/bundle/refschema/output/root.txt index aa8a83ea6cd..dcbd46b145c 100644 --- a/acceptance/bundle/refschema/output/root.txt +++ b/acceptance/bundle/refschema/output/root.txt @@ -148,6 +148,7 @@ environments.*.variables.*.lookup.pipeline string INPUT environments.*.variables.*.lookup.query string INPUT environments.*.variables.*.lookup.service_principal string INPUT environments.*.variables.*.lookup.warehouse string INPUT +environments.*.variables.*.sensitive bool INPUT environments.*.variables.*.type variable.VariableType INPUT environments.*.variables.*.value any INPUT environments.*.workspace *config.Workspace INPUT @@ -364,6 +365,7 @@ targets.*.variables.*.lookup.pipeline string INPUT targets.*.variables.*.lookup.query string INPUT targets.*.variables.*.lookup.service_principal string INPUT targets.*.variables.*.lookup.warehouse string INPUT +targets.*.variables.*.sensitive bool INPUT targets.*.variables.*.type variable.VariableType INPUT targets.*.variables.*.value any INPUT targets.*.workspace *config.Workspace INPUT @@ -447,6 +449,7 @@ variables.*.lookup.pipeline string INPUT variables.*.lookup.query string INPUT variables.*.lookup.service_principal string INPUT variables.*.lookup.warehouse string INPUT +variables.*.sensitive bool INPUT variables.*.type variable.VariableType INPUT variables.*.value any INPUT workspace config.Workspace INPUT diff --git a/acceptance/bundle/resources/secrets/basic/output.txt b/acceptance/bundle/resources/secrets/basic/output.txt index a01530e1b66..f501c7c28e2 100644 --- a/acceptance/bundle/resources/secrets/basic/output.txt +++ b/acceptance/bundle/resources/secrets/basic/output.txt @@ -16,6 +16,7 @@ { "secret_value": { "description": "Test secret value", + "sensitive": true, "value": "[redacted]" } } diff --git a/acceptance/bundle/variables/sensitive/invalid-resource-field/databricks.yml b/acceptance/bundle/variables/sensitive/invalid-resource-field/databricks.yml new file mode 100644 index 00000000000..bb83355cc4b --- /dev/null +++ b/acceptance/bundle/variables/sensitive/invalid-resource-field/databricks.yml @@ -0,0 +1,25 @@ +bundle: + name: sensitive-variables + +variables: + api_credential: + description: Sensitive because it is used as the value of a secret + +resources: + secrets: + example_secret: + name: example_secret + catalog_name: main + schema_name: default + value: ${var.api_credential} + comment: "Credential: ${var.api_credential}" + + jobs: + example_job: + name: example_job + tasks: + - task_key: main + notebook_task: + notebook_path: /Workspace/Shared/notebook + base_parameters: + token: ${var.api_credential} diff --git a/acceptance/bundle/variables/sensitive/invalid-variable/databricks.yml b/acceptance/bundle/variables/sensitive/invalid-variable/databricks.yml new file mode 100644 index 00000000000..7ec4f66c33c --- /dev/null +++ b/acceptance/bundle/variables/sensitive/invalid-variable/databricks.yml @@ -0,0 +1,9 @@ +bundle: + name: sensitive-variables + +variables: + api_credential: + sensitive: true + api_header: + description: Not marked as sensitive, so it must not reference a sensitive variable + default: "Bearer ${var.api_credential}" diff --git a/acceptance/bundle/variables/sensitive/out.test.toml b/acceptance/bundle/variables/sensitive/out.test.toml new file mode 100644 index 00000000000..a927a5fbc06 --- /dev/null +++ b/acceptance/bundle/variables/sensitive/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/variables/sensitive/output.txt b/acceptance/bundle/variables/sensitive/output.txt new file mode 100644 index 00000000000..f203db3e56e --- /dev/null +++ b/acceptance/bundle/variables/sensitive/output.txt @@ -0,0 +1,97 @@ + +=== A variable used as the value of a secret is sensitive and redacted in validate -o json + +>>> [CLI] bundle validate -o json +{ + "variables": { + "api_credential": { + "description": "Credential for the external API, passed via BUNDLE_VAR_api_credential. Sensitive because it is used as the value of a secret.", + "sensitive": true, + "value": "[redacted]" + }, + "api_header": { + "default": "[redacted]", + "description": "A sensitive variable may reference another sensitive variable", + "sensitive": true, + "value": "[redacted]" + }, + "target_catalog": { + "default": "example_catalog", + "value": "example_catalog" + }, + "target_schema": { + "default": "example_schema", + "value": "example_schema" + } + }, + "resources": { + "secrets": { + "example_secret": { + "catalog_name": "example_catalog", + "name": "example_secret", + "schema_name": "example_schema", + "value": "[redacted]" + } + } + } +} + +=== Sensitive variable used in non-sensitive resource fields + +>>> [CLI] bundle validate +Error: Sensitive variable "api_credential" is used in a non-sensitive field + at resources.jobs.example_job.tasks[0].notebook_task.base_parameters.token + in databricks.yml:25:22 + +The variable "api_credential" is sensitive, so its value must not appear in plain text. +A variable is sensitive when it is marked with "sensitive: true" or used as the value of a sensitive resource field (such as resources.secrets.*.value). It can only be used in such a field or in the definition of another sensitive variable. + +Error: Sensitive variable "api_credential" is used in a non-sensitive field + at resources.secrets.example_secret.comment + in databricks.yml:15:16 + +The variable "api_credential" is sensitive, so its value must not appear in plain text. +A variable is sensitive when it is marked with "sensitive: true" or used as the value of a sensitive resource field (such as resources.secrets.*.value). It can only be used in such a field or in the definition of another sensitive variable. + +Name: sensitive-variables +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/sensitive-variables/default + +Found 2 errors + +=== Sensitive variable used in the default of a non-sensitive variable + +>>> [CLI] bundle validate +Error: Sensitive variable "api_credential" is used in a non-sensitive field + at variables.api_header.default + in databricks.yml:9:14 + +The variable "api_credential" is sensitive, so its value must not appear in plain text. +A variable is sensitive when it is marked with "sensitive: true" or used as the value of a sensitive resource field (such as resources.secrets.*.value). It can only be used in such a field or in the definition of another sensitive variable. + +Name: sensitive-variables +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/sensitive-variables/default + +Found 1 error + +=== Sensitive variable passed as the value of a non-sensitive variable + +>>> [CLI] bundle validate --var target_schema=${var.api_credential} +Error: Sensitive variable "api_credential" is used in a non-sensitive field + at variables.target_schema.value + +The variable "api_credential" is sensitive, so its value must not appear in plain text. +A variable is sensitive when it is marked with "sensitive: true" or used as the value of a sensitive resource field (such as resources.secrets.*.value). It can only be used in such a field or in the definition of another sensitive variable. + +Name: sensitive-variables +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/sensitive-variables/default + +Found 1 error diff --git a/acceptance/bundle/variables/sensitive/redact/databricks.yml b/acceptance/bundle/variables/sensitive/redact/databricks.yml new file mode 100644 index 00000000000..1b222380d4c --- /dev/null +++ b/acceptance/bundle/variables/sensitive/redact/databricks.yml @@ -0,0 +1,22 @@ +bundle: + name: sensitive-variables + +variables: + target_catalog: + default: example_catalog + target_schema: + default: example_schema + api_credential: + description: Credential for the external API, passed via BUNDLE_VAR_api_credential. Sensitive because it is used as the value of a secret. + api_header: + description: A sensitive variable may reference another sensitive variable + sensitive: true + default: "Bearer ${var.api_credential}" + +resources: + secrets: + example_secret: + name: example_secret + catalog_name: ${var.target_catalog} + schema_name: ${var.target_schema} + value: ${var.api_credential} diff --git a/acceptance/bundle/variables/sensitive/script b/acceptance/bundle/variables/sensitive/script new file mode 100644 index 00000000000..04a225a8d00 --- /dev/null +++ b/acceptance/bundle/variables/sensitive/script @@ -0,0 +1,11 @@ +title "A variable used as the value of a secret is sensitive and redacted in validate -o json\n" +withdir redact trace $CLI bundle validate -o json | contains.py '!dummy-credential' | jq '{variables, resources}' + +title "Sensitive variable used in non-sensitive resource fields\n" +withdir invalid-resource-field musterr trace $CLI bundle validate + +title "Sensitive variable used in the default of a non-sensitive variable\n" +withdir invalid-variable musterr trace $CLI bundle validate + +title "Sensitive variable passed as the value of a non-sensitive variable\n" +withdir redact musterr trace $CLI bundle validate --var 'target_schema=${var.api_credential}' diff --git a/acceptance/bundle/variables/sensitive/test.toml b/acceptance/bundle/variables/sensitive/test.toml new file mode 100644 index 00000000000..7b2c2829d8d --- /dev/null +++ b/acceptance/bundle/variables/sensitive/test.toml @@ -0,0 +1,3 @@ +Env.BUNDLE_VAR_api_credential = "dummy-credential" + +Ignore = [".databricks"] diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index a6dbdfb8fd4..2397defd153 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/databricks/cli/bundle/config/mutator/resourcemutator" + "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/logdiag" @@ -342,6 +343,14 @@ func (m *pythonMutator) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagno return mutateDiags } + // Resources added or updated by Python were not loaded when the initialize phase + // checked sensitive variable references. Check them on the full config: normalization + // below only merges resources back, so a marker inferred inside it would be lost. + bundle.ApplyContext(ctx, b, validate.SensitiveVariableReferences()) + if logdiag.HasError(ctx) { + return mutateDiags + } + resourcemutator.NormalizeAndInitializeResources(ctx, b, result.AddedResources) if logdiag.HasError(ctx) { return mutateDiags diff --git a/bundle/config/validate/sensitive_variable_references.go b/bundle/config/validate/sensitive_variable_references.go new file mode 100644 index 00000000000..4ae8ae9c76d --- /dev/null +++ b/bundle/config/validate/sensitive_variable_references.go @@ -0,0 +1,166 @@ +package validate + +import ( + "context" + "fmt" + "reflect" + "slices" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/direct/dresources" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" + "github.com/databricks/cli/libs/structs/structpath" +) + +type sensitiveVariableReferences struct{} + +// SensitiveVariableReferences marks every variable referenced from a sensitive resource +// field (a field listed in the resource type's sensitive_fields, such as a secret's value) +// as sensitive, then validates that a sensitive variable is only referenced from places +// that are themselves sensitive: a sensitive resource field or another sensitive variable. +// Anywhere else the secret would end up in plain text in the deployed resource or in CLI +// output. +// +// It inspects unresolved references, so it must run before variable resolution. +func SensitiveVariableReferences() bundle.Mutator { + return &sensitiveVariableReferences{} +} + +func (m *sensitiveVariableReferences) Name() string { + return "validate:sensitive_variable_references" +} + +// variableReference is an unresolved reference to the variable name, found in value at path. +type variableReference struct { + path dyn.Path + name string + value dyn.Value +} + +func (m *sensitiveVariableReferences) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics { + refs := variableReferences(b.Config.Value()) + + err := inferSensitiveVariables(b, refs) + if err != nil { + return diag.FromErr(err) + } + + var diags diag.Diagnostics + for _, r := range refs { + if !isSensitiveVariable(b, r.name) || isSensitiveLocation(b, r.path) { + continue + } + diags = append(diags, diag.Diagnostic{ + Severity: diag.Error, + Summary: fmt.Sprintf("Sensitive variable %q is used in a non-sensitive field", r.name), + Detail: fmt.Sprintf(`The variable "%s" is sensitive, so its value must not appear in plain text. +A variable is sensitive when it is marked with "sensitive: true" or used as the value of a sensitive resource field (such as resources.secrets.*.value). It can only be used in such a field or in the definition of another sensitive variable.`, r.name), + Locations: r.value.Locations(), + Paths: dyn.ToStructPaths(r.path), + }) + } + return diags +} + +// variableReferences collects the unresolved variable references in the config. +func variableReferences(root dyn.Value) []variableReference { + var refs []variableReference + _ = dyn.WalkReadOnly(root, func(p dyn.Path, v dyn.Value) error { + // Target overrides are merged into the root before this runs; walking them + // too would report the same reference twice and flag targets not being deployed. + if len(p) == 1 && p[0].Key() == "targets" { + return dyn.ErrSkip + } + // SetVariables copies the default into the value; skip the copy so that a + // reference in the default is reported once. + if len(p) == 3 && p[0].Key() == "variables" && p[2].Key() == "value" { + d, err := dyn.GetByPath(root, dyn.NewPath(p[0], p[1], dyn.Key("default"))) + if err == nil && reflect.DeepEqual(d.AsAny(), v.AsAny()) { + return dyn.ErrSkip + } + } + ref, ok := dynvar.NewRef(v) + if !ok { + return nil + } + for _, r := range ref.References() { + name, ok := referencedVariable(r) + if !ok { + continue + } + // The walk reuses the path's backing array for sibling nodes. + refs = append(refs, variableReference{path: slices.Clone(p), name: name, value: v}) + } + return nil + }) + return refs +} + +// inferSensitiveVariables marks every variable referenced from a sensitive resource +// field as sensitive, so the marker does not have to be repeated on such a variable. +func inferSensitiveVariables(b *bundle.Bundle, refs []variableReference) error { + return b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { + for _, r := range refs { + // A reference to an undeclared variable is reported by variable resolution. + v, ok := b.Config.Variables[r.name] + if !ok || v.Sensitive || !isSensitiveResourceField(r.path) { + continue + } + var err error + root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("variables"), dyn.Key(r.name), dyn.Key("sensitive")), dyn.V(true)) + if err != nil { + return dyn.InvalidValue, err + } + } + return root, nil + }) +} + +func isSensitiveVariable(b *bundle.Bundle, name string) bool { + v, ok := b.Config.Variables[name] + return ok && v.Sensitive +} + +// referencedVariable returns the variable a reference like "var.foo" or +// "variables.foo.value" points to. +func referencedVariable(ref string) (string, bool) { + p, err := dyn.NewPathFromString(ref) + if err != nil || len(p) < 2 { + return "", false + } + switch p[0].Key() { + case "var", "variables": + return p[1].Key(), true + default: + return "", false + } +} + +// isSensitiveLocation reports whether a value at path p may hold a secret: it is part +// of the definition of a sensitive variable or a sensitive resource field. +func isSensitiveLocation(b *bundle.Bundle, p dyn.Path) bool { + if len(p) >= 2 && p[0].Key() == "variables" { + return isSensitiveVariable(b, p[1].Key()) + } + return isSensitiveResourceField(p) +} + +// isSensitiveResourceField reports whether p is a field under resources.. +// that the resource type declares as sensitive. +func isSensitiveResourceField(p dyn.Path) bool { + if len(p) < 4 || p[0].Key() != "resources" { + return false + } + field, err := structpath.ParsePath(p[3:].String()) + if err != nil { + return false + } + for _, rule := range dresources.GetResourceConfig(p[1].Key()).SensitiveFields { + if field.HasPatternPrefix(rule.Field) { + return true + } + } + return false +} diff --git a/bundle/config/validate/sensitive_variable_references_test.go b/bundle/config/validate/sensitive_variable_references_test.go new file mode 100644 index 00000000000..73f51c2fc82 --- /dev/null +++ b/bundle/config/validate/sensitive_variable_references_test.go @@ -0,0 +1,142 @@ +package validate + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func sensitiveVariablesBundle() *bundle.Bundle { + return &bundle.Bundle{ + Config: config.Root{ + Variables: map[string]*variable.Variable{ + "secret": {Sensitive: true}, + "other_secret": {Sensitive: true, Default: "placeholder"}, + "plain": {Default: "placeholder"}, + }, + Resources: config.Resources{ + Secrets: map[string]*resources.Secret{ + "s": {Secret: catalog.Secret{Name: "s", Value: "placeholder", Comment: "placeholder"}}, + }, + Jobs: map[string]*resources.Job{ + "j": {JobSettings: jobs.JobSettings{ + Name: "placeholder", + Tasks: []jobs.Task{{ + TaskKey: "main", + NotebookTask: &jobs.NotebookTask{ + NotebookPath: "/notebook", + BaseParameters: map[string]string{"token": "placeholder"}, + }, + }}, + }}, + }, + }, + }, + } +} + +func TestSensitiveVariableReferences(t *testing.T) { + tests := []struct { + name string + path string + value string + // Path of the expected diagnostic; empty when the reference is allowed. + wantPath string + }{ + {"secret value", "resources.secrets.s.value", "${var.secret}", ""}, + {"sensitive variable default", "variables.other_secret.default", "Bearer ${var.secret}", ""}, + {"long form reference in sensitive variable", "variables.other_secret.default", "${variables.secret.value}", ""}, + {"non-sensitive variable referenced in job", "resources.jobs.j.name", "${var.plain}", ""}, + {"non-sensitive variable default", "variables.plain.default", "${var.secret}", "variables.plain.default"}, + {"long form reference in non-sensitive variable", "variables.plain.default", "${variables.secret.value}", "variables.plain.default"}, + {"secret comment", "resources.secrets.s.comment", "${var.secret}", "resources.secrets.s.comment"}, + {"job name", "resources.jobs.j.name", "job ${var.secret}", "resources.jobs.j.name"}, + {"nested job field", "resources.jobs.j.tasks[0].notebook_task.base_parameters.token", "${var.secret}", "resources.jobs.j.tasks[0].notebook_task.base_parameters.token"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + b := sensitiveVariablesBundle() + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, tc.path, dyn.V(tc.value)) + }) + + diags := SensitiveVariableReferences().Apply(t.Context(), b) + if tc.wantPath == "" { + assert.Empty(t, diags) + return + } + require.Len(t, diags, 1) + assert.Equal(t, diag.Error, diags[0].Severity) + assert.Equal(t, `Sensitive variable "secret" is used in a non-sensitive field`, diags[0].Summary) + require.Len(t, diags[0].Paths, 1) + assert.Equal(t, tc.wantPath, diags[0].Paths[0].String()) + }) + } +} + +func TestSensitiveVariableReferencesSkipsTargets(t *testing.T) { + b := sensitiveVariablesBundle() + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "targets", dyn.V(map[string]dyn.Value{ + "dev": dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "jobs": dyn.V(map[string]dyn.Value{ + "j": dyn.V(map[string]dyn.Value{"name": dyn.V("${var.secret}")}), + }), + }), + }), + })) + }) + + assert.Empty(t, SensitiveVariableReferences().Apply(t.Context(), b)) +} + +func TestSensitiveVariableReferencesInfersFromSensitiveResourceField(t *testing.T) { + b := sensitiveVariablesBundle() + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.secrets.s.value", dyn.V("${var.plain}")) + }) + + assert.Empty(t, SensitiveVariableReferences().Apply(t.Context(), b)) + assert.True(t, b.Config.Variables["plain"].Sensitive) + v, err := dyn.GetByPath(b.Config.Value(), dyn.MustPathFromString("variables.plain.sensitive")) + require.NoError(t, err) + assert.Equal(t, true, v.AsAny()) +} + +func TestSensitiveVariableReferencesInferredVariableInNonSensitiveField(t *testing.T) { + b := sensitiveVariablesBundle() + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + v, err := dyn.Set(v, "resources.secrets.s.value", dyn.V("${var.plain}")) + if err != nil { + return v, err + } + return dyn.Set(v, "resources.jobs.j.name", dyn.V("${var.plain}")) + }) + + diags := SensitiveVariableReferences().Apply(t.Context(), b) + require.Len(t, diags, 1) + assert.Equal(t, `Sensitive variable "plain" is used in a non-sensitive field`, diags[0].Summary) + require.Len(t, diags[0].Paths, 1) + assert.Equal(t, "resources.jobs.j.name", diags[0].Paths[0].String()) +} + +func TestSensitiveVariableReferencesIgnoresUndeclaredVariable(t *testing.T) { + b := sensitiveVariablesBundle() + bundletest.Mutate(t, b, func(v dyn.Value) (dyn.Value, error) { + return dyn.Set(v, "resources.secrets.s.value", dyn.V("${var.undeclared}")) + }) + + assert.Empty(t, SensitiveVariableReferences().Apply(t.Context(), b)) +} diff --git a/bundle/config/variable/variable.go b/bundle/config/variable/variable.go index 5f8ea6138c4..b6fbf56d260 100644 --- a/bundle/config/variable/variable.go +++ b/bundle/config/variable/variable.go @@ -39,6 +39,13 @@ type Variable struct { // Documentation for this input variable Description string `json:"description,omitempty"` + // Marks the variable as holding a secret. Set automatically for a variable used as the + // value of a sensitive resource field (e.g. the value of a UC secret). Its value is + // redacted in the output of `bundle validate -o json` and `bundle summary -o json`, and + // it may only be referenced from sensitive resource fields or from other sensitive + // variables, so that the secret never ends up in plain text. + Sensitive bool `json:"sensitive,omitempty"` + // This field stores the resolved value for the variable. The variable are // resolved in the following priority order (from highest to lowest) // diff --git a/bundle/direct/dresources/redact_config.go b/bundle/direct/dresources/redact_config.go index dbd6ccda83d..8812caef453 100644 --- a/bundle/direct/dresources/redact_config.go +++ b/bundle/direct/dresources/redact_config.go @@ -11,14 +11,12 @@ import ( // RedactSensitiveConfigValues walks the bundle config and replaces the // value of every field declared as sensitive_fields for its resource type with -// "[redacted]". It also redacts the entire variables section when any sensitive -// resource fields are present, since variables are the standard mechanism for -// supplying secret values and their resolved values must not appear in the -// output of `bundle validate -o json`. +// "[redacted]". It also redacts every sensitive variable, since variables are the +// standard mechanism for supplying secret values and their resolved values must +// not appear in the output of `bundle validate -o json`. func RedactSensitiveConfigValues(root *config.Root) (*config.Root, error) { fields := getSensitiveFields() - var hasSensitiveFields bool for resourceType, fieldRules := range fields { resources, err := structaccess.GetByString(root, "resources."+resourceType) if err != nil { @@ -29,7 +27,6 @@ func RedactSensitiveConfigValues(root *config.Root) (*config.Root, error) { // The first segment of the path is the resource key, so we need to skip it and check the rest of the path rest := path.SkipPrefix(1) if rest.HasPatternPrefix(fieldRule.Field) { - hasSensitiveFields = true _ = structaccess.SetByString(root, "resources."+resourceType+path.String(), sensitiveRedactedMarker) } } @@ -39,25 +36,30 @@ func RedactSensitiveConfigValues(root *config.Root) (*config.Root, error) { } } - // Redact all variable values when any sensitive resource fields are present. - // Variables are the standard mechanism for supplying secret values and their - // resolved values must not appear in plaintext in the validate output. - if hasSensitiveFields { - for _, variable := range root.Variables { - if variable == nil { - continue - } - if _, ok := variable.Value.(string); ok { - variable.Value = sensitiveRedactedMarker - } - } - } + redactSensitiveVariables(root) return root, nil } const sensitiveRedactedMarker = "[redacted]" +// redactSensitiveVariables redacts the variables marked sensitive. The default is +// redacted too: it may reference another sensitive variable and has been resolved +// to the secret by the time the config is rendered. +func redactSensitiveVariables(root *config.Root) { + for _, variable := range root.Variables { + if variable == nil || !variable.Sensitive { + continue + } + if variable.Default != nil { + variable.Default = sensitiveRedactedMarker + } + if variable.Value != nil { + variable.Value = sensitiveRedactedMarker + } + } +} + // getSensitiveFields returns the map of resource type to list of sensitive fields. func getSensitiveFields() map[string][]FieldRule { cfg := MustLoadConfig() diff --git a/bundle/direct/dresources/redact_config_test.go b/bundle/direct/dresources/redact_config_test.go new file mode 100644 index 00000000000..c4bbabaf5dd --- /dev/null +++ b/bundle/direct/dresources/redact_config_test.go @@ -0,0 +1,67 @@ +package dresources + +import ( + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedactSensitiveConfigValues(t *testing.T) { + secrets := map[string]*resources.Secret{ + "s": {Secret: catalog.Secret{Name: "s", Value: "top-secret"}}, + } + + tests := []struct { + name string + root config.Root + expected map[string]*variable.Variable + }{ + { + name: "sensitive variables are redacted, default included, others are not", + root: config.Root{ + Variables: map[string]*variable.Variable{ + "catalog": {Default: "main", Value: "main"}, + "count": {Default: 1, Value: 1}, + "secret": {Sensitive: true, Value: "top-secret"}, + "header": {Sensitive: true, Default: "Bearer top-secret", Value: "Bearer top-secret"}, + }, + Resources: config.Resources{Secrets: secrets}, + }, + expected: map[string]*variable.Variable{ + "catalog": {Default: "main", Value: "main"}, + "count": {Default: 1, Value: 1}, + "secret": {Sensitive: true, Value: "[redacted]"}, + "header": {Sensitive: true, Default: "[redacted]", Value: "[redacted]"}, + }, + }, + { + name: "sensitive variables are redacted without sensitive resource fields", + root: config.Root{ + Variables: map[string]*variable.Variable{ + "catalog": {Default: "main", Value: "main"}, + "secret": {Sensitive: true, Value: "top-secret"}, + }, + }, + expected: map[string]*variable.Variable{ + "catalog": {Default: "main", Value: "main"}, + "secret": {Sensitive: true, Value: "[redacted]"}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + root, err := RedactSensitiveConfigValues(&tc.root) + require.NoError(t, err) + assert.Equal(t, tc.expected, root.Variables) + for _, s := range root.Resources.Secrets { + assert.Equal(t, "[redacted]", s.Value) + } + }) + } +} diff --git a/bundle/internal/schema/annotations.yml b/bundle/internal/schema/annotations.yml index d4885599805..bfda83a4d92 100644 --- a/bundle/internal/schema/annotations.yml +++ b/bundle/internal/schema/annotations.yml @@ -2305,6 +2305,9 @@ targets: "lookup": "description": |- The name of the alert, cluster_policy, cluster, dashboard, instance_pool, job, metastore, pipeline, query, service_principal, or warehouse object for which to retrieve an ID. + "sensitive": + "description": |- + Whether the variable holds a secret. A variable used as the value of a secret is sensitive by default. The value of a sensitive variable is redacted in the output of `bundle validate -o json`, and the variable can only be used as the value of a sensitive resource field, such as the value of a secret, or in the definition of another sensitive variable. "type": "description": |- The type of the variable. @@ -2368,6 +2371,9 @@ variables: "warehouse": "description": |- The name of the warehouse for which to retrieve an ID. + "sensitive": + "description": |- + Whether the variable holds a secret. A variable used as the value of a secret is sensitive by default. The value of a sensitive variable is redacted in the output of `bundle validate -o json`, and the variable can only be used as the value of a sensitive resource field, such as the value of a secret, or in the definition of another sensitive variable. "type": "description": |- The type of the variable. diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index ef74a786ca1..c855935598f 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -116,6 +116,13 @@ func Initialize(ctx context.Context, b *bundle.Bundle) { // Resolves and sets values for bundle variables in the following order: from environment variables, from variable files and then defaults mutator.SetVariables(), + // Reads (dynamic): * (strings) (searches for references to variables) + // Updates (dynamic): variables.*.sensitive (marks variables used as the value of a sensitive resource field) + // Rejects references to sensitive variables outside sensitive resource fields and other sensitive variables. + // Runs after SetVariables so values passed via --var, environment or file are checked too, and before + // any resolution while references are still visible. + validate.SensitiveVariableReferences(), + // Reads (dynamic): variables.*.lookup (checks for variable references in lookup fields) // Updates (dynamic): variables.*.lookup (resolves variable references in lookup fields) // Prevents circular references between lookup variables diff --git a/bundle/schema/jsonschema.json b/bundle/schema/jsonschema.json index 959273d2586..7d12e0d2e5f 100644 --- a/bundle/schema/jsonschema.json +++ b/bundle/schema/jsonschema.json @@ -3729,6 +3729,10 @@ "description": "The name of the alert, cluster_policy, cluster, dashboard, instance_pool, job, metastore, pipeline, query, service_principal, or warehouse object for which to retrieve an ID.", "$ref": "#/$defs/github.com/databricks/cli/bundle/config/variable.Lookup" }, + "sensitive": { + "description": "Whether the variable holds a secret. A variable used as the value of a secret is sensitive by default. The value of a sensitive variable is redacted in the output of `bundle validate -o json`, and the variable can only be used as the value of a sensitive resource field, such as the value of a secret, or in the definition of another sensitive variable.", + "$ref": "#/$defs/bool" + }, "type": { "description": "The type of the variable.", "$ref": "#/$defs/github.com/databricks/cli/bundle/config/variable.VariableType" @@ -3756,6 +3760,10 @@ "$ref": "#/$defs/github.com/databricks/cli/bundle/config/variable.Lookup", "markdownDescription": "The name of the `alert`, `cluster_policy`, `cluster`, `dashboard`, `instance_pool`, `job`, `metastore`, `pipeline`, `query`, `service_principal`, or `warehouse` object for which to retrieve an ID." }, + "sensitive": { + "description": "Whether the variable holds a secret. A variable used as the value of a secret is sensitive by default. The value of a sensitive variable is redacted in the output of `bundle validate -o json`, and the variable can only be used as the value of a sensitive resource field, such as the value of a secret, or in the definition of another sensitive variable.", + "$ref": "#/$defs/bool" + }, "type": { "description": "The type of the variable.", "$ref": "#/$defs/github.com/databricks/cli/bundle/config/variable.VariableType"