[hotfix] Auth + Deployment Action #57
Conversation
WalkthroughThis change removes the previous environment-based token authentication system, replacing it with a database-backed API key mechanism. It introduces new database models, migrations, and repositories for API keys, updates middleware to validate tokens using the database, and refactors CLI and server code for account and token management. Configuration and workflow files are also updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Caddy
participant API
participant Middleware
participant DB
Client->>Caddy: HTTP Request (with X-API-Username, X-API-Key, X-API-Signature)
Caddy->>API: Proxied Request (with headers)
API->>Middleware: Route Handling
Middleware->>DB: Find API Key by Account Name
DB-->>Middleware: API Key Record
Middleware->>Middleware: Validate Token Format & Signature
alt Success
Middleware->>API: Pass to Handler
API-->>Client: Response
else Failure
Middleware-->>Client: 403 Forbidden/Error
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gocanto
force-pushed
the
hotfix/gh-deploy-action
branch
from
e7e6809 to
8caac81
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
database/infra/migrations/000002_api_keys.up.sql (1)
4-6: Consider adding validation constraints for API keys.While the current structure is functional, consider adding check constraints to validate key formats if they follow specific patterns (e.g., length requirements, character sets). Also, verify that VARCHAR(255) is appropriate for your key generation algorithm - it might be excessive depending on the actual key length.
Could you verify the expected format and length of the API keys to ensure VARCHAR(255) is appropriately sized?
cli/accounts/handler.go (1)
17-25: Consider logging the created account details for audit purposesWhile the success message is helpful for CLI users, consider adding structured logging of account creation events for audit and debugging purposes, especially in a production environment.
_, err = h.Tokens.Create(database.APIKeyAttr{ AccountName: token.AccountName, SecretKey: token.SecretKey, PublicKey: token.PublicKey, }) if err != nil { return fmt.Errorf("failed to create account: %v", err) } + if h.IsDebugging { + fmt.Printf("Created account: %s with public key: %s\n", token.AccountName, token.PublicKey) + } + cli.Successln("Account created successfully")cli/accounts/factory.go (1)
9-13: Consider adding validation for TokenLength fieldWhile the current usage sets TokenLength to a constant, consider adding validation to ensure it's within acceptable bounds if this field might be configurable in the future.
type Handler struct { Tokens *repository.ApiKeys - TokenLength int + TokenLength int `validate:"min=16,max=128"` IsDebugging bool }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.env.example(0 hunks).env.prod.example(0 hunks).github/workflows/deploy.yml(1 hunks)boost/app.go(2 hunks)boost/factory.go(1 hunks)boost/router.go(1 hunks)caddy/Caddyfile.prod(1 hunks)cli/accounts/factory.go(1 hunks)cli/accounts/handler.go(1 hunks)cli/main.go(3 hunks)cli/panel/menu.go(3 hunks)cli/posts/factory.go(2 hunks)config/makefile/app.mk(2 hunks)database/attrs.go(1 hunks)database/infra/migrations/000001_schema.up.sql(2 hunks)database/infra/migrations/000002_api_keys.up.sql(1 hunks)database/model.go(2 hunks)database/repository/api_keys.go(1 hunks)env/app.go(1 hunks)pkg/auth/schema.go(1 hunks)pkg/auth/token.go(1 hunks)pkg/http/middleware/pipeline.go(1 hunks)pkg/http/middleware/token.go(0 hunks)pkg/http/middleware/token_middleware.go(1 hunks)pkg/http/middleware/username.go(0 hunks)
💤 Files with no reviewable changes (4)
- .env.prod.example
- .env.example
- pkg/http/middleware/token.go
- pkg/http/middleware/username.go
🧰 Additional context used
🧬 Code Graph Analysis (12)
boost/router.go (2)
pkg/http/middleware/pipeline.go (1)
Pipeline(9-12)database/repository/api_keys.go (1)
ApiKeys(11-13)
boost/factory.go (1)
env/env.go (1)
GetEnvVar(16-18)
pkg/http/middleware/pipeline.go (2)
env/env.go (1)
Environment(8-14)database/repository/api_keys.go (1)
ApiKeys(11-13)
boost/app.go (4)
boost/factory.go (1)
MakeDbConnection(37-45)boost/router.go (1)
Router(11-15)pkg/http/middleware/pipeline.go (1)
Pipeline(9-12)database/repository/api_keys.go (1)
ApiKeys(11-13)
cli/panel/menu.go (2)
pkg/cli/colour.go (2)
RedColour(4-4)Reset(3-3)pkg/auth/schema.go (1)
AccountNameMinLength(9-9)
cli/posts/factory.go (3)
cli/posts/input.go (1)
Input(3-5)pkg/client.go (1)
Client(11-17)database/connection.go (1)
Connection(12-17)
cli/accounts/factory.go (4)
database/repository/api_keys.go (1)
ApiKeys(11-13)cli/posts/factory.go (2)
MakeHandler(22-33)Handler(14-20)database/connection.go (1)
Connection(12-17)pkg/auth/schema.go (1)
TokenMinLength(8-8)
cli/accounts/handler.go (4)
cli/accounts/factory.go (1)
Handler(9-13)pkg/auth/token.go (1)
SetupNewAccount(11-30)database/attrs.go (1)
APIKeyAttr(7-11)pkg/cli/message.go (1)
Successln(17-19)
pkg/auth/token.go (1)
pkg/auth/schema.go (7)
Token(12-17)PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)TokenMinLength(8-8)ValidatedToken(19-22)LevelPublic(6-6)LevelSecret(7-7)
cli/main.go (6)
database/connection.go (1)
Connection(12-17)boost/factory.go (1)
MakeDbConnection(37-45)pkg/cli/message.go (2)
Errorln(9-11)Error(5-7)cli/panel/menu.go (1)
Menu(17-21)cli/accounts/factory.go (1)
MakeHandler(15-23)cli/posts/factory.go (1)
MakeHandler(22-33)
pkg/http/middleware/token_middleware.go (3)
database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/http/schema.go (2)
ApiHandler(23-23)ApiError(10-13)pkg/auth/token.go (1)
ValidateBearerToken(52-72)
database/repository/api_keys.go (4)
database/connection.go (1)
Connection(12-17)database/attrs.go (1)
APIKeyAttr(7-11)database/model.go (1)
APIKey(26-35)pkg/gorm/support.go (1)
HasDbIssues(16-18)
🔇 Additional comments (23)
database/attrs.go (1)
7-11: LGTM! Well-structured API key attributes.The
APIKeyAttrstruct follows Go naming conventions and fits well with the existing pattern of attribute structs in the file. The fields appropriately represent the components needed for API key management.env/app.go (1)
7-10: Verified clean removal ofCredentialsfieldRipgrep searches for
.App.CredentialsandCredentials.*auth.Tokenreturned no matches, confirming there are no orphaned references. The change is safe—approving this update.boost/router.go (1)
18-20: Confirm Pipeline.ApiKeys wiring in Router setupI didn’t find any place where you instantiate
middleware.Pipelineor assign itsApiKeysfield when creating theRouter. Please:
Check your application bootstrap (e.g. in
main.goor wherever you build the HTTP server) to ensure you pass a non-nil*repository.ApiKeysintoRouter.Pipeline.ApiKeys.If not already in place, consider adding a
NewRouter(env *env.Environment, mux *baseHttp.ServeMux, apiKeys *repository.ApiKeys) *Routerconstructor that populates:&Router{ Env: env, Mux: mux, Pipeline: middleware.Pipeline{ Env: env, ApiKeys: apiKeys, }, }This will guarantee
Pipeline.ApiKeysis always initialized and avoid nil-pointer panics.pkg/http/middleware/pipeline.go (1)
9-12: LGTM! Proper dependency injection structure.The addition of the
ApiKeysfield to the Pipeline struct enables clean dependency injection of the API keys repository for token validation middleware.database/infra/migrations/000001_schema.up.sql (1)
7-13: All SQL data types are consistently capitalized.The only lowercase matches (
uuid UUID …) are column names, not data type keywords. No further action required.boost/factory.go (1)
63-65: LGTM! Clean removal of static credentials.The refactoring correctly removes the static token credentials from the environment configuration, which aligns with the shift to database-backed API key authentication.
database/infra/migrations/000002_api_keys.up.sql (1)
14-17: Good indexing strategy for performance.The indexes on searchable columns and deleted_at are well-designed for the expected query patterns in an API key authentication system.
config/makefile/app.mk (3)
1-1: Good addition to phony targets.Correctly added the new target to the .PHONY directive to ensure it runs regardless of file existence.
3-3: Well-designed configurable variable.The APP_CADDY_CONFIG_FILE variable with a sensible default allows flexibility for different environments while maintaining convention.
56-58: Comprehensive Caddy validation target.The target properly combines formatting and validation steps, making it useful for both development and CI/CD workflows. The macOS installation note is helpful for developers.
cli/panel/menu.go (2)
88-89: Clear menu option updates.The menu text changes accurately reflect the new functionality - "Parse Blog Posts" is more descriptive than "Parse Posts", and "Create new account" clearly indicates the new capability.
121-136: Well-implemented account name capture with proper validation.The method correctly:
- Handles input reading with error checking
- Trims whitespace from user input
- Validates minimum length using the auth constant
- Provides clear, colored error messages for better UX
- Returns appropriate error types
boost/app.go (2)
23-23: Good extraction of database connection.Creating a local
dbvariable allows proper reuse for both the app struct and middleware pipeline injection.
37-38: Proper dependency injection for API key authentication.The injection of the ApiKeys repository into the middleware pipeline correctly provides database access for the new token authentication middleware, replacing the previous static credential approach.
.github/workflows/deploy.yml (2)
84-94: LGTM: Enhanced deployment visibilityThe addition of image listing before and after retagging provides valuable debugging information for deployment troubleshooting. The retagging logic correctly maps the versioned images to the expected compose service names.
97-97: Good safety improvement in Docker prune commandChanging from
docker image prune -aftodocker image prune -fis a safer approach as it only removes dangling images rather than all unused images, reducing the risk of accidentally removing images that might be needed later.cli/accounts/handler.go (1)
27-29: LGTM: Clean success handlingThe success message and nil return provide clear feedback to the user and proper function completion.
cli/accounts/factory.go (1)
15-23: LGTM: Clean factory pattern implementationThe factory function follows a consistent pattern with other handlers in the codebase and properly initializes the repository with the database connection. The default values are reasonable for the use case.
cli/posts/factory.go (2)
6-6: LGTM: Clean import refactoringThe import changes properly reflect the shift from environment-based database connection creation to direct database connection injection.
22-33: LGTM: Consistent with new dependency injection patternThe function signature change to accept a database connection directly is consistent with the new pattern seen in the accounts factory and removes the coupling to the boost package. This improves testability and follows better dependency injection practices.
database/model.go (1)
26-35: Review unique constraints on APIKey fieldsEach field (AccountName, PublicKey, SecretKey) is marked as
unique, and they're also part of a composite unique indexuq_account_keys. This seems redundant - if each field must be unique individually, the composite index adds no value. Consider whether you want:
- Individual uniqueness (current setup minus the composite index), or
- Composite uniqueness (remove individual
uniquetags, keep only the composite index)The current setup prevents reusing public/secret keys across different accounts, which may or may not be intended.
database/repository/api_keys.go (1)
35-51: Remove redundant and incorrect UUID validationThe UUID validation logic is flawed. If
gorm.HasDbIssuesreturns false (no error), the key was found successfully. The subsequent UUID check will return the key even if UUID is empty, which could happen with an uninitialized struct.Apply this diff to fix the logic:
func (a ApiKeys) FindBy(accountName string) *database.APIKey { key := database.APIKey{} result := a.DB.Sql(). Where("LOWER(account_name) = ?", strings.ToLower(accountName)). First(&key) if gorm.HasDbIssues(result.Error) { return nil } - if strings.Trim(key.UUID, " ") != "" { - return &key - } - - return nil + return &key }Likely an incorrect or invalid review comment.
cli/main.go (1)
16-100: Well-structured refactoringThe refactoring improves code organization by:
- Properly initializing the database connection in
init()- Extracting menu option handlers into dedicated functions
- Maintaining consistent error handling throughout
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
pkg/http/middleware/token_middleware.go (2)
39-41: Validate that the token is a public keyAfter validating the token format, ensure that only public keys (not secret keys) are accepted for API authentication.
Add validation to check that the token has the public key prefix:
if err := auth.ValidateTokenFormat(publicToken); err != nil { return t.getInvalidTokenFormatError(publicToken) } + +if !strings.HasPrefix(publicToken, auth.PublicKeyPrefix) { + return &http.ApiError{ + Message: "invalid token type for this endpoint", + Status: baseHttp.StatusForbidden, + } +}
72-72: Fix typo in error message- "invalid request. Please, provide a valid token, signature and accout name headers. [account: %s, public token: %s, signature: %s]", + "invalid request. Please, provide a valid token, signature and account name headers. [account: %s, public token: %s, signature: %s]",pkg/auth/token.go (3)
12-31: Token length validation and schema mismatchThe function has validation issues:
TokenLengthparameter is not validated againstTokenMinLength- The schema requires
len=16for keys, butgenerateSecureTokenproduces 64-character hex stringsAdd validation and fix the length mismatch:
func SetupNewAccount(accountName string, TokenLength int) (*Token, error) { + if TokenLength < TokenMinLength { + return nil, fmt.Errorf("token length must be at least %d", TokenMinLength) + } + token := Token{} - pk, err := generateSecureToken(TokenLength) + // Generate tokens matching schema requirement (16 chars without prefix) + pk, err := generateSecureToken(16) if err != nil { return nil, fmt.Errorf("failed to generate public key: %w", err) } - sk, err := generateSecureToken(TokenLength) + sk, err := generateSecureToken(16)
33-51: Function doesn't respect the requested length parameterThe function always returns a 64-character hex string because SHA256 produces a fixed output. To generate tokens of the requested length:
func generateSecureToken(length int) (string, error) { if length < TokenMinLength { - return "", fmt.Errorf("the token length should be >= %d", length) + return "", fmt.Errorf("token length must be at least %d", TokenMinLength) } - salt := make([]byte, length) + // Generate random bytes for half the length (hex encoding doubles it) + byteLength := (length + 1) / 2 + randomBytes := make([]byte, byteLength) - if _, err := rand.Read(salt); err != nil { - return "", fmt.Errorf("failed to generate secure tokens salt: %v", err) + if _, err := rand.Read(randomBytes); err != nil { + return "", fmt.Errorf("failed to generate secure token: %w", err) } - hasher := sha256.New() - hasher.Write(salt) - - // Get the resulting hash and encode it as a hex string. - hashBytes := hasher.Sum(nil) - - return hex.EncodeToString(hashBytes), nil + // Encode to hex and truncate to exact length if needed + token := hex.EncodeToString(randomBytes) + if len(token) > length { + token = token[:length] + } + + return token, nil }
64-64: Security issue: Remove token from error messageExposing the token value in error messages is a security risk.
-return fmt.Errorf("the given token [%s] is not valid", token) +return fmt.Errorf("invalid token format")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/auth/token.go(1 hunks)pkg/http/middleware/token_middleware.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/http/middleware/token_middleware.go (5)
database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/http/schema.go (2)
ApiHandler(23-23)ApiError(10-13)pkg/auth/token.go (2)
ValidateTokenFormat(53-65)SafeDisplay(74-89)database/model.go (1)
APIKey(26-35)pkg/auth/schema.go (1)
Token(12-17)
pkg/auth/token.go (1)
pkg/auth/schema.go (4)
Token(12-17)PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)TokenMinLength(8-8)
🔇 Additional comments (2)
pkg/http/middleware/token_middleware.go (1)
84-89: Good security practice with SafeDisplayThe use of
auth.SafeDisplayproperly addresses the security concern about exposing sensitive token data in error messages.pkg/auth/token.go (1)
74-89: Well-implemented token maskingThe
SafeDisplayfunction properly masks sensitive token data while showing enough information for debugging purposes.
gocanto
force-pushed
the
hotfix/gh-deploy-action
branch
from
03bdf20 to
aaa526d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini |
|
It looks like you tried to invoke me with If you're looking for a code review, please use Let me know if you have any other questions! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new authentication system based on API keys and signatures. The changes are extensive and improve security by moving away from static tokens. However, there are critical security issues around the storage of secret keys in plaintext and potential information leakage in the production reverse proxy configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pkg/http/middleware/token_middleware.go (2)
72-72: Fix typo in error message.There is a typo in the commented error message where "accout" should be "account".
- "invalid request. Please, provide a valid token, signature and accout name headers. [account: %s, public token: %s, signature: %s]", + "invalid request. Please, provide a valid token, signature and account name headers. [account: %s, public token: %s, signature: %s]",
70-82: Avoid logging sensitive token data in error messages.The commented error handling functions include actual token values in error messages, which poses a security risk by exposing sensitive authentication credentials in logs.
Remove token values from error messages when implementing the authentication logic.
pkg/auth/token.go (2)
60-84: Critical: Token generation ignores TokenMinLength parameter.The
generateSecureTokenmethod generates a fixed 64-character hex string from SHA256 hash regardless of theTokenMinLengthparameter. This creates tokens that are actually 67 characters long (3-char prefix + 64-char hash), which doesn't match the validation requirements from the schema (expecting 16-character tokens).This is the same issue identified in past reviews. The function should generate tokens of the actual requested length rather than a fixed SHA256 hash length.
86-98: Security issue: Token exposed in error message and incorrect validation logic.Two critical issues in this function:
- Line 97 exposes the token value in the error message, which is a security risk
- Line 93 uses OR logic but should use AND with negation - a token can only have one prefix, not both
Apply this diff to fix both issues:
-if strings.HasPrefix(token, PublicKeyPrefix) || strings.HasPrefix(token, SecretKeyPrefix) { +if strings.HasPrefix(token, PublicKeyPrefix) || strings.HasPrefix(token, SecretKeyPrefix) { return nil } -return fmt.Errorf("the given token [%s] is not valid", token) +return fmt.Errorf("invalid token format")
🧹 Nitpick comments (1)
pkg/auth/encryption.go (1)
11-35: Good cryptographic implementation with room for input validation.The
Encryptfunction correctly implements AES-GCM authenticated encryption with proper nonce generation and error handling. However, consider adding input validation for security hardening.func Encrypt(plaintext []byte, key []byte) ([]byte, error) { + if len(key) != 16 && len(key) != 24 && len(key) != 32 { + return nil, fmt.Errorf("invalid key length: expected 16, 24, or 32 bytes, got %d", len(key)) + } + if plaintext == nil { + return nil, fmt.Errorf("plaintext cannot be nil") + } + block, err := aes.NewCipher(key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.env.example(2 hunks)boost/factory.go(1 hunks)caddy/Caddyfile.prod(1 hunks)cli/accounts/factory.go(1 hunks)cli/accounts/handler.go(1 hunks)cli/main.go(3 hunks)database/attrs.go(1 hunks)database/infra/migrations/000002_api_keys.up.sql(1 hunks)database/model.go(2 hunks)database/repository/api_keys.go(1 hunks)database/repository/categories.go(1 hunks)database/repository/tags.go(1 hunks)database/repository/users.go(1 hunks)env/app.go(1 hunks)pkg/auth/encryption.go(1 hunks)pkg/auth/schema.go(1 hunks)pkg/auth/token.go(1 hunks)pkg/http/middleware/token_middleware.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/auth/schema.go
🚧 Files skipped from review as they are similar to previous changes (11)
- boost/factory.go
- database/infra/migrations/000002_api_keys.up.sql
- cli/accounts/handler.go
- database/attrs.go
- .env.example
- cli/accounts/factory.go
- env/app.go
- database/model.go
- caddy/Caddyfile.prod
- cli/main.go
- database/repository/api_keys.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/http/middleware/token_middleware.go (2)
database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/http/schema.go (2)
ApiHandler(23-23)ApiError(10-13)
pkg/auth/token.go (2)
pkg/auth/schema.go (6)
TokenMinLength(6-6)AccountNameMinLength(7-7)Token(10-16)PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)SecureToken(18-21)pkg/auth/encryption.go (1)
Encrypt(11-35)
🔇 Additional comments (8)
database/repository/categories.go (1)
26-26: Good refactoring to use database metadata for existence checks.Using
result.RowsAffected > 0is more reliable than checking field content and aligns with GORM best practices. This change improves consistency across repository implementations.database/repository/users.go (1)
26-26: Consistent improvement aligning with repository refactoring.This change matches the pattern applied across other repositories and correctly uses GORM's
RowsAffectedmetadata for existence validation.database/repository/tags.go (1)
48-48: Consistent refactoring completed across repositories.This change completes the standardization of existence checks using
RowsAffectedacross all repositoryFindBymethods, improving code consistency and reliability.pkg/auth/encryption.go (1)
37-64: Secure decryption implementation with proper validation.The
Decryptfunction correctly handles nonce extraction, ciphertext length validation, and authenticated decryption. The implementation follows cryptographic best practices.pkg/auth/token.go (4)
100-105: LGTM: Secure HMAC signature generation.The
CreateSignatureFromfunction correctly uses HMAC with SHA256 for secure signature generation. The implementation follows cryptographic best practices.
107-122: LGTM: Safe token display implementation.The
SafeDisplayfunction properly truncates sensitive tokens for safe logging/display while preserving the prefix for identification. The implementation correctly handles different prefix lengths.
124-138: LGTM: Secure signature validation using constant-time comparison.The token validation methods correctly use
hmac.Equalfor constant-time comparison, preventing timing attacks. The signature generation and validation logic is sound.
34-58: Generated tokens already satisfy minimum length validation
TheValidateTokenFormatfunction enforces only a minimum length of 16 characters, not an exact length. All tokens produced bygenerateSecureToken(prefix + 64-char SHA-256 hex) are 67 characters long—well above the 16-char minimum. No validation failures will occur, so no changes are required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/accounts/handler.go (1)
10-15: Add input validation for account name.The method doesn't validate the account name before processing. Consider adding validation to ensure the account name meets minimum requirements and doesn't contain potentially problematic characters.
pkg/auth/token.go (2)
27-29: Fix incorrect error message for account name validation.The error message incorrectly refers to "token length" when it should refer to "account name length" for the
accountNameMinLengthvalidation.Apply this diff to fix the error message:
if accountNameMinLength < AccountNameMinLength { - return nil, fmt.Errorf("the token length should be at least %d", AccountNameMinLength) + return nil, fmt.Errorf("the account name length should be at least %d", AccountNameMinLength) }
121-121: Security issue: Token exposed in error message.The error message exposes the token value, which is a security risk.
Apply this diff to fix the security issue:
- return fmt.Errorf("the given token [%s] is not valid", token) + return fmt.Errorf("invalid token format")
🧹 Nitpick comments (1)
cli/main.go (1)
63-71: Consider adding a comment to clarify the purpose.This signature generation appears to be for testing/debugging purposes. Consider adding a comment to clarify when and why this feature would be used.
+ // Generate test signature using local environment tokens case 4: signature := auth.CreateSignatureFrom(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cli/accounts/factory.go(1 hunks)cli/accounts/handler.go(1 hunks)cli/main.go(3 hunks)cli/panel/menu.go(3 hunks)pkg/auth/encryption.go(1 hunks)pkg/auth/schema.go(1 hunks)pkg/auth/token.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/panel/menu.go
- pkg/auth/schema.go
- cli/accounts/factory.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/auth/encryption.go (1)
pkg/auth/schema.go (1)
EncryptionKeyLength(8-8)
pkg/auth/token.go (2)
pkg/auth/schema.go (7)
TokenMinLength(6-6)AccountNameMinLength(7-7)EncryptionKeyLength(8-8)Token(11-17)SecureToken(19-22)PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)pkg/auth/encryption.go (2)
Decrypt(47-74)Encrypt(21-45)
cli/accounts/handler.go (4)
cli/accounts/factory.go (1)
Handler(11-16)pkg/auth/token.go (2)
TokenHandler(12-16)SafeDisplay(131-146)database/attrs.go (1)
APIKeyAttr(7-11)pkg/cli/message.go (3)
Successln(17-19)Blueln(41-43)Warningln(25-27)
🔇 Additional comments (14)
pkg/auth/encryption.go (3)
11-19: LGTM! Secure AES key generation implementation.The function correctly uses
crypto/randfor cryptographically secure random key generation and properly handles errors with descriptive messages.
21-45: LGTM! Secure AES-GCM encryption implementation.The function correctly implements AES-GCM authenticated encryption with proper nonce handling. The nonce is generated randomly for each encryption and prepended to the ciphertext, which is the standard approach for GCM mode.
47-74: LGTM! Secure AES-GCM decryption implementation.The function correctly implements AES-GCM decryption with proper nonce extraction and ciphertext validation. The integrity verification is handled automatically by the GCM mode's
Openmethod.cli/accounts/handler.go (1)
32-59: LGTM! Secure account retrieval and display implementation.The method properly handles account lookup, secure token decoding, and uses
SafeDisplayto avoid exposing full secret keys in the CLI output. The error handling and user feedback are well implemented.cli/main.go (4)
18-25: LGTM! Clean database initialization.The direct database connection initialization simplifies the architecture by removing the credential guard dependency and provides a clean foundation for the new account management features.
92-107: LGTM! Blog post creation function is well implemented.The function properly handles input capture, HTTP client creation, and post processing with appropriate error handling.
109-127: LGTM! API account creation function is properly implemented.The function handles account name capture, handler creation, and account creation with appropriate error handling throughout the workflow.
149-162: LGTM! App encryption key generation is properly implemented.The function uses the secure
GenerateAESKeyfunction and provides clear, well-formatted output for the generated encryption key.pkg/auth/token.go (6)
38-57: LGTM! Secure token decoding implementation.The method properly decrypts stored encrypted keys and constructs the Token struct with both plaintext and encrypted forms. Error handling includes proper context wrapping.
59-82: LGTM! Account setup method properly implemented.The method correctly validates account name length, generates secure token pairs, and returns a properly constructed Token struct. The previous issues with token length parameters have been resolved.
84-108: LGTM! Secure token generation with proper encryption.The method generates cryptographically secure tokens using configurable salt length, SHA256 hashing, and AES encryption. The approach addresses previous concerns about fixed token lengths by using the configurable
TokenMinLength.
124-129: LGTM! Proper HMAC-SHA256 signature implementation.The function correctly implements HMAC-SHA256 signature generation with proper key handling and hex encoding of the result.
131-146: LGTM! Safe display function properly implemented.The function correctly truncates sensitive token data for safe display while preserving the prefix for identification. This is essential for secure CLI output.
148-162: LGTM! Secure signature validation implementation.The methods properly implement signature validation using constant-time comparison (
hmac.Equal) to prevent timing attacks. The use ofCreateSignatureFromensures consistency in signature generation and validation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/accounts/handler.go (1)
10-30: Input validation handled by TokenHandler.SetupNewAccountThe method correctly delegates account name validation to
TokenHandler.SetupNewAccountwhich performs the necessary validation checks as seen inpkg/auth/handler.golines 53-55. The error handling and database operations are well implemented.cli/main.go (1)
125-143: Error handling correctly implementedThe error handling pattern correctly captures and returns errors from
ReadAccount, addressing the issue identified in previous reviews.pkg/http/middleware/token_middleware.go (1)
85-97: Fix typo in error messageThere's a typo in the error message where "accout" should be "account".
- "invalid request. Please, provide a valid token, signature and accout name headers. [account: %s, public token: %s, signature: %s]", + "invalid request. Please, provide a valid token, signature and account name headers. [account: %s, public token: %s, signature: %s]",
🧹 Nitpick comments (5)
pkg/auth/render.go (1)
5-20: Consider making the visible character count configurable.The
SafeDisplayfunction correctly handles secret truncation for both public and secret key prefixes. However, the hardcodedvisibleChars = 10reduces flexibility for different security requirements.Consider making this configurable:
-func SafeDisplay(secret string) string { +func SafeDisplay(secret string) string { + return SafeDisplayWithLength(secret, 10) +} + +func SafeDisplayWithLength(secret string, visibleChars int) string { var prefixLen int - visibleChars := 10This maintains backward compatibility while allowing customization when needed.
pkg/auth/encryption.go (1)
87-99: Token format validation ensures minimum security requirements.The validation correctly checks for:
- Non-empty tokens with minimum length requirements
- Proper prefix validation for public/secret keys
- Clear error messages for debugging
Consider improving the error message to avoid exposing the actual token value:
- return fmt.Errorf("the given token [%s] is not valid", token) + return fmt.Errorf("the given token format is not valid")This prevents sensitive data from appearing in error logs.
cli/main.go (1)
165-178: Consider security implications of displaying encryption keysWhile the function correctly generates and displays the encryption key, outputting cryptographic keys to the console could pose security risks if console output is logged or visible to unauthorized users. Consider adding a warning or implementing secure key display practices.
pkg/http/middleware/token_middleware.go (2)
106-118: Consider using SafeDisplay for signature in error messagesWhile the code correctly uses
auth.SafeDisplayfor tokens, the signature is displayed in full in error messages. Consider applyingSafeDisplayto the signature as well for consistency in sensitive data handling.- signature, + auth.SafeDisplay(signature),
74-78: Fix typo in log messageThere's a typo in the log message where "tour" should be "your".
- slog.Error(fmt.Sprintf("the given public token does not match tour records [%s]: %v", item.AccountName, err)) + slog.Error(fmt.Sprintf("the given public token does not match your records [%s]: %v", item.AccountName, err))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
boost/app.go(2 hunks)boost/router.go(1 hunks)cli/accounts/factory.go(1 hunks)cli/accounts/handler.go(1 hunks)cli/main.go(3 hunks)cli/panel/menu.go(3 hunks)main.go(2 hunks)pkg/auth/encryption.go(1 hunks)pkg/auth/handler.go(1 hunks)pkg/auth/render.go(1 hunks)pkg/http/middleware/pipeline.go(1 hunks)pkg/http/middleware/token_middleware.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/http/middleware/pipeline.go
- cli/panel/menu.go
- cli/accounts/factory.go
🧰 Additional context used
🧬 Code Graph Analysis (8)
boost/router.go (3)
pkg/http/middleware/pipeline.go (1)
Pipeline(10-14)pkg/auth/handler.go (1)
TokenHandler(10-14)database/repository/api_keys.go (1)
ApiKeys(11-13)
main.go (1)
boost/app.go (1)
MakeApp(24-56)
pkg/auth/render.go (1)
pkg/auth/schema.go (2)
PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)
boost/app.go (6)
env/env.go (1)
Environment(8-14)pkg/auth/handler.go (2)
MakeTokensHandler(16-26)TokenHandler(10-14)boost/factory.go (1)
MakeDbConnection(37-45)boost/router.go (1)
Router(11-15)pkg/http/middleware/pipeline.go (1)
Pipeline(10-14)database/repository/api_keys.go (1)
ApiKeys(11-13)
pkg/auth/encryption.go (1)
pkg/auth/schema.go (4)
EncryptionKeyLength(8-8)TokenMinLength(6-6)PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)
pkg/auth/handler.go (2)
pkg/auth/schema.go (7)
TokenMinLength(6-6)AccountNameMinLength(7-7)EncryptionKeyLength(8-8)Token(11-17)SecureToken(19-22)PublicKeyPrefix(4-4)SecretKeyPrefix(5-5)pkg/auth/encryption.go (2)
Decrypt(51-78)Encrypt(25-49)
cli/main.go (7)
database/connection.go (1)
Connection(12-17)boost/factory.go (1)
MakeDbConnection(37-45)pkg/cli/message.go (4)
Errorln(9-11)Error(5-7)Successln(17-19)Magentaln(33-35)cli/panel/menu.go (1)
Menu(17-21)cli/accounts/factory.go (2)
MakeHandler(18-33)Handler(11-16)cli/posts/factory.go (2)
MakeHandler(22-33)Handler(14-20)pkg/auth/encryption.go (1)
GenerateAESKey(15-23)
pkg/http/middleware/token_middleware.go (6)
database/repository/api_keys.go (1)
ApiKeys(11-13)pkg/auth/handler.go (1)
TokenHandler(10-14)pkg/http/schema.go (2)
ApiHandler(23-23)ApiError(10-13)pkg/auth/encryption.go (2)
ValidateTokenFormat(87-99)CreateSignatureFrom(80-85)database/model.go (1)
APIKey(26-35)pkg/auth/render.go (1)
SafeDisplay(5-20)
🔇 Additional comments (17)
boost/router.go (1)
18-21: Token middleware properly integrates username, token, and signature validationThe new
TokenCheckMiddlewarestill enforces:
- Presence of the
X-API-Username,X-API-Key, andX-API-Signatureheaders- Lookup of the account in
ApiKeys(rejects if not found)- Token format validation via
auth.ValidateTokenFormat- Public-key match and signature verification using
TokenHandlerWith all checks consolidated into a single middleware, the separate
UsernameCheckstage is no longer needed. No gaps were found in the implementation.Looks good to merge.
main.go (1)
17-23: Proper error handling added for the updated MakeApp function.The changes correctly handle the new error-returning signature of
MakeApp. Panicking in theinit()function for initialization failures is appropriate, and the formatted error message provides clear context.boost/app.go (2)
24-31: Proper error handling added for token handler initialization.The error handling for
auth.MakeTokensHandleris well-implemented with descriptive error wrapping. This ensures initialization failures are properly communicated rather than causing silent issues later.
46-50: Authentication pipeline properly configured with new components.The middleware pipeline initialization correctly integrates the new
ApiKeysrepository andTokenHandler, enabling the database-backed authentication system while maintaining the existing environment configuration.pkg/auth/encryption.go (4)
15-23: AES key generation follows cryptographic best practices.The function correctly uses
crypto/randfor generating cryptographically secure random keys with the proper length defined byEncryptionKeyLength.
25-49: AES-GCM encryption implementation is secure and well-documented.The encryption function properly implements AES-GCM with:
- Unique nonce generation for each encryption
- Authenticated encryption providing both confidentiality and integrity
- Proper nonce prepending for use during decryption
The comments clearly explain the cryptographic concepts, which is excellent for maintainability.
51-78: Decryption function includes proper validation and error handling.The function correctly:
- Validates ciphertext length before processing
- Extracts the nonce from the prepended position
- Uses GCM's built-in authentication to prevent tampering
- Returns appropriate errors for invalid inputs
80-85: HMAC signature generation uses appropriate cryptographic hash.The
CreateSignatureFromfunction correctly implements HMAC-SHA256 for message authentication, which is suitable for API token signatures.cli/accounts/handler.go (1)
32-60: Well-implemented account reading with proper security practicesThe method correctly handles missing accounts, safely displays sensitive information using
auth.SafeDisplay, and provides clear separation between decrypted and encrypted values. The CLI formatting enhances user experience.pkg/auth/handler.go (4)
16-26: Proper encryption key validationThe constructor correctly validates the encryption key length before creating the handler, which is essential for cryptographic security.
28-47: Secure token decoding with proper error handlingThe method correctly decrypts both keys with appropriate error wrapping and returns a complete Token structure containing both plaintext and encrypted versions.
49-72: Well-implemented account setup with proper validationThe method includes proper account name length validation and secure token generation for both public and secret keys with comprehensive error handling.
74-98: Cryptographically secure token generationThe method uses appropriate cryptographic primitives (
crypto/rand, SHA-256) and proper error handling throughout the token generation and encryption process.cli/main.go (2)
88-103: Clean blog post creation implementationThe function follows good practices with proper error handling and clear separation of concerns by delegating to the posts handler.
105-163: Consistent and well-structured account management functionsAll account management functions follow a consistent pattern with proper error handling, input validation, and clear separation of concerns.
pkg/http/middleware/token_middleware.go (2)
30-53: Authentication logic is properly implementedThe middleware correctly implements token validation with header extraction, format validation, and rejection logic. The previous concern about bypassed authentication appears to have been resolved.
55-83: Public key verification correctly implementedThe method properly verifies that the provided public token matches the stored public key (line 74) and validates the signature, addressing the security concerns about token/key matching.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores