feat(transport): add universe domain support #2355
Conversation
|
Extracted from and depends on #2296. |
quartzmo
force-pushed
the
transport-universe-domain
branch
from
5c22acb to
348f707
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if settings.GetUniverseDomain() != credsUniverseDomain { |
There was a problem hiding this comment.
Won't this always happen if someone sets the value with WithUniverseDomain and creds returns the default?
There was a problem hiding this comment.
Correct.
The verification can be done by comparing the universe_domain from the end-user configuration to the universe_domain from the credentials. If they match, the credentials can be used, if not the client libraries must raise an error instead of making a service call to the user-specified universe.
There was a problem hiding this comment.
What is the point of the option in that case? They will never match unless it is set in the creds. Because we return a default value: https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.16.0:google/default.go;l=92
There was a problem hiding this comment.
I'm not seeing this creds method directly, do we have cases where creds have no universe embedded? Do we need to check for a non-empty universe from creds?
There was a problem hiding this comment.
They will never match unless it is set in the creds.
Here's more spec:
Credentials are universe-specific. Whether a user provides the credentials or they are received via ADC, the credentials object (or equivalent) is expected to have the universe_domain property. The Client Libraries must read this property in order to verify that the credentials are valid for the user-supplied universe domain.
There was a problem hiding this comment.
I'm not seeing this creds method directly, do we have cases where creds have no universe embedded? Do we need to check for a non-empty universe from creds?
See behavior of creds at https://github.com/golang/oauth2/blob/master/google/default.go#L91-L92
There was a problem hiding this comment.
Thanks for the reference. I think my largest concern is related to non-cloud resolution (e.g. a service not in GDU currently). With the current implementation, both settings and creds yield up the GDU as the default value so I don't think it's an issue, but it is something for future consideration.
There was a problem hiding this comment.
@shollyman This would be good to bring up with the spec authors. Afaik, the Go implementation adheres to the spec.
There was a problem hiding this comment.
After looking at this with fresh eyes and thinking though it I think this is fine here, but really feels like it should be in the oauth2 layer. If we set the the override that is plumbed through to the oauth2 layer and is the value used by the credential I think? So it feels like this should never err perhaps? If that is the case it should be a test and not a in code validation. Approving.
There was a problem hiding this comment.
The WithUniverseDomain override is in fact NOT plumbed through to the oauth2 layer, it is only used to construct the endpoint in the client, pending a number of error condition checks like the one discussed here. The creds instance here needs to have the same universe domain value as is provided to settings, but only settings gets it via the WithUniverseDomain override. The creds instance gets it either from credentials JSON, the compute MDS, or oauth2's google.CredentialsParams. If the two separately-sourced values do not match, we need to return an error here.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if settings.GetUniverseDomain() != credsUniverseDomain { |
There was a problem hiding this comment.
I'm not seeing this creds method directly, do we have cases where creds have no universe embedded? Do we need to check for a non-empty universe from creds?
No description provided.