[PPSC-563] feat(auth): remove auth-endpoint flag#98
[PPSC-563] feat(auth): remove auth-endpoint flag#98yiftach-armis wants to merge 8 commits intomainfrom
Conversation
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.
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 7 |
Total: 7
View all 7 findings
🟠 HIGH (7)
CWE-522_armis-cli_38295677_internal/cmd/auth.go_62_2_62_19 - Insecure Design (CWE-522
Location: internal/cmd/auth.go:62
Insufficiently Protected Credentials): The runAuth function obtains a JWT token using the provided client credentials and then prints the raw token directly to standard output with fmt.Println(token). This output can be captured by other processes, logged, or appear in command histories, exposing the credential. The token originates from the client secret (a sensitive source) and reaches the stdout sink without any sanitization, making the credential reachable. The command-line interface represents script-to-script interaction, giving it an exposure level of 5, which yields a low likelihood score (5). This matches CWE-522: Insufficiently Protected Credentials.
Code snippet is redacted as it contains secrets.
CWEs: CWE-522: Insufficiently Protected Credentials
CWE-522_armis-cli_38295677_internal/cmd/root.go_159_2_159_31 - Insecure Design (CWE-522
Location: internal/cmd/root.go:159
Insufficiently Protected Credentials): The init function registers a --token (or -t) command‑line flag that stores an API token in the variable token. Because the token can be supplied directly on the command line, it becomes part of the process's argument list, which is visible to other users on the same system via typical process‑listing tools. The value from this flag (or its default environment variable) is later passed unchanged to the authentication provider in getAuthProvider, where it is used for API calls. No sanitization or masking occurs before the token is used. Consequently, an attacker who can view process arguments can obtain the credential, satisfying CWE‑522 (Insufficiently Protected Credentials). The vulnerability is exposed through a user‑facing CLI interface, making it immediately reachable to external actors.
Code snippet is redacted as it contains secrets.
CWEs: CWE-522: Insufficiently Protected Credentials
CWE-522_armis-cli_38295677_internal/cmd/root.go_165_2_165_169 - Insecure Design (CWE-522
Location: internal/cmd/root.go:165
Insufficiently Protected Credentials): The init function registers a persistent CLI flag --client-secret that stores the provided value in the clientSecret variable. Because this flag is supplied on the command line, the secret becomes part of the process's argument list, which is visible to any user who can list processes on the host (e.g., via ps). The code does not apply any protection, masking, or sanitization before the value is later passed to the authentication provider. Consequently, an attacker with local system access can read the command‑line arguments and obtain the client secret, enabling unauthorized use of the JWT authentication flow. This matches CWE‑522 (Insufficiently Protected Credentials) and is exploitable through ordinary process‑listing techniques. The exposure level is classified as 5 (script‑to‑script interaction) because the credential can be harvested by other processes on the same machine.
Code snippet is redacted as it contains secrets.
CWEs: CWE-522: Insufficiently Protected Credentials
CWE-319_armis-cli_38295677_internal/auth/client.go_101_2_101_124 - Cryptography Failures (CWE-319
Location: internal/auth/client.go:101
Cleartext Transmission of Sensitive Information): The Authenticate method builds a JSON body that includes the clientSecret argument and sends it with an HTTP POST request using c.httpClient.Do(req). Because the client permits the use of the HTTP scheme for localhost URLs, the request may travel over an unencrypted connection. Consequently, the secret credential is transmitted in cleartext to the authentication endpoint, exposing it to any local network observer. No sanitization or encryption is applied to the secret before transmission, making the flow from source (function argument) to sink (network request) reachable. This constitutes a cleartext transmission of sensitive information, matching CWE‑319.
Code snippet is redacted as it contains secrets.
CWEs: CWE-319: Cleartext Transmission of Sensitive Information
CWE-532_armis-cli_38295677_internal/auth/client.go_120_4_120_105 - Security Logging and Monitoring Failures (CWE-532
Location: internal/auth/client.go:120
Information Exposure Through Log Files): The Authenticate method reads the HTTP response body from an external authentication service. When the request fails (non‑200 status) and the client is configured with debug mode enabled, the code writes the full response body to standard error via fmt.Fprintf(os.Stderr, ...). Because the response body originates from an external source, it can contain sensitive information such as error details, internal identifiers, or even tokens. Writing this data to stderr may cause it to be captured in application logs, which could be accessed by other processes or personnel, leading to unintended information disclosure. No sanitization is performed before logging, so the tainted data reaches the log sink directly. This matches CWE‑532: Information Exposure Through Log Files, making the finding a true positive.
CWEs: CWE-532: Information Exposure Through Log Files
CWE-522_armis-cli_38295677_internal/auth/auth.go_179_2_179_32 - Insecure Design (CWE-522
Location: internal/auth/auth.go:179
Insufficiently Protected Credentials): The GetRawToken method (lines 167‑180) returns the JWT or legacy token exactly as stored in p.credentials.Token (or p.config.Token for legacy mode). No masking, encryption, or additional protection is applied before the token is handed to the caller. Because this method is used by the CLI to print the token to stdout, an attacker who can invoke the CLI (or any code that calls this method) can obtain the raw authentication credential and reuse it to impersonate the user against the API. The token originates from the authentication exchange and is propagated unchanged to the return value, with no sanitizers present. This constitutes an instance of CWE‑522: Insufficiently Protected Credentials. The functionality is reachable from a command‑line interface, which is classified as script‑to‑script interaction, giving it an exposure rating of 5 and a corresponding low likelihood score.
Code snippet is redacted as it contains secrets.
CWEs: CWE-522: Insufficiently Protected Credentials
CWE-522_armis-cli_38295677_internal/auth/auth.go_169_3_169_28 - Insecure Design (CWE-522
Location: internal/auth/auth.go:169
Insufficiently Protected Credentials): The GetRawToken method returns the authentication token in plain form, without any protection or masking. This directly exposes credentials to any caller that can invoke the method, which matches CWE‑522 (Insufficiently Protected Credentials). The function is part of a CLI tool that can be triggered by a user, making the token reachable through a user‑facing interface. No sanitization is applied before the token is returned, so the credential flow is reachable. Consequently, the vulnerability is a true positive with high severity and a high likelihood of exploitation due to the immediate external exposure.
Code snippet is redacted as it contains secrets.
CWEs: CWE-522: Insufficiently Protected Credentials
Test Coverage Reporttotal: (statements) 80.7% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR updates JWT authentication to use the configured API base URL (Moose) instead of a separate --auth-endpoint, switches the token exchange route to /api/v1/auth/token, and extends JWT parsing to extract an optional region claim.
Changes:
- Removed the
--auth-endpoint/ARMIS_AUTH_ENDPOINTconfiguration path and wired JWT auth to use the API base URL (getAPIBaseURL()/ARMIS_API_URLoverride). - Updated the auth client to call
POST /api/v1/auth/tokenand adjusted command/docs/tests accordingly. - Added
regionclaim parsing +GetRegion()API on the auth provider, with tests for backward compatibility.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/root.go | Removes authEndpoint flag/var and passes API base URL into auth provider config. |
| internal/cmd/root_test.go | Updates root auth-provider tests to reflect removal of authEndpoint. |
| internal/cmd/auth.go | Removes --auth-endpoint requirement for the (hidden) auth command. |
| internal/cmd/auth_test.go | Updates auth command tests to use ARMIS_API_URL and new route. |
| internal/auth/client.go | Renames endpoint→baseURL and switches token exchange to /api/v1/auth/token. |
| internal/auth/auth.go | Renames config field to BaseURL and adds region extraction + accessor. |
| internal/auth/auth_test.go | Updates tests for BaseURL + new endpoint; adds coverage for region parsing/accessor. |
| docs/FEATURES.md | Removes ARMIS_AUTH_ENDPOINT from documented env vars. |
| CLAUDE.md | Updates internal docs to reflect the new token endpoint and removes ARMIS_AUTH_ENDPOINT. |
Comments suppressed due to low confidence (1)
CLAUDE.md:90
- This environment variable list no longer mentions
ARMIS_AUTH_ENDPOINT, but it also doesn't mentionARMIS_API_URL, which the CLI uses as an override for the Moose/API base URL (and is now also used for JWT token exchange). Please addARMIS_API_URLhere so internal docs match the current configuration knobs.
### Environment Variables
- `ARMIS_CLIENT_ID` - Client ID for JWT authentication (recommended)
- `ARMIS_CLIENT_SECRET` - Client secret for JWT authentication
- `ARMIS_API_TOKEN` - API token for Basic authentication (fallback)
- `ARMIS_TENANT_ID` - Tenant identifier (required only with Basic auth; JWT extracts it from token)
- `ARMIS_FORMAT` - Default output format
- `ARMIS_PAGE_LIMIT` - Results pagination size
- `ARMIS_THEME` - Terminal background theme: auto, dark, light (default: auto)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if origAPIURL == "" { | ||
| _ = os.Unsetenv("ARMIS_API_URL") | ||
| } else { | ||
| _ = os.Setenv("ARMIS_API_URL", origAPIURL) | ||
| } |
There was a problem hiding this comment.
In this test cleanup, os.Unsetenv/os.Setenv errors are ignored implicitly. With errcheck enabled (per repo lint config), bare calls like this are typically flagged. Please assign the returned error to _ (or handle it) to match patterns used elsewhere in internal/cmd tests and keep lint clean.
| authEndpoint = server.URL | ||
| } else { | ||
| authEndpoint = "" | ||
| _ = os.Setenv("ARMIS_API_URL", server.URL) |
There was a problem hiding this comment.
os.Setenv returns an error; calling it without checking or assigning to _ is likely to be flagged by errcheck during CI. Please use _ = os.Setenv(...) (or assert no error) here.
| if config.ClientID != "" && config.ClientSecret != "" { | ||
| // JWT auth | ||
| // JWT auth via moose | ||
| p.isLegacy = false | ||
| if config.AuthEndpoint == "" { | ||
| return nil, fmt.Errorf("--auth-endpoint is required when using client credentials") | ||
| if config.BaseURL == "" { | ||
| return nil, fmt.Errorf("base URL is required for JWT authentication") | ||
| } |
There was a problem hiding this comment.
The comments around NewAuthProvider/AuthConfig still refer to using a specific "endpoint" for JWT auth, but the config field has been renamed to BaseURL and is now described as the Moose API base URL. Please update the nearby doc/comment text to consistently use “base URL” (or clarify the distinction if there still is one) so the public contract of this package remains accurate.
| | `ARMIS_CLIENT_SECRET` | Client secret for JWT authentication | | ||
| | `ARMIS_AUTH_ENDPOINT` | Authentication service endpoint URL | | ||
| | `ARMIS_API_TOKEN` | API token for Basic authentication | | ||
| | `ARMIS_TENANT_ID` | Tenant identifier (required for Basic auth only) | |
There was a problem hiding this comment.
This env var table documents auth-related configuration, but it omits ARMIS_API_URL, which the CLI uses to override the API (and now JWT auth) base URL. Since ARMIS_AUTH_ENDPOINT is being removed here, consider adding ARMIS_API_URL to keep the docs aligned with the actual configuration surface.
| | `ARMIS_TENANT_ID` | Tenant identifier (required for Basic auth only) | | |
| | `ARMIS_TENANT_ID` | Tenant identifier (required for Basic auth only) | | |
| | `ARMIS_API_URL` | Override base URL for Armis API and authentication (advanced) | |
- Update NewAuthProvider comment to say "base URL" instead of "endpoint" - Document ARMIS_API_URL in FEATURES.md and CLAUDE.md env var sections
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetCacheFilePath returns the validated path to a cache file. | ||
| // Returns empty string if the path cannot be determined or validated. | ||
| func GetCacheFilePath(filename string) string { | ||
| cacheDir := GetCacheDir() | ||
| if cacheDir == "" { | ||
| return "" | ||
| } | ||
|
|
||
| filePath := filepath.Join(cacheDir, filename) | ||
|
|
||
| // Re-validate the full path (filename could contain traversal attempts) | ||
| sanitized, err := SanitizePath(filePath) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
|
|
||
| return sanitized |
There was a problem hiding this comment.
GetCacheFilePath does not actually prevent escaping the cache directory when filename is an absolute path (e.g. "/etc/passwd" or "C:\Windows\..."). filepath.Join(cacheDir, filename) will drop cacheDir in that case, and SanitizePath only rejects ".." components (it does not reject absolute paths or enforce containment). This can lead callers to read/write arbitrary files if filename is ever influenced by input. Suggestion: explicitly reject absolute paths and path separators in filename, or enforce containment after joining (e.g., via filepath.Abs + filepath.Rel check).
| func TestGetCacheFilePath_SafeFilenames(t *testing.T) { | ||
| // This function is designed to be called with safe, constant filenames. | ||
| // The security boundary is at the caller level - callers should only pass | ||
| // known-safe filenames like "region-cache.json" or "update-check.json". | ||
| // | ||
| // Note: filepath.Join handles absolute paths by stripping the leading slash, | ||
| // so even malicious filenames like "/etc/passwd" become "cache-dir/etc/passwd" | ||
| // which stays within the cache directory. | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| filename string | ||
| wantContains string | ||
| }{ | ||
| {"simple json file", "test.json", "test.json"}, | ||
| {"hyphenated name", "region-cache.json", "region-cache.json"}, | ||
| {"absolute path becomes relative", "/etc/passwd", "etc/passwd"}, // stripped of leading / | ||
| } |
There was a problem hiding this comment.
The test and comment assume filepath.Join(cacheDir, "/etc/passwd") produces "cacheDir/etc/passwd" by stripping the leading slash, but Go's filepath.Join discards earlier elements when a later element is absolute (so it becomes "/etc/passwd"). As written, this test is incorrect and will fail on Unix-like systems (and is also platform-specific due to path separators). If GetCacheFilePath is meant to be safe, the expected behavior should likely be to reject absolute paths (return empty/error) rather than accept them.
| | `ARMIS_AUTH_ENDPOINT` | Authentication service endpoint URL | | ||
| | `ARMIS_API_TOKEN` | API token for Basic authentication | | ||
| | `ARMIS_TENANT_ID` | Tenant identifier (required for Basic auth only) | | ||
| | `ARMIS_API_URL` | Override base URL for Armis API and authentication (advanced) | |
There was a problem hiding this comment.
The environment variable list was updated to add ARMIS_API_URL, but the new --region flag / ARMIS_REGION env var (added in root command) is not documented here. Please add an entry describing ARMIS_REGION (authentication region override) so users can discover it.
| | `ARMIS_API_URL` | Override base URL for Armis API and authentication (advanced) | | |
| | `ARMIS_API_URL` | Override base URL for Armis API and authentication (advanced) | | |
| | `ARMIS_REGION` | Authentication region override (advanced; corresponds to `--region` flag) | |
| - `ARMIS_AUTH_ENDPOINT` - JWT authentication service endpoint URL | ||
| - `ARMIS_API_TOKEN` - API token for Basic authentication (fallback) | ||
| - `ARMIS_TENANT_ID` - Tenant identifier (required only with Basic auth; JWT extracts it from token) | ||
| - `ARMIS_API_URL` - Override base URL for Armis API (advanced; defaults based on --dev flag) |
There was a problem hiding this comment.
The env var list removes ARMIS_AUTH_ENDPOINT and adds ARMIS_API_URL, but it does not mention the newly introduced region override (--region / ARMIS_REGION). Please document ARMIS_REGION here as well to keep the CLI configuration summary accurate.
| - `ARMIS_API_URL` - Override base URL for Armis API (advanced; defaults based on --dev flag) | |
| - `ARMIS_API_URL` - Override base URL for Armis API (advanced; defaults based on --dev flag) | |
| - `ARMIS_REGION` - Override Armis cloud region (equivalent to `--region`; affects default API URL selection) |
internal/auth/region_cache_test.go
Outdated
| // This test uses the real cache directory, so we just verify the functions | ||
| // don't panic and maintain the basic contract. | ||
|
|
||
| // Clear any existing cache | ||
| clearCachedRegion() | ||
|
|
||
| // Load from empty should return false | ||
| region, ok := loadCachedRegion("test-client-pkg-level") | ||
| if ok { | ||
| // Cache might have leftover data from other tests; just verify contract |
There was a problem hiding this comment.
TestPackageLevelFunctions calls clearCachedRegion() which removes the real on-disk cache file in the user's cache directory. Unit tests should not modify user state on the machine running tests (including CI dev machines). Suggestion: temporarily replace defaultCache with a RegionCache{cacheDir: t.TempDir()} inside the test (save/restore in t.Cleanup) so package-level helpers operate on a temp location.
| // This test uses the real cache directory, so we just verify the functions | |
| // don't panic and maintain the basic contract. | |
| // Clear any existing cache | |
| clearCachedRegion() | |
| // Load from empty should return false | |
| region, ok := loadCachedRegion("test-client-pkg-level") | |
| if ok { | |
| // Cache might have leftover data from other tests; just verify contract | |
| // Use a temporary cache directory for this test so we don't modify the user's real cache. | |
| origCache := defaultCache | |
| defaultCache = &RegionCache{cacheDir: t.TempDir()} | |
| t.Cleanup(func() { | |
| defaultCache = origCache | |
| }) | |
| // Clear any existing cache in the temporary directory | |
| clearCachedRegion() | |
| // Load from empty should return false | |
| region, ok := loadCachedRegion("test-client-pkg-level") | |
| if ok { | |
| // Cache might have leftover data from other tests in this temp dir; just verify contract |
| func TestRegionCache_FilePermissions(t *testing.T) { | ||
| tempDir := t.TempDir() | ||
| cache := &RegionCache{cacheDir: tempDir} | ||
|
|
||
| cache.Save("client-123", testRegionUS1) | ||
|
|
||
| // Verify file permissions are restrictive (0600) | ||
| cachePath := filepath.Join(tempDir, regionCacheFileName) | ||
| info, err := os.Stat(cachePath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to stat cache file: %v", err) | ||
| } | ||
|
|
||
| perm := info.Mode().Perm() | ||
| if perm != 0o600 { | ||
| t.Errorf("Expected file permissions 0600, got %o", perm) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestRegionCache_FilePermissions asserts the cache file has mode 0600. This is not reliable on Windows (and sometimes on other platforms/filesystems) where Chmod/permission bits are not enforced the same way, and os.WriteFile's perm argument may be ignored. Since CI runs windows-latest, this test is likely to fail there. Suggestion: skip this assertion on Windows (runtime.GOOS == "windows"), or relax the check to only validate permissions on Unix-like systems.
- Skip file permissions test on Windows (Unix permissions not supported) - Use filepath.Join for platform-agnostic path separators in test assertions
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func GetCacheFilePath(filename string) string { | ||
| cacheDir := GetCacheDir() | ||
| if cacheDir == "" { | ||
| return "" | ||
| } | ||
|
|
||
| // Reject absolute paths - filepath.Join would discard cacheDir (CWE-22) | ||
| if filepath.IsAbs(filename) { | ||
| return "" | ||
| } | ||
|
|
||
| // Reject path separators - filename should be a simple name like "cache.json" | ||
| if strings.ContainsAny(filename, `/\`) { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
GetCacheFilePath currently accepts an empty filename (or ".") and will end up returning the cache directory path itself, not a file path. That can cause callers to attempt to read/write a directory as if it were a file. Consider explicitly rejecting empty/"."/".." filenames (and ideally trimming whitespace) before joining.
internal/util/cache.go
Outdated
| // Final containment check: ensure result is within cache directory | ||
| if !strings.HasPrefix(sanitized, cacheDir) { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
The final containment check uses strings.HasPrefix(sanitized, cacheDir), which is not a robust path containment check (e.g., it’s sensitive to missing path separators and can be incorrect on platforms with case-insensitive paths). If you want a containment guarantee, prefer filepath.Rel-based containment (as in util.SafeJoinPath) or ensure the prefix match includes a path separator boundary; otherwise consider removing this redundant check since filename separators are already rejected.
| // Final containment check: ensure result is within cache directory | |
| if !strings.HasPrefix(sanitized, cacheDir) { | |
| return "" | |
| } | |
| // Final containment check: ensure result is within cache directory using a robust path-based check | |
| rel, err := filepath.Rel(cacheDir, sanitized) | |
| if err != nil { | |
| return "" | |
| } | |
| if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { | |
| return "" | |
| } |
| // Load cached region once per process (memoize to avoid repeated disk I/O) | ||
| if !p.regionLoaded { | ||
| if region, ok := loadCachedRegion(p.config.ClientID); ok { | ||
| p.cachedRegion = region | ||
| } | ||
| p.regionLoaded = true | ||
| } | ||
|
|
||
| // Determine region hint - explicit flag takes priority over cache | ||
| var regionHint *string | ||
| var usingCachedHint bool | ||
| if p.config.Region != "" { | ||
| // Explicit --region flag - don't retry on failure (user error) | ||
| regionHint = &p.config.Region | ||
| } else if p.cachedRegion != "" { | ||
| // Cached region - will retry without hint on failure | ||
| regionHint = &p.cachedRegion | ||
| usingCachedHint = true | ||
| } | ||
|
|
||
| result, err := p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret, regionHint) | ||
| if err != nil { | ||
| return err | ||
| // If auth failed with a cached region hint, clear cache and retry without hint | ||
| // This handles stale cache (region changed) without requiring user to re-run | ||
| if usingCachedHint { | ||
| clearCachedRegion() | ||
| p.cachedRegion = "" | ||
| // Retry without region hint - let server auto-discover | ||
| result, err = p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The new region-caching/region-hint retry logic (clearing the cache and retrying without the hint on failure) isn’t covered by tests in this package. Adding an auth provider test that simulates a failing request when a cached region is provided and a succeeding request without the hint would help prevent regressions in this behavior.
CLAUDE.md
Outdated
| - `ARMIS_API_TOKEN` - API token for Basic authentication (fallback) | ||
| - `ARMIS_TENANT_ID` - Tenant identifier (required only with Basic auth; JWT extracts it from token) | ||
| - `ARMIS_API_URL` - Override base URL for Armis API (advanced; defaults based on --dev flag) | ||
| - `ARMIS_REGION` - Override Armis cloud region (equivalent to `--region`; affects default API URL selection) |
There was a problem hiding this comment.
This doc line says ARMIS_REGION “affects default API URL selection”, but getAPIBaseURL() currently only looks at ARMIS_API_URL and --dev; the region flag/env is passed to auth but does not influence base URL selection. Please update the wording or implement the described behavior so the docs match reality.
| - `ARMIS_REGION` - Override Armis cloud region (equivalent to `--region`; affects default API URL selection) | |
| - `ARMIS_REGION` - Override Armis cloud region (equivalent to `--region`; used for region-aware authentication, does not change API base URL selection) |
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
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify result is within cache directory | ||
| cacheDir := GetCacheDir() | ||
| if !strings.HasPrefix(path, cacheDir) { |
There was a problem hiding this comment.
The containment assertion here uses strings.HasPrefix(path, cacheDir), which is not a reliable path-boundary check (e.g., "/tmp/armis-cli-2" has prefix "/tmp/armis-cli"; Windows case-insensitivity/cleaning can also make this flaky). Since the production code uses filepath.Rel-based containment, the test should use a similar filepath.Rel check (or at least enforce a path separator boundary).
| // Verify result is within cache directory | |
| cacheDir := GetCacheDir() | |
| if !strings.HasPrefix(path, cacheDir) { | |
| // Verify result is within cache directory using a path-safe check | |
| cacheDir := GetCacheDir() | |
| rel, err := filepath.Rel(cacheDir, path) | |
| if err != nil { | |
| t.Fatalf("filepath.Rel(%q, %q) error: %v", cacheDir, path, err) | |
| } | |
| if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { |
| data, err := os.ReadFile(path) //nolint:gosec // path validated by getFilePath | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
|
|
||
| var cache regionCacheEntry | ||
| if err := json.Unmarshal(data, &cache); err != nil { | ||
| return "", false | ||
| } |
There was a problem hiding this comment.
RegionCache.Load reads the entire cache file with os.ReadFile without any size limit. A corrupted or maliciously large cache file under the user cache dir could cause unnecessary memory usage/DoS. Consider bounding reads (e.g., check os.Stat size against a small max like 64KB, or read via io.LimitReader) before json.Unmarshal.
| result, err := p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret, regionHint) | ||
| if err != nil { | ||
| return err | ||
| // If auth failed with a cached region hint, clear cache and retry without hint | ||
| // This handles stale cache (region changed) without requiring user to re-run | ||
| if usingCachedHint { | ||
| clearCachedRegion() | ||
| p.cachedRegion = "" | ||
| // Retry without region hint - let server auto-discover | ||
| result, err = p.authClient.Authenticate(ctx, p.config.ClientID, p.config.ClientSecret, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When using a cached region hint, any Authenticate() error triggers cache clear + an immediate retry without the hint. This means transient failures (network/5xx), or genuinely invalid credentials, will always cause a second request and will also wipe a potentially-correct cache entry. Consider only clearing/retrying on a specific “region hint rejected” condition (e.g., a typed error that carries HTTP status like 401/403), and avoid retrying on transport errors.
| // JWT authentication | ||
| rootCmd.PersistentFlags().StringVar(&clientID, "client-id", os.Getenv("ARMIS_CLIENT_ID"), "Client ID for JWT authentication (env: ARMIS_CLIENT_ID)") | ||
| rootCmd.PersistentFlags().StringVar(&clientSecret, "client-secret", os.Getenv("ARMIS_CLIENT_SECRET"), "Client secret for JWT authentication (env: ARMIS_CLIENT_SECRET)") | ||
| rootCmd.PersistentFlags().StringVar(&authEndpoint, "auth-endpoint", os.Getenv("ARMIS_AUTH_ENDPOINT"), "Authentication service endpoint URL (env: ARMIS_AUTH_ENDPOINT)") | ||
| rootCmd.PersistentFlags().StringVar(®ion, "region", os.Getenv("ARMIS_REGION"), "Override region for authentication (bypasses auto-discovery) (env: ARMIS_REGION)") | ||
|
|
There was a problem hiding this comment.
This change removes the previously-supported --auth-endpoint / ARMIS_AUTH_ENDPOINT configuration surface in favor of implicit base URL selection (and ARMIS_API_URL override). If users/scripts depended on the old flag/env var, this is a breaking CLI change. Consider keeping --auth-endpoint as a deprecated alias (mapping to BaseURL / ARMIS_API_URL) for at least one release, or emitting a clear migration error when ARMIS_AUTH_ENDPOINT is set.
Related Issue
Type of Change
Problem
The
--auth-endpointflag required users to manually specify the JWT authentication service endpoint, creating unnecessary configuration burden and potential attack surface. Auth credentials now route through moose's unified proxy endpoint automatically.Solution
AuthEndpointconfig withBaseURLderived from--devflag or production defaults/api/v1/auth/token(moose) instead of/api/v1/authenticate(Silk)--auth-endpointflag andARMIS_AUTH_ENDPOINTenvironment variableendpoint→baseURLinNewAuthClient)Testing
Reviewer Notes
This change fixes findings from deep-review analysis:
NewAuthClientnaming to match semantic change from endpoint to base URLIncludes region support groundwork for PPSC-543.