Fix race in tidy status with cert counting (#18899)

* Read total cert counts with atomic.LoadUint32(...) When generating the tidy status, we read the values of two backend atomics, b.certCount and b.revokedCertCount, without using the atomic load operation. This resulted in a data race when the status was read at the same time as an on-going tidy operation: WARNING: DATA RACE Write at 0x00c00c77680c by goroutine 90522: sync/atomic.AddInt32() /usr/local/go/src/runtime/race_amd64.s:281 +0xb sync/atomic.AddUint32() <autogenerated>:1 +0x1a github.com/hashicorp/vault/builtin/logical/pki.(*backend).tidyStatusIncRevokedCertCount() /home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:1236 +0x107 github.com/hashicorp/vault/builtin/logical/pki.(*backend).doTidyRevocationStore() /home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:525 +0x1404 github.com/hashicorp/vault/builtin/logical/pki.(*backend).startTidyOperation.func1.1() /home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:290 +0x1a4 github.com/hashicorp/vault/builtin/logical/pki.(*backend).startTidyOperation.func1() /home/circleci/go/src/github.com/hashicorp/vault/builtin/logical/pki/path_tidy.go:342 +0x278 Previous read at 0x00c00c77680c by goroutine 90528: reflect.Value.Uint() /usr/local/go/src/reflect/value.go:2584 +0x195 encoding/json.uintEncoder() /usr/local/go/src/encoding/json/encode.go:562 +0x45 encoding/json.ptrEncoder.encode() /usr/local/go/src/encoding/json/encode.go:944 +0x3c2 encoding/json.ptrEncoder.encode-fm() <autogenerated>:1 +0x90 encoding/json.(*encodeState).reflectValue() /usr/local/go/src/encoding/json/encode.go:359 +0x88 encoding/json.interfaceEncoder() /usr/local/go/src/encoding/json/encode.go:715 +0x17b encoding/json.mapEncoder.encode() /usr/local/go/src/encoding/json/encode.go:813 +0x854 ... more stack trace pointing into JSON encoding and http handler... In particular, because the tidy status was directly reading the uint value without resorting to the atomic side, the JSON serialization could race with a later atomic update. Signed-off-by: Alexander Scheel <[email protected]> * Also use atomic load in tests Because no tidy operation is running here, it should be safe to read the pointed value directly, but use the safer atomic.Load for consistency. Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]> --------- Signed-off-by: Alexander Scheel <[email protected]>
hashicorp · Jan 30, 2023 · d39fef0 · d39fef0
1 parent cd70976
commit d39fef0
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 4 deletions.
diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go
@@ -5641,11 +5641,11 @@ func TestBackend_InitializeCertificateCounts(t *testing.T) {
 	b.initializeStoredCertificateCounts(ctx)
 
 	// Test certificate count
-	if *(b.certCount) != 8 {
+	if atomic.LoadUint32(b.certCount) != 8 {
 		t.Fatalf("Failed to initialize count of certificates root, A,B,C,D,E,F,G counted %d certs", *(b.certCount))
 	}
 
-	if *(b.revokedCertCount) != 4 {
+	if atomic.LoadUint32(b.revokedCertCount) != 4 {
 		t.Fatalf("Failed to count revoked certificates A,B,C,D counted %d certs", *(b.revokedCertCount))
 	}
 

diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go
@@ -1020,8 +1020,8 @@ func (b *backend) pathTidyStatusRead(_ context.Context, _ *logical.Request, _ *f
 		resp.Data["time_finished"] = b.tidyStatus.timeFinished
 	}
 
-	resp.Data["current_cert_store_count"] = b.certCount
-	resp.Data["current_revoked_cert_count"] = b.revokedCertCount
+	resp.Data["current_cert_store_count"] = atomic.LoadUint32(b.certCount)
+	resp.Data["current_revoked_cert_count"] = atomic.LoadUint32(b.revokedCertCount)
 
 	if !b.certsCounted.Load() {
 		resp.AddWarning("Certificates in storage are still being counted, current counts provided may be " +

diff --git a/changelog/18899.txt b/changelog/18899.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: fix race between tidy's cert counting and tidy status reporting.
+```