[crypto] acme/autocert: fix renewal timer issue

[Roland Shoemaker (Gerrit)]

Roland Shoemaker submitted this change.

View Change

2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: acme/autocert/autocert.go
Insertions: 1, Deletions: 1.

@@ -609,7 +609,7 @@
 	}
 	state.cert = der
 	state.leaf = leaf
-	go m.startRenew(ck, state.key, state.leaf.NotAfter)
+	m.startRenew(ck, state.key, state.leaf.NotAfter)
 	return state.tlscert()
 }
 
```

Approvals: Gopher Robot: TryBots succeeded Bryan Mills: Looks good to me, approved Roland Shoemaker: Run TryBots

acme/autocert: fix renewal timer issue

Block when creating the renewal timer, rather than doing it in a
goroutine. This fixes an issue where startRenew and stopRenew are called
very closely together, and due to lock ordering, stopRenew may be called
before startRenew, resulting in the appearance that the renewal timer
has been stopped before it has actually been created.

This is only an issue in tests, as that is the only place stopRenew is
actually used. In particular this issue manifests in TestGetCertiifcate
sub-tests, where a httptest server reuses a port across two of the
sub-tests. In this case, the renewal calls end up creating dirty state
for the subsequent test, which can cause confusing behavior (such as
attempting to register an account twice.)

Another solution to this problem would be introducing a bool, protected
by renewalMu, which indicates if renewal has been halted, and to check
it in startRenew to check if stopRenew has already been called, which
would allow us to continue calling startRenew in a goroutine and relying
on renewalMu locking for ordering. That said I don't see a particularly
strong reason to call startRenew concurrently, so this seems like the
simplest solution for now.

Fixes golang/go#52494

Change-Id: I95420d3fd877572a0b9e408d2f8cd353f6a4e80e
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/433016
TryBot-Result: Gopher Robot <go...@golang.org>
Run-TryBot: Roland Shoemaker <rol...@golang.org>
Reviewed-by: Bryan Mills <bcm...@google.com>
---
M acme/autocert/autocert.go
M acme/autocert/autocert_test.go
2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
index 0061c28..6b4cdf4 100644
--- a/acme/autocert/autocert.go
+++ b/acme/autocert/autocert.go
@@ -463,7 +463,7 @@
 		leaf: cert.Leaf,
 	}
 	m.state[ck] = s
-	go m.startRenew(ck, s.key, s.leaf.NotAfter)
+	m.startRenew(ck, s.key, s.leaf.NotAfter)
 	return cert, nil
 }
 
@@ -609,7 +609,7 @@
 	}
 	state.cert = der
 	state.leaf = leaf
-	go m.startRenew(ck, state.key, state.leaf.NotAfter)
+	m.startRenew(ck, state.key, state.leaf.NotAfter)
 	return state.tlscert()
 }
 
diff --git a/acme/autocert/autocert_test.go b/acme/autocert/autocert_test.go
index 59f6e0e..789a441 100644
--- a/acme/autocert/autocert_test.go
+++ b/acme/autocert/autocert_test.go
@@ -382,7 +382,7 @@
 			},
 		},
 		{
-			name:   "expiredCache",
+			name:   "almostExpiredCache",
 			hello:  clientHelloInfo("example.org", algECDSA),
 			domain: "example.org",
 			prepare: func(t *testing.T, man *Manager, s *acmetest.CAServer) {

To view, visit change 433016. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crypto

Gerrit-Branch: master

Gerrit-Change-Id: I95420d3fd877572a0b9e408d2f8cd353f6a4e80e

Gerrit-Change-Number: 433016

Gerrit-PatchSet: 4

Gerrit-Owner: Roland Shoemaker <rol...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Bryan Mills <bcm...@google.com>

Gerrit-Reviewer: Gopher Robot <go...@golang.org>

Gerrit-Reviewer: Roland Shoemaker <rol...@golang.org>

Gerrit-CC: Filippo Valsorda <fil...@golang.org>

Gerrit-MessageType: merged