diff --git a/ee/server/service/maintained_apps.go b/ee/server/service/maintained_apps.go index f459d098cb13..f195e99158ba 100644 --- a/ee/server/service/maintained_apps.go +++ b/ee/server/service/maintained_apps.go @@ -44,7 +44,17 @@ 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. + 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") } 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..81b539c5b1f4 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -96,7 +96,17 @@ 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. + 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") } installerID, titleID, err := svc.ds.MatchOrCreateSoftwareInstaller(ctx, payload) @@ -228,7 +238,17 @@ 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. + 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") } // get software by ID, fail if it does not exist or does not have an existing installer @@ -463,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 7bea24d99a44..88f8a305f473 100644 --- a/server/service/base_client_errors.go +++ b/server/service/base_client_errors.go @@ -104,17 +104,40 @@ 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 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 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..76acce2cebd2 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,42 @@ 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") + 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, + TeamID: &team.ID, + SelfService: true, + PreInstallQuery: "SELECT 1", + InstallScript: "echo foo", + PostInstallScript: "echo done $FLEET_SECRET_INVALID2", + UninstallScript: "echo", + } + respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity) + 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, + 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) + 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 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) + } } }