From 2b9a8c6c49c5472b3d95b390ba68e7d0b7f19997 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 30 Jan 2023 14:13:40 -0500 Subject: [PATCH] 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() :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() :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 * 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 * Add changelog entry Signed-off-by: Alexander Scheel --------- Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 4 ++-- builtin/logical/pki/path_tidy.go | 4 ++-- changelog/18899.txt | 3 +++ 3 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 changelog/18899.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index aef9d9451..51f986b53 100644 --- 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 index b8902fca0..8bd22da10 100644 --- 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 new file mode 100644 index 000000000..92f2474ed --- /dev/null +++ b/changelog/18899.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: fix race between tidy's cert counting and tidy status reporting. +```