L85: Native cross-process communication on Android using BinderChannel#259
L85: Native cross-process communication on Android using BinderChannel#259sifmelcara wants to merge 8 commits intogrpc:masterfrom
Conversation
sifmelcara
changed the title
sifmelcara
force-pushed
the
core-binder-transport
branch
from
a79dc38 to
9c52280
Compare
|
cc @markdroth |
L85-core-binder-transport.md
Outdated
| We propose to introduce a new API that returns credentials for binder server: | ||
|
|
||
| ```Cpp | ||
| std::shared_ptr<ServerCredentials> BinderServerCredentials(std::shared_ptr<grpc::binder::SecurityPolicy> security_policy); |
There was a problem hiding this comment.
We should include the C API grpc_server_add_binder_port into this RFC per discussions made in grpc/grpc#27036.
There was a problem hiding this comment.
Mark and Craig: Can you confirm if we only need C API for this specific API are we need C API for every C++ API? What's the reason to introduce C API?
L85-core-binder-transport.md
Outdated
| class SecurityPolicy { | ||
| public: | ||
| // return true if the uid is authorized to connect | ||
| virtual bool IsAuthorized(void* jni_env, int uid) = 0; |
There was a problem hiding this comment.
To my knowledge, there is no way to get access to PackageManager in NDK, so it would be easier on implementing teams if this function included the package name and signing certificate(s) to the c++ client; otherwise clients trying to implement security will need to include extra java code to call into PackageManager.
Alternatively, including a utility function to get the package name + signing certificate from c++ code could be used.
There was a problem hiding this comment.
Our plan is to provide many possible implementations of SecurityPolicy that makes sense, so client unlikely need to customize this
Yeah we can include a C++ utility function to get package name + signing certificate so that user can check certificate in pure C++. (This is a little bit off topic, and the utility function not necessary to be part of gRPC I guess)
L85-core-binder-transport.md
Outdated
|
|
||
| ### Android API level requirement | ||
|
|
||
| For Java interoperability, we will require the device to have API level 32 |
There was a problem hiding this comment.
What features require API level 32? It seems unfortunate that this would rule out use of BinderChannel on all existing Android devices until receiving an OS update. Is it possible to target a lower API level?
There was a problem hiding this comment.
A new API for disabling header
Is it possible to target a lower API level?
For C++ Server <--> C++ Client scenario it is possible to target API level 31
| currently don't have a plan to support it in C++. When we see a parcelable | ||
| object in `Parcel`, we will print an error message and fail the RPC call. | ||
|
|
||
| ### Error handling |
There was a problem hiding this comment.
Could you also make a note about what happens to the BinderChannel if the remote process is killed? Will the channel automatically try to reconnect the way a TCP or UDP channel does, or does the calling code need to manage reconnection?
There was a problem hiding this comment.
Added. Our Java class will receive notification when the connection is broken, and our code should be able to handle the connection recovery (and check authorization again if the server app is updated) for user.
Ref: https://developer.android.com/reference/android/content/ServiceConnection
sifmelcara
force-pushed
the
core-binder-transport
branch
from
ed810de to
6a03466
Compare
|
Hi Craig would you be able to take a look or advice us how to proceed? Thank you! |
which will allow the channel to initially be in a disconnected state after creation.
| separate APIs for creating binder transport channels. | ||
|
|
||
| ```cpp | ||
| std::shared_ptr<grpc::Channel> CreateBinderChannel( |
There was a problem hiding this comment.
Do you actually need a special channel creation function? Could you instead pass jni_env and context via channel args, package_name and class_name via the URI, and security_policy via a new credential type?
There was a problem hiding this comment.
jni_env and context is mandatory for establishing the connection and they are pointers to special JVM structure. I'm not sure about the convention and assumed that channel_args is mainly for specifying configurations and might not be suitable for this use case. jni_env and context are non-optional and they don't feel like "channel arguments" since they are basically only used for establishing the connection and is not related to grpc::Channel logic.
For package_name and class_name it is possible to encode them into URI but it is simpler to not include them, since 1. package_name and class_name are also only used single-time when establishing the connection (in Java). 2. package_name and class_name is not the only way to establish the connection. For example, in the future we might support creating channel from a pointer to binder, in that case it is similar to grpc_insecure_channel_create_from_fd and URI does not make too much sense for that case.
For security_policy I think it cannot be easily implemented with gRPC's credential design (I might be wrong). gRPC credential is about writing/reading some kind of token/signature in wire data. However our security policies are using system calls to verify if the connection is allowed and has nothing to do with wire data or headers.
So in the end I decided that it is easier (for users and for us) to introduce a separate interfaces instead of squeezing everything into existing CreateChannel interface.
|
The implementation was completed, lasted a while, then we deleted it, and now it seems this PR is never going to be terribly useful to finish. |
5 participants
Preview: https://github.com/sifmelcara/proposal/blob/core-binder-transport/L85-core-binder-transport.md