Skip to content

Commit

Permalink
Judicious relaying of untrusted baggage (hasura#903)
Browse files Browse the repository at this point in the history
### What

Telemetry-baggage is propagated via headers from incoming requests to a
service and relayed when the service itself calls another service.

However, when a service is open to the public it may not want just
anyone to be able to pass it baggage.

This PR adds the ability to configure the policy towards baggage
relaying in the tracing-util crate.

### How

When the argument `initialize_tracing(..., propagate_caller_baggage =
false)` we add to the globally defined text map propagator a derived
version of the `BaggagePropagator` which cannot extract baggage from
incoming requests, only inject its own context baggage into outgoing
requests.

V3_GIT_ORIGIN_REV_ID: af9a51c20a8fe7ae2085e8218a4f1d5e01b26ae1
  • Loading branch information
plcplc authored and hasura-bot committed Jul 29, 2024
1 parent a95eaa4 commit 671ea8d
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 2 deletions.
1 change: 1 addition & 0 deletions v3/crates/auth/dev-auth-webhook/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ async fn main() -> anyhow::Result<()> {
otlp_endpoint_option.as_deref(),
env!("CARGO_PKG_NAME"),
None,
true,
)?;

let app = Router::new()
Expand Down
2 changes: 1 addition & 1 deletion v3/crates/auth/hasura-authn-jwt/src/jwt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ mod tests {
#[tokio::test]
// This test emulates scenarios where multiple JWKs are present and only the correct encoded JWT is used to decode the Hasura claims
async fn test_jwk() -> anyhow::Result<()> {
tracing_util::initialize_tracing(None, "test_jwk", None)?;
tracing_util::initialize_tracing(None, "test_jwk", None, false)?;
let mut server = mockito::Server::new_async().await;

let url = server.url();
Expand Down
1 change: 1 addition & 0 deletions v3/crates/engine/bin/engine/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ async fn main() {
server.otlp_endpoint.as_deref(),
"graphql-engine",
Some(VERSION),
false,
)
.unwrap();

Expand Down
57 changes: 56 additions & 1 deletion v3/crates/utils/tracing-util/src/setup.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use opentelemetry::baggage::BaggageExt;
use opentelemetry::propagation::TextMapPropagator;
use opentelemetry::trace::Span; // 'Span'-the-trait, c.f. to 'Span'-the-struct. Used for its
// 'set_attribute' method.
use opentelemetry::propagation::composite::TextMapCompositePropagator;
Expand All @@ -9,18 +10,43 @@ use opentelemetry_sdk::propagation::{BaggagePropagator, TraceContextPropagator};
use opentelemetry_sdk::trace::{SpanProcessor, TracerProvider};
use opentelemetry_semantic_conventions as semcov;

/// Initialize the tracing setup.
///
/// This includes setting the global tracer and propagators:
///
/// * Exporting to OTEL (e.g. Jaeger)
/// * Exporting to Zipkin (Yours truly don't know why though)
/// * Exporting to StdOut
/// * Propagating context to the standard 'traceparent' etc. headers
/// * Propagating context to the experimental 'traceresponse' header (which is subtly different from 'traceparent' in a way yours truly cannot relay faithfully, but which is what the console uses, so it's important not to break it)
/// * Propagating Baggage via http headers
/// * Adding every baggage item as a span attribute to every span
///
/// A service using the tracer-util may or may not want to propagate baggage from its callers. For
/// example, a service that faces the internet directly may not want to anyone set baggage items
/// since these end up in every span which may have an adversarial effect.
///
/// To this end we provide the 'propagate_caller_baggage' argument. When set to 'true' baggage
/// times provided by callers in the 'baggage' header is carried over without prejudice. When set
/// to 'false' we do not propagate incoming baggage, but will stil export baggage the service
/// provides itself.
pub fn initialize_tracing(
endpoint: Option<&str>,
service_name: &'static str,
service_version: Option<&'static str>,
propagate_caller_baggage: bool,
) -> Result<(), TraceError> {
// install global collector configured based on RUST_LOG env var.
tracing_subscriber::fmt::init();

global::set_text_map_propagator(TextMapCompositePropagator::new(vec![
Box::new(TraceContextPropagator::new()),
Box::new(opentelemetry_zipkin::Propagator::new()),
Box::new(BaggagePropagator::new()),
if propagate_caller_baggage {
Box::new(BaggagePropagator::new())
} else {
Box::new(InjectOnlyTextMapPropagator(BaggagePropagator::new()))
},
Box::new(TraceContextResponsePropagator::new()),
]));

Expand Down Expand Up @@ -87,6 +113,35 @@ impl SpanProcessor for BaggageSpanProcessor {
}
}

/// Produce from another TextMapPropagator a derived TextMapPropagator that only supports
/// injection, i.e., serializing the current Context into some text map (the canonical example of
/// such a map being http headers). Such a propagator will not extract any context values from
/// callers.
#[derive(Debug)]
struct InjectOnlyTextMapPropagator<T>(T);

impl<T: TextMapPropagator> TextMapPropagator for InjectOnlyTextMapPropagator<T> {
fn inject_context(
&self,
cx: &opentelemetry::Context,
injector: &mut dyn opentelemetry::propagation::Injector,
) {
self.0.inject_context(cx, injector);
}

fn extract_with_context(
&self,
cx: &opentelemetry::Context,
_extractor: &dyn opentelemetry::propagation::Extractor,
) -> opentelemetry::Context {
cx.clone()
}

fn fields(&self) -> opentelemetry::propagation::text_map_propagator::FieldIter<'_> {
self.0.fields()
}
}

pub fn shutdown_tracer() {
global::shutdown_tracer_provider();
}

0 comments on commit 671ea8d

Please sign in to comment.