Feature request: allow direct access to message binary

[adamkewley]

A gRPC-based project I'm working on requires signing requests (with RSA or HMAC). In our implementation, we concatenate the method name, relevant headers, and the message binary to create a digest (inspiration stole from this RFC draft). However, I couldn't find an easy way of accessing the message binary server-side (interceptors only give access to method + headers), so I had to hack this in.

I understand that this may not be suitable for streaming messages, etc. but I thought I'd contribute this PR to open a discussion on the idea. Overall, this change increases the lifetime of a message's binary buffer to be the same as the lifetime of the context, which will increase the overall (on-average) memory usage of a gRPC server because it gives the GC less to play with across the lifetime of one gRPC request.

[thelinuxfoundation]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• One or more commits are not associated with a GitHub user. The email used in the Git commit must match a verified email address in a GitHub user profile. For more information, please see https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

Regards,
CLA GitHub bot

[gnossen]

I haven't taken the time to fully digest the implications of this change yet, but I have two initial observations:

1. It's not sufficient to just add a method to _server.py. The method will also need to be added to the Server ServicerContext interface in __init__.py
2. Since this is a user-facing API change, it is going to require a gRFC.

[adamkewley]

@gnossen the method has been added to the Context's API, which is a user-facing API change that will apply on a per-message basis.

If any python devs know a way to get access to the gRPC message binary pre deserialization then that's my main requirement: the signing procedure being used requires direct access to those bytes.

[gnossen]

@adamkewley Would it be possible to accomplish your use case with a custom deserializer?

[adamkewley]

@gnossen possibly, but the deserializers that are currently matched against each gRPC method are generated using protobuf. I guess I could decorate a (hopefully non-generated) class with methods that do the signing step. However, anything further downstream (e.g. the implementation methods that override the stub) are already too late in the process :(

[thelinuxfoundation]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• One or more commits are not associated with a GitHub user. The email used in the Git commit must match a verified email address in a GitHub user profile. For more information, please see https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

Regards,
CLA GitHub bot

[adamkewley]

So, for my purposes (signing/encrypting a gRPC message), I needed to mod in the latest push I made. Essentially, it required adding a request_mapper and response_mapper to the outer API, so that our codebase could use those to map requests/responses hitting our gRPC server

[gnossen]

I can't help but wonder if this use case wouldn't be better served by a complete set of client-side and server-side interceptors. The client-side support is already there, but server-side support is pending.

[adamkewley]

Yes, something like that is pretty much exactly what's needed for this particular use-case. I wasn't sure what that type of feature would be called (the existing api already uses "interceptors"), so I missed this existing work. Thanks for pointing it out: I'll look into it.

[stale]

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!