diff --git a/AUDIT-CONCURRENCY.md b/AUDIT-CONCURRENCY.md new file mode 100644 index 0000000..5116b86 --- /dev/null +++ b/AUDIT-CONCURRENCY.md @@ -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. diff --git a/pkg/crypt/crypt.go b/pkg/crypt/crypt.go index 02dd66c..c33004b 100644 --- a/pkg/crypt/crypt.go +++ b/pkg/crypt/crypt.go @@ -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. diff --git a/pkg/crypt/race_test.go b/pkg/crypt/race_test.go new file mode 100644 index 0000000..5878be9 --- /dev/null +++ b/pkg/crypt/race_test.go @@ -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() +}