Security issue: upgrade prost depedency to 0.8.0

[flavio]

I run cargo audit against a project of mine, Adding the opentelemetry-otlp dependency lead to this discovery:

Crate:         prost-types
Version:       0.7.0
Title:         Conversion from `prost_types::Timestamp` to `SystemTime` can cause an overflow and panic
Date:          2021-07-08
ID:            RUSTSEC-2021-0073
URL:           https://rustsec.org/advisories/RUSTSEC-2021-0073
Solution:      Upgrade to >=0.8.0
Dependency tree: 
prost-types 0.7.0
└── prost-build 0.7.0
    └── tonic-build 0.4.2
        └── opentelemetry-otlp 0.7.0

I've tried to contribute a fix for that by forking this repo and updating the prost dependency. Unfortunately prost 0.8 requires tonic 0.5, which introduces some breaking changes inside of the automatically generated code.

The breaking change happens inside of the automatically generated TraceServiceClient.

Code generated with prost 0.7 and tonic 0.4

This is the relevant snippet from the opentelemetry.proto.collector.trace.v1.rs file that is automatically generated:

pub mod trace_service_client {
     #![allow(unused_variables, dead_code, missing_docs)]
     use tonic::codegen::*;
     #[doc = " Service that can be used to push spans between one Application instrumented with"]
     #[doc = " OpenTelemetry and an collector, or between an collector and a central collector (in this"]
     #[doc = " case spans are sent/received to/from multiple Applications)."]
     pub struct TraceServiceClient<T> {
         inner: tonic::client::Grpc<T>,
     }
     impl TraceServiceClient<tonic::transport::Channel> {
         #[doc = r" Attempt to create a new client by connecting to a given endpoint."]
         pub async fn connect<D>(dst: D) -> Result<Self, tonic::transport::Error>
         where
             D: std::convert::TryInto<tonic::transport::Endpoint>,
             D::Error: Into<StdError>,
         {
             let conn = tonic::transport::Endpoint::new(dst)?.connect().await?;
             Ok(Self::new(conn))
         }
     }
     impl<T> TraceServiceClient<T>
     where
         T: tonic::client::GrpcService<tonic::body::BoxBody>,
         T::ResponseBody: Body + HttpBody + Send + 'static,
         T::Error: Into<StdError>,
         <T::ResponseBody as HttpBody>::Error: Into<StdError> + Send,
     {
         pub fn new(inner: T) -> Self {
             let inner = tonic::client::Grpc::new(inner);
             Self { inner }
         }
         pub fn with_interceptor(inner: T, interceptor: impl Into<tonic::Interceptor>) -> Self {
             let inner = tonic::client::Grpc::with_interceptor(inner, interceptor);
             Self { inner }
         }

As you can see, new and with_interceptor always return an instance of TraceServiceClient.

Code generated with prost 0.8 and tonic 0.5

This is the same code, generated with the latest versions of the crates:

 pub mod trace_service_client {
     #![allow(unused_variables, dead_code, missing_docs)]
     use tonic::codegen::*;
     #[doc = " Service that can be used to push spans between one Application instrumented with"]
     #[doc = " OpenTelemetry and an collector, or between an collector and a central collector (in this"]
     #[doc = " case spans are sent/received to/from multiple Applications)."]
     #[derive(Debug, Clone)]
     pub struct TraceServiceClient<T> {
         inner: tonic::client::Grpc<T>,
     }
     impl TraceServiceClient<tonic::transport::Channel> {
         #[doc = r" Attempt to create a new client by connecting to a given endpoint."]
         pub async fn connect<D>(dst: D) -> Result<Self, tonic::transport::Error>
         where
             D: std::convert::TryInto<tonic::transport::Endpoint>,
             D::Error: Into<StdError>,
         {
             let conn = tonic::transport::Endpoint::new(dst)?.connect().await?;
             Ok(Self::new(conn))
         }
     }
     impl<T> TraceServiceClient<T>
     where
         T: tonic::client::GrpcService<tonic::body::BoxBody>,
         T::ResponseBody: Body + Send + Sync + 'static,
         T::Error: Into<StdError>,
         <T::ResponseBody as Body>::Error: Into<StdError> + Send,
     {
         pub fn new(inner: T) -> Self {
             let inner = tonic::client::Grpc::new(inner);
             Self { inner }
         }
         pub fn with_interceptor<F>(
             inner: T,
             interceptor: F,
         ) -> TraceServiceClient<InterceptedService<T, F>>
         where
             F: FnMut(tonic::Request<()>) -> Result<tonic::Request<()>, tonic::Status>,
             T: Service<
                 http::Request<tonic::body::BoxBody>,
                 Response = http::Response<
                     <T as tonic::client::GrpcService<tonic::body::BoxBody>>::ResponseBody,
                 >,
             >,
             <T as Service<http::Request<tonic::body::BoxBody>>>::Error:
                 Into<StdError> + Send + Sync,
         {
             TraceServiceClient::new(InterceptedService::new(inner, interceptor))
         }

Now the return type of the new and with_interceptor methods is different.

That leads to the following compilation error:

error[E0308]: `match` arms have incompatible types
   --> opentelemetry-otlp/src/span.rs:358:17
    |
355 |           let client = match tonic_config.metadata.to_owned() {
    |                        -------------------------------------- `match` arms have incompatible types
356 |               None => TonicTraceServiceClient::new(channel),
    |                       ------------------------------------- this is found to be of type `TraceServiceClient<Channel>`
357 |               Some(metadata) => {
358 | /                 TonicTraceServiceClient::with_interceptor(channel, move |mut req: Request<()>| {
359 | |                     for key_and_value in metadata.iter() {
360 | |                         match key_and_value {
361 | |                             KeyAndValueRef::Ascii(key, value) => {
...   |
370 | |                     Ok(req)
371 | |                 })
    | |__________________^ expected struct `Channel`, found struct `tonic::codegen::InterceptedService`
    |
    = note: expected type `TraceServiceClient<Channel>`
             found struct `TraceServiceClient<tonic::codegen::InterceptedService<Channel, [closure@opentelemetry-otlp/src/span.rs:358:68: 371:18]>>`

I don't know how to best solve this problem... I spent some time trying, but I couldn't find a good solution. I've to admit, I'm also not experienced with protobuf code generation

[TommyCpp]

Thanks! I wonder how much overhead the interceptor will bring. If it's not that much, we can probably just create a "pass through" interceptor where the request will be returned without any changes.

[flavio]

Just as reference, this is the change I did:

diff --git a/opentelemetry-otlp/Cargo.toml b/opentelemetry-otlp/Cargo.toml
index 104a350..8b61eab 100644
--- a/opentelemetry-otlp/Cargo.toml
+++ b/opentelemetry-otlp/Cargo.toml
@@ -39,10 +39,10 @@ async-trait = "0.1"
 futures = "0.3"
 grpcio = { version = "0.9", optional = true }
 opentelemetry = { version = "0.15", default-features = false, features = ["trace"], path = "../opentelemetry" }
-prost = { version = "0.7", optional = true }
+prost = { version = "0.8", optional = true }
 protobuf = { version = "2.18", optional = true }
 thiserror = "1.0"
-tonic = { version = "0.4", optional = true }
+tonic = { version = "0.5", optional = true }
 tokio = { version = "1.0", features = ["full"], optional = true }
 opentelemetry-http = { version = "0.4", path = "../opentelemetry-http", optional = true }
 reqwest = { version = "0.11", optional = true, default-features = false }
@@ -75,5 +75,5 @@ reqwest-rustls = ["reqwest", "reqwest/rustls-tls-native-roots"]
 surf-client = ["surf", "opentelemetry-http/surf"]
 
 [build-dependencies]
-tonic-build = { version = "0.4", optional = true }
-prost-build = {version = "0.7", optional = true }
+tonic-build = { version = "0.5", optional = true }
+prost-build = {version = "0.8", optional = true }

[flavio]

@TommyCpp do you have any idea about when a new release of opentelemetry-otlp which includes this fix will be tagged?

The v0.8.0 release doesn't include it yet

[TommyCpp]

@jtescher Maybe we should cut a new release as it has been a while since the last release?

[jtescher]

@TommyCpp yeah let's get a new release out 👍

You can get a release PR ready if you want, else I will do it later today or this weekend