Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom codec #461

Closed
wants to merge 4 commits into from
Closed

Custom codec #461

wants to merge 4 commits into from

Conversation

roblabla
Copy link
Contributor

Motivation

Allow the user to specify a custom prost codec implementation.

My use-case is to provide application-layer encryption, by (en/de)crypting the messages at serialization time, before writing them to the wire. Other use-cases include serializing as JSON instead of ProtoBuf (once support for it lands in prost).

Solution

The idea is to allow the user to specify a path to their codec implementation, e.g.

    tonic_crypto_build::configure()
        .codec_path("crate::MyCodec")
        .compile(&protos, &["proto".into()])
        .unwrap();

This is technically a breaking change, as it requires moving Method::CODEC_PATH to be a method instead. It's probably possible to avoid the breaking change (by leaving CODEC_PATH, and making codec_path have a default implementation returning CODEC_PATH), but since a 0.7 version seems to be in the work, I assume a breaking change isn't a big problem.

This allows the underlying codec to be configured by a build script,
useful when using a codec wrapper.
@LucioFranco
Copy link
Member

We have custom codec support already via https://docs.rs/tonic-build/0.3.1/tonic_build/server/fn.generate.html and https://docs.rs/tonic-build/0.3.1/tonic_build/client/fn.generate.html that use https://docs.rs/tonic-build/0.3.1/tonic_build/trait.Service.html. You should be able to override the current prost one and just replace the codec path?

@roblabla
Copy link
Contributor Author

While this does work, it means I'd have to copy paste the whole prost file in my build.rs, just to make a very minor modification to the PROST_CODEC_PATH to point to my own wrapper codec. This PR specifically aims to make it easier to create codecs that simply wrap prost's, instead of full codec replacements.

My use-case, in particular, is to allow encrypting the gRPC payload. I still want to use the underlying prost types and serializer, but I want to add a small layer on top that encrypts it (using an AEAD) before writing it to the wire. AFAIK, the codec is the only place in tonic I can hook into to do this.

@LucioFranco
Copy link
Member

LucioFranco commented Sep 29, 2020

Yeah, I see your point, I specifically didn't include this because I wanted to push people to use the trait rather than a method with a string (which I think is a finky and bad way to expose things personally).

I think what you want is compression for your prost messages and I think this is a different problem all together with how we incorporate it. I have not had much time to think about it but we could possibly provide hooks for this. Maybe, when you create the client you can pass some encoding config or something like that?

In the end this feels like a hack that we could avoid if we just implemented the proper solution for compression.

@roblabla
Copy link
Contributor Author

I think what you want is compression for your prost messages and I think this is a different problem all together with how we incorporate it.

I mean, that really depends. I'm not well-versed in how compression works in gRPC. My middleware needs to have two information at hand to work:

  • It needs to run per-message. For instance, if there's a streaming request or response, it should run independently for each message. I'm not sure how gRPC compression works, but if it can span across multiple messages, then it won't be a very interesting hooking point for me. This is both to ensure compatibility with the Python gRPC library, and to make it easier to model/understand.
  • It needs to have some information about the request (such as the URL) to determine if/how to encrypt. Some endpoints need to be in plain text (any endpoint that are intended to be used before a common cryptographic key is found).

FWIW, I originally thought this whole logic could run in an Interceptor, but it turns out tonic interceptors are a lot less powerful than their Python counterpart. In Python, interceptors can wrap the underlying serialiser, deserialiser, and callback. For example:

class CryptoInterceptor(grpc.aio.ServerInterceptor):
    async def intercept_service(self, continuation, handler_call_details):
        # Returns the RpcMethodHandler associated with this req
        inner = await continuation(handler_call_details)
        # Creates a wrapper handler
        handler = grpc.RpcMethodHandler()
        handler.request_streaming = inner.request_streaming
        handler.response_streaming = inner.response_streaming
        handler.unary_unary = inner.unary_unary
        handler.unary_stream = inner.unary_stream
        handler.stream_unary = inner.stream_unary
        handler.stream_stream = inner.stream_stream
        # Injects aes crypto in the wrapper handler
        handler.request_deserializer = wrap_deserializer(inner)
        handler.response_serializer = wrap_serializer(inner)
        return handler

In Tonic, as far as I understand, Interceptors can only inspect the headers and optionally return an error to stop the request from being processed any further. I suppose this is meant to be used for authentication? But it prevents us from adding our crypto layer here.

@LucioFranco
Copy link
Member

Yeah, there are two main ways to do compression: 1) Compressing the body like any other http request (using content-type iirc) 2) each messages gets compressed discretely. I think 1 should be pretty easy to just like place ontop of tonic using tower, 2 would require a custom codec and some changes to Grpc structs.

@roblabla roblabla mentioned this pull request Jun 29, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants