PPSC-1037: Add auth login/logout/whoami with OAuth2 device flow#257
Open
dmitrysmirnov-armis wants to merge 5 commits into
Open
Conversation
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
| 🟡 MEDIUM | 1 |
Total: 2
View all 2 findings
🟠 HIGH (1)
CWE-327 - Cryptography Failures (CWE-327
Location: internal/auth/device.go:258
Use of Broken or Risky Cryptographic Algorithm): The program talks to the authentication server and receives a JWT access token. It then calls parseAccessTokenClaims to read the token’s contents, but it never checks the token’s digital signature. Because the signature isn’t verified, anyone who can supply a forged token could make the client believe the token contains whatever user, role, or tenant information they want. The client would then use those false claims for things like deciding which tenant it belongs to or what permissions it has. Since the token comes from a network request, an attacker who can influence that response could exploit this weakness. Adding proper signature verification (or using a library that does it automatically) would prevent the client from trusting tampered tokens.
Code snippet is redacted as it contains secrets.
CWEs: CWE-327: Use of Broken or Risky Cryptographic Algorithm
🟡 MEDIUM (1)
CWE-253 - In the `runAuthLogout` function the code calls `store.Load(env)` and assigns the result to `existing`, but it completely ignores the second return value (the error)
Location: internal/cmd/auth_logout.go:53
In the runAuthLogout function the code calls store.Load(env) and assigns the result to existing, but it completely ignores the second return value (the error). If store.Load fails—for example because the token file is missing, corrupted, or the program lacks permission—the error is discarded and the program proceeds as if nothing was stored. This means a problem with loading the credentials can go unnoticed, and the user will see a misleading message that no credentials were removed. Because this command is part of a command‑line tool that any user can run, the mistake can be triggered simply by invoking the logout command. There is no additional check or fallback to handle a load failure, so the vulnerable code may hide real errors and give a false sense of security. To fix the issue, the code should examine the error returned by store.Load and handle it appropriately (e.g., report the failure to the user or abort the operation). This ensures that problems with reading stored credentials are not silently ignored.
return nil
}
fmt.Fprintf(os.Stderr, "%s Signed out of %d environment(s).\n", output.IconSuccess, len(envs))
return nil
}
env := getAPIBaseURL()
// Report whether anything was actually stored, so logout is informative
// rather than silently succeeding when not logged in.
existing, _ := store.Load(env)
if err := store.Clear(env); err != nil {
return fmt.Errorf("failed to remove stored credentials: %w", err)
}
if existing == nil {
fmt.Fprintf(os.Stderr, "No stored credentials to remove for %s.\n", env)
return nil
}
fmt.Fprintf(os.Stderr, "%s Signed out of %s.\n", output.IconSuccess, env)
return nil
CWEs: CWE-253: Incorrect Check of Function Return Value
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Test Coverage Reporttotal: (statements) 73.8% Coverage by function |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
The CLI only supported static credentials (client-credentials JWT and a legacy API token). Developers had no way to authenticate via browser-based SSO against their corporate IdP, and there was no shared session that the CLI and the Armis MCP plugins could both reuse from a single sign-in.
Solution
Implements the
authcommand group built on the OAuth2 Device Authorization Grant (RFC 8628):auth login— requests a device code (POST /oauth2/device,client_iddefaults toarmis-cli), auto-opens the browser atverification_uri_complete(falls back to printing the URL +user_codeover SSH/headless), then pollsPOST /oauth2/tokenhonoringauthorization_pending/slow_down/expired_token/access_denied. On success it stores the token and prints the identity and tenant.auth logout— clears the stored token for the current environment;--allclears every environment.auth whoami— shows environment, auth method, identity, tenant, region, and token expiry.ARMIS_CLIENT_ID/ARMIS_CLIENT_SECRET→ legacyARMIS_API_TOKEN→ an error pointing atauth login. Explicit--client-id/--client-secret/--tokenoverride the stored token (escape hatch). CI/CD env-var auth is unchanged.Token storage is a
0600JSON file at~/.armis/.sessions(a JSON array of{env, token}keyed by the resolved API base URL), giving a single source of truth that the Python MCP plugins can share —go-keyringdoes not interoperate with Python'skeyring, and the backend's refresh-token reuse-detection makes a divergent second store dangerous.Changes layered on top of the original ticket scope:
auth login/logout/whoami). The originalauth(raw JWT from client credentials) is preserved as a hiddenauth tokensubcommand.ARMIS_DEFAULT_AUTH_METHODenv var — set toSSOto auto-trigger the device-flow login on the first command that needs credentials when none are configured (requiresARMIS_TENANT_ID/--tenant-id). Gated so it never shadows a working CI/service-account setup.ARMIS_DEFAULT_AUTH_METHOD.Testing
Automated Tests
New/updated coverage: device-flow client (
device_test.go), token store (tokenstore_test.go), OAuth provider refresh (oauth_provider_test.go), browser opener (browser_test.go), and the end-to-end command flow including the SSO auto-login gating and trigger (auth_flow_test.go).Manual Testing
go build ./...,go test ./internal/cmd/ ./internal/auth/, andgolangci-lint runall pass locally. End-to-end login/whoami/logout exercised against a mock OAuth2 device-flow server inauth_flow_test.go.Reviewer Notes
getAuthProvidernow takes acontext.Contextso the interactive auto-login can run for the command's full lifetime rather than a short request timeout.Checklist