diff --git a/changes/34229-gitops-label-validation b/changes/34229-gitops-label-validation new file mode 100644 index 0000000000..5ca88ac922 --- /dev/null +++ b/changes/34229-gitops-label-validation @@ -0,0 +1 @@ +* Fixed `fleetctl gitops` silently accepting labels with invalid parameter combinations (e.g. manual labels with query/criteria/platform). diff --git a/cmd/fleetctl/fleetctl/generate_gitops.go b/cmd/fleetctl/fleetctl/generate_gitops.go index 384bfdde36..80cf4641b2 100644 --- a/cmd/fleetctl/fleetctl/generate_gitops.go +++ b/cmd/fleetctl/fleetctl/generate_gitops.go @@ -2251,7 +2251,7 @@ func (cmd *GenerateGitopsCommand) generateLabels(team *fleet.Team) ([]map[string jsonFieldName(t, "Description"): label.Description, jsonFieldName(t, "LabelMembershipType"): label.LabelMembershipType, } - if label.Platform != "" { + if label.LabelMembershipType == fleet.LabelMembershipTypeDynamic && label.Platform != "" { labelSpec[jsonFieldName(t, "Platform")] = label.Platform } switch label.LabelMembershipType { diff --git a/cmd/fleetctl/fleetctl/get.go b/cmd/fleetctl/fleetctl/get.go index 8b08f98dd5..7a59ad50ff 100644 --- a/cmd/fleetctl/fleetctl/get.go +++ b/cmd/fleetctl/fleetctl/get.go @@ -92,6 +92,24 @@ func printYaml(spec interface{}, writer io.Writer) error { return nil } +// stripMismatchedLabelFields clears fields that are not meaningful for the +// label's membership type so that exported YAML can be re-applied cleanly. +func stripMismatchedLabelFields(label *fleet.LabelSpec) { + switch label.LabelMembershipType { + case fleet.LabelMembershipTypeManual: + label.Query = "" + label.Platform = "" + label.HostVitalsCriteria = nil + case fleet.LabelMembershipTypeDynamic: + label.Hosts = nil + label.HostVitalsCriteria = nil + case fleet.LabelMembershipTypeHostVitals: + label.Query = "" + label.Platform = "" + label.Hosts = nil + } +} + func printLabel(c *cli.Context, label *fleet.LabelSpec) error { spec := specGeneric{ Kind: fleet.LabelKind, @@ -725,6 +743,7 @@ func getLabelsCommand() *cli.Command { if c.Bool(yamlFlagName) || c.Bool(jsonFlagName) { for _, label := range labels { + stripMismatchedLabelFields(label) printLabel(c, label) //nolint:errcheck } return nil @@ -761,6 +780,7 @@ func getLabelsCommand() *cli.Command { return err } + stripMismatchedLabelFields(label) printLabel(c, label) //nolint:errcheck return nil }, diff --git a/pkg/spec/gitops.go b/pkg/spec/gitops.go index e926b12b0f..e9e180ed1e 100644 --- a/pkg/spec/gitops.go +++ b/pkg/spec/gitops.go @@ -1405,18 +1405,17 @@ func parseLabels(top map[string]json.RawMessage, result *GitOps, baseDir string, multiError = multierror.Append(multiError, errors.New("name is required for each label")) } - if l.LabelMembershipType != fleet.LabelMembershipTypeManual && l.Query == "" && l.HostVitalsCriteria == nil { - multiError = multierror.Append(multiError, errors.New("a SQL query or host vitals criteria is required for each non-manual label")) + // Validate mutually exclusive field combinations per label membership type + if err := fleet.ValidateLabelMembershipFields(l); err != nil { + for _, inv := range err.Invalid() { + multiError = multierror.Append(multiError, fmt.Errorf("%s", inv["reason"])) + } } // Don't use non-ASCII if !isASCII(l.Name) { multiError = multierror.Append(multiError, fmt.Errorf("label name must be in ASCII: %s", l.Name)) } - - if _, ok := fleet.ValidLabelPlatformVariants[l.Platform]; !ok { - multiError = multierror.Append(multiError, fmt.Errorf("invalid platform for label %q: %s", l.Name, l.Platform)) - } // Check that host vitals criteria is valid if l.HostVitalsCriteria != nil { criteriaJson, err := json.Marshal(l.HostVitalsCriteria) diff --git a/pkg/spec/gitops_test.go b/pkg/spec/gitops_test.go index df5c9589ef..3755909e17 100644 --- a/pkg/spec/gitops_test.go +++ b/pkg/spec/gitops_test.go @@ -458,6 +458,215 @@ labels: assert.Empty(t, gitops.Labels[0].Hosts) } +func TestLabelInvalidFieldCombinations(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + label string + wantErrs []string + notWantErrs []string + }{ + { + name: "manual label with query", + label: ` +labels: + - name: bad + label_membership_type: manual + query: SELECT 1`, + wantErrs: []string{`label "bad" is declared as manual but contains a query`}, + }, + { + name: "manual label with criteria", + label: ` +labels: + - name: bad + label_membership_type: manual + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering`, + wantErrs: []string{`label "bad" is declared as manual but contains criteria`}, + }, + { + name: "manual label with platform", + label: ` +labels: + - name: bad + label_membership_type: manual + platform: darwin`, + wantErrs: []string{`label "bad" is declared as manual but contains a platform`}, + }, + { + name: "manual label with all invalid fields", + label: ` +labels: + - name: bad + label_membership_type: manual + query: SELECT 1 + platform: darwin + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering`, + wantErrs: []string{ + `label "bad" is declared as manual but contains a query`, + `label "bad" is declared as manual but contains criteria`, + `label "bad" is declared as manual but contains a platform`, + }, + }, + { + name: "dynamic label with criteria", + label: ` +labels: + - name: bad + label_membership_type: dynamic + query: SELECT 1 + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering`, + wantErrs: []string{`label "bad" is declared as dynamic but contains criteria`}, + }, + { + name: "dynamic label with hosts", + label: ` +labels: + - name: bad + label_membership_type: dynamic + query: SELECT 1 + hosts: + - host1`, + wantErrs: []string{`label "bad" is declared as dynamic but contains hosts`}, + }, + { + name: "dynamic label missing query", + label: ` +labels: + - name: bad + label_membership_type: dynamic`, + wantErrs: []string{`label "bad" is declared as dynamic but is missing a query`}, + }, + { + name: "dynamic label with invalid platform", + label: ` +labels: + - name: bad + label_membership_type: dynamic + query: SELECT 1 + platform: invalidplatform`, + wantErrs: []string{`label "bad" has invalid platform: "invalidplatform"`}, + }, + { + name: "dynamic label with valid platform is ok", + label: ` +labels: + - name: ok + label_membership_type: dynamic + query: SELECT 1 + platform: darwin`, + }, + { + name: "host_vitals label with query", + label: ` +labels: + - name: bad + label_membership_type: host_vitals + query: SELECT 1 + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering`, + wantErrs: []string{`label "bad" is declared as host_vitals but contains a query`}, + }, + { + name: "host_vitals label with platform", + label: ` +labels: + - name: bad + label_membership_type: host_vitals + platform: darwin + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering`, + wantErrs: []string{`label "bad" is declared as host_vitals but contains a platform`}, + }, + { + name: "host_vitals label with hosts", + label: ` +labels: + - name: bad + label_membership_type: host_vitals + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering + hosts: + - host1`, + wantErrs: []string{`label "bad" is declared as host_vitals but contains hosts`}, + }, + { + name: "host_vitals label missing criteria", + label: ` +labels: + - name: bad + label_membership_type: host_vitals`, + wantErrs: []string{`label "bad" is declared as host_vitals but is missing criteria`}, + }, + { + name: "valid manual label", + label: ` +labels: + - name: ok + label_membership_type: manual + hosts: + - host1`, + }, + { + name: "valid dynamic label", + label: ` +labels: + - name: ok + label_membership_type: dynamic + query: SELECT 1`, + }, + { + name: "valid host_vitals label", + label: ` +labels: + - name: ok + label_membership_type: host_vitals + criteria: + vital: end_user_idp_group + operator: "=" + value: Engineering`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + config := getGlobalConfig([]string{}) + config += tt.label + _, err := gitOpsFromString(t, config) + if len(tt.wantErrs) == 0 { + require.NoError(t, err) + } else { + require.Error(t, err) + for _, wantErr := range tt.wantErrs { + require.ErrorContains(t, err, wantErr) + } + } + for _, notWantErr := range tt.notWantErrs { + if err != nil { + assert.NotContains(t, err.Error(), notWantErr) + } + } + }) + } +} + func TestDuplicateQueryNames(t *testing.T) { t.Parallel() config := getGlobalConfig([]string{"reports"}) diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index 60ffa6d5fb..21c2baa9fc 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -578,7 +578,7 @@ func (ds *Datastore) GetLabelSpecs(ctx context.Context, filter fleet.TeamFilter) func (ds *Datastore) GetLabelSpec(ctx context.Context, filter fleet.TeamFilter, name string) (*fleet.LabelSpec, error) { var specs []*fleet.LabelSpec query, params, err := applyLabelTeamFilter(` -SELECT l.id, l.name, l.description, l.query, l.platform, l.label_type, l.label_membership_type, l.team_id +SELECT l.id, l.name, l.description, l.query, l.platform, l.label_type, l.label_membership_type, l.criteria, l.team_id FROM labels l WHERE l.name = ?`, filter, name) if err != nil { diff --git a/server/fleet/labels.go b/server/fleet/labels.go index cde6819f32..fda08e6af6 100644 --- a/server/fleet/labels.go +++ b/server/fleet/labels.go @@ -147,6 +147,57 @@ var ValidLabelPlatformVariants = map[string]struct{}{ "centos": {}, } +// ValidateLabelMembershipFields checks that the fields on a label spec are +// consistent with its declared membership type. It returns an +// InvalidArgumentError with field-specific entries, or nil if valid. +func ValidateLabelMembershipFields(spec *LabelSpec) *InvalidArgumentError { + var invalid InvalidArgumentError + switch spec.LabelMembershipType { + case LabelMembershipTypeManual: + if spec.Query != "" { + invalid.Append("query", fmt.Sprintf("label %q is declared as manual but contains a query", spec.Name)) + } + if spec.HostVitalsCriteria != nil { + invalid.Append("criteria", fmt.Sprintf("label %q is declared as manual but contains criteria", spec.Name)) + } + if spec.Platform != "" { + invalid.Append("platform", fmt.Sprintf("label %q is declared as manual but contains a platform", spec.Name)) + } + case LabelMembershipTypeDynamic: + if strings.TrimSpace(spec.Query) == "" { + invalid.Append("query", fmt.Sprintf("label %q is declared as dynamic but is missing a query", spec.Name)) + } + if spec.HostVitalsCriteria != nil { + invalid.Append("criteria", fmt.Sprintf("label %q is declared as dynamic but contains criteria", spec.Name)) + } + if len(spec.Hosts) > 0 { + invalid.Append("hosts", fmt.Sprintf("label %q is declared as dynamic but contains hosts", spec.Name)) + } + if spec.Platform != "" { + if _, ok := ValidLabelPlatformVariants[spec.Platform]; !ok { + invalid.Append("platform", fmt.Sprintf("label %q has invalid platform: %q", spec.Name, spec.Platform)) + } + } + case LabelMembershipTypeHostVitals: + if spec.HostVitalsCriteria == nil { + invalid.Append("criteria", fmt.Sprintf("label %q is declared as host_vitals but is missing criteria", spec.Name)) + } + if spec.Query != "" { + invalid.Append("query", fmt.Sprintf("label %q is declared as host_vitals but contains a query", spec.Name)) + } + if spec.Platform != "" { + invalid.Append("platform", fmt.Sprintf("label %q is declared as host_vitals but contains a platform", spec.Name)) + } + if len(spec.Hosts) > 0 { + invalid.Append("hosts", fmt.Sprintf("label %q is declared as host_vitals but contains hosts", spec.Name)) + } + } + if invalid.HasErrors() { + return &invalid + } + return nil +} + type Label struct { UpdateCreateTimestamps ID uint `json:"id"` diff --git a/server/service/labels.go b/server/service/labels.go index 86bd7f5ab0..1da36dc869 100644 --- a/server/service/labels.go +++ b/server/service/labels.go @@ -48,10 +48,6 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet. return nil, nil, fleet.ErrNoContext } - if _, ok := fleet.ValidLabelPlatformVariants[p.Platform]; !ok { - return nil, nil, fleet.NewInvalidArgumentError("platform", fmt.Sprintf("invalid platform: %s", p.Platform)) - } - if len(p.Hosts) > 0 && len(p.HostIDs) > 0 { return nil, nil, fleet.NewInvalidArgumentError("hosts", `Only one of either "hosts" or "host_ids" can be included in the request.`) } @@ -97,6 +93,17 @@ func (svc *Service) NewLabel(ctx context.Context, p fleet.LabelPayload) (*fleet. label.Platform = p.Platform label.Description = p.Description + // Validate field combinations for the inferred membership type + if err := fleet.ValidateLabelMembershipFields(&fleet.LabelSpec{ + Name: label.Name, + Query: label.Query, + Platform: label.Platform, + LabelMembershipType: label.LabelMembershipType, + HostVitalsCriteria: label.HostVitalsCriteria, + }); err != nil { + return nil, nil, err + } + for name := range fleet.ReservedLabelNames() { if label.Name == name { return nil, nil, fleet.NewInvalidArgumentError("name", fmt.Sprintf("cannot add label '%s' because it conflicts with the name of a built-in label", name)) @@ -615,22 +622,9 @@ func (svc *Service) ApplyLabelSpecs(ctx context.Context, specs []*fleet.LabelSpe var specLabelNamesNeedingMoving []string // should match namesToMove once specs have been checked for _, spec := range specs { - if _, ok := fleet.ValidLabelPlatformVariants[spec.Platform]; !ok { - return fleet.NewUserMessageError( - ctxerr.Errorf(ctx, "invalid platform: %s", spec.Platform), http.StatusUnprocessableEntity, - ) - } - - if spec.LabelMembershipType == fleet.LabelMembershipTypeDynamic && len(spec.Hosts) > 0 { - return fleet.NewUserMessageError( - ctxerr.Errorf(ctx, "label %s is declared as dynamic but contains `hosts` key", spec.Name), http.StatusUnprocessableEntity, - ) - } - if spec.LabelMembershipType == fleet.LabelMembershipTypeHostVitals && spec.HostVitalsCriteria == nil { - // Criteria is required for host vitals labels. - return fleet.NewUserMessageError( - ctxerr.Errorf(ctx, "label %s is declared as host vitals but contains no `criteria` key", spec.Name), http.StatusUnprocessableEntity, - ) + // Validate mutually exclusive field combinations per label membership type + if err := fleet.ValidateLabelMembershipFields(spec); err != nil { + return err.WithStatus(http.StatusUnprocessableEntity) } if spec.LabelType == fleet.LabelTypeBuiltIn { // We allow specs to contain built-in labels as long as they are not being modified. diff --git a/server/service/labels_test.go b/server/service/labels_test.go index e226a299b1..2eb78855ae 100644 --- a/server/service/labels_test.go +++ b/server/service/labels_test.go @@ -94,7 +94,7 @@ func TestWhenCreatingNewLabelsPlatformIsValidated(t *testing.T) { Platform: platform, }) if tc.ShouldFail { - require.Contains(t, err.Error(), fmt.Sprintf("invalid platform: %s", platform)) + require.Contains(t, err.Error(), fmt.Sprintf("invalid platform: %q", platform)) } else { require.NoError(t, err) require.NotNil(t, actualNewLabel) @@ -109,7 +109,7 @@ func TestWhenCreatingNewLabelsPlatformIsValidated(t *testing.T) { Platform: platform, }}, nil, nil) if tc.ShouldFail { - require.Contains(t, err.Error(), fmt.Sprintf("invalid platform: %s", platform)) + require.Contains(t, err.Error(), fmt.Sprintf("invalid platform: %q", platform)) } else { require.NoError(t, err) } @@ -770,7 +770,108 @@ func TestApplyLabelSpecsManualLabelNilHosts(t *testing.T) { }, }, nil, nil) require.Error(t, err) - require.ErrorContains(t, err, "declared as dynamic but contains `hosts` key") + require.ErrorContains(t, err, "declared as dynamic but contains hosts") + + // Dynamic label without query should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "dynamic_no_query", + LabelMembershipType: fleet.LabelMembershipTypeDynamic, + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as dynamic but is missing a query") + + // Dynamic label with criteria should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "dynamic_with_criteria", + Query: "SELECT 1", + LabelMembershipType: fleet.LabelMembershipTypeDynamic, + HostVitalsCriteria: new(json.RawMessage(`{"vital":"end_user_idp_group","operator":"=","value":"Engineering"}`)), + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as dynamic but contains criteria") + + // Manual label with query should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "manual_with_query", + LabelMembershipType: fleet.LabelMembershipTypeManual, + Query: "SELECT 1", + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as manual but contains a query") + + // Manual label with criteria should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "manual_with_criteria", + LabelMembershipType: fleet.LabelMembershipTypeManual, + HostVitalsCriteria: new(json.RawMessage(`{"vital":"end_user_idp_group","operator":"=","value":"Engineering"}`)), + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as manual but contains criteria") + + // Manual label with platform should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "manual_with_platform", + LabelMembershipType: fleet.LabelMembershipTypeManual, + Platform: "darwin", + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as manual but contains a platform") + + // Host_vitals label without criteria should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "host_vitals_no_criteria", + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as host_vitals but is missing criteria") + + // Host_vitals label with query should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "host_vitals_with_query", + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + HostVitalsCriteria: new(json.RawMessage(`{"vital":"end_user_idp_group","operator":"=","value":"Engineering"}`)), + Query: "SELECT 1", + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as host_vitals but contains a query") + + // Host_vitals label with platform should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "host_vitals_with_platform", + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + HostVitalsCriteria: new(json.RawMessage(`{"vital":"end_user_idp_group","operator":"=","value":"Engineering"}`)), + Platform: "darwin", + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as host_vitals but contains a platform") + + // Host_vitals label with hosts should be rejected + err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{ + { + Name: "host_vitals_with_hosts", + LabelMembershipType: fleet.LabelMembershipTypeHostVitals, + HostVitalsCriteria: new(json.RawMessage(`{"vital":"end_user_idp_group","operator":"=","value":"Engineering"}`)), + Hosts: []string{"host1"}, + }, + }, nil, nil) + require.Error(t, err) + require.ErrorContains(t, err, "declared as host_vitals but contains hosts") } func TestNewManualLabel(t *testing.T) { @@ -890,6 +991,54 @@ func TestNewHostVitalsLabel(t *testing.T) { }) } +func TestNewLabelFieldValidation(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) + + ds.NewLabelFunc = func(ctx context.Context, lbl *fleet.Label, opts ...fleet.OptionalArg) (*fleet.Label, error) { + lbl.ID = 1 + return lbl, nil + } + + // Manual label (no query) with platform should be rejected + _, _, err := svc.NewLabel(ctx, fleet.LabelPayload{ + Name: "manual_with_platform", + Platform: "darwin", + }) + require.Error(t, err) + require.ErrorContains(t, err, "declared as manual but contains a platform") + + // Host_vitals label with platform should be rejected + _, _, err = svc.NewLabel(ctx, fleet.LabelPayload{ + Name: "vitals_with_platform", + Platform: "darwin", + Criteria: &fleet.HostVitalCriteria{ + Vital: ptr.String("end_user_idp_group"), + Value: ptr.String("admin"), + }, + }) + require.Error(t, err) + require.ErrorContains(t, err, "declared as host_vitals but contains a platform") + + // Dynamic label with invalid platform should be rejected + _, _, err = svc.NewLabel(ctx, fleet.LabelPayload{ + Name: "dynamic_bad_platform", + Query: "SELECT 1", + Platform: "invalidplatform", + }) + require.Error(t, err) + require.ErrorContains(t, err, "invalid platform") + + // Dynamic label with valid platform should succeed + _, _, err = svc.NewLabel(ctx, fleet.LabelPayload{ + Name: "dynamic_good_platform", + Query: "SELECT 1", + Platform: "darwin", + }) + require.NoError(t, err) +} + func TestLabelActivities(t *testing.T) { ds := new(mock.Store) opts := &TestServerOpts{}