Embedded secrets validation (#24624)

#24549 Co-authored-by: Victor Lyuboslavsky <victor.lyuboslavsky@gmail.com>
fleetdm · Dec 17, 2024 · c9bdae8 · c9bdae8
1 parent addaaa3
commit c9bdae8
Show file tree

Hide file tree

Showing 20 changed files with 448 additions and 14 deletions.
diff --git a/changes/24549-validate-script-profle-secrets b/changes/24549-validate-script-profle-secrets
@@ -0,0 +1 @@
+- Validate fleet secrets embedded into scripts and profiles on ingestion
diff --git a/cmd/fleetctl/testing_utils.go b/cmd/fleetctl/testing_utils.go
@@ -152,6 +152,9 @@ func runServerWithMockedDS(t *testing.T, opts ...*service.TestServerOpts) (*http
 	ds.ApplyYaraRulesFunc = func(context.Context, []fleet.YaraRule) error {
 		return nil
 	}
+	ds.ValidateEmbeddedSecretsFunc = func(ctx context.Context, documents []string) error {
+		return nil
+	}
 
 	var cachedDS fleet.Datastore
 	if len(opts) > 0 && opts[0].NoCacheDatastore {

diff --git a/ee/server/service/setup_experience.go b/ee/server/service/setup_experience.go
@@ -76,6 +76,10 @@ func (svc *Service) SetSetupExperienceScript(ctx context.Context, teamID *uint,
 		ScriptContents: string(b),
 	}
 
+	if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{script.ScriptContents}); err != nil {
+		return fleet.NewInvalidArgumentError("script", err.Error())
+	}
+
 	// setup experience is only supported for macOS currently so we need to override the file
 	// extension check in the general script validation
 	if filepath.Ext(script.Name) != ".sh" {

diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go
@@ -186,10 +186,10 @@ func expandEnv(s string, failOnSecret bool) (string, error) {
 		case strings.HasPrefix(env, fleet.ServerVarPrefix):
 			// Don't expand fleet vars -- they will be expanded on the server
 			return "", false
-		case strings.HasPrefix(env, fleet.FLEET_SECRET_PREFIX):
+		case strings.HasPrefix(env, fleet.ServerSecretPrefix):
 			if failOnSecret {
 				err = multierror.Append(err, fmt.Errorf("environment variables with %q prefix are only allowed in profiles and scripts: %q",
-					fleet.FLEET_SECRET_PREFIX, env))
+					fleet.ServerSecretPrefix, env))
 			}
 			return "", false
 		}
@@ -231,7 +231,7 @@ func LookupEnvSecrets(s string, secretsMap map[string]string) error {
 	}
 	var err *multierror.Error
 	_ = fleet.MaybeExpand(s, func(env string) (string, bool) {
-		if strings.HasPrefix(env, fleet.FLEET_SECRET_PREFIX) {
+		if strings.HasPrefix(env, fleet.ServerSecretPrefix) {
 			// lookup the secret and save it, but don't replace
 			v, ok := os.LookupEnv(env)
 			if !ok {

diff --git a/server/datastore/mysql/secret_variables.go b/server/datastore/mysql/secret_variables.go
@@ -68,3 +68,80 @@ func (ds *Datastore) GetSecretVariables(ctx context.Context, names []string) ([]
 
 	return secretVariables, nil
 }
+
+func (ds *Datastore) ExpandEmbeddedSecrets(ctx context.Context, document string) (string, error) {
+	embeddedSecrets := fleet.ContainsPrefixVars(document, fleet.ServerSecretPrefix)
+
+	secrets, err := ds.GetSecretVariables(ctx, embeddedSecrets)
+	if err != nil {
+		return "", ctxerr.Wrap(ctx, err, "expanding embedded secrets")
+	}
+
+	secretMap := make(map[string]string, len(secrets))
+
+	for _, secret := range secrets {
+		secretMap[secret.Name] = secret.Value
+	}
+
+	missingSecrets := []string{}
+
+	for _, wantSecret := range embeddedSecrets {
+		if _, ok := secretMap[wantSecret]; !ok {
+			missingSecrets = append(missingSecrets, wantSecret)
+		}
+	}
+
+	if len(missingSecrets) > 0 {
+		return "", fleet.MissingSecretsError{MissingSecrets: missingSecrets}
+	}
+
+	expanded := fleet.MaybeExpand(document, func(s string) (string, bool) {
+		if !strings.HasPrefix(s, fleet.ServerSecretPrefix) {
+			return "", false
+		}
+		val, ok := secretMap[strings.TrimPrefix(s, fleet.ServerSecretPrefix)]
+		return val, ok
+	})
+
+	return expanded, nil
+}
+
+func (ds *Datastore) ValidateEmbeddedSecrets(ctx context.Context, documents []string) error {
+	wantSecrets := make(map[string]struct{})
+	haveSecrets := make(map[string]struct{})
+
+	for _, document := range documents {
+		vars := fleet.ContainsPrefixVars(document, fleet.ServerSecretPrefix)
+		for _, v := range vars {
+			wantSecrets[v] = struct{}{}
+		}
+	}
+
+	wantSecretsList := make([]string, 0, len(wantSecrets))
+	for wantSecret := range wantSecrets {
+		wantSecretsList = append(wantSecretsList, wantSecret)
+	}
+
+	dbSecrets, err := ds.GetSecretVariables(ctx, wantSecretsList)
+	if err != nil {
+		return ctxerr.Wrap(ctx, err, "validating document embedded secrets")
+	}
+
+	for _, dbSecret := range dbSecrets {
+		haveSecrets[dbSecret.Name] = struct{}{}
+	}
+
+	missingSecrets := []string{}
+
+	for wantSecret := range wantSecrets {
+		if _, ok := haveSecrets[wantSecret]; !ok {
+			missingSecrets = append(missingSecrets, wantSecret)
+		}
+	}
+
+	if len(missingSecrets) > 0 {
+		return &fleet.MissingSecretsError{MissingSecrets: missingSecrets}
+	}
+
+	return nil
+}
diff --git a/server/datastore/mysql/secret_variables_test.go b/server/datastore/mysql/secret_variables_test.go
@@ -17,6 +17,8 @@ func TestSecretVariables(t *testing.T) {
 		fn   func(t *testing.T, ds *Datastore)
 	}{
 		{"UpsertSecretVariables", testUpsertSecretVariables},
+		{"ValidateEmbeddedSecrets", testValidateEmbeddedSecrets},
+		{"ExpandEmbeddedSecrets", testExpandEmbeddedSecrets},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
@@ -70,3 +72,97 @@ func testUpsertSecretVariables(t *testing.T, ds *Datastore) {
 	assert.Equal(t, secretMap[results[0].Name], results[0].Value)
 
 }
+
+func testValidateEmbeddedSecrets(t *testing.T, ds *Datastore) {
+	noSecrets := `
+This document contains to fleet secrets.
+$FLEET_VAR_XX $HOSTNAME ${SOMETHING_ELSE}
+`
+
+	validSecret := `
+This document contains a valid ${FLEET_SECRET_VALID}.
+Another $FLEET_SECRET_ALSO_VALID.
+`
+
+	invalidSecret := `
+This document contains a secret not stored in the database.
+Hello doc${FLEET_SECRET_INVALID}. $FLEET_SECRET_ALSO_INVALID
+`
+	ctx := context.Background()
+	secretMap := map[string]string{
+		"VALID":      "testValue1",
+		"ALSO_VALID": "testValue2",
+	}
+
+	secrets := make([]fleet.SecretVariable, 0, len(secretMap))
+	for name, value := range secretMap {
+		secrets = append(secrets, fleet.SecretVariable{Name: name, Value: value})
+	}
+
+	err := ds.UpsertSecretVariables(ctx, secrets)
+	require.NoError(t, err)
+
+	err = ds.ValidateEmbeddedSecrets(ctx, []string{noSecrets})
+	require.NoError(t, err)
+
+	err = ds.ValidateEmbeddedSecrets(ctx, []string{validSecret})
+	require.NoError(t, err)
+
+	err = ds.ValidateEmbeddedSecrets(ctx, []string{noSecrets, validSecret})
+	require.NoError(t, err)
+
+	err = ds.ValidateEmbeddedSecrets(ctx, []string{invalidSecret})
+	require.ErrorContains(t, err, "$FLEET_SECRET_INVALID")
+	require.ErrorContains(t, err, "$FLEET_SECRET_ALSO_INVALID")
+
+	err = ds.ValidateEmbeddedSecrets(ctx, []string{noSecrets, validSecret, invalidSecret})
+	require.ErrorContains(t, err, "$FLEET_SECRET_INVALID")
+	require.ErrorContains(t, err, "$FLEET_SECRET_ALSO_INVALID")
+}
+
+func testExpandEmbeddedSecrets(t *testing.T, ds *Datastore) {
+	noSecrets := `
+This document contains to fleet secrets.
+$FLEET_VAR_XX $HOSTNAME ${SOMETHING_ELSE}
+`
+
+	validSecret := `
+This document contains a valid ${FLEET_SECRET_VALID}.
+Another $FLEET_SECRET_ALSO_VALID.
+`
+	validSecretExpanded := `
+This document contains a valid testValue1.
+Another testValue2.
+`
+
+	invalidSecret := `
+This document contains a secret not stored in the database.
+Hello doc${FLEET_SECRET_INVALID}. $FLEET_SECRET_ALSO_INVALID
+`
+
+	ctx := context.Background()
+	secretMap := map[string]string{
+		"VALID":      "testValue1",
+		"ALSO_VALID": "testValue2",
+	}
+
+	secrets := make([]fleet.SecretVariable, 0, len(secretMap))
+	for name, value := range secretMap {
+		secrets = append(secrets, fleet.SecretVariable{Name: name, Value: value})
+	}
+
+	err := ds.UpsertSecretVariables(ctx, secrets)
+	require.NoError(t, err)
+
+	expanded, err := ds.ExpandEmbeddedSecrets(ctx, noSecrets)
+	require.NoError(t, err)
+	require.Equal(t, noSecrets, expanded)
+
+	expanded, err = ds.ExpandEmbeddedSecrets(ctx, validSecret)
+	require.NoError(t, err)
+	require.Equal(t, validSecretExpanded, expanded)
+
+	_, err = ds.ExpandEmbeddedSecrets(ctx, invalidSecret)
+	require.ErrorContains(t, err, "$FLEET_SECRET_INVALID")
+	require.ErrorContains(t, err, "$FLEET_SECRET_ALSO_INVALID")
+}
diff --git a/server/fleet/datastore.go b/server/fleet/datastore.go
@@ -1883,6 +1883,14 @@ type Datastore interface {
 
 	// GetSecretVariables retrieves secret variables from the database.
 	GetSecretVariables(ctx context.Context, names []string) ([]SecretVariable, error)
+
+	// ValidateEmbeddedSecrets parses fleet secrets from a list of
+	// documents and checks that they exist in the database.
+	ValidateEmbeddedSecrets(ctx context.Context, documents []string) error
+
+	// ExpandEmbeddedSecrets expands the fleet secrets in a
+	// document using the secrets stored in the datastore.
+	ExpandEmbeddedSecrets(ctx context.Context, document string) (string, error)
 }
 
 // MDMAppleStore wraps nanomdm's storage and adds methods to deal with

diff --git a/server/fleet/fleet_vars_test.go b/server/fleet/fleet_vars_test.go
@@ -16,7 +16,7 @@ echo words${FLEET_SECRET_BAR}words
 $FLEET_SECRET_BAZ
 ${FLEET_SECRET_QUX}
 `
-	secrets := ContainsPrefixVars(script, FLEET_SECRET_PREFIX)
+	secrets := ContainsPrefixVars(script, ServerSecretPrefix)
 	require.Contains(t, secrets, "FOO")
 	require.Contains(t, secrets, "BAR")
 	require.Contains(t, secrets, "BAZ")
@@ -39,8 +39,8 @@ We want to remember BREAD and alsoSHORTCAKEare important.
 	}
 
 	mapper := func(s string) (string, bool) {
-		if strings.HasPrefix(s, FLEET_SECRET_PREFIX) {
-			return mapping[strings.TrimPrefix(s, FLEET_SECRET_PREFIX)], true
+		if strings.HasPrefix(s, ServerSecretPrefix) {
+			return mapping[strings.TrimPrefix(s, ServerSecretPrefix)], true
 		}
 		return "", false
 	}

diff --git a/server/fleet/secrets.go b/server/fleet/secrets.go
@@ -1,3 +1,24 @@
 package fleet
 
-const FLEET_SECRET_PREFIX = "FLEET_SECRET_"
+import (
+	"fmt"
+	"strings"
+)
+
+const ServerSecretPrefix = "FLEET_SECRET_"
+
+type MissingSecretsError struct {
+	MissingSecrets []string
+}
+
+func (e MissingSecretsError) Error() string {
+	secretVars := make([]string, 0, len(e.MissingSecrets))
+	for _, secret := range e.MissingSecrets {
+		secretVars = append(secretVars, fmt.Sprintf("\"$%s%s\"", ServerSecretPrefix, secret))
+	}
+	plural := ""
+	if len(secretVars) > 1 {
+		plural = "s"
+	}
+	return fmt.Sprintf("Couldn't add. Variable%s %s missing", plural, strings.Join(secretVars, ", "))
+}
diff --git a/server/mock/datastore_mock.go b/server/mock/datastore_mock.go
@@ -1177,6 +1177,10 @@ type UpsertSecretVariablesFunc func(ctx context.Context, secretVariables []fleet
 
 type GetSecretVariablesFunc func(ctx context.Context, names []string) ([]fleet.SecretVariable, error)
 
+type ValidateEmbeddedSecretsFunc func(ctx context.Context, documents []string) error
+
+type ExpandEmbeddedSecretsFunc func(ctx context.Context, document string) (string, error)
+
 type DataStore struct {
 	HealthCheckFunc        HealthCheckFunc
 	HealthCheckFuncInvoked bool
@@ -2912,6 +2916,12 @@ type DataStore struct {
 	GetSecretVariablesFunc        GetSecretVariablesFunc
 	GetSecretVariablesFuncInvoked bool
 
+	ValidateEmbeddedSecretsFunc        ValidateEmbeddedSecretsFunc
+	ValidateEmbeddedSecretsFuncInvoked bool
+
+	ExpandEmbeddedSecretsFunc        ExpandEmbeddedSecretsFunc
+	ExpandEmbeddedSecretsFuncInvoked bool
+
 	mu sync.Mutex
 }
 
@@ -6960,3 +6970,17 @@ func (s *DataStore) GetSecretVariables(ctx context.Context, names []string) ([]f
 	s.mu.Unlock()
 	return s.GetSecretVariablesFunc(ctx, names)
 }
+
+func (s *DataStore) ValidateEmbeddedSecrets(ctx context.Context, documents []string) error {
+	s.mu.Lock()
+	s.ValidateEmbeddedSecretsFuncInvoked = true
+	s.mu.Unlock()
+	return s.ValidateEmbeddedSecretsFunc(ctx, documents)
+}
+
+func (s *DataStore) ExpandEmbeddedSecrets(ctx context.Context, document string) (string, error) {
+	s.mu.Lock()
+	s.ExpandEmbeddedSecretsFuncInvoked = true
+	s.mu.Unlock()
+	return s.ExpandEmbeddedSecretsFunc(ctx, document)
+}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		- Validate fleet secrets embedded into scripts and profiles on ingestion