Detect unknown keys in top-level GitOps settings (#41303)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #41280 # Details Phase 2 of the "detect unknown keys in GitOps" work. The `org_settings` and `settings` top-level keys mainly shadow the `fleet.AppConfig` and `fleet.TeamConfig` types, but they have a couple of extra GitOps-only fields, so we add new GitOps-specific types for them (similar to what we already have for `GitOpsControls` and `GitOpsSoftware`. The `org_settings:` case is further complicated by the fact that its extra fields are themselves `any` types which we need to parse, so we add those to the `anyFieldTypes` registry in the validator to tell it what types to check them against. Also had to add some new logic to handle the GoogleCalendarAPI case which doesn't expose its keys as `json` tags at all, since we use a special method to obfuscate the values. I've tested this by routing the output from `fleetctl generate_gitops` back through `fleetctl gitops`, which is how I caught the `end_user_license_agreement` issue. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [ ] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. n/a - already added in previous PR ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually Did the `fleetctl generate-gitops` -> `fleetctl gitops` loop as mentioned above. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added support for managing secrets and certificate authorities through GitOps configuration * Improved detection of configuration errors with clear error messages when using unknown or misspelled settings keys, including suggestions for common typos * Enhanced error reporting for nested configuration files with precise location information <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Ian Littman <iansltx@gmail.com>
This commit is contained in:
co-authored by
Ian Littman
parent
f12a73eeaa
commit
2bf46b14ad
@@ -1046,16 +1046,8 @@ func (cmd *GenerateGitopsCommand) generateEULA() (string, error) {
|
||||
return path, nil
|
||||
}
|
||||
|
||||
// This struct is used to represent the MDM configuration that is used with GitOps.
|
||||
// It includes an additonal end user license agreement (EULA) field, which is
|
||||
// not present in the fleet.MDM struct.
|
||||
type gitopsMDM struct {
|
||||
fleet.MDM
|
||||
EndUserLicenseAgreement string `json:"end_user_license_agreement,omitempty"`
|
||||
}
|
||||
|
||||
func (cmd *GenerateGitopsCommand) generateMDM(mdm *fleet.MDM) (map[string]interface{}, error) {
|
||||
t := reflect.TypeOf(gitopsMDM{})
|
||||
t := reflect.TypeFor[spec.GitOpsMDM]()
|
||||
result := map[string]interface{}{
|
||||
jsonFieldName(t, "AppleServerURL"): mdm.AppleServerURL,
|
||||
jsonFieldName(t, "EndUserAuthentication"): mdm.EndUserAuthentication,
|
||||
|
||||
@@ -292,6 +292,29 @@ type Software struct {
|
||||
FleetMaintainedApps []fleet.MaintainedAppSpec `json:"fleet_maintained_apps"`
|
||||
}
|
||||
|
||||
// GitOpsMDM extends fleet.MDM with gitops-only fields that are not part of the server type.
|
||||
type GitOpsMDM struct {
|
||||
fleet.MDM
|
||||
EndUserLicenseAgreement any `json:"end_user_license_agreement,omitempty"`
|
||||
}
|
||||
|
||||
// GitOpsOrgSettings defines the valid keys for the top-level `org_settings:` section.
|
||||
// It embeds fleet.AppConfig for all standard settings and adds gitops-only keys
|
||||
// that are extracted before the config is sent to the server API.
|
||||
type GitOpsOrgSettings struct {
|
||||
fleet.AppConfig
|
||||
Secrets any `json:"secrets"`
|
||||
CertificateAuthorities any `json:"certificate_authorities"`
|
||||
}
|
||||
|
||||
// GitOpsFleetSettings defines the valid keys for the top-level `settings:` section (fleet-level).
|
||||
// It embeds fleet.TeamConfig for all standard settings and adds gitops-only keys
|
||||
// that are extracted before the config is sent to the server API.
|
||||
type GitOpsFleetSettings struct {
|
||||
fleet.TeamConfig
|
||||
Secrets any `json:"secrets"`
|
||||
}
|
||||
|
||||
type GitOps struct {
|
||||
TeamID *uint
|
||||
TeamName *string
|
||||
@@ -527,7 +550,9 @@ func parseOrgSettings(raw json.RawMessage, result *GitOps, baseDir string, fileP
|
||||
return multierror.Append(multiError, MaybeParseTypeError(filePath, []string{"org_settings"}, err))
|
||||
}
|
||||
noError := true
|
||||
settingsFilePath := filePath
|
||||
if orgSettingsTop.Path != nil {
|
||||
settingsFilePath = *orgSettingsTop.Path
|
||||
fileBytes, err := os.ReadFile(resolveApplyRelativePath(baseDir, *orgSettingsTop.Path))
|
||||
if err != nil {
|
||||
noError = false
|
||||
@@ -568,6 +593,8 @@ func parseOrgSettings(raw json.RawMessage, result *GitOps, baseDir string, fileP
|
||||
} else {
|
||||
multiError = parseSecrets(result, multiError)
|
||||
}
|
||||
// Validate unknown keys in org_settings section.
|
||||
multiError = multierror.Append(multiError, validateYAMLKeys(raw, reflect.TypeFor[GitOpsOrgSettings](), settingsFilePath, []string{"org_settings"})...)
|
||||
// TODO: Validate that integrations.(jira|zendesk)[].api_token is not empty or fleet.MaskedPassword
|
||||
}
|
||||
return multiError
|
||||
@@ -579,7 +606,9 @@ func parseTeamSettings(raw json.RawMessage, result *GitOps, baseDir string, file
|
||||
return multierror.Append(multiError, MaybeParseTypeError(filePath, []string{"settings"}, err))
|
||||
}
|
||||
noError := true
|
||||
settingsFilePath := filePath
|
||||
if teamSettingsTop.Path != nil {
|
||||
settingsFilePath = *teamSettingsTop.Path
|
||||
fileBytes, err := os.ReadFile(resolveApplyRelativePath(baseDir, *teamSettingsTop.Path))
|
||||
if err != nil {
|
||||
noError = false
|
||||
@@ -622,6 +651,8 @@ func parseTeamSettings(raw json.RawMessage, result *GitOps, baseDir string, file
|
||||
// Validate webhook settings for regular teams
|
||||
multiError = validateTeamWebhookSettings(result.TeamSettings, multiError)
|
||||
}
|
||||
// Validate unknown keys in team settings section.
|
||||
multiError = multierror.Append(multiError, validateYAMLKeys(raw, reflect.TypeFor[GitOpsFleetSettings](), settingsFilePath, []string{"settings"})...)
|
||||
}
|
||||
return multiError
|
||||
}
|
||||
|
||||
@@ -2445,6 +2445,160 @@ software:
|
||||
assert.Contains(t, err.Error(), "unknown_array_field")
|
||||
})
|
||||
|
||||
t.Run("unknown key in org_settings", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
org_settings:
|
||||
server_settings:
|
||||
server_url: https://fleet.example.com
|
||||
org_info:
|
||||
contact_url: https://example.com/contact
|
||||
org_name: Test Org
|
||||
unknown_org_field: true
|
||||
secrets:
|
||||
controls:
|
||||
agent_options:
|
||||
reports:
|
||||
policies:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, nil, nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "unknown_org_field")
|
||||
})
|
||||
|
||||
t.Run("unknown nested key in org_settings", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
org_settings:
|
||||
server_settings:
|
||||
server_url: https://fleet.example.com
|
||||
unknown_server_field: true
|
||||
org_info:
|
||||
contact_url: https://example.com/contact
|
||||
org_name: Test Org
|
||||
secrets:
|
||||
controls:
|
||||
agent_options:
|
||||
reports:
|
||||
policies:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, nil, nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `unknown key "org_settings.server_settings.unknown_server_field"`)
|
||||
})
|
||||
|
||||
t.Run("unknown key in org_settings with typo suggestion", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
org_settings:
|
||||
server_settigns:
|
||||
server_url: https://fleet.example.com
|
||||
org_info:
|
||||
contact_url: https://example.com/contact
|
||||
org_name: Test Org
|
||||
secrets:
|
||||
controls:
|
||||
agent_options:
|
||||
reports:
|
||||
policies:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, nil, nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "org_settings.server_settigns")
|
||||
assert.Contains(t, err.Error(), `did you mean "server_settings"?`)
|
||||
})
|
||||
|
||||
t.Run("unknown key in fleet settings", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
name: FleetName
|
||||
settings:
|
||||
secrets:
|
||||
unknown_fleet_field: true
|
||||
agent_options:
|
||||
controls:
|
||||
reports:
|
||||
policies:
|
||||
software:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "unknown_fleet_field")
|
||||
})
|
||||
|
||||
t.Run("unknown nested key in fleet settings webhook_settings", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
name: FleetName
|
||||
settings:
|
||||
secrets:
|
||||
webhook_settings:
|
||||
unknown_webhook_field: true
|
||||
agent_options:
|
||||
controls:
|
||||
reports:
|
||||
policies:
|
||||
software:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `unknown key "settings.webhook_settings.unknown_webhook_field"`)
|
||||
})
|
||||
|
||||
t.Run("unknown key in org_settings via path", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
org_settings:
|
||||
path: org_settings.yml
|
||||
controls:
|
||||
agent_options:
|
||||
reports:
|
||||
policies:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
orgSettingsYAML := `
|
||||
server_settings:
|
||||
server_url: https://fleet.example.com
|
||||
org_info:
|
||||
contact_url: https://example.com/contact
|
||||
org_name: Test Org
|
||||
unknown_org_path_field: true
|
||||
secrets:
|
||||
`
|
||||
require.NoError(t, os.WriteFile(filepath.Join(basePath, "org_settings.yml"), []byte(orgSettingsYAML), 0o644))
|
||||
_, err := GitOpsFromFile(path, basePath, nil, nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `unknown key "org_settings.unknown_org_path_field" in "org_settings.yml"`)
|
||||
})
|
||||
|
||||
t.Run("unknown key in fleet settings via path", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := `
|
||||
name: FleetName
|
||||
settings:
|
||||
path: fleet_settings.yml
|
||||
agent_options:
|
||||
controls:
|
||||
reports:
|
||||
policies:
|
||||
software:
|
||||
`
|
||||
path, basePath := createTempFile(t, "", config)
|
||||
fleetSettingsYAML := `
|
||||
secrets:
|
||||
unknown_fleet_path_field: true
|
||||
`
|
||||
require.NoError(t, os.WriteFile(filepath.Join(basePath, "fleet_settings.yml"), []byte(fleetSettingsYAML), 0o644))
|
||||
_, err := GitOpsFromFile(path, basePath, premiumAppConfig(), nopLogf)
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `unknown key "settings.unknown_fleet_path_field" in "fleet_settings.yml"`)
|
||||
})
|
||||
|
||||
t.Run("unknown key in policy install_software package_path", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
config := getTeamConfig([]string{"policies", "software"})
|
||||
|
||||
@@ -19,6 +19,14 @@ type fieldInfo struct {
|
||||
typ reflect.Type
|
||||
}
|
||||
|
||||
// ValidKeysProvider is implemented by types with custom JSON marshaling
|
||||
// that want to declare valid keys for gitops unknown-key validation.
|
||||
type ValidKeysProvider interface {
|
||||
ValidKeys() []string
|
||||
}
|
||||
|
||||
var validKeysProviderType = reflect.TypeFor[ValidKeysProvider]()
|
||||
|
||||
var (
|
||||
knownKeysCache = make(map[reflect.Type]map[string]fieldInfo)
|
||||
knownKeysCacheMu sync.Mutex
|
||||
@@ -26,6 +34,7 @@ var (
|
||||
|
||||
// knownJSONKeys extracts the set of valid JSON field names from a struct type,
|
||||
// including fields from embedded structs. Results are cached per type.
|
||||
// For types implementing ValidKeysProvider, the declared keys are used instead.
|
||||
func knownJSONKeys(t reflect.Type) map[string]fieldInfo {
|
||||
if t.Kind() == reflect.Ptr {
|
||||
t = t.Elem()
|
||||
@@ -42,7 +51,22 @@ func knownJSONKeys(t reflect.Type) map[string]fieldInfo {
|
||||
}
|
||||
|
||||
keys := make(map[string]fieldInfo)
|
||||
collectFields(t, keys)
|
||||
|
||||
// If the type (or pointer to it) implements ValidKeysProvider, use those
|
||||
// keys instead of reflecting on struct fields. This handles types with
|
||||
// custom JSON marshaling (e.g. GoogleCalendarApiKey).
|
||||
if reflect.PointerTo(t).Implements(validKeysProviderType) || t.Implements(validKeysProviderType) {
|
||||
provider := reflect.New(t).Interface().(ValidKeysProvider)
|
||||
for _, name := range provider.ValidKeys() {
|
||||
keys[name] = fieldInfo{
|
||||
jsonName: name,
|
||||
typ: reflect.TypeFor[any](),
|
||||
}
|
||||
}
|
||||
} else {
|
||||
collectFields(t, keys)
|
||||
}
|
||||
|
||||
knownKeysCache[t] = keys
|
||||
return keys
|
||||
}
|
||||
@@ -108,6 +132,10 @@ var anyFieldTypes = map[reflect.Type]map[string]reflect.Type{
|
||||
"windows_settings": reflect.TypeFor[fleet.WindowsSettings](),
|
||||
"android_settings": reflect.TypeFor[fleet.AndroidSettings](),
|
||||
},
|
||||
reflect.TypeFor[GitOpsOrgSettings](): {
|
||||
"certificate_authorities": reflect.TypeFor[fleet.GroupedCertificateAuthorities](),
|
||||
"mdm": reflect.TypeFor[GitOpsMDM](),
|
||||
},
|
||||
}
|
||||
|
||||
// suggestKey returns the closest known key name if one is within a reasonable
|
||||
@@ -159,7 +187,9 @@ func validateMapKeys(data map[string]any, targetType reflect.Type, path []string
|
||||
}
|
||||
|
||||
known := knownJSONKeys(targetType)
|
||||
if known == nil {
|
||||
if len(known) == 0 {
|
||||
// No JSON-tagged fields: either not a struct or a struct with custom
|
||||
// serialization. Skip validation since we don't know the expected keys.
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -181,13 +211,13 @@ func validateMapKeys(data map[string]any, targetType reflect.Type, path []string
|
||||
// Determine the type to recurse into.
|
||||
fieldType := fi.typ
|
||||
|
||||
// If the field type is `any` (interface{}), check the override registry.
|
||||
if fieldType.Kind() == reflect.Interface {
|
||||
if override, ok := parentOverrides[key]; ok { // indexing a nil map is safe; ok will be false
|
||||
fieldType = override
|
||||
} else {
|
||||
continue // any-typed field with no override, skip
|
||||
}
|
||||
// Check the override registry for this field. This handles two cases:
|
||||
// 1. `any`/`interface{}` fields that need a concrete type for recursion
|
||||
// 2. Struct fields that need a gitops-extended type (e.g. fleet.MDM -> GitOpsMDM)
|
||||
if override, ok := parentOverrides[key]; ok {
|
||||
fieldType = override
|
||||
} else if fieldType.Kind() == reflect.Interface {
|
||||
continue // any-typed field with no override, skip
|
||||
}
|
||||
|
||||
// Recurse into nested structs or slices.
|
||||
|
||||
@@ -129,6 +129,42 @@ func TestValidateUnknownKeys(t *testing.T) {
|
||||
assert.Len(t, errs, 2)
|
||||
})
|
||||
|
||||
t.Run("ValidKeysProvider accepts declared keys", func(t *testing.T) {
|
||||
// GoogleCalendarApiKey implements ValidKeysProvider to declare accepted
|
||||
// keys for its custom JSON marshaling.
|
||||
data := map[string]any{
|
||||
"google_calendar": []any{
|
||||
map[string]any{
|
||||
"domain": "example.com",
|
||||
"api_key_json": map[string]any{
|
||||
"client_email": "test@example.com",
|
||||
"private_key": "some value",
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
errs := validateUnknownKeys(data, reflect.TypeFor[fleet.Integrations](), []string{"org_settings", "integrations"}, "test.yml")
|
||||
assert.Empty(t, errs)
|
||||
})
|
||||
|
||||
t.Run("ValidKeysProvider rejects undeclared keys", func(t *testing.T) {
|
||||
data := map[string]any{
|
||||
"google_calendar": []any{
|
||||
map[string]any{
|
||||
"domain": "example.com",
|
||||
"api_key_json": map[string]any{
|
||||
"client_email": "test@example.com",
|
||||
"private_key": "nothing to see here",
|
||||
"bad_field": "unknown",
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
errs := validateUnknownKeys(data, reflect.TypeFor[fleet.Integrations](), []string{"org_settings", "integrations"}, "test.yml")
|
||||
require.Len(t, errs, 1)
|
||||
assert.Contains(t, errs[0].Error(), "bad_field")
|
||||
})
|
||||
|
||||
t.Run("scalar data no errors", func(t *testing.T) {
|
||||
errs := validateUnknownKeys("just a string", reflect.TypeFor[fleet.QuerySpec](), nil, "test.yml")
|
||||
assert.Empty(t, errs)
|
||||
@@ -308,6 +344,26 @@ func TestAnyFieldTypeRegistry(t *testing.T) {
|
||||
assert.Contains(t, overrides, "windows_settings")
|
||||
assert.Contains(t, overrides, "android_settings")
|
||||
})
|
||||
|
||||
t.Run("org_settings certificate_authorities any-field recursion", func(t *testing.T) {
|
||||
data := map[string]any{
|
||||
"certificate_authorities": map[string]any{
|
||||
"ndes_scep_proxy": map[string]any{},
|
||||
"digicert": []any{},
|
||||
"unknown_ca_type": "bad",
|
||||
},
|
||||
}
|
||||
errs := validateUnknownKeys(data, reflect.TypeFor[GitOpsOrgSettings](), []string{"org_settings"}, "test.yml")
|
||||
require.Len(t, errs, 1)
|
||||
assert.Contains(t, errs[0].Error(), "unknown_ca_type")
|
||||
assert.Contains(t, errs[0].Error(), "org_settings.certificate_authorities")
|
||||
})
|
||||
|
||||
t.Run("org_settings registered types present", func(t *testing.T) {
|
||||
overrides, ok := anyFieldTypes[reflect.TypeFor[GitOpsOrgSettings]()]
|
||||
require.True(t, ok)
|
||||
assert.Contains(t, overrides, "certificate_authorities")
|
||||
})
|
||||
}
|
||||
|
||||
func TestValidateRawKeys(t *testing.T) {
|
||||
|
||||
@@ -389,6 +389,10 @@ const (
|
||||
GoogleCalendarPrivateKey = "private_key"
|
||||
)
|
||||
|
||||
// googleCalendarKeyNames lists all valid JSON keys for GoogleCalendarApiKey,
|
||||
// used by ValidKeys() for gitops unknown-key validation.
|
||||
var googleCalendarKeyNames = []string{GoogleCalendarEmail, GoogleCalendarPrivateKey}
|
||||
|
||||
// GoogleCalendarApiKey is a custom type for the Google Calendar API key JSON.
|
||||
// It handles JSON marshaling/unmarshaling with support for masking sensitive data.
|
||||
// When marshaled in masked state, it serializes to just "********".
|
||||
@@ -400,6 +404,13 @@ type GoogleCalendarApiKey struct {
|
||||
masked bool
|
||||
}
|
||||
|
||||
// ValidKeys returns the set of accepted JSON keys for this type.
|
||||
// This is used by gitops validation to check for unknown keys in types
|
||||
// with custom JSON marshaling.
|
||||
func (GoogleCalendarApiKey) ValidKeys() []string {
|
||||
return googleCalendarKeyNames
|
||||
}
|
||||
|
||||
// MarshalJSON implements json.Marshaler. When masked, returns "********".
|
||||
// Otherwise, returns the JSON object representation of the values.
|
||||
func (k GoogleCalendarApiKey) MarshalJSON() ([]byte, error) {
|
||||
@@ -566,7 +577,7 @@ func ValidateGoogleCalendarIntegrations(intgs []*GoogleCalendarIntegration, inva
|
||||
)
|
||||
}
|
||||
}
|
||||
if privateKey, ok := intg.ApiKey.Values["private_key"]; !ok {
|
||||
if privateKey, ok := intg.ApiKey.Values[GoogleCalendarPrivateKey]; !ok {
|
||||
invalid.Append(
|
||||
fmt.Sprintf("integrations.google_calendar.api_key_json.%s", GoogleCalendarPrivateKey),
|
||||
fmt.Sprintf("%s is required", GoogleCalendarPrivateKey),
|
||||
|
||||
Reference in New Issue
Block a user