diff --git a/server/BUILD.bazel b/server/BUILD.bazel index 1286be262..bf8cbd1a0 100644 --- a/server/BUILD.bazel +++ b/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", diff --git a/server/grpc_asset.go b/server/grpc_asset.go index 79b283ecd..85ee9244c 100644 --- a/server/grpc_asset.go +++ b/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 } diff --git a/server/grpc_asset_test.go b/server/grpc_asset_test.go index f53be9801..03a24ca42 100644 --- a/server/grpc_asset_test.go +++ b/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