From a154f4ca0429e968e78b03aef4666d6a40408a9a Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Wed, 12 Jun 2024 09:33:25 -0400 Subject: [PATCH] Improve handling of timeouts and maximum size for the upload software installer endpoint (#19657) --- ...improve-software-installer-upload-endpoint | 1 + cmd/fleet/serve.go | 18 ++++++++++++++++- server/service/software_installers.go | 20 +++++++++++++++---- 3 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 changes/19453-improve-software-installer-upload-endpoint diff --git a/changes/19453-improve-software-installer-upload-endpoint b/changes/19453-improve-software-installer-upload-endpoint new file mode 100644 index 0000000000..c4402ca71f --- /dev/null +++ b/changes/19453-improve-software-installer-upload-endpoint @@ -0,0 +1 @@ +* Extended the timeout for the endpoint to upload a software installer (`POST /fleet/software/package`), and improved handling of the maximum size. diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index 90721bf545..c669a43199 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -1027,7 +1027,7 @@ the way that the Fleet server works. } } - // We must wrap the Handler here to set special per-endpoint Write + // We must wrap the Handler here to set special per-endpoint Read/Write // timeouts, so that we have access to the raw http.ResponseWriter. // Otherwise, the handler is wrapped by the promhttp response delegator, // which does not support the Unwrap call needed to work with @@ -1038,6 +1038,9 @@ the way that the Fleet server works. // does not implement. rootMux.HandleFunc("/api/", func(rw http.ResponseWriter, req *http.Request) { if req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/fleet/scripts/run/sync") { + // when running a script synchronously, we wait a while for a script + // execution result, so the write timeout (to write the response) + // must be extended. rc := http.NewResponseController(rw) // add an additional 30 seconds to prevent race conditions where the // request is terminated early. @@ -1045,6 +1048,19 @@ the way that the Fleet server works. level.Error(logger).Log("msg", "http middleware failed to override endpoint write timeout", "err", err) } } + + if req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/fleet/software/package") { + // when uploading a software installer, the file might be large so + // the read timeout (to read the full request body) must be extended. + rc := http.NewResponseController(rw) + // the frontend times out waiting for the upload after 2 minutes, so + // use that same timeout: + // https://www.figma.com/design/oQl2oQUG0iRkUy0YOxc307/%2314921-Deploy-security-agents-to-macOS%2C-Windows%2C-and-Linux-hosts?node-id=773-18032&t=QjEU6tc73tddNSqn-0 + if err := rc.SetReadDeadline(time.Now().Add(2 * time.Minute)); err != nil { + level.Error(logger).Log("msg", "http middleware failed to override endpoint read timeout", "err", err) + } + req.Body = http.MaxBytesReader(rw, req.Body, service.MaxSoftwareInstallerSize) + } apiHandler.ServeHTTP(rw, req) }) rootMux.Handle("/", frontendHandler) diff --git a/server/service/software_installers.go b/server/service/software_installers.go index f03f05d0b7..d22100a4ef 100644 --- a/server/service/software_installers.go +++ b/server/service/software_installers.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "io" "mime/multipart" @@ -29,14 +30,25 @@ type uploadSoftwareInstallerResponse struct { Err error `json:"error,omitempty"` } +// MaxSoftwareInstallerSize is the maximum size allowed for software +// installers. This is enforced by the endpoint that uploads installers. +const MaxSoftwareInstallerSize = 500 * units.MiB + // TODO: We parse the whole body before running svc.authz.Authorize. // An authenticated but unauthorized user could abuse this. func (uploadSoftwareInstallerRequest) DecodeRequest(ctx context.Context, r *http.Request) (interface{}, error) { decoded := uploadSoftwareInstallerRequest{} err := r.ParseMultipartForm(512 * units.MiB) if err != nil { + var mbe *http.MaxBytesError + if errors.As(err, &mbe) { + return nil, &fleet.BadRequestError{ + Message: "The maximum file size is 500 MB.", + InternalErr: err, + } + } return nil, &fleet.BadRequestError{ - Message: "failed to parse multipart form", + Message: "failed to parse multipart form: " + err.Error(), InternalErr: err, } } @@ -49,9 +61,9 @@ func (uploadSoftwareInstallerRequest) DecodeRequest(ctx context.Context, r *http } decoded.File = r.MultipartForm.File["software"][0] - - if decoded.File.Size > 500*units.MiB { - // TODO: Should we try to assess the size earlier in the request processing (before parsing the form)? + if decoded.File.Size > MaxSoftwareInstallerSize { + // Should never happen here since the request's body is limited to the + // maximum size. return nil, &fleet.BadRequestError{ Message: "The maximum file size is 500 MB.", }