From 1e115ffafefc3c320fbb5baf5860f68dd2183d8c Mon Sep 17 00:00:00 2001 From: Malte Poll <1780588+malt3@users.noreply.github.com> Date: Fri, 7 Mar 2025 14:05:06 +0100 Subject: [PATCH 1/4] remote asset: return rpc errors when encountering invalid arguments See also: https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L123-L125 --- server/grpc_asset.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/server/grpc_asset.go b/server/grpc_asset.go index 79b283ecd..64ff7dda7 100644 --- a/server/grpc_asset.go +++ b/server/grpc_asset.go @@ -23,8 +23,12 @@ 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") + ) func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest) (*asset.FetchBlobResponse, error) { @@ -60,12 +64,7 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest) 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:" From f72adacc19a9c06ff57a83ea7fabbced28d29bbc Mon Sep 17 00:00:00 2001 From: Malte Poll <1780588+malt3@users.noreply.github.com> Date: Fri, 7 Mar 2025 15:01:30 +0100 Subject: [PATCH 2/4] remote asset: reject unusable checksum.sri qualifier Ignoring unusable checksums is insecure: The server has to reject any qualifiers that it cannot understand. Otherwise, clients cannot rely on the validation performed by the server. See: https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L109-L115 --- server/grpc_asset.go | 14 ++++++++++---- server/grpc_asset_test.go | 31 ++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/server/grpc_asset.go b/server/grpc_asset.go index 64ff7dda7..a0efc3e6f 100644 --- a/server/grpc_asset.go +++ b/server/grpc_asset.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "encoding/base64" "encoding/hex" + "fmt" "io" "net/http" "net/url" @@ -28,7 +29,9 @@ var ( "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) { @@ -75,16 +78,19 @@ 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) diff --git a/server/grpc_asset_test.go b/server/grpc_asset_test.go index f53be9801..a43a8a0fb 100644 --- a/server/grpc_asset_test.go +++ b/server/grpc_asset_test.go @@ -11,7 +11,7 @@ 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" + // pb "github.com/buchgr/bazel-remote/v2/genproto/build/bazel/remote/execution/v2" "google.golang.org/grpc/codes" @@ -63,6 +63,35 @@ 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") + } +} + type testGetServer struct { srv *httptest.Server From 3ffab324e8f4b2c86b79a12dd7f8415a431d550c Mon Sep 17 00:00:00 2001 From: Malte Poll <1780588+malt3@users.noreply.github.com> Date: Fri, 7 Mar 2025 15:05:02 +0100 Subject: [PATCH 3/4] remote asset: reformat --- server/grpc_asset.go | 1 - 1 file changed, 1 deletion(-) diff --git a/server/grpc_asset.go b/server/grpc_asset.go index a0efc3e6f..c34f4ed64 100644 --- a/server/grpc_asset.go +++ b/server/grpc_asset.go @@ -34,7 +34,6 @@ var ( ) func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest) (*asset.FetchBlobResponse, error) { - var sha256Str string // Q: which combinations of qualifiers to support? From 1fe6a801c7d0c672b35ebc171e3a35d72e9368b6 Mon Sep 17 00:00:00 2001 From: Malte Poll <1780588+malt3@users.noreply.github.com> Date: Fri, 7 Mar 2025 15:35:17 +0100 Subject: [PATCH 4/4] remote asset: reject unsupported Qualifiers The specification requires that any `Fetch` requests containing unsupported qualifiers are rejected with an `INVALID_ARGUMENT` rpc error: - https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L109-L112 - https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L127-L128 Additionally, the status returned should include a `BadRequest` error detail with `FieldViolation`s indicating the names of unsupported qualifiers: - https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L139-L143 --- server/BUILD.bazel | 2 ++ server/grpc_asset.go | 37 ++++++++++++++++++++++---- server/grpc_asset_test.go | 55 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 88 insertions(+), 6 deletions(-) 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 c34f4ed64..85ee9244c 100644 --- a/server/grpc_asset.go +++ b/server/grpc_asset.go @@ -12,6 +12,7 @@ import ( "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" @@ -64,6 +65,7 @@ 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 nil, errNilQualifier @@ -93,12 +95,17 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest) } 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) @@ -108,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 } @@ -214,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 a43a8a0fb..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" ) @@ -92,6 +94,57 @@ func TestAssetMismatchingSRIAlgorithm(t *testing.T) { } } +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