Reject unsupported qualifiers in Remote Asset API

server/BUILD.bazel

@@ -31,6 +31,7 @@ go_library(
         "@com_github_mostynb_zstdpool_syncpool//:go_default_library",
         "@org_golang_google_genproto_googleapis_bytestream//:go_default_library",
         "@org_golang_google_genproto_googleapis_rpc//code:go_default_library",
+        "@org_golang_google_genproto_googleapis_rpc//errdetails:go_default_library",
         "@org_golang_google_genproto_googleapis_rpc//status:go_default_library",
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//codes:go_default_library",
@@ -64,6 +65,7 @@ go_test(
         "@com_github_google_uuid//:go_default_library",
         "@com_github_klauspost_compress//zstd:go_default_library",
         "@org_golang_google_genproto_googleapis_bytestream//:go_default_library",
+        "@org_golang_google_genproto_googleapis_rpc//errdetails:go_default_library",
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//codes:go_default_library",
         "@org_golang_google_grpc//credentials/insecure:go_default_library",

server/grpc_asset.go

@@ -6,11 +6,13 @@ import (
 	"crypto/sha256"
 	"encoding/base64"
 	"encoding/hex"
+	"fmt"
 	"io"
 	"net/http"
 	"net/url"
 	"strings"
 
+	"google.golang.org/genproto/googleapis/rpc/errdetails"
 	"google.golang.org/genproto/googleapis/rpc/status"
 	"google.golang.org/grpc/codes"
 	grpc_status "google.golang.org/grpc/status"
@@ -23,11 +25,16 @@ import (
 
 // FetchServer implementation
 
-var errNilFetchBlobRequest = grpc_status.Error(codes.InvalidArgument,
-	"expected a non-nil *FetchBlobRequest")
+var (
+	errNilFetchBlobRequest = grpc_status.Error(codes.InvalidArgument,
+		"expected a non-nil *FetchBlobRequest")
+	errNilQualifier = grpc_status.Error(codes.InvalidArgument,
+		"expected a non-nil *Qualifier")
+	errUnsupportedDigestFunction = grpc_status.Error(codes.InvalidArgument,
+		"unsupported digest function")
+)
 
 func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest) (*asset.FetchBlobResponse, error) {
-
 	var sha256Str string
 
 	// Q: which combinations of qualifiers to support?
@@ -58,14 +65,10 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest)
 
 	headers := http.Header{}
 
+	var unsupportedQualifierNames []string
 	for _, q := range req.GetQualifiers() {
 		if q == nil {
-			return &asset.FetchBlobResponse{
-				Status: &status.Status{
-					Code:    int32(codes.InvalidArgument),
-					Message: "unexpected nil qualifier in FetchBlobRequest",
-				},
-			}, nil
+			return nil, errNilQualifier
 		}
 
 		const QualifierHTTPHeaderPrefix = "http_header:"
@@ -76,25 +79,33 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest)
 			continue
 		}
 
-		if q.Name == "checksum.sri" && strings.HasPrefix(q.Value, "sha256-") {
+		if q.Name == "checksum.sri" {
 			// Ref: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity
 
-			b64hash := strings.TrimPrefix(q.Value, "sha256-")
+			b64hash, ok := strings.CutPrefix(q.Value, "sha256-")
+			if !ok {
+				return nil, grpc_status.Error(codes.InvalidArgument, fmt.Sprintf(`unsupported digest function in "checksum.sri" qualifier %q`, q.Value))
+			}
 
 			decoded, err := base64.StdEncoding.DecodeString(b64hash)
 			if err != nil {
 				s.errorLogger.Printf("failed to base64 decode \"%s\": %v",
 					b64hash, err)
-				continue
+				return nil, grpc_status.Error(codes.InvalidArgument, fmt.Errorf(`invalid sri in "checksum.sri" qualifier for %q: base64 decode:  %w`, q.Value, err).Error())
 			}
 
 			sha256Str = hex.EncodeToString(decoded)
+			continue
+		}
 
-			found, size := s.cache.Contains(ctx, cache.CAS, sha256Str, -1)
-			if !found {
-				continue
-			}
+		unsupportedQualifierNames = append(unsupportedQualifierNames, q.Name)
+	}
+	if len(unsupportedQualifierNames) > 0 {
+		return nil, s.unsupportedQualifiersErrStatus(unsupportedQualifierNames)
+	}
 
+	if len(sha256Str) != 0 {
+		if found, size := s.cache.Contains(ctx, cache.CAS, sha256Str, -1); found {
 			if size < 0 {
 				// We don't know the size yet (bad http backend?).
 				r, actualSize, err := s.cache.Get(ctx, cache.CAS, sha256Str, -1, 0)
@@ -104,7 +115,8 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest)
 				if err != nil || actualSize < 0 {
 					s.errorLogger.Printf("failed to get CAS %s from proxy backend size: %d err: %v",
 						sha256Str, actualSize, err)
-					continue
+					return nil, grpc_status.Error(codes.Internal, fmt.Sprintf("failed to get CAS %s from proxy backend size: %d err: %v",
+						sha256Str, actualSize, err))
 				}
 				size = actualSize
 			}
@@ -210,6 +222,25 @@ func (s *grpcServer) fetchItem(ctx context.Context, uri string, headers http.Hea
 	return true, expectedHash, expectedSize
 }
 
+// unsupportedQualifiersErrStatus creates a gRPC status error that includes a list of unsupported qualifiers.
+func (s *grpcServer) unsupportedQualifiersErrStatus(qualifierNames []string) error {
+	fieldViolations := make([]*errdetails.BadRequest_FieldViolation, 0, len(qualifierNames))
+	for _, name := range qualifierNames {
+		fieldViolations = append(fieldViolations, &errdetails.BadRequest_FieldViolation{
+			Field:       "qualifiers.name",
+			Description: fmt.Sprintf("%q not supported", name),
+		})
+	}
+	statusWithoutDetails := grpc_status.New(codes.InvalidArgument, fmt.Sprintf("Unsupported qualifiers: %s", strings.Join(qualifierNames, ", ")))
+	statusWithDetails, err := statusWithoutDetails.WithDetails(&errdetails.BadRequest{FieldViolations: fieldViolations})
+	// should never happen
+	if err != nil {
+		s.errorLogger.Printf("failed to add details to status: %v", err)
+		return statusWithoutDetails.Err()
+	}
+	return statusWithDetails.Err()
+}
+
 func (s *grpcServer) FetchDirectory(context.Context, *asset.FetchDirectoryRequest) (*asset.FetchDirectoryResponse, error) {
 	return nil, nil
 }

server/grpc_asset_test.go

@@ -11,9 +11,11 @@ import (
 	"testing"
 
 	asset "github.com/buchgr/bazel-remote/v2/genproto/build/bazel/remote/asset/v1"
-	//pb "github.com/buchgr/bazel-remote/v2/genproto/build/bazel/remote/execution/v2"
 
+	"google.golang.org/genproto/googleapis/rpc/errdetails"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
 
 	testutils "github.com/buchgr/bazel-remote/v2/utils"
 )
@@ -63,6 +65,86 @@ func TestAssetFetchBlob(t *testing.T) {
 	}
 }
 
+func TestAssetMismatchingSRIAlgorithm(t *testing.T) {
+	t.Parallel()
+
+	fixture := grpcTestSetup(t)
+	defer os.Remove(fixture.tempdir)
+
+	ts := newTestGetServer()
+
+	req := asset.FetchBlobRequest{
+		Uris: []string{
+			ts.srv.URL + "/" + ts.path, // This URL should work.
+		},
+		Qualifiers: []*asset.Qualifier{
+			{
+				Name: "checksum.sri",
+				// This is a mismatching algorithm
+				// and also a mismatching hash.
+				// This should cause an error.
+				Value: "sha512-ieYjnbXfruIhY0rGSc4H5uYoEFP42Bj6jtnVK0dlzORoEOE0nJxDBRcjJdN9KHIQkB1y4UYdvHKe1u8/ELU+Ow==",
+			},
+		},
+	}
+
+	_, err := fixture.assetClient.FetchBlob(ctx, &req)
+	if err == nil {
+		t.Fatal("expected rpc error from fetch")
+	}
+}
+
+func TestAssetUnsupportedQualifier(t *testing.T) {
+	t.Parallel()
+
+	fixture := grpcTestSetup(t)
+	defer os.Remove(fixture.tempdir)
+
+	ts := newTestGetServer()
+
+	req := asset.FetchBlobRequest{
+		Uris: []string{
+			ts.srv.URL + "/" + ts.path, // This URL should work.
+		},
+		Qualifiers: []*asset.Qualifier{
+			{
+				Name:  "unknown-qualifier",
+				Value: "some-value",
+			},
+		},
+	}
+
+	_, err := fixture.assetClient.FetchBlob(ctx, &req)
+	if err == nil {
+		t.Fatal(err, "expected rpc error from fetch")
+	}
+	gstatus, ok := status.FromError(err)
+	if !ok {
+		t.Fatal("expected a status in rpc error")
+	}
+	if gstatus.Code() != codes.InvalidArgument {
+		t.Fatalf("expected %v status code, got %v", codes.InvalidArgument, gstatus.Code())
+	}
+	if len(gstatus.Details()) != 1 {
+		t.Fatal("expected one detail in rpc status")
+	}
+	expectedDetail := &errdetails.BadRequest{
+		FieldViolations: []*errdetails.BadRequest_FieldViolation{
+			{
+				Field:       "qualifiers.name",
+				Description: `"unknown-qualifier" not supported`,
+			},
+		},
+	}
+	protoDetail, ok := gstatus.Details()[0].(*errdetails.BadRequest)
+	if !ok {
+		t.Fatal("expected BadRequest detail in rpc status")
+	}
+	if !proto.Equal(protoDetail, expectedDetail) {
+		t.Fatalf("expected %v BadRequest, got %v", expectedDetail, protoDetail)
+	}
+}
+
 type testGetServer struct {
 	srv *httptest.Server