From 2ce4b2fe044845946d68e89efd9ea70c00423c2b Mon Sep 17 00:00:00 2001 From: Mark G Date: Thu, 5 Dec 2024 14:42:33 +0100 Subject: [PATCH] bcrypt added --- TODO.md | 106 +++++++++++++++++++++++++++++++++++++++++ bin/nodelocker/main.go | 9 +++- go.mod | 2 + go.sum | 2 + internal/x_basics.go | 10 ++-- internal/x_password.go | 51 ++++++++++++++++++++ 6 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 TODO.md create mode 100644 internal/x_password.go diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..7648ee1 --- /dev/null +++ b/TODO.md @@ -0,0 +1,106 @@ +# NodeLocker TODO List + +## Security Improvements +- [ ] Replace SHA1 password hashing with bcrypt or Argon2 + - Priority: High + - Impact: Critical for password security + - Details: Current implementation uses static salts and SHA1 + +- [ ] Move TLS certificates to secure storage + - Priority: High + - Impact: Production security + - Details: Current `/dev/shm` storage is temporary and insecure + +- [ ] Implement rate limiting for API endpoints + - Priority: High + - Impact: Protection against brute force attacks + - Details: Add rate limiting middleware using token bucket algorithm + +- [ ] Add secure headers + - Priority: Medium + - Impact: Improved security posture + - Details: Implement security headers (HSTS, CSP, etc.) + +## Code Quality +- [ ] Implement proper error handling + - Priority: High + - Impact: Reliability and debugging + - Details: Remove `trunk-ignore` directives and handle all errors + +- [ ] Add structured logging + - Priority: Medium + - Impact: Observability + - Details: Replace fmt.Println with proper logging framework + +- [ ] Refactor long functions + - Priority: Medium + - Impact: Maintainability + - Details: Split `adminHandler` and similar large functions + +- [ ] Implement configuration management + - Priority: High + - Impact: Deployment flexibility + - Details: Add support for config files and environment variables + +- [ ] Update HTTP methods + - Priority: Medium + - Impact: REST compliance + - Details: Use proper POST/PUT/DELETE methods instead of GET + +## Documentation +- [ ] Add API documentation + - Priority: High + - Impact: Developer experience + - Details: Implement Swagger/OpenAPI documentation + +- [ ] Improve code documentation + - Priority: Medium + - Impact: Maintainability + - Details: Add godoc comments for all exported functions + +- [ ] Create deployment guide + - Priority: Medium + - Impact: Operations + - Details: Document production deployment steps + +- [ ] Add architecture documentation + - Priority: Medium + - Impact: System understanding + - Details: Document system design and component interaction + +## Testing +- [ ] Add unit tests for core functionality + - Priority: High + - Impact: Code reliability + - Details: Test all core business logic functions + +- [ ] Implement integration tests + - Priority: High + - Impact: System reliability + - Details: Add Redis integration tests + +- [ ] Add API endpoint tests + - Priority: Medium + - Impact: API reliability + - Details: Test all HTTP endpoints + +- [ ] Create test environment + - Priority: Medium + - Impact: Testing reliability + - Details: Setup isolated test environment with Docker + +## Future Enhancements +- [ ] Add metrics collection + - Priority: Low + - Impact: Monitoring + - Details: Implement Prometheus metrics + +- [ ] Add health check endpoints + - Priority: Medium + - Impact: Operations + - Details: Implement readiness and liveness probes + +- [ ] Implement Redis connection pooling + - Priority: Low + - Impact: Performance + - Details: Configure and optimize Redis connection pool diff --git a/bin/nodelocker/main.go b/bin/nodelocker/main.go index 892990a..d2221c8 100644 --- a/bin/nodelocker/main.go +++ b/bin/nodelocker/main.go @@ -296,7 +296,14 @@ func regHandler(w http.ResponseWriter, r *http.Request) { // now error till this point, let's register the new user if c.HttpErr == x.C_HTTP_OK { - if !x.RSetSingle("user", c.User, x.CryptString(c.Token), 0) { + hashedPassword, err := x.HashPassword(c.Token) + if err != nil { + c.HttpErr = http.StatusInternalServerError + res.Messages = append(res.Messages, "Error hashing password") + return + } + + if !x.RSetSingle("user", c.User, hashedPassword, 0) { c.HttpErr = http.StatusInternalServerError res.Messages = append(res.Messages, x.ERR_UserSetupFailed) diff --git a/go.mod b/go.mod index 56187ca..282664f 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,8 @@ go 1.21.6 require github.com/go-redis/redis v6.15.9+incompatible +require golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d + require ( github.com/go-chi/chi/v5 v5.0.11 github.com/onsi/ginkgo v1.16.5 // indirect diff --git a/go.sum b/go.sum index bb2ede0..5a89522 100644 --- a/go.sum +++ b/go.sum @@ -39,6 +39,8 @@ github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9dec golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d h1:sK3txAijHtOK88l68nt020reeT1ZdKLIYetKl95FzVY= +golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= diff --git a/internal/x_basics.go b/internal/x_basics.go index 4145083..568014d 100644 --- a/internal/x_basics.go +++ b/internal/x_basics.go @@ -194,7 +194,6 @@ func IsExistingUser(userName string) bool { // // Returns: `true` if user is valid (password is matching) func IsValidUser(userName string, userToken string) bool { - if DEBUG { fmt.Println("IsValidUser <<", userName) } @@ -203,8 +202,13 @@ func IsValidUser(userName string, userToken string) bool { if err == nil && len(redisPwd) > 0 { // found user & password - - if redisPwd == CryptString(userToken) { + if CheckPassword(userToken, redisPwd) { + // If using old hash format, upgrade to bcrypt + if NeedsUpgrade(redisPwd) { + if newHash, err := HashPassword(userToken); err == nil { + RSetSingle("user", userName, newHash, 0) + } + } if DEBUG { fmt.Println("IsValidUser >>", true) } diff --git a/internal/x_password.go b/internal/x_password.go new file mode 100644 index 0000000..4089644 --- /dev/null +++ b/internal/x_password.go @@ -0,0 +1,51 @@ +package x + +import ( + "crypto/sha1" + "fmt" + "strings" + + "golang.org/x/crypto/bcrypt" +) + +const ( + // BcryptCost for bcrypt hashing (between 10 and 14 recommended for production) + BcryptCost = 12 + + // Hash version prefixes + bcryptPrefix = "$2a$" + sha1Prefix = "sha1$" + + // Legacy SHA1 salts + preSalt = "68947b1f416c3a5655e1ff9e7c7935f6" + postSalt = "5f09dd9c81596ea3cc93ce0df58e26d8" +) + +// HashPassword creates a bcrypt hash of the password with version prefix +func HashPassword(password string) (string, error) { + bytes, err := bcrypt.GenerateFromPassword([]byte(password), BcryptCost) + if err != nil { + return "", err + } + return string(bytes), nil +} + +// CheckPassword compares a password against a hashed password, handling both old and new formats +func CheckPassword(password, hashedPassword string) bool { + // Check if it's a bcrypt hash + if strings.HasPrefix(hashedPassword, bcryptPrefix) { + err := bcrypt.CompareHashAndPassword([]byte(hashedPassword), []byte(password)) + return err == nil + } + + // Fall back to SHA1 for legacy passwords + h := sha1.New() + h.Write([]byte(preSalt + password + postSalt)) + sha1Hash := fmt.Sprintf("%x", h.Sum(nil)) + return sha1Hash == hashedPassword +} + +// NeedsUpgrade checks if the password hash needs to be upgraded +func NeedsUpgrade(hashedPassword string) bool { + return !strings.HasPrefix(hashedPassword, bcryptPrefix) +}