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

Introduce channel Attributes; Use them to pass the binder target user to the NameResolver #11524

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jdcormie
Copy link
Member

@jdcormie jdcormie commented Sep 13, 2024

In multi-user Android, the target user for an Intent is unfortunately not part of the intent
itself. Instead, it's passed around as a separate argument wherever that Intent is needed.
Following suit, the binder grpc transport accepts the target Android user as
a parameter to BinderChannelBuilder -- unfortunately it's not in the target URI or the direct SocketAddress, despite being part of the server's location.

But downstream plugins such as NameResolvers need access to this UserHandle too. This change introduces a new Channel Attributes mechanism and uses it to expose the Channel's target user (for use in an upcoming NameResolverProvider).

@jdcormie jdcormie marked this pull request as ready for review September 16, 2024 17:55
@ejona86
Copy link
Member

ejona86 commented Sep 16, 2024

We're trying to remove most usages of Attributes. In general, we are replacing them with the same pattern of having a reference-equality Key with a generic for typing.

In this specific case, I'm still favoring exposing ChannelCredentials to the NameResolver, and adding the android context to the ChannelCredentials.

@jdcormie
Copy link
Member Author

We're trying to remove most usages of Attributes. In general, we are replacing them with the same pattern of having a reference-equality Key with a generic for typing.

Can you point me to an example of this I can emulate?

In this specific case, I'm still favoring exposing ChannelCredentials to the NameResolver, and adding the android context to the ChannelCredentials.

But this PR isn't about the Android Context at all, it's about the target UserHandle which definitely doesn't belong in ChannelCredentials.

@ejona86
Copy link
Member

ejona86 commented Sep 16, 2024

We're trying to remove most usages of Attributes. In general, we are replacing them with the same pattern of having a reference-equality Key with a generic for typing.

Can you point me to an example of this I can emulate?

All the "Key"s in the API. CallOptions, Attributes, Context, CreateSubchannelArgs. Where they look different, there is probably an open issue to change one of them.

But this PR isn't about the Android Context at all, it's about the target UserHandle which definitely doesn't belong in ChannelCredentials.

This is the destination identity? And this is for cross-user binder calls? I guess the system might do some of that. UserHandle can be created from a uid, but once created there's no way to learn anything about it; you can only pass it to the system?

How commonly would this be used?

@jdcormie
Copy link
Member Author

jdcormie commented Sep 17, 2024

This is the destination identity? And this is for cross-user binder calls? I guess the system might do some of that.

Correct, it's the destination Android user for cross-user binder Channels (issue #10173, dd: go/cross-user-ods). And yes it is mostly used by Android system apps because ordinary apps don't normally have permission to reach outside the per-user sandboxing. Our team's is using this in production today. I presume original author wwtbuaa01 is using it too. Not sure who else.

The missing piece, though, is that the NameResolver needs a way to know the target user if we're going to have a single instance of cs/symbol:AndroidIntentNameResolverProvider as you requested. Because that information is per-Channel.

UserHandle can be created from a uid, but once created there's no way to learn anything about it; you can only pass it to the system?

Yeah UserHandle mostly is just a handle that you pass to other Android APIs. In particular, cs/symbol:AndroidIntentNameResolverProvider needs it to call hidden system API PackageManager.queryIntentServicesAsUser.

@jdcormie
Copy link
Member Author

We're trying to remove most usages of Attributes. In general, we are replacing them with the same pattern of having a reference-equality Key with a generic for typing.

Can you point me to an example of this I can emulate?

All the "Key"s in the API. CallOptions, Attributes, Context, CreateSubchannelArgs. Where they look different, there is probably an open issue to change one of them.

OK happy to do this if you are on board with the essence of this change.

@ejona86
Copy link
Member

ejona86 commented Sep 24, 2024

I spoke to the TLs of the other languages on Friday. The options seem to be channel options (similar to discussed here) and constructing the NameResolverProvider with the UserHandle to pass to channel construction (as in #11055 (comment)).

Since this is only for system apps, I'd favor not creating new infrastructure to support it, so I'm preferential to "pass the UserHandle when manually creating the NR provider."

@jdcormie
Copy link
Member Author

I've thought about it and favor the approach in this PR because it:

  1. fits with the existing/intended architecture of a single global NRR with one shared NRP of each type. You've convinced me that creating an NRR/NRP per Channel is pretty strange since the target URI is fixed and so there's no need for a registry or scheme lookup. Sharing NRPs is actually helpful for intent:/// resolution because all NRs are interested in the same Android ACTION_PACKAGE_CHANGED event and so they could all very naturally share a single BroadcastReceiver.
  2. lets the Channel creator specify the target UserHandle just once. Your proposal requires specifying it redundantly: once to BinderChannelBuilder#bindAsUser() and again in the NRP constructor. If the creator forgets one or the other, things fail mysteriously, or worse, appear to work sometimes but fail in the case where different Android users have different servers enabled/disabled (less common).
  3. lets my NRP be @Internal, just like all the others, giving me more freedom to change things later. Your proposal forces me to make the new NRP public so that users can construct it.
  4. could be refactored to look more like grpc-core's channel options, as you point out, although I'd like to hear more about how you think this would look.

Can you say more about your objection to adding the minimal Channel -> NRP infrastructure that I need here?

@jdcormie
Copy link
Member Author

Ping?

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