fix: Refresh client cert when CA rotation #934

hessjcg · 2025-01-31T21:58:26Z

When client CA is rotated, this can cause TLS read errors after the Dialer.Dial() has returned. The certificate should be refreshed.

This adds logic to the dialer to refresh the connection cache if a TLS error occurs after net.Conn is returned to the database driver.

Fixes #932

jackwotherspoon · 2025-02-04T21:09:29Z

dialer.go

+	key := createKey(i)
+	if cachedC, ok := d.cache[key]; ok && cachedC == c {
+		delete(d.cache, key)
+	}


nit: can we add a comment around this logic?

What case is this for?

Comments updated. removeCached() may be called multiple times for the same monitoredCache instance, for example if several sockets are open and they all error at the same time.

The logic here will ensure that the cache entry is safely removed from the cache. This accounts for the case where this instance was already removed, and for the case where it was removed and replaced with a new instance.

jackwotherspoon · 2025-02-04T21:12:44Z

dialer_test.go

+			// Note: this succeeds with lazy refresh, but fails with lazy.
+			// because dialer.ForceRefresh does not block connections while the
+			// refresh is in progress.


I think there is a typo in this comment...?

+1 This looks like a dev-artifact that isn't true anymore?

Fixed. Deleted the comment - it was a dev artifact.

enocom · 2025-02-04T21:17:38Z

dialer.go

-	}, d.dialerID, cn.String())
+	},
+		func(err error) {
+			// ignore EOF


Could we expand this comment on why we're ignoring EOF? I think EOFs are the server sending an RST packet, but that's not obvious knowledge for future maintainers.

enocom · 2025-02-04T21:19:10Z

dialer.go

@@ -435,7 +437,22 @@ func (d *Dialer) Dial(ctx context.Context, icn string, opts ...DialOption) (conn
 	iConn := newInstrumentedConn(tlsConn, func() {
 		n := atomic.AddUint64(c.openConnsCount, ^uint64(0)) // c.openConnsCount = c.openConnsCount - 1
 		trace.RecordOpenConnections(context.Background(), int64(n), d.dialerID, cn.String())
-	}, d.dialerID, cn.String())
+	},
+		func(err error) {


whitespace nit: putting this up a line would make it more obvious it's an argument at a glance.

Alternatively, you could pull this anonymous function out into a variable:

errFunc := func(err error) { // ... } iConn := newInstrumentedConn(/*...*/, errFunc)

enocom · 2025-02-04T21:20:55Z

dialer.go

@@ -446,12 +463,19 @@ func (d *Dialer) Dial(ctx context.Context, icn string, opts ...DialOption) (conn
 	}
 	return iConn, nil
 }
+func (d *Dialer) isTLSError(err error) bool {
+	if nErr, ok := err.(net.Error); ok {
+		return !nErr.Timeout() && // it's a permanent net error


Have you manually tested this logic as part of doing a cert rotation to confirm it's working as intended?

Yes. Also the new tests in dialer_test.go create genuine TLS certificate errors, so this is covered by unit tests too.

Might be worth adding an e2e test for this, but otherwise this seems fine.

This only happens on a support action: when the client CA is rotated, but the server certificate remains the same. There is no public API for that. So I decided e2e was overkill.

Ah got it. I missed that detail. I agree then -- no e2e is fine.

enocom · 2025-02-04T21:22:52Z

dialer_test.go

+			// Note: this succeeds with lazy refresh, but fails with lazy.
+			// because dialer.ForceRefresh does not block connections while the
+			// refresh is in progress.


+1 This looks like a dev-artifact that isn't true anymore?

enocom · 2025-02-04T21:23:52Z

dialer_test.go

+	// Expect an error on read. This should trigger the dialer to refresh.
+	_, err = io.ReadAll(conn)
+	if err != nil {
+		t.Log("Got error on read as expected.", err)


t.Logf("Got error on read as expected: %v")

enocom · 2025-02-04T21:24:41Z

dialer_test.go

+	t.Log("Second attempt should fail...")
+	_, err := d.Dial(context.Background(), "my-project:my-region:my-instance")
+	if err != nil {
+		t.Log("Got error on dial as expected.", err)


enocom · 2025-02-04T21:24:47Z

dialer_test.go

+	if err != nil {
+		t.Log("Got error on dial as expected.", err)
+	} else {
+		t.Fatal("Want dial error, got no error")


s/Want/want/

enocom · 2025-02-04T21:25:17Z

dialer_test.go

+	t.Log("Third attempt OK.")
+}
+
+func TestDialerRefreshesAfterRotateServerCA(t *testing.T) {


Could this test and the one above be combined into a table test by parameterizing the only differences between the two tests?

Test parameterized.

enocom · 2025-02-04T21:26:07Z

internal/mock/cloudsql.go

 	if f.serverCAMode == "" || f.serverCAMode == "GOOGLE_MANAGED_INTERNAL_CA" {
 		// legacy server mode, return only the server cert
 		return toPEMFormat(f.certs.serverCert)
 	}
 	return toPEMFormat(f.certs.casServerCertificate, f.certs.serverIntermediateCaCert, f.certs.serverCaCert)
+


nit: let's omit these changes from the PR since they're unrelated

enocom · 2025-02-04T21:26:44Z

internal/mock/sqladmin.go

 	r := &Request{
 		reqMethod: http.MethodGet,
 		reqPath:   fmt.Sprintf("/sql/v1beta4/projects/%s/instances/%s/connectSettings", i.project, i.name),
 		reqCt:     ct,
 		handle: func(resp http.ResponseWriter, _ *http.Request) {
+			var ips []*sqladmin.IpMapping


Is this a necessary change? Looks like formatting to me?

Yes, this is necessary. The Server CA changes during the test. The ServerCaCert response field needs to hold the up-to-date value. Thus, this method needs to create the response at the time that the request is made.

Got it -- a comment to that effect above i.serverCACert() would help future maintainers.

wip: merged down new tests

enocom · 2025-02-05T21:46:37Z

dialer.go

@@ -446,12 +463,19 @@ func (d *Dialer) Dial(ctx context.Context, icn string, opts ...DialOption) (conn
 	}
 	return iConn, nil
 }
+func (d *Dialer) isTLSError(err error) bool {
+	if nErr, ok := err.(net.Error); ok {
+		return !nErr.Timeout() && // it's a permanent net error


Might be worth adding an e2e test for this, but otherwise this seems fine.

enocom · 2025-02-05T21:50:28Z

dialer.go

 ) {
 	d.logger.Debugf(
 		ctx,
 		"[%v] Removing connection info from cache: %v",
 		i.String(),
 		err,
 	)
+
+	// If this instance of monitoredCache is still in the cache, remove it.


Would we just always call delete, regardless of whether the monitoredCache is present or not in cache?

The removeCached() method can be called multiple times for the same monitoredCache instance, and needs to be idempotent. Therefore, it should only delete the cache entry if the monitoredCache instance is the same. Otherwise, it is not idempotent and will cause incorrect behavior.

Here's an example scenario:

Suppose at the start that d.cache["proj:reg:inst"] contains a valid monitoredCache instance.

Then, the instance "proj:reg:inst" Client CA certificates for are rotated. Now the dialer no longer has valid metadata for this instance, new sockets will fail with a TLS error after being returned from d.Dial().

The following actions occur across 3 different goroutines. By chance, they occur in this specific order.

(goroutine 1) The application calls d.Dial("proj:reg:inst"), it establishes a connection that is about to fail.

(goroutine 2) The application calls d.Dial("proj:reg:inst"), it establishes a second connection that is about to fail.

(goroutine 1) The first connection fails, and removeCached(c) is called. d.cache["proj:reg:inst"] == c, so the the invalid monitoredCache instance removed from the d.cache. Then the invalid monitoredCache instance is closed.

(goroutine 3) Then the application calls d.Dial() again, Dialer does not contain a cache entry, so it fetches the instance data and creates a new, valid monitoredCache instance. A third successful connection is created.

Now d.cache contains a new, valid monitoredCache instance that was created during the third call to d.Dial(). However the second connection hasn't failed yet. Then...

(goroutine 2) The second connection fails, and removeCached(c) is called for the old, invalid instance. d.cache["proj:reg:inst"] != c. Instead, the cache holds the valid, new monitoredCache instance.

The removeCache(c) method should leave the new, valid monitoredCache instance in the cache.

I see. That's probably an edge case, but also worth solving for. Maybe add a note like, "in the case of multiple goroutines creating cache entries, only purge the cache if it's for a known failed attempt and don't overwrite separate goroutine entries." This is a subtle little detail otherwise.

Or maybe just:

// If this instance was already removed from the cache or *a separate goroutine* replaced it with // a new instance, do nothing.

enocom · 2025-02-05T21:50:48Z

dialer_test.go

+		wantErrorOnDial bool
+		wantErrorOnRead bool
+		useLazyRefresh  bool
+	}{{


nit: this isn't consistently formatted as below

enocom · 2025-02-05T21:51:24Z

dialer_test.go

+
+			opts := []Option{
+				WithTokenSource(mock.EmptyTokenSource{}),
+				//WithDebugLogger(&dialerTestLogger{t: t}),


Can this be deleted?

I uncommented it. It helps to understand the test.

enocom · 2025-02-05T21:52:01Z

internal/mock/sqladmin.go

 	r := &Request{
 		reqMethod: http.MethodGet,
 		reqPath:   fmt.Sprintf("/sql/v1beta4/projects/%s/instances/%s/connectSettings", i.project, i.name),
 		reqCt:     ct,
 		handle: func(resp http.ResponseWriter, _ *http.Request) {
+			var ips []*sqladmin.IpMapping


Got it -- a comment to that effect above i.serverCACert() would help future maintainers.

enocom · 2025-02-06T20:46:41Z

dialer.go

 ) {
 	d.logger.Debugf(
 		ctx,
 		"[%v] Removing connection info from cache: %v",
 		i.String(),
 		err,
 	)
+
+	// If this instance of monitoredCache is still in the cache, remove it.


I see. That's probably an edge case, but also worth solving for. Maybe add a note like, "in the case of multiple goroutines creating cache entries, only purge the cache if it's for a known failed attempt and don't overwrite separate goroutine entries." This is a subtle little detail otherwise.

enocom · 2025-02-06T20:47:51Z

dialer.go

 ) {
 	d.logger.Debugf(
 		ctx,
 		"[%v] Removing connection info from cache: %v",
 		i.String(),
 		err,
 	)
+
+	// If this instance of monitoredCache is still in the cache, remove it.


Or maybe just:

// If this instance was already removed from the cache or *a separate goroutine* replaced it with // a new instance, do nothing.

jackwotherspoon

LGTM ✅

Thanks for the extra assist here @enocom 👏

hessjcg force-pushed the gh-932-refresh-on-tls-error branch from 6a61292 to 20c6162 Compare January 31, 2025 22:27

hessjcg force-pushed the gh-932-test-certs branch 2 times, most recently from 28e50db to 3a7c320 Compare January 31, 2025 22:31

hessjcg force-pushed the gh-932-refresh-on-tls-error branch from 20c6162 to bb306f8 Compare January 31, 2025 22:32

hessjcg force-pushed the gh-932-test-certs branch from 3a7c320 to d237673 Compare January 31, 2025 22:39

hessjcg force-pushed the gh-932-refresh-on-tls-error branch from bb306f8 to 0ae60ad Compare January 31, 2025 22:39

hessjcg marked this pull request as ready for review January 31, 2025 22:41

hessjcg requested a review from a team as a code owner January 31, 2025 22:41

blunderbuss-gcf bot assigned jackwotherspoon Jan 31, 2025

hessjcg requested a review from jackwotherspoon January 31, 2025 22:41

hessjcg force-pushed the gh-932-test-certs branch from d237673 to 1d5dd2e Compare January 31, 2025 22:45

hessjcg force-pushed the gh-932-refresh-on-tls-error branch 2 times, most recently from 95d468c to fb0dcbf Compare January 31, 2025 22:47

jackwotherspoon requested changes Feb 4, 2025

View reviewed changes

jackwotherspoon requested a review from enocom February 4, 2025 21:13

enocom reviewed Feb 4, 2025

View reviewed changes

hessjcg force-pushed the gh-932-test-certs branch 2 times, most recently from bb7814e to 78d62bf Compare February 5, 2025 20:48

hessjcg force-pushed the gh-932-refresh-on-tls-error branch 2 times, most recently from 4b7b13e to 27125b6 Compare February 5, 2025 20:56

Base automatically changed from gh-932-test-certs to main February 5, 2025 21:00

wip: Failing test - error on read after dial.

e7649f9

wip: merged down new tests

hessjcg force-pushed the gh-932-refresh-on-tls-error branch from 27125b6 to 0f5f778 Compare February 5, 2025 21:01

hessjcg requested review from jackwotherspoon and enocom February 5, 2025 21:01

enocom reviewed Feb 5, 2025

View reviewed changes

hessjcg force-pushed the gh-932-refresh-on-tls-error branch 2 times, most recently from eb3a6db to 6b9221f Compare February 6, 2025 18:14

hessjcg requested a review from enocom February 6, 2025 20:32

enocom approved these changes Feb 6, 2025

View reviewed changes

jackwotherspoon approved these changes Feb 7, 2025

View reviewed changes

fix: Refresh client cert when it is rejected by the server.

76c0a59

hessjcg force-pushed the gh-932-refresh-on-tls-error branch from 6b9221f to 76c0a59 Compare February 7, 2025 18:59

hessjcg merged commit c22e2d4 into main Feb 7, 2025
14 checks passed

hessjcg deleted the gh-932-refresh-on-tls-error branch February 7, 2025 19:17

release-please bot mentioned this pull request Feb 7, 2025

chore(main): release 1.15.0 #940

Merged

fix: Refresh client cert when CA rotation #934

fix: Refresh client cert when CA rotation #934

Conversation

hessjcg commented Jan 31, 2025 • 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

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

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

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

jackwotherspoon left a comment

Choose a reason for hiding this comment

hessjcg commented Jan 31, 2025 •

edited by jackwotherspoon

Loading