From 6dbd5c3d60b10bb3004742eaca8360e15daf2254 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Mon, 30 Dec 2024 14:07:25 -0600 Subject: [PATCH 1/2] Improve secret variables error on software upload. --- ee/server/service/maintained_apps.go | 14 +++++- ee/server/service/software_installers.go | 35 ++++++++++++++- server/service/base_client_errors.go | 15 +++++-- server/service/integration_enterprise_test.go | 44 ++++++++++++++++--- server/service/testing_client.go | 21 +++++++-- 5 files changed, 112 insertions(+), 17 deletions(-) diff --git a/ee/server/service/maintained_apps.go b/ee/server/service/maintained_apps.go index f459d098cb13..cf77da4a98f7 100644 --- a/ee/server/service/maintained_apps.go +++ b/ee/server/service/maintained_apps.go @@ -44,7 +44,19 @@ func (svc *Service) AddFleetMaintainedApp( } if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{installScript, postInstallScript, uninstallScript}); err != nil { - return 0, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("script", err.Error())) + // We redo the validation on each script to find out which script has the missing secret. + // This is done to provide a more informative error message to the UI user. + if errInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{installScript}); errInstallScript != nil { + return 0, fleet.NewInvalidArgumentError("install script", errInstallScript.Error()) + } + if errPostInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{postInstallScript}); errPostInstallScript != nil { + return 0, fleet.NewInvalidArgumentError("post-install script", errPostInstallScript.Error()) + } + if errUninstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{uninstallScript}); errUninstallScript != nil { + return 0, fleet.NewInvalidArgumentError("uninstall script", errUninstallScript.Error()) + } + // We should not get to this point. If we did, it means we have another issue, such as large read replica latency. + return 0, ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets") } app, err := svc.ds.GetMaintainedAppByID(ctx, appID) diff --git a/ee/server/service/software_installers.go b/ee/server/service/software_installers.go index 930677637202..c48af52494a4 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -96,7 +96,19 @@ func (svc *Service) UploadSoftwareInstaller(ctx context.Context, payload *fleet. preProcessUninstallScript(payload) if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.InstallScript, payload.PostInstallScript, payload.UninstallScript}); err != nil { - return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("script", err.Error())) + // We redo the validation on each script to find out which script has the missing secret. + // This is done to provide a more informative error message to the UI user. + if errInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.InstallScript}); errInstallScript != nil { + return fleet.NewInvalidArgumentError("install script", errInstallScript.Error()) + } + if errPostInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.PostInstallScript}); errPostInstallScript != nil { + return fleet.NewInvalidArgumentError("post-install script", errPostInstallScript.Error()) + } + if errUninstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.UninstallScript}); errUninstallScript != nil { + return fleet.NewInvalidArgumentError("uninstall script", errUninstallScript.Error()) + } + // We should not get to this point. If we did, it means we have another issue, such as large read replica latency. + return ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets") } installerID, titleID, err := svc.ds.MatchOrCreateSoftwareInstaller(ctx, payload) @@ -228,7 +240,26 @@ func (svc *Service) UpdateSoftwareInstaller(ctx context.Context, payload *fleet. } if err := svc.ds.ValidateEmbeddedSecrets(ctx, scripts); err != nil { - return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("script", err.Error())) + // We redo the validation on each script to find out which script has the missing secret. + // This is done to provide a more informative error message to the UI user. + if payload.InstallScript != nil { + if errInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{*payload.InstallScript}); errInstallScript != nil { + return nil, fleet.NewInvalidArgumentError("install script", errInstallScript.Error()) + } + } + if payload.PostInstallScript != nil { + if errPostInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, + []string{*payload.PostInstallScript}); errPostInstallScript != nil { + return nil, fleet.NewInvalidArgumentError("post-install script", errPostInstallScript.Error()) + } + } + if payload.UninstallScript != nil { + if errUninstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{*payload.UninstallScript}); errUninstallScript != nil { + return nil, fleet.NewInvalidArgumentError("uninstall script", errUninstallScript.Error()) + } + } + // We should not get to this point. If we did, it means we have another issue, such as large read replica latency. + return nil, ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets") } // get software by ID, fail if it does not exist or does not have an existing installer diff --git a/server/service/base_client_errors.go b/server/service/base_client_errors.go index 7bea24d99a44..49aa443fbc0a 100644 --- a/server/service/base_client_errors.go +++ b/server/service/base_client_errors.go @@ -104,17 +104,24 @@ type serverError struct { } func extractServerErrorText(body io.Reader) string { + _, reason := extractServerErrorNameReason(body) + return reason +} + +func extractServerErrorNameReason(body io.Reader) (string, string) { var serverErr serverError if err := json.NewDecoder(body).Decode(&serverErr); err != nil { - return "unknown" + return "", "unknown" } - errText := serverErr.Message + errName := "" + errReason := serverErr.Message if len(serverErr.Errors) > 0 { - errText += ": " + serverErr.Errors[0].Reason + errReason += ": " + serverErr.Errors[0].Reason + errName = serverErr.Errors[0].Name } - return errText + return errName, errReason } type statusCodeErr struct { diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 13b058436b7d..a0fa1870231b 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -8930,7 +8930,8 @@ func (s *integrationEnterpriseTestSuite) TestAllSoftwareTitles() { PostInstallScript: "d", SelfService: true, } - s.uploadSoftwareInstaller(t, payloadEmacsMissingSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID") + s.uploadSoftwareInstallerWithErrorNameReason(t, payloadEmacsMissingSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID", + "install script") payloadEmacsMissingPostSecret := &fleet.UploadSoftwareInstallerPayload{ InstallScript: "install", @@ -8938,7 +8939,8 @@ func (s *integrationEnterpriseTestSuite) TestAllSoftwareTitles() { PostInstallScript: "d $FLEET_SECRET_INVALID", SelfService: true, } - s.uploadSoftwareInstaller(t, payloadEmacsMissingPostSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID") + s.uploadSoftwareInstallerWithErrorNameReason(t, payloadEmacsMissingPostSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID", + "post-install script") payloadEmacsMissingUnSecret := &fleet.UploadSoftwareInstallerPayload{ InstallScript: "install", @@ -8947,7 +8949,8 @@ func (s *integrationEnterpriseTestSuite) TestAllSoftwareTitles() { UninstallScript: "delet $FLEET_SECRET_INVALID", SelfService: true, } - s.uploadSoftwareInstaller(t, payloadEmacsMissingUnSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID") + s.uploadSoftwareInstallerWithErrorNameReason(t, payloadEmacsMissingUnSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID", + "uninstall script") payloadEmacs := &fleet.UploadSoftwareInstallerPayload{ InstallScript: "install", @@ -15943,10 +15946,37 @@ func (s *integrationEnterpriseTestSuite) TestMaintainedApps() { UninstallScript: "echo $FLEET_SECRET_INVALID3", } respBadSecret := s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) - errMsg := extractServerErrorText(respBadSecret.Body) - require.Contains(t, errMsg, "$FLEET_SECRET_INVALID1") - require.Contains(t, errMsg, "$FLEET_SECRET_INVALID2") - require.Contains(t, errMsg, "$FLEET_SECRET_INVALID3") + errName, errMsg := extractServerErrorNameReason(respBadSecret.Body) + assert.Equal(t, "install script", errName) + assert.Contains(t, errMsg, "$FLEET_SECRET_INVALID1") + + reqInvalidSecret = &addFleetMaintainedAppRequest{ + AppID: 1, + TeamID: &team.ID, + SelfService: true, + PreInstallQuery: "SELECT 1", + InstallScript: "echo foo", + PostInstallScript: "echo done $FLEET_SECRET_INVALID2", + UninstallScript: "echo $FLEET_SECRET_INVALID3", + } + respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) + errName, errMsg = extractServerErrorNameReason(respBadSecret.Body) + assert.Equal(t, "post-install script", errName) + assert.Contains(t, errMsg, "$FLEET_SECRET_INVALID2") + + reqInvalidSecret = &addFleetMaintainedAppRequest{ + AppID: 1, + TeamID: &team.ID, + SelfService: true, + PreInstallQuery: "SELECT 1", + InstallScript: "echo foo", + PostInstallScript: "echo done", + UninstallScript: "echo $FLEET_SECRET_INVALID3", + } + respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) + errName, errMsg = extractServerErrorNameReason(respBadSecret.Body) + assert.Equal(t, "uninstall script", errName) + assert.Contains(t, errMsg, "$FLEET_SECRET_INVALID3") // Add an ingested app to the team var addMAResp addFleetMaintainedAppResponse diff --git a/server/service/testing_client.go b/server/service/testing_client.go index 9c6ce0df8f16..b3e0c8966b5a 100644 --- a/server/service/testing_client.go +++ b/server/service/testing_client.go @@ -557,6 +557,16 @@ func (ts *withServer) uploadSoftwareInstaller( payload *fleet.UploadSoftwareInstallerPayload, expectedStatus int, expectedError string, +) { + ts.uploadSoftwareInstallerWithErrorNameReason(t, payload, expectedStatus, expectedError, "") +} + +func (ts *withServer) uploadSoftwareInstallerWithErrorNameReason( + t *testing.T, + payload *fleet.UploadSoftwareInstallerPayload, + expectedStatus int, + expectedErrorReason string, + expectedErrorName string, ) { t.Helper() @@ -621,9 +631,14 @@ func (ts *withServer) uploadSoftwareInstaller( r := ts.DoRawWithHeaders("POST", "/api/latest/fleet/software/package", b.Bytes(), expectedStatus, headers) defer r.Body.Close() - if expectedError != "" { - errMsg := extractServerErrorText(r.Body) - require.Contains(t, errMsg, expectedError) + if expectedErrorReason != "" || expectedErrorName != "" { + errName, errReason := extractServerErrorNameReason(r.Body) + if expectedErrorName != "" { + require.Equal(t, expectedErrorName, errName) + } + if expectedErrorReason != "" { + require.Contains(t, errReason, expectedErrorReason) + } } } From 22ccf7289b05db0b34a44d3d22877373ff9560ba Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Mon, 30 Dec 2024 14:46:32 -0600 Subject: [PATCH 2/2] Further improvements. --- ee/server/service/maintained_apps.go | 14 +++--- ee/server/service/software_installers.go | 49 ++++++++++--------- server/service/base_client_errors.go | 16 ++++++ server/service/integration_enterprise_test.go | 25 ++++++---- 4 files changed, 63 insertions(+), 41 deletions(-) diff --git a/ee/server/service/maintained_apps.go b/ee/server/service/maintained_apps.go index cf77da4a98f7..f195e99158ba 100644 --- a/ee/server/service/maintained_apps.go +++ b/ee/server/service/maintained_apps.go @@ -46,14 +46,12 @@ func (svc *Service) AddFleetMaintainedApp( if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{installScript, postInstallScript, uninstallScript}); err != nil { // We redo the validation on each script to find out which script has the missing secret. // This is done to provide a more informative error message to the UI user. - if errInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{installScript}); errInstallScript != nil { - return 0, fleet.NewInvalidArgumentError("install script", errInstallScript.Error()) - } - if errPostInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{postInstallScript}); errPostInstallScript != nil { - return 0, fleet.NewInvalidArgumentError("post-install script", errPostInstallScript.Error()) - } - if errUninstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{uninstallScript}); errUninstallScript != nil { - return 0, fleet.NewInvalidArgumentError("uninstall script", errUninstallScript.Error()) + var argErr *fleet.InvalidArgumentError + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "install script", &installScript, argErr) + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "post-install script", &postInstallScript, argErr) + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "uninstall script", &uninstallScript, argErr) + if argErr != nil { + return 0, argErr } // We should not get to this point. If we did, it means we have another issue, such as large read replica latency. return 0, ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets") diff --git a/ee/server/service/software_installers.go b/ee/server/service/software_installers.go index c48af52494a4..81b539c5b1f4 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -98,14 +98,12 @@ func (svc *Service) UploadSoftwareInstaller(ctx context.Context, payload *fleet. if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.InstallScript, payload.PostInstallScript, payload.UninstallScript}); err != nil { // We redo the validation on each script to find out which script has the missing secret. // This is done to provide a more informative error message to the UI user. - if errInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.InstallScript}); errInstallScript != nil { - return fleet.NewInvalidArgumentError("install script", errInstallScript.Error()) - } - if errPostInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.PostInstallScript}); errPostInstallScript != nil { - return fleet.NewInvalidArgumentError("post-install script", errPostInstallScript.Error()) - } - if errUninstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.UninstallScript}); errUninstallScript != nil { - return fleet.NewInvalidArgumentError("uninstall script", errUninstallScript.Error()) + var argErr *fleet.InvalidArgumentError + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "install script", &payload.InstallScript, argErr) + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "post-install script", &payload.PostInstallScript, argErr) + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "uninstall script", &payload.UninstallScript, argErr) + if argErr != nil { + return argErr } // We should not get to this point. If we did, it means we have another issue, such as large read replica latency. return ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets") @@ -242,21 +240,12 @@ func (svc *Service) UpdateSoftwareInstaller(ctx context.Context, payload *fleet. if err := svc.ds.ValidateEmbeddedSecrets(ctx, scripts); err != nil { // We redo the validation on each script to find out which script has the missing secret. // This is done to provide a more informative error message to the UI user. - if payload.InstallScript != nil { - if errInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{*payload.InstallScript}); errInstallScript != nil { - return nil, fleet.NewInvalidArgumentError("install script", errInstallScript.Error()) - } - } - if payload.PostInstallScript != nil { - if errPostInstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, - []string{*payload.PostInstallScript}); errPostInstallScript != nil { - return nil, fleet.NewInvalidArgumentError("post-install script", errPostInstallScript.Error()) - } - } - if payload.UninstallScript != nil { - if errUninstallScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{*payload.UninstallScript}); errUninstallScript != nil { - return nil, fleet.NewInvalidArgumentError("uninstall script", errUninstallScript.Error()) - } + var argErr *fleet.InvalidArgumentError + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "install script", payload.InstallScript, argErr) + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "post-install script", payload.PostInstallScript, argErr) + argErr = svc.validateEmbeddedSecretsOnScript(ctx, "uninstall script", payload.UninstallScript, argErr) + if argErr != nil { + return nil, argErr } // We should not get to this point. If we did, it means we have another issue, such as large read replica latency. return nil, ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets") @@ -494,6 +483,20 @@ func (svc *Service) UpdateSoftwareInstaller(ctx context.Context, payload *fleet. return updatedInstaller, nil } +func (svc *Service) validateEmbeddedSecretsOnScript(ctx context.Context, scriptName string, script *string, + argErr *fleet.InvalidArgumentError) *fleet.InvalidArgumentError { + if script != nil { + if errScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{*script}); errScript != nil { + if argErr != nil { + argErr.Append(scriptName, errScript.Error()) + } else { + argErr = fleet.NewInvalidArgumentError(scriptName, errScript.Error()) + } + } + } + return argErr +} + func ValidateSoftwareLabelsForUpdate(ctx context.Context, svc fleet.Service, existingInstaller *fleet.SoftwareInstaller, includeAny, excludeAny []string) (shouldUpdate bool, validatedLabels *fleet.LabelIdentsWithScope, err error) { if authctx, ok := authz_ctx.FromContext(ctx); !ok { return false, nil, fleet.NewAuthRequiredError("batch validate labels: missing authorization context") diff --git a/server/service/base_client_errors.go b/server/service/base_client_errors.go index 49aa443fbc0a..88f8a305f473 100644 --- a/server/service/base_client_errors.go +++ b/server/service/base_client_errors.go @@ -124,6 +124,22 @@ func extractServerErrorNameReason(body io.Reader) (string, string) { return errName, errReason } +func extractServerErrorNameReasons(body io.Reader) ([]string, []string) { + var serverErr serverError + if err := json.NewDecoder(body).Decode(&serverErr); err != nil { + return []string{""}, []string{"unknown"} + } + + var errName []string + var errReason []string + for _, err := range serverErr.Errors { + errName = append(errName, err.Name) + errReason = append(errReason, err.Reason) + } + + return errName, errReason +} + type statusCodeErr struct { code int body string diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index a0fa1870231b..76acce2cebd2 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -15946,9 +15946,12 @@ func (s *integrationEnterpriseTestSuite) TestMaintainedApps() { UninstallScript: "echo $FLEET_SECRET_INVALID3", } respBadSecret := s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) - errName, errMsg := extractServerErrorNameReason(respBadSecret.Body) - assert.Equal(t, "install script", errName) - assert.Contains(t, errMsg, "$FLEET_SECRET_INVALID1") + errNames, errReasons := extractServerErrorNameReasons(respBadSecret.Body) + assert.ElementsMatch(t, []string{"install script", "post-install script", "uninstall script"}, errNames) + assert.Len(t, errReasons, 3) + for _, reason := range errReasons { + assert.Contains(t, reason, "$FLEET_SECRET_INVALID") + } reqInvalidSecret = &addFleetMaintainedAppRequest{ AppID: 1, @@ -15957,12 +15960,13 @@ func (s *integrationEnterpriseTestSuite) TestMaintainedApps() { PreInstallQuery: "SELECT 1", InstallScript: "echo foo", PostInstallScript: "echo done $FLEET_SECRET_INVALID2", - UninstallScript: "echo $FLEET_SECRET_INVALID3", + UninstallScript: "echo", } respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) - errName, errMsg = extractServerErrorNameReason(respBadSecret.Body) - assert.Equal(t, "post-install script", errName) - assert.Contains(t, errMsg, "$FLEET_SECRET_INVALID2") + errNames, errReasons = extractServerErrorNameReasons(respBadSecret.Body) + assert.ElementsMatch(t, []string{"post-install script"}, errNames) + require.Len(t, errReasons, 1) + assert.Contains(t, errReasons[0], "$FLEET_SECRET_INVALID2") reqInvalidSecret = &addFleetMaintainedAppRequest{ AppID: 1, @@ -15974,9 +15978,10 @@ func (s *integrationEnterpriseTestSuite) TestMaintainedApps() { UninstallScript: "echo $FLEET_SECRET_INVALID3", } respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) - errName, errMsg = extractServerErrorNameReason(respBadSecret.Body) - assert.Equal(t, "uninstall script", errName) - assert.Contains(t, errMsg, "$FLEET_SECRET_INVALID3") + errNames, errReasons = extractServerErrorNameReasons(respBadSecret.Body) + assert.ElementsMatch(t, []string{"uninstall script"}, errNames) + require.Len(t, errReasons, 1) + assert.Contains(t, errReasons[0], "$FLEET_SECRET_INVALID3") // Add an ingested app to the team var addMAResp addFleetMaintainedAppResponse