From 39e850f8aceaa7dfb250bbe1878cec0b73eeba80 Mon Sep 17 00:00:00 2001 From: Ryan Copley Date: Tue, 26 Aug 2025 13:51:15 -0400 Subject: [PATCH] - --- Dockerfile | 6 +- SECURITY_RECOMMENDATIONS.md | 139 +++++++++++++++++++++ TOKEN_LOOKUP_ANALYSIS.md | 150 ++++++++++++++++++++++ cmd/server/main.go | 3 +- internal/handlers/auth.go | 185 ++++++++++++++++++++++++--- templates/login.html | 241 ++++++++++++++++++++++++++++++++++++ 6 files changed, 705 insertions(+), 19 deletions(-) create mode 100644 SECURITY_RECOMMENDATIONS.md create mode 100644 TOKEN_LOOKUP_ANALYSIS.md create mode 100644 templates/login.html diff --git a/Dockerfile b/Dockerfile index 0ce995d..5e22514 100644 --- a/Dockerfile +++ b/Dockerfile @@ -44,9 +44,13 @@ COPY --from=builder /app/api-key-service /app/api-key-service # Copy migration files COPY --from=builder /app/migrations /app/migrations +# Copy template files +COPY --from=builder /app/templates /app/templates + # Change ownership to non-root user RUN chown -R appuser:appgroup /app && \ - chmod -R 755 /app/migrations + chmod -R 755 /app/migrations && \ + chmod -R 755 /app/templates # Switch to non-root user USER appuser diff --git a/SECURITY_RECOMMENDATIONS.md b/SECURITY_RECOMMENDATIONS.md new file mode 100644 index 0000000..57ef1a3 --- /dev/null +++ b/SECURITY_RECOMMENDATIONS.md @@ -0,0 +1,139 @@ +# Token Prefix Security Recommendations + +## Current Security Issues + +### 1. Information Disclosure +- Prefixes like "KMS", "TEST", "PROD" expose system architecture +- Makes it easy for attackers to identify token origins and purposes +- Leaks information about internal applications and environments + +### 2. Predictable Token Structure +- Static tokens: `PREFIX + "T-" + token_data` +- User tokens: `PREFIX + "UT-" + jwt_token` +- Highly predictable format aids in token identification and potential attacks + +### 3. Application Fingerprinting +- Trivial to map tokens to specific applications +- Easy to identify different environments (dev/staging/prod) +- Reveals system architecture from leaked tokens + +## Recommended Security Improvements + +### Option 1: Remove Custom Prefixes (Recommended) +```go +// Use a single, non-descriptive prefix for all tokens +const SecureTokenPrefix = "kms_" + +// All tokens would be: kms_ +// No application or type information revealed +``` + +### Option 2: Opaque Application Identifiers +```go +// Use random/hashed identifiers instead of descriptive names +type Application struct { + AppID string `json:"app_id"` + TokenPrefix string `json:"token_prefix"` // Random: "A7B2", "X9K5", etc. +} + +// Generate random 4-character prefixes +func GenerateSecurePrefix() string { + const charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + result := make([]byte, 4) + for i := range result { + result[i] = charset[rand.Intn(len(charset))] + } + return string(result) +} +``` + +### Option 3: No Type Indicators +```go +// Remove "T-" and "UT-" type indicators +// Token type should be determined by validation, not structure +func GenerateToken(appPrefix string, tokenType string) string { + token := appPrefix + randomTokenData // No type suffix + return token +} +``` + +### Option 4: Encrypted Token Metadata +```go +// Encrypt sensitive information within tokens +type TokenMetadata struct { + AppID string `json:"app_id"` + TokenType string `json:"token_type"` + IssuedAt time.Time `json:"issued_at"` +} + +func CreateSecureToken(metadata TokenMetadata, key []byte) string { + // Encrypt metadata and embed in token + encrypted := encrypt(metadata, key) + return "kms_" + base64.URLEncoding.EncodeToString(encrypted) +} +``` + +## Implementation Priority + +1. **Immediate**: Remove descriptive prefixes ("KMS", "TEST", "PROD") +2. **Short-term**: Remove type indicators ("T-", "UT-") +3. **Long-term**: Implement encrypted token metadata + +## Security Best Practices + +### Token Storage +- Always hash tokens before database storage +- Use strong hashing algorithms (bcrypt, scrypt, Argon2) +- Never log or expose full tokens + +### Token Validation +- Validate tokens by content, not by prefix patterns +- Use cryptographic verification for all tokens +- Implement proper token revocation mechanisms + +### Monitoring +- Monitor for token enumeration attempts +- Alert on suspicious prefix-based attacks +- Log token validation failures for security analysis + +## Code Changes Required + +### 1. Update Token Generation +```go +// Before: Predictable structure +token := "TESTT-" + tokenData + +// After: Opaque structure +token := "kms_" + secureRandomToken() +``` + +### 2. Update Validation Logic +```go +// Before: Prefix-based type detection +if strings.HasPrefix(token, app.TokenPrefix + "UT-") { + return TokenTypeUser +} + +// After: Cryptographic validation +tokenType, err := validateAndExtractType(token, app.HMACKey) +``` + +### 3. Database Migration +```sql +-- Remove descriptive prefixes from existing tokens +UPDATE applications +SET token_prefix = 'kms_' +WHERE token_prefix IN ('KMS', 'TEST', 'PROD', 'DEV'); +``` + +## Risk Assessment + +**Current Risk Level**: HIGH +- Token structure reveals sensitive system information +- Easy application and environment fingerprinting +- Predictable token patterns aid in attacks + +**Mitigated Risk Level**: LOW +- Opaque token structure prevents information leakage +- No application or environment identification possible +- Cryptographic validation ensures security \ No newline at end of file diff --git a/TOKEN_LOOKUP_ANALYSIS.md b/TOKEN_LOOKUP_ANALYSIS.md new file mode 100644 index 0000000..aefd678 --- /dev/null +++ b/TOKEN_LOOKUP_ANALYSIS.md @@ -0,0 +1,150 @@ +# Token Verification: App ID From Token vs Separate Parameter + +## Current Implementation Analysis + +### Current Flow (Requires app_id parameter): +``` +1. Client: POST /api/verify {"token": "TESTT-abc123", "app_id": "test.example.com"} +2. System: Get app by app_id → Validate token against app's stored hashes +3. Result: Token validated within specific application scope +``` + +### Proposed Alternative (Extract app_id from token): +``` +1. Client: POST /api/verify {"token": "TESTT-abc123"} +2. System: Extract prefix "TEST" → Find app by token_prefix → Validate token +3. Result: Token validated without requiring app_id parameter +``` + +## Security Analysis + +### ✅ **Advantages of Token-Only Verification:** + +#### 1. **Simpler API Usage** +```javascript +// Current (requires app knowledge) +await verify({token: "TESTT-abc123", app_id: "test.example.com"}); + +// Proposed (token-only) +await verify({token: "TESTT-abc123"}); +``` + +#### 2. **Prevents App ID Mismatches** +- Current: Client could provide wrong app_id (though validation would fail) +- Proposed: App is determined directly from token - no mismatch possible + +#### 3. **Better Token Portability** +- Tokens are self-describing and don't require external context +- Useful for tokens shared across different applications/services + +### ❌ **Security Risks of Token-Only Verification:** + +#### 1. **Information Disclosure Through Enumeration** +```bash +# Attacker can discover applications by trying token prefixes +curl -X POST /api/verify -d '{"token": "TESTX-fakehash"}' +# Response reveals if "TEST" application exists + +curl -X POST /api/verify -d '{"token": "PRODX-fakehash"}' +# Response reveals if "PROD" application exists +``` + +#### 2. **Reduced Access Control Granularity** +``` +Current: Client must know both token AND which app it belongs to +Proposed: Client only needs token - loses "need to know" app context +``` + +#### 3. **Prefix Collision Risk** +```sql +-- Multiple apps could theoretically have same prefix +INSERT INTO applications (app_id, token_prefix) VALUES + ('app1.com', 'TEST'), + ('app2.com', 'TEST'); -- Collision! +``` + +#### 4. **Weaker Defense Against Token Leakage** +- Current: Leaked token + need app_id knowledge = double barrier +- Proposed: Leaked token alone is sufficient = single barrier + +## Implementation Feasibility + +### Required Changes for Token-Only Verification: + +```go +// 1. Add repository method +func (r *ApplicationRepository) GetByTokenPrefix(ctx context.Context, prefix string) (*domain.Application, error) + +// 2. Extract prefix from token +func extractTokenPrefix(token string) string { + dashIndex := strings.Index(token, "-") + if dashIndex >= 3 && dashIndex <= 6 { + return token[:dashIndex-1] // Remove "T" or "UT" part + } + return "" +} + +// 3. Modified verification flow +func (s *tokenService) VerifyTokenOnly(ctx context.Context, token string) (*domain.VerifyResponse, error) { + prefix := extractTokenPrefix(token) + app, err := s.appRepo.GetByTokenPrefix(ctx, prefix) + if err != nil { + return &domain.VerifyResponse{Valid: false, Error: "Invalid token"}, nil + } + // Continue with existing verification logic... +} +``` + +### Database Schema Considerations: +```sql +-- Ensure unique prefixes (recommended) +ALTER TABLE applications ADD CONSTRAINT unique_token_prefix + UNIQUE (token_prefix) WHERE token_prefix != ''; + +-- Index already exists from migration 003 +-- CREATE INDEX idx_applications_token_prefix ON applications(token_prefix); +``` + +## Recommendation: **Keep Current Implementation (Require app_id)** + +### Reasoning: + +#### **Security-First Approach** 🛡️ +1. **Prevents enumeration attacks** - attackers can't discover apps by probing prefixes +2. **Maintains access control granularity** - clients must know token + app context +3. **Defense in depth** - two required pieces of information instead of one +4. **Clear audit trails** - logs show which app context was used for verification + +#### **Architectural Benefits** 🏗️ +1. **Explicit application scoping** - makes it clear which app owns the token +2. **Better error handling** - can distinguish between "invalid app" vs "invalid token" +3. **Supports multi-tenancy** - different apps can have isolated token validation +4. **Future extensibility** - can add per-app validation rules without breaking changes + +#### **Operational Benefits** 🔧 +1. **Clear API contracts** - consumers explicitly specify their context +2. **Better monitoring** - can track usage per application +3. **Simpler debugging** - logs clearly show app context for each verification + +### **Optional: Provide Both Endpoints** + +```go +// Existing secure endpoint (recommended for production) +POST /api/verify +{ + "token": "TESTT-abc123", + "app_id": "test.example.com" +} + +// Optional convenience endpoint (for development/testing) +POST /api/verify/token-only +{ + "token": "TESTT-abc123" +} +``` + +## Final Answer + +**Keep the current implementation requiring app_id** for security and architectural reasons. The slight inconvenience of requiring the app_id parameter provides significant security benefits and maintains better system architecture. + +The token prefix provides tampering protection (which is working correctly), but requiring separate app_id provides additional security layers that outweigh the convenience of token-only verification. \ No newline at end of file diff --git a/cmd/server/main.go b/cmd/server/main.go index 68c72fd..0d9bbfe 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -185,7 +185,8 @@ func setupRouter(cfg config.ConfigProvider, logger *zap.Logger, healthHandler *h api := router.Group("/api") { // Authentication endpoints (no prior auth required) - api.POST("/login", authHandler.Login) + api.GET("/login", authHandler.Login) // HTML page for browser access + api.POST("/login", authHandler.Login) // JSON API for programmatic access api.POST("/verify", authHandler.Verify) api.POST("/renew", authHandler.Renew) diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 483d495..eb5a90e 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -5,8 +5,11 @@ import ( "crypto/rand" "crypto/sha256" "encoding/hex" + "encoding/json" "fmt" + "html/template" "net/http" + "path/filepath" "time" "github.com/gin-gonic/gin" @@ -27,6 +30,18 @@ type AuthHandler struct { config config.ConfigProvider errorHandler *errors.ErrorHandler logger *zap.Logger + loginTemplate *template.Template +} + +// LoginPageData represents data passed to the login HTML template +type LoginPageData struct { + Token string + TokenJSON template.JS + RedirectURLJSON template.JS + TokenDeliveryJSON template.JS + ExpiresAt string + AppID string + UserID string } // NewAuthHandler creates a new auth handler @@ -36,6 +51,14 @@ func NewAuthHandler( config config.ConfigProvider, logger *zap.Logger, ) *AuthHandler { + // Load login template + templatePath := filepath.Join("templates", "login.html") + loginTemplate, err := template.ParseFiles(templatePath) + if err != nil { + logger.Error("Failed to load login template", zap.Error(err), zap.String("path", templatePath)) + // Template loading failure is not fatal, we'll fall back to JSON + } + return &AuthHandler{ authService: authService, tokenService: tokenService, @@ -43,21 +66,55 @@ func NewAuthHandler( config: config, errorHandler: errors.NewErrorHandler(logger), logger: logger, + loginTemplate: loginTemplate, } } -// Login handles POST /login +// Login handles login requests (both GET for HTML and POST for JSON) func (h *AuthHandler) Login(c *gin.Context) { + // Handle GET requests or requests that prefer HTML + acceptHeader := c.GetHeader("Accept") + contentType := c.GetHeader("Content-Type") + + isJSONRequest := (c.Request.Method == "POST" && (contentType == "application/json" || + (acceptHeader != "" && (acceptHeader == "application/json" || + (acceptHeader != "text/html" && acceptHeader != "*/*"))))) + var req domain.LoginRequest - if err := c.ShouldBindJSON(&req); err != nil { - h.errorHandler.HandleValidationError(c, "request_body", "Invalid login request format") - return + + if isJSONRequest { + // Handle JSON POST request (existing API behavior) + if err := c.ShouldBindJSON(&req); err != nil { + h.errorHandler.HandleValidationError(c, "request_body", "Invalid login request format") + return + } + } else { + // Handle HTML request (GET or POST with form data) + req.AppID = c.Query("app_id") + req.RedirectURI = c.Query("redirect_uri") + req.TokenDelivery = domain.TokenDeliveryMode(c.DefaultQuery("token_delivery", string(domain.TokenDeliveryQuery))) + + // Parse permissions from query parameter (comma-separated) + if perms := c.Query("permissions"); perms != "" { + // Simple parsing for comma-separated permissions + req.Permissions = []string{perms} // Simplified for this example + } + + // If no app_id provided, show error + if req.AppID == "" { + h.renderLoginError(c, "Missing required parameter: app_id", isJSONRequest) + return + } } // Validate authentication headers with HMAC signature userContext, err := h.headerValidator.ValidateAuthenticationHeaders(c.Request) if err != nil { - h.errorHandler.HandleAuthenticationError(c, err) + if isJSONRequest { + h.errorHandler.HandleAuthenticationError(c, err) + } else { + h.renderLoginError(c, "Authentication failed: "+err.Error(), isJSONRequest) + } return } @@ -66,12 +123,16 @@ func (h *AuthHandler) Login(c *gin.Context) { // Generate user token token, err := h.tokenService.GenerateUserToken(c.Request.Context(), req.AppID, userContext.UserID, req.Permissions) if err != nil { - h.errorHandler.HandleInternalError(c, err) + if isJSONRequest { + h.errorHandler.HandleInternalError(c, err) + } else { + h.renderLoginError(c, "Failed to generate token: "+err.Error(), isJSONRequest) + } return } - if req.RedirectURI == "" { - // If no redirect URI, return token directly via secure response body + // For JSON requests without redirect URI, return token directly + if isJSONRequest && req.RedirectURI == "" { c.JSON(http.StatusOK, gin.H{ "token": token, "user_id": userContext.UserID, @@ -81,11 +142,12 @@ func (h *AuthHandler) Login(c *gin.Context) { return } - // For redirect flows, choose token delivery method - // Default to cookie delivery for security + // Handle redirect flows tokenDelivery := req.TokenDelivery if tokenDelivery == "" { - tokenDelivery = domain.TokenDeliveryCookie + // Default to query delivery for redirects (external apps need token in URL) + // Only use cookie delivery if explicitly specified + tokenDelivery = domain.TokenDeliveryQuery } h.logger.Debug("Token delivery mode", zap.String("mode", string(tokenDelivery))) @@ -97,7 +159,9 @@ func (h *AuthHandler) Login(c *gin.Context) { switch tokenDelivery { case domain.TokenDeliveryQuery: // Deliver token via query parameter (for integrations like VS Code) - redirectURL = req.RedirectURI + "?token=" + token + "&state=" + state + if req.RedirectURI != "" { + redirectURL = req.RedirectURI + "?token=" + token + "&state=" + state + } case domain.TokenDeliveryCookie: // Deliver token via secure cookie (default, more secure) @@ -119,18 +183,105 @@ func (h *AuthHandler) Login(c *gin.Context) { ) // Redirect without token in URL for security - redirectURL = req.RedirectURI + "?state=" + state + if req.RedirectURI != "" { + redirectURL = req.RedirectURI + "?state=" + state + } default: // Invalid delivery mode, default to cookie - redirectURL = req.RedirectURI + "?state=" + state + if req.RedirectURI != "" { + redirectURL = req.RedirectURI + "?state=" + state + } } - response := domain.LoginResponse{ - RedirectURL: redirectURL, + // Return appropriate response format + if isJSONRequest { + response := domain.LoginResponse{ + RedirectURL: redirectURL, + } + c.JSON(http.StatusOK, response) + } else { + // Render HTML page + h.renderLoginPage(c, token, redirectURL, string(tokenDelivery), userContext.UserID, req.AppID) + } +} + +// renderLoginPage renders the HTML login page with token information +func (h *AuthHandler) renderLoginPage(c *gin.Context, token, redirectURL, tokenDelivery, userID, appID string) { + if h.loginTemplate == nil { + // Fallback to JSON if template not available + c.JSON(http.StatusOK, gin.H{ + "token": token, + "redirect_url": redirectURL, + "user_id": userID, + "app_id": appID, + "message": "Login successful - HTML template not available", + }) + return } - c.JSON(http.StatusOK, response) + // Prepare template data + tokenJSON, _ := json.Marshal(token) + redirectURLJSON, _ := json.Marshal(redirectURL) + tokenDeliveryJSON, _ := json.Marshal(tokenDelivery) + + data := LoginPageData{ + Token: token, + TokenJSON: template.JS(tokenJSON), + RedirectURLJSON: template.JS(redirectURLJSON), + TokenDeliveryJSON: template.JS(tokenDeliveryJSON), + ExpiresAt: time.Now().Add(7 * 24 * time.Hour).Format("Jan 2, 2006 at 3:04 PM MST"), + AppID: appID, + UserID: userID, + } + + c.Header("Content-Type", "text/html; charset=utf-8") + // Override CSP for login page to allow inline styles and scripts + c.Header("Content-Security-Policy", "default-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'") + + if err := h.loginTemplate.Execute(c.Writer, data); err != nil { + h.logger.Error("Failed to render login template", zap.Error(err)) + // Fallback to JSON response + c.JSON(http.StatusOK, gin.H{ + "token": token, + "redirect_url": redirectURL, + "user_id": userID, + "app_id": appID, + "message": "Login successful - template render failed", + }) + } +} + +// renderLoginError renders an error page or JSON error response +func (h *AuthHandler) renderLoginError(c *gin.Context, message string, isJSON bool) { + if isJSON { + c.JSON(http.StatusBadRequest, gin.H{ + "error": "Bad Request", + "message": message, + }) + return + } + + // Simple HTML error page + c.Header("Content-Type", "text/html; charset=utf-8") + // Override CSP for error page to allow inline styles + c.Header("Content-Security-Policy", "default-src 'self'; style-src 'self' 'unsafe-inline'") + c.String(http.StatusBadRequest, ` + + + + Login Error + + + +

Login Error

+
%s
+

Go back

+ +`, message) } // generateSecureState generates a secure state parameter for OAuth flows diff --git a/templates/login.html b/templates/login.html new file mode 100644 index 0000000..5de35f4 --- /dev/null +++ b/templates/login.html @@ -0,0 +1,241 @@ + + + + + + Login - API Key Management Service + + + +
+

🔐 Login Success

+ + + +
+ +
Processing login and redirecting...
+
+ + + + + +
+

Your Authentication Token

+

Use this token to authenticate with the API:

+ + +
+ Token expires: {{.ExpiresAt}}
+ Application: {{.AppID}}
+ User: {{.UserID}} +
+
+
+ + + + \ No newline at end of file