From ce513bb9daa6a27909d03cc3ac62c6fbce4bb3f8 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Wed, 22 Mar 2023 12:44:13 +0100 Subject: [PATCH 1/2] fix: enforce hash format and fix regression Signed-off-by: Miguel Martinez Trivino --- app/cli/internal/action/artifact_download.go | 6 ++--- internal/casclient/downloader.go | 17 +++++++++++---- internal/casclient/uploader.go | 23 ++++++++++++++------ internal/casclient/uploader_test.go | 21 +++++++++++++++--- 4 files changed, 50 insertions(+), 17 deletions(-) diff --git a/app/cli/internal/action/artifact_download.go b/app/cli/internal/action/artifact_download.go index 1230bbd38..389bedb9c 100644 --- a/app/cli/internal/action/artifact_download.go +++ b/app/cli/internal/action/artifact_download.go @@ -54,9 +54,9 @@ func (a *ArtifactDownload) Run(downloadPath, digest string) error { client := casclient.New(a.artifactsCASConn) ctx := context.Background() - info, err := client.Describe(ctx, h.Hex) + info, err := client.Describe(ctx, h.String()) if err != nil { - return fmt.Errorf("resource with digest %s not found", h) + return fmt.Errorf("artifact with digest %s not found", h) } if downloadPath == "" { @@ -84,7 +84,7 @@ func (a *ArtifactDownload) Run(downloadPath, digest string) error { go renderOperationStatus(ctx, client.ProgressStatus, info.Size) defer close(client.ProgressStatus) - err = client.Download(ctx, w, h.Hex) + err = client.Download(ctx, w, h.String()) if err != nil { a.Logger.Debug().Err(err).Msg("problem downloading file") return errors.New("problem downloading file") diff --git a/internal/casclient/downloader.go b/internal/casclient/downloader.go index 33944b3fb..1e8648046 100644 --- a/internal/casclient/downloader.go +++ b/internal/casclient/downloader.go @@ -22,20 +22,23 @@ import ( "io" v1 "github.com/chainloop-dev/chainloop/app/artifact-cas/api/cas/v1" + cr_v1 "github.com/google/go-containerregistry/pkg/v1" "google.golang.org/genproto/googleapis/bytestream" ) // Download downloads a file from the CAS and writes it to the provided writer func (c *Client) Download(ctx context.Context, w io.Writer, digest string) error { - if digest == "" { - return errors.New("a digest is required") + // Check digest format, including the algorithm and the hex portion + h, err := cr_v1.NewHash(digest) + if err != nil { + return fmt.Errorf("decoding digest: %w", err) } ctx, cancel := context.WithCancel(ctx) defer cancel() // Open the stream to start reading chunks - reader, err := bytestream.NewByteStreamClient(c.conn).Read(ctx, &bytestream.ReadRequest{ResourceName: digest}) + reader, err := bytestream.NewByteStreamClient(c.conn).Read(ctx, &bytestream.ReadRequest{ResourceName: h.Hex}) if err != nil { return fmt.Errorf("creating the gRPC client: %w", err) } @@ -76,8 +79,14 @@ func (c *Client) Download(ctx context.Context, w io.Writer, digest string) error // Describe returns the metadata of a resource by its digest // We use this to get the filename and the total size of the artifact func (c *Client) Describe(ctx context.Context, digest string) (*ResourceInfo, error) { + // Check digest format, including the algorithm and the hex portion + h, err := cr_v1.NewHash(digest) + if err != nil { + return nil, fmt.Errorf("decoding digest: %w", err) + } + client := v1.NewResourceServiceClient(c.conn) - resp, err := client.Describe(ctx, &v1.ResourceServiceDescribeRequest{Digest: digest}) + resp, err := client.Describe(ctx, &v1.ResourceServiceDescribeRequest{Digest: h.Hex}) if err != nil { return nil, fmt.Errorf("contacting API to get resource Info: %w", err) } diff --git a/internal/casclient/uploader.go b/internal/casclient/uploader.go index 4c3216cde..9082d33ac 100644 --- a/internal/casclient/uploader.go +++ b/internal/casclient/uploader.go @@ -56,19 +56,25 @@ func (c *Client) UploadFile(ctx context.Context, filepath string) (*UpDownStatus return nil, fmt.Errorf("rewinding file pointer: %w", err) } - return c.Upload(ctx, f, path.Base(filepath), hash.Hex) + return c.Upload(ctx, f, path.Base(filepath), hash.String()) } func (c *Client) Upload(ctx context.Context, r io.Reader, filename, digest string) (*UpDownStatus, error) { + // Check digest format, including the algorithm and the hex portion + h, err := cr_v1.NewHash(digest) + if err != nil { + return nil, fmt.Errorf("decoding digest: %w", err) + } + ctx, cancel := context.WithCancel(ctx) defer cancel() - resource, err := encodeResource(filename, digest) + resource, err := encodeResource(filename, h.Hex) if err != nil { return nil, fmt.Errorf("encoding resource name: %w", err) } - c.logger.Info().Msgf("uploading %s - sha256:%s", filename, digest) + c.logger.Info().Msgf("uploading %s - %s", filename, h) stream, err := bytestream.NewByteStreamClient(c.conn).Write(ctx) if err != nil { @@ -127,7 +133,7 @@ doUpload: latestStatus = &UpDownStatus{ Filename: filename, - Digest: digest, + Digest: h.String(), ProcessedBytes: totalUploaded, } @@ -156,13 +162,16 @@ func encodeResource(fileName, digest string) (string, error) { return "", fmt.Errorf("file name is empty") } - if digest == "" { - return "", fmt.Errorf("digest is empty") + // Check digest format, including the algorithm and the hex portion + h, err := cr_v1.NewHash(digest) + if err != nil { + return "", fmt.Errorf("decoding digest: %w", err) } var encodedResource bytes.Buffer enc := gob.NewEncoder(&encodedResource) - r := &v1.CASResource{FileName: fileName, Digest: digest} + // Currently we only support SHA256 + r := &v1.CASResource{FileName: fileName, Digest: h.Hex} if err := enc.Encode(r); err != nil { return "", err diff --git a/internal/casclient/uploader_test.go b/internal/casclient/uploader_test.go index 8048bfef1..25dad6488 100644 --- a/internal/casclient/uploader_test.go +++ b/internal/casclient/uploader_test.go @@ -19,6 +19,7 @@ import ( "bytes" "encoding/base64" "encoding/gob" + "fmt" "testing" v1 "github.com/chainloop-dev/chainloop/app/artifact-cas/api/cas/v1" @@ -26,6 +27,8 @@ import ( ) func TestEncodeResource(t *testing.T) { + const validDigestHex = "b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c" + testCases := []struct { name string fileName string @@ -44,10 +47,22 @@ func TestEncodeResource(t *testing.T) { wantError: true, }, { - name: "valid fields", - digest: "deadbeef", + name: "uncompleted digest", + digest: "deadbeef", + fileName: "foo.txt", + wantError: true, + }, + { + name: "invalid digest", + digest: "sha256:deadbeef", + fileName: "foo.txt", + wantError: true, + }, + { + name: "valid", + digest: fmt.Sprintf("sha256:%s", validDigestHex), fileName: "foo.txt", - want: &v1.CASResource{FileName: "foo.txt", Digest: "deadbeef"}, + want: &v1.CASResource{FileName: "foo.txt", Digest: validDigestHex}, }, } From 32cc46f4e86155ac5214c5e768bf184faf0d0756 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Wed, 22 Mar 2023 12:53:07 +0100 Subject: [PATCH 2/2] fix: enforce hash format and fix regression Signed-off-by: Miguel Martinez Trivino --- internal/casclient/uploader.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/casclient/uploader.go b/internal/casclient/uploader.go index 9082d33ac..bc3fd9db2 100644 --- a/internal/casclient/uploader.go +++ b/internal/casclient/uploader.go @@ -69,7 +69,7 @@ func (c *Client) Upload(ctx context.Context, r io.Reader, filename, digest strin ctx, cancel := context.WithCancel(ctx) defer cancel() - resource, err := encodeResource(filename, h.Hex) + resource, err := encodeResource(filename, h.String()) if err != nil { return nil, fmt.Errorf("encoding resource name: %w", err) }