luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

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 <alex.scheel@hashicorp.com>

* 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 <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

---------

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This commit is contained in:
Alexander Scheel 2023-01-30 14:13:40 -05:00 committed by GitHub
parent 9d47c4b779
commit 2b9a8c6c49
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 7 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
builtin/logical/pki/backend_test.go
View File

@ -5641,11 +5641,11 @@ func TestBackend_InitializeCertificateCounts(t *testing.T) {
b.initializeStoredCertificateCounts(ctx) b.initializeStoredCertificateCounts(ctx)
// Test certificate count // 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)) 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)) t.Fatalf("Failed to count revoked certificates A,B,C,D counted %d certs", *(b.revokedCertCount))
} }

4
builtin/logical/pki/path_tidy.go
View File

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

3
changelog/18899.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:bug
secrets/pki: fix race between tidy's cert counting and tidy status reporting.
```