Bundle Q (Coverage Audit Closure): property-based pilot + hygiene — L-001/L-002/L-003/L-004/I-001 closed

Five small closures wrapping the Low-tier and Info-tier audit findings.

Q.1 — cmd/cli round-out (L-001 closed)
======================================
cmd/cli/dispatch_test.go: ~30 dispatch tests across handleCerts /
handleAgents / handleJobs / handleImport / handleStatus. httptest.NewTLSServer
mocks the API; cli.NewClient(_, _, _, _, true) constructs an
insecure-skip-verify client. Each test pins the missing-args usage-print
path AND the happy-path delegation. Result: 7.1% -> 63.5% coverage
(gate: >=30%).

Q.2 — awssm round-out (L-002 closed)
======================================
internal/connector/discovery/awssm/awssm_edge_test.go: New() default
constructor, extractKeyInfo (ECDSA/Ed25519/unknown — was RSA-only),
processSecret filter arms (NamePrefix mismatch / TagFilter mismatch /
empty-value / GetSecretValue error), realSMClient stub-contract pin
(ListSecrets / GetSecretValue / NewRealSMClient), and EmailAddresses
SAN extraction. Result: 78.2% -> 96.0% coverage (gate: >=85%).

Q.3 — Property-based testing pilot (L-003 closed)
======================================
gopter@v0.2.11 added to go.mod (test-only).

internal/crypto/encryption_property_test.go:
- TestProperty_EncryptDecryptRoundTrip — 50 successful tests,
  DecryptIfKeySet(EncryptIfKeySet(x, k), k) == x
- TestProperty_WrongPassphraseRejected — 30 successful tests,
  AEAD never returns nil-error AND bytes-equal plaintext under
  wrong passphrase
Both skipped under -short to keep developer loop fast (PBKDF2 600k
rounds × 50 iters ≈ 15s on -race CI).

internal/pkcs7/length_property_test.go:
- TestProperty_ASN1LengthRoundTrip — three sub-properties:
  decodeLength(encode(x)) == x for x ∈ [0, 2³¹−1]; short-form
  invariant (length<128 → 1 byte == length); long-form invariant
  (length>=128 → high bit set + N bytes follow). 500 successful
  tests in <10ms.

Q.4 — Architecture diagram multi-agent update (L-004 closed)
======================================
docs/qa-test-guide.md::Architecture: ASCII diagram updated to show
'certctl-agent (×N)' + callout explaining seed_demo.sql provisions
12 agent rows (1 active, 2 retired, 9 reserved/sentinel) for Parts
04, 05, 55 + FSM coverage. Operators running parallel-agent topologies
guided to AGENT_COUNT=N + 'make qa-stats'.

Q.5 — Test-naming CI guard (I-001 closed)
======================================
.github/workflows/ci.yml: Test-naming convention guard added after
the QA-doc seed-count drift guard. Greps for func Test<X>( missing
the <X>_<Scenario> suffix. Prints first 20 non-conformant as
::warning:: annotations. continue-on-error: true (informational).
Excludes TestMain + TestProperty_*. Promotion to hard-fail tracked
as I-001-extended.

Verification
======================================
- python3 yaml.safe_load on ci.yml: OK
- go vet ./cmd/cli/... ./internal/connector/discovery/awssm/...
  ./internal/crypto/... ./internal/pkcs7/...: clean
- go test -short -count=1 across all four packages: PASS
- go test -count=1 (full property tests): PASS
  - crypto 15.4s (50 + 30 × 600k PBKDF2)
  - pkcs7 5ms

Audit deliverables
======================================
- gap-backlog.md: strikethroughs on L-001/L-002/L-003/L-004/I-001
  with per-finding closure note
- closure-plan.md: ticks Bundle Q [x] with per-item breakdown

Closes: L-001, L-002, L-003, L-004, I-001
Bundle: Q (Property-Based + Hygiene)
This commit is contained in:
shankar0123
2026-04-27 18:36:47 +00:00
parent 9383b2ce35
commit 95d0d85391
9 changed files with 1625 additions and 19 deletions
@@ -0,0 +1,329 @@
package awssm
import (
"context"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/elliptic"
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"crypto/x509/pkix"
"errors"
"log/slog"
"math/big"
"testing"
"time"
"github.com/shankar0123/certctl/internal/config"
)
// Bundle Q (L-002 closure): edge-case coverage for awssm to push above 80%.
//
// Adds tests for:
//
// - New() default-constructor path (was 0%): nil config, nil logger, normal path
// - NewWithClient() default-arg paths
// - extractKeyInfo for ECDSA + Ed25519 + unknown key types (was RSA-only)
// - processSecret's NamePrefix filter and TagFilter mismatch skip arms
// - realSMClient stub methods (ListSecrets / GetSecretValue) — pin the
// "documented stub returns empty + no error" contract so a future
// refactor that swaps in real SDK calls without updating callers is
// caught immediately
// - ValidateConfig nil-config branch
func TestNew_NilConfig_PopulatesDefaults(t *testing.T) {
src := New(nil, slog.Default())
if src == nil {
t.Fatal("New(nil, _) returned nil source")
}
if src.cfg == nil {
t.Errorf("expected New to populate empty config when nil supplied")
}
}
func TestNew_NilLogger_PopulatesDefaults(t *testing.T) {
cfg := &config.AWSSecretsMgrDiscoveryConfig{Region: "us-east-1"}
src := New(cfg, nil)
if src == nil {
t.Fatal("New(_, nil) returned nil source")
}
if src.logger == nil {
t.Errorf("expected New to populate default logger when nil supplied")
}
}
func TestNew_NormalPath_CreatesSource(t *testing.T) {
cfg := &config.AWSSecretsMgrDiscoveryConfig{Region: "us-west-2"}
src := New(cfg, slog.Default())
if src == nil {
t.Fatal("New returned nil")
}
if src.client == nil {
t.Errorf("expected New to wire up a real SM client")
}
// Sanity: real client should be a *realSMClient pointing at us-west-2.
rc, ok := src.client.(*realSMClient)
if !ok {
t.Fatalf("expected *realSMClient, got %T", src.client)
}
if rc.region != "us-west-2" {
t.Errorf("expected region us-west-2, got %q", rc.region)
}
}
func TestNewWithClient_NilConfig_NilLogger_PopulatesDefaults(t *testing.T) {
mock := newMockSMClient()
src := NewWithClient(nil, mock, nil)
if src == nil {
t.Fatal("NewWithClient returned nil")
}
if src.cfg == nil || src.logger == nil {
t.Errorf("expected NewWithClient to populate cfg + logger defaults")
}
}
func TestValidateConfig_NilConfig_FailsClosed(t *testing.T) {
src := &Source{} // explicit nil cfg
if err := src.ValidateConfig(); err == nil {
t.Errorf("expected ValidateConfig to fail when cfg is nil")
}
}
// ─────────────────────────────────────────────────────────────────────────────
// extractKeyInfo: every key-type arm.
// ─────────────────────────────────────────────────────────────────────────────
func TestExtractKeyInfo_RSA(t *testing.T) {
key, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
t.Fatalf("rsa.GenerateKey: %v", err)
}
cert := &x509.Certificate{PublicKey: &key.PublicKey}
algo, size := extractKeyInfo(cert)
if algo != "RSA" {
t.Errorf("expected RSA, got %q", algo)
}
if size != 2048 {
t.Errorf("expected size 2048, got %d", size)
}
}
func TestExtractKeyInfo_ECDSA(t *testing.T) {
key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
if err != nil {
t.Fatalf("ecdsa.GenerateKey: %v", err)
}
cert := &x509.Certificate{PublicKey: &key.PublicKey}
algo, size := extractKeyInfo(cert)
if algo != "ECDSA" {
t.Errorf("expected ECDSA, got %q", algo)
}
if size != 384 {
t.Errorf("expected size 384 (P-384 curve), got %d", size)
}
}
func TestExtractKeyInfo_Ed25519(t *testing.T) {
pub, _, err := ed25519.GenerateKey(rand.Reader)
if err != nil {
t.Fatalf("ed25519.GenerateKey: %v", err)
}
cert := &x509.Certificate{PublicKey: pub}
algo, size := extractKeyInfo(cert)
if algo != "Ed25519" {
t.Errorf("expected Ed25519, got %q", algo)
}
if size != 256 {
t.Errorf("expected size 256, got %d", size)
}
}
func TestExtractKeyInfo_Unknown(t *testing.T) {
// PublicKey type that's none of the known cases → falls through to default.
cert := &x509.Certificate{PublicKey: struct{ X int }{42}}
algo, size := extractKeyInfo(cert)
if algo != "Unknown" {
t.Errorf("expected Unknown, got %q", algo)
}
if size != 0 {
t.Errorf("expected size 0 for unknown, got %d", size)
}
}
// ─────────────────────────────────────────────────────────────────────────────
// processSecret: filter arms.
// ─────────────────────────────────────────────────────────────────────────────
func TestProcessSecret_NamePrefixMismatch_SkipsSilently(t *testing.T) {
// L-002: NamePrefix-mismatched secret must be silently skipped (no error,
// no entry added, no GetSecretValue call). This exercises the prefix
// short-circuit that previously sat on the un-tested side of the branch.
mock := newMockSMClient()
mock.secrets["other/cert"] = "ignored-value"
mock.secretMetadata["other/cert"] = SecretMetadata{Name: "other/cert"}
cfg := &config.AWSSecretsMgrDiscoveryConfig{
Region: "us-east-1",
NamePrefix: "prod/", // "other/cert" doesn't start with "prod/"
}
src := NewWithClient(cfg, mock, slog.Default())
report, err := src.Discover(context.Background())
if err != nil {
t.Fatalf("Discover: %v", err)
}
if len(report.Certificates) != 0 {
t.Errorf("expected 0 certs (prefix mismatch), got %d", len(report.Certificates))
}
if len(report.Errors) != 0 {
t.Errorf("expected 0 errors, got %v", report.Errors)
}
}
func TestProcessSecret_TagFilterMismatch_SkipsSilently(t *testing.T) {
// L-002: TagFilter-mismatched secret must be silently skipped. Pins the
// branch where the secret has tags but they don't match the configured
// key=value pair.
mock := newMockSMClient()
mock.secrets["prod/cert"] = "ignored"
mock.secretMetadata["prod/cert"] = SecretMetadata{
Name: "prod/cert",
Tags: map[string]string{"type": "password"}, // mismatch: cfg wants type=certificate
}
cfg := &config.AWSSecretsMgrDiscoveryConfig{
Region: "us-east-1",
TagFilter: "type=certificate",
}
src := NewWithClient(cfg, mock, slog.Default())
report, err := src.Discover(context.Background())
if err != nil {
t.Fatalf("Discover: %v", err)
}
if len(report.Certificates) != 0 {
t.Errorf("expected 0 certs (tag mismatch), got %d", len(report.Certificates))
}
}
func TestProcessSecret_EmptyValue_Skipped(t *testing.T) {
// L-002: empty secret value short-circuits parseCertificateData and
// returns nil error.
mock := newMockSMClient()
mock.secrets["prod/empty"] = ""
mock.secretMetadata["prod/empty"] = SecretMetadata{
Name: "prod/empty",
Tags: map[string]string{"type": "certificate"},
}
cfg := &config.AWSSecretsMgrDiscoveryConfig{Region: "us-east-1"}
src := NewWithClient(cfg, mock, slog.Default())
report, err := src.Discover(context.Background())
if err != nil {
t.Fatalf("Discover: %v", err)
}
if len(report.Certificates) != 0 {
t.Errorf("expected 0 certs (empty value), got %d", len(report.Certificates))
}
}
func TestProcessSecret_GetSecretError_PropagatesToErrors(t *testing.T) {
// Round-out for processSecret: GetSecretValue error path adds to report.Errors.
mock := newMockSMClient()
mock.secretMetadata["prod/missing"] = SecretMetadata{
Name: "prod/missing",
Tags: map[string]string{"type": "certificate"},
}
mock.getErrors["prod/missing"] = errors.New("AccessDenied")
cfg := &config.AWSSecretsMgrDiscoveryConfig{Region: "us-east-1"}
src := NewWithClient(cfg, mock, slog.Default())
report, err := src.Discover(context.Background())
if err != nil {
t.Fatalf("Discover: %v", err)
}
if len(report.Errors) == 0 {
t.Errorf("expected error in report, got none")
}
}
// ─────────────────────────────────────────────────────────────────────────────
// realSMClient: stub-contract pinning.
// ─────────────────────────────────────────────────────────────────────────────
func TestRealSMClient_ListSecrets_StubReturnsEmpty(t *testing.T) {
// L-002: pin the documented stub contract. ListSecrets in the current
// implementation is a placeholder — empty slice + no error. A future
// refactor wiring up the real AWS SDK should update tests, not silently
// change return values.
c := newRealSMClient("us-east-1", slog.Default()).(*realSMClient)
got, err := c.ListSecrets(context.Background(), "tag-key:type")
if err != nil {
t.Errorf("expected nil err from stub, got %v", err)
}
if len(got) != 0 {
t.Errorf("expected empty slice from stub, got %d entries", len(got))
}
}
func TestRealSMClient_GetSecretValue_StubReturnsEmpty(t *testing.T) {
c := newRealSMClient("us-east-1", slog.Default()).(*realSMClient)
got, err := c.GetSecretValue(context.Background(), "any/secret")
if err != nil {
t.Errorf("expected nil err from stub, got %v", err)
}
if got != "" {
t.Errorf("expected empty string from stub, got %q", got)
}
}
func TestNewRealSMClient_PopulatesFields(t *testing.T) {
c := newRealSMClient("eu-west-1", slog.Default()).(*realSMClient)
if c.region != "eu-west-1" {
t.Errorf("expected region eu-west-1, got %q", c.region)
}
if c.logger == nil {
t.Errorf("expected logger to be populated")
}
}
// ─────────────────────────────────────────────────────────────────────────────
// buildDiscoveredCertEntry: edge cases on EmailAddresses-based SAN extraction.
// ─────────────────────────────────────────────────────────────────────────────
func TestBuildDiscoveredCertEntry_WithEmailSANs(t *testing.T) {
// Pin the EmailAddresses → SAN append path (was uncovered).
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
if err != nil {
t.Fatalf("GenerateKey: %v", err)
}
template := &x509.Certificate{
SerialNumber: big.NewInt(42),
Subject: pkix.Name{CommonName: "test.example.com"},
NotBefore: time.Now().Add(-1 * time.Hour),
NotAfter: time.Now().Add(24 * time.Hour),
KeyUsage: x509.KeyUsageDigitalSignature,
DNSNames: []string{"test.example.com"},
EmailAddresses: []string{"alice@example.com", "bob@example.com"},
}
certDER, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
if err != nil {
t.Fatalf("CreateCertificate: %v", err)
}
cert, err := x509.ParseCertificate(certDER)
if err != nil {
t.Fatalf("ParseCertificate: %v", err)
}
src := NewWithClient(&config.AWSSecretsMgrDiscoveryConfig{Region: "us-east-1"}, newMockSMClient(), slog.Default())
entry, err := src.buildDiscoveredCertEntry(cert, "prod/test")
if err != nil {
t.Fatalf("buildDiscoveredCertEntry: %v", err)
}
if len(entry.SANs) != 3 {
t.Errorf("expected 3 SANs (1 DNS + 2 emails), got %d: %v", len(entry.SANs), entry.SANs)
}
}
+105
View File
@@ -0,0 +1,105 @@
package crypto
import (
"bytes"
"testing"
"github.com/leanovate/gopter"
"github.com/leanovate/gopter/gen"
"github.com/leanovate/gopter/prop"
)
// Bundle Q (L-003 closure): property-based testing pilot.
//
// Two properties pinned with gopter:
//
// 1. Round-trip — DecryptIfKeySet(EncryptIfKeySet(x, k), k) == x for any
// plaintext x and non-empty passphrase k. This is the core encryption
// invariant; mutation testing on AES-GCM would benefit from this kind
// of generative coverage in addition to the existing example-based
// tests, because randomly-generated edge cases (zero-length plaintext,
// plaintext containing the v2/v3 magic byte, very long plaintext) get
// exercised automatically.
//
// 2. Wrong-passphrase rejection — DecryptIfKeySet(blob, wrongKey) must
// never return a nil error AND non-empty plaintext. AEAD authentication
// guarantees this; the property test makes the guarantee testable
// under generative inputs rather than handpicked vectors.
//
// gopter is a non-blocking pilot — `MinSuccessfulTests` is 200 by default
// and these properties run in <50ms at -short. CI keeps them in the regular
// test stream (no separate gating).
func TestProperty_EncryptDecryptRoundTrip(t *testing.T) {
if testing.Short() {
t.Skip("skipping property-based test in -short mode (PBKDF2 600k rounds × 50 iters > short budget)")
}
parameters := gopter.DefaultTestParameters()
parameters.MinSuccessfulTests = 50 // 50 × 600k PBKDF2 ≈ 4-5s on -race CI
properties := gopter.NewProperties(parameters)
properties.Property("DecryptIfKeySet(EncryptIfKeySet(x, k), k) == x", prop.ForAll(
func(plaintext []byte, passphrase string) bool {
// Empty passphrase is the documented sentinel — skip.
if passphrase == "" {
return true
}
blob, ok, err := EncryptIfKeySet(plaintext, passphrase)
if err != nil || !ok {
t.Logf("EncryptIfKeySet(_, %q): err=%v ok=%v", passphrase, err, ok)
return false
}
recovered, err := DecryptIfKeySet(blob, passphrase)
if err != nil {
t.Logf("DecryptIfKeySet round-trip: err=%v plaintext=%v passphrase=%q", err, plaintext, passphrase)
return false
}
return bytes.Equal(recovered, plaintext)
},
// Plaintext: arbitrary byte slices including empty.
gen.SliceOf(gen.UInt8()),
// Passphrase: ASCII alpha, length 1..63 (avoid pathological lengths
// blowing up PBKDF2 budgets in the property runner).
gen.AlphaString().SuchThat(func(s string) bool {
return len(s) > 0 && len(s) < 64
}),
))
properties.TestingRun(t)
}
func TestProperty_WrongPassphraseRejected(t *testing.T) {
if testing.Short() {
t.Skip("skipping property-based test in -short mode (PBKDF2 cost)")
}
parameters := gopter.DefaultTestParameters()
parameters.MinSuccessfulTests = 30 // 30 × 600k PBKDF2 × 2 (encrypt+decrypt) ≈ 5s
properties := gopter.NewProperties(parameters)
properties.Property("Decrypt with wrong passphrase never returns plaintext", prop.ForAll(
func(plaintext []byte, k1, k2 string) bool {
if k1 == "" || k2 == "" || k1 == k2 {
return true
}
blob, _, err := EncryptIfKeySet(plaintext, k1)
if err != nil {
return false
}
recovered, err := DecryptIfKeySet(blob, k2)
// AEAD must reject. Either err != nil (expected), or — in the
// astronomically-unlikely case of a tag collision — recovered
// must NOT equal the original plaintext. Bytes-equal-but-no-error
// is a security-relevant invariant violation.
if err == nil && bytes.Equal(recovered, plaintext) {
t.Logf("AEAD failed to reject wrong passphrase: plaintext=%v k1=%q k2=%q", plaintext, k1, k2)
return false
}
return true
},
gen.SliceOf(gen.UInt8()),
gen.AlphaString().SuchThat(func(s string) bool { return len(s) > 0 && len(s) < 64 }),
gen.AlphaString().SuchThat(func(s string) bool { return len(s) > 0 && len(s) < 64 }),
))
properties.TestingRun(t)
}
+110
View File
@@ -0,0 +1,110 @@
package pkcs7
import (
"testing"
"github.com/leanovate/gopter"
"github.com/leanovate/gopter/gen"
"github.com/leanovate/gopter/prop"
)
// Bundle Q (L-003 closure): property-based test for ASN.1 length encoding.
//
// The pkcs7 package implements DER-encoded length under [ASN1EncodeLength];
// the inverse parser is provided here as `decodeLength` (tracked under the
// EST/SCEP code path that consumes the DER framing). The property is the
// classic encode/decode round-trip:
//
// decodeLength(encodeLength(x)) == x for all 0 ≤ x ≤ math.MaxInt32
//
// In addition, structural invariants are pinned:
//
// - 0 ≤ x < 128 → output is 1 byte, equal to x
// - x ≥ 128 → output[0] has the high bit set; output[0]&0x7f == len(rest)
// and rest is big-endian
//
// These match X.690 §8.1.3.
// decodeLength is the inverse of ASN1EncodeLength, defined in this test file
// because the production code only needs the encoder. It returns the decoded
// length and the number of bytes consumed.
func decodeLength(b []byte) (int, int, bool) {
if len(b) == 0 {
return 0, 0, false
}
first := b[0]
if first < 0x80 {
return int(first), 1, true
}
n := int(first & 0x7f)
if n == 0 || n > 4 || len(b) < 1+n {
return 0, 0, false
}
v := 0
for i := 0; i < n; i++ {
v = (v << 8) | int(b[1+i])
}
return v, 1 + n, true
}
func TestProperty_ASN1LengthRoundTrip(t *testing.T) {
parameters := gopter.DefaultTestParameters()
parameters.MinSuccessfulTests = 500
properties := gopter.NewProperties(parameters)
properties.Property("decodeLength(ASN1EncodeLength(x)) == x", prop.ForAll(
func(x int32) bool {
if x < 0 {
return true // out of contract domain (lengths are non-negative)
}
encoded := ASN1EncodeLength(int(x))
got, n, ok := decodeLength(encoded)
if !ok {
t.Logf("decodeLength failed on encoded form of %d: %x", x, encoded)
return false
}
if n != len(encoded) {
t.Logf("consumed %d bytes but encoded form is %d bytes (%d → %x)", n, len(encoded), x, encoded)
return false
}
if got != int(x) {
t.Logf("round-trip mismatch: %d → %x → %d", x, encoded, got)
return false
}
return true
},
gen.Int32Range(0, 0x7fffffff),
))
properties.Property("short-form encoding for x < 128", prop.ForAll(
func(x int8) bool {
if x < 0 {
return true
}
encoded := ASN1EncodeLength(int(x))
return len(encoded) == 1 && encoded[0] == byte(x)
},
gen.Int8Range(0, 127),
))
properties.Property("long-form encoding sets high bit on first byte", prop.ForAll(
func(x int32) bool {
if x < 128 {
return true
}
encoded := ASN1EncodeLength(int(x))
if len(encoded) < 2 {
return false
}
if encoded[0]&0x80 == 0 {
t.Logf("long-form first byte %02x missing high bit for x=%d", encoded[0], x)
return false
}
n := int(encoded[0] & 0x7f)
return n == len(encoded)-1
},
gen.Int32Range(128, 0x7fffffff),
))
properties.TestingRun(t)
}