From 9404c13f91b762339b724dba9b4577d39bdf9b56 Mon Sep 17 00:00:00 2001 From: John Sell Date: Fri, 5 Jun 2026 13:03:29 -0400 Subject: [PATCH 1/3] fix(api-server): address coderabbit review on encryption implementation - Track success counts separately from candidates in CLI summary - Reject duplicate key versions that normalize to same integer (e.g. "1" vs "01") - Assert actual plaintext value in API token test, not just shape - Check DAO errors in row-swap test instead of swallowing them - Fail closed when encrypted-token probe hits DB errors - Guard s.events nil in Replace and Delete (not just Create) Co-Authored-By: Claude Opus 4.6 (1M context) --- .../pkg/cmd/encrypt_credentials.go | 21 +++++++------ .../ambient-api-server/pkg/crypto/encrypt.go | 5 ++++ .../plugins/credentials/encryption.go | 8 ++--- .../encryption_integration_test.go | 19 ++++++++---- .../plugins/credentials/service.go | 30 +++++++++++-------- 5 files changed, 52 insertions(+), 31 deletions(-) diff --git a/components/ambient-api-server/pkg/cmd/encrypt_credentials.go b/components/ambient-api-server/pkg/cmd/encrypt_credentials.go index 270da79d96..c54038f26b 100644 --- a/components/ambient-api-server/pkg/cmd/encrypt_credentials.go +++ b/components/ambient-api-server/pkg/cmd/encrypt_credentials.go @@ -79,7 +79,8 @@ func runEncrypt(db *gorm.DB, keyring *crypto.Keyring, activeVersion int, dryRun glog.Fatalf("Failed to query credentials: %v", err) } - var plaintext, reencrypt, current, failed int + var plaintextCandidates, reencryptCandidates, current int + var plaintextSuccess, reencryptSuccess, failed int var failedIDs []string for _, row := range rows { @@ -90,7 +91,7 @@ func runEncrypt(db *gorm.DB, keyring *crypto.Keyring, activeVersion int, dryRun token := *row.Token if !crypto.IsEncrypted(token) { - plaintext++ + plaintextCandidates++ if dryRun { continue } @@ -107,13 +108,14 @@ func runEncrypt(db *gorm.DB, keyring *crypto.Keyring, activeVersion int, dryRun fmt.Fprintf(os.Stderr, "ERROR: update %s: %v\n", row.ID, err) continue } + plaintextSuccess++ } else { v, _ := crypto.TokenVersion(token) if v == activeVersion { current++ continue } - reencrypt++ + reencryptCandidates++ if dryRun { continue } @@ -137,24 +139,25 @@ func runEncrypt(db *gorm.DB, keyring *crypto.Keyring, activeVersion int, dryRun fmt.Fprintf(os.Stderr, "ERROR: update %s: %v\n", row.ID, err) continue } + reencryptSuccess++ } } if dryRun { - fmt.Printf("Would encrypt: %d plaintext, Would re-encrypt: %d (→ v%d), Already current: %d\n", plaintext, reencrypt, activeVersion, current) + fmt.Printf("Would encrypt: %d plaintext, Would re-encrypt: %d (→ v%d), Already current: %d\n", plaintextCandidates, reencryptCandidates, activeVersion, current) return } - if plaintext+reencrypt == 0 && failed == 0 { + if plaintextSuccess+reencryptSuccess == 0 && failed == 0 { fmt.Println("0 credentials need encryption. All up to date.") return } - if plaintext > 0 { - fmt.Printf("%d credentials encrypted (plaintext → v%d)\n", plaintext-failed, activeVersion) + if plaintextSuccess > 0 { + fmt.Printf("%d credentials encrypted (plaintext → v%d)\n", plaintextSuccess, activeVersion) } - if reencrypt > 0 { - fmt.Printf("%d credentials re-encrypted to v%d\n", reencrypt, activeVersion) + if reencryptSuccess > 0 { + fmt.Printf("%d credentials re-encrypted to v%d\n", reencryptSuccess, activeVersion) } if failed > 0 { fmt.Fprintf(os.Stderr, "%d credentials failed: %v\n", failed, failedIDs) diff --git a/components/ambient-api-server/pkg/crypto/encrypt.go b/components/ambient-api-server/pkg/crypto/encrypt.go index 7f81d81389..f18cec2c45 100644 --- a/components/ambient-api-server/pkg/crypto/encrypt.go +++ b/components/ambient-api-server/pkg/crypto/encrypt.go @@ -28,11 +28,16 @@ func NewKeyring(keys map[string]string, activeVersion int) (*Keyring, error) { } aeadMap := make(map[int]cipher.AEAD, len(keys)) + seen := make(map[int]string, len(keys)) for vStr, encoded := range keys { v, err := strconv.Atoi(vStr) if err != nil { return nil, fmt.Errorf("invalid key version %q: %w", vStr, err) } + if prev, exists := seen[v]; exists { + return nil, fmt.Errorf("duplicate key version %d: %q conflicts with %q", v, vStr, prev) + } + seen[v] = vStr raw, err := base64.StdEncoding.DecodeString(encoded) if err != nil { return nil, fmt.Errorf("key version %d: invalid base64: %w", v, err) diff --git a/components/ambient-api-server/plugins/credentials/encryption.go b/components/ambient-api-server/plugins/credentials/encryption.go index 498f84599d..9251e3289a 100644 --- a/components/ambient-api-server/plugins/credentials/encryption.go +++ b/components/ambient-api-server/plugins/credentials/encryption.go @@ -64,8 +64,8 @@ func ValidateEncryptionStartup(db *gorm.DB, keyring *crypto.Keyring) { var count int64 if err := db.Table("credentials").Where("token LIKE 'enc:v%'").Count(&count).Error; err != nil { - fmt.Fprintf(os.Stderr, "WARNING: could not check for encrypted tokens: %v\n", err) - return + fmt.Fprintf(os.Stderr, "FATAL: could not check for encrypted tokens: %v\n", err) + os.Exit(1) } if count > 0 { @@ -81,8 +81,8 @@ func ValidateEncryptionStartupFromDAO(ctx context.Context, dao CredentialDao, ke all, err := dao.All(ctx) if err != nil { - fmt.Fprintf(os.Stderr, "WARNING: could not check for encrypted tokens: %v\n", err) - return + fmt.Fprintf(os.Stderr, "FATAL: could not check for encrypted tokens: %v\n", err) + os.Exit(1) } for _, c := range all { diff --git a/components/ambient-api-server/plugins/credentials/encryption_integration_test.go b/components/ambient-api-server/plugins/credentials/encryption_integration_test.go index db7402301a..bb53a3a34b 100644 --- a/components/ambient-api-server/plugins/credentials/encryption_integration_test.go +++ b/components/ambient-api-server/plugins/credentials/encryption_integration_test.go @@ -3,6 +3,7 @@ package credentials_test import ( "context" "encoding/base64" + "encoding/json" "fmt" "net/http" "strings" @@ -113,9 +114,13 @@ func TestEncryptedCredentialViaAPI(t *testing.T) { Expect(restyResp.StatusCode()).To(Equal(http.StatusOK)) // API returns plaintext (encryption is transparent) - body := restyResp.String() - Expect(body).To(ContainSubstring(`"token"`)) - Expect(body).NotTo(ContainSubstring("enc:v1:"), "API must not return ciphertext") + var tokenResponse struct { + Token string `json:"token"` + } + err = json.Unmarshal(restyResp.Body(), &tokenResponse) + Expect(err).NotTo(HaveOccurred()) + Expect(tokenResponse.Token).To(Equal(*credentialInput.Token), "GET /token must return the original plaintext token") + Expect(tokenResponse.Token).NotTo(HavePrefix("enc:v1:"), "API must not return ciphertext") } func TestPlaintextTokenPassthrough(t *testing.T) { @@ -178,8 +183,12 @@ func TestAADPreventsRowSwap(t *testing.T) { cred2 := newEncryptedCredential(t, h.NewID(), "secret_B") dao := credentials.NewCredentialDao(&environments.Environment().Database.SessionFactory) - raw1, _ := dao.Get(context.Background(), cred1.ID) - raw2, _ := dao.Get(context.Background(), cred2.ID) + raw1, err := dao.Get(context.Background(), cred1.ID) + Expect(err).NotTo(HaveOccurred()) + Expect(raw1.Token).NotTo(BeNil()) + raw2, err := dao.Get(context.Background(), cred2.ID) + Expect(err).NotTo(HaveOccurred()) + Expect(raw2.Token).NotTo(BeNil()) // Swap ciphertexts kr := testKeyring(t) diff --git a/components/ambient-api-server/plugins/credentials/service.go b/components/ambient-api-server/plugins/credentials/service.go index 1904703fd1..d1e128a374 100644 --- a/components/ambient-api-server/plugins/credentials/service.go +++ b/components/ambient-api-server/plugins/credentials/service.go @@ -170,13 +170,15 @@ func (s *sqlCredentialService) Replace(ctx context.Context, credential *Credenti return nil, services.HandleUpdateError("Credential", err) } - _, evErr := s.events.Create(ctx, &api.Event{ - Source: "Credentials", - SourceID: credential.ID, - EventType: api.UpdateEventType, - }) - if evErr != nil { - return nil, services.HandleUpdateError("Credential", evErr) + if s.events != nil { + _, evErr := s.events.Create(ctx, &api.Event{ + Source: "Credentials", + SourceID: credential.ID, + EventType: api.UpdateEventType, + }) + if evErr != nil { + return nil, services.HandleUpdateError("Credential", evErr) + } } return credential, nil @@ -187,12 +189,14 @@ func (s *sqlCredentialService) Delete(ctx context.Context, id string) *errors.Se return services.HandleDeleteError("Credential", errors.GeneralError("Unable to delete credential: %s", err)) } - if _, evErr := s.events.Create(ctx, &api.Event{ - Source: "Credentials", - SourceID: id, - EventType: api.DeleteEventType, - }); evErr != nil { - logger.NewLogger(ctx).Warning(fmt.Sprintf("Credential %s deleted but event creation failed: %v", id, evErr)) + if s.events != nil { + if _, evErr := s.events.Create(ctx, &api.Event{ + Source: "Credentials", + SourceID: id, + EventType: api.DeleteEventType, + }); evErr != nil { + logger.NewLogger(ctx).Warning(fmt.Sprintf("Credential %s deleted but event creation failed: %v", id, evErr)) + } } return nil From 51873d044331bf05efcc622385ad9195a21c8cee Mon Sep 17 00:00:00 2001 From: John Sell Date: Fri, 5 Jun 2026 13:05:57 -0400 Subject: [PATCH 2/3] test(api-server): improve crypto package coverage to 93.8% MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add tests for Versions(), TokenVersion(), and duplicate key version rejection ("1" vs "01"). Coverage: 82.5% → 93.8%. Remaining gaps are stdlib error paths (broken rand.Reader, impossible AES/GCM failures). Co-Authored-By: Claude Opus 4.6 (1M context) --- .../pkg/crypto/encrypt_test.go | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/components/ambient-api-server/pkg/crypto/encrypt_test.go b/components/ambient-api-server/pkg/crypto/encrypt_test.go index 0ccbb944b1..4335caed0c 100644 --- a/components/ambient-api-server/pkg/crypto/encrypt_test.go +++ b/components/ambient-api-server/pkg/crypto/encrypt_test.go @@ -280,6 +280,74 @@ func TestEncrypt_EmptyPlaintext(t *testing.T) { } } +func TestVersions(t *testing.T) { + key1 := testKey() + key2 := make([]byte, 32) + for i := range key2 { + key2[i] = byte(i + 50) + } + + kr, _ := NewKeyring(map[string]string{ + "1": base64.StdEncoding.EncodeToString(key1), + "3": base64.StdEncoding.EncodeToString(key2), + }, 1) + + versions := kr.Versions() + if len(versions) != 2 { + t.Fatalf("expected 2 versions, got %d", len(versions)) + } + has1, has3 := false, false + for _, v := range versions { + if v == 1 { + has1 = true + } + if v == 3 { + has3 = true + } + } + if !has1 || !has3 { + t.Fatalf("expected versions [1, 3], got %v", versions) + } +} + +func TestTokenVersion_Valid(t *testing.T) { + kr := testKeyring(t) + ct, _ := kr.Encrypt("secret", "cred-001") + + v, ok := TokenVersion(ct) + if !ok { + t.Fatal("expected ok=true for valid ciphertext") + } + if v != 1 { + t.Fatalf("expected version 1, got %d", v) + } +} + +func TestTokenVersion_Plaintext(t *testing.T) { + _, ok := TokenVersion("ghp_abc123") + if ok { + t.Fatal("expected ok=false for plaintext") + } +} + +func TestNewKeyring_DuplicateVersions(t *testing.T) { + key := base64.StdEncoding.EncodeToString(testKey()) + key2 := make([]byte, 32) + for i := range key2 { + key2[i] = byte(i + 99) + } + _, err := NewKeyring(map[string]string{ + "1": key, + "01": base64.StdEncoding.EncodeToString(key2), + }, 1) + if err == nil { + t.Fatal("expected error for duplicate version (1 and 01)") + } + if !strings.Contains(err.Error(), "duplicate key version") { + t.Fatalf("expected duplicate error, got: %v", err) + } +} + func TestEncrypt_LargePayload(t *testing.T) { kr := testKeyring(t) large := strings.Repeat("a]kubeconfig-content-here[", 1000) From c89d104d75d25d38bac9e0d442cffeb2d8708104 Mon Sep 17 00:00:00 2001 From: John Sell Date: Fri, 5 Jun 2026 13:15:43 -0400 Subject: [PATCH 3/3] fix(api-server): table-driven subtests and error handling in crypto tests Address coderabbit review on PR #1657: - Convert TokenVersion tests to table-driven subtests with 5 cases - Check errors from NewKeyring and Encrypt instead of discarding them Co-Authored-By: Claude Opus 4.6 (1M context) --- .../pkg/crypto/encrypt_test.go | 46 +++++++++++++------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/components/ambient-api-server/pkg/crypto/encrypt_test.go b/components/ambient-api-server/pkg/crypto/encrypt_test.go index 4335caed0c..8822d84688 100644 --- a/components/ambient-api-server/pkg/crypto/encrypt_test.go +++ b/components/ambient-api-server/pkg/crypto/encrypt_test.go @@ -287,10 +287,13 @@ func TestVersions(t *testing.T) { key2[i] = byte(i + 50) } - kr, _ := NewKeyring(map[string]string{ + kr, err := NewKeyring(map[string]string{ "1": base64.StdEncoding.EncodeToString(key1), "3": base64.StdEncoding.EncodeToString(key2), }, 1) + if err != nil { + t.Fatalf("NewKeyring: %v", err) + } versions := kr.Versions() if len(versions) != 2 { @@ -310,23 +313,36 @@ func TestVersions(t *testing.T) { } } -func TestTokenVersion_Valid(t *testing.T) { +func TestTokenVersion(t *testing.T) { kr := testKeyring(t) - ct, _ := kr.Encrypt("secret", "cred-001") - - v, ok := TokenVersion(ct) - if !ok { - t.Fatal("expected ok=true for valid ciphertext") - } - if v != 1 { - t.Fatalf("expected version 1, got %d", v) + ct, err := kr.Encrypt("secret", "cred-001") + if err != nil { + t.Fatalf("Encrypt: %v", err) } -} -func TestTokenVersion_Plaintext(t *testing.T) { - _, ok := TokenVersion("ghp_abc123") - if ok { - t.Fatal("expected ok=false for plaintext") + tests := []struct { + name string + input string + wantVer int + wantOK bool + }{ + {"valid ciphertext", ct, 1, true}, + {"plaintext PAT", "ghp_abc123", 0, false}, + {"empty string", "", 0, false}, + {"partial prefix", "enc:v1", 0, false}, + {"non-integer version", "enc:vX:data", 0, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + v, ok := TokenVersion(tc.input) + if ok != tc.wantOK { + t.Fatalf("TokenVersion(%q): ok=%v, want %v", tc.input, ok, tc.wantOK) + } + if v != tc.wantVer { + t.Fatalf("TokenVersion(%q): version=%d, want %d", tc.input, v, tc.wantVer) + } + }) } }