From 895e8b20b33fdfd3375e82980f52c97a7518bb18 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Sat, 20 Jul 2024 14:39:24 +0200 Subject: [PATCH 01/11] Fix artifact v4 upload above 8MB --- routers/api/actions/artifactsv4.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index e78ed7a0c2..0f4d768af1 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -271,6 +271,8 @@ func (r *artifactV4Routes) createArtifact(ctx *ArtifactContext) { return } artifact.ContentEncoding = ArtifactV4ContentEncoding + artifact.FileSize = 0 + artifact.FileCompressedSize = 0 if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { log.Error("Error UpdateArtifactByID: %v", err) ctx.Error(http.StatusInternalServerError, "Error UpdateArtifactByID") @@ -301,11 +303,6 @@ func (r *artifactV4Routes) uploadArtifact(ctx *ArtifactContext) { return } - if comp == "block" { - artifact.FileSize = 0 - artifact.FileCompressedSize = 0 - } - _, err = appendUploadChunk(r.fs, ctx, artifact, artifact.FileSize, ctx.Req.ContentLength, artifact.RunID) if err != nil { log.Error("Error runner api getting task: task is not running") From a6fb8a5b266462e73b1a5b8d4f9033b3dd593845 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Sat, 20 Jul 2024 21:13:30 +0200 Subject: [PATCH 02/11] Basic blockList Support for artifacts v4 --- routers/api/actions/artifacts_chunks.go | 45 ++++++++- routers/api/actions/artifactsv4.go | 119 ++++++++++++++++++------ 2 files changed, 135 insertions(+), 29 deletions(-) diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index 3d1a3891d9..d07ab196c9 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -123,6 +123,49 @@ func listChunksByRunID(st storage.ObjectStorage, runID int64) (map[int64][]*chun return chunksMap, nil } +func listChunksByRunIDV4(st storage.ObjectStorage, runID int64, artifactID int64, blist *BlockList) ([]*chunkFileItem, error) { + storageDir := fmt.Sprintf("tmp%d", runID) + var chunks []*chunkFileItem + chunkMap := map[string]*chunkFileItem{} + dummy := &chunkFileItem{} + for _, name := range blist.Latest { + chunkMap[name] = dummy + } + if err := st.IterateObjects(storageDir, func(fpath string, obj storage.Object) error { + baseName := filepath.Base(fpath) + if !strings.HasPrefix(baseName, "block-") { + return nil + } + // when read chunks from storage, it only contains storage dir and basename, + // no matter the subdirectory setting in storage config + item := chunkFileItem{Path: storageDir + "/" + baseName, ArtifactID: artifactID} + var size int64 + var chunkName string + if _, err := fmt.Sscanf(baseName, "block-%d-%d-%s", &item.RunID, &size, &chunkName); err != nil { + return fmt.Errorf("parse content range error: %v", err) + } + item.End = item.Start + size - 1 + if _, ok := chunkMap[chunkName]; ok { + chunkMap[chunkName] = &item + } + return nil + }); err != nil { + return nil, err + } + for i, name := range blist.Latest { + chunk, ok := chunkMap[name] + if !ok || chunk.Path == "" { + return nil, fmt.Errorf("missing Chunk (%d/%d): %s", i, len(blist.Latest), name) + } + chunks = append(chunks, chunk) + if i > 0 { + chunk.Start = chunkMap[blist.Latest[i-1]].End + 1 + chunk.End += chunk.Start + } + } + return chunks, nil +} + func mergeChunksForRun(ctx *ArtifactContext, st storage.ObjectStorage, runID int64, artifactName string) error { // read all db artifacts by name artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{ @@ -230,7 +273,7 @@ func mergeChunksForArtifact(ctx *ArtifactContext, chunks []*chunkFileItem, st st rawChecksum := hash.Sum(nil) actualChecksum := hex.EncodeToString(rawChecksum) if !strings.HasSuffix(checksum, actualChecksum) { - return fmt.Errorf("update artifact error checksum is invalid") + return fmt.Errorf("update artifact error checksum is invalid %v vs %v", checksum, actualChecksum) } } diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 0f4d768af1..7e9ac9750d 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -24,8 +24,15 @@ package actions // PUT: http://localhost:3000/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact?sig=mO7y35r4GyjN7fwg0DTv3-Fv1NDXD84KLEgLpoPOtDI=&expires=2024-01-23+21%3A48%3A37.20833956+%2B0100+CET&artifactName=test&taskID=75&comp=block // 1.3. Continue Upload Zip Content to Blobstorage (unauthenticated request), repeat until everything is uploaded // PUT: http://localhost:3000/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact?sig=mO7y35r4GyjN7fwg0DTv3-Fv1NDXD84KLEgLpoPOtDI=&expires=2024-01-23+21%3A48%3A37.20833956+%2B0100+CET&artifactName=test&taskID=75&comp=appendBlock -// 1.4. Unknown xml payload to Blobstorage (unauthenticated request), ignored for now +// 1.4. BlockList xml payload to Blobstorage (unauthenticated request) +// Files of about 800MB are parallel in parallel and / or out of order, this file is needed to enshure the correct order // PUT: http://localhost:3000/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact?sig=mO7y35r4GyjN7fwg0DTv3-Fv1NDXD84KLEgLpoPOtDI=&expires=2024-01-23+21%3A48%3A37.20833956+%2B0100+CET&artifactName=test&taskID=75&comp=blockList +// Request +// +// +// blockId1 +// blockId2 +// // 1.5. FinalizeArtifact // Post: /twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact // Request @@ -82,6 +89,7 @@ import ( "crypto/hmac" "crypto/sha256" "encoding/base64" + "encoding/xml" "fmt" "io" "net/http" @@ -295,33 +303,57 @@ func (r *artifactV4Routes) uploadArtifact(ctx *ArtifactContext) { comp := ctx.Req.URL.Query().Get("comp") switch comp { case "block", "appendBlock": - // get artifact by name - artifact, err := r.getArtifactByName(ctx, task.Job.RunID, artifactName) - if err != nil { - log.Error("Error artifact not found: %v", err) - ctx.Error(http.StatusNotFound, "Error artifact not found") - return - } + blockid := ctx.Req.URL.Query().Get("blockid") + if blockid == "" { + // get artifact by name + artifact, err := r.getArtifactByName(ctx, task.Job.RunID, artifactName) + if err != nil { + log.Error("Error artifact not found: %v", err) + ctx.Error(http.StatusNotFound, "Error artifact not found") + return + } - _, err = appendUploadChunk(r.fs, ctx, artifact, artifact.FileSize, ctx.Req.ContentLength, artifact.RunID) + _, err = appendUploadChunk(r.fs, ctx, artifact, artifact.FileSize, ctx.Req.ContentLength, artifact.RunID) + if err != nil { + log.Error("Error runner api getting task: task is not running") + ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") + return + } + artifact.FileCompressedSize += ctx.Req.ContentLength + artifact.FileSize += ctx.Req.ContentLength + if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { + log.Error("Error UpdateArtifactByID: %v", err) + ctx.Error(http.StatusInternalServerError, "Error UpdateArtifactByID") + return + } + } else { + _, err := r.fs.Save(fmt.Sprintf("tmp%d/block-%d-%d-%s", task.Job.RunID, task.Job.RunID, ctx.Req.ContentLength, blockid), ctx.Req.Body, -1) + if err != nil { + log.Error("Error runner api getting task: task is not running") + ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") + return + } + } + ctx.JSON(http.StatusCreated, "appended") + case "blocklist": + _, err := r.fs.Save(fmt.Sprintf("tmp%d/%d-blocklist", task.Job.RunID, task.Job.RunID), ctx.Req.Body, -1) if err != nil { log.Error("Error runner api getting task: task is not running") ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") return } - artifact.FileCompressedSize += ctx.Req.ContentLength - artifact.FileSize += ctx.Req.ContentLength - if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { - log.Error("Error UpdateArtifactByID: %v", err) - ctx.Error(http.StatusInternalServerError, "Error UpdateArtifactByID") - return - } - ctx.JSON(http.StatusCreated, "appended") - case "blocklist": ctx.JSON(http.StatusCreated, "created") } } +type BlockList struct { + Latest []string `xml:"Latest"` +} + +type Latest struct { + Value string `xml:",chardata"` +} + func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { var req FinalizeArtifactRequest @@ -340,18 +372,49 @@ func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { ctx.Error(http.StatusNotFound, "Error artifact not found") return } - chunkMap, err := listChunksByRunID(r.fs, runID) + + blockListName := fmt.Sprintf("tmp%d/%d-blocklist", runID, runID) + var chunks []*chunkFileItem + s, err := r.fs.Open(blockListName) if err != nil { - log.Error("Error merge chunks: %v", err) - ctx.Error(http.StatusInternalServerError, "Error merge chunks") - return - } - chunks, ok := chunkMap[artifact.ID] - if !ok { - log.Error("Error merge chunks") - ctx.Error(http.StatusInternalServerError, "Error merge chunks") - return + log.Warn("Error merge chunks: %v", err) + chunkMap, err := listChunksByRunID(r.fs, runID) + if err != nil { + log.Error("Error merge chunks: %v", err) + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + chunks, ok = chunkMap[artifact.ID] + if !ok { + log.Error("Error merge chunks") + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + } else { + err = r.fs.Delete(blockListName) + if err != nil { + log.Warn("Failed to delete blockList %s: %v", blockListName, err) + } + + xdec := xml.NewDecoder(s) + blockList := &BlockList{} + err = xdec.Decode(blockList) + if err != nil { + log.Error("Error merge chunks: %v", err) + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + chunks, err = listChunksByRunIDV4(r.fs, runID, artifact.ID, blockList) + + if err != nil { + log.Error("Error merge chunks: %v", err) + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + artifact.FileSize = chunks[len(chunks)-1].End + 1 + artifact.FileCompressedSize = chunks[len(chunks)-1].End + 1 } + checksum := "" if req.Hash != nil { checksum = req.Hash.Value From 1c641fe817a7c7a87dc0cb0fff7437f6bd7d78e0 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Sat, 20 Jul 2024 21:22:10 +0200 Subject: [PATCH 03/11] format code --- routers/api/actions/artifacts_chunks.go | 2 +- routers/api/actions/artifactsv4.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index d07ab196c9..23e4846b0d 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -123,7 +123,7 @@ func listChunksByRunID(st storage.ObjectStorage, runID int64) (map[int64][]*chun return chunksMap, nil } -func listChunksByRunIDV4(st storage.ObjectStorage, runID int64, artifactID int64, blist *BlockList) ([]*chunkFileItem, error) { +func listChunksByRunIDV4(st storage.ObjectStorage, runID, artifactID int64, blist *BlockList) ([]*chunkFileItem, error) { storageDir := fmt.Sprintf("tmp%d", runID) var chunks []*chunkFileItem chunkMap := map[string]*chunkFileItem{} diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 7e9ac9750d..3fabfcf274 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -405,7 +405,6 @@ func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { return } chunks, err = listChunksByRunIDV4(r.fs, runID, artifact.ID, blockList) - if err != nil { log.Error("Error merge chunks: %v", err) ctx.Error(http.StatusInternalServerError, "Error merge chunks") From 4cde6db95cac44aff51d8ab4cce2bf773a644a97 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Sat, 20 Jul 2024 21:40:20 +0200 Subject: [PATCH 04/11] encode blockid via base64 url encoding to avoid security problems --- routers/api/actions/artifacts_chunks.go | 9 +++++++-- routers/api/actions/artifactsv4.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index 23e4846b0d..1a43f079b1 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -140,10 +140,15 @@ func listChunksByRunIDV4(st storage.ObjectStorage, runID, artifactID int64, blis // no matter the subdirectory setting in storage config item := chunkFileItem{Path: storageDir + "/" + baseName, ArtifactID: artifactID} var size int64 - var chunkName string - if _, err := fmt.Sscanf(baseName, "block-%d-%d-%s", &item.RunID, &size, &chunkName); err != nil { + var b64chunkName string + if _, err := fmt.Sscanf(baseName, "block-%d-%d-%s", &item.RunID, &size, &b64chunkName); err != nil { return fmt.Errorf("parse content range error: %v", err) } + rchunkName, err := base64.URLEncoding.DecodeString(b64chunkName) + if err != nil { + return fmt.Errorf("failed to parse chunkName: %v", err) + } + chunkName := string(rchunkName) item.End = item.Start + size - 1 if _, ok := chunkMap[chunkName]; ok { chunkMap[chunkName] = &item diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 3fabfcf274..502f9127b1 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -327,7 +327,7 @@ func (r *artifactV4Routes) uploadArtifact(ctx *ArtifactContext) { return } } else { - _, err := r.fs.Save(fmt.Sprintf("tmp%d/block-%d-%d-%s", task.Job.RunID, task.Job.RunID, ctx.Req.ContentLength, blockid), ctx.Req.Body, -1) + _, err := r.fs.Save(fmt.Sprintf("tmp%d/block-%d-%d-%s", task.Job.RunID, task.Job.RunID, ctx.Req.ContentLength, base64.URLEncoding.EncodeToString([]byte(blockid))), ctx.Req.Body, -1) if err != nil { log.Error("Error runner api getting task: task is not running") ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") From 5a2a3958bb2185057ac9762d32b98a765cc1b849 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Sun, 21 Jul 2024 17:15:10 +0200 Subject: [PATCH 05/11] fixes and move new v4 file naming to custom tmpv4 folder --- routers/api/actions/artifacts_chunks.go | 2 +- routers/api/actions/artifactsv4.go | 56 ++++++++++++++----------- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index 1a43f079b1..cf48da12aa 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -124,7 +124,7 @@ func listChunksByRunID(st storage.ObjectStorage, runID int64) (map[int64][]*chun } func listChunksByRunIDV4(st storage.ObjectStorage, runID, artifactID int64, blist *BlockList) ([]*chunkFileItem, error) { - storageDir := fmt.Sprintf("tmp%d", runID) + storageDir := fmt.Sprintf("tmpv4%d", runID) var chunks []*chunkFileItem chunkMap := map[string]*chunkFileItem{} dummy := &chunkFileItem{} diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 502f9127b1..6161f7a64c 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -160,31 +160,34 @@ func ArtifactsV4Routes(prefix string) *web.Router { return m } -func (r artifactV4Routes) buildSignature(endp, expires, artifactName string, taskID int64) []byte { +func (r artifactV4Routes) buildSignature(endp, expires, artifactName string, taskID, artifactID int64) []byte { mac := hmac.New(sha256.New, setting.GetGeneralTokenSigningSecret()) mac.Write([]byte(endp)) mac.Write([]byte(expires)) mac.Write([]byte(artifactName)) mac.Write([]byte(fmt.Sprint(taskID))) + mac.Write([]byte(fmt.Sprint(artifactID))) return mac.Sum(nil) } -func (r artifactV4Routes) buildArtifactURL(ctx *ArtifactContext, endp, artifactName string, taskID int64) string { +func (r artifactV4Routes) buildArtifactURL(ctx *ArtifactContext, endp, artifactName string, taskID, artifactID int64) string { expires := time.Now().Add(60 * time.Minute).Format("2006-01-02 15:04:05.999999999 -0700 MST") uploadURL := strings.TrimSuffix(httplib.GuessCurrentAppURL(ctx), "/") + strings.TrimSuffix(r.prefix, "/") + - "/" + endp + "?sig=" + base64.URLEncoding.EncodeToString(r.buildSignature(endp, expires, artifactName, taskID)) + "&expires=" + url.QueryEscape(expires) + "&artifactName=" + url.QueryEscape(artifactName) + "&taskID=" + fmt.Sprint(taskID) + "/" + endp + "?sig=" + base64.URLEncoding.EncodeToString(r.buildSignature(endp, expires, artifactName, taskID, artifactID)) + "&expires=" + url.QueryEscape(expires) + "&artifactName=" + url.QueryEscape(artifactName) + "&taskID=" + fmt.Sprint(taskID) + "&artifactID=" + fmt.Sprint(artifactID) return uploadURL } func (r artifactV4Routes) verifySignature(ctx *ArtifactContext, endp string) (*actions.ActionTask, string, bool) { rawTaskID := ctx.Req.URL.Query().Get("taskID") + rawArtifactID := ctx.Req.URL.Query().Get("artifactID") sig := ctx.Req.URL.Query().Get("sig") expires := ctx.Req.URL.Query().Get("expires") artifactName := ctx.Req.URL.Query().Get("artifactName") dsig, _ := base64.URLEncoding.DecodeString(sig) taskID, _ := strconv.ParseInt(rawTaskID, 10, 64) + artifactID, _ := strconv.ParseInt(rawArtifactID, 10, 64) - expecedsig := r.buildSignature(endp, expires, artifactName, taskID) + expecedsig := r.buildSignature(endp, expires, artifactName, taskID, artifactID) if !hmac.Equal(dsig, expecedsig) { log.Error("Error unauthorized") ctx.Error(http.StatusUnauthorized, "Error unauthorized") @@ -289,7 +292,7 @@ func (r *artifactV4Routes) createArtifact(ctx *ArtifactContext) { respData := CreateArtifactResponse{ Ok: true, - SignedUploadUrl: r.buildArtifactURL(ctx, "UploadArtifact", artifactName, ctx.ActionTask.ID), + SignedUploadUrl: r.buildArtifactURL(ctx, "UploadArtifact", artifactName, ctx.ActionTask.ID, artifact.ID), } r.sendProtbufBody(ctx, &respData) } @@ -327,7 +330,7 @@ func (r *artifactV4Routes) uploadArtifact(ctx *ArtifactContext) { return } } else { - _, err := r.fs.Save(fmt.Sprintf("tmp%d/block-%d-%d-%s", task.Job.RunID, task.Job.RunID, ctx.Req.ContentLength, base64.URLEncoding.EncodeToString([]byte(blockid))), ctx.Req.Body, -1) + _, err := r.fs.Save(fmt.Sprintf("tmpv4%d/block-%d-%d-%s", task.Job.RunID, task.Job.RunID, ctx.Req.ContentLength, base64.URLEncoding.EncodeToString([]byte(blockid))), ctx.Req.Body, -1) if err != nil { log.Error("Error runner api getting task: task is not running") ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") @@ -336,7 +339,9 @@ func (r *artifactV4Routes) uploadArtifact(ctx *ArtifactContext) { } ctx.JSON(http.StatusCreated, "appended") case "blocklist": - _, err := r.fs.Save(fmt.Sprintf("tmp%d/%d-blocklist", task.Job.RunID, task.Job.RunID), ctx.Req.Body, -1) + rawArtifactID := ctx.Req.URL.Query().Get("artifactID") + artifactID, _ := strconv.ParseInt(rawArtifactID, 10, 64) + _, err := r.fs.Save(fmt.Sprintf("tmpv4%d/%d-%d-blocklist", task.Job.RunID, task.Job.RunID, artifactID), ctx.Req.Body, -1) if err != nil { log.Error("Error runner api getting task: task is not running") ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") @@ -354,6 +359,23 @@ type Latest struct { Value string `xml:",chardata"` } +func (r *artifactV4Routes) readBlockList(runID, artifactID int64) (*BlockList, error) { + blockListName := fmt.Sprintf("tmpv4%d/%d-%d-blocklist", runID, runID, artifactID) + s, err := r.fs.Open(blockListName) + if err != nil { + return nil, err + } + err = r.fs.Delete(blockListName) + if err != nil { + log.Warn("Failed to delete blockList %s: %v", blockListName, err) + } + + xdec := xml.NewDecoder(s) + blockList := &BlockList{} + err = xdec.Decode(blockList) + return blockList, err +} + func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { var req FinalizeArtifactRequest @@ -373,11 +395,10 @@ func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { return } - blockListName := fmt.Sprintf("tmp%d/%d-blocklist", runID, runID) var chunks []*chunkFileItem - s, err := r.fs.Open(blockListName) + blockList, err := r.readBlockList(runID, artifact.ID) if err != nil { - log.Warn("Error merge chunks: %v", err) + log.Warn("Failed to read BlockList, fallback to old behavior: %v", err) chunkMap, err := listChunksByRunID(r.fs, runID) if err != nil { log.Error("Error merge chunks: %v", err) @@ -391,19 +412,6 @@ func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { return } } else { - err = r.fs.Delete(blockListName) - if err != nil { - log.Warn("Failed to delete blockList %s: %v", blockListName, err) - } - - xdec := xml.NewDecoder(s) - blockList := &BlockList{} - err = xdec.Decode(blockList) - if err != nil { - log.Error("Error merge chunks: %v", err) - ctx.Error(http.StatusInternalServerError, "Error merge chunks") - return - } chunks, err = listChunksByRunIDV4(r.fs, runID, artifact.ID, blockList) if err != nil { log.Error("Error merge chunks: %v", err) @@ -514,7 +522,7 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { } } if respData.SignedUrl == "" { - respData.SignedUrl = r.buildArtifactURL(ctx, "DownloadArtifact", artifactName, ctx.ActionTask.ID) + respData.SignedUrl = r.buildArtifactURL(ctx, "DownloadArtifact", artifactName, ctx.ActionTask.ID, artifact.ID) } r.sendProtbufBody(ctx, &respData) } From d9f55bce39f407db887b586591f36abb88d3abce Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 24 Jul 2024 12:14:21 +0200 Subject: [PATCH 06/11] add test to verify exploit failure --- .../api_actions_artifact_v4_test.go | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index f58f876849..40d096d9ff 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -7,12 +7,14 @@ import ( "bytes" "crypto/sha256" "encoding/hex" + "encoding/xml" "io" "net/http" "strings" "testing" "time" + "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/routers/api/actions" actions_service "code.gitea.io/gitea/services/actions" "code.gitea.io/gitea/tests" @@ -170,6 +172,69 @@ func TestActionsArtifactV4UploadSingleFileWithRetentionDays(t *testing.T) { assert.True(t, finalizeResp.Ok) } +func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + token, err := actions_service.CreateAuthorizationToken(48, 792, 193) + assert.NoError(t, err) + + // acquire artifact upload url + req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{ + Version: 4, + Name: "artifactWithPotentialHarmfulBlockID", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var uploadResp actions.CreateArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &uploadResp) + assert.True(t, uploadResp.Ok) + assert.Contains(t, uploadResp.SignedUploadUrl, "/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact") + + // get upload urls + idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/") + url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=%2f..%2fmyfile" + blockListUrl := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" + + // upload artifact chunk + body := strings.Repeat("A", 1024) + req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)) + MakeRequest(t, req, http.StatusCreated) + + // verify that the exploit didn't work + _, err = storage.Actions.Stat("myfile") + assert.Error(t, err) + + // upload artifact blockList + blockList := &actions.BlockList{ + Latest: []string{ + "/../myfile", + }, + } + rawBlockList, err := xml.Marshal(blockList) + assert.NoError(t, err) + req = NewRequestWithBody(t, "PUT", blockListUrl, bytes.NewReader(rawBlockList)) + MakeRequest(t, req, http.StatusCreated) + + t.Logf("Create artifact confirm") + + sha := sha256.Sum256([]byte(body)) + + // confirm artifact upload + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact", toProtoJSON(&actions.FinalizeArtifactRequest{ + Name: "artifactWithPotentialHarmfulBlockID", + Size: 1024, + Hash: wrapperspb.String("sha256:" + hex.EncodeToString(sha[:])), + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + var finalizeResp actions.FinalizeArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp) + assert.True(t, finalizeResp.Ok) +} + func TestActionsArtifactV4DownloadSingle(t *testing.T) { defer tests.PrepareTestEnv(t)() From 02479bf756fec621d05667c78458698951cb8327 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 24 Jul 2024 12:21:35 +0200 Subject: [PATCH 07/11] add TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder --- .../api_actions_artifact_v4_test.go | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index 40d096d9ff..d573d1b923 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -235,6 +235,71 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing assert.True(t, finalizeResp.Ok) } +func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + token, err := actions_service.CreateAuthorizationToken(48, 792, 193) + assert.NoError(t, err) + + // acquire artifact upload url + req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{ + Version: 4, + Name: "artifactWithChunksOutOfOrder", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var uploadResp actions.CreateArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &uploadResp) + assert.True(t, uploadResp.Ok) + assert.Contains(t, uploadResp.SignedUploadUrl, "/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact") + + // get upload urls + idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/") + block1Url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block1" + block2Url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block2" + blockListUrl := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" + + // upload artifact chunks + bodyb := strings.Repeat("B", 1024) + req = NewRequestWithBody(t, "PUT", block2Url, strings.NewReader(bodyb)) + MakeRequest(t, req, http.StatusCreated) + + bodya := strings.Repeat("A", 1024) + req = NewRequestWithBody(t, "PUT", block1Url, strings.NewReader(bodya)) + MakeRequest(t, req, http.StatusCreated) + + // upload artifact blockList + blockList := &actions.BlockList{ + Latest: []string{ + "block1", + "block2", + }, + } + rawBlockList, err := xml.Marshal(blockList) + assert.NoError(t, err) + req = NewRequestWithBody(t, "PUT", blockListUrl, bytes.NewReader(rawBlockList)) + MakeRequest(t, req, http.StatusCreated) + + t.Logf("Create artifact confirm") + + sha := sha256.Sum256([]byte(bodya + bodyb)) + + // confirm artifact upload + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact", toProtoJSON(&actions.FinalizeArtifactRequest{ + Name: "artifactWithChunksOutOfOrder", + Size: 2048, + Hash: wrapperspb.String("sha256:" + hex.EncodeToString(sha[:])), + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + var finalizeResp actions.FinalizeArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp) + assert.True(t, finalizeResp.Ok) +} + func TestActionsArtifactV4DownloadSingle(t *testing.T) { defer tests.PrepareTestEnv(t)() From add8925a27d6728c27a860c5c0e86a9a6bbed9e1 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 24 Jul 2024 12:30:00 +0200 Subject: [PATCH 08/11] update var name --- .../integration/api_actions_artifact_v4_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index d573d1b923..ec0fbbfa60 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -194,7 +194,7 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing // get upload urls idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/") url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=%2f..%2fmyfile" - blockListUrl := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" + blockListURL := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" // upload artifact chunk body := strings.Repeat("A", 1024) @@ -213,7 +213,7 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing } rawBlockList, err := xml.Marshal(blockList) assert.NoError(t, err) - req = NewRequestWithBody(t, "PUT", blockListUrl, bytes.NewReader(rawBlockList)) + req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList)) MakeRequest(t, req, http.StatusCreated) t.Logf("Create artifact confirm") @@ -256,17 +256,17 @@ func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) { // get upload urls idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/") - block1Url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block1" - block2Url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block2" - blockListUrl := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" + block1URL := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block1" + block2URL := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block2" + blockListURL := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" // upload artifact chunks bodyb := strings.Repeat("B", 1024) - req = NewRequestWithBody(t, "PUT", block2Url, strings.NewReader(bodyb)) + req = NewRequestWithBody(t, "PUT", block2URL, strings.NewReader(bodyb)) MakeRequest(t, req, http.StatusCreated) bodya := strings.Repeat("A", 1024) - req = NewRequestWithBody(t, "PUT", block1Url, strings.NewReader(bodya)) + req = NewRequestWithBody(t, "PUT", block1URL, strings.NewReader(bodya)) MakeRequest(t, req, http.StatusCreated) // upload artifact blockList @@ -278,7 +278,7 @@ func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) { } rawBlockList, err := xml.Marshal(blockList) assert.NoError(t, err) - req = NewRequestWithBody(t, "PUT", blockListUrl, bytes.NewReader(rawBlockList)) + req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList)) MakeRequest(t, req, http.StatusCreated) t.Logf("Create artifact confirm") From 06f3e93ccf55445fb3f546b77a89f9fbcead6526 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 24 Jul 2024 13:21:55 +0200 Subject: [PATCH 09/11] attempt to workaround minio and azureite storage availability problems via retries --- routers/api/actions/artifactsv4.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 6161f7a64c..a42aa3fa49 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -361,7 +361,15 @@ type Latest struct { func (r *artifactV4Routes) readBlockList(runID, artifactID int64) (*BlockList, error) { blockListName := fmt.Sprintf("tmpv4%d/%d-%d-blocklist", runID, runID, artifactID) - s, err := r.fs.Open(blockListName) + // Workaround minio and azureite storage availability problems via retries + var s storage.Object + var err error + for i := 0; i < 5; i++ { + if i != 0 { + time.Sleep(1000) + } + s, err = r.fs.Open(blockListName) + } if err != nil { return nil, err } From a96be7c090a6f86a1fb5677d38ddc487a9845814 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 24 Jul 2024 13:22:50 +0200 Subject: [PATCH 10/11] fix logic --- routers/api/actions/artifactsv4.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index a42aa3fa49..00c59288a1 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -369,6 +369,9 @@ func (r *artifactV4Routes) readBlockList(runID, artifactID int64) (*BlockList, e time.Sleep(1000) } s, err = r.fs.Open(blockListName) + if err == nil { + break + } } if err != nil { return nil, err From a7ef2389974f713a7b7fe7b580ca5f2e536e7dfb Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 24 Jul 2024 13:53:49 +0200 Subject: [PATCH 11/11] fix minio --- routers/api/actions/artifactsv4.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 00c59288a1..9e463cceeb 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -361,29 +361,19 @@ type Latest struct { func (r *artifactV4Routes) readBlockList(runID, artifactID int64) (*BlockList, error) { blockListName := fmt.Sprintf("tmpv4%d/%d-%d-blocklist", runID, runID, artifactID) - // Workaround minio and azureite storage availability problems via retries - var s storage.Object - var err error - for i := 0; i < 5; i++ { - if i != 0 { - time.Sleep(1000) - } - s, err = r.fs.Open(blockListName) - if err == nil { - break - } - } + s, err := r.fs.Open(blockListName) if err != nil { return nil, err } - err = r.fs.Delete(blockListName) - if err != nil { - log.Warn("Failed to delete blockList %s: %v", blockListName, err) - } xdec := xml.NewDecoder(s) blockList := &BlockList{} err = xdec.Decode(blockList) + + delerr := r.fs.Delete(blockListName) + if delerr != nil { + log.Warn("Failed to delete blockList %s: %v", blockListName, delerr) + } return blockList, err }