-
Notifications
You must be signed in to change notification settings - Fork 310
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
New default grpc channel for google? #93
Comments
@jboeuf would you be happy for you use case if this new helper function required Requests and didn't give you an option to use any other HTTP transport? |
@jboeuf ping - I have some free time to make this happen - care to chime in on my last comment? |
oops. Sorry and thanks for the ping :). I think that would be fine by me indeed since it is part of the same package. Maybe we could make the request object overrideable as well: e.g.:
Sorry if that does not make sense, my python is a bit rusty. |
I'm leaning towards not doing this, as with gRPC you likely want to use |
Not sure I understand your last comment regarding the JWT. The really nice thing about the JWT in gRPC is that gRPC chooses the audience for you (sets it to the endpoint being accessed) so that the client cannot be fooled into sending a JWT with, let's say a pubsub.googleapis.com audience to a joehacker.com endpoint. |
We recently removed the ability for the audience to be dynamically determined preferring for the audience to be specified ahead of time to reduce thrashing. Our client libraries have this audience readily available so it's much better performance-wise for us to not generate a jwt for every single request. |
Adding @jtattermusch, @nathanielmanistaatgoogle and @dhermes for visibility. I'm sorry but that looks like the wrong approach to me.
I understand the performance issue but this should be addressed with a smart cache as opposed to leveraging the user's input for the audience. Please let me know if you have more questions, I'm happy to answer them. Thanks! |
That's fine, that's what these discussions are for. I believe I ran this by everyone (except @jtattermusch, who I've never interacted with) when we removed it. We explicitly talked about the possibility of adding another jwt.Credentials class that can do on-demand token generation, we just didn't want that to be the only way and we didn't want to conflate that with a single-audience based-credentials.
Yes, but generally users are not using gRPC-level APIs directly, instead, they are using
Not to derail into security stuff, but a bad actor capable of convincing a user to send them JWTs can do damage regardless of the audience setting.
Great, we have a concrete use-case for on-demand token generation. I'd love to learn more about this so that I can document the justification for this kind of credentials. Also it seems weird to me that adding a load balancer in front of your API would change its audience, that would actually throw up red flags if I were paying attention to logs. I also don't completely understand why internally redirecting would also require the audience to change, or even how the metadata plugin API interface mitigates that as the URL sent to the metadata plugin wouldn't be changed by a redirect as far as I know.
Cool. Once I learn a bit more about the justification I can write a feature request for this. What we have today works for all known APIs without issues so adding this new, complex credentials class will be lower priority for me. I'm happy to help if someone else wants to take it on before me. |
We'll likely need to take an additional dependency on cachetools to appropriately implement the caching mechanism. |
Adding also @ctiller and @a11r since we discussed these types of issues in the past.
So that means that, in this case, the user of the API does not set 2 URLs essentially, correct? I'm still not super psyched about this but this is not as worrisome in the short time even though it could still impact those who use gRPC libraries directly to talk to google services (which do not have dedicated libs just yet for example).
If the JWT is not set to the right audience for an interesting service, it is basically worthless. For example, if I send a JWT to joehacker.com with the audience joehacker.com, Joe cannot replay this JWT to storage.googleapis.com or anything else basically since no service will accept a JWT with this audience.
When a user creates a channel (logical connection), internally gRPC creates potentially a bunch of subchannels which represent the underlying concrete connections to the different load balancers and backends. These may live at different endpoints, under a different name, than what the user has specified in the first place depending on the configuration of the service. This is typically the case for load balancing and a redirect would also be work this way. The metadata plugin API is called from the subchannels which have the knowledge of which full qualified service URL is being invoked for each of these: this knowledge is passed to the implementer of the plugin (as params to the callback) so that the right audience for a JWT can be set for example.
Assuming that the vast majority of users use the cloud APIs lib and that they don't have to set the audience themselves, I'm not as worried even though this is not future proof. If they do have to set this audience manually, we should probably talk offline about priorities. |
Yep, only direct users of gRPC-level APIs would have to do this. In fact, none of the very few samples we have even use JWT credentials, they all use application default credentials. See here. Also, currently our client libraries also use application default credentials. We haven't yet added the code to upgrade the credentials to JWT credentials, but we should once we get this new type implemented (cc @lukesneeringer).
That's the case. I'll make the feature a blocker for 1.0.0 of this library, which I hopefully plan to do by the end of April. Filed #136 for tracking. |
Awesome. Thanks much Jon. |
Right now, to create a new gprc channel to google, our API users have to do:
It seems that we should be able to factor all these lines by providing an API like this:
The text was updated successfully, but these errors were encountered: