chore: code review fixes 2026-05 (with breaking API tightenings)

apis/v1/cocoonset_types.go

@@ -142,9 +142,13 @@ type CocoonSetStatus struct {
 	DesiredToolboxes int32 `json:"desiredToolboxes"`
 
 	// +optional
+	// +listType=map
+	// +listMapKey=slot
 	Agents []AgentStatus `json:"agents,omitempty"`
 
 	// +optional
+	// +listType=map
+	// +listMapKey=name
 	Toolboxes []ToolboxStatus `json:"toolboxes,omitempty"`
 
 	// +optional

apis/v1/crds/cocoonset.cocoonstack.io_cocoonsets.yaml

@@ -443,6 +443,9 @@ spec:
                   - slot
                   type: object
                 type: array
+                x-kubernetes-list-map-keys:
+                - slot
+                x-kubernetes-list-type: map
               conditions:
                 items:
                   description: Condition contains details for one aspect of the current
@@ -562,6 +565,9 @@ spec:
                   - name
                   type: object
                 type: array
+                x-kubernetes-list-map-keys:
+                - name
+                x-kubernetes-list-type: map
             type: object
         type: object
     served: true

apis/v1/enums.go

@@ -1,5 +1,10 @@
 package v1
 
+import (
+	"cmp"
+	"slices"
+)
+
 const (
 	AgentModeClone AgentMode = "clone"
 	AgentModeRun   AgentMode = "run"
@@ -32,6 +37,17 @@ const (
 	BackendFirecracker     Backend = "firecracker"
 )
 
+// Per-type valid-value tables. Keep ordering aligned with the const
+// block above so a new enum member is one edit in each place.
+var (
+	agentModeValid      = []AgentMode{AgentModeClone, AgentModeRun}
+	toolboxModeValid    = []ToolboxMode{ToolboxModeRun, ToolboxModeClone, ToolboxModeStatic}
+	osTypeValid         = []OSType{OSLinux, OSWindows, OSAndroid}
+	snapshotPolicyValid = []SnapshotPolicy{SnapshotPolicyAlways, SnapshotPolicyMainOnly, SnapshotPolicyNever}
+	connTypeValid       = []ConnType{ConnTypeSSH, ConnTypeRDP, ConnTypeVNC, ConnTypeADB}
+	backendValid        = []Backend{BackendCloudHypervisor, BackendFirecracker}
+)
+
 // AgentMode defines the mode of an agent VM.
 // +kubebuilder:validation:Enum=clone;run
 type AgentMode string
@@ -66,78 +82,39 @@ type ConnType string
 type Backend string
 
 // IsValid reports whether m is a recognized AgentMode value.
-func (m AgentMode) IsValid() bool {
-	return m == AgentModeClone || m == AgentModeRun
-}
+func (m AgentMode) IsValid() bool { return slices.Contains(agentModeValid, m) }
 
 // Default returns m when set, otherwise AgentModeClone.
-func (m AgentMode) Default() AgentMode {
-	if m == "" {
-		return AgentModeClone
-	}
-	return m
-}
+func (m AgentMode) Default() AgentMode { return cmp.Or(m, AgentModeClone) }
 
 // IsValid reports whether m is a recognized ToolboxMode value.
-func (m ToolboxMode) IsValid() bool {
-	return m == ToolboxModeRun || m == ToolboxModeClone || m == ToolboxModeStatic
-}
+func (m ToolboxMode) IsValid() bool { return slices.Contains(toolboxModeValid, m) }
 
 // Default returns m when set, otherwise ToolboxModeRun.
-func (m ToolboxMode) Default() ToolboxMode {
-	if m == "" {
-		return ToolboxModeRun
-	}
-	return m
-}
+func (m ToolboxMode) Default() ToolboxMode { return cmp.Or(m, ToolboxModeRun) }
 
 // IsValid reports whether o is a recognized OSType value.
-func (o OSType) IsValid() bool {
-	return o == OSLinux || o == OSWindows || o == OSAndroid
-}
+func (o OSType) IsValid() bool { return slices.Contains(osTypeValid, o) }
 
 // Default returns o when set, otherwise OSLinux.
-func (o OSType) Default() OSType {
-	if o == "" {
-		return OSLinux
-	}
-	return o
-}
+func (o OSType) Default() OSType { return cmp.Or(o, OSLinux) }
 
 // IsValid reports whether p is a recognized SnapshotPolicy value.
-func (p SnapshotPolicy) IsValid() bool {
-	return p == SnapshotPolicyAlways || p == SnapshotPolicyMainOnly || p == SnapshotPolicyNever
-}
+func (p SnapshotPolicy) IsValid() bool { return slices.Contains(snapshotPolicyValid, p) }
 
 // Default returns p when set, otherwise SnapshotPolicyAlways.
-func (p SnapshotPolicy) Default() SnapshotPolicy {
-	if p == "" {
-		return SnapshotPolicyAlways
-	}
-	return p
-}
+func (p SnapshotPolicy) Default() SnapshotPolicy { return cmp.Or(p, SnapshotPolicyAlways) }
 
 // IsValid reports whether c is a recognized ConnType value.
-func (c ConnType) IsValid() bool {
-	return c == ConnTypeSSH || c == ConnTypeRDP || c == ConnTypeVNC || c == ConnTypeADB
-}
+func (c ConnType) IsValid() bool { return slices.Contains(connTypeValid, c) }
 
 // Default returns c unchanged. Unlike the other enums, ConnType has no
 // static default: an empty value signals "infer from OS and runtime"
 // (see meta.ConnectionType), so this method exists only for API symmetry.
-func (c ConnType) Default() ConnType {
-	return c
-}
+func (c ConnType) Default() ConnType { return c }
 
 // IsValid reports whether b is a recognized Backend value.
-func (b Backend) IsValid() bool {
-	return b == BackendCloudHypervisor || b == BackendFirecracker
-}
+func (b Backend) IsValid() bool { return slices.Contains(backendValid, b) }
 
 // Default returns b when set, otherwise BackendCloudHypervisor.
-func (b Backend) Default() Backend {
-	if b == "" {
-		return BackendCloudHypervisor
-	}
-	return b
-}
+func (b Backend) Default() Backend { return cmp.Or(b, BackendCloudHypervisor) }

auth/session.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"strings"
+	"time"
 )
 
 // Session holds the claims embedded in an HMAC-signed cookie.
@@ -32,7 +33,7 @@ func SignSession(sess Session, key []byte) (string, error) {
 }
 
 // VerifySession validates the HMAC signature and decodes the session.
-// Returns nil and false if the signature is invalid or decoding fails.
+// Exp == 0 means "no expiry".
 func VerifySession(cookie string, key []byte) (*Session, bool) {
 	payload, sig, ok := strings.Cut(cookie, ".")
 	if !ok {
@@ -52,5 +53,8 @@ func VerifySession(cookie string, key []byte) (*Session, bool) {
 	if json.Unmarshal(data, sess) != nil {
 		return nil, false
 	}
+	if sess.Exp != 0 && sess.Exp <= time.Now().Unix() {
+		return nil, false
+	}
 	return sess, true
 }

auth/session_test.go

@@ -60,6 +60,34 @@ func TestVerifySessionWrongKey(t *testing.T) {
 	}
 }
 
+func TestVerifySessionRejectsExpired(t *testing.T) {
+	t.Parallel()
+
+	key := []byte("test-secret-key-32-bytes-long!!!")
+	expired := Session{User: "dave", Exp: time.Now().Add(-time.Hour).Unix()}
+	cookie, err := SignSession(expired, key)
+	if err != nil {
+		t.Fatalf("SignSession: %v", err)
+	}
+	if _, ok := VerifySession(cookie, key); ok {
+		t.Error("expected expired cookie to fail")
+	}
+}
+
+func TestVerifySessionZeroExpAccepted(t *testing.T) {
+	t.Parallel()
+
+	key := []byte("test-secret-key-32-bytes-long!!!")
+	// Exp == 0 means "no expiry" — must remain accepted.
+	cookie, err := SignSession(Session{User: "eve"}, key)
+	if err != nil {
+		t.Fatalf("SignSession: %v", err)
+	}
+	if _, ok := VerifySession(cookie, key); !ok {
+		t.Error("expected session with zero Exp to be accepted")
+	}
+}
+
 func TestRandomState(t *testing.T) {
 	t.Parallel()
 

auth/state.go

@@ -3,12 +3,16 @@ package auth
 import (
 	"crypto/rand"
 	"encoding/hex"
+	"fmt"
 )
 
 // RandomState returns a cryptographically random 32-character hex string
 // suitable for OAuth state parameters and CSRF nonces.
+// Panics on crypto/rand failure — a weak nonce silently breaks CSRF.
 func RandomState() string {
 	b := make([]byte, 16)
-	_, _ = rand.Read(b) //nolint:errcheck // crypto/rand.Read never fails on supported platforms
+	if _, err := rand.Read(b); err != nil {
+		panic(fmt.Sprintf("crypto/rand.Read: %v", err))
+	}
 	return hex.EncodeToString(b)
 }

k8s/netip.go

@@ -1,21 +1,31 @@
 package k8s
 
-import "net"
+import (
+	"errors"
+	"fmt"
+	"net"
+)
 
-// DetectNodeIP returns the first non-loopback IPv4 address, or "127.0.0.1" if none found.
-func DetectNodeIP() string {
+// ErrNoNodeIP is returned when no non-loopback IPv4 address is
+// reachable. Callers pick the fallback — auto-substituting localhost
+// would mask misconfigured network namespaces.
+var ErrNoNodeIP = errors.New("no non-loopback IPv4 address found")
+
+// DetectNodeIP returns the first non-loopback IPv4 address, or
+// ErrNoNodeIP if none exists.
+func DetectNodeIP() (string, error) {
 	addrs, err := net.InterfaceAddrs()
 	if err != nil {
-		return localhost
+		return "", fmt.Errorf("list interface addresses: %w", err)
 	}
 	for _, addr := range addrs {
 		ipNet, ok := addr.(*net.IPNet)
 		if !ok || ipNet.IP == nil || ipNet.IP.IsLoopback() {
 			continue
 		}
 		if ip4 := ipNet.IP.To4(); ip4 != nil {
-			return ip4.String()
+			return ip4.String(), nil
 		}
 	}
-	return localhost
+	return "", ErrNoNodeIP
 }

k8s/tls.go

@@ -1,52 +1,100 @@
 package k8s
 
 import (
+	"context"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
+	"errors"
 	"fmt"
+	"io/fs"
 	"math/big"
 	"net"
 	"os"
 	"time"
+
+	"github.com/projecteru2/core/log"
 )
 
 const localhost = "127.0.0.1"
 
-// LoadOrGenerateCert loads a TLS keypair from disk, falling back to a self-signed cert.
-// Returns a source label for logging ("disk <path>" or "self-signed").
-func LoadOrGenerateCert(certPath, keyPath, hostname, ip string) (tls.Certificate, string, error) {
-	if certPath != "" && keyPath != "" {
-		if _, err := os.Stat(certPath); err == nil {
-			cert, err := tls.LoadX509KeyPair(certPath, keyPath)
-			if err != nil {
-				return tls.Certificate{}, "", fmt.Errorf("load tls keypair %s: %w", certPath, err)
-			}
-			return cert, fmt.Sprintf("disk %s", certPath), nil
-		}
+// LoadOrGenerateCert loads a TLS keypair from disk, falling back to a
+// self-signed cert when paths are empty, the cert is missing, or the
+// cert is expired. Returns a source label for logging.
+func LoadOrGenerateCert(ctx context.Context, certPath, keyPath, hostname, ip string) (tls.Certificate, string, error) {
+	cert, source, err := tryLoadDiskCert(ctx, certPath, keyPath)
+	if err != nil {
+		return tls.Certificate{}, "", err
 	}
-	cert, err := GenerateSelfSignedCert(hostname, ip)
+	if source != "" {
+		return cert, source, nil
+	}
+	cert, err = GenerateSelfSignedCert(hostname, ip)
 	if err != nil {
 		return tls.Certificate{}, "", fmt.Errorf("generate self-signed cert: %w", err)
 	}
 	return cert, "self-signed", nil
 }
 
-// GenerateSelfSignedCert creates an in-memory ECDSA P-256 self-signed cert for hostname and ip.
+// tryLoadDiskCert returns ("", "", nil) when the caller should fall
+// back to self-signed (paths empty, cert missing, or expired) and an
+// error only when a configured keypair fails to load.
+func tryLoadDiskCert(ctx context.Context, certPath, keyPath string) (tls.Certificate, string, error) {
+	if certPath == "" || keyPath == "" {
+		return tls.Certificate{}, "", nil
+	}
+	if _, err := os.Stat(certPath); err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			return tls.Certificate{}, "", nil
+		}
+		return tls.Certificate{}, "", fmt.Errorf("stat tls cert %s: %w", certPath, err)
+	}
+	cert, err := tls.LoadX509KeyPair(certPath, keyPath)
+	if err != nil {
+		return tls.Certificate{}, "", fmt.Errorf("load tls keypair %s: %w", certPath, err)
+	}
+	if isCertExpired(ctx, cert, certPath) {
+		return tls.Certificate{}, "", nil
+	}
+	return cert, fmt.Sprintf("disk %s", certPath), nil
+}
+
+// isCertExpired returns true when the leaf cert is past NotAfter.
+// Parse failures are warned and treated as "not expired".
+func isCertExpired(ctx context.Context, cert tls.Certificate, certPath string) bool {
+	logger := log.WithFunc("k8s.LoadOrGenerateCert")
+	if len(cert.Certificate) == 0 {
+		return false
+	}
+	parsed, err := x509.ParseCertificate(cert.Certificate[0])
+	if err != nil {
+		logger.Warnf(ctx, "parse disk cert %s: %v (keeping cert)", certPath, err)
+		return false
+	}
+	if time.Now().After(parsed.NotAfter) {
+		logger.Warnf(ctx, "disk cert %s expired at %s, falling back to self-signed", certPath, parsed.NotAfter.Format(time.RFC3339))
+		return true
+	}
+	return false
+}
+
+// GenerateSelfSignedCert creates an in-memory ECDSA P-256 self-signed
+// cert for hostname and ip.
 func GenerateSelfSignedCert(hostname, ip string) (tls.Certificate, error) {
 	priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	if err != nil {
 		return tls.Certificate{}, err
 	}
+	now := time.Now()
 	template := x509.Certificate{
 		SerialNumber: big.NewInt(1),
 		Subject:      pkix.Name{CommonName: hostname},
-		NotBefore:    time.Now(),
-		NotAfter:     time.Now().Add(10 * 365 * 24 * time.Hour),
+		NotBefore:    now,
+		NotAfter:     now.Add(10 * 365 * 24 * time.Hour),
 		KeyUsage:     x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
 		ExtKeyUsage:  []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 		DNSNames:     []string{hostname, "localhost"},