fix(crypt): resolve race condition in service initialization
A data race was identified in the lazy initialization of the RSA and PGP services within the `crypt` package. The non-thread-safe `if s.service == nil` check could lead to multiple initializations when accessed concurrently. This commit resolves the race condition by using `sync.Once` to ensure that the initialization for each service is performed exactly once, making the `Service` struct safe for concurrent use. Additionally, a new test file, `race_test.go`, has been added to provide a regression test for this specific scenario. A new file, `AUDIT-CONCURRENCY.md`, has been created to document the findings of the concurrency audit, the remediation steps taken, and the verification process. Co-authored-by: Snider <631881+Snider@users.noreply.github.com>
This commit is contained in:
parent
86f4e33b1a
commit
7c2949a939
3 changed files with 78 additions and 5 deletions
43
AUDIT-CONCURRENCY.md
Normal file
43
AUDIT-CONCURRENCY.md
Normal file
|
|
@ -0,0 +1,43 @@
|
|||
# Concurrency and Race Condition Analysis (`AUDIT-CONCURRENCY.md`)
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
A concurrency audit was performed on the codebase to identify potential race conditions and thread-safety issues. The audit focused on encryption operations, connection pooling, cache synchronization, and key management.
|
||||
|
||||
A critical data race was identified in the lazy initialization of the RSA and PGP services within the `pkg/crypt` package. This could lead to unpredictable behavior and resource leaks under concurrent use.
|
||||
|
||||
The issue was resolved by implementing thread-safe initialization using the `sync.Once` primitive from the Go standard library.
|
||||
|
||||
## 2. Methodology
|
||||
|
||||
The audit consisted of the following steps:
|
||||
|
||||
1. **Static Analysis:** A manual review of the codebase was conducted, focusing on areas with high potential for concurrency issues, such as the `pkg/crypt` package.
|
||||
2. **Dynamic Analysis:** The Go race detector (`go test -race ./...`) was used to identify data races during test execution. A new test case was created to specifically trigger the suspected race condition.
|
||||
|
||||
## 3. Findings
|
||||
|
||||
### 3.1. Data Race in Lazy Initialization
|
||||
|
||||
- **Location:** `pkg/crypt/crypt.go`, in the `ensureRSA()` and `ensurePGP()` methods.
|
||||
- **Description:** The `ensureRSA()` and `ensurePGP()` methods used a non-thread-safe `if s.rsa == nil` check for lazy initialization. This created a race condition where multiple goroutines could enter the initialization block simultaneously, leading to multiple service instantiations and overwriting the service pointer.
|
||||
- **Impact:** This could cause unpredictable behavior, memory leaks, and potentially panics if the service initialization had side effects.
|
||||
- **Reproduction:** The race condition was reliably reproduced by creating a new test case (`pkg/crypt/race_test.go`) that called the `ensure` methods from multiple goroutines in parallel.
|
||||
|
||||
## 4. Remediation
|
||||
|
||||
The data race was remediated by using `sync.Once` to ensure that the initialization of the `rsa.Service` and `pgp.Service` happens exactly once, even when called from multiple goroutines.
|
||||
|
||||
The following changes were made to `pkg/crypt/crypt.go`:
|
||||
|
||||
1. Added `rsaOnce sync.Once` and `pgpOnce sync.Once` fields to the `Service` struct.
|
||||
2. Modified `ensureRSA()` to use `s.rsaOnce.Do()`.
|
||||
3. Modified `ensurePGP()` to use `s.pgpOnce.Do()`.
|
||||
|
||||
## 5. Verification
|
||||
|
||||
The fix was verified by running the test suite with the race detector (`go test -race ./...`). The tests passed without any race warnings, confirming that the fix is effective. The new test case in `pkg/crypt/race_test.go` now serves as a regression test for this specific issue.
|
||||
|
||||
## 6. Conclusion
|
||||
|
||||
The audit identified and remediated a critical data race in the `pkg/crypt` package. The codebase is now more robust and safe for concurrent use. No other concurrency issues were identified in the audited areas.
|
||||
|
|
@ -3,13 +3,14 @@ package crypt
|
|||
import (
|
||||
"crypto/md5"
|
||||
"crypto/sha1"
|
||||
"errors"
|
||||
"crypto/sha256"
|
||||
"crypto/sha512"
|
||||
"encoding/binary"
|
||||
"encoding/hex"
|
||||
"errors"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
|
||||
"github.com/Snider/Enchantrix/pkg/crypt/std/lthn"
|
||||
"github.com/Snider/Enchantrix/pkg/crypt/std/pgp"
|
||||
|
|
@ -21,6 +22,9 @@ import (
|
|||
type Service struct {
|
||||
rsa *rsa.Service
|
||||
pgp *pgp.Service
|
||||
|
||||
rsaOnce sync.Once
|
||||
pgpOnce sync.Once
|
||||
}
|
||||
|
||||
// NewService creates a new crypt Service and initialises its embedded services.
|
||||
|
|
@ -165,9 +169,9 @@ func (s *Service) Fletcher64(payload string) uint64 {
|
|||
|
||||
// ensureRSA initializes the RSA service if it is not already.
|
||||
func (s *Service) ensureRSA() {
|
||||
if s.rsa == nil {
|
||||
s.rsaOnce.Do(func() {
|
||||
s.rsa = rsa.NewService()
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// GenerateRSAKeyPair creates a new RSA key pair.
|
||||
|
|
@ -192,9 +196,9 @@ func (s *Service) DecryptRSA(privateKey, ciphertext, label []byte) ([]byte, erro
|
|||
|
||||
// ensurePGP initializes the PGP service if it is not already.
|
||||
func (s *Service) ensurePGP() {
|
||||
if s.pgp == nil {
|
||||
s.pgpOnce.Do(func() {
|
||||
s.pgp = pgp.NewService()
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// GeneratePGPKeyPair creates a new PGP key pair.
|
||||
|
|
|
|||
26
pkg/crypt/race_test.go
Normal file
26
pkg/crypt/race_test.go
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
package crypt
|
||||
|
||||
import (
|
||||
"sync"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestServiceRace(t *testing.T) {
|
||||
s := &Service{}
|
||||
var wg sync.WaitGroup
|
||||
|
||||
//These goroutines will race to initialize the pgp and rsa services.
|
||||
for i := 0; i < 10; i++ {
|
||||
wg.Add(2)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
s.ensurePGP()
|
||||
}()
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
s.ensureRSA()
|
||||
}()
|
||||
}
|
||||
|
||||
wg.Wait()
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue