feat: add support for Auto IAM AuthN #358

enocom · 2023-08-16T20:04:43Z

AlloyDB uses a metadata exchange prior to handing over to the database driver to begin the Postgres protocol. As part of this metadata exchange, the dialer sends the IAM principal's OAuth2 token which the proxy server uses to verify connection permissions and, if IAM AuthN is requested, to authenticate to the database.

dialer.go

ttosta-google · 2023-11-07T18:28:49Z

dialer.go

 	"fmt"
 	"net"
 	"strings"
 	"sync"
 	"sync/atomic"
 	"time"

-	alloydbadmin "cloud.google.com/go/alloydb/apiv1beta"
+	alloydbadmin "cloud.google.com/go/alloydb/apiv1alpha"


apiv1 should support this feature right?

Unless v1 generated clients are not ready yet, this should also work with v1 endpoints.

The v1 changes for go should be out now based on this release: googleapis/google-cloud-go@b44c4b3

The changes aren't in a released version yet. So I bumped to beta. I'll send a separate PR to bump to v1 when the support is tagged in a release.

ttosta-google · 2023-11-07T18:37:05Z

dialer.go

+	ts := cfg.tokenSource
+	if ts == nil {
+		var err error
+		ts, err = google.DefaultTokenSource(ctx, "https://www.googleapis.com/auth/cloud-platform")


you could use CloudPlatformScope from options.go

cthumuluru · 2023-11-07T18:57:29Z

dialer.go

 	"fmt"
 	"net"
 	"strings"
 	"sync"
 	"sync/atomic"
 	"time"

-	alloydbadmin "cloud.google.com/go/alloydb/apiv1beta"
+	alloydbadmin "cloud.google.com/go/alloydb/apiv1alpha"


Unless v1 generated clients are not ready yet, this should also work with v1 endpoints.

cthumuluru · 2023-11-07T19:12:08Z

dialer.go

+	}
+
+	// Reset IO deadline before read
+	err = conn.SetDeadline(time.Now().Add(ioTimeout))


You have to clear the deadline too similar to line # 315?

Yes, good catch. Fixed.

ttosta-google · 2023-11-07T18:57:34Z

dialer.go

@@ -42,6 +48,9 @@ const (
 	defaultTCPKeepAlive = 30 * time.Second
 	// serverProxyPort is the port the server-side proxy receives connections on.
 	serverProxyPort = "5433"
+	// ioTimeout is the maximum amount of time to wait before aborting a
+	// metadata exhange
+	ioTimeout = time.Minute


Would 30 seconds be enough?

A minute at least is consistent with the timeout we have on the server side if that matters

I am +1 to making this 30 seconds potentially. I think the decision should come down to does 1 minute serve a purpose? If we change it to 30 seconds would that make any difference? (do we expect anything to change between 30s and 60s) If the metadata exchange has not resolved within 30 seconds than something is probably wrong, why wait until a minute to surface the error?

Made it 30 seconds. We can further adjust as needed. It's probably better for the server to be more lenient on the timeout and have the client wait for only 30 seconds.

ttosta-google · 2023-11-07T19:29:03Z

internal/mock/alloydb.go

+		return err
+	}
+
+	resp := &connectorspb.MetadataExchangeResponse{


What if the response code is a parameter to this function so you can add a test to simulate the server returning an error?

Yes, good idea. This will require some restructing of the test code. Let's do that in a separate PR. Meanwhile, thinking about error paths made me realize we weren't closing the TLS connection when the metadata exchange protocol fails. The test cleanup will help put an assertion around this.

Filed #427 for this work.

ttosta-google · 2023-11-07T19:29:39Z

dialer_test.go

-	conn, err := d.Dial(ctx, "/projects/my-project/locations/my-region/clusters/my-cluster/instances/my-instance")
-	if err != nil {
-		t.Fatalf("expected Dial to succeed, but got error: %v", err)
+	for i := 0; i < 10; i++ {


why is this for loop necessary?

Added a comment for clarity. Short answer: we're ensuring there's no state left over in our shared buffer.

nancynh

left a few comments

jackwotherspoon · 2023-11-08T15:23:23Z

dialer.go

@@ -42,6 +48,9 @@ const (
 	defaultTCPKeepAlive = 30 * time.Second
 	// serverProxyPort is the port the server-side proxy receives connections on.
 	serverProxyPort = "5433"
+	// ioTimeout is the maximum amount of time to wait before aborting a
+	// metadata exhange
+	ioTimeout = time.Minute


I am +1 to making this 30 seconds potentially. I think the decision should come down to does 1 minute serve a purpose? If we change it to 30 seconds would that make any difference? (do we expect anything to change between 30s and 60s) If the metadata exchange has not resolved within 30 seconds than something is probably wrong, why wait until a minute to surface the error?

jackwotherspoon · 2023-11-08T15:36:22Z

dialer.go

@@ -86,6 +95,12 @@ type Dialer struct {
 	// dialFunc is the function used to connect to the address on the named
 	// network. By default it is golang.org/x/net/proxy#Dial.
 	dialFunc func(cxt context.Context, network, addr string) (net.Conn, error)
+
+	useIAMAuthN bool
+	tokenSource oauth2.TokenSource


should this be iamTokenSource for consistency with cloud sql?: https://github.com/GoogleCloudPlatform/cloud-sql-go-connector/blob/main/dialer.go#L97

Yes, fixed.

jackwotherspoon · 2023-11-08T15:39:25Z

dialer.go

+//     response is not OK, return the response's error. If there is no error, the
+//     metadata exchange has succeeded and the connection is complete.
+//
+// Subsequent interactions with the test server use the database protocol.


is this meant to say "test server"?

Nope. Fixed.

AlloyDB uses a metadata exchange prior to handing over to the database driver to begin the Postgres protocol. As part of this metadata exchange, the dialer sends the IAM principal's OAuth2 token which the proxy server uses to verify connection permissions and, if IAM AuthN is requested, to authenticate to the database.

ttosta-google

LGTM

cthumuluru

Dailer changes related to metadata exchange looks good to me.

nancynh

Thanks Eno

enocom commented Aug 24, 2023

View reviewed changes

dialer.go Outdated Show resolved Hide resolved

enocom commented Aug 24, 2023

View reviewed changes

dialer.go Outdated Show resolved Hide resolved

enocom commented Aug 24, 2023

View reviewed changes

dialer.go Show resolved Hide resolved

enocom force-pushed the auto-iam-authn branch from d7a65b3 to 57a3439 Compare September 29, 2023 16:29

enocom force-pushed the auto-iam-authn branch 3 times, most recently from ea170c7 to 9d1a0cb Compare October 25, 2023 16:38

enocom force-pushed the auto-iam-authn branch from 9d1a0cb to 1ddd3ee Compare November 7, 2023 18:13

enocom marked this pull request as ready for review November 7, 2023 18:19

enocom requested review from cthumuluru, nancynh and a team as code owners November 7, 2023 18:19

blunderbuss-gcf bot assigned jackwotherspoon Nov 7, 2023

ttosta-google reviewed Nov 7, 2023

View reviewed changes

cthumuluru reviewed Nov 7, 2023

View reviewed changes

ttosta-google reviewed Nov 7, 2023

View reviewed changes

nancynh reviewed Nov 7, 2023

View reviewed changes

jackwotherspoon reviewed Nov 8, 2023

View reviewed changes

enocom force-pushed the auto-iam-authn branch 2 times, most recently from b32005b to b522e31 Compare November 8, 2023 16:35

enocom force-pushed the auto-iam-authn branch from b522e31 to 1928c58 Compare November 8, 2023 16:53

enocom mentioned this pull request Nov 8, 2023

Track connection count and configure response in test server #427

Closed

enocom requested review from cthumuluru, nancynh, jackwotherspoon and ttosta-google November 8, 2023 16:55

ttosta-google reviewed Nov 8, 2023

View reviewed changes

ttosta-google approved these changes Nov 8, 2023

View reviewed changes

jackwotherspoon approved these changes Nov 8, 2023

View reviewed changes

enocom mentioned this pull request Nov 8, 2023

Automatic IAM Authentication GoogleCloudPlatform/alloydb-auth-proxy#345

Closed

cthumuluru approved these changes Nov 8, 2023

View reviewed changes

nancynh approved these changes Nov 8, 2023

View reviewed changes

enocom merged commit e50dd25 into main Nov 15, 2023
16 checks passed

enocom deleted the auto-iam-authn branch November 15, 2023 16:33

release-please bot mentioned this pull request Nov 14, 2023

chore(main): release 1.5.0 #415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Auto IAM AuthN #358

feat: add support for Auto IAM AuthN #358

enocom commented Aug 16, 2023

ttosta-google Nov 7, 2023

cthumuluru Nov 7, 2023

nancynh Nov 7, 2023

enocom Nov 8, 2023

ttosta-google Nov 7, 2023

enocom Nov 8, 2023

cthumuluru Nov 7, 2023

cthumuluru Nov 7, 2023 •

edited by jackwotherspoon

Loading

enocom Nov 8, 2023

ttosta-google Nov 7, 2023

nancynh Nov 7, 2023

jackwotherspoon Nov 8, 2023

enocom Nov 8, 2023

ttosta-google Nov 7, 2023

enocom Nov 8, 2023

enocom Nov 8, 2023

ttosta-google Nov 7, 2023

enocom Nov 8, 2023

nancynh left a comment

jackwotherspoon Nov 8, 2023

jackwotherspoon Nov 8, 2023

enocom Nov 8, 2023

jackwotherspoon Nov 8, 2023

enocom Nov 8, 2023

ttosta-google left a comment

cthumuluru left a comment

nancynh left a comment

feat: add support for Auto IAM AuthN #358

feat: add support for Auto IAM AuthN #358

Conversation

enocom commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthumuluru Nov 7, 2023 • edited by jackwotherspoon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nancynh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttosta-google left a comment

Choose a reason for hiding this comment

cthumuluru left a comment

Choose a reason for hiding this comment

nancynh left a comment

Choose a reason for hiding this comment

cthumuluru Nov 7, 2023 •

edited by jackwotherspoon

Loading