From 554e024f7b27aee8048e88eee10b47556df079ad Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Mon, 7 Aug 2023 13:51:11 -0400 Subject: [PATCH] Fix gitops access when using `--dry-run` with `fleetctl apply` (#13178) --- changes/issue-13177-fix-gitops-fleetctl-apply | 1 + cmd/fleetctl/apply_test.go | 29 +++++++++++++++++++ ee/server/service/mdm.go | 10 +++++-- server/service/appconfig.go | 10 +++++-- server/service/apple_mdm.go | 13 ++++----- server/service/integration_mdm_test.go | 11 +++++++ 6 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 changes/issue-13177-fix-gitops-fleetctl-apply diff --git a/changes/issue-13177-fix-gitops-fleetctl-apply b/changes/issue-13177-fix-gitops-fleetctl-apply new file mode 100644 index 0000000000..e225858c4f --- /dev/null +++ b/changes/issue-13177-fix-gitops-fleetctl-apply @@ -0,0 +1 @@ +* Fixed an issue when a user with `gitops` role was used to validate a configuration with `fleetctl apply --dry-run`. diff --git a/cmd/fleetctl/apply_test.go b/cmd/fleetctl/apply_test.go index c2175f87d6..e6adbcf53c 100644 --- a/cmd/fleetctl/apply_test.go +++ b/cmd/fleetctl/apply_test.go @@ -887,6 +887,11 @@ spec: pack_delimiter: / overrides: {} `) + + // test applying with dry-run flag + assert.Equal(t, "[+] would've applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"})) + + // test applying for real assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name})) assert.True(t, currentAppConfig.Features.EnableHostUsers) @@ -914,6 +919,11 @@ spec: macos_setup_assistant: %s windows_enabled_and_configured: true `, mobileConfigPath, emptySetupAsst)) + + // first apply with dry-run + assert.Equal(t, "[+] would've applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"})) + + // then apply for real assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name})) // features left untouched, not provided assert.True(t, currentAppConfig.Features.EnableHostUsers) @@ -945,6 +955,11 @@ spec: macos_setup: bootstrap_package: %s `, bootstrapURL)) + + // first apply with dry-run + assert.Equal(t, "[+] would've applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"})) + + // then apply for real assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name})) // features left untouched, not provided assert.True(t, currentAppConfig.Features.EnableHostUsers) @@ -988,6 +1003,10 @@ spec: - secret: BBB `, mobileConfigPath)) + // first apply with dry-run + require.Equal(t, "[+] would've applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"})) + + // then apply for real require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name})) assert.JSONEq(t, string(json.RawMessage(`{"config":{"views":{"foo":"qux"}}}`)), string(*savedTeam.Config.AgentOptions)) assert.Equal(t, fleet.TeamMDM{ @@ -1015,6 +1034,11 @@ spec: macos_setup: macos_setup_assistant: %s `, emptySetupAsst)) + + // first apply with dry-run + require.Equal(t, "[+] would've applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"})) + + // then apply for real require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name})) require.True(t, ds.GetMDMAppleSetupAssistantFuncInvoked) require.True(t, ds.SetOrUpdateMDMAppleSetupAssistantFuncInvoked) @@ -1045,6 +1069,11 @@ spec: macos_setup: bootstrap_package: %s `, bootstrapURL)) + + // first apply with dry-run + require.Equal(t, "[+] would've applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"})) + + // then apply for real require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name})) // all left untouched, only bootstrap package added assert.Equal(t, fleet.TeamMDM{ diff --git a/ee/server/service/mdm.go b/ee/server/service/mdm.go index f86b14632a..5cea7df71c 100644 --- a/ee/server/service/mdm.go +++ b/ee/server/service/mdm.go @@ -219,7 +219,10 @@ func (svc *Service) UpdateMDMAppleSetup(ctx context.Context, payload fleet.MDMAp } func (svc *Service) updateAppConfigMDMAppleSetup(ctx context.Context, payload fleet.MDMAppleSetupPayload) error { - ac, err := svc.AppConfigObfuscated(ctx) + // appconfig is only used internally, it's fine to read it unobfuscated + // (svc.AppConfigObfuscated must not be used because the write-only users + // such as gitops will fail to access it). + ac, err := svc.ds.AppConfig(ctx) if err != nil { return err } @@ -264,7 +267,10 @@ func (svc *Service) updateMacOSSetupEnableEndUserAuth(ctx context.Context, enabl } func (svc *Service) validateMDMAppleSetupPayload(ctx context.Context, payload fleet.MDMAppleSetupPayload) error { - ac, err := svc.AppConfigObfuscated(ctx) + // appconfig is only used internally, it's fine to read it unobfuscated + // (svc.AppConfigObfuscated must not be used because the write-only users + // such as gitops will fail to access it). + ac, err := svc.ds.AppConfig(ctx) if err != nil { return err } diff --git a/server/service/appconfig.go b/server/service/appconfig.go index 25a2773d62..36417d44cd 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -363,8 +363,14 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle if legacyUsedWarning != nil { return nil, legacyUsedWarning } - // must reload to get the unchanged app config - return svc.AppConfigObfuscated(ctx) + + // must reload to get the unchanged app config (retrieve with obfuscated secrets) + obfuscatedAppConfig, err := svc.ds.AppConfig(ctx) + if err != nil { + return nil, err + } + obfuscatedAppConfig.Obfuscate() + return obfuscatedAppConfig, nil } // Perform validation of the applied SMTP settings. diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 4cad0b2d56..28c38ab66b 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -1681,12 +1681,8 @@ func (svc *Service) UpdateMDMAppleSettings(ctx context.Context, payload fleet.MD // for now, assume all settings require premium (this is true for the first // supported setting, enable_disk_encryption. Adjust as needed in the future // if this is not always the case). - license, err := svc.License(ctx) - if err != nil { - svc.authz.SkipAuthorization(ctx) // so that the error message is not replaced by "forbidden" - return err - } - if !license.IsPremium() { + lic, _ := license.FromContext(ctx) + if lic == nil || !lic.IsPremium() { svc.authz.SkipAuthorization(ctx) // so that the error message is not replaced by "forbidden" return ErrMissingLicense } @@ -1706,7 +1702,10 @@ func (svc *Service) UpdateMDMAppleSettings(ctx context.Context, payload fleet.MD } func (svc *Service) updateAppConfigMDMAppleSettings(ctx context.Context, payload fleet.MDMAppleSettingsPayload) error { - ac, err := svc.AppConfigObfuscated(ctx) + // appconfig is only used internally, it's fine to read it unobfuscated + // (svc.AppConfigObfuscated must not be used because the write-only users + // such as gitops will fail to access it). + ac, err := svc.ds.AppConfig(ctx) if err != nil { return err } diff --git a/server/service/integration_mdm_test.go b/server/service/integration_mdm_test.go index 7b378d2c4d..ec0175cd07 100644 --- a/server/service/integration_mdm_test.go +++ b/server/service/integration_mdm_test.go @@ -4882,6 +4882,17 @@ func (s *integrationMDMTestSuite) TestGitOpsUserActions() { }`), http.StatusOK, &acResp) assert.True(t, acResp.MDM.MacOSSettings.EnableDiskEncryption) + // Attempt to setup Apple MDM, will fail but the important thing is that it + // fails with 422 (cannot enable end user auth because no IdP is configured) + // and not 403 forbidden. + s.Do("PATCH", "/api/latest/fleet/mdm/apple/setup", + fleet.MDMAppleSetupPayload{TeamID: ptr.Uint(0), EnableEndUserAuthentication: ptr.Bool(true)}, http.StatusUnprocessableEntity) + + // Attempt to update the Apple MDM settings but with no change, just to + // validate the access. + s.Do("PATCH", "/api/latest/fleet/mdm/apple/settings", + fleet.MDMAppleSettingsPayload{}, http.StatusNoContent) + // Attempt to set profile batch globally, should allow. globalProfiles := [][]byte{ mobileconfigForTest("N1", "I1"),