From c94ced75037630c1e0238c9eb56bcae53f4c03f5 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Tue, 18 Aug 2026 14:33:29 -0700 Subject: [PATCH 01/17] feat(auth): implement Device Code login flow and token refresh Implements `lfx auth login`, `lfx auth token`, `lfx auth status`, and `lfx auth logout` on top of golang.org/x/oauth2's device authorization grant support (Config.DeviceAuth/DeviceAccessToken) and refresh-token TokenSource, targeting the static per-environment Auth0 client IDs provisioned in LFXV2-2513. - internal/auth0: new package resolving --env to its IdP domain and compiled-in client ID, and driving the device code request, poll, and refresh-token exchange. - internal/commands/auth.go: real login (interactive device code flow or --with-token for headless use), token (cached-token fast path, refresh-and-recache on expiry, clear error on invalid/expired refresh token), status, and logout implementations. - internal/credstore: extends DeviceState with environment/audience (needed to replay the same IdP/client on refresh) and adds DeleteDeviceState for logout. Deliberately does not persist a device ID: Auth0's device flow has no such concept, and the `gh` CLI precedent cited when this story was written turned out to be an unrelated telemetry identifier, not part of its OAuth flow. - Includes outstanding .cspell.json wordlist additions. LFXV2-2515, LFXV2-2516 Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- .cspell.json | 5 + go.mod | 1 + go.sum | 2 + internal/auth0/device.go | 190 +++++++++++++++++ internal/commands/auth.go | 355 ++++++++++++++++++++++++++++++-- internal/credstore/credstore.go | 42 +++- 6 files changed, 574 insertions(+), 21 deletions(-) create mode 100644 internal/auth0/device.go diff --git a/.cspell.json b/.cspell.json index 4de95cc..6601992 100644 --- a/.cspell.json +++ b/.cspell.json @@ -6,8 +6,10 @@ "artipacked", "binname", "binpath", + "bsdtar", "cimd", "clidocs", + "containedctx", "coverprofile", "cpuprof", "credstore", @@ -28,7 +30,10 @@ "memprof", "mgechev", "mktemp", + "mtimes", + "nolint", "pipefail", + "rundll", "techdocs", "trimpath", "urfave", diff --git a/go.mod b/go.mod index 7199b68..c967d6c 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/99designs/keyring v1.2.2 github.com/urfave/cli-docs/v3 v3.1.0 github.com/urfave/cli/v3 v3.10.1 + golang.org/x/oauth2 v0.36.0 ) require ( diff --git a/go.sum b/go.sum index 77c7dc3..652414f 100644 --- a/go.sum +++ b/go.sum @@ -33,6 +33,8 @@ github.com/urfave/cli-docs/v3 v3.1.0 h1:Sa5xm19IpE5gpm6tZzXdfjdFxn67PnEsE4dpXF7v github.com/urfave/cli-docs/v3 v3.1.0/go.mod h1:59d+5Hz1h6GSGJ10cvcEkbIe3j233t4XDqI72UIx7to= github.com/urfave/cli/v3 v3.10.1 h1:7Kx9H50hrHbRbyxgO1KP6/BcbiGRz0uYh5YyQ30JEEY= github.com/urfave/cli/v3 v3.10.1/go.mod h1:ysVLtOEmg2tOy6PknnYVhDoouyC/6N42TMeoMzskhso= +golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= +golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= diff --git a/internal/auth0/device.go b/internal/auth0/device.go new file mode 100644 index 0000000..4bbe920 --- /dev/null +++ b/internal/auth0/device.go @@ -0,0 +1,190 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +// Package auth0 configures the Auth0 Device Authorization Grant (RFC 8628) +// and refresh-token exchange used by `lfx auth login` and `lfx auth token`, +// on top of golang.org/x/oauth2's device flow support +// (Config.DeviceAuth/DeviceAccessToken and TokenSource-based refresh). +package auth0 + +import ( + "context" + "errors" + "fmt" + "net/http" + + "golang.org/x/oauth2" +) + +// Environment identifies which LFX Auth0 tenant/IdP a login targets. +type Environment string + +// Supported environments, selected via the `--env` flag. Each maps to a +// fixed IdP domain and a static, pre-provisioned Auth0 native application +// client ID (device_code + refresh_token grants; see LFXV2-2513 and +// auth0-terraform PR #348). CIMD was evaluated and abandoned for this flow: +// Auth0 silently drops CIMD client registration for the device_code grant. +const ( + EnvProd Environment = "prod" + EnvStaging Environment = "staging" + EnvDevelopment Environment = "development" +) + +// domains maps each Environment to its Auth0 tenant domain. +var domains = map[Environment]string{ + EnvProd: "linuxfoundation.auth0.com", + EnvStaging: "linuxfoundation-staging.auth0.com", + EnvDevelopment: "linuxfoundation-dev.auth0.com", +} + +// clientIDs maps each Environment to its compiled-in, pre-provisioned +// Auth0 native application client ID for the device code grant. +// cspell:disable -- opaque, randomly-generated Auth0 client IDs, not words. +var clientIDs = map[Environment]string{ + EnvProd: "kkCpM0c9zJ0vNZZDDOGqcyzocOBircOn", + EnvStaging: "9XzXgDfAB9O7IoHqhBj5mg4VLvdBM8ci", + EnvDevelopment: "0TN1OElqQY146vLEPdV5qfejRKpc9IAZ", +} + +// cspell:enable + +// DefaultAudience is the production LFX v2 API audience used unless +// overridden via `--audience`. It intentionally does not vary with `--env`: +// per LFXV2-2515, the audience is independent of the selected environment +// and must be set explicitly when testing a non-prod API. +const DefaultAudience = "https://lfx-api.v2.cluster.lfx.dev/" + +// ErrInvalidEnvironment is returned by Resolve for an unrecognized +// Environment value. +var ErrInvalidEnvironment = errors.New("auth0: invalid environment") + +// Resolve returns the IdP domain and client ID for env. +func Resolve(env Environment) (domain, clientID string, err error) { + domain, ok := domains[env] + if !ok { + return "", "", fmt.Errorf("%w: %q (must be one of prod, staging, development)", ErrInvalidEnvironment, env) + } + return domain, clientIDs[env], nil +} + +// Client drives the Auth0 device code and refresh-token exchanges for a +// single environment, via an underlying oauth2.Config. Auth0's device code +// application is a public native client (Token Endpoint Authentication +// Method: None; is_first_party = true), so ClientSecret is deliberately +// left unset. +type Client struct { + // Domain is the Auth0 tenant domain, e.g. "linuxfoundation.auth0.com". + Domain string + // ClientID is the Auth0 application client ID used for both the + // device code request and subsequent token/refresh exchanges. + ClientID string + // HTTPClient is used for all requests. Defaults to + // http.DefaultClient if nil. + HTTPClient *http.Client +} + +// config builds the oauth2.Config used for a device code flow requesting +// scopes, pointed at this Client's Auth0 tenant. +func (c *Client) config(scopes []string) *oauth2.Config { + return &oauth2.Config{ + ClientID: c.ClientID, + Scopes: scopes, + Endpoint: oauth2.Endpoint{ + DeviceAuthURL: "https://" + c.Domain + "/oauth/device/code", + TokenURL: "https://" + c.Domain + "/oauth/token", + AuthStyle: oauth2.AuthStyleInParams, + }, + } +} + +// context attaches Client's HTTPClient to ctx, per the convention +// documented on oauth2.HTTPClient, so all requests made by the resulting +// oauth2.Config use it. +func (c *Client) context(ctx context.Context) context.Context { + if c.HTTPClient == nil { + return ctx + } + return context.WithValue(ctx, oauth2.HTTPClient, c.HTTPClient) +} + +// DeviceCode wraps the RFC 8628 §3.2 device authorization response, +// carrying the oauth2.Config needed to complete the flow via Poll. +type DeviceCode struct { + *oauth2.DeviceAuthResponse + cfg *oauth2.Config + ctx context.Context //nolint:containedctx // ctx is captured for reuse by Poll, matching Client.RequestDeviceCode/Poll's split across a user-facing pause. +} + +// RequestDeviceCode starts the device authorization flow for the given +// audience and scopes. +func (c *Client) RequestDeviceCode(ctx context.Context, audience string, scopes []string) (*DeviceCode, error) { + cfg := c.config(scopes) + authCtx := c.context(ctx) + + var opts []oauth2.AuthCodeOption + if audience != "" { + opts = append(opts, oauth2.SetAuthURLParam("audience", audience)) + } + + resp, err := cfg.DeviceAuth(authCtx, opts...) + if err != nil { + return nil, fmt.Errorf("auth0: request device code: %w", err) + } + return &DeviceCode{DeviceAuthResponse: resp, cfg: cfg, ctx: authCtx}, nil +} + +// Errors surfaced by Poll when the token endpoint reports the user has +// explicitly declined (RFC 8628 §3.5's "access_denied") or the device code +// expired before the flow completed ("expired_token"). Poll otherwise +// blocks internally on "authorization_pending"/"slow_down" until one of +// these, success, or ctx's deadline (bounded by the device code's own +// expiry). +var ( + // ErrAccessDenied indicates the user explicitly declined the request. + ErrAccessDenied = errors.New("auth0: access denied") + // ErrExpiredToken indicates the device code expired before the user + // completed the flow. + ErrExpiredToken = errors.New("auth0: device code expired") + // ErrInvalidGrant indicates the supplied refresh token is expired, + // revoked, or otherwise no longer valid (Auth0's "invalid_grant" + // error from the refresh_token grant). Callers should treat this as + // requiring the user to run `lfx auth login` again. + ErrInvalidGrant = errors.New("auth0: invalid or expired refresh token") +) + +// Poll blocks until the user completes (or rejects) the device flow, +// polling the token endpoint at the interval Auth0 specified (respecting +// "slow_down" backoff) via oauth2.Config.DeviceAccessToken. It returns +// ErrAccessDenied or ErrExpiredToken for those respective outcomes. +func (dc *DeviceCode) Poll() (*oauth2.Token, error) { + tok, err := dc.cfg.DeviceAccessToken(dc.ctx, dc.DeviceAuthResponse) + if err == nil { + return tok, nil + } + + var retrieveErr *oauth2.RetrieveError + if errors.As(err, &retrieveErr) { + switch retrieveErr.ErrorCode { + case "access_denied": + return nil, ErrAccessDenied + case "expired_token": + return nil, ErrExpiredToken + } + } + return nil, fmt.Errorf("auth0: poll for token: %w", err) +} + +// RefreshToken exchanges a refresh token for a new access token. +func (c *Client) RefreshToken(ctx context.Context, refreshToken string) (*oauth2.Token, error) { + cfg := c.config(nil) + tok, err := cfg.TokenSource(c.context(ctx), &oauth2.Token{RefreshToken: refreshToken}).Token() + if err == nil { + return tok, nil + } + + var retrieveErr *oauth2.RetrieveError + if errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "invalid_grant" { + return nil, ErrInvalidGrant + } + return nil, fmt.Errorf("auth0: refresh token: %w", err) +} diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 407629c..def32a9 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -5,9 +5,21 @@ package commands import ( + "bufio" "context" + "encoding/base64" + "encoding/json" + "errors" "fmt" + "io" + "os" + "os/exec" + "runtime" + "strings" + "time" + "github.com/linuxfoundation/lfx-cli/internal/auth0" + "github.com/linuxfoundation/lfx-cli/internal/credstore" "github.com/urfave/cli/v3" ) @@ -16,17 +28,25 @@ import ( // unencrypted file. const insecureStorageFlagName = "insecure-storage" +// Flag names shared by the login command. +const ( + webFlagName = "web" + withTokenFlagName = "with-token" + envFlagName = "env" + audienceFlagName = "audience" +) + +// scopes requested during the device code flow. offline_access is required +// to receive a refresh token; the rest identify the user for `auth status`. +var loginScopes = []string{"openid", "profile", "email", "offline_access"} + // NewAuthCommand builds the `lfx auth` command group with its subcommands. // -// The --insecure-storage flag is shared by all subcommands and controls -// whether credentials bypass the system keychain in favor of credstore's -// plain (unencrypted) file fallback, e.g. for headless/CI use. -// -// The login, token, status, and logout actions are currently stubs; real -// implementations land in LFXV2-2515 (login flow) and LFXV2-2516 (token -// command), at which point they'll build a credstore.Store via -// credstore.New(credstore.Options{Insecure: -// cmd.Bool(insecureStorageFlagName)}). +// The --insecure-storage flag is shared by all subcommands (it is not +// declared as a "Local" flag, so urfave/cli resolves it for subcommand +// actions via cmd.Bool) and controls whether credentials bypass the system +// keychain in favor of credstore's plain (unencrypted) file fallback, e.g. +// for headless/CI use. func NewAuthCommand() *cli.Command { return &cli.Command{ Name: "auth", @@ -46,23 +66,222 @@ func NewAuthCommand() *cli.Command { } } +// credStoreFromCommand builds a credstore.Store using the group-level +// --insecure-storage flag, however deep in the `auth` subcommand tree cmd +// is. +func credStoreFromCommand(cmd *cli.Command) (credstore.Store, error) { + return credstore.New(credstore.Options{Insecure: cmd.Bool(insecureStorageFlagName)}) +} + func newAuthLoginCommand() *cli.Command { return &cli.Command{ Name: "login", Usage: "Log in to the LFX platform via the Auth0 Device Code flow", - Action: func(_ context.Context, _ *cli.Command) error { - fmt.Println("lfx auth login: not yet implemented (see LFXV2-2515)") - return nil + Flags: []cli.Flag{ + &cli.BoolFlag{ + Name: webFlagName, + Aliases: []string{"w"}, + Usage: "Automatically open the verification URL in the default browser", + }, + &cli.BoolFlag{ + Name: withTokenFlagName, + Usage: "Read a refresh token from stdin instead of performing the interactive Device Code flow", + }, + &cli.StringFlag{ + Name: envFlagName, + Usage: "Target environment: prod, staging, or development", + Value: string(auth0.EnvProd), + }, + &cli.StringFlag{ + Name: audienceFlagName, + Usage: "Auth0 API audience to request tokens for (independent of --env)", + Value: auth0.DefaultAudience, + }, }, + Action: runAuthLogin, + } +} + +func runAuthLogin(ctx context.Context, cmd *cli.Command) error { + store, err := credStoreFromCommand(cmd) + if err != nil { + return err + } + + env := auth0.Environment(cmd.String(envFlagName)) + domain, clientID, err := auth0.Resolve(env) + if err != nil { + return err + } + audience := cmd.String(audienceFlagName) + + if cmd.Bool(withTokenFlagName) { + return loginWithToken(store, env, domain, audience) + } + + client := &auth0.Client{Domain: domain, ClientID: clientID} + return loginWithDeviceCode(ctx, cmd, client, store, env, domain, audience) +} + +// loginWithToken implements `--with-token`: it reads a refresh token from +// stdin (one line, trimmed) for headless/CI use, e.g. +// `echo "$REFRESH_TOKEN" | lfx auth login --with-token`. No access token is +// cached; the next `lfx auth token` call exchanges the refresh token for +// one. +func loginWithToken(store credstore.Store, env auth0.Environment, domain, audience string) error { + reader := bufio.NewReader(os.Stdin) + line, err := reader.ReadString('\n') + if err != nil && !errors.Is(err, io.EOF) { + return fmt.Errorf("read refresh token from stdin: %w", err) + } + refreshToken := strings.TrimSpace(line) + if refreshToken == "" { + return errors.New("no refresh token provided on stdin") + } + + if err := store.SaveCredentials(credstore.Credentials{RefreshToken: refreshToken}); err != nil { + return fmt.Errorf("save credentials: %w", err) + } + if err := store.SaveDeviceState(credstore.DeviceState{ + IDPDomain: domain, + Environment: string(env), + Audience: audience, + }); err != nil { + return fmt.Errorf("save device state: %w", err) + } + + fmt.Println("Logged in with a supplied refresh token.") + return nil +} + +// loginWithDeviceCode performs the interactive Auth0 Device Code flow: +// request a device code, show the user code and verification URL +// (optionally opening it in a browser), then poll until the user completes +// or the flow expires/is denied. +func loginWithDeviceCode( + ctx context.Context, + cmd *cli.Command, + client *auth0.Client, + store credstore.Store, + env auth0.Environment, + domain, audience string, +) error { + dc, err := client.RequestDeviceCode(ctx, audience, loginScopes) + if err != nil { + return err + } + + fmt.Printf("First copy your one-time code: %s\n", dc.UserCode) + if cmd.Bool(webFlagName) { + fmt.Printf("Opening %s in your browser...\n", dc.VerificationURIComplete) + if err := openBrowser(dc.VerificationURIComplete); err != nil { + fmt.Printf("Couldn't open browser automatically: %v\n", err) + fmt.Printf("Please visit: %s\n", dc.VerificationURIComplete) + } + } else { + fmt.Printf("Then visit: %s\n", dc.VerificationURIComplete) + } + fmt.Println("Waiting for authentication...") + + // Poll blocks internally until success or failure, honoring the + // device code's own expiry and Auth0's requested polling interval + // (including "slow_down" backoff). + token, err := dc.Poll() + switch { + case err == nil: + case errors.Is(err, auth0.ErrAccessDenied): + return errors.New("login was denied") + case errors.Is(err, auth0.ErrExpiredToken): + return errors.New("device code expired before login completed") + default: + return err + } + + if err := store.SaveCredentials(credstore.Credentials{ + RefreshToken: token.RefreshToken, + AccessToken: token.AccessToken, + AccessTokenExpiry: token.Expiry, + }); err != nil { + return fmt.Errorf("save credentials: %w", err) } + if err := store.SaveDeviceState(credstore.DeviceState{ + IDPDomain: domain, + Environment: string(env), + Audience: audience, + }); err != nil { + return fmt.Errorf("save device state: %w", err) + } + + fmt.Println("Login successful.") + if idToken, ok := token.Extra("id_token").(string); ok { + if identity := identityFromIDToken(idToken); identity != "" { + fmt.Printf("Logged in as %s.\n", identity) + } + } + return nil } func newAuthTokenCommand() *cli.Command { return &cli.Command{ Name: "token", Usage: "Print a valid access token for the LFX platform", - Action: func(_ context.Context, _ *cli.Command) error { - fmt.Println("lfx auth token: not yet implemented (see LFXV2-2516)") + Action: func(ctx context.Context, cmd *cli.Command) error { + store, err := credStoreFromCommand(cmd) + if err != nil { + return err + } + + creds, err := store.LoadCredentials() + if errors.Is(err, credstore.ErrNotFound) { + return errors.New("not logged in; run `lfx auth login` first") + } + if err != nil { + return err + } + + if creds.ValidAccessToken() { + fmt.Println(creds.AccessToken) + return nil + } + + if creds.RefreshToken == "" { + return errors.New("no refresh token available; run `lfx auth login` again") + } + + state, err := store.LoadDeviceState() + if err != nil { + return fmt.Errorf("load device state: %w", err) + } + + _, clientID, err := auth0.Resolve(auth0.Environment(state.Environment)) + if err != nil { + return err + } + client := &auth0.Client{Domain: state.IDPDomain, ClientID: clientID} + + token, err := client.RefreshToken(ctx, creds.RefreshToken) + if errors.Is(err, auth0.ErrInvalidGrant) { + return errors.New("session expired or revoked; run `lfx auth login` to log in again") + } + if err != nil { + return fmt.Errorf("refresh access token: %w", err) + } + + refreshToken := token.RefreshToken + if refreshToken == "" { + // Auth0 may not rotate the refresh token on every + // exchange; keep the existing one in that case. + refreshToken = creds.RefreshToken + } + if err := store.SaveCredentials(credstore.Credentials{ + RefreshToken: refreshToken, + AccessToken: token.AccessToken, + AccessTokenExpiry: token.Expiry, + }); err != nil { + return fmt.Errorf("save refreshed credentials: %w", err) + } + + fmt.Println(token.AccessToken) return nil }, } @@ -72,8 +291,48 @@ func newAuthStatusCommand() *cli.Command { return &cli.Command{ Name: "status", Usage: "Show the current authentication status", - Action: func(_ context.Context, _ *cli.Command) error { - fmt.Println("lfx auth status: not yet implemented (see LFXV2-2515)") + Action: func(_ context.Context, cmd *cli.Command) error { + store, err := credStoreFromCommand(cmd) + if err != nil { + return err + } + + creds, err := store.LoadCredentials() + if errors.Is(err, credstore.ErrNotFound) { + fmt.Println("Not logged in.") + return nil + } + if err != nil { + return err + } + + state, err := store.LoadDeviceState() + if err != nil && !errors.Is(err, credstore.ErrNotFound) { + return fmt.Errorf("load device state: %w", err) + } + + backend := "system keychain" + if cmd.Bool(insecureStorageFlagName) { + backend = "plain file (--insecure-storage)" + } + + fmt.Println("Logged in.") + if state.Environment != "" { + fmt.Printf(" Environment: %s\n", state.Environment) + } + if state.IDPDomain != "" { + fmt.Printf(" IdP domain: %s\n", state.IDPDomain) + } + if state.Audience != "" { + fmt.Printf(" Audience: %s\n", state.Audience) + } + fmt.Printf(" Credential backend: %s\n", backend) + if creds.ValidAccessToken() { + fmt.Printf(" Access token expires: %s\n", creds.AccessTokenExpiry.Format(time.RFC3339)) + } else { + fmt.Println(" Access token: expired or not cached (will refresh on next `lfx auth token`)") + } + return nil }, } @@ -83,9 +342,69 @@ func newAuthLogoutCommand() *cli.Command { return &cli.Command{ Name: "logout", Usage: "Remove stored LFX platform credentials", - Action: func(_ context.Context, _ *cli.Command) error { - fmt.Println("lfx auth logout: not yet implemented (see LFXV2-2515)") + Action: func(_ context.Context, cmd *cli.Command) error { + store, err := credStoreFromCommand(cmd) + if err != nil { + return err + } + + if err := store.DeleteCredentials(); err != nil { + return fmt.Errorf("delete credentials: %w", err) + } + if err := store.DeleteDeviceState(); err != nil { + return fmt.Errorf("delete device state: %w", err) + } + + fmt.Println("Logged out.") return nil }, } } + +// identityFromIDToken extracts a human-readable identifier (email or +// subject) from an unverified decode of the ID token's JWT payload. It is +// used only for a friendly "Logged in as ..." message; the access token +// (verified server-side on every API call) is the actual credential. +func identityFromIDToken(idToken string) string { + parts := strings.Split(idToken, ".") + if len(parts) != 3 { + return "" + } + + payload, err := base64URLDecode(parts[1]) + if err != nil { + return "" + } + + var claims struct { + Email string `json:"email"` + Sub string `json:"sub"` + } + if err := json.Unmarshal(payload, &claims); err != nil { + return "" + } + + if claims.Email != "" { + return claims.Email + } + return claims.Sub +} + +// base64URLDecode decodes a base64url-encoded JWT segment, tolerating the +// missing padding that JWTs conventionally omit. +func base64URLDecode(s string) ([]byte, error) { + return base64.RawURLEncoding.DecodeString(s) +} + +// openBrowser opens url in the user's default browser, following the +// per-OS conventions used by tools like `gh`. +func openBrowser(url string) error { + switch runtime.GOOS { + case "darwin": + return exec.Command("open", url).Start() + case "windows": + return exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start() + default: + return exec.Command("xdg-open", url).Start() + } +} diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 660c015..9f83c07 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -22,7 +22,8 @@ // acceptable, and is deliberately less secure than the keyring-backed // storage. // -// Non-sensitive state (device ID, IdP domain used at login) is always stored +// Non-sensitive state (environment, IdP domain, and audience used at login) +// is always stored // as plain JSON under the XDG state directory (~/.local/state/lfx-cli/ by // default), per XDG Base Directory conventions for mutable runtime state. package credstore @@ -94,10 +95,30 @@ func (c Credentials) ValidAccessToken() bool { // DeviceState holds non-sensitive information persisted between CLI // invocations so that commands like `lfx auth token` don't need to -// re-specify the IdP domain used at login. +// re-specify the environment, IdP domain, or audience used at login. +// +// Note: this deliberately does not include a persistent "device ID". One +// was considered (see LFXV2-2515/LFXV2-2509 discussion) on the assumption +// that `gh` uses one as part of its OAuth device flow, but `gh`'s +// `~/.local/state/gh/device-id` is actually just an anonymous telemetry +// identifier (see `internal/telemetry.getOrCreateDeviceID` in +// github.com/cli/cli) -- it plays no role in the OAuth device +// authorization grant and isn't sent to GitHub's API. Since the LFX CLI +// has no telemetry pipeline, and Auth0's device flow has no concept of a +// device ID at all, there's nothing here for one to do. Revisit if/when +// opt-in CLI telemetry is added. type DeviceState struct { - DeviceID string `json:"device_id"` IDPDomain string `json:"idp_domain,omitempty"` + // Environment is the `--env` value used at login (prod, staging, or + // development), determining which compiled-in client ID is used to + // refresh the access token. + Environment string `json:"environment,omitempty"` + // Audience is the `--audience` value used at login. Auth0's + // refresh_token grant automatically ties the refreshed access token + // to the audience it was originally issued for, so this isn't sent + // back on refresh; it's persisted purely for display in + // `lfx auth status`. + Audience string `json:"audience,omitempty"` } // Store is the credential storage abstraction used by the auth commands. @@ -117,6 +138,9 @@ type Store interface { // LoadDeviceState returns the persisted device state, or ErrNotFound if // none has been saved. LoadDeviceState() (DeviceState, error) + // DeleteDeviceState removes any persisted device state. It is a no-op + // if none exists. + DeleteDeviceState() error } // Options configures a Store returned by New. @@ -297,6 +321,18 @@ func (s *store) LoadDeviceState() (DeviceState, error) { return state, nil } +// DeleteDeviceState implements Store. +func (s *store) DeleteDeviceState() error { + path := filepath.Join(s.stateDir, stateFileName) + + err := os.Remove(path) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("credstore: delete device state: %w", err) + } + + return nil +} + // keyringSecrets is a secretsBackend that stores Credentials in a real // system credential store via keyring.Keyring (see systemBackends). type keyringSecrets struct { From 05b5affc67a67853492bf504249c25351df1ab52 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 09:32:10 -0700 Subject: [PATCH 02/17] fix(review): address PR #4 Copilot review feedback - auth token: use auth0.Resolve's domain (trusted) for the refresh client instead of the persisted state.json value, while still sanity-checking it against the stored domain to catch a tampered or corrupted state file before it can redirect a refresh request. - credstore: add DeviceState.Insecure, recording which credential backend (keychain vs. --insecure-storage) wrote state.json. auth token/status/logout now validate this against the invocation's own --insecure-storage flag before trusting or deleting state.json, so switching backends no longer silently corrupts or destroys the other backend's metadata. - README.md/AGENTS.md: update the stale "auth commands are stubs" note now that lfx auth is fully implemented; only lfx api remains a stub. - auth0: map DeviceAccessToken's context.DeadlineExceeded (returned once the device code's own expiry passes, ahead of the token endpoint ever reporting expired_token) to ErrExpiredToken, so the documented error and "device code expired" message are reliably produced. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- AGENTS.md | 14 ++-- README.md | 4 +- internal/auth0/device.go | 13 ++++ internal/commands/auth.go | 112 ++++++++++++++++++++++++++++---- internal/credstore/credstore.go | 9 +++ 5 files changed, 129 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c761719..ee97003 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,15 +46,11 @@ lfx-cli/ ### Current State -This repo is under active scaffolding. Auth and API commands are currently -stubs; real implementations land in follow-on work: - -- `lfx auth login` / `status` / `logout` -- `lfx auth token` -- `lfx api` - -Credential storage (system keychain via `99designs/keyring`) and the Auth0 -CIMD client are tracked separately. +`lfx auth login` / `status` / `token` / `logout` are fully implemented, +including the Auth0 Device Code flow, refresh-token exchange, and +credential storage (system keychain via `99designs/keyring`, with a plain +`--insecure-storage` fallback). `lfx api` remains a stub; its +implementation lands in follow-on work. **No container build**: this project produces binary artifacts only, distributed via GitHub Releases, the `install.sh` curl-style installer diff --git a/README.md b/README.md index 44f4b9d..0f7f734 100644 --- a/README.md +++ b/README.md @@ -58,8 +58,8 @@ lfx auth login --insecure-storage Run `lfx --help` or `lfx --help` for full details on any command. -> **Note:** This project is under active development. Authentication and API -> commands are currently stubs; see the +> **Note:** This project is under active development. `lfx auth` is fully +> implemented; `lfx api` is currently a stub. See the > [LFXV2-2509 epic](https://linuxfoundation.atlassian.net/browse/LFXV2-2509) > for status. diff --git a/internal/auth0/device.go b/internal/auth0/device.go index 4bbe920..4cf0504 100644 --- a/internal/auth0/device.go +++ b/internal/auth0/device.go @@ -171,6 +171,19 @@ func (dc *DeviceCode) Poll() (*oauth2.Token, error) { return nil, ErrExpiredToken } } + + // DeviceAccessToken derives its own polling deadline from the device + // code's Expiry and returns the bare context.DeadlineExceeded (not a + // *oauth2.RetrieveError) once that deadline passes, rather than + // waiting for the token endpoint to report "expired_token" itself. + // Since this Client never sets its own deadline on the context it + // passes in (see RequestDeviceCode), any DeadlineExceeded seen here + // can only come from that internal one, so it's safe to always treat + // it as ErrExpiredToken. + if errors.Is(err, context.DeadlineExceeded) { + return nil, ErrExpiredToken + } + return nil, fmt.Errorf("auth0: poll for token: %w", err) } diff --git a/internal/commands/auth.go b/internal/commands/auth.go index def32a9..eeb28ed 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -114,13 +114,14 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { return err } audience := cmd.String(audienceFlagName) + insecure := cmd.Bool(insecureStorageFlagName) if cmd.Bool(withTokenFlagName) { - return loginWithToken(store, env, domain, audience) + return loginWithToken(store, env, domain, audience, insecure) } client := &auth0.Client{Domain: domain, ClientID: clientID} - return loginWithDeviceCode(ctx, cmd, client, store, env, domain, audience) + return loginWithDeviceCode(ctx, cmd, client, store, env, domain, audience, insecure) } // loginWithToken implements `--with-token`: it reads a refresh token from @@ -128,7 +129,7 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { // `echo "$REFRESH_TOKEN" | lfx auth login --with-token`. No access token is // cached; the next `lfx auth token` call exchanges the refresh token for // one. -func loginWithToken(store credstore.Store, env auth0.Environment, domain, audience string) error { +func loginWithToken(store credstore.Store, env auth0.Environment, domain, audience string, insecure bool) error { reader := bufio.NewReader(os.Stdin) line, err := reader.ReadString('\n') if err != nil && !errors.Is(err, io.EOF) { @@ -146,6 +147,7 @@ func loginWithToken(store credstore.Store, env auth0.Environment, domain, audien IDPDomain: domain, Environment: string(env), Audience: audience, + Insecure: insecure, }); err != nil { return fmt.Errorf("save device state: %w", err) } @@ -165,6 +167,7 @@ func loginWithDeviceCode( store credstore.Store, env auth0.Environment, domain, audience string, + insecure bool, ) error { dc, err := client.RequestDeviceCode(ctx, audience, loginScopes) if err != nil { @@ -208,6 +211,7 @@ func loginWithDeviceCode( IDPDomain: domain, Environment: string(env), Audience: audience, + Insecure: insecure, }); err != nil { return fmt.Errorf("save device state: %w", err) } @@ -221,6 +225,69 @@ func loginWithDeviceCode( return nil } +// loadDeviceStateForBackend loads the persisted device state and validates +// it against the current invocation before returning it: +// +// - state.Insecure must match cmd's --insecure-storage flag. state.json is +// shared by both the keychain and plain-file credential backends (see +// credstore.DeviceState.Insecure), so a mismatch means this invocation's +// credentials were saved under a different backend than the one that +// last wrote state.json -- trusting it here would silently mix a +// refresh token from one backend with IdP/environment metadata written +// for the other. +// - the IdP domain implied by state.Environment (via auth0.Resolve, the +// source of truth) must match the persisted state.IDPDomain, guarding +// against a tampered or corrupted state.json redirecting a refresh +// request -- and its long-lived refresh token -- to another host. +// +// On success it returns the state along with the trusted domain and client +// ID to use for any Auth0 request (always auth0.Resolve's values, never the +// persisted ones). +func loadDeviceStateForBackend(store credstore.Store, cmd *cli.Command) (state credstore.DeviceState, domain, clientID string, err error) { + state, err = store.LoadDeviceState() + if err != nil { + return credstore.DeviceState{}, "", "", err + } + + if state.Insecure != cmd.Bool(insecureStorageFlagName) { + return credstore.DeviceState{}, "", "", fmt.Errorf( + "stored login state belongs to %s; pass %s to match, or run `lfx auth login` again", + backendDescription(state.Insecure), insecureStorageUsageHint(state.Insecure), + ) + } + + domain, clientID, err = auth0.Resolve(auth0.Environment(state.Environment)) + if err != nil { + return credstore.DeviceState{}, "", "", err + } + if domain != state.IDPDomain { + return credstore.DeviceState{}, "", "", fmt.Errorf( + "stored IdP domain %q does not match %q for environment %q; run `lfx auth login` again", + state.IDPDomain, domain, state.Environment, + ) + } + + return state, domain, clientID, nil +} + +// backendDescription renders a human-readable name for a credential +// backend, for use in error messages. +func backendDescription(insecure bool) string { + if insecure { + return "the plain-file (--insecure-storage) backend" + } + return "the system keychain" +} + +// insecureStorageUsageHint renders the flag (or its absence) needed to +// select the given backend, for use in error messages. +func insecureStorageUsageHint(insecure bool) string { + if insecure { + return "--insecure-storage" + } + return "no --insecure-storage" +} + func newAuthTokenCommand() *cli.Command { return &cli.Command{ Name: "token", @@ -248,16 +315,11 @@ func newAuthTokenCommand() *cli.Command { return errors.New("no refresh token available; run `lfx auth login` again") } - state, err := store.LoadDeviceState() + _, domain, clientID, err := loadDeviceStateForBackend(store, cmd) if err != nil { return fmt.Errorf("load device state: %w", err) } - - _, clientID, err := auth0.Resolve(auth0.Environment(state.Environment)) - if err != nil { - return err - } - client := &auth0.Client{Domain: state.IDPDomain, ClientID: clientID} + client := &auth0.Client{Domain: domain, ClientID: clientID} token, err := client.RefreshToken(ctx, creds.RefreshToken) if errors.Is(err, auth0.ErrInvalidGrant) { @@ -317,6 +379,13 @@ func newAuthStatusCommand() *cli.Command { } fmt.Println("Logged in.") + if err == nil && state.Insecure != cmd.Bool(insecureStorageFlagName) { + fmt.Printf( + " Note: stored login state below belongs to %s, not this credential backend; "+ + "it may not describe these credentials. Run `lfx auth login` to refresh it.\n", + backendDescription(state.Insecure), + ) + } if state.Environment != "" { fmt.Printf(" Environment: %s\n", state.Environment) } @@ -351,8 +420,27 @@ func newAuthLogoutCommand() *cli.Command { if err := store.DeleteCredentials(); err != nil { return fmt.Errorf("delete credentials: %w", err) } - if err := store.DeleteDeviceState(); err != nil { - return fmt.Errorf("delete device state: %w", err) + + // state.json is shared by both credential backends (see + // credstore.DeviceState.Insecure), so only delete it when it + // actually describes this invocation's backend; otherwise + // logging out of one backend would destroy metadata (env, IdP + // domain) still needed by the other backend's credentials. + state, err := store.LoadDeviceState() + switch { + case errors.Is(err, credstore.ErrNotFound): + // Nothing to delete. + case err != nil: + return fmt.Errorf("load device state: %w", err) + case state.Insecure == cmd.Bool(insecureStorageFlagName): + if err := store.DeleteDeviceState(); err != nil { + return fmt.Errorf("delete device state: %w", err) + } + default: + fmt.Printf( + "Note: leaving stored login state in place; it belongs to %s.\n", + backendDescription(state.Insecure), + ) } fmt.Println("Logged out.") diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 9f83c07..9b9136d 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -119,6 +119,15 @@ type DeviceState struct { // back on refresh; it's persisted purely for display in // `lfx auth status`. Audience string `json:"audience,omitempty"` + // Insecure records whether `--insecure-storage` was passed at login, + // i.e. whether Credentials live in the plain-file backend rather than + // the system keychain. state.json itself is not namespaced by + // backend (both share the same file), so callers must check this + // against the invocation's own --insecure-storage flag before trusting + // the rest of the state: without that check, logging into one backend + // silently overwrites the metadata (env, IdP domain) that the other + // backend's still-present credentials depend on. + Insecure bool `json:"insecure,omitempty"` } // Store is the credential storage abstraction used by the auth commands. From 161c203764ffadb16f57e098338efc5a10a48ae7 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 10:32:52 -0700 Subject: [PATCH 03/17] fix(auth0): use prod custom domain sso.linuxfoundation.org lfx auth login --env prod was sending device code and token requests to linuxfoundation.auth0.com, but end users only ever authenticate at the prod tenant's custom domain, sso.linuxfoundation.org (see auth0-terraform's auth0_domain variable). The mismatch surfaced as an Auth0 invalid_request: Missing required parameter: response_type error completing login, and a login that never resolved when polling. Since this Client only talks to the device code and token endpoints (never the Auth0 Management API), there's no need to separately track each environment's underlying tenant name -- only the IdP domain end users authenticate against, which for prod is the custom domain rather than the *.auth0.com domain. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/auth0/device.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/internal/auth0/device.go b/internal/auth0/device.go index 4cf0504..18d7b13 100644 --- a/internal/auth0/device.go +++ b/internal/auth0/device.go @@ -30,9 +30,15 @@ const ( EnvDevelopment Environment = "development" ) -// domains maps each Environment to its Auth0 tenant domain. +// domains maps each Environment to the Auth0 IdP domain end users +// authenticate against, matching auth0-terraform's own `auth0_domain` +// variable. This is deliberately not each tenant's *.auth0.com domain: prod +// fronts its tenant with the custom domain sso.linuxfoundation.org, and +// since this Client never calls the Auth0 Management API (only the device +// code and token endpoints), there's no need to separately track the +// underlying tenant name. var domains = map[Environment]string{ - EnvProd: "linuxfoundation.auth0.com", + EnvProd: "sso.linuxfoundation.org", EnvStaging: "linuxfoundation-staging.auth0.com", EnvDevelopment: "linuxfoundation-dev.auth0.com", } @@ -73,7 +79,9 @@ func Resolve(env Environment) (domain, clientID string, err error) { // Method: None; is_first_party = true), so ClientSecret is deliberately // left unset. type Client struct { - // Domain is the Auth0 tenant domain, e.g. "linuxfoundation.auth0.com". + // Domain is the Auth0 IdP domain end users authenticate against, e.g. + // "linuxfoundation-dev.auth0.com" or, for a tenant fronted by a custom + // domain, "sso.linuxfoundation.org". Domain string // ClientID is the Auth0 application client ID used for both the // device code request and subsequent token/refresh exchanges. From d94eda8135f7c210ef2e0249edbec0713a3f0b0b Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 10:35:45 -0700 Subject: [PATCH 04/17] fix(review): address second round of Copilot review comments - Poll(): distinguish a caller-supplied context deadline from DeviceAccessToken's own internally-derived one before mapping DeadlineExceeded to ErrExpiredToken, so an unrelated caller timeout is no longer misreported as an expired device code. - loginWithDeviceCode: fall back to VerificationURI when VerificationURIComplete is empty (it's optional per RFC 8628 section 3.2), so a login doesn't try to open/print an empty URL. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/auth0/device.go | 11 ++++++----- internal/commands/auth.go | 15 +++++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/auth0/device.go b/internal/auth0/device.go index 18d7b13..7389bf9 100644 --- a/internal/auth0/device.go +++ b/internal/auth0/device.go @@ -184,11 +184,12 @@ func (dc *DeviceCode) Poll() (*oauth2.Token, error) { // code's Expiry and returns the bare context.DeadlineExceeded (not a // *oauth2.RetrieveError) once that deadline passes, rather than // waiting for the token endpoint to report "expired_token" itself. - // Since this Client never sets its own deadline on the context it - // passes in (see RequestDeviceCode), any DeadlineExceeded seen here - // can only come from that internal one, so it's safe to always treat - // it as ErrExpiredToken. - if errors.Is(err, context.DeadlineExceeded) { + // dc.ctx may also carry a caller-supplied deadline of its own (e.g. if + // RequestDeviceCode was called with one), so only treat + // DeadlineExceeded as an expired device code if dc.ctx itself isn't + // what expired -- otherwise this would misreport an unrelated caller + // timeout as ErrExpiredToken. + if errors.Is(err, context.DeadlineExceeded) && dc.ctx.Err() == nil { return nil, ErrExpiredToken } diff --git a/internal/commands/auth.go b/internal/commands/auth.go index eeb28ed..1c40e3f 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -175,14 +175,21 @@ func loginWithDeviceCode( } fmt.Printf("First copy your one-time code: %s\n", dc.UserCode) + // VerificationURIComplete is optional per RFC 8628 §3.2; fall back to + // VerificationURI (always present) plus the user code if the IdP + // doesn't supply it. + verificationURI := dc.VerificationURIComplete + if verificationURI == "" { + verificationURI = dc.VerificationURI + } if cmd.Bool(webFlagName) { - fmt.Printf("Opening %s in your browser...\n", dc.VerificationURIComplete) - if err := openBrowser(dc.VerificationURIComplete); err != nil { + fmt.Printf("Opening %s in your browser...\n", verificationURI) + if err := openBrowser(verificationURI); err != nil { fmt.Printf("Couldn't open browser automatically: %v\n", err) - fmt.Printf("Please visit: %s\n", dc.VerificationURIComplete) + fmt.Printf("Please visit: %s\n", verificationURI) } } else { - fmt.Printf("Then visit: %s\n", dc.VerificationURIComplete) + fmt.Printf("Then visit: %s\n", verificationURI) } fmt.Println("Waiting for authentication...") From b6388d1e8a0e814ec2d26d928d75560f212f0568 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 10:41:55 -0700 Subject: [PATCH 05/17] feat(auth): show LFID username in the login greeting lfx auth login's "Logged in as ..." message now prefers the LFID username (the https://sso.linuxfoundation.org/claims/ custom ID token claim) over email, since username is the conventional LFX identifier. Shows "username (email)" when both are present, falling back to "email (no username)" in the unexpected case the custom claim is missing, and finally the subject claim if neither is present. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 1c40e3f..e64a832 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -456,10 +456,18 @@ func newAuthLogoutCommand() *cli.Command { } } -// identityFromIDToken extracts a human-readable identifier (email or -// subject) from an unverified decode of the ID token's JWT payload. It is -// used only for a friendly "Logged in as ..." message; the access token -// (verified server-side on every API call) is the actual credential. +// lfidClaimsNamespace prefixes the custom LFID claims Auth0 adds to the ID +// token, namely username. Distinct from the shorter "http://lfx.dev/claims" +// LFX claims namespace used elsewhere. +const lfidClaimsNamespace = "https://sso.linuxfoundation.org/claims/" + +// identityFromIDToken extracts a human-readable identity from an unverified +// decode of the ID token's JWT payload, for a friendly "Logged in as ..." +// message; the access token (verified server-side on every API call) is the +// actual credential. LFX usernames (the custom claim above) are the +// conventional identifier; email is included alongside it when present, and +// email or subject alone are used as fallbacks if the custom claim is +// unexpectedly missing. func identityFromIDToken(idToken string) string { parts := strings.Split(idToken, ".") if len(parts) != 3 { @@ -471,18 +479,25 @@ func identityFromIDToken(idToken string) string { return "" } - var claims struct { - Email string `json:"email"` - Sub string `json:"sub"` - } + var claims map[string]any if err := json.Unmarshal(payload, &claims); err != nil { return "" } - if claims.Email != "" { - return claims.Email + username, _ := claims[lfidClaimsNamespace+"username"].(string) + email, _ := claims["email"].(string) + sub, _ := claims["sub"].(string) + + switch { + case username != "" && email != "": + return fmt.Sprintf("%s (%s)", username, email) + case username != "": + return username + case email != "": + return fmt.Sprintf("%s (no username)", email) + default: + return sub } - return claims.Sub } // base64URLDecode decodes a base64url-encoded JWT segment, tolerating the From b632b5eeed1d12305f1b87a17a894d206015248d Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 16:57:04 -0700 Subject: [PATCH 06/17] fix(auth): remove code duplication flagged by jscpd Extract persistLogin and loadStoredCredentials helpers to eliminate the two Go clones MegaLinter's jscpd check flagged in internal/commands/auth.go: the duplicated SaveCredentials + SaveDeviceState pairs in the --with-token and device-code login paths, and the duplicated LoadCredentials/ErrNotFound loading pattern shared by the token and status commands. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 91 +++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 38 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index e64a832..701e04f 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -140,16 +140,12 @@ func loginWithToken(store credstore.Store, env auth0.Environment, domain, audien return errors.New("no refresh token provided on stdin") } - if err := store.SaveCredentials(credstore.Credentials{RefreshToken: refreshToken}); err != nil { - return fmt.Errorf("save credentials: %w", err) - } - if err := store.SaveDeviceState(credstore.DeviceState{ - IDPDomain: domain, - Environment: string(env), - Audience: audience, - Insecure: insecure, - }); err != nil { - return fmt.Errorf("save device state: %w", err) + if err := persistLogin( + store, + credstore.Credentials{RefreshToken: refreshToken}, + credstore.DeviceState{IDPDomain: domain, Environment: string(env), Audience: audience, Insecure: insecure}, + ); err != nil { + return err } fmt.Println("Logged in with a supplied refresh token.") @@ -207,20 +203,16 @@ func loginWithDeviceCode( return err } - if err := store.SaveCredentials(credstore.Credentials{ - RefreshToken: token.RefreshToken, - AccessToken: token.AccessToken, - AccessTokenExpiry: token.Expiry, - }); err != nil { - return fmt.Errorf("save credentials: %w", err) - } - if err := store.SaveDeviceState(credstore.DeviceState{ - IDPDomain: domain, - Environment: string(env), - Audience: audience, - Insecure: insecure, - }); err != nil { - return fmt.Errorf("save device state: %w", err) + if err := persistLogin( + store, + credstore.Credentials{ + RefreshToken: token.RefreshToken, + AccessToken: token.AccessToken, + AccessTokenExpiry: token.Expiry, + }, + credstore.DeviceState{IDPDomain: domain, Environment: string(env), Audience: audience, Insecure: insecure}, + ); err != nil { + return err } fmt.Println("Login successful.") @@ -295,23 +287,51 @@ func insecureStorageUsageHint(insecure bool) string { return "no --insecure-storage" } +// persistLogin saves creds and state together, wrapping any failure with +// context on which write failed. Note: the two writes are not atomic; if +// SaveDeviceState fails after SaveCredentials succeeds, the caller is left +// with new credentials paired with stale or missing environment metadata. +func persistLogin(store credstore.Store, creds credstore.Credentials, state credstore.DeviceState) error { + if err := store.SaveCredentials(creds); err != nil { + return fmt.Errorf("save credentials: %w", err) + } + if err := store.SaveDeviceState(state); err != nil { + return fmt.Errorf("save device state: %w", err) + } + return nil +} + +// loadStoredCredentials builds a credstore.Store for cmd and loads its +// credentials, returning (creds, false, nil) when none are stored +// (credstore.ErrNotFound) instead of treating that as an error, since +// callers report "not logged in" differently. +func loadStoredCredentials(cmd *cli.Command) (store credstore.Store, creds credstore.Credentials, found bool, err error) { + store, err = credStoreFromCommand(cmd) + if err != nil { + return nil, credstore.Credentials{}, false, err + } + creds, err = store.LoadCredentials() + if errors.Is(err, credstore.ErrNotFound) { + return store, credstore.Credentials{}, false, nil + } + if err != nil { + return nil, credstore.Credentials{}, false, err + } + return store, creds, true, nil +} + func newAuthTokenCommand() *cli.Command { return &cli.Command{ Name: "token", Usage: "Print a valid access token for the LFX platform", Action: func(ctx context.Context, cmd *cli.Command) error { - store, err := credStoreFromCommand(cmd) + store, creds, found, err := loadStoredCredentials(cmd) if err != nil { return err } - - creds, err := store.LoadCredentials() - if errors.Is(err, credstore.ErrNotFound) { + if !found { return errors.New("not logged in; run `lfx auth login` first") } - if err != nil { - return err - } if creds.ValidAccessToken() { fmt.Println(creds.AccessToken) @@ -361,19 +381,14 @@ func newAuthStatusCommand() *cli.Command { Name: "status", Usage: "Show the current authentication status", Action: func(_ context.Context, cmd *cli.Command) error { - store, err := credStoreFromCommand(cmd) + store, creds, found, err := loadStoredCredentials(cmd) if err != nil { return err } - - creds, err := store.LoadCredentials() - if errors.Is(err, credstore.ErrNotFound) { + if !found { fmt.Println("Not logged in.") return nil } - if err != nil { - return err - } state, err := store.LoadDeviceState() if err != nil && !errors.Is(err, credstore.ErrNotFound) { From 5488a60f822bcf2e4de8a6ff15a64312bbd5e3b8 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 16:57:12 -0700 Subject: [PATCH 07/17] docs(agents): add Go toolchain version policy, downgrade go.mod Document a Go toolchain upgrade policy in the Contributing Guidelines: freely bump go.mod's go directive to the latest patch release, but only bump the minor version when the user explicitly asks for it and it has been validated against the Go version MegaLinter itself bundles -- MegaLinter runs several linters (e.g. golangci-lint) against its own bundled Go version, and a go.mod directive newer than that bundled version breaks those checks. Includes the concrete steps to look up MegaLinter's bundled Go version from its pinned flavor tag, and a one-liner using the go.dev/dl JSON feed to find the latest patch release for the minor version currently pinned in go.mod. Downgrade go.mod's go directive from 1.26.5 to 1.25.14 (the latest 1.25.x patch release). Verified against .github/workflows/mega-linter.yml, which pins oxsecurity/megalinter's go flavor to v9.6.0; that flavor's Dockerfile bundles Go 1.26.3 (GO_ALPINE_VERSION=1.26.3-r0), so the previous 1.26.5 directive was already newer than MegaLinter's own Go toolchain -- exactly the failure mode this policy exists to prevent. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- AGENTS.md | 47 ++++++++++++++++++++++++++++++++++++++++++++--- go.mod | 2 +- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ee97003..c82ec77 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -229,6 +229,47 @@ release binaries may be missing even though the GitHub Release exists. 2. **Package Comments**: Every new `*.go` file must include the same `// Package ...` doc comment as the rest of its package 3. **Dependencies**: Run `go get -u ./... && go mod tidy` before every PR to - keep dependencies current -4. **Code Quality**: Run `make check` before commits -5. **Documentation**: Update README.md for user-facing changes + keep dependencies current. This upgrades module dependencies only, not the + Go toolchain itself (`go.mod`'s `go` directive) -- see the toolchain policy + below before touching that. +4. **Go toolchain version**: Freely bump `go.mod`'s `go` directive to the + latest available *patch* release (e.g. `1.X.Y` → `1.X.{Y+1}`) to pick up + security fixes. Do **not** bump the *minor* version (e.g. `1.X.x` → + `1.{X+1}.x`) unless the user explicitly asks for it, **and** you've + validated it against the Go version MegaLinter itself bundles -- + MegaLinter runs several linters (e.g. `golangci-lint`) against its own + bundled Go version, and a `go.mod` directive newer than that bundled + version breaks those checks. + + To find MegaLinter's bundled Go version: + + ```bash + # 1. Find the MegaLinter flavor and pinned version tag used in CI. + grep -A1 'oxsecurity/megalinter' .github/workflows/*.yml + # e.g. "uses: oxsecurity/megalinter/flavors/@ # " + + # 2. Fetch that flavor's Dockerfile and read its GO_ALPINE_VERSION (or + # GO_IMAGE_VERSION) build arg. + curl -s "https://raw.githubusercontent.com/oxsecurity/megalinter//flavors//Dockerfile" \ + | grep -i 'GO_ALPINE_VERSION\|GO_IMAGE_VERSION' + ``` + + `go.mod`'s `go` directive must never exceed that bundled version. Staying + one minor version behind it (rather than matching its minor *and* patch + exactly) leaves room to always take the latest patch release for security + fixes without ever being blocked by MegaLinter's own bundled patch version + lagging behind a newly disclosed vulnerability. + + There's no built-in `go` subcommand to look up the latest patch release + for a given minor version -- query the official `go.dev/dl` JSON feed + instead: + + ```bash + # Find the latest patch release for the minor version pinned in go.mod. + MINOR=$(grep '^go ' go.mod | awk '{print $2}' | cut -d. -f1,2) + curl -s "https://go.dev/dl/?mode=json&include=all" \ + | jq -r --arg m "go${MINOR}." '.[].version | select(startswith($m))' \ + | sort -V | tail -1 + ``` +5. **Code Quality**: Run `make check` before commits +6. **Documentation**: Update README.md for user-facing changes diff --git a/go.mod b/go.mod index c967d6c..e65b3ed 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ // SPDX-License-Identifier: MIT module github.com/linuxfoundation/lfx-cli -go 1.26.5 +go 1.25.14 require ( github.com/99designs/keyring v1.2.2 From 98b10ccf6221428b72e01982c34da25190bbed37 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 19 Aug 2026 17:09:48 -0700 Subject: [PATCH 08/17] fix(auth): roll back saved credentials if device-state save fails persistLogin saves credentials and device state in two separate, non-atomic writes. Per Copilot review feedback on PR #4, if SaveDeviceState fails after SaveCredentials has already succeeded, the new refresh token was left paired with stale or missing environment metadata, so a later refresh could target the wrong Auth0 tenant. Delete the newly saved credentials when SaveDeviceState fails, so a partial failure leaves the previous login state undisturbed rather than a mismatched credentials/metadata pair. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 701e04f..3cde10f 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -287,15 +287,19 @@ func insecureStorageUsageHint(insecure bool) string { return "no --insecure-storage" } -// persistLogin saves creds and state together, wrapping any failure with -// context on which write failed. Note: the two writes are not atomic; if -// SaveDeviceState fails after SaveCredentials succeeds, the caller is left -// with new credentials paired with stale or missing environment metadata. +// persistLogin saves creds and state together. The two writes are not +// atomic, so if SaveDeviceState fails after SaveCredentials succeeds, the +// newly saved credentials are rolled back (deleted) rather than left paired +// with stale or missing environment metadata, which could otherwise send a +// later refresh to the wrong Auth0 tenant. func persistLogin(store credstore.Store, creds credstore.Credentials, state credstore.DeviceState) error { if err := store.SaveCredentials(creds); err != nil { return fmt.Errorf("save credentials: %w", err) } if err := store.SaveDeviceState(state); err != nil { + if delErr := store.DeleteCredentials(); delErr != nil { + return fmt.Errorf("save device state: %w (and rollback of saved credentials also failed: %v)", err, delErr) + } return fmt.Errorf("save device state: %w", err) } return nil From 7ad645bdf318b321d1be336ba51d056b88fbbcbb Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Thu, 20 Aug 2026 13:07:04 -0700 Subject: [PATCH 09/17] feat(auth): add `lfx auth backends` to list credential-store backends Lists the system credential-store backends compiled into this binary for the current OS (macOS Keychain, Linux Secret Service/KWallet, Windows Credential Manager, or pass), in the priority order `lfx auth login` tries them via keyring.Open. This only reports what's compiled in per-OS build tags (credstore.AvailableBackends wraps keyring.AvailableBackends), not whether a given backend is actually usable at runtime. Also renames user-facing "system keychain" wording to "system backend" (--insecure-storage flag usage text, `auth status` output, and backendDescription's error-message text) since it's a more accurate, OS-agnostic term now that `auth backends` exists to name individual backends like the macOS Keychain specifically. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 35 ++++++++++++++++++++++--- internal/credstore/credstore.go | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 3cde10f..6f2e225 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -45,7 +45,7 @@ var loginScopes = []string{"openid", "profile", "email", "offline_access"} // The --insecure-storage flag is shared by all subcommands (it is not // declared as a "Local" flag, so urfave/cli resolves it for subcommand // actions via cmd.Bool) and controls whether credentials bypass the system -// keychain in favor of credstore's plain (unencrypted) file fallback, e.g. +// backend in favor of credstore's plain (unencrypted) file fallback, e.g. // for headless/CI use. func NewAuthCommand() *cli.Command { return &cli.Command{ @@ -54,7 +54,7 @@ func NewAuthCommand() *cli.Command { Flags: []cli.Flag{ &cli.BoolFlag{ Name: insecureStorageFlagName, - Usage: "Store credentials in a plain (unencrypted) file instead of the system keychain", + Usage: "Store credentials in a plain (unencrypted) file instead of the system backend", }, }, Commands: []*cli.Command{ @@ -62,6 +62,7 @@ func NewAuthCommand() *cli.Command { newAuthTokenCommand(), newAuthStatusCommand(), newAuthLogoutCommand(), + newAuthBackendsCommand(), }, } } @@ -275,7 +276,7 @@ func backendDescription(insecure bool) string { if insecure { return "the plain-file (--insecure-storage) backend" } - return "the system keychain" + return "the system backend" } // insecureStorageUsageHint renders the flag (or its absence) needed to @@ -399,7 +400,7 @@ func newAuthStatusCommand() *cli.Command { return fmt.Errorf("load device state: %w", err) } - backend := "system keychain" + backend := "system backend" if cmd.Bool(insecureStorageFlagName) { backend = "plain file (--insecure-storage)" } @@ -475,6 +476,32 @@ func newAuthLogoutCommand() *cli.Command { } } +// newAuthBackendsCommand builds `lfx auth backends`, which lists the system +// credential-store backends compiled into this binary for the current OS +// (see credstore.AvailableBackends), in the priority order `lfx auth login` +// would try them. It does not attempt to open any backend, so a listed +// backend may still turn out to be unusable at runtime (e.g. no D-Bus +// session for Secret Service, `pass` not initialized). +func newAuthBackendsCommand() *cli.Command { + return &cli.Command{ + Name: "backends", + Usage: "List the system credential-store backends available on this OS", + Action: func(_ context.Context, _ *cli.Command) error { + backends := credstore.AvailableBackends() + if len(backends) == 0 { + fmt.Println("No system credential-store backends are available on this OS; `lfx auth login` requires --insecure-storage.") + return nil + } + + fmt.Println("Available credential-store backends, in the order `lfx auth login` tries them:") + for _, b := range backends { + fmt.Printf(" %-15s %s\n", b.Name, b.DisplayName) + } + return nil + }, + } +} + // lfidClaimsNamespace prefixes the custom LFID claims Auth0 adds to the ID // token, namely username. Distinct from the shorter "http://lfx.dev/claims" // LFX claims namespace used elsewhere. diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 9b9136d..ad8cf4d 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -77,6 +77,52 @@ var systemBackends = []keyring.BackendType{ keyring.PassBackend, } +// backendDisplayNames maps keyring.BackendType values to the human-readable +// names shown by `lfx auth backends`. +var backendDisplayNames = map[keyring.BackendType]string{ + keyring.SecretServiceBackend: "Secret Service (GNOME Keyring, KeePassXC, etc.)", + keyring.KeychainBackend: "macOS Keychain", + keyring.KWalletBackend: "KDE Wallet (kwallet)", + keyring.WinCredBackend: "Windows Credential Manager", + keyring.PassBackend: "gpg-encrypted vault (passwordstore.org)", +} + +// Backend describes one of the system credential-store backends compiled +// into this binary for the current OS, as reported by `lfx auth backends`. +type Backend struct { + // Name is the keyring.BackendType identifier (e.g. "keychain"). + Name string + // DisplayName is a human-readable label for Name. + DisplayName string +} + +// AvailableBackends reports the system credential-store backends compiled +// into this binary for the current OS (Go build tags determine which +// backends are even possible per-platform; see the per-backend source +// files in github.com/99designs/keyring), in the same priority order (see +// systemBackends) that New passes to keyring.Open as AllowedBackends. It +// does not attempt to open any backend, so a backend listed here may still +// fail at login time if it isn't actually usable at runtime (e.g. no D-Bus +// session for Secret Service, `pass` not initialized, etc.). +func AvailableBackends() []Backend { + available := make(map[keyring.BackendType]bool) + for _, b := range keyring.AvailableBackends() { + available[b] = true + } + + var backends []Backend + for _, b := range systemBackends { + if !available[b] { + continue + } + backends = append(backends, Backend{ + Name: string(b), + DisplayName: backendDisplayNames[b], + }) + } + return backends +} + // Credentials holds the secrets needed to authenticate with the LFX // platform: the long-lived Auth0 refresh token, and an optional cached // access token with its expiry. From af44710967397b0f205d5041fe2d12732d8074cc Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 15:00:24 -0700 Subject: [PATCH 10/17] Address PR review feedback and simplify auth0 device-code flow Address PR #4 review feedback (LFXV2-2515): - Fix wrapped doc comment in credstore.go. - Document persistLogin's rollback-on-failure limitation (a failed re-login can log the user out rather than preserve the prior session) instead of changing the behavior. - Reject an empty refresh token in loginWithDeviceCode instead of silently persisting a broken session, since Auth0 does not guarantee offline_access is honored for every audience. - Add tests for credstore's Save/Load/Delete round-trip and ValidAccessToken, and for persistLogin's rollback path, loadDeviceStateForBackend's backend/domain mismatch guards, and identityFromIDToken's claim-selection logic. Simplify the Auth0 device-code integration: - Remove the internal/auth0 package's Client/DeviceCode wrapper types and unused HTTPClient test seam; call golang.org/x/oauth2 directly from internal/commands/auth.go. - Move environment-to-domain/client-ID resolution into internal/commands/environment.go, the only real business logic the old package added. - Map context.DeadlineExceeded and RetrieveError{ErrorCode: "expired_token"} to the same "timed out waiting for login" message: both represent the device code's RFC 8628 verification window expiring (a client-side deadline vs. a server-confirmed one), not a token or network timeout. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/auth0/device.go | 212 ------------------------ internal/commands/auth.go | 136 ++++++++++----- internal/commands/auth_test.go | 236 +++++++++++++++++++++++++++ internal/commands/environment.go | 68 ++++++++ internal/credstore/credstore.go | 6 +- internal/credstore/credstore_test.go | 133 +++++++++++++++ 6 files changed, 535 insertions(+), 256 deletions(-) delete mode 100644 internal/auth0/device.go create mode 100644 internal/commands/auth_test.go create mode 100644 internal/commands/environment.go create mode 100644 internal/credstore/credstore_test.go diff --git a/internal/auth0/device.go b/internal/auth0/device.go deleted file mode 100644 index 7389bf9..0000000 --- a/internal/auth0/device.go +++ /dev/null @@ -1,212 +0,0 @@ -// Copyright The Linux Foundation and each contributor to LFX. -// SPDX-License-Identifier: MIT - -// Package auth0 configures the Auth0 Device Authorization Grant (RFC 8628) -// and refresh-token exchange used by `lfx auth login` and `lfx auth token`, -// on top of golang.org/x/oauth2's device flow support -// (Config.DeviceAuth/DeviceAccessToken and TokenSource-based refresh). -package auth0 - -import ( - "context" - "errors" - "fmt" - "net/http" - - "golang.org/x/oauth2" -) - -// Environment identifies which LFX Auth0 tenant/IdP a login targets. -type Environment string - -// Supported environments, selected via the `--env` flag. Each maps to a -// fixed IdP domain and a static, pre-provisioned Auth0 native application -// client ID (device_code + refresh_token grants; see LFXV2-2513 and -// auth0-terraform PR #348). CIMD was evaluated and abandoned for this flow: -// Auth0 silently drops CIMD client registration for the device_code grant. -const ( - EnvProd Environment = "prod" - EnvStaging Environment = "staging" - EnvDevelopment Environment = "development" -) - -// domains maps each Environment to the Auth0 IdP domain end users -// authenticate against, matching auth0-terraform's own `auth0_domain` -// variable. This is deliberately not each tenant's *.auth0.com domain: prod -// fronts its tenant with the custom domain sso.linuxfoundation.org, and -// since this Client never calls the Auth0 Management API (only the device -// code and token endpoints), there's no need to separately track the -// underlying tenant name. -var domains = map[Environment]string{ - EnvProd: "sso.linuxfoundation.org", - EnvStaging: "linuxfoundation-staging.auth0.com", - EnvDevelopment: "linuxfoundation-dev.auth0.com", -} - -// clientIDs maps each Environment to its compiled-in, pre-provisioned -// Auth0 native application client ID for the device code grant. -// cspell:disable -- opaque, randomly-generated Auth0 client IDs, not words. -var clientIDs = map[Environment]string{ - EnvProd: "kkCpM0c9zJ0vNZZDDOGqcyzocOBircOn", - EnvStaging: "9XzXgDfAB9O7IoHqhBj5mg4VLvdBM8ci", - EnvDevelopment: "0TN1OElqQY146vLEPdV5qfejRKpc9IAZ", -} - -// cspell:enable - -// DefaultAudience is the production LFX v2 API audience used unless -// overridden via `--audience`. It intentionally does not vary with `--env`: -// per LFXV2-2515, the audience is independent of the selected environment -// and must be set explicitly when testing a non-prod API. -const DefaultAudience = "https://lfx-api.v2.cluster.lfx.dev/" - -// ErrInvalidEnvironment is returned by Resolve for an unrecognized -// Environment value. -var ErrInvalidEnvironment = errors.New("auth0: invalid environment") - -// Resolve returns the IdP domain and client ID for env. -func Resolve(env Environment) (domain, clientID string, err error) { - domain, ok := domains[env] - if !ok { - return "", "", fmt.Errorf("%w: %q (must be one of prod, staging, development)", ErrInvalidEnvironment, env) - } - return domain, clientIDs[env], nil -} - -// Client drives the Auth0 device code and refresh-token exchanges for a -// single environment, via an underlying oauth2.Config. Auth0's device code -// application is a public native client (Token Endpoint Authentication -// Method: None; is_first_party = true), so ClientSecret is deliberately -// left unset. -type Client struct { - // Domain is the Auth0 IdP domain end users authenticate against, e.g. - // "linuxfoundation-dev.auth0.com" or, for a tenant fronted by a custom - // domain, "sso.linuxfoundation.org". - Domain string - // ClientID is the Auth0 application client ID used for both the - // device code request and subsequent token/refresh exchanges. - ClientID string - // HTTPClient is used for all requests. Defaults to - // http.DefaultClient if nil. - HTTPClient *http.Client -} - -// config builds the oauth2.Config used for a device code flow requesting -// scopes, pointed at this Client's Auth0 tenant. -func (c *Client) config(scopes []string) *oauth2.Config { - return &oauth2.Config{ - ClientID: c.ClientID, - Scopes: scopes, - Endpoint: oauth2.Endpoint{ - DeviceAuthURL: "https://" + c.Domain + "/oauth/device/code", - TokenURL: "https://" + c.Domain + "/oauth/token", - AuthStyle: oauth2.AuthStyleInParams, - }, - } -} - -// context attaches Client's HTTPClient to ctx, per the convention -// documented on oauth2.HTTPClient, so all requests made by the resulting -// oauth2.Config use it. -func (c *Client) context(ctx context.Context) context.Context { - if c.HTTPClient == nil { - return ctx - } - return context.WithValue(ctx, oauth2.HTTPClient, c.HTTPClient) -} - -// DeviceCode wraps the RFC 8628 §3.2 device authorization response, -// carrying the oauth2.Config needed to complete the flow via Poll. -type DeviceCode struct { - *oauth2.DeviceAuthResponse - cfg *oauth2.Config - ctx context.Context //nolint:containedctx // ctx is captured for reuse by Poll, matching Client.RequestDeviceCode/Poll's split across a user-facing pause. -} - -// RequestDeviceCode starts the device authorization flow for the given -// audience and scopes. -func (c *Client) RequestDeviceCode(ctx context.Context, audience string, scopes []string) (*DeviceCode, error) { - cfg := c.config(scopes) - authCtx := c.context(ctx) - - var opts []oauth2.AuthCodeOption - if audience != "" { - opts = append(opts, oauth2.SetAuthURLParam("audience", audience)) - } - - resp, err := cfg.DeviceAuth(authCtx, opts...) - if err != nil { - return nil, fmt.Errorf("auth0: request device code: %w", err) - } - return &DeviceCode{DeviceAuthResponse: resp, cfg: cfg, ctx: authCtx}, nil -} - -// Errors surfaced by Poll when the token endpoint reports the user has -// explicitly declined (RFC 8628 §3.5's "access_denied") or the device code -// expired before the flow completed ("expired_token"). Poll otherwise -// blocks internally on "authorization_pending"/"slow_down" until one of -// these, success, or ctx's deadline (bounded by the device code's own -// expiry). -var ( - // ErrAccessDenied indicates the user explicitly declined the request. - ErrAccessDenied = errors.New("auth0: access denied") - // ErrExpiredToken indicates the device code expired before the user - // completed the flow. - ErrExpiredToken = errors.New("auth0: device code expired") - // ErrInvalidGrant indicates the supplied refresh token is expired, - // revoked, or otherwise no longer valid (Auth0's "invalid_grant" - // error from the refresh_token grant). Callers should treat this as - // requiring the user to run `lfx auth login` again. - ErrInvalidGrant = errors.New("auth0: invalid or expired refresh token") -) - -// Poll blocks until the user completes (or rejects) the device flow, -// polling the token endpoint at the interval Auth0 specified (respecting -// "slow_down" backoff) via oauth2.Config.DeviceAccessToken. It returns -// ErrAccessDenied or ErrExpiredToken for those respective outcomes. -func (dc *DeviceCode) Poll() (*oauth2.Token, error) { - tok, err := dc.cfg.DeviceAccessToken(dc.ctx, dc.DeviceAuthResponse) - if err == nil { - return tok, nil - } - - var retrieveErr *oauth2.RetrieveError - if errors.As(err, &retrieveErr) { - switch retrieveErr.ErrorCode { - case "access_denied": - return nil, ErrAccessDenied - case "expired_token": - return nil, ErrExpiredToken - } - } - - // DeviceAccessToken derives its own polling deadline from the device - // code's Expiry and returns the bare context.DeadlineExceeded (not a - // *oauth2.RetrieveError) once that deadline passes, rather than - // waiting for the token endpoint to report "expired_token" itself. - // dc.ctx may also carry a caller-supplied deadline of its own (e.g. if - // RequestDeviceCode was called with one), so only treat - // DeadlineExceeded as an expired device code if dc.ctx itself isn't - // what expired -- otherwise this would misreport an unrelated caller - // timeout as ErrExpiredToken. - if errors.Is(err, context.DeadlineExceeded) && dc.ctx.Err() == nil { - return nil, ErrExpiredToken - } - - return nil, fmt.Errorf("auth0: poll for token: %w", err) -} - -// RefreshToken exchanges a refresh token for a new access token. -func (c *Client) RefreshToken(ctx context.Context, refreshToken string) (*oauth2.Token, error) { - cfg := c.config(nil) - tok, err := cfg.TokenSource(c.context(ctx), &oauth2.Token{RefreshToken: refreshToken}).Token() - if err == nil { - return tok, nil - } - - var retrieveErr *oauth2.RetrieveError - if errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "invalid_grant" { - return nil, ErrInvalidGrant - } - return nil, fmt.Errorf("auth0: refresh token: %w", err) -} diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 6f2e225..e2172bd 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -18,9 +18,9 @@ import ( "strings" "time" - "github.com/linuxfoundation/lfx-cli/internal/auth0" "github.com/linuxfoundation/lfx-cli/internal/credstore" "github.com/urfave/cli/v3" + "golang.org/x/oauth2" ) // insecureStorageFlagName is the auth command group's flag controlling @@ -91,12 +91,12 @@ func newAuthLoginCommand() *cli.Command { &cli.StringFlag{ Name: envFlagName, Usage: "Target environment: prod, staging, or development", - Value: string(auth0.EnvProd), + Value: string(envProd), }, &cli.StringFlag{ Name: audienceFlagName, Usage: "Auth0 API audience to request tokens for (independent of --env)", - Value: auth0.DefaultAudience, + Value: defaultAudience, }, }, Action: runAuthLogin, @@ -109,8 +109,8 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { return err } - env := auth0.Environment(cmd.String(envFlagName)) - domain, clientID, err := auth0.Resolve(env) + env := authEnvironment(cmd.String(envFlagName)) + domain, clientID, err := resolveEnvironment(env) if err != nil { return err } @@ -121,8 +121,7 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { return loginWithToken(store, env, domain, audience, insecure) } - client := &auth0.Client{Domain: domain, ClientID: clientID} - return loginWithDeviceCode(ctx, cmd, client, store, env, domain, audience, insecure) + return loginWithDeviceCode(ctx, cmd, domain, clientID, store, env, audience, insecure) } // loginWithToken implements `--with-token`: it reads a refresh token from @@ -130,7 +129,7 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { // `echo "$REFRESH_TOKEN" | lfx auth login --with-token`. No access token is // cached; the next `lfx auth token` call exchanges the refresh token for // one. -func loginWithToken(store credstore.Store, env auth0.Environment, domain, audience string, insecure bool) error { +func loginWithToken(store credstore.Store, env authEnvironment, domain, audience string, insecure bool) error { reader := bufio.NewReader(os.Stdin) line, err := reader.ReadString('\n') if err != nil && !errors.Is(err, io.EOF) { @@ -160,24 +159,37 @@ func loginWithToken(store credstore.Store, env auth0.Environment, domain, audien func loginWithDeviceCode( ctx context.Context, cmd *cli.Command, - client *auth0.Client, + domain, clientID string, store credstore.Store, - env auth0.Environment, - domain, audience string, + env authEnvironment, + audience string, insecure bool, ) error { - dc, err := client.RequestDeviceCode(ctx, audience, loginScopes) + cfg := &oauth2.Config{ + ClientID: clientID, + Scopes: loginScopes, + Endpoint: oauth2.Endpoint{ + DeviceAuthURL: "https://" + domain + "/oauth/device/code", + TokenURL: "https://" + domain + "/oauth/token", + AuthStyle: oauth2.AuthStyleInParams, + }, + } + var opts []oauth2.AuthCodeOption + if audience != "" { + opts = append(opts, oauth2.SetAuthURLParam("audience", audience)) + } + resp, err := cfg.DeviceAuth(ctx, opts...) if err != nil { - return err + return fmt.Errorf("request device code: %w", err) } - fmt.Printf("First copy your one-time code: %s\n", dc.UserCode) + fmt.Printf("First copy your one-time code: %s\n", resp.UserCode) // VerificationURIComplete is optional per RFC 8628 §3.2; fall back to // VerificationURI (always present) plus the user code if the IdP // doesn't supply it. - verificationURI := dc.VerificationURIComplete + verificationURI := resp.VerificationURIComplete if verificationURI == "" { - verificationURI = dc.VerificationURI + verificationURI = resp.VerificationURI } if cmd.Bool(webFlagName) { fmt.Printf("Opening %s in your browser...\n", verificationURI) @@ -190,18 +202,40 @@ func loginWithDeviceCode( } fmt.Println("Waiting for authentication...") - // Poll blocks internally until success or failure, honoring the - // device code's own expiry and Auth0's requested polling interval - // (including "slow_down" backoff). - token, err := dc.Poll() - switch { - case err == nil: - case errors.Is(err, auth0.ErrAccessDenied): - return errors.New("login was denied") - case errors.Is(err, auth0.ErrExpiredToken): - return errors.New("device code expired before login completed") - default: - return err + // DeviceAccessToken blocks until the user completes or rejects the + // flow, polling at the interval Auth0 specified (honoring "slow_down" + // backoff), bounded by the device code's own expiry. + token, err := cfg.DeviceAccessToken(ctx, resp) + if err != nil { + // DeviceAccessToken derives its polling deadline from the device + // code's expiry (RFC 8628 "expires_in") and, once that elapses, + // returns a bare context.DeadlineExceeded instead of waiting for + // one more poll where the server would otherwise answer + // error=expired_token itself. Both mean the same thing -- the + // device code's verification window ran out, not a token or + // network timeout -- so both map to the same message. + var retrieveErr *oauth2.RetrieveError + switch { + case errors.Is(err, context.DeadlineExceeded): + return errors.New("timed out waiting for login") + case errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "expired_token": + return errors.New("timed out waiting for login") + case errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "access_denied": + return errors.New("login was denied") + default: + return fmt.Errorf("unexpected error waiting for login: %w", err) + } + } + + // offline_access was requested (see loginScopes), but Auth0 does not + // guarantee a refresh_token is issued -- e.g. a custom API/audience + // can have offline access disabled server-side regardless of the + // scopes requested. Persisting an empty refresh token here would let + // `lfx auth login` report success for a session that silently stops + // working (or worse, is treated as logged-in with no way to refresh) + // as soon as the cached access token expires, so fail loudly instead. + if token.RefreshToken == "" { + return errors.New("login did not receive a refresh token; offline access may be disabled for this API") } if err := persistLogin( @@ -235,14 +269,15 @@ func loginWithDeviceCode( // last wrote state.json -- trusting it here would silently mix a // refresh token from one backend with IdP/environment metadata written // for the other. -// - the IdP domain implied by state.Environment (via auth0.Resolve, the -// source of truth) must match the persisted state.IDPDomain, guarding -// against a tampered or corrupted state.json redirecting a refresh -// request -- and its long-lived refresh token -- to another host. +// - the IdP domain implied by state.Environment (via resolveEnvironment, +// the source of truth) must match the persisted state.IDPDomain, +// guarding against a tampered or corrupted state.json redirecting a +// refresh request -- and its long-lived refresh token -- to another +// host. // // On success it returns the state along with the trusted domain and client -// ID to use for any Auth0 request (always auth0.Resolve's values, never the -// persisted ones). +// ID to use for any Auth0 request (always resolveEnvironment's values, +// never the persisted ones). func loadDeviceStateForBackend(store credstore.Store, cmd *cli.Command) (state credstore.DeviceState, domain, clientID string, err error) { state, err = store.LoadDeviceState() if err != nil { @@ -256,7 +291,7 @@ func loadDeviceStateForBackend(store credstore.Store, cmd *cli.Command) (state c ) } - domain, clientID, err = auth0.Resolve(auth0.Environment(state.Environment)) + domain, clientID, err = resolveEnvironment(authEnvironment(state.Environment)) if err != nil { return credstore.DeviceState{}, "", "", err } @@ -288,11 +323,22 @@ func insecureStorageUsageHint(insecure bool) string { return "no --insecure-storage" } -// persistLogin saves creds and state together. The two writes are not -// atomic, so if SaveDeviceState fails after SaveCredentials succeeds, the -// newly saved credentials are rolled back (deleted) rather than left paired +// persistLogin saves creds and state as a pair. The two writes are not +// atomic: if SaveDeviceState fails after SaveCredentials succeeds, the +// just-saved credentials are rolled back (deleted) rather than left paired // with stale or missing environment metadata, which could otherwise send a // later refresh to the wrong Auth0 tenant. +// +// This rollback is a delete, not a restore: on a re-login (the user was +// already authenticated), SaveCredentials has already overwritten the +// prior working credentials before SaveDeviceState is attempted, so a +// SaveDeviceState failure here logs the user out rather than leaving the +// previous, still-valid session in place. This is considered acceptable: +// the failure mode requires state.json's write to fail (e.g. disk full or +// permissions) immediately after a credentials write succeeded, and the +// fix -- snapshotting and restoring prior credentials instead of deleting +// -- adds meaningful complexity for a narrow, easily-recovered case (rerun +// `lfx auth login`). func persistLogin(store credstore.Store, creds credstore.Credentials, state credstore.DeviceState) error { if err := store.SaveCredentials(creds); err != nil { return fmt.Errorf("save credentials: %w", err) @@ -351,10 +397,18 @@ func newAuthTokenCommand() *cli.Command { if err != nil { return fmt.Errorf("load device state: %w", err) } - client := &auth0.Client{Domain: domain, ClientID: clientID} + cfg := &oauth2.Config{ + ClientID: clientID, + Endpoint: oauth2.Endpoint{ + DeviceAuthURL: "https://" + domain + "/oauth/device/code", + TokenURL: "https://" + domain + "/oauth/token", + AuthStyle: oauth2.AuthStyleInParams, + }, + } - token, err := client.RefreshToken(ctx, creds.RefreshToken) - if errors.Is(err, auth0.ErrInvalidGrant) { + token, err := cfg.TokenSource(ctx, &oauth2.Token{RefreshToken: creds.RefreshToken}).Token() + var retrieveErr *oauth2.RetrieveError + if errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "invalid_grant" { return errors.New("session expired or revoked; run `lfx auth login` to log in again") } if err != nil { diff --git a/internal/commands/auth_test.go b/internal/commands/auth_test.go new file mode 100644 index 0000000..aae9b68 --- /dev/null +++ b/internal/commands/auth_test.go @@ -0,0 +1,236 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +package commands + +import ( + "context" + "encoding/base64" + "encoding/json" + "errors" + "testing" + + "github.com/linuxfoundation/lfx-cli/internal/credstore" + "github.com/urfave/cli/v3" +) + +// newTestCommand builds a *cli.Command with the --insecure-storage flag +// registered (as newAuthLoginCommand and friends do), parses args against +// it, and returns the parsed *cli.Command handed to fn's Action so tests +// can read flag values the way the real commands do. +func newTestCommand(t *testing.T, args []string, fn func(cmd *cli.Command)) { + t.Helper() + cmd := &cli.Command{ + Name: "test", + Flags: []cli.Flag{ + &cli.BoolFlag{Name: insecureStorageFlagName}, + }, + Action: func(_ context.Context, cmd *cli.Command) error { + fn(cmd) + return nil + }, + } + if err := cmd.Run(context.Background(), append([]string{"test"}, args...)); err != nil { + t.Fatalf("cmd.Run: %v", err) + } +} + +func newInsecureStore(t *testing.T) credstore.Store { + t.Helper() + store, err := credstore.New(credstore.Options{Insecure: true, StateDir: t.TempDir()}) + if err != nil { + t.Fatalf("credstore.New: %v", err) + } + return store +} + +func TestPersistLoginSuccess(t *testing.T) { + store := newInsecureStore(t) + + creds := credstore.Credentials{RefreshToken: "refresh-token"} + state := credstore.DeviceState{IDPDomain: "linuxfoundation-dev.auth0.com", Environment: "development", Insecure: true} + + if err := persistLogin(store, creds, state); err != nil { + t.Fatalf("persistLogin: %v", err) + } + + gotCreds, err := store.LoadCredentials() + if err != nil { + t.Fatalf("LoadCredentials: %v", err) + } + if gotCreds != creds { + t.Errorf("LoadCredentials = %+v, want %+v", gotCreds, creds) + } + + gotState, err := store.LoadDeviceState() + if err != nil { + t.Fatalf("LoadDeviceState: %v", err) + } + if gotState != state { + t.Errorf("LoadDeviceState = %+v, want %+v", gotState, state) + } +} + +// failingDeviceStateStore wraps a real Store but always fails +// SaveDeviceState, to exercise persistLogin's rollback path without a real +// disk failure. +type failingDeviceStateStore struct { + credstore.Store +} + +func (f failingDeviceStateStore) SaveDeviceState(credstore.DeviceState) error { + return errors.New("simulated state write failure") +} + +func TestPersistLoginRollsBackCredentialsOnStateFailure(t *testing.T) { + store := failingDeviceStateStore{Store: newInsecureStore(t)} + + err := persistLogin(store, credstore.Credentials{RefreshToken: "refresh-token"}, credstore.DeviceState{}) + if err == nil { + t.Fatal("persistLogin: got nil error, want failure from SaveDeviceState") + } + + // The rollback (documented on persistLogin) deletes the just-saved + // credentials rather than leaving them orphaned without matching + // device state. + if _, err := store.LoadCredentials(); !errors.Is(err, credstore.ErrNotFound) { + t.Fatalf("LoadCredentials after rollback: got err %v, want ErrNotFound", err) + } +} + +func TestLoadDeviceStateForBackendBackendMismatch(t *testing.T) { + store := newInsecureStore(t) + if err := store.SaveDeviceState(credstore.DeviceState{ + IDPDomain: "linuxfoundation-dev.auth0.com", + Environment: "development", + Insecure: true, + }); err != nil { + t.Fatalf("SaveDeviceState: %v", err) + } + + // Parse a command *without* --insecure-storage, so its flag value (false) + // disagrees with the persisted state's Insecure: true. + newTestCommand(t, nil, func(cmd *cli.Command) { + if _, _, _, err := loadDeviceStateForBackend(store, cmd); err == nil { + t.Fatal("loadDeviceStateForBackend: got nil error, want backend-mismatch error") + } + }) +} + +func TestLoadDeviceStateForBackendDomainMismatch(t *testing.T) { + store := newInsecureStore(t) + if err := store.SaveDeviceState(credstore.DeviceState{ + // Environment resolves to a different domain than the one stored + // here, simulating a tampered or corrupted state.json. + IDPDomain: "tampered.example.com", + Environment: "development", + Insecure: true, + }); err != nil { + t.Fatalf("SaveDeviceState: %v", err) + } + + newTestCommand(t, []string{"--insecure-storage"}, func(cmd *cli.Command) { + if _, _, _, err := loadDeviceStateForBackend(store, cmd); err == nil { + t.Fatal("loadDeviceStateForBackend: got nil error, want IdP domain mismatch error") + } + }) +} + +func TestLoadDeviceStateForBackendOK(t *testing.T) { + store := newInsecureStore(t) + if err := store.SaveDeviceState(credstore.DeviceState{ + IDPDomain: "linuxfoundation-dev.auth0.com", + Environment: "development", + Insecure: true, + }); err != nil { + t.Fatalf("SaveDeviceState: %v", err) + } + + newTestCommand(t, []string{"--insecure-storage"}, func(cmd *cli.Command) { + _, domain, clientID, err := loadDeviceStateForBackend(store, cmd) + if err != nil { + t.Fatalf("loadDeviceStateForBackend: %v", err) + } + if domain != "linuxfoundation-dev.auth0.com" { + t.Errorf("domain = %q, want linuxfoundation-dev.auth0.com", domain) + } + if clientID == "" { + t.Error("clientID = \"\", want a non-empty compiled-in client ID") + } + }) +} + +// fakeIDToken builds an unsigned JWT with the given claims as its payload, +// matching the shape identityFromIDToken decodes (header/payload/signature +// separated by ".", payload base64url-encoded JSON). The header and +// signature segments are never inspected, so their content is arbitrary. +func fakeIDToken(t *testing.T, claims map[string]any) string { + t.Helper() + payload, err := json.Marshal(claims) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + return "header." + base64.RawURLEncoding.EncodeToString(payload) + ".signature" +} + +func TestIdentityFromIDToken(t *testing.T) { + tests := []struct { + name string + token func(t *testing.T) string + claims map[string]any + want string + }{ + { + name: "username and email", + claims: map[string]any{lfidClaimsNamespace + "username": "jdoe", "email": "jdoe@example.com"}, + want: "jdoe (jdoe@example.com)", + }, + { + name: "username only", + claims: map[string]any{lfidClaimsNamespace + "username": "jdoe"}, + want: "jdoe", + }, + { + name: "email only, no username claim", + claims: map[string]any{"email": "jdoe@example.com"}, + want: "jdoe@example.com (no username)", + }, + { + name: "subject only", + claims: map[string]any{"sub": "auth0|abc123"}, + want: "auth0|abc123", + }, + { + name: "no usable claims", + claims: map[string]any{}, + want: "", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + token := fakeIDToken(t, tc.claims) + if got := identityFromIDToken(token); got != tc.want { + t.Errorf("identityFromIDToken(%q) = %q, want %q", token, got, tc.want) + } + }) + } + + t.Run("malformed token", func(t *testing.T) { + if got := identityFromIDToken("not-a-jwt"); got != "" { + t.Errorf("identityFromIDToken(malformed) = %q, want \"\"", got) + } + }) + + t.Run("invalid base64 payload", func(t *testing.T) { + if got := identityFromIDToken("header.not!base64url.signature"); got != "" { + t.Errorf("identityFromIDToken(invalid base64) = %q, want \"\"", got) + } + }) + + t.Run("payload not JSON", func(t *testing.T) { + token := "header." + base64.RawURLEncoding.EncodeToString([]byte("not json")) + ".signature" + if got := identityFromIDToken(token); got != "" { + t.Errorf("identityFromIDToken(non-JSON payload) = %q, want \"\"", got) + } + }) +} diff --git a/internal/commands/environment.go b/internal/commands/environment.go new file mode 100644 index 0000000..04a4667 --- /dev/null +++ b/internal/commands/environment.go @@ -0,0 +1,68 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +// Package commands implements the lfx CLI subcommands. +package commands + +import ( + "errors" + "fmt" +) + +// authEnvironment identifies which LFX Auth0 tenant/IdP a login targets, +// selected via the `--env` flag. +type authEnvironment string + +// Supported environments. Each maps to a fixed IdP domain and a static, +// pre-provisioned Auth0 native application client ID (device_code + +// refresh_token grants). CIMD was evaluated and abandoned for this flow: +// Auth0 silently drops CIMD client registration for the device_code grant. +const ( + envProd authEnvironment = "prod" + envStaging authEnvironment = "staging" + envDevelopment authEnvironment = "development" +) + +// authDomains maps each authEnvironment to the Auth0 IdP domain end users +// authenticate against, matching auth0-terraform's own `auth0_domain` +// variable. This is deliberately not each tenant's *.auth0.com domain: prod +// fronts its tenant with the custom domain sso.linuxfoundation.org, and +// since this CLI never calls the Auth0 Management API (only the device +// code and token endpoints), there's no need to separately track the +// underlying tenant name. +var authDomains = map[authEnvironment]string{ + envProd: "sso.linuxfoundation.org", + envStaging: "linuxfoundation-staging.auth0.com", + envDevelopment: "linuxfoundation-dev.auth0.com", +} + +// authClientIDs maps each authEnvironment to its compiled-in, +// pre-provisioned Auth0 native application client ID for the device code +// grant. +// cspell:disable -- opaque, randomly-generated Auth0 client IDs, not words. +var authClientIDs = map[authEnvironment]string{ + envProd: "kkCpM0c9zJ0vNZZDDOGqcyzocOBircOn", + envStaging: "9XzXgDfAB9O7IoHqhBj5mg4VLvdBM8ci", + envDevelopment: "0TN1OElqQY146vLEPdV5qfejRKpc9IAZ", +} + +// cspell:enable + +// defaultAudience is the production LFX v2 API audience used unless +// overridden via `--audience`. It intentionally does not vary with `--env`: +// the audience is independent of the selected environment and must be set +// explicitly when testing a non-prod API. +const defaultAudience = "https://lfx-api.v2.cluster.lfx.dev/" + +// errInvalidEnvironment is returned by resolveEnvironment for an +// unrecognized authEnvironment value. +var errInvalidEnvironment = errors.New("invalid environment") + +// resolveEnvironment returns the IdP domain and client ID for env. +func resolveEnvironment(env authEnvironment) (domain, clientID string, err error) { + domain, ok := authDomains[env] + if !ok { + return "", "", fmt.Errorf("%w: %q (must be one of prod, staging, development)", errInvalidEnvironment, env) + } + return domain, authClientIDs[env], nil +} diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index ad8cf4d..0408bd8 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -23,9 +23,9 @@ // storage. // // Non-sensitive state (environment, IdP domain, and audience used at login) -// is always stored -// as plain JSON under the XDG state directory (~/.local/state/lfx-cli/ by -// default), per XDG Base Directory conventions for mutable runtime state. +// is always stored as plain JSON under the XDG state directory +// (~/.local/state/lfx-cli/ by default), per XDG Base Directory conventions +// for mutable runtime state. package credstore import ( diff --git a/internal/credstore/credstore_test.go b/internal/credstore/credstore_test.go new file mode 100644 index 0000000..ff0a1cc --- /dev/null +++ b/internal/credstore/credstore_test.go @@ -0,0 +1,133 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +package credstore + +import ( + "errors" + "testing" + "time" +) + +// newTestStore builds a Store rooted at a temporary directory using the +// insecure (plain-file) backend, so tests never touch the real OS +// keychain. +func newTestStore(t *testing.T) Store { + t.Helper() + store, err := New(Options{Insecure: true, StateDir: t.TempDir()}) + if err != nil { + t.Fatalf("New: %v", err) + } + return store +} + +func TestCredentialsRoundTrip(t *testing.T) { + store := newTestStore(t) + + if _, err := store.LoadCredentials(); !errors.Is(err, ErrNotFound) { + t.Fatalf("LoadCredentials before save: got err %v, want ErrNotFound", err) + } + + want := Credentials{ + RefreshToken: "refresh-token", + AccessToken: "access-token", + AccessTokenExpiry: time.Now().Add(time.Hour).Truncate(time.Second), + } + if err := store.SaveCredentials(want); err != nil { + t.Fatalf("SaveCredentials: %v", err) + } + + got, err := store.LoadCredentials() + if err != nil { + t.Fatalf("LoadCredentials: %v", err) + } + if got != want { + t.Fatalf("LoadCredentials = %+v, want %+v", got, want) + } + + if err := store.DeleteCredentials(); err != nil { + t.Fatalf("DeleteCredentials: %v", err) + } + if _, err := store.LoadCredentials(); !errors.Is(err, ErrNotFound) { + t.Fatalf("LoadCredentials after delete: got err %v, want ErrNotFound", err) + } + + // DeleteCredentials is documented as a no-op when nothing is stored. + if err := store.DeleteCredentials(); err != nil { + t.Fatalf("DeleteCredentials on empty store: %v", err) + } +} + +func TestDeviceStateRoundTrip(t *testing.T) { + store := newTestStore(t) + + if _, err := store.LoadDeviceState(); !errors.Is(err, ErrNotFound) { + t.Fatalf("LoadDeviceState before save: got err %v, want ErrNotFound", err) + } + + want := DeviceState{ + IDPDomain: "linuxfoundation-dev.auth0.com", + Environment: "development", + Audience: "https://example.test/", + Insecure: true, + } + if err := store.SaveDeviceState(want); err != nil { + t.Fatalf("SaveDeviceState: %v", err) + } + + got, err := store.LoadDeviceState() + if err != nil { + t.Fatalf("LoadDeviceState: %v", err) + } + if got != want { + t.Fatalf("LoadDeviceState = %+v, want %+v", got, want) + } + + if err := store.DeleteDeviceState(); err != nil { + t.Fatalf("DeleteDeviceState: %v", err) + } + if _, err := store.LoadDeviceState(); !errors.Is(err, ErrNotFound) { + t.Fatalf("LoadDeviceState after delete: got err %v, want ErrNotFound", err) + } + + // DeleteDeviceState is documented as a no-op when nothing is stored. + if err := store.DeleteDeviceState(); err != nil { + t.Fatalf("DeleteDeviceState on empty store: %v", err) + } +} + +func TestValidAccessToken(t *testing.T) { + tests := []struct { + name string + creds Credentials + want bool + }{ + { + name: "no access token", + creds: Credentials{AccessTokenExpiry: time.Now().Add(time.Hour)}, + want: false, + }, + { + name: "expired", + creds: Credentials{AccessToken: "tok", AccessTokenExpiry: time.Now().Add(-time.Minute)}, + want: false, + }, + { + name: "within clock-skew buffer of expiry", + creds: Credentials{AccessToken: "tok", AccessTokenExpiry: time.Now().Add(10 * time.Second)}, + want: false, + }, + { + name: "valid", + creds: Credentials{AccessToken: "tok", AccessTokenExpiry: time.Now().Add(time.Hour)}, + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.creds.ValidAccessToken(); got != tc.want { + t.Errorf("ValidAccessToken() = %v, want %v", got, tc.want) + } + }) + } +} From cb34d6bc5246b945f4c78f9d91f29271654b8b50 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 15:11:32 -0700 Subject: [PATCH 11/17] Clarify AGENTS.md: package comments not required in _test.go files Verified directly against revive's default rules (both ./... and per-file list mode, matching MegaLinter's GO_REVIVE_CLI_LINT_MODE: list_of_files): the package-comments rule never flags *_test.go files, so they don't need the duplicated // Package doc comment other non-test files in a package require. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- AGENTS.md | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c82ec77..ca03943 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -146,16 +146,17 @@ func NewExampleCommand() *cli.Command { ### Package Comments -Every file in a package must start with the same `// Package ...` -doc comment immediately above the `package` declaration. Revive's -`package-comments` rule itself only requires one such comment per package, -but MegaLinter's `GO_REVIVE` linter defaults to `GO_REVIVE_CLI_LINT_MODE: -list_of_files`, invoking revive with a flat list of files instead of -`./...`. Under that mode revive loses per-package grouping and flags any -file lacking the comment, so duplicating the identical comment across -every file in a package is a required workaround for how MegaLinter calls -revive here, not an inherent revive requirement. Do not vary the wording -between files in the same package. +Every non-test (`*.go`, not `*_test.go`) file in a package must start with +the same `// Package ...` doc comment immediately above the +`package` declaration. Revive's `package-comments` rule itself only +requires one such comment per package, but MegaLinter's `GO_REVIVE` linter +defaults to `GO_REVIVE_CLI_LINT_MODE: list_of_files`, invoking revive with +a flat list of files instead of `./...`. Under that mode revive loses +per-package grouping and flags any non-test file lacking the comment, so +duplicating the identical comment across every non-test file in a package +is a required workaround for how MegaLinter calls revive here, not an +inherent revive requirement. Do not vary the wording between files in the +same package. ## Documentation Generation @@ -226,8 +227,9 @@ release binaries may be missing even though the GitHub Release exists. 1. **Add Commands**: Create new commands in `internal/commands/` following the established pattern -2. **Package Comments**: Every new `*.go` file must include the same - `// Package ...` doc comment as the rest of its package +2. **Package Comments**: Every new non-test `*.go` file must include the + same `// Package ...` doc comment as the rest of its package + (see "Package Comments" above; `*_test.go` files are exempt) 3. **Dependencies**: Run `go get -u ./... && go mod tidy` before every PR to keep dependencies current. This upgrades module dependencies only, not the Go toolchain itself (`go.mod`'s `go` directive) -- see the toolchain policy From ad86e5515584af97b61ef3039f4fb4fc7bab53ff Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 15:19:15 -0700 Subject: [PATCH 12/17] Fix flaky TestCredentialsRoundTrip under UTC (e.g. CI) Comparing Credentials via struct-level != breaks whenever AccessTokenExpiry's time.Time round-trips through JSON in an environment where time.Parse assigns it a different *Location than the original value, even though both represent the identical instant (time.Time.Equal returns true). This only reproduced under TZ=UTC (GitHub Actions' default), not the author's local PDT environment, which is why it passed locally but failed in CI. Compare AccessTokenExpiry with Equal instead of as part of the struct-level comparison. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/credstore/credstore_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/credstore/credstore_test.go b/internal/credstore/credstore_test.go index ff0a1cc..49634be 100644 --- a/internal/credstore/credstore_test.go +++ b/internal/credstore/credstore_test.go @@ -41,7 +41,11 @@ func TestCredentialsRoundTrip(t *testing.T) { if err != nil { t.Fatalf("LoadCredentials: %v", err) } - if got != want { + // AccessTokenExpiry round-trips through JSON, which can change its + // time.Time representation (e.g. Location) without changing the + // instant it represents, so compare it with Equal rather than as + // part of a struct-level != comparison. + if got.RefreshToken != want.RefreshToken || got.AccessToken != want.AccessToken || !got.AccessTokenExpiry.Equal(want.AccessTokenExpiry) { t.Fatalf("LoadCredentials = %+v, want %+v", got, want) } From 6eb01de32551a8e6d2f547432bcf587e7191641f Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 15:38:30 -0700 Subject: [PATCH 13/17] Pin credential storage to a specific keyring backend keyring.Open probes systemBackends in priority order and silently uses whichever one currently opens, so the same machine can land on a different backend across invocations (e.g. Secret Service reachable in one shell session but not another, falling back to pass). Since state.json is shared across all backends, that can silently pair a refresh token from one backend with IdP/environment metadata written by another. Add --backend (credstore.Options.Backend / DeviceState.Backend) to pin keyring.Open to a single backend. Once a login has pinned one, later commands must pass the same --backend value or fail loudly, mirroring the existing --insecure-storage mismatch guard rather than silently trusting stale state. An unpinned login (the previous, still-default behavior) can't be protected the same way: there's no recorded backend to check a later invocation against. This mirrors aws-vault's own approach: as the most established consumer of the same keyring library, it requires --backend/ AWS_VAULT_BACKEND to pin a backend explicitly rather than trusting keyring.Open's own auto-detection to be stable across invocations. Also: - Show the pinned backend (from persisted state, not just the current flag) as its own line in `lfx auth status`, and align all of its output labels to a common column. - Add a repo-level .jscpd.json excluding *_test.go from duplication scanning, needed once MegaLinter's own default (which excludes **/*.yaml, **/*.md, etc. but not *_test.go) is replaced by ours; otherwise structurally-similar-but-distinct negative-path test cases (e.g. the new backend-pin-mismatch test alongside the existing insecure-storage/domain-mismatch ones) trip jscpd's 0% duplication threshold. The existing shared test helpers (newTestCommand, newInsecureStore, fakeIDToken) already cover the reusable parts; forcing further abstraction of each test's short, intentionally-similar arrange/assert block would hurt readability for no real benefit. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- .jscpd.json | 22 ++++++++ README.md | 19 ++++--- internal/commands/auth.go | 89 +++++++++++++++++++++++++-------- internal/commands/auth_test.go | 56 +++++++++++++++++++-- internal/credstore/credstore.go | 67 +++++++++++++++++++++++-- 5 files changed, 218 insertions(+), 35 deletions(-) create mode 100644 .jscpd.json diff --git a/.jscpd.json b/.jscpd.json new file mode 100644 index 0000000..07ba83f --- /dev/null +++ b/.jscpd.json @@ -0,0 +1,22 @@ +{ + "threshold": 0, + "reporters": ["console"], + "ignore": [ + "**/.git/**", + "**/megalinter-reports/**", + "**/*cache*/**", + "**/*.json", + "**/*.yaml", + "**/*.yml", + "**/*.md", + "**/*.html", + "**/*.xml", + "**/*.jpg", + "**/*.png", + "**/*.svg", + "**/*.zip", + "**/*.bin", + "**/bin/**", + "**/*_test.go" + ] +} diff --git a/README.md b/README.md index 0f7f734..45cda85 100644 --- a/README.md +++ b/README.md @@ -44,16 +44,21 @@ lfx api Credentials (refresh token, cached access token) are stored in your operating system's credential store by default (macOS Keychain, Windows -Credential Manager, Linux Secret Service/KWallet/`pass`). Pass -`--insecure-storage` to any `auth` subcommand to instead store credentials -in a plain, unencrypted, owner-only file, at the cost of weaker protection -for the stored tokens. On Windows, this owner-only mode relies on inherited -directory permissions rather than a real ACL, since Go's `Chmod(0600)` maps -to the read-only attribute there rather than restricting access to the -current user. +Credential Manager, Linux Secret Service/KWallet/`pass`). Which of these is +actually used can vary between invocations on the same machine (e.g. Secret +Service reachable in one shell session but not another); pass +`--backend` to pin it to one explicitly (see `lfx auth backends` +for the available names). Once a login has pinned a backend, later commands +must pass the same `--backend` value. Pass `--insecure-storage` to +any `auth` subcommand to instead store credentials in a plain, unencrypted, +owner-only file, at the cost of weaker protection for the stored tokens. On +Windows, this owner-only mode relies on inherited directory permissions +rather than a real ACL, since Go's `Chmod(0600)` maps to the read-only +attribute there rather than restricting access to the current user. ```bash lfx auth login --insecure-storage +lfx auth login --backend=keychain ``` Run `lfx --help` or `lfx --help` for full details on any command. diff --git a/internal/commands/auth.go b/internal/commands/auth.go index e2172bd..6967ef7 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -28,6 +28,13 @@ import ( // unencrypted file. const insecureStorageFlagName = "insecure-storage" +// backendFlagName is the auth command group's flag pinning +// credential storage to a single system keyring backend (see +// `lfx auth backends`), instead of letting keyring.Open silently pick +// whichever backend currently opens. Ignored when --insecure-storage is +// set. +const backendFlagName = "backend" + // Flag names shared by the login command. const ( webFlagName = "web" @@ -42,11 +49,15 @@ var loginScopes = []string{"openid", "profile", "email", "offline_access"} // NewAuthCommand builds the `lfx auth` command group with its subcommands. // -// The --insecure-storage flag is shared by all subcommands (it is not -// declared as a "Local" flag, so urfave/cli resolves it for subcommand -// actions via cmd.Bool) and controls whether credentials bypass the system +// The --insecure-storage and --backend flags are shared by all +// subcommands (they are not declared as "Local" flags, so urfave/cli +// resolves them for subcommand actions via cmd.Bool/cmd.String). +// --insecure-storage controls whether credentials bypass the system // backend in favor of credstore's plain (unencrypted) file fallback, e.g. -// for headless/CI use. +// for headless/CI use. --backend pins credential storage to a +// single system backend rather than letting keyring.Open silently pick +// whichever one currently opens; see credstore.DeviceState.Backend for why +// that matters once a login has pinned one. func NewAuthCommand() *cli.Command { return &cli.Command{ Name: "auth", @@ -56,6 +67,10 @@ func NewAuthCommand() *cli.Command { Name: insecureStorageFlagName, Usage: "Store credentials in a plain (unencrypted) file instead of the system backend", }, + &cli.StringFlag{ + Name: backendFlagName, + Usage: "Pin credential storage to a specific system backend (see `lfx auth backends`); ignored with --insecure-storage", + }, }, Commands: []*cli.Command{ newAuthLoginCommand(), @@ -68,10 +83,15 @@ func NewAuthCommand() *cli.Command { } // credStoreFromCommand builds a credstore.Store using the group-level -// --insecure-storage flag, however deep in the `auth` subcommand tree cmd -// is. +// --insecure-storage and --backend flags, however deep in the +// `auth` subcommand tree cmd is. func credStoreFromCommand(cmd *cli.Command) (credstore.Store, error) { - return credstore.New(credstore.Options{Insecure: cmd.Bool(insecureStorageFlagName)}) + insecure := cmd.Bool(insecureStorageFlagName) + backend := cmd.String(backendFlagName) + if insecure && backend != "" { + return nil, fmt.Errorf("--%s cannot be combined with --%s", backendFlagName, insecureStorageFlagName) + } + return credstore.New(credstore.Options{Insecure: insecure, Backend: backend}) } func newAuthLoginCommand() *cli.Command { @@ -116,12 +136,13 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { } audience := cmd.String(audienceFlagName) insecure := cmd.Bool(insecureStorageFlagName) + backend := cmd.String(backendFlagName) if cmd.Bool(withTokenFlagName) { - return loginWithToken(store, env, domain, audience, insecure) + return loginWithToken(store, env, domain, audience, insecure, backend) } - return loginWithDeviceCode(ctx, cmd, domain, clientID, store, env, audience, insecure) + return loginWithDeviceCode(ctx, cmd, domain, clientID, store, env, audience, insecure, backend) } // loginWithToken implements `--with-token`: it reads a refresh token from @@ -129,7 +150,7 @@ func runAuthLogin(ctx context.Context, cmd *cli.Command) error { // `echo "$REFRESH_TOKEN" | lfx auth login --with-token`. No access token is // cached; the next `lfx auth token` call exchanges the refresh token for // one. -func loginWithToken(store credstore.Store, env authEnvironment, domain, audience string, insecure bool) error { +func loginWithToken(store credstore.Store, env authEnvironment, domain, audience string, insecure bool, backend string) error { reader := bufio.NewReader(os.Stdin) line, err := reader.ReadString('\n') if err != nil && !errors.Is(err, io.EOF) { @@ -143,7 +164,7 @@ func loginWithToken(store credstore.Store, env authEnvironment, domain, audience if err := persistLogin( store, credstore.Credentials{RefreshToken: refreshToken}, - credstore.DeviceState{IDPDomain: domain, Environment: string(env), Audience: audience, Insecure: insecure}, + credstore.DeviceState{IDPDomain: domain, Environment: string(env), Audience: audience, Insecure: insecure, Backend: backend}, ); err != nil { return err } @@ -164,6 +185,7 @@ func loginWithDeviceCode( env authEnvironment, audience string, insecure bool, + backend string, ) error { cfg := &oauth2.Config{ ClientID: clientID, @@ -245,7 +267,7 @@ func loginWithDeviceCode( AccessToken: token.AccessToken, AccessTokenExpiry: token.Expiry, }, - credstore.DeviceState{IDPDomain: domain, Environment: string(env), Audience: audience, Insecure: insecure}, + credstore.DeviceState{IDPDomain: domain, Environment: string(env), Audience: audience, Insecure: insecure, Backend: backend}, ); err != nil { return err } @@ -269,6 +291,12 @@ func loginWithDeviceCode( // last wrote state.json -- trusting it here would silently mix a // refresh token from one backend with IdP/environment metadata written // for the other. +// - if state.Backend was pinned (non-empty; see +// credstore.DeviceState.Backend), it must match cmd's --backend +// flag. Unlike Insecure, an unpinned state.Backend ("") can't be +// checked at all: keyring.Open can silently land on a different system +// backend across invocations, and once it does there's no recorded +// value to compare against. // - the IdP domain implied by state.Environment (via resolveEnvironment, // the source of truth) must match the persisted state.IDPDomain, // guarding against a tampered or corrupted state.json redirecting a @@ -290,6 +318,12 @@ func loadDeviceStateForBackend(store credstore.Store, cmd *cli.Command) (state c backendDescription(state.Insecure), insecureStorageUsageHint(state.Insecure), ) } + if state.Backend != "" && state.Backend != cmd.String(backendFlagName) { + return credstore.DeviceState{}, "", "", fmt.Errorf( + "stored login was pinned to keyring backend %q; pass --%s=%s to match, or run `lfx auth login --%s=%s` again", + state.Backend, backendFlagName, state.Backend, backendFlagName, state.Backend, + ) + } domain, clientID, err = resolveEnvironment(authEnvironment(state.Environment)) if err != nil { @@ -323,6 +357,18 @@ func insecureStorageUsageHint(insecure bool) string { return "no --insecure-storage" } +// stateMatchesInvocation reports whether state was written by an +// invocation using the same --insecure-storage and (if pinned) +// --backend as cmd, i.e. whether it's safe to trust or overwrite +// state.json for this invocation. See loadDeviceStateForBackend for the +// same checks used when actually consuming the state. +func stateMatchesInvocation(state credstore.DeviceState, cmd *cli.Command) bool { + if state.Insecure != cmd.Bool(insecureStorageFlagName) { + return false + } + return state.Backend == "" || state.Backend == cmd.String(backendFlagName) +} + // persistLogin saves creds and state as a pair. The two writes are not // atomic: if SaveDeviceState fails after SaveCredentials succeeds, the // just-saved credentials are rolled back (deleted) rather than left paired @@ -460,7 +506,7 @@ func newAuthStatusCommand() *cli.Command { } fmt.Println("Logged in.") - if err == nil && state.Insecure != cmd.Bool(insecureStorageFlagName) { + if err == nil && !stateMatchesInvocation(state, cmd) { fmt.Printf( " Note: stored login state below belongs to %s, not this credential backend; "+ "it may not describe these credentials. Run `lfx auth login` to refresh it.\n", @@ -468,19 +514,22 @@ func newAuthStatusCommand() *cli.Command { ) } if state.Environment != "" { - fmt.Printf(" Environment: %s\n", state.Environment) + fmt.Printf(" %-22s %s\n", "Environment:", state.Environment) } if state.IDPDomain != "" { - fmt.Printf(" IdP domain: %s\n", state.IDPDomain) + fmt.Printf(" %-22s %s\n", "IdP domain:", state.IDPDomain) } if state.Audience != "" { - fmt.Printf(" Audience: %s\n", state.Audience) + fmt.Printf(" %-22s %s\n", "Audience:", state.Audience) + } + fmt.Printf(" %-22s %s\n", "Credential backend:", backend) + if state.Backend != "" { + fmt.Printf(" %-22s %s\n", "Pinned backend:", state.Backend) } - fmt.Printf(" Credential backend: %s\n", backend) if creds.ValidAccessToken() { - fmt.Printf(" Access token expires: %s\n", creds.AccessTokenExpiry.Format(time.RFC3339)) + fmt.Printf(" %-22s %s\n", "Access token expires:", creds.AccessTokenExpiry.Format(time.RFC3339)) } else { - fmt.Println(" Access token: expired or not cached (will refresh on next `lfx auth token`)") + fmt.Printf(" %-22s %s\n", "Access token:", "expired or not cached (will refresh on next `lfx auth token`)") } return nil @@ -513,7 +562,7 @@ func newAuthLogoutCommand() *cli.Command { // Nothing to delete. case err != nil: return fmt.Errorf("load device state: %w", err) - case state.Insecure == cmd.Bool(insecureStorageFlagName): + case stateMatchesInvocation(state, cmd): if err := store.DeleteDeviceState(); err != nil { return fmt.Errorf("delete device state: %w", err) } diff --git a/internal/commands/auth_test.go b/internal/commands/auth_test.go index aae9b68..1f779c1 100644 --- a/internal/commands/auth_test.go +++ b/internal/commands/auth_test.go @@ -14,16 +14,18 @@ import ( "github.com/urfave/cli/v3" ) -// newTestCommand builds a *cli.Command with the --insecure-storage flag -// registered (as newAuthLoginCommand and friends do), parses args against -// it, and returns the parsed *cli.Command handed to fn's Action so tests -// can read flag values the way the real commands do. +// newTestCommand builds a *cli.Command with the --insecure-storage and +// --backend flags registered (as newAuthLoginCommand and friends +// do), parses args against it, and returns the parsed *cli.Command handed +// to fn's Action so tests can read flag values the way the real commands +// do. func newTestCommand(t *testing.T, args []string, fn func(cmd *cli.Command)) { t.Helper() cmd := &cli.Command{ Name: "test", Flags: []cli.Flag{ &cli.BoolFlag{Name: insecureStorageFlagName}, + &cli.StringFlag{Name: backendFlagName}, }, Action: func(_ context.Context, cmd *cli.Command) error { fn(cmd) @@ -136,6 +138,52 @@ func TestLoadDeviceStateForBackendDomainMismatch(t *testing.T) { }) } +func TestLoadDeviceStateForBackendKeyringBackendMismatch(t *testing.T) { + store := newInsecureStore(t) + if err := store.SaveDeviceState(credstore.DeviceState{ + IDPDomain: "linuxfoundation-dev.auth0.com", + Environment: "development", + Insecure: true, + Backend: "keychain", + }); err != nil { + t.Fatalf("SaveDeviceState: %v", err) + } + + // A pinned state.Backend ("keychain") must match --backend on + // every later command; omitting the flag entirely disagrees with it. + newTestCommand(t, []string{"--insecure-storage"}, func(cmd *cli.Command) { + if _, _, _, err := loadDeviceStateForBackend(store, cmd); err == nil { + t.Fatal("loadDeviceStateForBackend: got nil error, want backend mismatch error") + } + }) + + // A different pinned value also disagrees. + newTestCommand(t, []string{"--insecure-storage", "--" + backendFlagName + "=pass"}, func(cmd *cli.Command) { + if _, _, _, err := loadDeviceStateForBackend(store, cmd); err == nil { + t.Fatal("loadDeviceStateForBackend: got nil error, want backend mismatch error") + } + }) +} + +func TestLoadDeviceStateForBackendUnpinnedAllowsAnyKeyringBackend(t *testing.T) { + store := newInsecureStore(t) + if err := store.SaveDeviceState(credstore.DeviceState{ + IDPDomain: "linuxfoundation-dev.auth0.com", + Environment: "development", + Insecure: true, + // Backend intentionally left unset: an unpinned login can't be + // checked against --backend at all. + }); err != nil { + t.Fatalf("SaveDeviceState: %v", err) + } + + newTestCommand(t, []string{"--insecure-storage", "--" + backendFlagName + "=keychain"}, func(cmd *cli.Command) { + if _, _, _, err := loadDeviceStateForBackend(store, cmd); err != nil { + t.Fatalf("loadDeviceStateForBackend: %v, want nil (unpinned state.Backend can't be checked)", err) + } + }) +} + func TestLoadDeviceStateForBackendOK(t *testing.T) { store := newInsecureStore(t) if err := store.SaveDeviceState(credstore.DeviceState{ diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 0408bd8..1b1e447 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -7,9 +7,13 @@ // Secrets (refresh token, cached access token and its expiry) are stored in // the operating system's credential store via github.com/99designs/keyring: // macOS Keychain, Windows Credential Manager, Linux Secret Service/KWallet, -// or the `pass` password store. keyring's own encrypted-file backend is -// deliberately excluded from this list: it isn't a real system keychain, and -// its Remove behavior doesn't match keyring.ErrKeyNotFound (see +// or the `pass` password store. Which of these keyring.Open actually +// selects can vary between invocations on the same machine (e.g. Secret +// Service reachable in one shell session but not another); --backend +// pins it to one, and once pinned, the DeviceState.Backend it was pinned to +// must match on every later command. keyring's own encrypted-file backend +// is deliberately excluded from this list: it isn't a real system keychain, +// and its Remove behavior doesn't match keyring.ErrKeyNotFound (see // keyringSecrets.Delete). If none of the allowed backends are available, // New returns an error rather than silently falling back to a file. // @@ -34,6 +38,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/99designs/keyring" @@ -123,6 +128,29 @@ func AvailableBackends() []Backend { return backends } +// isSystemBackend reports whether bt is one of the backends New is willing +// to select at all (see systemBackends). +func isSystemBackend(bt keyring.BackendType) bool { + for _, b := range systemBackends { + if b == bt { + return true + } + } + return false +} + +// backendNames renders systemBackends' keyring.BackendType identifiers +// (e.g. "keychain") for use in error messages about an invalid --keyring- +// backend value; these are the same strings AvailableBackends reports as +// Backend.Name. +func backendNames() []string { + names := make([]string, len(systemBackends)) + for i, b := range systemBackends { + names[i] = string(b) + } + return names +} + // Credentials holds the secrets needed to authenticate with the LFX // platform: the long-lived Auth0 refresh token, and an optional cached // access token with its expiry. @@ -174,6 +202,18 @@ type DeviceState struct { // silently overwrites the metadata (env, IdP domain) that the other // backend's still-present credentials depend on. Insecure bool `json:"insecure,omitempty"` + // Backend records the keyring.BackendType (e.g. "keychain") pinned via + // --backend at login, or "" if the backend was left to + // keyring.Open's own auto-detection. Unlike Insecure, an empty value + // here is not itself trustworthy: keyring.Open can silently select a + // *different* system backend across invocations (e.g. Secret Service + // is reachable in one shell session but not another, falling back to + // pass), so an unpinned login can't be protected against later landing + // on a different backend with the same state.json. Once a backend has + // been pinned, though, callers must require the same --backend + // value on every later command against this state, the same way + // Insecure is enforced. + Backend string `json:"backend,omitempty"` } // Store is the credential storage abstraction used by the auth commands. @@ -211,6 +251,13 @@ type Options struct { // leave empty to use $XDG_STATE_HOME/lfx-cli (or ~/.local/state/lfx-cli // if $XDG_STATE_HOME is unset). StateDir string + + // Backend pins keyring.Open to a single system backend (e.g. + // "keychain"; see AvailableBackends for the valid values on this OS), + // instead of letting it probe systemBackends in priority order and + // silently use whichever one currently opens. Ignored when Insecure is + // set. Leave empty to keep the previous auto-detecting behavior. + Backend string } // secretsBackend abstracts over the two ways Credentials can be persisted: @@ -242,6 +289,18 @@ func New(opts Options) (Store, error) { path: filepath.Join(stateDir, insecureCredentialsFileName), } } else { + allowedBackends := systemBackends + if opts.Backend != "" { + bt := keyring.BackendType(opts.Backend) + if !isSystemBackend(bt) { + return nil, fmt.Errorf( + "credstore: unknown --backend %q (available: %s)", + opts.Backend, strings.Join(backendNames(), ", "), + ) + } + allowedBackends = []keyring.BackendType{bt} + } + kr, err := keyring.Open(keyring.Config{ ServiceName: serviceName, // The pass backend namespaces entries via PassPrefix, not @@ -249,7 +308,7 @@ func New(opts Options) (Store, error) { // stored as a top-level "credentials" entry in the user's // password store, risking collisions with unrelated tools. PassPrefix: serviceName, - AllowedBackends: systemBackends, + AllowedBackends: allowedBackends, KeychainTrustApplication: true, }) if err != nil { From a15d6e9e8f822aee336ad6dac553a0817a600725 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 15:43:44 -0700 Subject: [PATCH 14/17] Update Go dependencies go get -u ./... && go mod tidy, per contributing guidelines. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index e65b3ed..e66af03 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ go 1.25.14 require ( github.com/99designs/keyring v1.2.2 github.com/urfave/cli-docs/v3 v3.1.0 - github.com/urfave/cli/v3 v3.10.1 + github.com/urfave/cli/v3 v3.11.0 golang.org/x/oauth2 v0.36.0 ) @@ -15,7 +15,7 @@ require ( github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect github.com/cpuguy83/go-md2man/v2 v2.0.7 // indirect github.com/danieljoos/wincred v1.2.3 // indirect - github.com/dvsekhvalnov/jose2go v1.8.0 // indirect + github.com/dvsekhvalnov/jose2go v1.10.0 // indirect github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c // indirect github.com/mtibben/percent v0.2.1 // indirect diff --git a/go.sum b/go.sum index 652414f..44c705b 100644 --- a/go.sum +++ b/go.sum @@ -8,8 +8,8 @@ github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMF github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dvsekhvalnov/jose2go v1.8.0 h1:LqkkVKAlHFfH9LOEl5fe4p/zL02OhWE7pCufMBG2jLA= -github.com/dvsekhvalnov/jose2go v1.8.0/go.mod h1:QsHjhyTlD/lAVqn/NSbVZmSCGeDehTB/mPZadG+mhXU= +github.com/dvsekhvalnov/jose2go v1.10.0 h1:5RmEnUoQBMBURnk346hX3dKqG60Jkf9qkp6dkLsFx60= +github.com/dvsekhvalnov/jose2go v1.10.0/go.mod h1:QsHjhyTlD/lAVqn/NSbVZmSCGeDehTB/mPZadG+mhXU= github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 h1:ZpnhV/YsD2/4cESfV5+Hoeu/iUR3ruzNvZ+yQfO03a0= github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2/go.mod h1:bBOAhwG1umN6/6ZUMtDFBMQR8jRg9O75tm9K00oMsK4= github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c h1:6rhixN/i8ZofjG1Y75iExal34USq5p+wiN1tpie8IrU= @@ -31,8 +31,8 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/urfave/cli-docs/v3 v3.1.0 h1:Sa5xm19IpE5gpm6tZzXdfjdFxn67PnEsE4dpXF7vsKw= github.com/urfave/cli-docs/v3 v3.1.0/go.mod h1:59d+5Hz1h6GSGJ10cvcEkbIe3j233t4XDqI72UIx7to= -github.com/urfave/cli/v3 v3.10.1 h1:7Kx9H50hrHbRbyxgO1KP6/BcbiGRz0uYh5YyQ30JEEY= -github.com/urfave/cli/v3 v3.10.1/go.mod h1:ysVLtOEmg2tOy6PknnYVhDoouyC/6N42TMeoMzskhso= +github.com/urfave/cli/v3 v3.11.0 h1:P/euJp99kb9p0tlVY+iYTLYYTAQlfl0hR2gUO1Img1Q= +github.com/urfave/cli/v3 v3.11.0/go.mod h1:ysVLtOEmg2tOy6PknnYVhDoouyC/6N42TMeoMzskhso= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= From fa9c0ec5f8002e1771b2c93b1f7138a8eb83d66e Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 15:48:17 -0700 Subject: [PATCH 15/17] Fix --backend validation and help text - New validated --backend against the cross-platform systemBackends list rather than AvailableBackends() (the subset actually compiled into this binary for the current OS). A value like "keychain" on Linux therefore passed validation only to fail later with a generic keyring-open error, and the "available" list in that validation error didn't match what `lfx auth backends` reports. Validate against AvailableBackends() instead. - --backend's help text said it was "ignored with --insecure-storage", but credStoreFromCommand rejects that combination outright. Describe them as mutually exclusive instead, matching the actual behavior. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 2 +- internal/credstore/credstore.go | 38 ++++++++++++++++------------ internal/credstore/credstore_test.go | 11 ++++++++ 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 6967ef7..0aad42c 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -69,7 +69,7 @@ func NewAuthCommand() *cli.Command { }, &cli.StringFlag{ Name: backendFlagName, - Usage: "Pin credential storage to a specific system backend (see `lfx auth backends`); ignored with --insecure-storage", + Usage: "Pin credential storage to a specific system backend (see `lfx auth backends`); mutually exclusive with --insecure-storage", }, }, Commands: []*cli.Command{ diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 1b1e447..f69626e 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -128,25 +128,31 @@ func AvailableBackends() []Backend { return backends } -// isSystemBackend reports whether bt is one of the backends New is willing -// to select at all (see systemBackends). -func isSystemBackend(bt keyring.BackendType) bool { - for _, b := range systemBackends { - if b == bt { +// isAvailableBackend reports whether bt is one of the backends actually +// compiled into this binary for the current OS (see AvailableBackends), +// not merely one of the cross-platform systemBackends identifiers. New +// validates --backend against this, not systemBackends: otherwise a value +// like "keychain" would be accepted on Linux (where it's never even +// compiled in, per the darwin-only build tag on +// github.com/99designs/keyring's Keychain backend) only to fail later with +// a generic keyring-open error instead of a clear "unsupported on this OS" +// message. +func isAvailableBackend(bt keyring.BackendType) bool { + for _, b := range AvailableBackends() { + if b.Name == string(bt) { return true } } return false } -// backendNames renders systemBackends' keyring.BackendType identifiers -// (e.g. "keychain") for use in error messages about an invalid --keyring- -// backend value; these are the same strings AvailableBackends reports as -// Backend.Name. -func backendNames() []string { - names := make([]string, len(systemBackends)) - for i, b := range systemBackends { - names[i] = string(b) +// availableBackendNames renders AvailableBackends' Name fields for use in +// error messages about an invalid --backend value. +func availableBackendNames() []string { + backends := AvailableBackends() + names := make([]string, len(backends)) + for i, b := range backends { + names[i] = b.Name } return names } @@ -292,10 +298,10 @@ func New(opts Options) (Store, error) { allowedBackends := systemBackends if opts.Backend != "" { bt := keyring.BackendType(opts.Backend) - if !isSystemBackend(bt) { + if !isAvailableBackend(bt) { return nil, fmt.Errorf( - "credstore: unknown --backend %q (available: %s)", - opts.Backend, strings.Join(backendNames(), ", "), + "credstore: unknown or unsupported --backend %q on this OS (available: %s)", + opts.Backend, strings.Join(availableBackendNames(), ", "), ) } allowedBackends = []keyring.BackendType{bt} diff --git a/internal/credstore/credstore_test.go b/internal/credstore/credstore_test.go index 49634be..8d7d363 100644 --- a/internal/credstore/credstore_test.go +++ b/internal/credstore/credstore_test.go @@ -21,6 +21,17 @@ func newTestStore(t *testing.T) Store { return store } +func TestNewRejectsUnavailableBackend(t *testing.T) { + // Not a real keyring.BackendType on any OS, so this must be rejected + // regardless of which backends this build's OS actually compiles in + // (see isAvailableBackend, which checks AvailableBackends() rather + // than the cross-platform systemBackends list). + _, err := New(Options{Backend: "totally-bogus-backend", StateDir: t.TempDir()}) + if err == nil { + t.Fatal("New: got nil error, want unavailable-backend error") + } +} + func TestCredentialsRoundTrip(t *testing.T) { store := newTestStore(t) From 54bd4f95bde71c4ad4f86fa7ffa0e3d33e48da3c Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 17:26:08 -0700 Subject: [PATCH 16/17] Validate pinned backend before returning cached token newAuthTokenCommand's ValidAccessToken fast path returned a cached access token before loadDeviceStateForBackend ran, so omitting a pinned --backend could still auto-open some other backend and print its cached token, bypassing the pin. Move the device-state validation ahead of the cache check so it always runs first. Also fix a stray "Ignored when --insecure-storage is set" doc comment on backendFlagName that should have been updated alongside the --backend/--insecure-storage mutual-exclusion fix in fa9c0ec. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 0aad42c..e369f1f 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -31,8 +31,8 @@ const insecureStorageFlagName = "insecure-storage" // backendFlagName is the auth command group's flag pinning // credential storage to a single system keyring backend (see // `lfx auth backends`), instead of letting keyring.Open silently pick -// whichever backend currently opens. Ignored when --insecure-storage is -// set. +// whichever backend currently opens. Mutually exclusive with +// --insecure-storage; credStoreFromCommand rejects passing both. const backendFlagName = "backend" // Flag names shared by the login command. @@ -430,6 +430,17 @@ func newAuthTokenCommand() *cli.Command { return errors.New("not logged in; run `lfx auth login` first") } + // Validate the persisted device state (insecure-storage and + // --backend pinning) before trusting or returning anything + // from creds, including the ValidAccessToken fast path below + // -- otherwise omitting a pinned --backend could still open + // some other auto-detected backend and print its cached + // token, defeating the pin. + _, domain, clientID, err := loadDeviceStateForBackend(store, cmd) + if err != nil { + return fmt.Errorf("load device state: %w", err) + } + if creds.ValidAccessToken() { fmt.Println(creds.AccessToken) return nil @@ -439,10 +450,6 @@ func newAuthTokenCommand() *cli.Command { return errors.New("no refresh token available; run `lfx auth login` again") } - _, domain, clientID, err := loadDeviceStateForBackend(store, cmd) - if err != nil { - return fmt.Errorf("load device state: %w", err) - } cfg := &oauth2.Config{ ClientID: clientID, Endpoint: oauth2.Endpoint{ From c6147944ae475062e578eee139fabc4ce4d02c6d Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Mon, 24 Aug 2026 17:27:27 -0700 Subject: [PATCH 17/17] Fix misleading backend-mismatch note in status/logout stateMatchesInvocation returns false for either an --insecure-storage mismatch or a --backend pin mismatch, but the advisory notes in `auth status` and `auth logout` rendered the reason using only backendDescription(state.Insecure), which only ever describes the Insecure case. For a pinned-backend mismatch (Insecure actually matches), the note misleadingly named the *current* invocation's own backend and never mentioned the pinned backend or the --backend flag needed to match it. Add stateMismatchReason, mirroring loadDeviceStateForBackend's existing backend-specific error message, and use it in both notes. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/auth.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index e369f1f..948c553 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -369,6 +369,22 @@ func stateMatchesInvocation(state credstore.DeviceState, cmd *cli.Command) bool return state.Backend == "" || state.Backend == cmd.String(backendFlagName) } +// stateMismatchReason renders a human-readable explanation of why +// stateMatchesInvocation returned false for state and cmd, for use in the +// advisory notes `status` and `logout` print instead of erroring outright +// (unlike loadDeviceStateForBackend, which does treat this as fatal). +// Distinguishing the two causes matters: backendDescription(state.Insecure) +// alone describes only the Insecure mismatch and, for a --backend pin +// mismatch where Insecure actually matches, would misleadingly reuse the +// current invocation's own backend name and omit the --backend value +// needed to fix it. +func stateMismatchReason(state credstore.DeviceState, cmd *cli.Command) string { + if state.Insecure != cmd.Bool(insecureStorageFlagName) { + return fmt.Sprintf("%s (pass %s to match)", backendDescription(state.Insecure), insecureStorageUsageHint(state.Insecure)) + } + return fmt.Sprintf("keyring backend %q pinned at login (pass --%s=%s to match)", state.Backend, backendFlagName, state.Backend) +} + // persistLogin saves creds and state as a pair. The two writes are not // atomic: if SaveDeviceState fails after SaveCredentials succeeds, the // just-saved credentials are rolled back (deleted) rather than left paired @@ -517,7 +533,7 @@ func newAuthStatusCommand() *cli.Command { fmt.Printf( " Note: stored login state below belongs to %s, not this credential backend; "+ "it may not describe these credentials. Run `lfx auth login` to refresh it.\n", - backendDescription(state.Insecure), + stateMismatchReason(state, cmd), ) } if state.Environment != "" { @@ -576,7 +592,7 @@ func newAuthLogoutCommand() *cli.Command { default: fmt.Printf( "Note: leaving stored login state in place; it belongs to %s.\n", - backendDescription(state.Insecure), + stateMismatchReason(state, cmd), ) }