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

auth: grpc.NewClient dns scheme incompatible with secure forward-proxies #11089

Closed
erezrokah opened this issue Nov 6, 2024 · 10 comments
Closed
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@erezrokah
Copy link

erezrokah commented Nov 6, 2024

Client

cloud.google.com/go/auth

Environment

go version go1.23.2 darwin/arm64

Code and Dependencies

This issue impacts all consumers of the auth package which I believe are all the different packages in this repo and also https://github.com/googleapis/google-api-go-client/blob/3347fa1ef84d6de6d5f0ce651bf6da6577791a9e/transport/grpc/dial.go#L22

go.mod
module modname

go 1.22.4

require (
	cloud.google.com/go/accessapproval v1.8.2
	cloud.google.com/go/aiplatform v1.68.0
	cloud.google.com/go/apigateway v1.7.2
	cloud.google.com/go/apikeys v0.6.0
	cloud.google.com/go/appengine v1.9.2
	cloud.google.com/go/artifactregistry v1.15.2
	cloud.google.com/go/asset v1.20.3
	cloud.google.com/go/baremetalsolution v1.3.2
	cloud.google.com/go/batch v1.11.2
	cloud.google.com/go/beyondcorp v1.1.2
	cloud.google.com/go/bigquery v1.63.1
	cloud.google.com/go/bigtable v1.33.0
	cloud.google.com/go/billing v1.19.2
	cloud.google.com/go/binaryauthorization v1.9.2
	cloud.google.com/go/certificatemanager v1.9.2
	cloud.google.com/go/cloudtasks v1.13.2
	cloud.google.com/go/compute v1.28.2
	cloud.google.com/go/container v1.41.0
	cloud.google.com/go/containeranalysis v0.13.2
	cloud.google.com/go/dataproc/v2 v2.10.0
	cloud.google.com/go/deploy v1.23.1
	cloud.google.com/go/domains v0.10.2
	cloud.google.com/go/errorreporting v0.3.1
	cloud.google.com/go/filestore v1.9.2
	cloud.google.com/go/firestore v1.17.0
	cloud.google.com/go/functions v1.19.2
	cloud.google.com/go/iam v1.2.2
	cloud.google.com/go/kms v1.20.1
	cloud.google.com/go/logging v1.12.0
	cloud.google.com/go/longrunning v0.6.2
	cloud.google.com/go/monitoring v1.21.2
	cloud.google.com/go/networkmanagement v1.15.0
	cloud.google.com/go/networkservices v0.2.2
	cloud.google.com/go/osconfig v1.14.2
	cloud.google.com/go/pubsub v1.45.1
	cloud.google.com/go/redis v1.17.2
	cloud.google.com/go/resourcemanager v1.10.2
	cloud.google.com/go/run v1.6.1
	cloud.google.com/go/scheduler v1.11.2
	cloud.google.com/go/secretmanager v1.14.2
	cloud.google.com/go/securitycenter v1.35.2
	cloud.google.com/go/serviceusage v1.9.2
	cloud.google.com/go/storage v1.45.0
	cloud.google.com/go/storagetransfer v1.11.2
	cloud.google.com/go/trace v1.11.2
	cloud.google.com/go/translate v1.12.2
	cloud.google.com/go/video v1.23.2
	cloud.google.com/go/vision/v2 v2.9.2
	cloud.google.com/go/vmmigration v1.8.2
	cloud.google.com/go/vpcaccess v1.8.2
	cloud.google.com/go/websecurityscanner v1.7.2
	cloud.google.com/go/workflows v1.13.2
	github.com/apache/arrow/go/v17 v17.0.0
	github.com/cdfmlr/ellipsis v0.0.1
	github.com/cloudquery/codegen v0.3.19
	github.com/cloudquery/plugin-sdk/v4 v4.68.2
	github.com/golang/mock v1.6.0
	github.com/googleapis/gax-go/v2 v2.13.0
	github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.1.0
	github.com/iancoleman/strcase v0.3.0
	github.com/invopop/jsonschema v0.12.0
	github.com/julienschmidt/httprouter v1.3.0
	github.com/rs/zerolog v1.33.0
	github.com/spf13/cast v1.5.0
	github.com/stretchr/testify v1.9.0
	github.com/thoas/go-funk v0.9.3
	github.com/wk8/go-ordered-map/v2 v2.1.8
	golang.org/x/exp v0.0.0-20241009180824-f66d83c29e7c
	golang.org/x/sync v0.8.0
	google.golang.org/api v0.203.0
	google.golang.org/genproto v0.0.0-20241021214115-324edc3d5d38
	google.golang.org/grpc v1.67.1
	google.golang.org/protobuf v1.35.1
)

Expected behavior

Resolving scheme should not change without communicating the breaking change

Actual behavior

Resolving scheme changed in a chore: #10780

Additional context

Started after upgrading to v0.9.2.

Setting GOOGLE_API_GO_EXPERIMENTAL_DISABLE_NEW_AUTH_LIB to true seems to fix the issue as it changes the logic here to use DialContext instead of NewClient from the auth package

Related to grpc/grpc-go#7556

Updated

Added a reproduction scenario via https://github.com/cloudquery/gcp-client-breakage

@quartzmo
Copy link
Member

quartzmo commented Nov 6, 2024

@erezrokah Can you please update this issue to include actual program output in the Actual behavior section demonstrating how the change from passthrough to dns is breaking your usage of the library? And if it is not obvious, maybe describe in Expected behavior how your program worked before the change? These sections are not intended for commentary, but instead for concrete demos. Thanks!

@quartzmo quartzmo added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Nov 6, 2024
@quartzmo
Copy link
Member

quartzmo commented Nov 6, 2024

See discussion and possible workaround in grpc/grpc-go#7556.

@erezrokah
Copy link
Author

erezrokah commented Nov 6, 2024

Thanks for the reply @quartzmo 👍

@erezrokah Can you please update this issue to include actual program output in the Actual behavior section demonstrating how the change from passthrough to dns is breaking your usage of the library? And if it is not obvious, maybe describe in Expected behavior how your program worked before the change? These sections are not intended for commentary, but instead for concrete demos. Thanks!

I'll add sample code but in order to observe the behavior you'd need put up a forward proxy like envoy and see it break.

See discussion and possible workaround in grpc/grpc-go#7556.

Won't this workaround disable DSN resolving altogether even when explicitly set by the scheme?

@quartzmo
Copy link
Member

quartzmo commented Nov 6, 2024

in order to observe the behavior you'd need put up a forward proxy like envoy and see it break.

We are asking you to provide this program output, thank you!

@quartzmo
Copy link
Member

quartzmo commented Nov 6, 2024

Another possible workaround tactic is to use option.WithEndpoint to set the service endpoint with the passthrough scheme. This was done in the handwritten Spanner client for emulator usage in /pull/10947.

@erezrokah
Copy link
Author

We are asking you to provide this program output, thank you!

Let me set up a repo that better shows how this is a breaking change and I'll update the issue with it.

Another possible workaround tactic is to use option.WithEndpoint to set the service endpoint with the passthrough scheme. This was done in the handwritten Spanner client for emulator usage in /pull/10947.

Is there a way of doing that without knowing each service endpoint in advance? We're using almost every module in this repo to interact with GCP so adding it manually will require a bit of work, and also we'd have to maintain the list of endpoints up to date

To clarify I'm not asking to revert the change, only a way to get the previous behavior that can be applied generically to all modules

@quartzmo
Copy link
Member

quartzmo commented Nov 6, 2024

To clarify I'm not asking to revert the change, only a way to get the previous behavior that can be applied generically to all modules

Sounds good. I agree that this is really important. As I have been doing, please link any tactical workarounds you come across, as one of these might offer an solution to be used either directly by users, or indirectly via some change to the client libraries or the auth library.

Note that grpc.Dial (where passthrough is the default) has been deprecated, and that there are considerable performance advantages with client-side load balancing in some environments for using its replacement grpc.NewClient (where dns is the default). These were the motivations for the change. We did not immediately see the potential for breakage, apologies for that.

@quartzmo quartzmo changed the title auth: Package behaviour is incompatible with secure forward-proxies starting from version 0.9.2 auth: grpc.NewClient dns scheme incompatible with secure forward-proxies Nov 6, 2024
@erezrokah
Copy link
Author

erezrokah commented Nov 8, 2024

Hi @quartzmo I've created a reproduction in the repo here https://github.com/cloudquery/gcp-client-breakage.
It shows how moving to DNS resolution to the client can break existing use cases when the client is behind a proxy with access control that's enforced via the domain of the request.

@quartzmo
Copy link
Member

Per the recommendation of @arjan-bal on grpc/grpc-go#7556 (comment), we are in agreement that we should temporarily revert usages of NewClient to Dial in the Go client libraries, pending a fix by gRPC for forward proxies.

@quartzmo
Copy link
Member

Closed by #11118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants