Skip to content

Commit 2f5ab89

Browse files
[PPSC-563] feat(auth): remove auth-endpoint flag (#98)
* [PPSC-563] feat(auth): remove auth-endpoint flag and add region support Simplify CLI authentication by removing the --auth-endpoint flag entirely. Auth now routes through moose's proxy endpoint automatically, with the base URL derived from the --dev flag or hardcoded production endpoint. Changes: - Replace AuthEndpoint config with BaseURL (derived from getAPIBaseURL) - Update auth client endpoint from /api/v1/authenticate to /api/v1/auth/token - Remove --auth-endpoint flag and ARMIS_AUTH_ENDPOINT environment variable - Add region claim support to JWT parsing for deployment-aware routing - Rename endpoint → baseURL in NewAuthClient for semantic clarity - Update error messages to reflect base URL terminology - Update all tests and documentation This simplifies the auth flow by removing a user-configurable endpoint, reducing attack surface and improving UX when configuring authentication. * fix(auth): check os.Setenv/Unsetenv error returns in tests * fix(docs): address PR review comments on auth documentation - Update NewAuthProvider comment to say "base URL" instead of "endpoint" - Document ARMIS_API_URL in FEATURES.md and CLAUDE.md env var sections * feat(auth): add region caching with retry and shared cache utility Add region caching to optimize JWT authentication by persisting the discovered region to disk and reusing it on subsequent CLI invocations. Key changes: - Add --region flag for explicit region override (bypasses auto-discovery) - Implement file-based region cache (~/.cache/armis-cli/region-cache.json) - Add automatic retry without region hint when cached region auth fails - Memoize cached region in AuthProvider to avoid repeated disk I/O - Extract shared cache directory utility (util.GetCacheDir/GetCacheFilePath) - Add comprehensive tests for region cache (client ID mismatch, corrupt JSON, permissions, etc.) Region selection priority: 1. --region flag - explicit override 2. Cached region - from previous successful auth 3. Auto-discovery - server tries regions until one succeeds Addresses review findings: - F1: No tests for region cache → 249 lines of tests added - F2: No retry on stale cache → Retry logic with usingCachedHint flag - F3: Redundant writes → Skip if region unchanged - F4: Repeated disk I/O → Memoization via cachedRegion/regionLoaded - F5: Duplicated cache constant → Extracted to util.CacheDirName * fix(test): make cache tests Windows-compatible - Skip file permissions test on Windows (Unix permissions not supported) - Use filepath.Join for platform-agnostic path separators in test assertions * fix(util): prevent path traversal in GetCacheFilePath (CWE-22) Address PR review comments from Copilot: - Reject absolute paths and path separators in cache filename parameter to prevent filepath.Join from discarding the cache directory - Add containment check to ensure result stays within cache directory - Fix incorrect test assumptions about filepath.Join behavior - Use temp directory in TestPackageLevelFunctions to avoid modifying real user cache during tests - Document ARMIS_REGION environment variable in FEATURES.md and CLAUDE.md * fix(util): harden cache path validation and add region retry test Address follow-up PR review comments: - Reject empty, whitespace-only, ".", and ".." filenames in GetCacheFilePath to prevent returning directory path instead of file path - Replace strings.HasPrefix with filepath.Rel for robust path containment check (handles case-insensitivity and path separator edge cases) - Add TestAuthProvider_CachedRegionRetryOnFailure to cover region-caching retry logic when auth fails with stale cached region - Fix CLAUDE.md: ARMIS_REGION is for region-aware authentication, not API URL selection * fix(auth): add Bearer prefix to JWT Authorization header The CLI was sending raw JWT tokens without the "Bearer" prefix, but the backend middleware expects "Authorization: Bearer <token>" per RFC 6750. This caused 401 errors when using JWT authentication. Changes: - auth.go: Return "Bearer " + token for JWT auth - client.go: Update comments to match actual behavior - auth_test.go: Update tests to expect Bearer prefix * fix(auth): address PR review comments for region cache and retry logic - Fix test containment assertion to use filepath.Rel instead of strings.HasPrefix for robust path-boundary checking - Bound RegionCache.Load file reads with 4KB size limit to prevent memory exhaustion from corrupted cache files - Add typed AuthError with HTTP status code to enable selective retry: only retry on region-specific rejections (not 401/transport errors) - Add deprecation warning when ARMIS_AUTH_ENDPOINT env var is set, directing users to ARMIS_API_URL or --region - Update CI docs to recommend JWT auth credentials
1 parent e237621 commit 2f5ab89

16 files changed

Lines changed: 1102 additions & 118 deletions

CLAUDE.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ go test -v ./internal/output/... -run TestHumanFormatter
4343

4444
### Core Packages
4545

46-
- `internal/auth/` - Authentication provider supporting two modes. JWT (priority): client credentials exchange at `/api/v1/authenticate`, auto-refresh 5min before expiry, tenant ID extracted from `customer_id` JWT claim. Basic (fallback): static token + explicit tenant ID. Implements `AuthHeaderProvider` interface used by the API client.
46+
- `internal/auth/` - Authentication provider supporting two modes. JWT (priority): client credentials exchange at `/api/v1/auth/token`, auto-refresh 5min before expiry, tenant ID extracted from `customer_id` JWT claim. Basic (fallback): static token + explicit tenant ID. Implements `AuthHeaderProvider` interface used by the API client.
4747
- `internal/api/` - API client for Armis Cloud. Two HTTP clients: one for general calls (60s timeout), one for uploads (streaming, no timeout, no retry). Functional options pattern (`WithHTTPClient()`, `WithUploadHTTPClient()`, `WithAllowLocalURLs()`). Upload uses `io.Pipe` streaming to avoid OOM on large files. Enforces HTTPS, validates presigned S3 URLs against SSRF.
4848
- `internal/model/` - Data structures: `Finding` (23 fields), `ScanResult`, `Summary`, `Fix`, `FindingValidation` (with taint/reachability analysis), API response types (`NormalizedFinding`, pagination).
4949
- `internal/output/` - Output formatters (human, json, sarif, junit) implementing the `Formatter` interface. `styles.go` defines ~50 lipgloss styles using Tailwind CSS color palette. `icons.go` defines Unicode constants (severity dots, box-drawing chars). `SyncColors()` switches between full-color and plain styles based on `cli.ColorsEnabled()`.
@@ -83,9 +83,10 @@ go test -v ./internal/output/... -run TestHumanFormatter
8383

8484
- `ARMIS_CLIENT_ID` - Client ID for JWT authentication (recommended)
8585
- `ARMIS_CLIENT_SECRET` - Client secret for JWT authentication
86-
- `ARMIS_AUTH_ENDPOINT` - JWT authentication service endpoint URL
8786
- `ARMIS_API_TOKEN` - API token for Basic authentication (fallback)
8887
- `ARMIS_TENANT_ID` - Tenant identifier (required only with Basic auth; JWT extracts it from token)
88+
- `ARMIS_API_URL` - Override base URL for Armis API (advanced; defaults based on --dev flag)
89+
- `ARMIS_REGION` - Override Armis cloud region (equivalent to `--region`; used for region-aware authentication)
8990
- `ARMIS_FORMAT` - Default output format
9091
- `ARMIS_PAGE_LIMIT` - Results pagination size
9192
- `ARMIS_THEME` - Terminal background theme: auto, dark, light (default: auto)

docs/CI-INTEGRATION.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -697,7 +697,7 @@ Configure `ARMIS_API_TOKEN` and `ARMIS_TENANT_ID` as [secured repository variabl
697697
#### "authentication required"
698698

699699
- No valid authentication credentials were provided
700-
- Set `ARMIS_API_TOKEN` and `ARMIS_TENANT_ID` environment variables or secrets
700+
- Set `ARMIS_CLIENT_ID` and `ARMIS_CLIENT_SECRET` for JWT auth (recommended), or `ARMIS_API_TOKEN` and `ARMIS_TENANT_ID` for legacy auth
701701

702702
#### "tenant ID required"
703703

docs/FEATURES.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,9 +294,10 @@ armis-cli scan repo . \
294294
|----------|-------------|
295295
| `ARMIS_CLIENT_ID` | Client ID for JWT authentication |
296296
| `ARMIS_CLIENT_SECRET` | Client secret for JWT authentication |
297-
| `ARMIS_AUTH_ENDPOINT` | Authentication service endpoint URL |
298297
| `ARMIS_API_TOKEN` | API token for Basic authentication |
299298
| `ARMIS_TENANT_ID` | Tenant identifier (required for Basic auth only) |
299+
| `ARMIS_API_URL` | Override base URL for Armis API and authentication (advanced) |
300+
| `ARMIS_REGION` | Authentication region override (advanced; corresponds to `--region` flag) |
300301

301302
**General:**
302303

internal/api/client.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -216,12 +216,9 @@ func (c *Client) IsDebug() bool {
216216
// request URL uses HTTPS (or localhost for testing). This prevents credential
217217
// exposure over insecure channels.
218218
//
219-
// For JWT auth: sends raw JWT token (no "Bearer" prefix)
219+
// For JWT auth: sends "Bearer <token>" per RFC 6750
220220
// For Basic auth: sends "Basic <token>" per RFC 7617
221221
//
222-
// NOTE: The backend expects raw JWT tokens without the "Bearer" prefix.
223-
// This is unconventional but matches the backend API contract.
224-
//
225222
// SECURITY NOTE: The localhost/127.0.0.1 exception is intentional for local
226223
// development and testing environments where HTTPS certificates are not available.
227224
// Production deployments must always use HTTPS.

internal/auth/auth.go

Lines changed: 97 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ import (
77
"context"
88
"encoding/base64"
99
"encoding/json"
10+
"errors"
1011
"fmt"
12+
"net/http"
1113
"strings"
1214
"sync"
1315
"time"
@@ -18,7 +20,8 @@ type AuthConfig struct {
1820
// JWT auth credentials
1921
ClientID string
2022
ClientSecret string //nolint:gosec // G117: This is a config field name, not a secret value
21-
AuthEndpoint string // Full URL to the authentication service
23+
BaseURL string // Moose API base URL (dev or prod)
24+
Region string // Optional region override - bypasses auto-discovery if set
2225

2326
// Legacy Basic auth
2427
Token string
@@ -33,21 +36,24 @@ type JWTCredentials struct {
3336
Token string
3437
TenantID string // Extracted from customer_id claim
3538
ExpiresAt time.Time
39+
Region string // Deployment region (e.g., "us1", "eu1", "au1")
3640
}
3741

3842
// AuthProvider manages authentication tokens with automatic refresh.
3943
// It supports both JWT authentication and legacy Basic authentication.
4044
// For JWT auth, tokens are automatically refreshed when within 5 minutes of expiry.
4145
type AuthProvider struct {
42-
config AuthConfig
43-
credentials *JWTCredentials
44-
authClient *AuthClient
45-
mu sync.RWMutex
46-
isLegacy bool // true if using Basic auth (--token)
46+
config AuthConfig
47+
credentials *JWTCredentials
48+
authClient *AuthClient
49+
mu sync.RWMutex
50+
isLegacy bool // true if using Basic auth (--token)
51+
cachedRegion string // memoized region from disk cache (loaded once)
52+
regionLoaded bool // true if cachedRegion has been loaded from disk
4753
}
4854

4955
// NewAuthProvider creates an AuthProvider from configuration.
50-
// If ClientID and ClientSecret are set, uses JWT auth with the specified endpoint.
56+
// If ClientID and ClientSecret are set, uses JWT auth with the specified base URL.
5157
// Otherwise falls back to legacy Basic auth with Token.
5258
func NewAuthProvider(config AuthConfig) (*AuthProvider, error) {
5359
p := &AuthProvider{
@@ -64,12 +70,12 @@ func NewAuthProvider(config AuthConfig) (*AuthProvider, error) {
6470

6571
// Determine auth mode: JWT credentials take priority
6672
if config.ClientID != "" && config.ClientSecret != "" {
67-
// JWT auth
73+
// JWT auth via moose
6874
p.isLegacy = false
69-
if config.AuthEndpoint == "" {
70-
return nil, fmt.Errorf("--auth-endpoint is required when using client credentials")
75+
if config.BaseURL == "" {
76+
return nil, fmt.Errorf("base URL is required for JWT authentication")
7177
}
72-
authClient, err := NewAuthClient(config.AuthEndpoint, config.Debug)
78+
authClient, err := NewAuthClient(config.BaseURL, config.Debug)
7379
if err != nil {
7480
return nil, fmt.Errorf("failed to create auth client: %w", err)
7581
}
@@ -86,14 +92,14 @@ func NewAuthProvider(config AuthConfig) (*AuthProvider, error) {
8692
return nil, fmt.Errorf("tenant ID required: use --tenant-id flag or ARMIS_TENANT_ID environment variable")
8793
}
8894
} else {
89-
return nil, fmt.Errorf("authentication required: use --token flag or ARMIS_API_TOKEN environment variable")
95+
return nil, fmt.Errorf("authentication required: set ARMIS_CLIENT_ID and ARMIS_CLIENT_SECRET for JWT auth, or ARMIS_API_TOKEN for legacy auth")
9096
}
9197

9298
return p, nil
9399
}
94100

95101
// GetAuthorizationHeader returns the Authorization header value.
96-
// For JWT auth: the raw JWT token (no "Bearer" prefix - backend expects raw JWT)
102+
// For JWT auth: "Bearer <token>" per RFC 6750
97103
// For Basic auth: "Basic <token>" per RFC 7617
98104
func (p *AuthProvider) GetAuthorizationHeader(ctx context.Context) (string, error) {
99105
if p.isLegacy {
@@ -108,8 +114,8 @@ func (p *AuthProvider) GetAuthorizationHeader(ctx context.Context) (string, erro
108114

109115
p.mu.RLock()
110116
defer p.mu.RUnlock()
111-
// Raw JWT token (no Bearer prefix) - backend expects raw JWT per API contract
112-
return p.credentials.Token, nil
117+
// Bearer token per RFC 6750
118+
return "Bearer " + p.credentials.Token, nil
113119
}
114120

115121
// GetTenantID returns the tenant ID for API requests.
@@ -129,6 +135,23 @@ func (p *AuthProvider) GetTenantID(ctx context.Context) (string, error) {
129135
return p.credentials.TenantID, nil
130136
}
131137

138+
// GetRegion returns the deployment region from the JWT token.
139+
// For JWT auth: extracted from region claim (may be empty for older tokens)
140+
// For Basic auth: returns empty string (no region available)
141+
func (p *AuthProvider) GetRegion(ctx context.Context) (string, error) {
142+
if p.isLegacy {
143+
return "", nil // Legacy auth doesn't have region
144+
}
145+
146+
if err := p.refreshIfNeeded(ctx); err != nil {
147+
return "", fmt.Errorf("failed to refresh token: %w", err)
148+
}
149+
150+
p.mu.RLock()
151+
defer p.mu.RUnlock()
152+
return p.credentials.Region, nil
153+
}
154+
132155
// IsLegacy returns true if using legacy Basic auth.
133156
func (p *AuthProvider) IsLegacy() bool {
134157
return p.isLegacy
@@ -160,6 +183,16 @@ func (p *AuthProvider) GetRawToken(ctx context.Context) (string, error) {
160183

161184
// exchangeCredentials exchanges client credentials for a JWT token.
162185
// Uses double-checked locking to prevent thundering herd of concurrent refreshes.
186+
// Leverages region caching to avoid auto-discovery overhead on subsequent requests.
187+
//
188+
// Region selection priority:
189+
// 1. --region flag (config.Region) - explicit override, bypasses cache and discovery
190+
// 2. Cached region - from previous successful auth for this client_id
191+
// 3. Auto-discovery - server tries regions until one succeeds
192+
//
193+
// Retry behavior: If auth fails with a cached region hint (not explicit --region),
194+
// the cache is cleared and auth is retried without the hint. This handles stale
195+
// cache gracefully without requiring user to re-run the command.
163196
func (p *AuthProvider) exchangeCredentials(ctx context.Context) error {
164197
p.mu.Lock()
165198
defer p.mu.Unlock()
@@ -169,21 +202,62 @@ func (p *AuthProvider) exchangeCredentials(ctx context.Context) error {
169202
return nil
170203
}
171204

172-
token, err := p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret)
205+
// Load cached region once per process (memoize to avoid repeated disk I/O)
206+
if !p.regionLoaded {
207+
if region, ok := loadCachedRegion(p.config.ClientID); ok {
208+
p.cachedRegion = region
209+
}
210+
p.regionLoaded = true
211+
}
212+
213+
// Determine region hint - explicit flag takes priority over cache
214+
var regionHint *string
215+
var usingCachedHint bool
216+
if p.config.Region != "" {
217+
// Explicit --region flag - don't retry on failure (user error)
218+
regionHint = &p.config.Region
219+
} else if p.cachedRegion != "" {
220+
// Cached region - will retry without hint on failure
221+
regionHint = &p.cachedRegion
222+
usingCachedHint = true
223+
}
224+
225+
result, err := p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret, regionHint)
173226
if err != nil {
174-
return err
227+
// If auth failed with a cached region hint, retry only for region-specific rejections.
228+
// Skip retry for: transport errors (not *AuthError), 401 (bad credentials).
229+
// This avoids double requests on network failures and prevents wiping correct cache entries.
230+
var authErr *AuthError
231+
if usingCachedHint && errors.As(err, &authErr) && authErr.StatusCode != http.StatusUnauthorized {
232+
clearCachedRegion()
233+
p.cachedRegion = ""
234+
// Retry without region hint - let server auto-discover
235+
result, err = p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret, nil)
236+
if err != nil {
237+
return err
238+
}
239+
} else {
240+
return err
241+
}
242+
}
243+
244+
// Cache the discovered region for future requests (skip if unchanged)
245+
if result.Region != "" && result.Region != p.cachedRegion {
246+
saveCachedRegion(p.config.ClientID, result.Region)
247+
p.cachedRegion = result.Region
175248
}
176249

177250
// Parse JWT to extract claims
178-
claims, err := parseJWTClaims(token)
251+
claims, err := parseJWTClaims(result.Token)
179252
if err != nil {
180253
return fmt.Errorf("failed to parse JWT: %w", err)
181254
}
182255

183256
p.credentials = &JWTCredentials{
184-
Token: token,
257+
Token: result.Token,
185258
TenantID: claims.CustomerID,
186259
ExpiresAt: claims.ExpiresAt,
260+
Region: claims.Region,
187261
}
188262

189263
return nil
@@ -207,6 +281,7 @@ func (p *AuthProvider) refreshIfNeeded(ctx context.Context) error {
207281
type jwtClaims struct {
208282
CustomerID string // maps to tenant_id
209283
ExpiresAt time.Time
284+
Region string // deployment region (optional)
210285
}
211286

212287
// parseJWTClaims extracts claims from a JWT without signature verification.
@@ -233,7 +308,8 @@ func parseJWTClaims(token string) (*jwtClaims, error) {
233308

234309
var data struct {
235310
CustomerID string `json:"customer_id"`
236-
Exp float64 `json:"exp"` // float64 to handle servers that return fractional timestamps
311+
Exp float64 `json:"exp"` // float64 to handle servers that return fractional timestamps
312+
Region string `json:"region"` // optional deployment region
237313
}
238314
if err := json.Unmarshal(payload, &data); err != nil {
239315
return nil, fmt.Errorf("failed to parse JWT payload: %w", err)
@@ -252,5 +328,6 @@ func parseJWTClaims(token string) (*jwtClaims, error) {
252328
return &jwtClaims{
253329
CustomerID: data.CustomerID,
254330
ExpiresAt: time.Unix(expSec, 0),
331+
Region: data.Region, // may be empty for backward compatibility
255332
}, nil
256333
}

0 commit comments

Comments
 (0)