Feature cli set up pki

[khoahuynhdev]

No description provided.

[khoahuynhdev]

@claude Please review this PR and suggest improvements

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @khoahuynhdev's task —— View job

---

PR Review: Feature CLI Set Up PKI

I've analyzed the PKI implementation and have comprehensive feedback on the architecture, security considerations, and code quality.

Review Progress:

• Read repository guidelines and setup instructions
• Analyze core CLI implementation
• Review PKI and Certificate Authority components
• Check security considerations
• Evaluate code organization and documentation
• Provide comprehensive feedback and suggestions

📋 Architecture Review

✅ Strengths

1. Clean Architecture: Well-organized separation of concerns with clear CLI/server command structure
2. Comprehensive PKI Structure: Proper directory organization following OpenSSL standards
3. Multi-Server Support: HTTP, TLS, and mTLS server modes with proper configuration
4. Good Development Tooling: Task runner setup with comprehensive development workflow

⚠️ Critical Issues

Security & PKI Implementation

1. Hard-coded File Paths (cert_controller.go:59,79,204,224): CA keys/certs are hard-coded to "caKey.pem" and "caCert.pem" - should use configuration-based paths
2. No Key Protection: Private keys are not encrypted/protected with passphrases
3. Fixed Certificate Values (cert_controller.go:138,282): Common name is hard-coded to "My Homelab" instead of using request parameters
4. File Permission Issues: Some files created with 0755 instead of more restrictive permissions for sensitive PKI data

Code Quality Issues

1. Commented Code: Large blocks of commented-out code in cert_controller.go (lines 45-56, 190-201, etc.)
2. Incomplete Implementation:
   • ca.go:8 - CreatRootCA() function is empty
   • ca.go:25 - Undefined key.GetPrivateKey() and slugger() functions
   • ca.go:31 - Function returns empty values
3. Logic Error (util/files.go:8-18): FileExists() function returns true when os.Stat() succeeds, but should return false for non-existent files
4. Inconsistent Error Handling: Mix of panic and graceful error handling approaches

Architectural Concerns

1. Hardcoded Default Values: Certificate validity periods and key sizes are hardcoded
2. No Input Validation: Request parameters are not validated in controller functions
3. Duplicate Code: Similar logic repeated between CreateClientCert and CreateServerCert functions
4. Missing CA Management: No proper CA certificate generation in the setup command

🔧 Specific Recommendations

High Priority Fixes

1. Fix FileExists() logic (util/files.go:8-18):

   func FileExists(fileName string) (bool, error) {
       _, err := os.Stat(fileName)
       if err == nil {
           return true, nil  // File exists
       }
       if os.IsNotExist(err) {
           return false, nil  // File doesn't exist
       }
       return false, err     // Other error
   }

2. Complete CA setup (cmd/cli/cli.go:32):

   // TODO: Setup Root CA
   caConfig := ca.CertificateConfiguration{
       Subject: ca.CertificateConfigurationSubject{
           CommonName: "Root CA",
           // ... other fields
       },
       // ... other config
   }
   ca.CreateNewCA(caConfig)

3. Fix certificate paths (controllers/cert_controller.go):

   // Use configuration-based paths instead of hardcoded strings
   caKeyPath := cfg.PKIPath + "/private/ca.key"
   caCertPath := cfg.PKIPath + "/certs/ca.crt"

Medium Priority Improvements

1. Add input validation for all controller endpoints
2. Implement proper logging instead of fmt.Printf statements
3. Add certificate revocation functionality
4. Implement proper key encryption with passphrases
5. Add unit tests for critical PKI functions

Code Style Improvements

1. Remove commented code - clean up all commented-out blocks
2. Add error handling for all file operations
3. Use consistent naming (e.g., CreatRootCA → CreateRootCA)
4. Add proper documentation for exported functions

📊 File-Specific Issues

internal/structure/pki_structure.go:87-97

• Bug: Wrong assignment in return statement (line 89 should be rootCACertsPath)

controllers/cert_controller.go:4

• Import Issue: ca-server/utils import should be ca-server/internal/util based on project structure

cmd/root.go:21

• Typo: "failed to run comnad" should be "failed to run command"

🚀 Next Steps

1. Fix the critical security and logic issues mentioned above
2. Complete the CA certificate generation functionality
3. Add proper error handling and logging throughout
4. Implement input validation for all endpoints
5. Add unit tests for the PKI functionality
6. Consider adding certificate management commands (list, revoke, etc.)

📈 Overall Assessment

This PR shows a solid architectural foundation for a PKI system, but has several critical issues that need attention before it's production-ready. The multi-server setup and development tooling are well-implemented, but the core PKI functionality needs security hardening and completion.

Recommendation: Address the security issues and complete the incomplete functions before merging.

---