search

hyperium / tonic

A native gRPC client & server implementation with async/await support.
https://docs.rs/tonic
MIT License
10.02k stars 1.02k forks source link

Released version of tonic_health contains blobs #2019

Open weiznich opened 3 weeks ago

weiznich commented 3 weeks ago

Bug Report

Version

tonic_health: 0.12.3

Platform

Unrelated, code is uploaded on crates.io

Crates

Tonic-Health

Description

While reviewing a dependency update for tonic-health I noticed that it contains a binary file: https://diff.rs/tonic-health/0.12.2/0.12.3/src%2Fgenerated%2Fgrpc_health_v1.bin

That is not desirable as it makes reviewing dependencies harder. It also might hide potential attacks. In this case I believe the blob is just the binary representation of the proto file and that it was accidentally uploaded to crates.io. Please consider to explicitly exclude this file for future updates.

weiznich commented 1 week ago

@tottoto Would it be possible to resolve this, given that this is potential security related?

tottoto commented 1 week ago

While reviewing a dependency update for tonic-health I noticed that it contains a binary file: https://diff.rs/tonic-health/0.12.2/0.12.3/src%2Fgenerated%2Fgrpc_health_v1.bin

The URL seems to show Not found.

In this case I believe the blob is just the binary representation of the proto file and that it was accidentally uploaded to crates.io.

It is used to provide the file descriptor set.

weiznich commented 1 week ago

Thanks for your answer

While reviewing a dependency update for tonic-health I noticed that it contains a binary file: https://diff.rs/tonic-health/0.12.2/0.12.3/src%2Fgenerated%2Fgrpc_health_v1.bin

The URL seems to show Not found.

Seems like something is broken with diff.rs now with this kind of direct links :disappointed: You can go to https://diff.rs/tonic-health/0.12.2/0.12.3/ and navigate manually to src/generated/grpc_health_v1.bin

It is used to provide the file descriptor set.

Can you point out where exactly it is used. For me it seems like it's just there. Additionally I would argue that it's enough to include the expanded rust code + possibly the proto file. You can always generate the binary file descriptor set from the proto file if required.

tottoto commented 1 week ago

Can you point out where exactly it is used. For me it seems like it's just there.

You can find it at https://github.com/hyperium/tonic/blob/v0.12.3/tonic-health/src/lib.rs#L33. If you think something is not being used, you can actually search the code to see if it is correct.

Additionally I would argue that it's enough to include the expanded rust code + possibly the proto file. You can always generate the binary file descriptor set from the proto file if required.

A similar discussion can be found at https://github.com/hyperium/tonic/pull/1942. This is something that can be resolved before release, so I don't think the feature should be removed, considering convenience reasons.

Incidentally, the implementation way for this has been changed since the release of 0.12.2 from committing and including it as a file to embedding these bytes data in the Rust code.

weiznich commented 1 week ago

@tottoto I'm sorry to write that but I don't see how this addresses the security concerns around a unreviewable blob. I can see how this might be convenient for certain uses, but it makes it really hard to review and reason about the actual code. By having it actually included in the compiled code that becomes more serve from my point of view