feat: Add audit logging and DNS management

[nfebe]

• Add audit logging system with SQLite storage
• Add middleware for request/response capture
• Add DNS plugins for external providers (Cloudflare, Route53, DigitalOcean, Hetzner)
• Add PowerDNS infrastructure service for self-hosted DNS
• Add API handlers for audit, DNS, and PowerDNS endpoints
• Extend plugin system with DNSPlugin interface

[sourceant]

Code Review Summary

This pull request introduces significant new features including a comprehensive audit logging system, PowerDNS integration for local DNS management, and a flexible plugin framework for external DNS providers (Cloudflare, AWS Route 53, DigitalOcean, Hetzner).

The changes enhance observability, infrastructure management capabilities, and provide extensible DNS control. The approach to separate concerns into new modules (internal/audit, internal/dns, pkg/plugins/dns) is good.

🚀 Key Improvements

• Audit Logging: A new audit system (internal/audit) captures API events, including actor details, actions, and request/response information, with sensitive data redaction.
• PowerDNS Management: New handlers and a manager (internal/dns/powerdns.go, internal/api/powerdns_handlers.go) enable full lifecycle management of a local PowerDNS server and DNS zones.
• Extensible DNS Plugins: A generic DNSPlugin interface (pkg/plugins/types.go) and implementations for major providers (cloudflare, route53, digitalocean, hetzner) allow for dynamic DNS record management.
• Performance-conscious Logging: Audit event logging is performed asynchronously to prevent blocking API requests.
• Dependency Updates: The go.mod file has been updated to use Go 1.24.0 and includes new dependencies for AWS SDK, Cloudflare, DigitalOcean, and golang.org/x/oauth2 to support the new DNS integrations.

💡 Minor Suggestions

• Consolidate duplicated context key constants into a single shared package.
• Improve the robustness of YAML file modifications by using unmarshaling and re-marshaling rather than string/regex manipulation.
• Refine the heuristic-based parsing of API paths for audit actions and resource IDs to ensure higher accuracy.

🚨 Critical Issues

• The generateRandomPassword function in internal/api/server.go uses a weak pseudo-random number generator, which is unsuitable for generating security-sensitive data like database passwords. This needs to be replaced with a cryptographically secure random source (e.g., crypto/rand).

[sourceant]

🔍 Code Review

💡 1. **internal/api/server.go** (Lines 860-865) - SECURITY

Using time.Now().UnixNano() as a seed for math/rand makes the generated passwords predictable, which is a security vulnerability. For cryptographic purposes like generating passwords, crypto/rand should be used instead.

Suggested Code:

func generateRandomPassword(length int) string {
	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
	b := make([]byte, length)
	_, err := rand.Read(b)
	if err != nil {
		// Fallback or panic, depending on desired error handling
		return ""
	}
	for i := range b {
		b[i] = charset[int(b[i])%len(charset)]
	}
	return string(b)
}

Current Code:

func generateRandomPassword(length int) string {
	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
	b := make([]byte, length)
	for i := range b {
		b[i] = charset[time.Now().UnixNano()%int64(len(charset))]
		time.Sleep(time.Nanosecond)
	}
	return string(b)
}

💡 2. **internal/api/server.go** (Lines 2425-2471) - REFACTOR

Modifying YAML content using string replacement and regex can be very brittle and lead to parsing errors or unexpected behavior if the YAML structure deviates slightly from the expected pattern. It's generally safer and more robust to unmarshal the YAML into a Go struct (or map[string]interface{}), modify the structured data, and then marshal it back into YAML. This ensures that the YAML remains valid and correctly structured.

Consider using gopkg.in/yaml.v3 to unmarshal, modify the composeFile struct, and then re-marshal.

Suggested Code:

// Replace hardcoded network should unmarshal, modify, and re-marshal YAML for robustness.

Current Code:

func replaceHardcodedNetwork(content, oldNetwork, newNetwork string) string {
	if oldNetwork == newNetwork {
		return content
	}

	lines := strings.Split(content, "\n")
	var result []string
	inNetworks := false
	inServicesNetworks := false
	indentLevel := 0

	for _, line := range lines {
		trimmed := strings.TrimSpace(line)
		currentIndent := len(line) - len(strings.TrimLeft(line, " \t"))

		if trimmed == "networks:" {
			if currentIndent == 0 {
				inNetworks = true
				indentLevel = currentIndent
			} else {
				inServicesNetworks = true
				indentLevel = currentIndent
			}
			result = append(result, line)
			continue
		}

		if inNetworks && currentIndent > indentLevel {
			if strings.HasPrefix(trimmed, oldNetwork+":") {
				line = strings.Replace(line, oldNetwork+":", newNetwork+":", 1)
			}
		} else if inNetworks && currentIndent <= indentLevel && trimmed != "" {
			inNetworks = false
		}

		if inServicesNetworks && currentIndent > indentLevel {
			if trimmed == "- "+oldNetwork {
				line = strings.Replace(line, "- "+oldNetwork, "- "+newNetwork, 1)
			}
		} else if inServicesNetworks && currentIndent <= indentLevel && trimmed != "" {
			inServicesNetworks = false
		}

		result = append(result, line)
	}

	return strings.Join(result, "\n")
}

💡 3. **internal/audit/middleware.go** (Lines 16-19) - REFACTOR

These context keys are also defined in internal/auth/middleware.go. To avoid duplication and ensure consistency, consider moving these shared constants to a dedicated internal/shared or internal/contextkeys package.

Suggested Code:

const (
	// Move these constants to a shared package, e.g., "github.com/flatrun/agent/internal/shared/contextkeys"
)

Current Code:

const (
	ContextKeyActorType    = "audit_actor_type"
	ContextKeyActorID      = "audit_actor_id"
	ContextKeyActorName    = "audit_actor_name"
	ContextKeyAPIKeyPrefix = "audit_api_key_prefix"
)

💡 4. **internal/audit/middleware.go** (Lines 139-158) - CLARITY

The isID function uses a heuristic (alphaNum.MatchString(s) && strings.ContainsAny(s, "0123456789")) which might incorrectly classify certain non-ID path segments as IDs. For example, a path like /users/flatrun-agent where flatrun-agent is a username could be misidentified as an ID if it meets the length and character criteria. Relying heavily on regex for ID detection can be brittle.

Consider a more explicit approach by annotating Gin routes with resource types and ID parameters, or by defining specific patterns for IDs where possible (e.g., only UUIDs or purely numeric IDs if that is consistently the case).

Suggested Code:

// Consider explicit route parameter extraction or more precise ID patterns.
// func isID(s string) bool { ... }

Current Code:

func isID(s string) bool {
	if s == "" {
		return false
	}
	if uuidRegex.MatchString(s) {
		return true
	}
	if numericRegex.MatchString(s) {
		return true
	}
	if len(s) >= 8 && len(s) <= 64 && !strings.Contains(s, " ") {
		alphaNum := regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)
		if alphaNum.MatchString(s) && strings.ContainsAny(s, "0123456789") {
			return true
		}
	}
	return false
}

💡 5. **internal/auth/middleware.go** (Lines 14-17) - REFACTOR

These context keys are also defined in internal/audit/middleware.go. To avoid duplication and ensure consistency, consider moving these shared constants to a dedicated internal/shared or internal/contextkeys package. The current approach leads to redundant definitions.

Suggested Code:

// Context keys should be imported from a shared package, e.g., `github.com/flatrun/agent/internal/shared/contextkeys`.

Current Code:

const (
	ContextKeyActorType    = "audit_actor_type"
	ContextKeyActorID      = "audit_actor_id"
	ContextKeyActorName    = "audit_actor_name"
	ContextKeyAPIKeyPrefix = "audit_api_key_prefix"
)

💡 6. **internal/dns/powerdns.go** (Lines 208-218) - REFACTOR

The initializeDatabase function uses docker run alpine:latest to execute SQLite commands for schema creation. While functional, this introduces an external Docker dependency for database initialization, making the setup less self-contained in Go. It also relies on the alpine image and apk package manager being available.

Consider implementing the SQLite schema creation directly within the Go code using database/sql's Exec method (similar to internal/audit/db.go). This would remove the dependency on an external container command for this task.

Suggested Code:

// Implement schema creation directly in Go using `db.conn.Exec(schema)` for better self-containment.
// absDataDir, err := filepath.Abs(dataDir)
// if err != nil { return fmt.Errorf("failed to get absolute path: %w", err) }
// conn, err := sql.Open("sqlite3", filepath.Join(absDataDir, "pdns.sqlite3"))
// if err != nil { return err }
// defer conn.Close()
// _, err = conn.Exec(schema)
// return err

Current Code:

func (m *PowerDNSManager) initializeDatabase(dataDir string) error {
	dbPath := filepath.Join(dataDir, "pdns.sqlite3")

	if _, err := os.Stat(dbPath); err == nil {
		return nil
	}

	schema := `
PRAGMA foreign_keys = 1;
...
`

	absDataDir, err := filepath.Abs(dataDir)
	if err != nil {
		return fmt.Errorf("failed to get absolute path: %w", err)
	}

	uid := fmt.Sprintf("%d", os.Getuid())
	gid := fmt.Sprintf("%d", os.Getgid())

	cmd := exec.Command("docker", "run", "--rm",
		"-v", absDataDir+":/data",
		"alpine:latest",
		"sh", "-c", "apk add --no-cache sqlite > /dev/null 2>&1 && sqlite3 /data/pdns.sqlite3 \""+schema+"\" && chown "+uid+":"+gid+" /data/pdns.sqlite3")
	output, err := cmd.CombinedOutput()
	if err != nil {
		return fmt.Errorf("database init failed: %s - %w", string(output), err)
	}

	return nil
}

Verdict: APPROVE

Posted as a comment because posting a review failed.

[openhands-ai]

Looks like there are a few issues preventing this PR from being merged!

• GitHub Actions are failing:
  • CI

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #51 at branch `feat/audit-dns`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}