From 6a961db197b7a1e4c6fd53dd0590dfb95484c1b0 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 23 Jul 2026 17:16:57 -0700 Subject: [PATCH 1/5] clientconfig: add a refreshing token identity type Logging in registered a fresh ed25519 key with the cloud every time, which is great for long-lived sessions but puts real load on the backend for a feature most people never use. The cloud already grew the other half of the answer (the device flow now returns a refresh token, and /auth/refresh rotates a pair), so the client needs a way to hold a token instead of a key. Adds a "token" identity carrying an access and refresh token, and a single TokenForIdentity seam that resolves any identity to a bearer token: challenge-response for keypair, cached-or-refreshed for token. The refresh endpoint consumes the token it is handed, so two miren processes refreshing at once would leave one holding a spent token and a spurious logout. Refreshes therefore serialize on a file lock and re-read the identity inside it, which turns the loser of a race into a no-op instead of a 401. Only a definitive 401 means re-login; a 5xx or a network blip keeps the existing tokens rather than stampeding every CLI user into re-authenticating at once. Two incidental fixes fell out along the way: SetLeafConfig was dropping the Keys map when building its in-memory view, and leaf configs were written non-atomically, which is a torn read waiting to happen now that they are rewritten on a token refresh. --- clientconfig/clientconfig.go | 69 +++- clientconfig/leafwrite_test.go | 114 ++++++ clientconfig/local.go | 53 +-- clientconfig/setleafconfig_keys_test.go | 32 ++ clientconfig/token.go | 355 ++++++++++++++++++ clientconfig/token_identity_test.go | 44 +++ .../token_refresh_concurrency_test.go | 207 ++++++++++ clientconfig/token_test.go | 206 ++++++++++ go.mod | 2 +- 9 files changed, 1029 insertions(+), 53 deletions(-) create mode 100644 clientconfig/leafwrite_test.go create mode 100644 clientconfig/setleafconfig_keys_test.go create mode 100644 clientconfig/token.go create mode 100644 clientconfig/token_identity_test.go create mode 100644 clientconfig/token_refresh_concurrency_test.go create mode 100644 clientconfig/token_test.go diff --git a/clientconfig/clientconfig.go b/clientconfig/clientconfig.go index 16103927f..669963e72 100644 --- a/clientconfig/clientconfig.go +++ b/clientconfig/clientconfig.go @@ -22,12 +22,14 @@ const ( // IdentityConfig holds authentication credentials that can be used across clusters type IdentityConfig struct { - Type string `yaml:"type"` // Type of identity: "keypair", "certificate", etc. - Issuer string `yaml:"issuer,omitempty"` // The auth server that issued this identity (e.g., "https://miren.cloud") - KeyRef string `yaml:"key_ref,omitempty"` // Reference to a key in the Keys section (for keypair auth) - PrivateKey string `yaml:"private_key,omitempty"` // PEM encoded private key (for keypair auth, deprecated - use KeyRef) - ClientCert string `yaml:"client_cert,omitempty"` // PEM encoded client certificate (for cert auth) - ClientKey string `yaml:"client_key,omitempty"` // PEM encoded client key (for cert auth) + Type string `yaml:"type"` // Type of identity: "keypair", "token", "certificate", etc. + Issuer string `yaml:"issuer,omitempty"` // The auth server that issued this identity (e.g., "https://miren.cloud") + KeyRef string `yaml:"key_ref,omitempty"` // Reference to a key in the Keys section (for keypair auth) + PrivateKey string `yaml:"private_key,omitempty"` // PEM encoded private key (for keypair auth, deprecated - use KeyRef) + ClientCert string `yaml:"client_cert,omitempty"` // PEM encoded client certificate (for cert auth) + ClientKey string `yaml:"client_key,omitempty"` // PEM encoded client key (for cert auth) + Token string `yaml:"token,omitempty"` // JWT access token (for ephemeral "token" auth) + RefreshToken string `yaml:"refresh_token,omitempty"` // Refresh token used to renew the access token (for "token" auth) } // KeyConfig holds a reusable cryptographic key @@ -257,6 +259,7 @@ func (c *Config) SetLeafConfig(name string, configData *ConfigData) { active: configData.Active, clusters: configData.Clusters, identities: configData.Identities, + keys: configData.Keys, sourcePath: name, // Store the name so we can identify this leaf config later } @@ -637,6 +640,34 @@ func isEmptyConfigData(d *ConfigData) bool { len(d.Keys) == 0) } +// atomicWriteFile writes data to path via a temp file in the same directory +// followed by a rename, so a concurrent reader never observes a partially +// written file. Leaf configs are now rewritten on a hot path (token refresh), +// so a torn read here would fail the whole LoadConfig. +func atomicWriteFile(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tmpPath := tmp.Name() + // Best-effort cleanup if we bail out before the rename. + defer func() { _ = os.Remove(tmpPath) }() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Chmod(perm); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmpPath, path) +} + // saveLeafConfigs saves all unsaved leaf configs to clientconfig.d/ directory func (c *Config) saveLeafConfigs(mainConfigPath string) error { // Determine the config.d directory path based on the main config path @@ -666,7 +697,7 @@ func (c *Config) saveLeafConfigs(mainConfigPath string) error { return fmt.Errorf("failed to marshal leaf config %s: %w", name, err) } - if err := os.WriteFile(leafPath, data, 0600); err != nil { + if err := atomicWriteFile(leafPath, data, 0600); err != nil { return fmt.Errorf("failed to write leaf config %s: %w", name, err) } } @@ -751,6 +782,30 @@ func (c *Config) GetClusterSource(name string) string { return "" } +// GetIdentitySource returns the source file path for an identity. +// For identities in the main config, returns the main config path. +// For identities in leaf configs, returns the leaf config path. +// Returns "" if the identity is not found. +func (c *Config) GetIdentitySource(name string) string { + // Check leaf configs first (they override main config) + for _, leafConfig := range c.leafConfigs { + if _, exists := leafConfig.identities[name]; exists { + if c.sourcePath != "" { + configDir := filepath.Dir(c.sourcePath) + return filepath.Join(configDir, "clientconfig.d", leafConfig.sourcePath+".yaml") + } + return leafConfig.sourcePath + } + } + + // Check main config + if _, exists := c.identities[name]; exists { + return c.sourcePath + } + + return "" +} + // HasAnyClusters checks if any clusters are configured func (c *Config) HasAnyClusters() bool { // Check main config diff --git a/clientconfig/leafwrite_test.go b/clientconfig/leafwrite_test.go new file mode 100644 index 000000000..b83341a76 --- /dev/null +++ b/clientconfig/leafwrite_test.go @@ -0,0 +1,114 @@ +package clientconfig + +import ( + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +func TestGetIdentitySource(t *testing.T) { + dir := t.TempDir() + mainPath := filepath.Join(dir, "clientconfig.yaml") + + cfg := NewConfig() + cfg.sourcePath = mainPath + + // Identity that lives directly in the main config. + cfg.SetIdentity("main-id", &IdentityConfig{Type: "keypair"}) + + // Identity that lives in a leaf config file. + cfg.SetLeafConfig("identity-leaf-id", &ConfigData{ + Identities: map[string]*IdentityConfig{ + "leaf-id": {Type: "token"}, + }, + }) + + require.Equal(t, mainPath, cfg.GetIdentitySource("main-id")) + require.Equal(t, + filepath.Join(dir, "clientconfig.d", "identity-leaf-id.yaml"), + cfg.GetIdentitySource("leaf-id")) + require.Equal(t, "", cfg.GetIdentitySource("missing")) +} + +// TestAtomicWriteFileNoTornRead hammers atomicWriteFile with a concurrent +// reader that unmarshals the file; a non-atomic write would occasionally yield +// a truncated document and a yaml error. +func TestAtomicWriteFileNoTornRead(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "leaf.yaml") + + // Seed a valid file so the reader always has something to read. + small := []byte("type: token\n") + require.NoError(t, atomicWriteFile(path, small, 0600)) + + // A large payload makes a torn read far more likely under a plain WriteFile. + big, err := yaml.Marshal(&ConfigData{ + Identities: func() map[string]*IdentityConfig { + m := make(map[string]*IdentityConfig) + for i := 0; i < 200; i++ { + m[string(rune('a'+i%26))+string(rune('0'+i%10))+"-"+itoa(i)] = &IdentityConfig{ + Type: "token", + Token: "eyJhbGciOiJFZERTQSJ9." + itoa(i), + RefreshToken: "eyJhbGciOiJFZERTQSJ9.refresh." + itoa(i), + } + } + return m + }(), + }) + require.NoError(t, err) + + var wg sync.WaitGroup + stop := make(chan struct{}) + + // Reader goroutine. + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + data, err := os.ReadFile(path) + if err != nil { + continue // File momentarily absent is fine; rename is what matters. + } + var cd ConfigData + require.NoError(t, yaml.Unmarshal(data, &cd), "reader observed a torn/partial write") + } + }() + + // Writer: alternate payloads so length changes each time. + for i := 0; i < 300; i++ { + payload := big + if i%2 == 0 { + payload = small + } + require.NoError(t, atomicWriteFile(path, payload, 0600)) + } + close(stop) + wg.Wait() + + // Final file must be valid and mode 0600. + info, err := os.Stat(path) + require.NoError(t, err) + require.Equal(t, os.FileMode(0600), info.Mode().Perm()) +} + +// itoa is a tiny helper to avoid pulling strconv into a hot loop's closure. +func itoa(i int) string { + if i == 0 { + return "0" + } + var b []byte + for i > 0 { + b = append([]byte{byte('0' + i%10)}, b...) + i /= 10 + } + return string(b) +} diff --git a/clientconfig/local.go b/clientconfig/local.go index 6cf931e2c..173fe6cf2 100644 --- a/clientconfig/local.go +++ b/clientconfig/local.go @@ -16,7 +16,6 @@ import ( "strings" "time" - "github.com/golang-jwt/jwt/v5" "github.com/quic-go/quic-go" "miren.dev/runtime/pkg/caauth" "miren.dev/runtime/pkg/cloudauth" @@ -110,27 +109,12 @@ foundAddress: // Handle different identity types switch identity.Type { - case "keypair": - // Get the private key (handles both direct PrivateKey and KeyRef) - privateKeyPEM, err := config.GetPrivateKeyPEM(identity) - if err != nil { - return nil, fmt.Errorf("failed to get private key: %w", err) - } - - // Load the private key - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) - if err != nil { - return nil, fmt.Errorf("failed to load private key: %w", err) - } - - // Use the issuer from the identity, or fall back to cluster hostname - authServer := identity.Issuer - if authServer == "" { - authServer = hostname - } - - // Get JWT token using the challenge-response flow - token, err := AuthenticateWithKey(ctx, authServer, keyPair) + case "keypair", "token": + // Both keypair and ephemeral token identities resolve to a bearer + // token; TokenForIdentity dispatches on the identity type (re-minting + // via challenge-response for keypair, or refreshing the cached token + // for a token identity). + token, err := config.TokenForIdentity(ctx, c.Identity, identity, hostname) if err != nil { return nil, fmt.Errorf("failed to authenticate with cloud: %w", err) } @@ -291,29 +275,8 @@ func getCachedToken(fingerprint string) (string, error) { tokenString := string(tokenData) - // Parse the JWT without verification to check expiry - parser := jwt.NewParser() - token, _, err := parser.ParseUnverified(tokenString, jwt.MapClaims{}) - if err != nil { - return "", nil // Invalid token, need new one - } - - claims, ok := token.Claims.(jwt.MapClaims) - if !ok { - return "", nil // Invalid claims, need new one - } - - // Check if token is expired - if exp, ok := claims["exp"].(float64); ok { - if time.Now().Unix() >= int64(exp) { - return "", nil // Token expired - } - // Add a buffer of 5 minutes to avoid edge cases - if time.Now().Unix() >= int64(exp)-300 { - return "", nil // Token expiring soon - } - } else { - return "", nil // No expiry claim, need new one + if !tokenFresh(tokenString, tokenExpiryBuffer) { + return "", nil // Missing, expired, or expiring soon — need a new one. } return tokenString, nil diff --git a/clientconfig/setleafconfig_keys_test.go b/clientconfig/setleafconfig_keys_test.go new file mode 100644 index 000000000..2c43ef32b --- /dev/null +++ b/clientconfig/setleafconfig_keys_test.go @@ -0,0 +1,32 @@ +package clientconfig + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestSetLeafConfigRetainsKeys guards against a regression where SetLeafConfig +// built its in-memory leaf Config without copying Keys, so a key registered via +// a leaf config was invisible to GetKey/HasKey until the config was reloaded +// from disk. +func TestSetLeafConfigRetainsKeys(t *testing.T) { + cfg := NewConfig() + + cfg.SetLeafConfig("key-miren-cli", &ConfigData{ + Keys: map[string]*KeyConfig{ + "miren-cli": { + Name: "miren-cli", + Type: "ed25519", + PrivateKey: "-----BEGIN PRIVATE KEY-----\nfake\n-----END PRIVATE KEY-----", + }, + }, + }) + + // The key must be visible immediately, before any Save/reload. + require.True(t, cfg.HasKey("miren-cli"), "HasKey should see a leaf-config key before reload") + + key, err := cfg.GetKey("miren-cli") + require.NoError(t, err, "GetKey should resolve a leaf-config key before reload") + require.Equal(t, "ed25519", key.Type) +} diff --git a/clientconfig/token.go b/clientconfig/token.go new file mode 100644 index 000000000..d74d1cd11 --- /dev/null +++ b/clientconfig/token.go @@ -0,0 +1,355 @@ +package clientconfig + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "os" + "path/filepath" + "strings" + "time" + + "github.com/gofrs/flock" + "github.com/golang-jwt/jwt/v5" + "gopkg.in/yaml.v3" + + "miren.dev/runtime/pkg/cloudauth" +) + +// ErrLoginRequired signals that a credential has expired or been revoked and +// the user must run `miren login` again. It is returned only for definitive +// server rejections (HTTP 401), never for transient failures (5xx, network +// errors) — treating a cloud blip as a logout would stampede every CLI user +// into re-authenticating at once. +var ErrLoginRequired = errors.New("login required: session expired or revoked") + +// ErrNoBearerToken signals that an identity does not authenticate via a bearer +// token (e.g. certificate identities). Callers should fall back to their own +// credential handling rather than treating this as a failure. +var ErrNoBearerToken = errors.New("identity does not use bearer-token authentication") + +// tokenPair is the access/refresh credential returned by the cloud's token +// endpoints (device flow and /auth/refresh). +type tokenPair struct { + AccessToken string `json:"access_token"` + RefreshToken string `json:"refresh_token"` + TokenType string `json:"token_type"` + ExpiresIn int `json:"expires_in"` +} + +// NormalizeIssuerURL ensures an auth-server URL carries a scheme, defaulting to +// https:// but using http:// for loopback hosts so local-dev clouds work. A +// URL that already has a scheme is returned with any trailing slash trimmed. +func NormalizeIssuerURL(server string) string { + if strings.HasPrefix(server, "http://") || strings.HasPrefix(server, "https://") { + return strings.TrimSuffix(server, "/") + } + if strings.Contains(server, "localhost") || strings.Contains(server, "127.0.0.1") { + return "http://" + server + } + return "https://" + server +} + +// refreshTokenPair exchanges a refresh token for a fresh access/refresh pair via +// POST {issuer}/auth/refresh. The endpoint rotates: it consumes the presented +// refresh token and returns a new one, so the caller MUST persist the returned +// pair atomically before using it. +// +// It returns ErrLoginRequired for a 401 (expired/revoked/already-spent token), +// and a plain wrapped error for transient failures (network, 5xx, malformed +// response) so callers can distinguish "must re-login" from "try again later". +func refreshTokenPair(ctx context.Context, issuer, refreshToken string) (*tokenPair, error) { + if refreshToken == "" { + return nil, ErrLoginRequired + } + + refreshURL, err := url.JoinPath(NormalizeIssuerURL(issuer), "/auth/refresh") + if err != nil { + return nil, fmt.Errorf("invalid issuer URL: %w", err) + } + + reqBody, err := json.Marshal(map[string]string{"refresh_token": refreshToken}) + if err != nil { + return nil, fmt.Errorf("failed to marshal refresh request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, refreshURL, bytes.NewReader(reqBody)) + if err != nil { + return nil, fmt.Errorf("failed to create refresh request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to send refresh request: %w", err) + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read refresh response: %w", err) + } + + switch resp.StatusCode { + case http.StatusOK: + // fall through to parsing below + case http.StatusUnauthorized: + // Definitive rejection: token expired, revoked, or already spent by a + // concurrent refresh. The refresh endpoint returns plain-text bodies. + return nil, ErrLoginRequired + default: + // Transient (5xx, rate-limit, etc.): keep the existing tokens. + return nil, fmt.Errorf("token refresh failed: server returned status %d: %s", + resp.StatusCode, strings.TrimSpace(string(body))) + } + + var pair tokenPair + if err := json.Unmarshal(body, &pair); err != nil { + return nil, fmt.Errorf("failed to parse refresh response: %w", err) + } + if pair.AccessToken == "" || pair.RefreshToken == "" { + return nil, fmt.Errorf("token refresh response missing access or refresh token") + } + + return &pair, nil +} + +// tokenExpiryBuffer is how long before a JWT's actual expiry we treat it as +// stale. Refreshing slightly early avoids handing a token to an RPC that then +// expires mid-flight due to clock skew or request latency. +const tokenExpiryBuffer = 5 * time.Minute + +// tokenFresh reports whether a JWT access token is safe to use for at least +// buffer longer. It parses the token WITHOUT verifying the signature — this is +// only a local staleness check, not an authorization decision; the server +// validates the signature. A token is considered stale (not fresh) when it is +// unparseable, carries no exp claim, or expires within buffer. +func tokenFresh(tokenString string, buffer time.Duration) bool { + parser := jwt.NewParser() + token, _, err := parser.ParseUnverified(tokenString, jwt.MapClaims{}) + if err != nil { + return false + } + + claims, ok := token.Claims.(jwt.MapClaims) + if !ok { + return false + } + + exp, ok := claims["exp"].(float64) + if !ok { + return false // No expiry claim, treat as stale so we fetch a fresh one. + } + + return time.Now().Unix() < int64(exp)-int64(buffer.Seconds()) +} + +// TokenForIdentity returns a bearer token to authenticate as the given identity. +// +// - keypair: re-mints a JWT via the challenge-response flow (AuthenticateWithKey). +// - token: returns the cached access token if still fresh; otherwise refreshes +// it via /auth/refresh under a cross-process lock and persists the rotated pair. +// - certificate: returns ErrNoBearerToken (the caller supplies a client cert). +// +// name identifies which identity leaf to write rotated tokens back to. Pass "" +// for an anonymous/in-memory identity (e.g. during login before it is named): +// a refresh is then used for this call only and not persisted. +// +// fallbackHost is used as the issuer when the identity carries no Issuer. +func (c *Config) TokenForIdentity(ctx context.Context, name string, identity *IdentityConfig, fallbackHost string) (string, error) { + switch identity.Type { + case "keypair": + privateKeyPEM, err := c.GetPrivateKeyPEM(identity) + if err != nil { + return "", fmt.Errorf("failed to get private key: %w", err) + } + keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) + if err != nil { + return "", fmt.Errorf("failed to load private key: %w", err) + } + authServer := identity.Issuer + if authServer == "" { + authServer = fallbackHost + } + return AuthenticateWithKey(ctx, authServer, keyPair) + + case "token": + return c.tokenForTokenIdentity(ctx, name, identity, fallbackHost) + + case "certificate": + return "", ErrNoBearerToken + + default: + return "", fmt.Errorf("unknown identity type: %s", identity.Type) + } +} + +// tokenForTokenIdentity implements the "token" arm of TokenForIdentity: a +// lock-free fast path for a still-fresh access token, and a locked slow path +// that refreshes and persists the rotated pair. +func (c *Config) tokenForTokenIdentity(ctx context.Context, name string, identity *IdentityConfig, fallbackHost string) (string, error) { + // Fast path: a fresh cached token needs no lock and no network. + if tokenFresh(identity.Token, tokenExpiryBuffer) { + return identity.Token, nil + } + + issuer := identity.Issuer + if issuer == "" { + issuer = fallbackHost + } + + // Anonymous identity: nothing to lock or persist, just refresh in memory. + if name == "" { + pair, err := refreshTokenPair(ctx, issuer, identity.RefreshToken) + if err != nil { + return "", err + } + return pair.AccessToken, nil + } + + source := c.GetIdentitySource(name) + if source == "" { + return "", fmt.Errorf("cannot locate config file for identity %q", name) + } + + // Serialize refreshes across processes. The refresh endpoint consumes the + // presented refresh token (single-use rotation), so two racers would leave + // one holding a spent token; the lock plus the in-lock re-read collapses the + // loser into a cheap no-op. + // + // The lock file is a hidden dotfile beside the config so it doesn't clutter + // the directory a user inspects. It is intentionally left in place rather than + // removed on unlock: unlinking a flock target lets a concurrent waiter and a + // new process end up locking two different inodes, defeating the mutex. + lock := flock.New(lockPathFor(source)) + if err := lock.Lock(); err != nil { + return "", fmt.Errorf("failed to acquire token refresh lock: %w", err) + } + defer func() { _ = lock.Unlock() }() + + // Re-read from disk inside the lock: a peer may have already rotated while we + // waited, in which case its fresh access token is right there. + current, err := readIdentityFromFile(source, name) + if err != nil { + return "", fmt.Errorf("failed to re-read identity %q: %w", name, err) + } + if tokenFresh(current.Token, tokenExpiryBuffer) { + return current.Token, nil + } + + pair, err := refreshTokenPair(ctx, issuer, current.RefreshToken) + if err != nil { + return "", err + } + + // Persist the rotated pair immediately — the old refresh token is now spent + // server-side, so losing the new one would force a re-login. + if err := persistIdentityTokens(source, name, pair.AccessToken, pair.RefreshToken); err != nil { + return "", fmt.Errorf("failed to persist refreshed tokens: %w", err) + } + + return pair.AccessToken, nil +} + +// lockPathFor returns the refresh-lock path for a config file: a hidden dotfile +// in the same directory (e.g. clientconfig.d/.identity-cloud.yaml.lock). Keeping +// it beside the file means every process derives the same path; the dot prefix +// keeps it out of the way of a user inspecting the directory. Being a non-.yaml +// name, loadConfigDir ignores it regardless. +func lockPathFor(source string) string { + return filepath.Join(filepath.Dir(source), "."+filepath.Base(source)+".lock") +} + +// readIdentityFromFile reads a single identity out of a config file (leaf or +// main). It reads only the named identity so it never depends on the rest of +// the file being complete. +func readIdentityFromFile(path, name string) (*IdentityConfig, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var cd ConfigData + if err := yaml.Unmarshal(data, &cd); err != nil { + return nil, err + } + id, ok := cd.Identities[name] + if !ok || id == nil { + return nil, fmt.Errorf("identity %q not found in %s", name, path) + } + return id, nil +} + +// persistIdentityTokens writes a rotated access/refresh pair into the identity's +// source file, preserving every other field, via an atomic replace. The caller +// must hold the identity's lock. +func persistIdentityTokens(path, name, accessToken, refreshToken string) error { + data, err := os.ReadFile(path) + if err != nil { + return err + } + var cd ConfigData + if err := yaml.Unmarshal(data, &cd); err != nil { + return err + } + id, ok := cd.Identities[name] + if !ok || id == nil { + return fmt.Errorf("identity %q not found in %s", name, path) + } + id.Token = accessToken + id.RefreshToken = refreshToken + + out, err := yaml.Marshal(&cd) + if err != nil { + return err + } + return atomicWriteFile(path, out, 0600) +} + +// RevokeRefreshToken best-effort revokes a refresh token via +// POST {issuer}/auth/revoke/refresh. The endpoint requires the access token as a +// bearer credential. Any error is returned to the caller, which should treat +// revocation as advisory — a failed revoke must never block logout. +func RevokeRefreshToken(ctx context.Context, issuer, accessToken, refreshToken string) error { + if refreshToken == "" || accessToken == "" { + return nil + } + + revokeURL, err := url.JoinPath(NormalizeIssuerURL(issuer), "/auth/revoke/refresh") + if err != nil { + return fmt.Errorf("invalid issuer URL: %w", err) + } + + reqBody, err := json.Marshal(map[string]string{ + "refresh_token": refreshToken, + "reason": "logout", + }) + if err != nil { + return err + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, revokeURL, bytes.NewReader(reqBody)) + if err != nil { + return err + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+accessToken) + + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.Copy(io.Discard, resp.Body) + + if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNoContent { + return fmt.Errorf("revoke returned status %d", resp.StatusCode) + } + return nil +} diff --git a/clientconfig/token_identity_test.go b/clientconfig/token_identity_test.go new file mode 100644 index 000000000..72fb02d98 --- /dev/null +++ b/clientconfig/token_identity_test.go @@ -0,0 +1,44 @@ +package clientconfig + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestTokenIdentityRoundTrip verifies a "token" identity persists its access +// and refresh tokens through a save/reload cycle and lands at mode 0600. +func TestTokenIdentityRoundTrip(t *testing.T) { + dir := t.TempDir() + t.Setenv(EnvConfigPath, dir) + + cfg := NewConfig() + cfg.SetLeafConfig("identity-cloud", &ConfigData{ + Identities: map[string]*IdentityConfig{ + "cloud": { + Type: "token", + Issuer: "https://miren.cloud", + Token: "access.jwt.value", + RefreshToken: "refresh.jwt.value", + }, + }, + }) + require.NoError(t, cfg.SaveToHome()) + + leafPath := filepath.Join(dir, "clientconfig.d", "identity-cloud.yaml") + info, err := os.Stat(leafPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0600), info.Mode().Perm()) + + reloaded, err := LoadConfig() + require.NoError(t, err) + + id, err := reloaded.GetIdentity("cloud") + require.NoError(t, err) + require.Equal(t, "token", id.Type) + require.Equal(t, "https://miren.cloud", id.Issuer) + require.Equal(t, "access.jwt.value", id.Token) + require.Equal(t, "refresh.jwt.value", id.RefreshToken) +} diff --git a/clientconfig/token_refresh_concurrency_test.go b/clientconfig/token_refresh_concurrency_test.go new file mode 100644 index 000000000..7a9a60868 --- /dev/null +++ b/clientconfig/token_refresh_concurrency_test.go @@ -0,0 +1,207 @@ +package clientconfig + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// singleUseRefreshServer emulates the cloud's rotating, single-use /auth/refresh: +// the first presentation of a given refresh token mints a fresh pair; any reuse +// of a spent token returns 401. It counts successful (minting) refreshes. +type singleUseRefreshServer struct { + mu sync.Mutex + spent map[string]bool + minted int + rotation int +} + +func newSingleUseRefreshServer() *singleUseRefreshServer { + return &singleUseRefreshServer{spent: map[string]bool{}} +} + +func freshAccessJWT(t *testing.T) string { + t.Helper() + return makeToken(t, jwt.MapClaims{"exp": float64(time.Now().Add(time.Hour).Unix())}) +} + +func (s *singleUseRefreshServer) handler(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + var req struct { + RefreshToken string `json:"refresh_token"` + } + _ = json.NewDecoder(r.Body).Decode(&req) + + s.mu.Lock() + defer s.mu.Unlock() + + if s.spent[req.RefreshToken] { + http.Error(w, "Refresh token has been revoked", http.StatusUnauthorized) + return + } + s.spent[req.RefreshToken] = true + s.minted++ + s.rotation++ + newRefresh := "refresh-rot-" + itoa(s.rotation) + + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": freshAccessJWT(t), + "refresh_token": newRefresh, + "token_type": "Bearer", + "expires_in": 3600, + }) + } +} + +// writeTokenIdentityConfig writes a MIREN_CONFIG dir containing a "token" +// identity whose access token is already expired, forcing a refresh. +func writeTokenIdentityConfig(t *testing.T, dir, issuer string) { + t.Helper() + expired := makeToken(t, jwt.MapClaims{"exp": float64(time.Now().Add(-time.Hour).Unix())}) + cd := ConfigData{ + Identities: map[string]*IdentityConfig{ + "cloud": { + Type: "token", + Issuer: issuer, + Token: expired, + RefreshToken: "refresh-original", + }, + }, + } + data, err := yaml.Marshal(&cd) + require.NoError(t, err) + leafDir := filepath.Join(dir, "clientconfig.d") + require.NoError(t, os.MkdirAll(leafDir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(leafDir, "identity-cloud.yaml"), data, 0600)) +} + +// TestTokenForIdentityConcurrentRefresh is the load-bearing test for MIR-1385: +// many concurrent callers (goroutines AND subprocesses) sharing one expired +// access token must trigger exactly ONE successful /auth/refresh, because the +// refresh token is single-use. Every caller must still receive a valid token. +func TestTokenForIdentityConcurrentRefresh(t *testing.T) { + srv := httptest.NewServer(nil) + server := newSingleUseRefreshServer() + srv.Config.Handler = server.handler(t) + defer srv.Close() + + dir := t.TempDir() + writeTokenIdentityConfig(t, dir, srv.URL) + // Set once, before any goroutine starts: t.Setenv mutates process-global + // state and must not be called concurrently. + t.Setenv(EnvConfigPath, dir) + + const goroutines = 12 + const subprocesses = 6 + + var wg sync.WaitGroup + var goroutineFailures atomic.Int32 + tokens := make(chan string, goroutines+subprocesses) + + // In-process goroutines, each with its own freshly-loaded Config (mirrors + // separate `m` invocations that share the on-disk config). + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + cfg, err := LoadConfig() + if err != nil { + goroutineFailures.Add(1) + return + } + id, err := cfg.GetIdentity("cloud") + if err != nil { + goroutineFailures.Add(1) + return + } + tok, err := cfg.TokenForIdentity(context.Background(), "cloud", id, srv.URL) + if err != nil { + goroutineFailures.Add(1) + return + } + tokens <- tok + }() + } + + // Subprocesses: prove the lock is cross-process, not just cross-goroutine. + for i := 0; i < subprocesses; i++ { + wg.Add(1) + go func() { + defer wg.Done() + cmd := exec.Command(os.Args[0], "-test.run=TestTokenForIdentityChildProcess") + cmd.Env = append(os.Environ(), + "MIREN_TEST_REFRESH_CHILD=1", + EnvConfigPath+"="+dir, + "MIREN_TEST_ISSUER="+srv.URL, + ) + out, err := cmd.CombinedOutput() + if err != nil { + goroutineFailures.Add(1) + t.Logf("child failed: %v\n%s", err, out) + return + } + // Child prints the token on a line prefixed with TOKEN=. + for _, line := range strings.Split(string(out), "\n") { + if strings.HasPrefix(line, "TOKEN=") { + tokens <- strings.TrimPrefix(line, "TOKEN=") + } + } + }() + } + + wg.Wait() + close(tokens) + + require.Zero(t, goroutineFailures.Load(), "no caller should fail to obtain a token") + + server.mu.Lock() + minted := server.minted + server.mu.Unlock() + require.Equal(t, 1, minted, "the single-use refresh token must be exchanged exactly once") + + // Every caller must have received a usable (fresh) access token. + count := 0 + for tok := range tokens { + require.True(t, tokenFresh(tok, tokenExpiryBuffer), "returned token must be fresh") + count++ + } + require.Equal(t, goroutines+subprocesses, count, "every caller must return a token") + + // On-disk pair must be the rotated one and internally consistent. + final, err := readIdentityFromFile(filepath.Join(dir, "clientconfig.d", "identity-cloud.yaml"), "cloud") + require.NoError(t, err) + require.True(t, tokenFresh(final.Token, tokenExpiryBuffer)) + require.True(t, strings.HasPrefix(final.RefreshToken, "refresh-rot-"), "refresh token must be rotated") +} + +// TestTokenForIdentityChildProcess is not a real test: it is the subprocess body +// re-executed by TestTokenForIdentityConcurrentRefresh. It is inert unless the +// MIREN_TEST_REFRESH_CHILD env var is set. +func TestTokenForIdentityChildProcess(t *testing.T) { + if os.Getenv("MIREN_TEST_REFRESH_CHILD") != "1" { + return + } + issuer := os.Getenv("MIREN_TEST_ISSUER") + cfg, err := LoadConfig() + require.NoError(t, err) + id, err := cfg.GetIdentity("cloud") + require.NoError(t, err) + tok, err := cfg.TokenForIdentity(context.Background(), "cloud", id, issuer) + require.NoError(t, err) + // Stdout is captured by the parent. + os.Stdout.WriteString("TOKEN=" + tok + "\n") +} diff --git a/clientconfig/token_test.go b/clientconfig/token_test.go new file mode 100644 index 000000000..a09db2413 --- /dev/null +++ b/clientconfig/token_test.go @@ -0,0 +1,206 @@ +package clientconfig + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" +) + +// makeToken builds an unsigned JWT with the given claims. tokenFresh parses +// tokens unverified, so the signature is irrelevant to these tests. +func makeToken(t *testing.T, claims jwt.MapClaims) string { + t.Helper() + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + s, err := tok.SignedString([]byte("test-secret")) + require.NoError(t, err) + return s +} + +func TestTokenFresh(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + token string + want bool + }{ + { + name: "valid well beyond buffer", + token: makeToken(t, jwt.MapClaims{"exp": float64(now.Add(time.Hour).Unix())}), + want: true, + }, + { + name: "expiring within buffer", + token: makeToken(t, jwt.MapClaims{"exp": float64(now.Add(4 * time.Minute).Unix())}), + want: false, + }, + { + name: "already expired", + token: makeToken(t, jwt.MapClaims{"exp": float64(now.Add(-time.Minute).Unix())}), + want: false, + }, + { + name: "no exp claim", + token: makeToken(t, jwt.MapClaims{"sub": "user-1"}), + want: false, + }, + { + name: "unparseable garbage", + token: "not-a-jwt", + want: false, + }, + { + name: "empty string", + token: "", + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, tokenFresh(tc.token, tokenExpiryBuffer)) + }) + } +} + +// TestTokenFreshBufferBoundary verifies the buffer is honored: a token that +// outlives exp-buffer is fresh, one that doesn't is stale. +func TestTokenFreshBufferBoundary(t *testing.T) { + now := time.Now() + + // Expires in 10 minutes; with a 5-minute buffer it is still fresh. + fresh := makeToken(t, jwt.MapClaims{"exp": float64(now.Add(10 * time.Minute).Unix())}) + require.True(t, tokenFresh(fresh, tokenExpiryBuffer)) + + // The same token is stale under a 15-minute buffer. + require.False(t, tokenFresh(fresh, 15*time.Minute)) +} + +func TestNormalizeIssuerURL(t *testing.T) { + tests := []struct { + in, want string + }{ + {"miren.cloud", "https://miren.cloud"}, + {"https://miren.cloud", "https://miren.cloud"}, + {"https://miren.cloud/", "https://miren.cloud"}, + {"http://miren.host:3001", "http://miren.host:3001"}, + {"localhost:3001", "http://localhost:3001"}, + {"127.0.0.1:3001", "http://127.0.0.1:3001"}, + } + for _, tc := range tests { + require.Equal(t, tc.want, NormalizeIssuerURL(tc.in)) + } +} + +func TestRefreshTokenPair(t *testing.T) { + t.Run("200 rotates and returns new pair", func(t *testing.T) { + var gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"access_token":"new-access","refresh_token":"new-refresh","token_type":"Bearer","expires_in":3600}`)) + })) + defer srv.Close() + + pair, err := refreshTokenPair(context.Background(), srv.URL, "old-refresh") + require.NoError(t, err) + require.Equal(t, "/auth/refresh", gotPath) + require.Equal(t, "new-access", pair.AccessToken) + require.Equal(t, "new-refresh", pair.RefreshToken) + require.NotEqual(t, "old-refresh", pair.RefreshToken, "server must rotate the refresh token") + require.Equal(t, 3600, pair.ExpiresIn) + }) + + t.Run("401 means login required", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Refresh token has been revoked", http.StatusUnauthorized) + })) + defer srv.Close() + + _, err := refreshTokenPair(context.Background(), srv.URL, "spent-refresh") + require.ErrorIs(t, err, ErrLoginRequired) + }) + + t.Run("500 is transient, NOT login required", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Token validation error", http.StatusInternalServerError) + })) + defer srv.Close() + + _, err := refreshTokenPair(context.Background(), srv.URL, "some-refresh") + require.Error(t, err) + require.NotErrorIs(t, err, ErrLoginRequired, "a 5xx must not force a logout") + }) + + t.Run("200 with empty access token errors", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"access_token":"","refresh_token":"r","token_type":"Bearer","expires_in":3600}`)) + })) + defer srv.Close() + + _, err := refreshTokenPair(context.Background(), srv.URL, "some-refresh") + require.Error(t, err) + require.NotErrorIs(t, err, ErrLoginRequired) + }) + + t.Run("empty refresh token short-circuits to login required", func(t *testing.T) { + _, err := refreshTokenPair(context.Background(), "https://miren.cloud", "") + require.ErrorIs(t, err, ErrLoginRequired) + }) + + t.Run("network failure is transient", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + url := srv.URL + srv.Close() // nothing listening now + + _, err := refreshTokenPair(context.Background(), url, "some-refresh") + require.Error(t, err) + require.False(t, errors.Is(err, ErrLoginRequired), "a network failure must not force a logout") + }) +} + +func TestRevokeRefreshToken(t *testing.T) { + t.Run("posts bearer and refresh token", func(t *testing.T) { + var path, auth, gotRefresh, gotReason string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path = r.URL.Path + auth = r.Header.Get("Authorization") + var body struct { + RefreshToken string `json:"refresh_token"` + Reason string `json:"reason"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotRefresh, gotReason = body.RefreshToken, body.Reason + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + err := RevokeRefreshToken(context.Background(), srv.URL, "access-x", "refresh-y") + require.NoError(t, err) + require.Equal(t, "/auth/revoke/refresh", path) + require.Equal(t, "Bearer access-x", auth) + require.Equal(t, "refresh-y", gotRefresh) + require.Equal(t, "logout", gotReason) + }) + + t.Run("empty tokens are a no-op", func(t *testing.T) { + require.NoError(t, RevokeRefreshToken(context.Background(), "https://x", "", "")) + }) + + t.Run("non-2xx is an error", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + require.Error(t, RevokeRefreshToken(context.Background(), srv.URL, "a", "b")) + }) +} diff --git a/go.mod b/go.mod index 2820276f1..659136745 100644 --- a/go.mod +++ b/go.mod @@ -245,7 +245,7 @@ require ( github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/goccy/go-yaml v1.18.0 // indirect github.com/godbus/dbus/v5 v5.1.0 // indirect - github.com/gofrs/flock v0.13.0 // indirect + github.com/gofrs/flock v0.13.0 github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v4 v4.5.2 // indirect github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect From 469ec0c23eaff63ecde22f8554d186f643f405be Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 23 Jul 2026 17:17:13 -0700 Subject: [PATCH 2/5] cli: make miren login ephemeral by default Eight call sites had each grown their own copy of "load the keypair, mint a JWT, use it as a bearer token", and several hard-rejected any identity that was not a keypair. Adding a token identity without touching them would have broken whoami, doctor, cluster switch and cluster add for anyone logging in fresh, so they all now go through TokenForIdentity and accept both kinds. With that in place, login stores the device flow tokens instead of registering a key. --persistent-key opts back into the old dance, and an identity that already has a key keeps it on re-login so we never silently orphan a key the server still knows about. If the cloud is old enough that it returns no refresh token, login falls back to registering a key rather than saving credentials that cannot renew. Logout now revokes the refresh token before deleting the local files, best effort, since a copied config would otherwise keep renewing for a week after the user thought they had logged out. A 401 also stops blaming permissions for what is usually just an expired session. --- cli/commands/cluster_add.go | 2 +- cli/commands/cluster_switch.go | 25 +-- cli/commands/config_bind_helpers.go | 25 +-- cli/commands/debug_connection.go | 44 ++-- cli/commands/doctor.go | 17 +- cli/commands/global.go | 2 +- cli/commands/login.go | 302 ++++++++++++++++---------- cli/commands/login_deviceflow_test.go | 128 +++++++++++ cli/commands/login_ephemeral_test.go | 168 ++++++++++++++ cli/commands/logout.go | 11 + cli/commands/logout_revoke_test.go | 95 ++++++++ cli/commands/whoami.go | 43 +--- docs/docs/command/login.md | 1 + 13 files changed, 634 insertions(+), 229 deletions(-) create mode 100644 cli/commands/login_deviceflow_test.go create mode 100644 cli/commands/login_ephemeral_test.go create mode 100644 cli/commands/logout_revoke_test.go diff --git a/cli/commands/cluster_add.go b/cli/commands/cluster_add.go index 28b639bdf..302d73952 100644 --- a/cli/commands/cluster_add.go +++ b/cli/commands/cluster_add.go @@ -103,7 +103,7 @@ func addCluster(ctx *Context, identityName, clusterName, address string, force b if clusterName == "" && address == "" { ctx.Info("Fetching available clusters from identity server...") - clusters, err := fetchAvailableClusters(ctx, mainConfig, identity) + clusters, err := fetchAvailableClusters(ctx, mainConfig, identityName, identity) if err != nil { return fmt.Errorf("failed to fetch available clusters: %w", err) } diff --git a/cli/commands/cluster_switch.go b/cli/commands/cluster_switch.go index b504edfe7..c20d47932 100644 --- a/cli/commands/cluster_switch.go +++ b/cli/commands/cluster_switch.go @@ -10,7 +10,6 @@ import ( "miren.dev/runtime/appconfig" "miren.dev/runtime/clientconfig" - "miren.dev/runtime/pkg/cloudauth" "miren.dev/runtime/pkg/ui" ) @@ -133,7 +132,7 @@ func checkClusterAccess(ctx *Context, cfg *clientconfig.Config, clusterName stri if clusterXID == "" { // Fall back to fetching clusters and finding by name (for backwards compatibility // with clusters added before XID was stored) - clusters, err := fetchAvailableClusters(ctx, cfg, identity) + clusters, err := fetchAvailableClusters(ctx, cfg, cluster.Identity, identity) if err != nil { // If we can't reach the cloud API, we'll allow the switch // The user will get an access denied error when they try to perform actions @@ -156,7 +155,7 @@ func checkClusterAccess(ctx *Context, cfg *clientconfig.Config, clusterName stri } // Check RBAC permission via the check-access endpoint - hasAccess, reason, err := checkClusterAccessRBAC(ctx, cfg, identity, clusterXID) + hasAccess, reason, err := checkClusterAccessRBAC(ctx, cfg, cluster.Identity, identity, clusterXID) if err != nil { // If we can't reach the cloud API, we'll allow the switch ctx.Warn("Could not verify cluster access: %v", err) @@ -175,9 +174,9 @@ func checkClusterAccess(ctx *Context, cfg *clientconfig.Config, clusterName stri } // checkClusterAccessRBAC calls the cloud API to check if the user has RBAC permission to access a cluster -func checkClusterAccessRBAC(ctx *Context, config *clientconfig.Config, identity *clientconfig.IdentityConfig, clusterXID string) (bool, string, error) { - if identity.Type != "keypair" { - return false, "", fmt.Errorf("RBAC check is only supported for keypair identities") +func checkClusterAccessRBAC(ctx *Context, config *clientconfig.Config, identityName string, identity *clientconfig.IdentityConfig, clusterXID string) (bool, string, error) { + if identity.Type != "keypair" && identity.Type != "token" { + return false, "", fmt.Errorf("RBAC check is only supported for keypair and token identities") } // Get the issuer URL @@ -186,20 +185,8 @@ func checkClusterAccessRBAC(ctx *Context, config *clientconfig.Config, identity return false, "", fmt.Errorf("identity has no issuer configured") } - // Get the private key (handles both direct PrivateKey and KeyRef) - privateKeyPEM, err := config.GetPrivateKeyPEM(identity) - if err != nil { - return false, "", fmt.Errorf("failed to get private key: %w", err) - } - - // Load the private key - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) - if err != nil { - return false, "", fmt.Errorf("failed to load private key: %w", err) - } - // Get JWT token - token, err := clientconfig.AuthenticateWithKey(ctx, issuerURL, keyPair) + token, err := config.TokenForIdentity(ctx, identityName, identity, issuerURL) if err != nil { return false, "", fmt.Errorf("failed to authenticate: %w", err) } diff --git a/cli/commands/config_bind_helpers.go b/cli/commands/config_bind_helpers.go index 16af79fcb..3ace615f0 100644 --- a/cli/commands/config_bind_helpers.go +++ b/cli/commands/config_bind_helpers.go @@ -15,7 +15,6 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" "miren.dev/runtime/clientconfig" - "miren.dev/runtime/pkg/cloudauth" "miren.dev/runtime/pkg/theme" "miren.dev/runtime/pkg/ui" ) @@ -195,10 +194,12 @@ func isPrivateAddress(host string) bool { return ip.IsPrivate() } -// fetchAvailableClusters queries the identity server for available clusters -func fetchAvailableClusters(ctx *Context, config *clientconfig.Config, identity *clientconfig.IdentityConfig) ([]ClusterResponse, error) { - if identity.Type != "keypair" { - return nil, fmt.Errorf("cluster listing is only supported for keypair identities") +// fetchAvailableClusters queries the identity server for available clusters. +// identityName may be "" for an anonymous in-memory identity (e.g. during login +// before it has been named), in which case token refreshes are not persisted. +func fetchAvailableClusters(ctx *Context, config *clientconfig.Config, identityName string, identity *clientconfig.IdentityConfig) ([]ClusterResponse, error) { + if identity.Type != "keypair" && identity.Type != "token" { + return nil, fmt.Errorf("cluster listing is only supported for keypair and token identities") } // Get the issuer URL @@ -207,20 +208,8 @@ func fetchAvailableClusters(ctx *Context, config *clientconfig.Config, identity return nil, fmt.Errorf("identity has no issuer configured") } - // Get the private key (handles both direct PrivateKey and KeyRef) - privateKeyPEM, err := config.GetPrivateKeyPEM(identity) - if err != nil { - return nil, fmt.Errorf("failed to get private key: %w", err) - } - - // Load the private key - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) - if err != nil { - return nil, fmt.Errorf("failed to load private key: %w", err) - } - // Get JWT token - token, err := clientconfig.AuthenticateWithKey(ctx, issuerURL, keyPair) + token, err := config.TokenForIdentity(ctx, identityName, identity, issuerURL) if err != nil { return nil, fmt.Errorf("failed to authenticate: %w", err) } diff --git a/cli/commands/debug_connection.go b/cli/commands/debug_connection.go index d966e72aa..66f2ea6e4 100644 --- a/cli/commands/debug_connection.go +++ b/cli/commands/debug_connection.go @@ -12,7 +12,6 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/quic-go/quic-go/http3" "miren.dev/runtime/clientconfig" - "miren.dev/runtime/pkg/cloudauth" ) // DebugAuthResponse represents the response from the debug-auth endpoint @@ -78,6 +77,7 @@ func DebugConnection(ctx *Context, opts struct { ctx.Info("Step 2: Preparing authentication...") var identity *clientconfig.IdentityConfig + var identityName string // Determine which identity to use if opts.Identity != "" { @@ -88,6 +88,7 @@ func DebugConnection(ctx *Context, opts struct { ctx.Warn("Failed to get identity '%s': %v", opts.Identity, err) return err } + identityName = opts.Identity ctx.Info("Using identity: %s", opts.Identity) } else { ctx.Warn("No configuration found. Please run 'miren login' first.") @@ -108,6 +109,7 @@ func DebugConnection(ctx *Context, opts struct { ctx.Warn("Failed to get identity '%s' for cluster '%s': %v", cluster.Identity, opts.Cluster, err) return err } + identityName = cluster.Identity ctx.Info("Using identity '%s' from cluster '%s'", cluster.Identity, opts.Cluster) } else if cluster.CloudAuth && cluster.ClientKey != "" { // Backward compatibility: create identity from cluster @@ -142,30 +144,14 @@ func DebugConnection(ctx *Context, opts struct { if identity != nil { switch identity.Type { - case "keypair": - // For keypair auth, we need to get a JWT token - // The server we're testing should be the cluster, not the auth server - - // Get the private key (handles both direct PrivateKey and KeyRef) - var privateKeyPEM string - if config != nil { - privateKeyPEM, err = config.GetPrivateKeyPEM(identity) - if err != nil { - ctx.Warn("Failed to get private key: %v", err) - return err - } - } else if identity.PrivateKey != "" { - // Fallback for backward compatibility (when identity is created on-the-fly) - privateKeyPEM = identity.PrivateKey - } else { - ctx.Warn("Identity has no private key or key reference") - return fmt.Errorf("no private key available") - } - - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) - if err != nil { - ctx.Warn("Failed to load private key: %v", err) - return err + case "keypair", "token": + // Both keypair and ephemeral token identities resolve to a JWT bearer + // token. The server we're testing is the cluster, not the auth server. + if config == nil { + // On-the-fly identities (legacy CloudAuth) require a config to + // resolve keys; without one there is nothing to authenticate with. + ctx.Warn("No configuration found. Please run 'miren login' first.") + return fmt.Errorf("no configuration found") } // Use the issuer from the identity @@ -176,17 +162,17 @@ func DebugConnection(ctx *Context, opts struct { ctx.Warn("Identity has no issuer configured, using test server as auth server") } - ctx.Info("Requesting JWT token using keypair from %s...", authServerURL) + ctx.Info("Requesting JWT token for %s identity from %s...", identity.Type, authServerURL) // Note: This will use the cached token if available - token, err := clientconfig.AuthenticateWithKey(ctx, authServerURL, keyPair) + token, err := config.TokenForIdentity(ctx, identityName, identity, authServerURL) if err != nil { - ctx.Warn("Failed to authenticate with keypair: %v", err) + ctx.Warn("Failed to authenticate: %v", err) ctx.Info("Note: Ensure the auth server is reachable at %s", authServerURL) return err } authHeader = "Bearer " + token - authMethod = "keypair/jwt" + authMethod = identity.Type + "/jwt" ctx.Completed("JWT token obtained") // Decode and print JWT claims for debugging diff --git a/cli/commands/doctor.go b/cli/commands/doctor.go index 6eaeafbac..6cef34fb7 100644 --- a/cli/commands/doctor.go +++ b/cli/commands/doctor.go @@ -13,7 +13,6 @@ import ( "github.com/charmbracelet/lipgloss" "miren.dev/runtime/clientconfig" "miren.dev/runtime/pkg/auth" - "miren.dev/runtime/pkg/cloudauth" "miren.dev/runtime/pkg/theme" ) @@ -98,30 +97,20 @@ func tryAuthenticate(ctx *Context, cfg *clientconfig.Config, cluster *clientconf result.IdentityName = cluster.Identity switch identity.Type { - case "keypair": - privateKeyPEM, err := cfg.GetPrivateKeyPEM(identity) - if err != nil { - return result - } - - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) - if err != nil { - return result - } - + case "keypair", "token": authServer := identity.Issuer if authServer == "" { authServer = cluster.Hostname } authServer = normalizeAuthServerURL(authServer) - token, err := clientconfig.AuthenticateWithKey(ctx, authServer, keyPair) + token, err := cfg.TokenForIdentity(ctx, cluster.Identity, identity, authServer) if err != nil { return result } result.Claims, _ = auth.ParseUnverifiedClaims(token) - result.Method = "keypair" + result.Method = identity.Type // Fetch user info from cloud result.UserInfo, _ = fetchCloudUserInfo(ctx, authServer, token) diff --git a/cli/commands/global.go b/cli/commands/global.go index 65fb41bc3..c9c101ba7 100644 --- a/cli/commands/global.go +++ b/cli/commands/global.go @@ -446,7 +446,7 @@ func (c *Context) wrapRPCError(err error) error { clusterName = "the cluster" } - c.Warn("access denied: you don't have permission to access %s\nPlease check your credentials or request access from the cluster administrator", clusterName) + c.Warn("access denied: you don't have permission to access %s\nPlease check your credentials or request access from the cluster administrator.\nIf you were logged in previously, your session may have expired — run 'miren login'.", clusterName) return ErrAccessDenied } return err diff --git a/cli/commands/login.go b/cli/commands/login.go index cacbb3598..eb45f1222 100644 --- a/cli/commands/login.go +++ b/cli/commands/login.go @@ -11,7 +11,6 @@ import ( "net/http" "net/url" "os" - "strings" "time" "miren.dev/runtime/clientconfig" @@ -43,10 +42,20 @@ type DeviceFlowExchangeResponse struct { Error string `json:"error,omitempty"` ErrorDescription string `json:"error_description,omitempty"` AccessToken string `json:"access_token,omitempty"` + RefreshToken string `json:"refresh_token,omitempty"` TokenType string `json:"token_type,omitempty"` ExpiresIn int `json:"expires_in,omitempty"` } +// deviceFlowTokens carries the credentials returned by a successful device-flow +// exchange. The refresh token is present only when the cloud supports ephemeral +// login; older clouds return an empty RefreshToken. +type deviceFlowTokens struct { + AccessToken string + RefreshToken string + ExpiresIn int +} + // BeginKeyRegistrationRequest represents the request to begin key registration type BeginKeyRegistrationRequest struct { Name string `json:"name"` @@ -113,21 +122,22 @@ func getOrCreateKey(ctx *Context, keyName string) (*cloudauth.KeyPair, error) { // LoginWithDefaults runs the login flow with default settings func LoginWithDefaults(ctx *Context) error { - return login(ctx, "https://miren.cloud", "cloud", "miren-cli", false, false) + return login(ctx, "https://miren.cloud", "cloud", "miren-cli", false, false, false) } // Login authenticates with miren.cloud using device flow func Login(ctx *Context, opts struct { - CloudURL string `short:"u" long:"url" description:"Cloud URL" default:"https://miren.cloud"` - IdentityName string `short:"i" long:"identity" description:"Name for this identity in config" default:"cloud"` - KeyName string `short:"k" long:"key-name" description:"Name for the authentication key" default:"miren-cli"` - NoSave bool `long:"no-save" description:"Don't save credentials to config file"` - Force bool `short:"f" long:"force" description:"Overwrite existing identity without prompting"` + CloudURL string `short:"u" long:"url" description:"Cloud URL" default:"https://miren.cloud"` + IdentityName string `short:"i" long:"identity" description:"Name for this identity in config" default:"cloud"` + KeyName string `short:"k" long:"key-name" description:"Name for the authentication key" default:"miren-cli"` + NoSave bool `long:"no-save" description:"Don't save credentials to config file"` + Force bool `short:"f" long:"force" description:"Overwrite existing identity without prompting"` + PersistentKey bool `long:"persistent-key" description:"Register a persistent key instead of the default ephemeral token login"` }) error { - return login(ctx, opts.CloudURL, opts.IdentityName, opts.KeyName, opts.NoSave, opts.Force) + return login(ctx, opts.CloudURL, opts.IdentityName, opts.KeyName, opts.NoSave, opts.Force, opts.PersistentKey) } -func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force bool) error { +func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force, persistentKey bool) error { // Check for existing identity (unless forcing or not saving) if !force && !noSave { config, err := clientconfig.LoadConfig() @@ -185,6 +195,19 @@ func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force b } } + // Decide whether to use the persistent-key flow. Ephemeral token login is the + // default; --persistent-key opts back in. An existing keypair identity keeps + // its key on re-login so we never silently orphan a registered server-side key. + usePersistentKey := persistentKey + if !usePersistentKey { + if existing, err := clientconfig.LoadConfig(); err == nil && existing != nil { + if id, err := existing.GetIdentity(identityName); err == nil && id != nil && id.Type == "keypair" { + usePersistentKey = true + ctx.Info("Identity '%s' already uses a persistent key; keeping it.", identityName) + } + } + } + // Initialize device flow ctx.Info("Initiating device flow authentication...") initResp, err := initiateDeviceFlow(cloudURL) @@ -226,7 +249,7 @@ func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force b pollInterval = time.Duration(initResp.PollingInterval) * time.Second } - token, err := pollForToken(ctx, cloudURL, initResp.DeviceCode, pollInterval, timeout, func(status string) { + tokens, err := pollForToken(ctx, cloudURL, initResp.DeviceCode, pollInterval, timeout, func(status string) { if status == "pending" { fmt.Print(".") } @@ -239,64 +262,116 @@ func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force b ctx.Completed("Authentication successful!") - // Get or create a keypair for future authentication + // An ephemeral login needs a refresh token to renew without re-login. If the + // cloud is too old to return one, fall back to the persistent-key flow so + // login still produces durable credentials. + if !usePersistentKey && tokens.RefreshToken == "" { + ctx.Warn("Cloud did not return a refresh token; falling back to persistent key authentication.") + usePersistentKey = true + } + + if noSave { + return printUnsavedCredentials(ctx, tokens, usePersistentKey, keyName) + } + + if usePersistentKey { + return persistKeypairIdentity(ctx, identityName, cloudURL, keyName, tokens.AccessToken) + } + return persistTokenIdentity(ctx, identityName, cloudURL, tokens) +} + +// persistKeypairIdentity runs the persistent-key flow: register (or reuse) an +// ed25519 key with the cloud, store a keypair identity, and auto-configure a +// cluster. accessToken is the device-flow JWT used only to authorize key +// registration. +func persistKeypairIdentity(ctx *Context, identityName, cloudURL, keyName, accessToken string) error { keyPair, err := getOrCreateKey(ctx, keyName) if err != nil { - ctx.Warn("Failed to get or create keypair: %v", err) - ctx.Info("You can still use token authentication") - keyPair = nil - } else { - // Register the public key with the server (only if it's a new key) - ctx.Info("Registering public key with server...") - if err := registerPublicKey(cloudURL, token, keyPair, keyName); err != nil { - ctx.Warn("Failed to register public key: %v", err) - ctx.Info("You can still use token authentication") - keyPair = nil // Don't save the key if registration failed - } else { - ctx.Info("Public key registered successfully") + return fmt.Errorf("failed to get or create keypair: %w", err) + } + + ctx.Info("Registering public key with server...") + if err := registerPublicKey(cloudURL, accessToken, keyPair, keyName); err != nil { + return fmt.Errorf("failed to register public key: %w", err) + } + ctx.Info("Public key registered successfully") + + if err := saveKeyPairToConfig(identityName, cloudURL, keyPair, keyName); err != nil { + return fmt.Errorf("failed to save identity to config: %w", err) + } + ctx.Info("Identity '%s' saved to config", identityName) + ctx.Info("Future authentication will use the keypair (no login required)") + ctx.Info("") + + privateKeyPEM, err := keyPair.PrivateKeyPEM() + if err != nil { + return fmt.Errorf("failed to encode private key: %w", err) + } + identity := &clientconfig.IdentityConfig{ + Type: "keypair", + Issuer: clientconfig.NormalizeIssuerURL(cloudURL), + PrivateKey: privateKeyPEM, + } + maybeAutoConfigureCluster(ctx, identityName, cloudURL, identity) + return nil +} + +// persistTokenIdentity runs the ephemeral flow: store the device-flow access and +// refresh tokens as a "token" identity and auto-configure a cluster. No key is +// registered with the cloud, which is the whole point of ephemeral login. +func persistTokenIdentity(ctx *Context, identityName, cloudURL string, tokens *deviceFlowTokens) error { + issuer := clientconfig.NormalizeIssuerURL(cloudURL) + identity := &clientconfig.IdentityConfig{ + Type: "token", + Issuer: issuer, + Token: tokens.AccessToken, + RefreshToken: tokens.RefreshToken, + } + + if err := saveTokenIdentityToConfig(identityName, identity); err != nil { + return fmt.Errorf("failed to save identity to config: %w", err) + } + ctx.Info("Identity '%s' saved to config", identityName) + ctx.Info("Future authentication reuses the saved token (renewed automatically)") + ctx.Info("") + + maybeAutoConfigureCluster(ctx, identityName, cloudURL, identity) + return nil +} + +// maybeAutoConfigureCluster attempts cluster auto-configuration, logging only +// genuinely unexpected errors (not the expected "nothing to configure" cases). +func maybeAutoConfigureCluster(ctx *Context, identityName, cloudURL string, identity *clientconfig.IdentityConfig) { + if err := autoConfigureCluster(ctx, identityName, cloudURL, identity); err != nil { + if !errors.Is(err, ErrNoAutoConfigNeeded) && !errors.Is(err, ErrAutoConfigFailed) { + ctx.Info("Note: %v", err) } } +} - // Save to config unless --no-save is specified - if !noSave { - if keyPair != nil { - // Save identity with keypair for future authentication - if err := saveKeyPairToConfig(identityName, cloudURL, keyPair, keyName); err != nil { - ctx.Warn("Failed to save identity to config: %v", err) - } else { - ctx.Info("Identity '%s' saved to config", identityName) - ctx.Info("Future authentication will use the keypair (no login required)") +// printUnsavedCredentials handles --no-save: it prints the obtained credentials +// instead of persisting them. For a persistent-key login it prints the freshly +// generated key; otherwise it prints the access token (and warns about the +// refresh token, a 7-day credential, landing in terminal scrollback). +func printUnsavedCredentials(ctx *Context, tokens *deviceFlowTokens, usePersistentKey bool, keyName string) error { + if usePersistentKey { + keyPair, err := getOrCreateKey(ctx, keyName) + if err == nil { + if privateKeyPEM, err := keyPair.PrivateKeyPEM(); err == nil { + ctx.Info("Private key (not saved):") + ctx.Info("%s", privateKeyPEM) ctx.Info("") - - // Check if we should auto-configure a cluster - if err := autoConfigureCluster(ctx, identityName, cloudURL, keyPair); err != nil { - // Don't fail the login, just log if there's a real error - if !errors.Is(err, ErrNoAutoConfigNeeded) && !errors.Is(err, ErrAutoConfigFailed) { - // Only log unexpected errors, not expected ones - ctx.Info("Note: %v", err) - } - } } - } else { - // No keypair was registered - ctx.Warn("Authentication successful but no persistent credentials were saved") - ctx.Info("You can still use the token with --token flag:") - ctx.Info(" export MIREN_TOKEN=%s", token) } - } else { - if keyPair != nil { - privateKeyPEM, _ := keyPair.PrivateKeyPEM() - ctx.Info("Private key (not saved):") - ctx.Info("%s", privateKeyPEM) - ctx.Info("") - } - ctx.Info("Token (not saved):") - ctx.Info(" %s", token) - ctx.Info("") - ctx.Info("Export as environment variable:") - ctx.Info(" export MIREN_TOKEN=%s", token) } + ctx.Info("Access token (not saved):") + ctx.Info(" %s", tokens.AccessToken) + ctx.Info("") + if tokens.RefreshToken != "" { + ctx.Warn("A refresh token was issued but not printed; it is a long-lived credential.") + ctx.Info("Re-run 'miren login' without --no-save to store it securely.") + } return nil } @@ -347,10 +422,10 @@ func initiateDeviceFlow(cloudURL string) (*DeviceFlowInitResponse, error) { return &initResp, nil } -func pollForToken(ctx context.Context, cloudURL, deviceCode string, interval, maxDuration time.Duration, progress func(string)) (string, error) { +func pollForToken(ctx context.Context, cloudURL, deviceCode string, interval, maxDuration time.Duration, progress func(string)) (*deviceFlowTokens, error) { url, err := url.JoinPath(cloudURL, "/api/v1/device/token") if err != nil { - return "", fmt.Errorf("invalid cloud URL: %w", err) + return nil, fmt.Errorf("invalid cloud URL: %w", err) } reqBody := map[string]string{ @@ -361,7 +436,7 @@ func pollForToken(ctx context.Context, cloudURL, deviceCode string, interval, ma jsonData, err := json.Marshal(reqBody) if err != nil { - return "", err + return nil, err } client := &http.Client{Timeout: 30 * time.Second} @@ -376,13 +451,15 @@ func pollForToken(ctx context.Context, cloudURL, deviceCode string, interval, ma select { case <-timeoutCtx.Done(): if errors.Is(timeoutCtx.Err(), context.DeadlineExceeded) { - return "", fmt.Errorf("authentication timed out after %v: %w", maxDuration, timeoutCtx.Err()) + return nil, fmt.Errorf("authentication timed out after %v: %w", maxDuration, timeoutCtx.Err()) } - return "", timeoutCtx.Err() + return nil, timeoutCtx.Err() case <-ticker.C: - req, err := http.NewRequest("POST", url, bytes.NewBuffer(jsonData)) + // Bind each poll request to the timeout context so a hung server + // cannot outlive the overall deadline. + req, err := http.NewRequestWithContext(timeoutCtx, "POST", url, bytes.NewBuffer(jsonData)) if err != nil { - return "", err + return nil, err } req.Header.Set("Content-Type", "application/json") @@ -404,21 +481,25 @@ func pollForToken(ctx context.Context, cloudURL, deviceCode string, interval, ma // Server always returns 200 with status in JSON var exchangeResp DeviceFlowExchangeResponse if err := json.Unmarshal(body, &exchangeResp); err != nil { - return "", fmt.Errorf("failed to parse response: %w", err) + return nil, fmt.Errorf("failed to parse response: %w", err) } switch exchangeResp.Status { case "authorized": if exchangeResp.AccessToken == "" { - return "", fmt.Errorf("server returned authorized status but no token") + return nil, fmt.Errorf("server returned authorized status but no token") } - return exchangeResp.AccessToken, nil + return &deviceFlowTokens{ + AccessToken: exchangeResp.AccessToken, + RefreshToken: exchangeResp.RefreshToken, + ExpiresIn: exchangeResp.ExpiresIn, + }, nil case "denied": - return "", fmt.Errorf("authorization denied by user") + return nil, fmt.Errorf("authorization denied by user") case "expired": - return "", fmt.Errorf("device code expired") + return nil, fmt.Errorf("device code expired") case "pending": progress("pending") @@ -433,7 +514,7 @@ func pollForToken(ctx context.Context, cloudURL, deviceCode string, interval, ma case "authorization_pending": progress("pending") default: - return "", fmt.Errorf("server error: %s - %s", exchangeResp.Error, exchangeResp.ErrorDescription) + return nil, fmt.Errorf("server error: %s - %s", exchangeResp.Error, exchangeResp.ErrorDescription) } default: @@ -571,11 +652,10 @@ func saveKeyPairToConfig(identityName, cloudURL string, keyPair *cloudauth.KeyPa return fmt.Errorf("failed to encode private key: %w", err) } - // Parse cloud URL to get the issuer - issuer := strings.TrimSuffix(cloudURL, "/") - if !strings.HasPrefix(issuer, "http://") && !strings.HasPrefix(issuer, "https://") { - issuer = "https://" + issuer - } + // Parse cloud URL to get the issuer. Share one normalization rule with the + // token save path so a keypair and token identity for the same --url always + // store an identical issuer (notably http:// for loopback dev clouds). + issuer := clientconfig.NormalizeIssuerURL(cloudURL) // Load or create the main client config mainConfig, err := clientconfig.LoadConfig() @@ -636,10 +716,36 @@ func saveKeyPairToConfig(identityName, cloudURL string, keyPair *cloudauth.KeyPa return mainConfig.Save() } +// saveTokenIdentityToConfig persists an ephemeral "token" identity (access + +// refresh token) to clientconfig.d/identity-{name}.yaml. Unlike the keypair +// flow there is no separate key file — the credentials live entirely in the +// identity leaf. +func saveTokenIdentityToConfig(identityName string, identity *clientconfig.IdentityConfig) error { + mainConfig, err := clientconfig.LoadConfig() + if err != nil { + if err == clientconfig.ErrNoConfig { + mainConfig = clientconfig.NewConfig() + } else { + return fmt.Errorf("failed to load config: %w", err) + } + } + + leafConfigData := &clientconfig.ConfigData{ + Identities: map[string]*clientconfig.IdentityConfig{ + identityName: identity, + }, + } + mainConfig.SetLeafConfig("identity-"+identityName, leafConfigData) + + return mainConfig.Save() +} + // autoConfigureCluster checks if there are any local clusters configured, // and if not, fetches available clusters from the server and automatically -// configures the client if there's only one cluster available -func autoConfigureCluster(ctx *Context, identityName, cloudURL string, keyPair *cloudauth.KeyPair) error { +// configures the client if there's only one cluster available. The identity is +// an in-memory (unnamed) credential used to authenticate the cluster lookup; it +// carries either a direct private key (keypair) or device-flow tokens (token). +func autoConfigureCluster(ctx *Context, identityName, cloudURL string, identity *clientconfig.IdentityConfig) error { // Load the main config to check if any clusters are configured mainConfig, err := clientconfig.LoadConfig() if err != nil && err != clientconfig.ErrNoConfig { @@ -654,30 +760,13 @@ func autoConfigureCluster(ctx *Context, identityName, cloudURL string, keyPair * ctx.Info("Checking for available clusters...") - // Create identity config for fetching clusters - issuer := strings.TrimSuffix(cloudURL, "/") - if !strings.HasPrefix(issuer, "http://") && !strings.HasPrefix(issuer, "https://") { - issuer = "https://" + issuer - } - - privateKeyPEM, err := keyPair.PrivateKeyPEM() - if err != nil { - return fmt.Errorf("failed to encode private key: %w", err) - } - - identity := &clientconfig.IdentityConfig{ - Type: "keypair", - Issuer: issuer, - PrivateKey: privateKeyPEM, - } - - // Fetch available clusters from the server - // Pass mainConfig even though this identity has a direct PrivateKey (not KeyRef) - // The helper will handle both cases + // Fetch available clusters from the server. The identity is anonymous + // (name ""), so any token refresh it triggers is used for this call only and + // not persisted — fine, since the tokens are fresh from the device flow. if mainConfig == nil { mainConfig = clientconfig.NewConfig() } - clusters, err := fetchAvailableClusters(ctx, mainConfig, identity) + clusters, err := fetchAvailableClusters(ctx, mainConfig, "", identity) if err != nil { return fmt.Errorf("failed to fetch available clusters: %w", err) } @@ -788,18 +877,7 @@ func getIdentityUserInfo(ctx *Context, config *clientconfig.Config, identityName return "" } - if identity.Type != "keypair" { - return "" - } - - // Get the private key - privateKeyPEM, err := config.GetPrivateKeyPEM(identity) - if err != nil { - return "" - } - - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) - if err != nil { + if identity.Type != "keypair" && identity.Type != "token" { return "" } @@ -810,7 +888,7 @@ func getIdentityUserInfo(ctx *Context, config *clientconfig.Config, identityName } // Authenticate and get JWT token - token, err := clientconfig.AuthenticateWithKey(ctx, authServer, keyPair) + token, err := config.TokenForIdentity(ctx, identityName, identity, authServer) if err != nil { return "" } diff --git a/cli/commands/login_deviceflow_test.go b/cli/commands/login_deviceflow_test.go new file mode 100644 index 000000000..fbd3b63cd --- /dev/null +++ b/cli/commands/login_deviceflow_test.go @@ -0,0 +1,128 @@ +package commands + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// deviceTokenServer serves a scripted sequence of /api/v1/device/token +// responses so pollForToken's state machine can be exercised deterministically. +func deviceTokenServer(t *testing.T, responses []map[string]any) *httptest.Server { + t.Helper() + var idx atomic.Int32 + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/device/token", r.URL.Path) + i := int(idx.Add(1)) - 1 + if i >= len(responses) { + i = len(responses) - 1 // repeat the last response + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(responses[i]) + })) +} + +func TestPollForToken(t *testing.T) { + interval := 5 * time.Millisecond + timeout := 2 * time.Second + + t.Run("pending then authorized returns access and refresh tokens", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{ + {"status": "pending"}, + {"status": "pending"}, + { + "status": "authorized", + "access_token": "access-jwt", + "refresh_token": "refresh-jwt", + "token_type": "Bearer", + "expires_in": 3600, + }, + }) + defer srv.Close() + + var pendings atomic.Int32 + tokens, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(status string) { + if status == "pending" { + pendings.Add(1) + } + }) + require.NoError(t, err) + require.Equal(t, "access-jwt", tokens.AccessToken) + require.Equal(t, "refresh-jwt", tokens.RefreshToken) + require.Equal(t, 3600, tokens.ExpiresIn) + require.GreaterOrEqual(t, pendings.Load(), int32(2)) + }) + + t.Run("authorized without refresh token (older cloud)", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{ + {"status": "authorized", "access_token": "access-only", "token_type": "Bearer", "expires_in": 3600}, + }) + defer srv.Close() + + tokens, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(string) {}) + require.NoError(t, err) + require.Equal(t, "access-only", tokens.AccessToken) + require.Empty(t, tokens.RefreshToken) + }) + + t.Run("denied", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{{"status": "denied"}}) + defer srv.Close() + + _, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(string) {}) + require.ErrorContains(t, err, "denied") + }) + + t.Run("expired", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{{"status": "expired"}}) + defer srv.Close() + + _, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(string) {}) + require.ErrorContains(t, err, "expired") + }) + + t.Run("authorized with empty access token errors", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{{"status": "authorized", "access_token": ""}}) + defer srv.Close() + + _, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(string) {}) + require.Error(t, err) + }) + + t.Run("slow_down then authorized", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{ + {"status": "error", "error": "slow_down"}, + {"status": "authorized", "access_token": "access-jwt", "refresh_token": "refresh-jwt"}, + }) + defer srv.Close() + + tokens, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(string) {}) + require.NoError(t, err) + require.Equal(t, "access-jwt", tokens.AccessToken) + }) + + t.Run("unknown error status is fatal", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{ + {"status": "error", "error": "boom", "error_description": "kaboom"}, + }) + defer srv.Close() + + _, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, timeout, func(string) {}) + require.ErrorContains(t, err, "boom") + }) + + t.Run("times out while pending", func(t *testing.T) { + srv := deviceTokenServer(t, []map[string]any{{"status": "pending"}}) + defer srv.Close() + + _, err := pollForToken(context.Background(), srv.URL, "dev-code", interval, 40*time.Millisecond, func(string) {}) + require.ErrorContains(t, err, "timed out") + }) +} diff --git a/cli/commands/login_ephemeral_test.go b/cli/commands/login_ephemeral_test.go new file mode 100644 index 000000000..d02e35a7b --- /dev/null +++ b/cli/commands/login_ephemeral_test.go @@ -0,0 +1,168 @@ +package commands + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" + + "miren.dev/runtime/clientconfig" +) + +// mockCloud is a fake miren.cloud implementing just enough of the device flow, +// key registration, and cluster listing for the login flow to run end-to-end. +type mockCloud struct { + keyBeginHits atomic.Int32 + keyCompleteHits atomic.Int32 + withRefresh bool +} + +func freshLoginJWT(t *testing.T) string { + t.Helper() + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ + "exp": float64(time.Now().Add(time.Hour).Unix()), + "sub": "user@example.com", + }) + s, err := tok.SignedString([]byte("secret")) + require.NoError(t, err) + return s +} + +func (m *mockCloud) server(t *testing.T) *httptest.Server { + mux := http.NewServeMux() + + mux.HandleFunc("/api/v1/device/code", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "device_code": "dev-code", + "user_code": "ABCD-1234", + "verification_uri": "https://example.com/verify", + "expires_in": 600, + "polling_interval": 1, + }) + }) + + mux.HandleFunc("/api/v1/device/token", func(w http.ResponseWriter, r *http.Request) { + resp := map[string]any{ + "status": "authorized", + "access_token": freshLoginJWT(t), + "token_type": "Bearer", + "expires_in": 3600, + } + if m.withRefresh { + resp["refresh_token"] = freshLoginJWT(t) + } + _ = json.NewEncoder(w).Encode(resp) + }) + + mux.HandleFunc("/api/v1/users/keys/begin", func(w http.ResponseWriter, r *http.Request) { + m.keyBeginHits.Add(1) + // 409 is treated as "already registered" success by registerPublicKey, + // which keeps the persistent-key test from needing a signing round-trip. + w.WriteHeader(http.StatusConflict) + }) + mux.HandleFunc("/api/v1/users/keys/complete", func(w http.ResponseWriter, r *http.Request) { + m.keyCompleteHits.Add(1) + w.WriteHeader(http.StatusOK) + }) + + mux.HandleFunc("/api/v1/users/clusters", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"clusters": []any{}}) + }) + + return httptest.NewServer(mux) +} + +// TestLoginEphemeralDefault is the MIR-1385 acceptance test: a default login must +// store a token identity and never touch the key-registration endpoints. +func TestLoginEphemeralDefault(t *testing.T) { + mc := &mockCloud{withRefresh: true} + srv := mc.server(t) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + + ctx := newTestContext() + err := login(ctx, srv.URL, "cloud", "miren-cli", false, false, false) + require.NoError(t, err) + + // The whole point: no persistent key registered. + require.Zero(t, mc.keyBeginHits.Load(), "ephemeral login must not begin key registration") + require.Zero(t, mc.keyCompleteHits.Load(), "ephemeral login must not complete key registration") + + // Identity leaf is a token identity carrying both tokens, at mode 0600. + leaf := filepath.Join(dir, "clientconfig.d", "identity-cloud.yaml") + info, err := os.Stat(leaf) + require.NoError(t, err) + require.Equal(t, os.FileMode(0600), info.Mode().Perm()) + + cfg, err := clientconfig.LoadConfig() + require.NoError(t, err) + id, err := cfg.GetIdentity("cloud") + require.NoError(t, err) + require.Equal(t, "token", id.Type) + require.NotEmpty(t, id.Token) + require.NotEmpty(t, id.RefreshToken) + + // No key file should have been written. + _, err = os.Stat(filepath.Join(dir, "clientconfig.d", "key-miren-cli.yaml")) + require.True(t, os.IsNotExist(err), "ephemeral login must not write a key file") +} + +// TestLoginPersistentKeyFlag verifies --persistent-key still registers a key and +// stores a keypair identity. +func TestLoginPersistentKeyFlag(t *testing.T) { + mc := &mockCloud{withRefresh: true} + srv := mc.server(t) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + + ctx := newTestContext() + err := login(ctx, srv.URL, "cloud", "miren-cli", false, false, true /* persistentKey */) + require.NoError(t, err) + + require.Equal(t, int32(1), mc.keyBeginHits.Load(), "persistent-key login must register a key") + + cfg, err := clientconfig.LoadConfig() + require.NoError(t, err) + id, err := cfg.GetIdentity("cloud") + require.NoError(t, err) + require.Equal(t, "keypair", id.Type) + require.Empty(t, id.Token) + + _, err = os.Stat(filepath.Join(dir, "clientconfig.d", "key-miren-cli.yaml")) + require.NoError(t, err, "persistent-key login must write a key file") +} + +// TestLoginFallsBackWhenNoRefreshToken verifies that if the cloud returns no +// refresh token, a default login falls back to the persistent-key flow rather +// than storing an unusable token identity. +func TestLoginFallsBackWhenNoRefreshToken(t *testing.T) { + mc := &mockCloud{withRefresh: false} + srv := mc.server(t) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + + ctx := newTestContext() + err := login(ctx, srv.URL, "cloud", "miren-cli", false, false, false) + require.NoError(t, err) + + require.Equal(t, int32(1), mc.keyBeginHits.Load(), "should fall back to key registration") + + cfg, err := clientconfig.LoadConfig() + require.NoError(t, err) + id, err := cfg.GetIdentity("cloud") + require.NoError(t, err) + require.Equal(t, "keypair", id.Type) +} diff --git a/cli/commands/logout.go b/cli/commands/logout.go index 50b9539d6..b242e6be6 100644 --- a/cli/commands/logout.go +++ b/cli/commands/logout.go @@ -112,6 +112,17 @@ func Logout(ctx *Context, opts struct { } } + // For ephemeral token identities, best-effort revoke the refresh token so a + // leaked ~/.config copy can't keep renewing after logout. Never let a failed + // revoke block the local logout. + if identity.Type == "token" && identity.RefreshToken != "" { + if err := clientconfig.RevokeRefreshToken(ctx, identity.Issuer, identity.Token, identity.RefreshToken); err != nil { + ctx.Warn("Could not revoke refresh token on the server (removing local credentials anyway): %v", err) + } else { + ctx.Info("Revoked refresh token on the server") + } + } + // Delete the identity file if err := os.Remove(identityFile); err != nil { if !os.IsNotExist(err) { diff --git a/cli/commands/logout_revoke_test.go b/cli/commands/logout_revoke_test.go new file mode 100644 index 000000000..b24353852 --- /dev/null +++ b/cli/commands/logout_revoke_test.go @@ -0,0 +1,95 @@ +package commands + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" + + "miren.dev/runtime/clientconfig" +) + +// writeTokenIdentity writes a token identity leaf into a MIREN_CONFIG dir. +func writeTokenIdentity(t *testing.T, dir, issuer string) { + t.Helper() + cd := clientconfig.ConfigData{ + Identities: map[string]*clientconfig.IdentityConfig{ + "cloud": { + Type: "token", + Issuer: issuer, + Token: "access-jwt", + RefreshToken: "refresh-jwt", + }, + }, + } + data, err := yaml.Marshal(&cd) + require.NoError(t, err) + leafDir := filepath.Join(dir, "clientconfig.d") + require.NoError(t, os.MkdirAll(leafDir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(leafDir, "identity-cloud.yaml"), data, 0600)) +} + +func runLogout(t *testing.T) error { + t.Helper() + return Logout(newTestContext(), struct { + ConfigCentric + IdentityName string `short:"i" long:"identity" description:"Name of the identity to remove"` + }{IdentityName: "cloud"}) +} + +func TestLogoutRevokesRefreshToken(t *testing.T) { + var revokeHits atomic.Int32 + var gotAuth, gotRefresh string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/auth/revoke/refresh" { + revokeHits.Add(1) + gotAuth = r.Header.Get("Authorization") + var body struct { + RefreshToken string `json:"refresh_token"` + Reason string `json:"reason"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotRefresh = body.RefreshToken + w.WriteHeader(http.StatusOK) + return + } + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + writeTokenIdentity(t, dir, srv.URL) + + require.NoError(t, runLogout(t)) + + require.Equal(t, int32(1), revokeHits.Load(), "logout should revoke the refresh token") + require.Equal(t, "Bearer access-jwt", gotAuth) + require.Equal(t, "refresh-jwt", gotRefresh) + + // The identity file must be gone regardless. + _, err := os.Stat(filepath.Join(dir, "clientconfig.d", "identity-cloud.yaml")) + require.True(t, os.IsNotExist(err)) +} + +func TestLogoutSucceedsWhenRevokeFails(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) // revoke fails + })) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + writeTokenIdentity(t, dir, srv.URL) + + // A failed revoke must not block logout. + require.NoError(t, runLogout(t)) + _, err := os.Stat(filepath.Join(dir, "clientconfig.d", "identity-cloud.yaml")) + require.True(t, os.IsNotExist(err), "identity file should still be removed") +} diff --git a/cli/commands/whoami.go b/cli/commands/whoami.go index bff860278..8d571395c 100644 --- a/cli/commands/whoami.go +++ b/cli/commands/whoami.go @@ -2,11 +2,9 @@ package commands import ( "fmt" - "strings" "miren.dev/runtime/clientconfig" "miren.dev/runtime/pkg/auth" - "miren.dev/runtime/pkg/cloudauth" ) // Whoami displays information about the current authenticated user @@ -33,43 +31,18 @@ func Whoami(ctx *Context, opts struct { if ctx.ClusterConfig.Identity != "" && ctx.ClientConfig != nil { var err error identity, err = ctx.ClientConfig.GetIdentity(ctx.ClusterConfig.Identity) - if err == nil && identity != nil && identity.Type == "keypair" { - // Get the private key (handles both direct PrivateKey and KeyRef) - privateKeyPEM, err := ctx.ClientConfig.GetPrivateKeyPEM(identity) - if err != nil { - ctx.Warn("Failed to get private key: %v", err) - } else { - keyPair, err := cloudauth.LoadKeyPairFromPEM(privateKeyPEM) + if err == nil && identity != nil { + switch identity.Type { + case "keypair", "token": + token, err = ctx.ClientConfig.TokenForIdentity(ctx, ctx.ClusterConfig.Identity, identity, hostname) if err != nil { - ctx.Warn("Failed to load keypair: %v", err) - } else { - // Get the auth server URL - authServer := identity.Issuer - if authServer == "" { - authServer = hostname - } - - // For local development, use HTTP - if !strings.HasPrefix(authServer, "http://") && !strings.HasPrefix(authServer, "https://") { - if strings.Contains(authServer, "localhost") || strings.Contains(authServer, "127.0.0.1") { - authServer = "http://" + authServer - } else { - authServer = "https://" + authServer - } - } - - // Get JWT token - token, err = clientconfig.AuthenticateWithKey(ctx, authServer, keyPair) - if err != nil { - return fmt.Errorf("failed to authenticate with keypair: %w", err) - } - authMethod = "keypair" + return fmt.Errorf("failed to authenticate: %w", err) } + authMethod = identity.Type + case "certificate": + authMethod = "certificate" } } - if identity != nil && identity.Type == "certificate" { - authMethod = "certificate" - } } // Try to parse JWT claims if we have a token diff --git a/docs/docs/command/login.md b/docs/docs/command/login.md index 76af04eac..9d96acbee 100644 --- a/docs/docs/command/login.md +++ b/docs/docs/command/login.md @@ -20,6 +20,7 @@ miren login [flags] - `--identity, -i` — Name for this identity in config (default: `cloud`) - `--key-name, -k` — Name for the authentication key (default: `miren-cli`) - `--no-save` — Don't save credentials to config file +- `--persistent-key` — Register a persistent key instead of the default ephemeral token login - `--url, -u` — Cloud URL (default: `https://miren.cloud`) ## Global Options From bf6b57e046f65cbfe827c3951f74ad601de4494c Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 23 Jul 2026 17:38:20 -0700 Subject: [PATCH 3/5] address review: register key under --no-save, make revoke work The refactor that split login into persist/print paths moved key registration inside the persisted branch, so --no-save --persistent-key printed a private key the cloud had never seen. Register before printing, since an unregistered key cannot authenticate anything. Logout revocation had a similar hole in practice: the stored access token lives an hour and is usually expired by the time anyone logs out, so the revoke call would quietly fail and the refresh token would stay live for its full week. Get a live token first, then revoke whatever refresh token that left current, and only claim success when a request actually succeeded. RevokeRefreshToken now says so rather than returning nil when it has no bearer to authorize with. Also take the refresh lock with the caller context so a cancelled command stops waiting, and synchronize the revoke test capture. --- cli/commands/login.go | 28 ++++++++++++----- cli/commands/login_ephemeral_test.go | 43 ++++++++++++++++++++++++++ cli/commands/logout.go | 35 ++++++++++++++++++--- cli/commands/logout_revoke_test.go | 46 ++++++++++++++++++++++------ clientconfig/token.go | 22 ++++++++++--- clientconfig/token_test.go | 12 ++++++-- 6 files changed, 158 insertions(+), 28 deletions(-) diff --git a/cli/commands/login.go b/cli/commands/login.go index eb45f1222..66349b786 100644 --- a/cli/commands/login.go +++ b/cli/commands/login.go @@ -271,7 +271,7 @@ func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force, } if noSave { - return printUnsavedCredentials(ctx, tokens, usePersistentKey, keyName) + return printUnsavedCredentials(ctx, cloudURL, tokens, usePersistentKey, keyName) } if usePersistentKey { @@ -353,16 +353,28 @@ func maybeAutoConfigureCluster(ctx *Context, identityName, cloudURL string, iden // instead of persisting them. For a persistent-key login it prints the freshly // generated key; otherwise it prints the access token (and warns about the // refresh token, a 7-day credential, landing in terminal scrollback). -func printUnsavedCredentials(ctx *Context, tokens *deviceFlowTokens, usePersistentKey bool, keyName string) error { +func printUnsavedCredentials(ctx *Context, cloudURL string, tokens *deviceFlowTokens, usePersistentKey bool, keyName string) error { if usePersistentKey { keyPair, err := getOrCreateKey(ctx, keyName) - if err == nil { - if privateKeyPEM, err := keyPair.PrivateKeyPEM(); err == nil { - ctx.Info("Private key (not saved):") - ctx.Info("%s", privateKeyPEM) - ctx.Info("") - } + if err != nil { + return fmt.Errorf("failed to get or create keypair: %w", err) + } + + // Register even though we aren't saving: a private key the cloud has + // never seen cannot authenticate, so printing it unregistered would hand + // the user a credential that silently fails later. + ctx.Info("Registering public key with server...") + if err := registerPublicKey(cloudURL, tokens.AccessToken, keyPair, keyName); err != nil { + return fmt.Errorf("failed to register public key: %w", err) } + + privateKeyPEM, err := keyPair.PrivateKeyPEM() + if err != nil { + return fmt.Errorf("failed to encode private key: %w", err) + } + ctx.Info("Private key (not saved):") + ctx.Info("%s", privateKeyPEM) + ctx.Info("") } ctx.Info("Access token (not saved):") diff --git a/cli/commands/login_ephemeral_test.go b/cli/commands/login_ephemeral_test.go index d02e35a7b..6777be2e8 100644 --- a/cli/commands/login_ephemeral_test.go +++ b/cli/commands/login_ephemeral_test.go @@ -143,6 +143,49 @@ func TestLoginPersistentKeyFlag(t *testing.T) { require.NoError(t, err, "persistent-key login must write a key file") } +// TestLoginNoSavePersistentKeyRegisters guards a regression: --no-save with +// --persistent-key printed a freshly generated private key without ever +// registering the public half, so the key it handed the user could not +// authenticate anything. +func TestLoginNoSavePersistentKeyRegisters(t *testing.T) { + mc := &mockCloud{withRefresh: true} + srv := mc.server(t) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + + ctx := newTestContext() + err := login(ctx, srv.URL, "cloud", "miren-cli", true /* noSave */, false, true /* persistentKey */) + require.NoError(t, err) + + require.Equal(t, int32(1), mc.keyBeginHits.Load(), + "a printed key must be registered with the cloud or it cannot authenticate") + + // --no-save must still persist nothing. + _, err = os.Stat(filepath.Join(dir, "clientconfig.d")) + require.True(t, os.IsNotExist(err), "--no-save must not write any config") +} + +// TestLoginNoSaveEphemeralSkipsKeyRegistration verifies the token path stays +// registration-free under --no-save. +func TestLoginNoSaveEphemeralSkipsKeyRegistration(t *testing.T) { + mc := &mockCloud{withRefresh: true} + srv := mc.server(t) + defer srv.Close() + + dir := t.TempDir() + t.Setenv("MIREN_CONFIG", dir) + + ctx := newTestContext() + err := login(ctx, srv.URL, "cloud", "miren-cli", true /* noSave */, false, false) + require.NoError(t, err) + + require.Zero(t, mc.keyBeginHits.Load(), "ephemeral --no-save must not register a key") + _, err = os.Stat(filepath.Join(dir, "clientconfig.d")) + require.True(t, os.IsNotExist(err), "--no-save must not write any config") +} + // TestLoginFallsBackWhenNoRefreshToken verifies that if the cloud returns no // refresh token, a default login falls back to the persistent-key flow rather // than storing an unusable token identity. diff --git a/cli/commands/logout.go b/cli/commands/logout.go index b242e6be6..f09e8dc19 100644 --- a/cli/commands/logout.go +++ b/cli/commands/logout.go @@ -116,11 +116,7 @@ func Logout(ctx *Context, opts struct { // leaked ~/.config copy can't keep renewing after logout. Never let a failed // revoke block the local logout. if identity.Type == "token" && identity.RefreshToken != "" { - if err := clientconfig.RevokeRefreshToken(ctx, identity.Issuer, identity.Token, identity.RefreshToken); err != nil { - ctx.Warn("Could not revoke refresh token on the server (removing local credentials anyway): %v", err) - } else { - ctx.Info("Revoked refresh token on the server") - } + revokeIdentitySession(ctx, cfg, identityName, identity) } // Delete the identity file @@ -149,6 +145,35 @@ func Logout(ctx *Context, opts struct { return nil } +// revokeIdentitySession best-effort revokes a token identity's refresh token so +// a copied config can't keep renewing after logout. +// +// The stored access token has a one hour life and is usually already expired by +// the time someone logs out, and the revoke endpoint needs a valid bearer. So we +// ask for a live token first, which may rotate the pair on disk; we then re-read +// the identity so we revoke the refresh token that is actually current rather +// than the one we just spent. +func revokeIdentitySession(ctx *Context, cfg *clientconfig.Config, identityName string, identity *clientconfig.IdentityConfig) { + accessToken, err := cfg.TokenForIdentity(ctx, identityName, identity, identity.Issuer) + if err != nil { + ctx.Warn("Could not obtain a valid token to revoke the session (removing local credentials anyway): %v", err) + return + } + + refreshToken := identity.RefreshToken + if reloaded, err := clientconfig.LoadConfig(); err == nil { + if current, err := reloaded.GetIdentity(identityName); err == nil && current.RefreshToken != "" { + refreshToken = current.RefreshToken + } + } + + if err := clientconfig.RevokeRefreshToken(ctx, identity.Issuer, accessToken, refreshToken); err != nil { + ctx.Warn("Could not revoke refresh token on the server (removing local credentials anyway): %v", err) + return + } + ctx.Info("Revoked refresh token on the server") +} + // getConfigDirPath returns the path to the clientconfig.d directory func getConfigDirPath() (string, error) { // Check environment variable first diff --git a/cli/commands/logout_revoke_test.go b/cli/commands/logout_revoke_test.go index b24353852..46dd08b02 100644 --- a/cli/commands/logout_revoke_test.go +++ b/cli/commands/logout_revoke_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sync" "sync/atomic" "testing" @@ -15,15 +16,18 @@ import ( "miren.dev/runtime/clientconfig" ) -// writeTokenIdentity writes a token identity leaf into a MIREN_CONFIG dir. -func writeTokenIdentity(t *testing.T, dir, issuer string) { +// writeTokenIdentity writes a token identity leaf into a MIREN_CONFIG dir and +// returns the access token it stored. The access token is a genuinely fresh JWT +// so logout's revoke path can use it directly instead of trying to refresh. +func writeTokenIdentity(t *testing.T, dir, issuer string) string { t.Helper() + accessToken := freshLoginJWT(t) cd := clientconfig.ConfigData{ Identities: map[string]*clientconfig.IdentityConfig{ "cloud": { Type: "token", Issuer: issuer, - Token: "access-jwt", + Token: accessToken, RefreshToken: "refresh-jwt", }, }, @@ -33,6 +37,7 @@ func writeTokenIdentity(t *testing.T, dir, issuer string) { leafDir := filepath.Join(dir, "clientconfig.d") require.NoError(t, os.MkdirAll(leafDir, 0755)) require.NoError(t, os.WriteFile(filepath.Join(leafDir, "identity-cloud.yaml"), data, 0600)) + return accessToken } func runLogout(t *testing.T) error { @@ -43,19 +48,40 @@ func runLogout(t *testing.T) error { }{IdentityName: "cloud"}) } +// revokeCapture records what the fake revoke endpoint saw. The handler runs on +// the server's goroutine while the test body reads, so it needs a mutex. +type revokeCapture struct { + mu sync.Mutex + auth string + refresh string + reason string +} + +func (c *revokeCapture) record(auth, refresh, reason string) { + c.mu.Lock() + defer c.mu.Unlock() + c.auth, c.refresh, c.reason = auth, refresh, reason +} + +func (c *revokeCapture) get() (auth, refresh, reason string) { + c.mu.Lock() + defer c.mu.Unlock() + return c.auth, c.refresh, c.reason +} + func TestLogoutRevokesRefreshToken(t *testing.T) { var revokeHits atomic.Int32 - var gotAuth, gotRefresh string + var capture revokeCapture + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/auth/revoke/refresh" { revokeHits.Add(1) - gotAuth = r.Header.Get("Authorization") var body struct { RefreshToken string `json:"refresh_token"` Reason string `json:"reason"` } _ = json.NewDecoder(r.Body).Decode(&body) - gotRefresh = body.RefreshToken + capture.record(r.Header.Get("Authorization"), body.RefreshToken, body.Reason) w.WriteHeader(http.StatusOK) return } @@ -65,13 +91,15 @@ func TestLogoutRevokesRefreshToken(t *testing.T) { dir := t.TempDir() t.Setenv("MIREN_CONFIG", dir) - writeTokenIdentity(t, dir, srv.URL) + accessToken := writeTokenIdentity(t, dir, srv.URL) require.NoError(t, runLogout(t)) require.Equal(t, int32(1), revokeHits.Load(), "logout should revoke the refresh token") - require.Equal(t, "Bearer access-jwt", gotAuth) - require.Equal(t, "refresh-jwt", gotRefresh) + auth, refresh, reason := capture.get() + require.Equal(t, "Bearer "+accessToken, auth, "revoke must authenticate with a live access token") + require.Equal(t, "refresh-jwt", refresh) + require.Equal(t, "logout", reason) // The identity file must be gone regardless. _, err := os.Stat(filepath.Join(dir, "clientconfig.d", "identity-cloud.yaml")) diff --git a/clientconfig/token.go b/clientconfig/token.go index d74d1cd11..2e2ea84dc 100644 --- a/clientconfig/token.go +++ b/clientconfig/token.go @@ -213,9 +213,11 @@ func (c *Config) tokenForTokenIdentity(ctx context.Context, name string, identit return pair.AccessToken, nil } + // Refreshing rotates the pair, so we need a file to write the new one back to. + // An identity that was never loaded from disk (an in-memory config) has none. source := c.GetIdentitySource(name) if source == "" { - return "", fmt.Errorf("cannot locate config file for identity %q", name) + return "", fmt.Errorf("identity %q has no config file on disk to store refreshed tokens in; run 'miren login' to save it", name) } // Serialize refreshes across processes. The refresh endpoint consumes the @@ -228,9 +230,15 @@ func (c *Config) tokenForTokenIdentity(ctx context.Context, name string, identit // removed on unlock: unlinking a flock target lets a concurrent waiter and a // new process end up locking two different inodes, defeating the mutex. lock := flock.New(lockPathFor(source)) - if err := lock.Lock(); err != nil { + // TryLockContext rather than Lock so a cancelled command or an expiring + // deadline stops waiting promptly instead of blocking on a peer's refresh. + locked, err := lock.TryLockContext(ctx, 50*time.Millisecond) + if err != nil { return "", fmt.Errorf("failed to acquire token refresh lock: %w", err) } + if !locked { + return "", fmt.Errorf("failed to acquire token refresh lock for identity %q", name) + } defer func() { _ = lock.Unlock() }() // Re-read from disk inside the lock: a peer may have already rotated while we @@ -316,8 +324,14 @@ func persistIdentityTokens(path, name, accessToken, refreshToken string) error { // bearer credential. Any error is returned to the caller, which should treat // revocation as advisory — a failed revoke must never block logout. func RevokeRefreshToken(ctx context.Context, issuer, accessToken, refreshToken string) error { - if refreshToken == "" || accessToken == "" { - return nil + if refreshToken == "" { + return nil // Nothing to revoke. + } + if accessToken == "" { + // The endpoint authenticates with the access token, so without one we + // cannot revoke. Report it rather than returning nil, which would let the + // caller claim a revocation that never happened. + return errors.New("no access token available to authorize revocation") } revokeURL, err := url.JoinPath(NormalizeIssuerURL(issuer), "/auth/revoke/refresh") diff --git a/clientconfig/token_test.go b/clientconfig/token_test.go index a09db2413..74d04d857 100644 --- a/clientconfig/token_test.go +++ b/clientconfig/token_test.go @@ -192,8 +192,16 @@ func TestRevokeRefreshToken(t *testing.T) { require.Equal(t, "logout", gotReason) }) - t.Run("empty tokens are a no-op", func(t *testing.T) { - require.NoError(t, RevokeRefreshToken(context.Background(), "https://x", "", "")) + t.Run("no refresh token is a no-op", func(t *testing.T) { + require.NoError(t, RevokeRefreshToken(context.Background(), "https://x", "access", "")) + }) + + t.Run("missing access token reports failure rather than silent success", func(t *testing.T) { + // Returning nil here would let the caller claim it revoked a token when + // no request was ever made. + err := RevokeRefreshToken(context.Background(), "https://x", "", "refresh-y") + require.Error(t, err) + require.Contains(t, err.Error(), "no access token") }) t.Run("non-2xx is an error", func(t *testing.T) { From 6a956b0eaf2d2cd9c71c9a93551d742f7126cafe Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 23 Jul 2026 17:49:55 -0700 Subject: [PATCH 4/5] address review: exercise the full key registration round trip The mock cloud short-circuited key registration with a 409, so the tests only proved that registration began. Have it return a real envelope and challenge and check the signature comes back, then assert completion too, so the registration tests prove a key is actually usable rather than merely attempted. --- cli/commands/login_ephemeral_test.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/cli/commands/login_ephemeral_test.go b/cli/commands/login_ephemeral_test.go index 6777be2e8..6efa17b45 100644 --- a/cli/commands/login_ephemeral_test.go +++ b/cli/commands/login_ephemeral_test.go @@ -1,6 +1,7 @@ package commands import ( + "encoding/base64" "encoding/json" "net/http" "net/http/httptest" @@ -61,13 +62,24 @@ func (m *mockCloud) server(t *testing.T) *httptest.Server { _ = json.NewEncoder(w).Encode(resp) }) + // Exercise the real begin -> sign -> complete round trip rather than the 409 + // "already registered" shortcut, so tests can prove a key was fully + // registered and not merely that begin was called. mux.HandleFunc("/api/v1/users/keys/begin", func(w http.ResponseWriter, r *http.Request) { m.keyBeginHits.Add(1) - // 409 is treated as "already registered" success by registerPublicKey, - // which keeps the persistent-key test from needing a signing round-trip. - w.WriteHeader(http.StatusConflict) + _ = json.NewEncoder(w).Encode(map[string]any{ + "envelope": "test-envelope", + "challenge": base64.StdEncoding.EncodeToString([]byte("challenge-bytes")), + }) }) mux.HandleFunc("/api/v1/users/keys/complete", func(w http.ResponseWriter, r *http.Request) { + var req struct { + Envelope string `json:"envelope"` + Signature string `json:"signature"` + } + _ = json.NewDecoder(r.Body).Decode(&req) + require.Equal(t, "test-envelope", req.Envelope, "complete must echo the envelope from begin") + require.NotEmpty(t, req.Signature, "complete must carry a signature over the challenge") m.keyCompleteHits.Add(1) w.WriteHeader(http.StatusOK) }) @@ -131,6 +143,7 @@ func TestLoginPersistentKeyFlag(t *testing.T) { require.NoError(t, err) require.Equal(t, int32(1), mc.keyBeginHits.Load(), "persistent-key login must register a key") + require.Equal(t, int32(1), mc.keyCompleteHits.Load(), "key registration must complete") cfg, err := clientconfig.LoadConfig() require.NoError(t, err) @@ -161,6 +174,8 @@ func TestLoginNoSavePersistentKeyRegisters(t *testing.T) { require.Equal(t, int32(1), mc.keyBeginHits.Load(), "a printed key must be registered with the cloud or it cannot authenticate") + require.Equal(t, int32(1), mc.keyCompleteHits.Load(), + "registration must actually complete, not just begin") // --no-save must still persist nothing. _, err = os.Stat(filepath.Join(dir, "clientconfig.d")) @@ -182,6 +197,7 @@ func TestLoginNoSaveEphemeralSkipsKeyRegistration(t *testing.T) { require.NoError(t, err) require.Zero(t, mc.keyBeginHits.Load(), "ephemeral --no-save must not register a key") + require.Zero(t, mc.keyCompleteHits.Load(), "ephemeral --no-save must not complete registration") _, err = os.Stat(filepath.Join(dir, "clientconfig.d")) require.True(t, os.IsNotExist(err), "--no-save must not write any config") } @@ -202,6 +218,7 @@ func TestLoginFallsBackWhenNoRefreshToken(t *testing.T) { require.NoError(t, err) require.Equal(t, int32(1), mc.keyBeginHits.Load(), "should fall back to key registration") + require.Equal(t, int32(1), mc.keyCompleteHits.Load(), "fallback key registration must complete") cfg, err := clientconfig.LoadConfig() require.NoError(t, err) From 1cb04431b93b64fec6f2ef7aea1161b793c05a17 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Fri, 24 Jul 2026 10:11:55 -0700 Subject: [PATCH 5/5] address review: type IdentityType, rename user-facing "ephemeral" phinze noted the identity Type was a bare string, so the exhaustive linter could not watch the switches on it; adding "token" meant hand-auditing every arm for the missing-case bug a named type would catch for free. Give it a type and consts. The switches now fail lint if a future identity type forgets an arm. Also rename the one place "ephemeral" leaked to users, the --persistent-key flag help, to "renewable": "ephemeral" collides with the unrelated ephemeral-deploy / ephemeral-version concept. Internal naming is left alone. Regenerated login.md to match. --- cli/commands/cluster_switch.go | 2 +- cli/commands/config_bind_helpers.go | 2 +- cli/commands/debug_connection.go | 12 ++++---- cli/commands/doctor.go | 6 ++-- cli/commands/login.go | 12 ++++---- cli/commands/login_ephemeral_test.go | 6 ++-- cli/commands/login_keys_test.go | 2 +- cli/commands/logout.go | 2 +- cli/commands/whoami.go | 6 ++-- clientconfig/clientconfig.go | 30 ++++++++++++++----- .../clientconfig_modification_test.go | 2 +- clientconfig/clientconfig_save_test.go | 4 +-- clientconfig/local.go | 4 +-- clientconfig/token.go | 6 ++-- clientconfig/token_identity_test.go | 2 +- docs/docs/command/login.md | 2 +- 16 files changed, 57 insertions(+), 43 deletions(-) diff --git a/cli/commands/cluster_switch.go b/cli/commands/cluster_switch.go index c20d47932..bf71feead 100644 --- a/cli/commands/cluster_switch.go +++ b/cli/commands/cluster_switch.go @@ -175,7 +175,7 @@ func checkClusterAccess(ctx *Context, cfg *clientconfig.Config, clusterName stri // checkClusterAccessRBAC calls the cloud API to check if the user has RBAC permission to access a cluster func checkClusterAccessRBAC(ctx *Context, config *clientconfig.Config, identityName string, identity *clientconfig.IdentityConfig, clusterXID string) (bool, string, error) { - if identity.Type != "keypair" && identity.Type != "token" { + if identity.Type != clientconfig.IdentityKeypair && identity.Type != clientconfig.IdentityToken { return false, "", fmt.Errorf("RBAC check is only supported for keypair and token identities") } diff --git a/cli/commands/config_bind_helpers.go b/cli/commands/config_bind_helpers.go index 3ace615f0..0f40ceb7a 100644 --- a/cli/commands/config_bind_helpers.go +++ b/cli/commands/config_bind_helpers.go @@ -198,7 +198,7 @@ func isPrivateAddress(host string) bool { // identityName may be "" for an anonymous in-memory identity (e.g. during login // before it has been named), in which case token refreshes are not persisted. func fetchAvailableClusters(ctx *Context, config *clientconfig.Config, identityName string, identity *clientconfig.IdentityConfig) ([]ClusterResponse, error) { - if identity.Type != "keypair" && identity.Type != "token" { + if identity.Type != clientconfig.IdentityKeypair && identity.Type != clientconfig.IdentityToken { return nil, fmt.Errorf("cluster listing is only supported for keypair and token identities") } diff --git a/cli/commands/debug_connection.go b/cli/commands/debug_connection.go index 66f2ea6e4..cfc84eac1 100644 --- a/cli/commands/debug_connection.go +++ b/cli/commands/debug_connection.go @@ -114,14 +114,14 @@ func DebugConnection(ctx *Context, opts struct { } else if cluster.CloudAuth && cluster.ClientKey != "" { // Backward compatibility: create identity from cluster identity = &clientconfig.IdentityConfig{ - Type: "keypair", + Type: clientconfig.IdentityKeypair, PrivateKey: cluster.ClientKey, } ctx.Info("Using legacy CloudAuth from cluster '%s'", opts.Cluster) } else if cluster.ClientCert != "" && cluster.ClientKey != "" { // Backward compatibility: certificate auth identity = &clientconfig.IdentityConfig{ - Type: "certificate", + Type: clientconfig.IdentityCertificate, ClientCert: cluster.ClientCert, ClientKey: cluster.ClientKey, } @@ -144,7 +144,7 @@ func DebugConnection(ctx *Context, opts struct { if identity != nil { switch identity.Type { - case "keypair", "token": + case clientconfig.IdentityKeypair, clientconfig.IdentityToken: // Both keypair and ephemeral token identities resolve to a JWT bearer // token. The server we're testing is the cluster, not the auth server. if config == nil { @@ -172,7 +172,7 @@ func DebugConnection(ctx *Context, opts struct { } authHeader = "Bearer " + token - authMethod = identity.Type + "/jwt" + authMethod = string(identity.Type) + "/jwt" ctx.Completed("JWT token obtained") // Decode and print JWT claims for debugging @@ -186,7 +186,7 @@ func DebugConnection(ctx *Context, opts struct { } } - case "certificate": + case clientconfig.IdentityCertificate: // Certificate auth is handled at TLS level authMethod = "certificate" ctx.Info("Using client certificate authentication") @@ -216,7 +216,7 @@ func DebugConnection(ctx *Context, opts struct { InsecureSkipVerify: opts.Insecure, } - if identity != nil && identity.Type == "certificate" { + if identity != nil && identity.Type == clientconfig.IdentityCertificate { // Add client certificate cert, err := tls.X509KeyPair([]byte(identity.ClientCert), []byte(identity.ClientKey)) if err != nil { diff --git a/cli/commands/doctor.go b/cli/commands/doctor.go index 6cef34fb7..cc2a206d6 100644 --- a/cli/commands/doctor.go +++ b/cli/commands/doctor.go @@ -97,7 +97,7 @@ func tryAuthenticate(ctx *Context, cfg *clientconfig.Config, cluster *clientconf result.IdentityName = cluster.Identity switch identity.Type { - case "keypair", "token": + case clientconfig.IdentityKeypair, clientconfig.IdentityToken: authServer := identity.Issuer if authServer == "" { authServer = cluster.Hostname @@ -110,12 +110,12 @@ func tryAuthenticate(ctx *Context, cfg *clientconfig.Config, cluster *clientconf } result.Claims, _ = auth.ParseUnverifiedClaims(token) - result.Method = identity.Type + result.Method = string(identity.Type) // Fetch user info from cloud result.UserInfo, _ = fetchCloudUserInfo(ctx, authServer, token) - case "certificate": + case clientconfig.IdentityCertificate: result.Method = "certificate" } diff --git a/cli/commands/login.go b/cli/commands/login.go index 66349b786..e46b3ff2d 100644 --- a/cli/commands/login.go +++ b/cli/commands/login.go @@ -132,7 +132,7 @@ func Login(ctx *Context, opts struct { KeyName string `short:"k" long:"key-name" description:"Name for the authentication key" default:"miren-cli"` NoSave bool `long:"no-save" description:"Don't save credentials to config file"` Force bool `short:"f" long:"force" description:"Overwrite existing identity without prompting"` - PersistentKey bool `long:"persistent-key" description:"Register a persistent key instead of the default ephemeral token login"` + PersistentKey bool `long:"persistent-key" description:"Register a persistent key instead of the default renewable token login"` }) error { return login(ctx, opts.CloudURL, opts.IdentityName, opts.KeyName, opts.NoSave, opts.Force, opts.PersistentKey) } @@ -201,7 +201,7 @@ func login(ctx *Context, cloudURL, identityName, keyName string, noSave, force, usePersistentKey := persistentKey if !usePersistentKey { if existing, err := clientconfig.LoadConfig(); err == nil && existing != nil { - if id, err := existing.GetIdentity(identityName); err == nil && id != nil && id.Type == "keypair" { + if id, err := existing.GetIdentity(identityName); err == nil && id != nil && id.Type == clientconfig.IdentityKeypair { usePersistentKey = true ctx.Info("Identity '%s' already uses a persistent key; keeping it.", identityName) } @@ -308,7 +308,7 @@ func persistKeypairIdentity(ctx *Context, identityName, cloudURL, keyName, acces return fmt.Errorf("failed to encode private key: %w", err) } identity := &clientconfig.IdentityConfig{ - Type: "keypair", + Type: clientconfig.IdentityKeypair, Issuer: clientconfig.NormalizeIssuerURL(cloudURL), PrivateKey: privateKeyPEM, } @@ -322,7 +322,7 @@ func persistKeypairIdentity(ctx *Context, identityName, cloudURL, keyName, acces func persistTokenIdentity(ctx *Context, identityName, cloudURL string, tokens *deviceFlowTokens) error { issuer := clientconfig.NormalizeIssuerURL(cloudURL) identity := &clientconfig.IdentityConfig{ - Type: "token", + Type: clientconfig.IdentityToken, Issuer: issuer, Token: tokens.AccessToken, RefreshToken: tokens.RefreshToken, @@ -714,7 +714,7 @@ func saveKeyPairToConfig(identityName, cloudURL string, keyPair *cloudauth.KeyPa leafConfigData := &clientconfig.ConfigData{ Identities: map[string]*clientconfig.IdentityConfig{ identityName: { - Type: "keypair", + Type: clientconfig.IdentityKeypair, Issuer: issuer, KeyRef: keyName, }, @@ -889,7 +889,7 @@ func getIdentityUserInfo(ctx *Context, config *clientconfig.Config, identityName return "" } - if identity.Type != "keypair" && identity.Type != "token" { + if identity.Type != clientconfig.IdentityKeypair && identity.Type != clientconfig.IdentityToken { return "" } diff --git a/cli/commands/login_ephemeral_test.go b/cli/commands/login_ephemeral_test.go index 6efa17b45..0b3305c78 100644 --- a/cli/commands/login_ephemeral_test.go +++ b/cli/commands/login_ephemeral_test.go @@ -119,7 +119,7 @@ func TestLoginEphemeralDefault(t *testing.T) { require.NoError(t, err) id, err := cfg.GetIdentity("cloud") require.NoError(t, err) - require.Equal(t, "token", id.Type) + require.Equal(t, clientconfig.IdentityToken, id.Type) require.NotEmpty(t, id.Token) require.NotEmpty(t, id.RefreshToken) @@ -149,7 +149,7 @@ func TestLoginPersistentKeyFlag(t *testing.T) { require.NoError(t, err) id, err := cfg.GetIdentity("cloud") require.NoError(t, err) - require.Equal(t, "keypair", id.Type) + require.Equal(t, clientconfig.IdentityKeypair, id.Type) require.Empty(t, id.Token) _, err = os.Stat(filepath.Join(dir, "clientconfig.d", "key-miren-cli.yaml")) @@ -224,5 +224,5 @@ func TestLoginFallsBackWhenNoRefreshToken(t *testing.T) { require.NoError(t, err) id, err := cfg.GetIdentity("cloud") require.NoError(t, err) - require.Equal(t, "keypair", id.Type) + require.Equal(t, clientconfig.IdentityKeypair, id.Type) } diff --git a/cli/commands/login_keys_test.go b/cli/commands/login_keys_test.go index 7807fc5f9..d6213bf6d 100644 --- a/cli/commands/login_keys_test.go +++ b/cli/commands/login_keys_test.go @@ -143,7 +143,7 @@ func TestSaveKeyPairToConfig(t *testing.T) { assert.True(t, config.HasIdentity(identityName), "Identity should be saved") identity, err := config.GetIdentity(identityName) require.NoError(t, err) - assert.Equal(t, "keypair", identity.Type) + assert.Equal(t, clientconfig.IdentityKeypair, identity.Type) assert.Equal(t, "https://test.miren.cloud", identity.Issuer) assert.Equal(t, keyName, identity.KeyRef, "Identity should reference the key") assert.Empty(t, identity.PrivateKey, "Identity should not have direct PrivateKey") diff --git a/cli/commands/logout.go b/cli/commands/logout.go index f09e8dc19..577e1bddc 100644 --- a/cli/commands/logout.go +++ b/cli/commands/logout.go @@ -115,7 +115,7 @@ func Logout(ctx *Context, opts struct { // For ephemeral token identities, best-effort revoke the refresh token so a // leaked ~/.config copy can't keep renewing after logout. Never let a failed // revoke block the local logout. - if identity.Type == "token" && identity.RefreshToken != "" { + if identity.Type == clientconfig.IdentityToken && identity.RefreshToken != "" { revokeIdentitySession(ctx, cfg, identityName, identity) } diff --git a/cli/commands/whoami.go b/cli/commands/whoami.go index 8d571395c..86490a67a 100644 --- a/cli/commands/whoami.go +++ b/cli/commands/whoami.go @@ -33,13 +33,13 @@ func Whoami(ctx *Context, opts struct { identity, err = ctx.ClientConfig.GetIdentity(ctx.ClusterConfig.Identity) if err == nil && identity != nil { switch identity.Type { - case "keypair", "token": + case clientconfig.IdentityKeypair, clientconfig.IdentityToken: token, err = ctx.ClientConfig.TokenForIdentity(ctx, ctx.ClusterConfig.Identity, identity, hostname) if err != nil { return fmt.Errorf("failed to authenticate: %w", err) } - authMethod = identity.Type - case "certificate": + authMethod = string(identity.Type) + case clientconfig.IdentityCertificate: authMethod = "certificate" } } diff --git a/clientconfig/clientconfig.go b/clientconfig/clientconfig.go index 669963e72..074a15f23 100644 --- a/clientconfig/clientconfig.go +++ b/clientconfig/clientconfig.go @@ -20,16 +20,30 @@ const ( EnvConfigPath = "MIREN_CONFIG" ) +// IdentityType names how an identity authenticates. Naming the set (rather than +// using a bare string) lets the exhaustive linter flag any switch that forgets +// an arm when a new type is added. +type IdentityType string + +const ( + // IdentityKeypair authenticates with an ed25519 key via challenge-response. + IdentityKeypair IdentityType = "keypair" + // IdentityToken authenticates with a stored JWT that is refreshed as needed. + IdentityToken IdentityType = "token" + // IdentityCertificate authenticates with a client TLS certificate. + IdentityCertificate IdentityType = "certificate" +) + // IdentityConfig holds authentication credentials that can be used across clusters type IdentityConfig struct { - Type string `yaml:"type"` // Type of identity: "keypair", "token", "certificate", etc. - Issuer string `yaml:"issuer,omitempty"` // The auth server that issued this identity (e.g., "https://miren.cloud") - KeyRef string `yaml:"key_ref,omitempty"` // Reference to a key in the Keys section (for keypair auth) - PrivateKey string `yaml:"private_key,omitempty"` // PEM encoded private key (for keypair auth, deprecated - use KeyRef) - ClientCert string `yaml:"client_cert,omitempty"` // PEM encoded client certificate (for cert auth) - ClientKey string `yaml:"client_key,omitempty"` // PEM encoded client key (for cert auth) - Token string `yaml:"token,omitempty"` // JWT access token (for ephemeral "token" auth) - RefreshToken string `yaml:"refresh_token,omitempty"` // Refresh token used to renew the access token (for "token" auth) + Type IdentityType `yaml:"type"` // How this identity authenticates + Issuer string `yaml:"issuer,omitempty"` // The auth server that issued this identity (e.g., "https://miren.cloud") + KeyRef string `yaml:"key_ref,omitempty"` // Reference to a key in the Keys section (for keypair auth) + PrivateKey string `yaml:"private_key,omitempty"` // PEM encoded private key (for keypair auth, deprecated - use KeyRef) + ClientCert string `yaml:"client_cert,omitempty"` // PEM encoded client certificate (for cert auth) + ClientKey string `yaml:"client_key,omitempty"` // PEM encoded client key (for cert auth) + Token string `yaml:"token,omitempty"` // JWT access token (for token auth) + RefreshToken string `yaml:"refresh_token,omitempty"` // Refresh token used to renew the access token (for token auth) } // KeyConfig holds a reusable cryptographic key diff --git a/clientconfig/clientconfig_modification_test.go b/clientconfig/clientconfig_modification_test.go index 3ef4bda87..c9c47ae8d 100644 --- a/clientconfig/clientconfig_modification_test.go +++ b/clientconfig/clientconfig_modification_test.go @@ -107,7 +107,7 @@ clusters: require.NoError(t, err) assert.Equal(t, "new-issuer", prodIdentitySaved.Issuer, "Should save modified issuer") assert.Equal(t, "new-key", prodIdentitySaved.PrivateKey, "Should save modified key") - assert.Equal(t, "keypair", prodIdentitySaved.Type, "Should preserve unmodified type") + assert.Equal(t, IdentityKeypair, prodIdentitySaved.Type, "Should preserve unmodified type") // Verify dev cluster (from config.d) was NOT saved _, err = savedConfig.GetCluster("dev") diff --git a/clientconfig/clientconfig_save_test.go b/clientconfig/clientconfig_save_test.go index 9067a4778..a78a94eab 100644 --- a/clientconfig/clientconfig_save_test.go +++ b/clientconfig/clientconfig_save_test.go @@ -287,7 +287,7 @@ func TestSetLeafConfigAndSaving(t *testing.T) { devIdentity, err := config.GetIdentity("dev-identity") require.NoError(t, err) - assert.Equal(t, "keypair", devIdentity.Type) + assert.Equal(t, IdentityKeypair, devIdentity.Type) // Verify counts include both main and leaf configs assert.Equal(t, 2, config.GetClusterCount()) // main-cluster + dev-cluster @@ -336,7 +336,7 @@ func TestSetLeafConfigAndSaving(t *testing.T) { assert.Equal(t, 1, len(leafConfigFromFile.Identities)) devIdentityFromFile, exists := leafConfigFromFile.Identities["dev-identity"] require.True(t, exists) - assert.Equal(t, "keypair", devIdentityFromFile.Type) + assert.Equal(t, IdentityKeypair, devIdentityFromFile.Type) } func TestSetLeafConfigUpdatesExisting(t *testing.T) { diff --git a/clientconfig/local.go b/clientconfig/local.go index 173fe6cf2..610668b48 100644 --- a/clientconfig/local.go +++ b/clientconfig/local.go @@ -109,7 +109,7 @@ foundAddress: // Handle different identity types switch identity.Type { - case "keypair", "token": + case IdentityKeypair, IdentityToken: // Both keypair and ephemeral token identities resolve to a bearer // token; TokenForIdentity dispatches on the identity type (re-minting // via challenge-response for keypair, or refreshing the cached token @@ -132,7 +132,7 @@ foundAddress: return base, nil - case "certificate": + case IdentityCertificate: // Handle certificate-based authentication from identity return []rpc.StateOption{ rpc.WithCertPEMs( diff --git a/clientconfig/token.go b/clientconfig/token.go index 2e2ea84dc..1bd21126c 100644 --- a/clientconfig/token.go +++ b/clientconfig/token.go @@ -164,7 +164,7 @@ func tokenFresh(tokenString string, buffer time.Duration) bool { // fallbackHost is used as the issuer when the identity carries no Issuer. func (c *Config) TokenForIdentity(ctx context.Context, name string, identity *IdentityConfig, fallbackHost string) (string, error) { switch identity.Type { - case "keypair": + case IdentityKeypair: privateKeyPEM, err := c.GetPrivateKeyPEM(identity) if err != nil { return "", fmt.Errorf("failed to get private key: %w", err) @@ -179,10 +179,10 @@ func (c *Config) TokenForIdentity(ctx context.Context, name string, identity *Id } return AuthenticateWithKey(ctx, authServer, keyPair) - case "token": + case IdentityToken: return c.tokenForTokenIdentity(ctx, name, identity, fallbackHost) - case "certificate": + case IdentityCertificate: return "", ErrNoBearerToken default: diff --git a/clientconfig/token_identity_test.go b/clientconfig/token_identity_test.go index 72fb02d98..c56eee32b 100644 --- a/clientconfig/token_identity_test.go +++ b/clientconfig/token_identity_test.go @@ -37,7 +37,7 @@ func TestTokenIdentityRoundTrip(t *testing.T) { id, err := reloaded.GetIdentity("cloud") require.NoError(t, err) - require.Equal(t, "token", id.Type) + require.Equal(t, IdentityToken, id.Type) require.Equal(t, "https://miren.cloud", id.Issuer) require.Equal(t, "access.jwt.value", id.Token) require.Equal(t, "refresh.jwt.value", id.RefreshToken) diff --git a/docs/docs/command/login.md b/docs/docs/command/login.md index 9d96acbee..27729a71b 100644 --- a/docs/docs/command/login.md +++ b/docs/docs/command/login.md @@ -20,7 +20,7 @@ miren login [flags] - `--identity, -i` — Name for this identity in config (default: `cloud`) - `--key-name, -k` — Name for the authentication key (default: `miren-cli`) - `--no-save` — Don't save credentials to config file -- `--persistent-key` — Register a persistent key instead of the default ephemeral token login +- `--persistent-key` — Register a persistent key instead of the default renewable token login - `--url, -u` — Cloud URL (default: `https://miren.cloud`) ## Global Options